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@xxxxxxxxxx>
> Reviewed-by: Paul Durrant <paul@xxxxxxx>
> ---
>
> 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);
