Hi Marc,
On Wed, Mar 10, 2021 at 8:58 AM Marc Zyngier maz@kernel.org wrote:
On Wed, 10 Mar 2021 00:30:22 +0000, Jing Zhang jingzhangos@google.com wrote:
Define ioctl commands for VM/vCPU aggregated statistics data retrieval in binary format and update corresponding API documentation.
The capability and ioctl are not enabled for now. No functional change intended.
Signed-off-by: Jing Zhang jingzhangos@google.com
Documentation/virt/kvm/api.rst | 79 ++++++++++++++++++++++++++++++++++ include/linux/kvm_host.h | 1 - include/uapi/linux/kvm.h | 60 ++++++++++++++++++++++++++ 3 files changed, 139 insertions(+), 1 deletion(-)
diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst index 1a2b5210cdbf..aa4b5270c966 100644 --- a/Documentation/virt/kvm/api.rst +++ b/Documentation/virt/kvm/api.rst @@ -4938,6 +4938,76 @@ see KVM_XEN_VCPU_SET_ATTR above. The KVM_XEN_VCPU_ATTR_TYPE_RUNSTATE_ADJUST type may not be used with the KVM_XEN_VCPU_GET_ATTR ioctl.
+4.131 KVM_STATS_GET_INFO +------------------------
+:Capability: KVM_CAP_STATS_BINARY_FORM +:Architectures: all +:Type: vm ioctl, vcpu ioctl +:Parameters: struct kvm_stats_info (out) +:Returns: 0 on success, < 0 on error
Missing description of the errors (this is throughout the document).
Will add errors description.
+::
- struct kvm_stats_info {
__u32 num_stats;- };
+This ioctl is used to get the information about VM or vCPU statistics data. +The number of statistics data would be returned in field num_stats in +struct kvm_stats_info. This ioctl only needs to be called once on running +VMs on the same architecture.
Is this allowed to be variable across VMs? Or is that a constant for a given host system boot?
It is a constant for a given host system boot.
Given that this returns a single field, is there any value in copying this structure across? Could it be returned by the ioctl itself instead, at the expense of only being a 31bit value?
It is done in this way for potential information we need in the future. One candidate is the length of stats names. I am considering to return the length of name string in this info structure instead of as a constant exported by uapi, which would be more flexible and put no limit on the length of stats names.
+4.132 KVM_STATS_GET_NAMES +-------------------------
+:Capability: KVM_CAP_STATS_BINARY_FORM +:Architectures: all +:Type: vm ioctl, vcpu ioctl +:Parameters: struct kvm_stats_names (in/out) +:Returns: 0 on success, < 0 on error
+::
- #define KVM_STATS_NAME_LEN 32
- struct kvm_stats_names {
__u32 size;__u8 names[0];- };
+This ioctl is used to get the string names of all the statistics data for VM +or vCPU. Users must use KVM_STATS_GET_INFO to find the number of statistics. +They must allocate a buffer of the size num_stats * KVM_STATS_NAME_LEN +immediately following struct kvm_stats_names. The size field of kvm_stats_name +must contain the buffer size as an input.
What is the unit for the buffer size? bytes? or number of "names"?
The unit for the buffer size is bytes. Will clearly indicate the unit.
+The buffer can be treated like a string array, each name string is null-padded +to a size of KVM_STATS_NAME_LEN;
Is this allowed to query fewer strings than described by kvm_stats_info? If it isn't, I question the need for the "size" field. Either there is enough space in the buffer passed by userspace, or -EFAULT is returned.
It isn't. Will remove the size field.
+This ioclt only needs to be called once on running VMs on the same architecture.
Same question about the immutability of these names.
The names are immutable for a given system boot.
+4.133 KVM_STATS_GET_DATA +-------------------------
+:Capability: KVM_CAP_STATS_BINARY_FORM +:Architectures: all +:Type: vm ioctl, vcpu ioctl +:Parameters: struct kvm_stats_data (in/out) +:Returns: 0 on success, < 0 on error
+::
- struct kvm_stats_data {
__u32 size;Same question about the actual need for this field.
Will remove size field.
__u64 data[0];So userspace always sees a 64bit quantify per stat. My earlier comment about the ulong/u64 discrepancy stands! ;-)
Yes, userspace always sees a 64 bit, but the ulong in KVM would be 64bit or 32bit.
- };
+This ioctl is used to get the aggregated statistics data for VM or vCPU. +Users must use KVM_STATS_GET_INFO to find the number of statistics. +They must allocate a buffer of the appropriate size num_stats * sizeof(data[0]) +immediately following struct kvm_stats_data. The size field of kvm_stats_data +must contain the buffer size as an input. +The data buffer 1-1 maps to name strings buffer in sequential order. +This ioctl is usually called periodically to pull statistics data.
Is there any provision to reset the counters on read?
Nope.
- The kvm_run structure
========================
@@ -6721,3 +6791,12 @@ vcpu_info is set. The KVM_XEN_HVM_CONFIG_RUNSTATE flag indicates that the runstate-related features KVM_XEN_VCPU_ATTR_TYPE_RUNSTATE_ADDR/_CURRENT/_DATA/_ADJUST are supported by the KVM_XEN_VCPU_SET_ATTR/KVM_XEN_VCPU_GET_ATTR ioctls.
+8.31 KVM_CAP_STATS_BINARY_FORM +------------------------------
+:Architectures: all
+This capability indicates that KVM supports retrieving aggregated statistics +data in binary format with corresponding VM/VCPU ioctl KVM_STATS_GET_INFO, +KVM_STATS_GET_NAMES and KVM_STATS_GET_DATA.
nit: for ease of reviewing, consider splitting the documentation in a separate patch.
Will do.
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 1ea297458306..f2dabf457717 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -1164,7 +1164,6 @@ static inline bool kvm_is_error_gpa(struct kvm *kvm, gpa_t gpa)
#define VM_STAT_COUNT (sizeof(struct kvm_vm_stat)/sizeof(ulong)) #define VCPU_STAT_COUNT (sizeof(struct kvm_vcpu_stat)/sizeof(u64)) -#define KVM_STATS_NAME_LEN 32
/* Make sure it is synced with fields in struct kvm_vm_stat. */ extern const char kvm_vm_stat_strings[][KVM_STATS_NAME_LEN]; diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index f6afee209620..ad185d4c5e42 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -1078,6 +1078,7 @@ struct kvm_ppc_resize_hpt { #define KVM_CAP_DIRTY_LOG_RING 192 #define KVM_CAP_X86_BUS_LOCK_EXIT 193 #define KVM_CAP_PPC_DAWR1 194 +#define KVM_CAP_STATS_BINARY_FORM 195
#ifdef KVM_CAP_IRQ_ROUTING
@@ -1853,4 +1854,63 @@ struct kvm_dirty_gfn { #define KVM_BUS_LOCK_DETECTION_OFF (1 << 0) #define KVM_BUS_LOCK_DETECTION_EXIT (1 << 1)
+/* Available with KVM_CAP_STATS_BINARY_FORM */
+#define KVM_STATS_NAME_LEN 32
+/**
- struct kvm_stats_info - statistics information
- Used as parameter for ioctl %KVM_STATS_GET_INFO.
- @num_stats: On return, the number of statistics data of vm or vcpu.
- */
+struct kvm_stats_info {
__u32 num_stats;+};
+/**
- struct kvm_stats_names - string list of statistics names
- Used as parameter for ioctl %KVM_STATS_GET_NAMES.
- @size: Input from user, indicating the size of buffer after the struture.
- @names: Buffer of name string list for vm or vcpu. Each string is
- null-padded to a size of %KVM_STATS_NAME_LEN.
- Users must use %KVM_STATS_GET_INFO to find the number of
- statistics. They must allocate a buffer of the appropriate
- size (>= &struct kvm_stats_info @num_stats * %KVM_STATS_NAME_LEN)
- immediately following this struture.
- */
+struct kvm_stats_names {
__u32 size;__u8 names[0];+};
+/**
- struct kvm_stats_data - statistics data array
- Used as parameter for ioctl %KVM_STATS_GET_DATA.
- @size: Input from user, indicating the size of buffer after the struture.
- @data: Buffer of statistics data for vm or vcpu.
- Users must use %KVM_STATS_GET_INFO to find the number of
- statistics. They must allocate a buffer of the appropriate
- size (>= &struct kvm_stats_info @num_stats * sizeof(@data[0])
- immediately following this structure.
- &struct kvm_stats_names @names 1-1 maps to &structkvm_stats_data
- @data in sequential order.
- */
+struct kvm_stats_data {
__u32 size;__u64 data[0];+};
+#define KVM_STATS_GET_INFO _IOR(KVMIO, 0xcc, struct kvm_stats_info) +#define KVM_STATS_GET_NAMES _IOR(KVMIO, 0xcd, struct kvm_stats_names) +#define KVM_STATS_GET_DATA _IOR(KVMIO, 0xce, struct kvm_stats_data)
#endif /* __LINUX_KVM_H */
2.30.1.766.gb4fecdf3b7-goog
Thanks,
M.-- Without deviation from the norm, progress is not possible.
Thanks, Jing