Re: [RFC PATCH 3/4] KVM: stats: Add ioctl commands to pull statistics in binary format

[Marc Zyngier <maz@kernel.org>]

On Wed, 10 Mar 2021 00:30:23 +0000,
Jing Zhang <jingzhangos@google.com> 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@google.com>
> ---
>  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.