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).
+::
- 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?
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?
+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 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.
+This ioclt only needs to be called once on running VMs on the same architecture.
Same question about the immutability of these names.
+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.
- __u64 data[0];
So userspace always sees a 64bit quantify per stat. My earlier comment about the ulong/u64 discrepancy stands! ;-)
- };
+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?
- 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.
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.