On Wed, 10 Mar 2021 00:30:23 +0000,
Jing Zhang <jingzhangos@xxxxxxxxxx> wrote:
>
> Three ioctl commands are added to support binary form statistics data
> retrieval. KVM_STATS_GET_INFO, KVM_STATS_GET_NAMES, KVM_STATS_GET_DATA.
> KVM_CAP_STATS_BINARY_FORM indicates the capability.
>
> Signed-off-by: Jing Zhang <jingzhangos@xxxxxxxxxx>
> ---
> virt/kvm/kvm_main.c | 115 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 115 insertions(+)
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 383df23514b9..87dd62516c8b 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3464,6 +3464,51 @@ static long kvm_vcpu_ioctl(struct file *filp,
> r = kvm_arch_vcpu_ioctl_set_fpu(vcpu, fpu);
> break;
> }
> + case KVM_STATS_GET_INFO: {
> + struct kvm_stats_info stats_info;
> +
> + r = -EFAULT;
> + stats_info.num_stats = VCPU_STAT_COUNT;
> + if (copy_to_user(argp, &stats_info, sizeof(stats_info)))
> + goto out;
> + r = 0;
> + break;
> + }
> + case KVM_STATS_GET_NAMES: {
> + struct kvm_stats_names stats_names;
> +
> + r = -EFAULT;
> + if (copy_from_user(&stats_names, argp, sizeof(stats_names)))
> + goto out;
> + r = -EINVAL;
> + if (stats_names.size < VCPU_STAT_COUNT * KVM_STATS_NAME_LEN)
> + goto out;
> +
> + r = -EFAULT;
> + if (copy_to_user(argp + sizeof(stats_names),
> + kvm_vcpu_stat_strings,
> + VCPU_STAT_COUNT * KVM_STATS_NAME_LEN))
> + goto out;
> + r = 0;
> + break;
> + }
> + case KVM_STATS_GET_DATA: {
> + struct kvm_stats_data stats_data;
> +
> + r = -EFAULT;
> + if (copy_from_user(&stats_data, argp, sizeof(stats_data)))
> + goto out;
> + r = -EINVAL;
> + if (stats_data.size < sizeof(vcpu->stat))
> + goto out;
> +
> + r = -EFAULT;
> + argp += sizeof(stats_data);
> + if (copy_to_user(argp, &vcpu->stat, sizeof(vcpu->stat)))
> + goto out;
> + r = 0;
> + break;
> + }
> default:
> r = kvm_arch_vcpu_ioctl(filp, ioctl, arg);
> }
> @@ -3695,6 +3740,7 @@ static long kvm_vm_ioctl_check_extension_generic(struct kvm *kvm, long arg)
> case KVM_CAP_CHECK_EXTENSION_VM:
> case KVM_CAP_ENABLE_CAP_VM:
> case KVM_CAP_HALT_POLL:
> + case KVM_CAP_STATS_BINARY_FORM:
> return 1;
> #ifdef CONFIG_KVM_MMIO
> case KVM_CAP_COALESCED_MMIO:
> @@ -3825,6 +3871,40 @@ static int kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm,
> }
> }
>
> +static long kvm_vm_ioctl_stats_get_data(struct kvm *kvm, unsigned long arg)
> +{
> + void __user *argp = (void __user *)arg;
> + struct kvm_vcpu *vcpu;
> + struct kvm_stats_data stats_data;
> + u64 *data = NULL, *pdata;
> + int i, j, ret = 0;
> + size_t dsize = (VM_STAT_COUNT + VCPU_STAT_COUNT) * sizeof(*data);
> +
> +
> + if (copy_from_user(&stats_data, argp, sizeof(stats_data)))
> + return -EFAULT;
> + if (stats_data.size < dsize)
> + return -EINVAL;
> + data = kzalloc(dsize, GFP_KERNEL_ACCOUNT);
> + if (!data)
> + return -ENOMEM;
> +
> + for (i = 0; i < VM_STAT_COUNT; i++)
> + *(data + i) = *((ulong *)&kvm->stat + i);
This kind of dance could be avoided if your stats were just an array,
or a union of the current data structure and an array.
> +
> + kvm_for_each_vcpu(j, vcpu, kvm) {
> + pdata = data + VM_STAT_COUNT;
> + for (i = 0; i < VCPU_STAT_COUNT; i++, pdata++)
> + *pdata += *((u64 *)&vcpu->stat + i);
Do you really need the in-kernel copy? Why not directly organise the
data structures in a way that would allow a bulk copy using
copy_to_user()?
Another thing is the atomicity of what you are reporting. Don't you
care about the consistency of the counters?
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
