On 2/3/23 3:35 PM, Alex Williamson wrote: > On Fri, 3 Feb 2023 12:29:01 -0500 > Matthew Rosato <mjrosato@xxxxxxxxxxxxx> wrote: ... > I'd probably go back to making this: > > void _vfio_device_get_kvm_safe(struct vfio_device *device, struct kvm *kvm); > > so the vfio_main function would handle setting and clearing > device->kvm. That way we could also move the lockdep into the > vfio_main functions. Once we do that, there's no reason to have a > group vs cdev put function and we end up with only a wrapper on the get > function, which should really never be used directly, so we prefix it > with an underscore. > > At that point (see incremental diff below), it's about a wash. Current v3: > > drivers/vfio/group.c | 32 +++++++++++++---- > drivers/vfio/vfio.h | 14 +++++++ > drivers/vfio/vfio_main.c | 70 +++++++++++++++++++++++++++++++++++---- > include/linux/vfio.h | 2 - > 4 files changed, 103 insertions(+), 15 deletions(-) > > Folding in below: > > drivers/vfio/group.c | 44 ++++++++++++++++++++++----- > drivers/vfio/vfio.h | 15 +++++++++ > drivers/vfio/vfio_main.c | 63 ++++++++++++++++++++++++++++++++++----- > include/linux/vfio.h | 2 - > 4 files changed, 109 insertions(+), 15 deletions(-) > > Unfortunately it seems I've talked myself into the answer that we > should maybe just pre-enable cdev by not adding a group reference in > vfio_main. Thanks, > > Alex > > diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c > index 7fed4233ca23..98621ac082f0 100644 > --- a/drivers/vfio/group.c > +++ b/drivers/vfio/group.c > @@ -154,6 +154,18 @@ static int vfio_group_ioctl_set_container(struct vfio_group *group, > return ret; > } > > +static void vfio_device_group_get_kvm_safe(struct vfio_device *device) > +{ > + spin_lock(&device->group->kvm_ref_lock); > + if (!device->group->kvm) > + goto unlock; > + > + _vfio_device_get_kvm_safe(device, device->group->kvm); > + > +unlock: > + spin_unlock(&device->group->kvm_ref_lock); > +} > + > static int vfio_device_group_open(struct vfio_device *device) > { > int ret; > @@ -173,7 +185,7 @@ static int vfio_device_group_open(struct vfio_device *device) > * the pointer in the device for use by drivers. > */ > if (device->open_count == 0) > - vfio_device_get_kvm_safe(device); > + vfio_device_group_get_kvm_safe(device); > > ret = vfio_device_open(device, device->group->iommufd, device->kvm); > > diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h > index 20d715b0a3a8..24d6cd285945 100644 > --- a/drivers/vfio/vfio.h > +++ b/drivers/vfio/vfio.h > @@ -253,10 +253,11 @@ enum { vfio_noiommu = false }; > #endif > > #ifdef CONFIG_HAVE_KVM > -void vfio_device_get_kvm_safe(struct vfio_device *device); > +void _vfio_device_get_kvm_safe(struct vfio_device *device, struct kvm *kvm); > void vfio_device_put_kvm(struct vfio_device *device); > #else > -static inline void vfio_device_get_kvm_safe(struct vfio_device *device) > +static inline void _vfio_device_get_kvm_safe(struct vfio_device *device, > + struct kvm *kvm) > { > } > > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c > index 4762550e9f42..00d4d5167d6c 100644 > --- a/drivers/vfio/vfio_main.c > +++ b/drivers/vfio/vfio_main.c > @@ -342,7 +342,7 @@ void vfio_unregister_group_dev(struct vfio_device *device) > EXPORT_SYMBOL_GPL(vfio_unregister_group_dev); > > #ifdef CONFIG_HAVE_KVM > -void vfio_device_get_kvm_safe(struct vfio_device *device) > +void _vfio_device_get_kvm_safe(struct vfio_device *device, struct kvm *kvm) > { > void (*pfn)(struct kvm *kvm); > bool (*fn)(struct kvm *kvm); > @@ -350,32 +350,25 @@ void vfio_device_get_kvm_safe(struct vfio_device *device) > > lockdep_assert_held(&device->dev_set->lock); > > - spin_lock(&device->group->kvm_ref_lock); > - if (!device->group->kvm) > - goto unlock; > - > pfn = symbol_get(kvm_put_kvm); > if (WARN_ON(!pfn)) > - goto unlock; > + return; > > fn = symbol_get(kvm_get_kvm_safe); > if (WARN_ON(!fn)) { > symbol_put(kvm_put_kvm); > - goto unlock; > + return; > } > > ret = fn(device->group->kvm); s/device->group->kvm/kvm/ With that small change, this looks good to me too (and testing looks good too). Do you want me to send a v4 for one last round of review? Thanks, Matt