On Wed, 10 Mar 2021 00:30:22 +0000,
Jing Zhang <jingzhangos@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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?
> +
> 5. 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.
--
Without deviation from the norm, progress is not possible.
