Re: [PATCH v2 4/6] KVM: Add KVM_(UN)REGISTER_COALESCED_MMIO2 ioctls

[Sean Christopherson <seanjc@google.com>]

On Thu, Jul 18, 2024, Ilias Stamatis wrote:
> Add 2 new ioctls, KVM_REGISTER_COALESCED_MMIO2 and
> KVM_UNREGISTER_COALESCED_MMIO2. These do the same thing as their v1
> equivalents except an fd returned by KVM_CREATE_COALESCED_MMIO_BUFFER
> needs to be passed as an argument to them.
> 
> The fd representing a ring buffer is associated with an MMIO region
> registered for coalescing and all writes to that region are accumulated
> there. This is in contrast to the v1 API where all regions have to share
> the same buffer. Nevertheless, userspace code can still use the same
> ring buffer for multiple zones if it wishes to do so.
> 
> Userspace can check for the availability of the new API by checking if
> the KVM_CAP_COALESCED_MMIO2 capability is supported.
> 
> Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
> Reviewed-by: Paul Durrant <paul@xen.org>
> ---
> 
> v1->v2:
>   - Rebased on top of kvm/queue resolving the conflict in kvm.h
> 
>  include/uapi/linux/kvm.h  | 16 ++++++++++++++++
>  virt/kvm/coalesced_mmio.c | 37 +++++++++++++++++++++++++++++++------
>  virt/kvm/coalesced_mmio.h |  7 ++++---
>  virt/kvm/kvm_main.c       | 36 +++++++++++++++++++++++++++++++++++-
>  4 files changed, 86 insertions(+), 10 deletions(-)
> 
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 87f79a820fc0..7e8d5c879986 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -480,6 +480,16 @@ struct kvm_coalesced_mmio_zone {
>  	};
>  };
>  
> +struct kvm_coalesced_mmio_zone2 {
> +	__u64 addr;
> +	__u32 size;
> +	union {
> +		__u32 pad;
> +		__u32 pio;
> +	};
> +	int buffer_fd;

Dunno if it matters, but KVM typically uses __u32 for file descriptors and then
does the verificaton on the backend (which needs to be there regardless).

> diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
> index 64428b0a4aad..729f3293f768 100644
> --- a/virt/kvm/coalesced_mmio.c
> +++ b/virt/kvm/coalesced_mmio.c
> @@ -17,6 +17,7 @@
>  #include <linux/kvm.h>
>  #include <linux/anon_inodes.h>
>  #include <linux/poll.h>
> +#include <linux/file.h>
>  
>  #include "coalesced_mmio.h"
>  
> @@ -279,19 +280,40 @@ int kvm_vm_ioctl_create_coalesced_mmio_buffer(struct kvm *kvm)
>  }
>  
>  int kvm_vm_ioctl_register_coalesced_mmio(struct kvm *kvm,
> -					 struct kvm_coalesced_mmio_zone *zone)
> +					 struct kvm_coalesced_mmio_zone2 *zone,
> +					 bool use_buffer_fd)
>  {
> -	int ret;
> +	int ret = 0;

Heh, init ret to zero, and then return a pile of error before ret is ever touched.
I assume ret can be set to '0' after mutex_unlock(&kvm->slots_lock); in the happy
path, which would also show that it is indeed the happy path.

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 9eb22287384f..ef7dcce80136 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -4892,6 +4892,7 @@ static int kvm_vm_ioctl_check_extension_generic(struct kvm *kvm, long arg)
>  #ifdef CONFIG_KVM_MMIO
>  	case KVM_CAP_COALESCED_MMIO:
>  		return KVM_COALESCED_MMIO_PAGE_OFFSET;
> +	case KVM_CAP_COALESCED_MMIO2:
>  	case KVM_CAP_COALESCED_PIO:
>  		return 1;
>  #endif
> @@ -5230,15 +5231,48 @@ static long kvm_vm_ioctl(struct file *filp,
>  #ifdef CONFIG_KVM_MMIO
>  	case KVM_REGISTER_COALESCED_MMIO: {
>  		struct kvm_coalesced_mmio_zone zone;
> +		struct kvm_coalesced_mmio_zone2 zone2;
>  
>  		r = -EFAULT;
>  		if (copy_from_user(&zone, argp, sizeof(zone)))
>  			goto out;
> -		r = kvm_vm_ioctl_register_coalesced_mmio(kvm, &zone);
> +
> +		zone2.addr = zone.addr;
> +		zone2.size = zone.size;
> +		zone2.pio = zone.pio;
> +		zone2.buffer_fd = -1;
> +
> +		r = kvm_vm_ioctl_register_coalesced_mmio(kvm, &zone2, false);
> +		break;
> +	}
> +	case KVM_REGISTER_COALESCED_MMIO2: {

Ah, the curly braces were just early :-)

> +		struct kvm_coalesced_mmio_zone2 zone;
> +
> +		r = -EFAULT;
> +		if (copy_from_user(&zone, argp, sizeof(zone)))
> +			goto out;
> +
> +		r = kvm_vm_ioctl_register_coalesced_mmio(kvm, &zone, true);