Re: [PATCH 09/29] KVM: implement plane file descriptors ioctl and creation

[Tom Lendacky <thomas.lendacky@amd.com>]

On 4/1/25 11:10, Paolo Bonzini wrote:
> Add the file_operations for planes, the means to create new file
> descriptors for them, and the KVM_CHECK_EXTENSION implementation for
> the two new capabilities.
> 
> KVM_SIGNAL_MSI and KVM_SET_MEMORY_ATTRIBUTES are now available
> through both vm and plane file descriptors, forward them to the
> same function that is used by the file_operations for planes.
> KVM_CHECK_EXTENSION instead remains separate, because it only
> advertises a very small subset of capabilities when applied to
> plane file descriptors.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/linux/kvm_host.h |  19 +++++
>  include/uapi/linux/kvm.h |   2 +
>  virt/kvm/kvm_main.c      | 154 +++++++++++++++++++++++++++++++++------
>  3 files changed, 154 insertions(+), 21 deletions(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 0a91b556767e..dbca418d64f5 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -342,6 +342,8 @@ struct kvm_vcpu {
>  	unsigned long guest_debug;
>  
>  	struct mutex mutex;
> +
> +	/* Shared for all planes */
>  	struct kvm_run *run;
>  
>  #ifndef __KVM_HAVE_ARCH_WQP
> @@ -922,6 +924,23 @@ static inline void kvm_vm_bugged(struct kvm *kvm)
>  }
>  
>  
> +#if KVM_MAX_VCPU_PLANES == 1
> +static inline int kvm_arch_nr_vcpu_planes(struct kvm *kvm)
> +{
> +	return KVM_MAX_VCPU_PLANES;
> +}

Should this be outside of the #if above?

> +
> +static inline struct kvm_plane *vcpu_to_plane(struct kvm_vcpu *vcpu)
> +{
> +	return vcpu->kvm->planes[0];
> +}
> +#else
> +static inline struct kvm_plane *vcpu_to_plane(struct kvm_vcpu *vcpu)
> +{
> +	return vcpu->kvm->planes[vcpu->plane_id];
> +}
> +#endif

Are two different functions needed? The vcpu->plane_id will be zero in
the KVM_MAX_VCPU_PLANES == 1 case, so that should get the same result as
the hard-coded 0, right?

> +


> @@ -5236,16 +5363,12 @@ static long kvm_vm_ioctl(struct file *filp,
>  		break;
>  	}
>  #ifdef CONFIG_HAVE_KVM_MSI
> -	case KVM_SIGNAL_MSI: {
> -		struct kvm_msi msi;
> -
> -		r = -EFAULT;
> -		if (copy_from_user(&msi, argp, sizeof(msi)))
> -			goto out;
> -		r = kvm_send_userspace_msi(kvm->planes[0], &msi);
> -		break;
> -	}
> +	case KVM_SIGNAL_MSI:
>  #endif
> +#ifdef CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES
> +	case KVM_SET_MEMORY_ATTRIBUTES:
> +#endif /* CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES */

If both of these aren't #defined, then you'll end up with just the
return statement from the next line, which will cause a build failure.

Thanks,
Tom

> +		return __kvm_plane_ioctl(kvm->planes[0], ioctl, arg);
>  #ifdef __KVM_HAVE_IRQ_LINE
>  	case KVM_IRQ_LINE_STATUS:
>  	case KVM_IRQ_LINE: {
> @@ -5301,18 +5424,6 @@ static long kvm_vm_ioctl(struct file *filp,
>  		break;
>  	}
>  #endif /* CONFIG_HAVE_KVM_IRQ_ROUTING */
> -#ifdef CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES
> -	case KVM_SET_MEMORY_ATTRIBUTES: {
> -		struct kvm_memory_attributes attrs;
> -
> -		r = -EFAULT;
> -		if (copy_from_user(&attrs, argp, sizeof(attrs)))
> -			goto out;
> -
> -		r = kvm_vm_ioctl_set_mem_attributes(kvm->planes[0], &attrs);
> -		break;
> -	}
> -#endif /* CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES */
>  	case KVM_CREATE_DEVICE: {
>  		struct kvm_create_device cd;
>  
> @@ -6467,6 +6578,7 @@ int kvm_init(unsigned vcpu_size, unsigned vcpu_align, struct module *module)
>  	kvm_chardev_ops.owner = module;
>  	kvm_vm_fops.owner = module;
>  	kvm_vcpu_fops.owner = module;
> +	kvm_plane_fops.owner = module;
>  	kvm_device_fops.owner = module;
>  
>  	kvm_preempt_ops.sched_in = kvm_sched_in;