| Message ID | 20230916003118.2540661-6-seanjc@google.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | KVM: vfio: Hide KVM internals from others | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Series has a cover letter |
| conchuod/tree_selection | success | Guessed tree name to be for-next at HEAD 0bb80ecc33a8 |
| conchuod/fixes_present | success | Fixes tag not required for -next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 5 and now 5 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 13 this patch: 13 |
| conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 9 this patch: 9 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | success | Errors and warnings before: 9 this patch: 9 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 25 this patch: 25 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 173 lines checked |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | No Fixes tag |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
On Fri, Sep 15, 2023 at 05:30:57PM -0700, Sean Christopherson 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@google.com> > --- > 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(-) I don't mind this, but Christoph had disliked my prior attempt to do this with function pointers.. The get can be inlined, IIRC, what about putting a pointer to the put inside the kvm struct? The the normal kvm get/put don't have to exported symbols at all? Jason
On Mon, Sep 18, 2023, Jason Gunthorpe wrote: > On Fri, Sep 15, 2023 at 05:30:57PM -0700, Sean Christopherson 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@google.com> > > --- > > 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(-) > > I don't mind this, but Christoph had disliked my prior attempt to do > this with function pointers.. > > The get can be inlined, IIRC, what about putting a pointer to the put > inside the kvm struct? That wouldn't allow us to achieve our goal, which is to hide the details of "struct kvm" from VFIO (and the rest of the kernel). What's the objection to handing VFIO a function pointer? > The the normal kvm get/put don't have to exported symbols at all? The export of kvm_get_kvm_safe() can go away (I forgot to do that in this series), but kvm_get_kvm() will hang around as it's needed by KVM sub-modules (PPC and x86), KVMGT (x86), and drivers/s390/crypto/vfio_ap_ops.c (no idea what to call that beast). Gah, KVMGT doesn't actually need to call get/put, that can be handled by kvm_page_track_register_notifier(). I am planning on making exports for sub-modules conditional on there actually being submodules, so that's 2 of the 3 gone, but tackling the s390 crypto driver is an entirely different story.
On Mon, Sep 18, 2023 at 08:49:57AM -0700, Sean Christopherson wrote: > On Mon, Sep 18, 2023, Jason Gunthorpe wrote: > > On Fri, Sep 15, 2023 at 05:30:57PM -0700, Sean Christopherson 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@google.com> > > > --- > > > 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(-) > > > > I don't mind this, but Christoph had disliked my prior attempt to do > > this with function pointers.. > > > > The get can be inlined, IIRC, what about putting a pointer to the put > > inside the kvm struct? > > That wouldn't allow us to achieve our goal, which is to hide the details of > "struct kvm" from VFIO (and the rest of the kernel). > What's the objection to handing VFIO a function pointer? Hmm, looks like it was this thread: https://lore.kernel.org/r/0-v1-33906a626da1+16b0-vfio_kvm_no_group_jgg@nvidia.com Your rational looks a little better to me. > > The the normal kvm get/put don't have to exported symbols at all? > > The export of kvm_get_kvm_safe() can go away (I forgot to do that in this series), > but kvm_get_kvm() will hang around as it's needed by KVM sub-modules (PPC and x86), > KVMGT (x86), and drivers/s390/crypto/vfio_ap_ops.c (no idea what to call that beast). My thought would be to keep it as an inline, there should be some way to do that without breaking your desire to hide the bulk of the kvm struct content. Like put the refcount as the first element in the struct and just don't ifdef it away?. Jason
On Fri, 15 Sep 2023 17:30:57 -0700 Sean Christopherson <seanjc@google.com> 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@google.com> > --- > 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@redhat.com> > 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); > }
On Mon, Sep 18, 2023, Jason Gunthorpe wrote: > On Mon, Sep 18, 2023 at 08:49:57AM -0700, Sean Christopherson wrote: > > On Mon, Sep 18, 2023, Jason Gunthorpe wrote: > > > On Fri, Sep 15, 2023 at 05:30:57PM -0700, Sean Christopherson 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@google.com> > > > > --- > > > > 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(-) > > > > > > I don't mind this, but Christoph had disliked my prior attempt to do > > > this with function pointers.. > > > > > > The get can be inlined, IIRC, what about putting a pointer to the put > > > inside the kvm struct? > > > > That wouldn't allow us to achieve our goal, which is to hide the details of > > "struct kvm" from VFIO (and the rest of the kernel). > > > What's the objection to handing VFIO a function pointer? > > Hmm, looks like it was this thread: > > https://lore.kernel.org/r/0-v1-33906a626da1+16b0-vfio_kvm_no_group_jgg@nvidia.com > > Your rational looks a little better to me. > > > > The the normal kvm get/put don't have to exported symbols at all? > > > > The export of kvm_get_kvm_safe() can go away (I forgot to do that in this series), > > but kvm_get_kvm() will hang around as it's needed by KVM sub-modules (PPC and x86), > > KVMGT (x86), and drivers/s390/crypto/vfio_ap_ops.c (no idea what to call that beast). > > My thought would be to keep it as an inline, there should be some way > to do that without breaking your desire to hide the bulk of the kvm > struct content. Like put the refcount as the first element in the > struct and just don't ifdef it away?. That doesn't work because of the need to invoke kvm_destroy_vm() when the last reference is put, i.e. all of kvm_destroy_vm() would need to be inlined (LOL) or VFIO would need to do a symbol lookup on kvm_destroy_vm(), which puts back us at square one. There's one more wrinkle: this patch is buggy in that it doesn't ensure the liveliness of KVM-the-module, i.e. nothing prevents userspace from unloading kvm.ko while VFIO still holds a reference to a kvm structure, and so invoking ->put_kvm() could jump into freed code. To fix that, KVM would also need to pass along a module pointer :-( One thought would be to have vac.ko (tentative name), which is the "base" module that will hold the KVM/virtualization bits that need to be singletons, i.e. can't be per-KVM, provide the symbols needed for VFIO to manage references. But that just ends up moving the module reference trickiness into VAC+KVM, e.g. vac.ko would still need to be handed a function pointer in order to call back into the correct kvm.ko code. Hrm, but I suspect the vac.ko <=> kvm.ko interactions will need to deal with module shenanigans anyways, and the shenanigans would only affect crazy people like us that actually want multiple KVM modules. I'll plan on going that route. The very worst case scenario is that it just punts this conversation down to a possibile future. Dropping this patch and the previous prep patch won't meaningful affect the goals of this series, as only kvm_get_kvm_safe() and kvm_get_kvm() would need to be exposed outside of #ifdef __KVM__. Then we can figure out what to do with them if/when the whole multi-KVM thing comes along.
On Fri, Dec 01, 2023 at 04:51:55PM -0800, Sean Christopherson wrote: > There's one more wrinkle: this patch is buggy in that it doesn't ensure the liveliness > of KVM-the-module, i.e. nothing prevents userspace from unloading kvm.ko while VFIO > still holds a reference to a kvm structure, and so invoking ->put_kvm() could jump > into freed code. To fix that, KVM would also need to pass along a module pointer :-( Maybe we should be refcounting the struct file not the struct kvm? Then we don't need special helpers and it keeps the module alive correctly. Jason
On Sun, Dec 03, 2023, Jason Gunthorpe wrote: > On Fri, Dec 01, 2023 at 04:51:55PM -0800, Sean Christopherson wrote: > > > There's one more wrinkle: this patch is buggy in that it doesn't ensure the liveliness > > of KVM-the-module, i.e. nothing prevents userspace from unloading kvm.ko while VFIO > > still holds a reference to a kvm structure, and so invoking ->put_kvm() could jump > > into freed code. To fix that, KVM would also need to pass along a module pointer :-( > > Maybe we should be refcounting the struct file not the struct kvm? > > Then we don't need special helpers and it keeps the module alive correctly. Huh. It took my brain a while to catch up, but this seems comically obvious in hindsight. I *love* this approach, both conceptually and from a code perspective. Handing VFIO (and any other external entities) a file makes it so that KVM effectively interacts with users via files, regardless of whether the user lives in userspace or the kernel. That makes it easier to reason about the safety of operations, e.g. in addition to ensuring KVM-the-module is pinned, having a file pointer allows KVM to verify that the incoming pointer does indeed represent a VM. Which isn't necessary by any means, but it's a nice sanity check. From a code perspective, it's far cleaner than manually grabbing module references, and having only a file pointers makes it a wee bit harder for non-KVM code to poke into KVM, e.g. keeps us honest. Assuming nothing blows up in testing, I'll go this route for v2. Thanks!
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); }
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@google.com> --- 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(-)