On Fri, 3 Feb 2023 12:29:01 -0500 Matthew Rosato <[email protected]> wrote:
> On 2/3/23 10:19 AM, Alex Williamson wrote: > > On Fri, 3 Feb 2023 14:54:44 +0000 > > "Liu, Yi L" <[email protected]> wrote: > > > >>> From: Alex Williamson <[email protected]> > >>> Sent: Friday, February 3, 2023 9:50 PM > >>> > >>> On Fri, 3 Feb 2023 13:32:09 +0000 > >>> "Liu, Yi L" <[email protected]> wrote: > >>> > >>>>> From: Tian, Kevin <[email protected]> > >>>>> Sent: Friday, February 3, 2023 10:00 AM > >>>>> > >>>>>> From: Alex Williamson <[email protected]> > >>>>>> Sent: Friday, February 3, 2023 7:13 AM > >>>>>> > >>>>>> On Thu, 2 Feb 2023 23:04:10 +0000 > >>>>>> "Tian, Kevin" <[email protected]> wrote: > >>>>>> > >>>>>>>> From: Alex Williamson <[email protected]> > >>>>>>>> Sent: Friday, February 3, 2023 3:42 AM > >>>>>>>> > >>>>>>>> > >>>>>>>> LGTM. I'm not sure moving the functions to vfio_main really buys > >>> us > >>>>>>>> anything since we're making so much use of group fields. The cdev > >>>>>>>> approach will necessarily be different, so the bulk of the get code > >>>>>>>> > >>> will > >>>>>>>> likely need to move back to group.c anyway. > >>>>>>>> > >>>>>>> > >>>>>>> well my last comment was based on Matthew's v2 where the get > >>> code > >>>>>>> gets a kvm passed in instead of implicitly retrieving group ref_lock > >>>>>>> internally. In that case the get/put helpers only contain device logic > >>>>>>> thus fit in vfio_main.c. > >>>>>>> > >>>>>>> with v3 then they have to be in group.c since we don't want to use > >>>>>>> group fields in vfio_main.c. > >>>>>>> > >>>>>>> but I still think v2 of the helpers is slightly better. The only > >>>>>>> difference > >>>>>>> between cdev and group when handling this race is using different > >>>>>>> ref_lock. the symbol get/put part is exactly same. So even if we > >>>>>>> merge v3 like this, very likely Yi has to change it back to v2 style > >>>>>>> to share the get/put helpers while just leaving the ref_lock part > >>>>>>> handled differently between the two path. > >>>>>> > >>>>>> I'm not really a fan of the asymmetry of the v2 version where the get > >>>>>> helper needs to be called under the new kvm_ref_lock, but the put > >>>>>> helper does not. Having the get helper handle that makes the caller > >>>>>> much cleaner. Thanks, > >>>>>> > >>>>> > >>>>> What about passing the lock pointer into the helper? it's still slightly > >>>>> asymmetry as the put helper doesn't carry the lock pointer but it > >>>>> could also be interpreted as if the pointer has been saved in the get > >>>>> then if it needs to be referenced by the put there is no need to pass > >>>>> it in again. > >>>> > >>>> For cdev, I may modify vfio_device_get_kvm_safe() to accept > >>>> struct kvm and let its caller hold a kvm_ref_lock (field within > >>>> struct vfio_device_file). Meanwhile, the group path holds > >>>> the group->kvm_ref_lock before invoking vfio_device_get_kvm_safe(). > >>>> vfio_device_get_kvm_safe() just includes the symbol get/put and > >>>> the device->kvm and put_kvm set. > >>> > >>> Sounds a lot like v2 :-\ > >> > >> Yes, like v2. 😊 > >> > >>> I'd look more towards group and cdev specific > >>> helpers that handle the locking so that the callers aren't exposed to > >>> the asymmetry of get vs put, and reduce a new > >>> _vfio_device_get_kvm_safe() in common code that only does the symbol > >>> work. Thanks, > >> > >> If so, looks like Matthew needs a v4. I'm waiting for the final version > >> of this patch and sending a new cdev series based on it. wish to see > >> it soon ^_^. > > > > cdev support is a future feature, why does it become a requirement for > > a fix to the current base? The refactoring could also happen in the > > cdev series. Thanks, > > > > Alex > > > > FWIW, that would bloat the fix by a bit and it's already a decent-sized > fix... I took a quick stab and such a v4 would end up with a total of 120 > insertions(+), 15 deletions(-). See below for a diff of what I tried on top > of v3. The idea would be to move the serialization and setting/clearing of > device->kvm into group/cdev and use device->kvm as the value within > vfio_device_get_kvm_safe for getting the kvm ref. Wrappers in group/cdev > would then be responsible for backing that device->kvm setting out on ref > failure. > Each side (group/cdev) can wrap their own serialization / load device->kvm > from the appropriate location / handle the failed get / clear device->kvm > when done (since get doesn't set it, put shouldn't clear it). > > If Alex wants, I can wrap something like this into a v4 -- Or, if we don't > want to include this kind of infrastructure in the fix, then Yi please feel > free to use it as a starting point for what you need in cdev. > > Thanks, > Matt > > diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c > index 7fed4233ca23..261af23975ae 100644 > --- a/drivers/vfio/group.c > +++ b/drivers/vfio/group.c > @@ -154,6 +154,31 @@ static int vfio_group_ioctl_set_container(struct > vfio_group *group, > return ret; > } > > +static void vfio_device_group_get_kvm(struct vfio_device *device) > +{ > + lockdep_assert_held(&device->dev_set->lock); > + > + spin_lock(&device->group->kvm_ref_lock); > + > + if (!device->group->kvm) > + goto unlock; > + > + device->kvm = device->group->kvm; > + if (!vfio_device_get_kvm_safe(device)) 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); symbol_put(kvm_get_kvm_safe); if (!ret) { symbol_put(kvm_put_kvm); - goto unlock; + return; } device->put_kvm = pfn; - device->kvm = device->group->kvm; - -unlock: - spin_unlock(&device->group->kvm_ref_lock); + device->kvm = kvm; } void vfio_device_put_kvm(struct vfio_device *device)
