On Fri, 15 Sep 2023 17:30:57 -0700
Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> Explicitly pass KVM's get/put helpers to VFIO when attaching a VM to
> VFIO instead of having VFIO do a symbol lookup back into KVM. Having both
> KVM and VFIO do symbol lookups increases the overall complexity and places
> an unnecessary dependency on KVM (from VFIO) without adding any value.
>
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
> drivers/vfio/vfio.h | 2 ++
> drivers/vfio/vfio_main.c | 74 +++++++++++++++++++---------------------
> include/linux/vfio.h | 4 ++-
> virt/kvm/vfio.c | 9 +++--
> 4 files changed, 47 insertions(+), 42 deletions(-)
Reviewed-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> index a1f741365075..eec51c7ee822 100644
> --- a/drivers/vfio/vfio.h
> +++ b/drivers/vfio/vfio.h
> @@ -19,6 +19,8 @@ struct vfio_container;
>
> struct vfio_kvm_reference {
> struct kvm *kvm;
> + bool (*get_kvm)(struct kvm *kvm);
> + void (*put_kvm)(struct kvm *kvm);
> spinlock_t lock;
> };
>
> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index e77e8c6aae2f..1f58ab6dbcd2 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -16,7 +16,6 @@
> #include <linux/fs.h>
> #include <linux/idr.h>
> #include <linux/iommu.h>
> -#include <linux/kvm_host.h>
> #include <linux/list.h>
> #include <linux/miscdevice.h>
> #include <linux/module.h>
> @@ -1306,38 +1305,22 @@ EXPORT_SYMBOL_GPL(vfio_file_enforced_coherent);
> void vfio_device_get_kvm_safe(struct vfio_device *device,
> struct vfio_kvm_reference *ref)
> {
> - void (*pfn)(struct kvm *kvm);
> - bool (*fn)(struct kvm *kvm);
> - bool ret;
> -
> lockdep_assert_held(&device->dev_set->lock);
>
> + /*
> + * Note! The "kvm" and "put_kvm" pointers *must* be transferred to the
> + * device so that the device can put its reference to KVM. KVM can
> + * invoke vfio_device_set_kvm() to detach from VFIO, i.e. nullify all
> + * pointers in @ref, even if a device holds a reference to KVM! That
> + * also means that detaching KVM from VFIO only prevents "new" devices
> + * from using KVM, it doesn't invalidate KVM references in existing
> + * devices.
> + */
> spin_lock(&ref->lock);
> -
> - if (!ref->kvm)
> - goto out;
> -
> - pfn = symbol_get(kvm_put_kvm);
> - if (WARN_ON(!pfn))
> - goto out;
> -
> - fn = symbol_get(kvm_get_kvm_safe);
> - if (WARN_ON(!fn)) {
> - symbol_put(kvm_put_kvm);
> - goto out;
> + if (ref->kvm && ref->get_kvm(ref->kvm)) {
> + device->kvm = ref->kvm;
> + device->put_kvm = ref->put_kvm;
> }
> -
> - ret = fn(ref->kvm);
> - symbol_put(kvm_get_kvm_safe);
> - if (!ret) {
> - symbol_put(kvm_put_kvm);
> - goto out;
> - }
> -
> - device->put_kvm = pfn;
> - device->kvm = ref->kvm;
> -
> -out:
> spin_unlock(&ref->lock);
> }
>
> @@ -1353,28 +1336,37 @@ void vfio_device_put_kvm(struct vfio_device *device)
>
> device->put_kvm(device->kvm);
> device->put_kvm = NULL;
> - symbol_put(kvm_put_kvm);
> -
> clear:
> device->kvm = NULL;
> }
>
> static void vfio_device_set_kvm(struct vfio_kvm_reference *ref,
> - struct kvm *kvm)
> + struct kvm *kvm,
> + bool (*get_kvm)(struct kvm *kvm),
> + void (*put_kvm)(struct kvm *kvm))
> {
> + if (WARN_ON_ONCE(kvm && (!get_kvm || !put_kvm)))
> + return;
> +
> spin_lock(&ref->lock);
> ref->kvm = kvm;
> + ref->get_kvm = get_kvm;
> + ref->put_kvm = put_kvm;
> spin_unlock(&ref->lock);
> }
>
> -static void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm)
> +static void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm,
> + bool (*get_kvm)(struct kvm *kvm),
> + void (*put_kvm)(struct kvm *kvm))
> {
> #if IS_ENABLED(CONFIG_VFIO_GROUP)
> - vfio_device_set_kvm(&group->kvm_ref, kvm);
> + vfio_device_set_kvm(&group->kvm_ref, kvm, get_kvm, put_kvm);
> #endif
> }
>
> -static void vfio_device_file_set_kvm(struct file *file, struct kvm *kvm)
> +static void vfio_device_file_set_kvm(struct file *file, struct kvm *kvm,
> + bool (*get_kvm)(struct kvm *kvm),
> + void (*put_kvm)(struct kvm *kvm))
> {
> struct vfio_device_file *df = file->private_data;
>
> @@ -1383,27 +1375,31 @@ static void vfio_device_file_set_kvm(struct file *file, struct kvm *kvm)
> * be propagated to vfio_device::kvm when the file is bound to
> * iommufd successfully in the vfio device cdev path.
> */
> - vfio_device_set_kvm(&df->kvm_ref, kvm);
> + vfio_device_set_kvm(&df->kvm_ref, kvm, get_kvm, put_kvm);
> }
>
> /**
> * vfio_file_set_kvm - Link a kvm with VFIO drivers
> * @file: VFIO group file or VFIO device file
> * @kvm: KVM to link
> + * @get_kvm: Callback to get a reference to @kvm
> + * @put_kvm: Callback to put a reference to @kvm
> *
> * When a VFIO device is first opened the KVM will be available in
> * device->kvm if one was associated with the file.
> */
> -void vfio_file_set_kvm(struct file *file, struct kvm *kvm)
> +void vfio_file_set_kvm(struct file *file, struct kvm *kvm,
> + bool (*get_kvm)(struct kvm *kvm),
> + void (*put_kvm)(struct kvm *kvm))
> {
> struct vfio_group *group;
>
> group = vfio_group_from_file(file);
> if (group)
> - vfio_group_set_kvm(group, kvm);
> + vfio_group_set_kvm(group, kvm, get_kvm, put_kvm);
>
> if (vfio_device_from_file(file))
> - vfio_device_file_set_kvm(file, kvm);
> + vfio_device_file_set_kvm(file, kvm, get_kvm, put_kvm);
> }
> EXPORT_SYMBOL_GPL(vfio_file_set_kvm);
> #endif
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index e80955de266c..35e970e3d3fb 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -312,7 +312,9 @@ static inline bool vfio_file_has_dev(struct file *file, struct vfio_device *devi
> bool vfio_file_is_valid(struct file *file);
> bool vfio_file_enforced_coherent(struct file *file);
> #if IS_ENABLED(CONFIG_KVM)
> -void vfio_file_set_kvm(struct file *file, struct kvm *kvm);
> +void vfio_file_set_kvm(struct file *file, struct kvm *kvm,
> + bool (*get_kvm)(struct kvm *kvm),
> + void (*put_kvm)(struct kvm *kvm));
> #endif
>
> #define VFIO_PIN_PAGES_MAX_ENTRIES (PAGE_SIZE/sizeof(unsigned long))
> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> index ca24ce120906..f14fcbb34bc6 100644
> --- a/virt/kvm/vfio.c
> +++ b/virt/kvm/vfio.c
> @@ -37,13 +37,18 @@ struct kvm_vfio {
>
> static void kvm_vfio_file_set_kvm(struct file *file, struct kvm *kvm)
> {
> - void (*fn)(struct file *file, struct kvm *kvm);
> + void (*fn)(struct file *file, struct kvm *kvm,
> + bool (*get_kvm)(struct kvm *kvm),
> + void (*put_kvm)(struct kvm *kvm));
>
> fn = symbol_get(vfio_file_set_kvm);
> if (!fn)
> return;
>
> - fn(file, kvm);
> + if (kvm)
> + fn(file, kvm, kvm_get_kvm_safe, kvm_put_kvm);
> + else
> + fn(file, kvm, NULL, NULL);
>
> symbol_put(vfio_file_set_kvm);
> }
