| Message ID | 58231B5C.3010506@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/09/2016 08:49 PM, Jike Song wrote: > +void vfio_group_attach_kvm(struct vfio_group *group, struct kvm *kvm, > + void (*fn)(struct kvm *)) > +{ > + mutex_lock(&group->udata.lock); This lock is needed, please see below. > + > + fn(kvm); > + blocking_notifier_call_chain(&group->udata.notifier, > + VFIO_GROUP_NOTIFY_ATTACH_KVM, kvm); As this is a callback before KVM releases its last refcount, i do not think vendor driver need to get additional KVM refcount. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/11/2016 14:06, Xiao Guangrong wrote: > > > On 11/09/2016 08:49 PM, Jike Song wrote: > >> +void vfio_group_attach_kvm(struct vfio_group *group, struct kvm *kvm, >> + void (*fn)(struct kvm *)) >> +{ >> + mutex_lock(&group->udata.lock); > > This lock is needed, please see below. *not* needed I guess. >> + >> + fn(kvm); >> + blocking_notifier_call_chain(&group->udata.notifier, >> + VFIO_GROUP_NOTIFY_ATTACH_KVM, kvm); > > As this is a callback before KVM releases its last refcount, i do not > think vendor driver need to get additional KVM refcount. The *group* driver doesn't need it indeed. The mdev vendor driver however does, so it will use kvm_get_kvm under its own mutex. That is: - attach kvm mutex_lock(mdev_driver->lock); mdev_driver->kvm = opaque; kvm_get_kvm(mdev_driver->kvm); mutex_unlock(mdev_driver->lock); - detach kvm mutex_lock(mdev_driver->lock); kvm_put_kvm(mdev_driver->kvm); WARN_ON(mdev_driver->kvm != opaque); mdev_driver->kvm = NULL; mutex_unlock(mdev_driver->lock); - use kvm mutex_lock(mdev_driver->lock); kvm = mdev_driver->kvm; ... mutex_unlock(mdev_driver->lock); or if safe: mutex_lock(mdev_driver->lock); kvm = mdev_driver->kvm; kvm_get_kvm(kvm); mutex_unlock(mdev_driver->lock); ... kvm_put_kvm(kvm); Thanks, Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/09/2016 09:31 PM, Paolo Bonzini wrote: > > > On 09/11/2016 14:06, Xiao Guangrong wrote: >> >> >> On 11/09/2016 08:49 PM, Jike Song wrote: >> >>> +void vfio_group_attach_kvm(struct vfio_group *group, struct kvm *kvm, >>> + void (*fn)(struct kvm *)) >>> +{ >>> + mutex_lock(&group->udata.lock); >> >> This lock is needed, please see below. > > *not* needed I guess. Yes, indeed. Sorry for the typo. :( > >>> + >>> + fn(kvm); >>> + blocking_notifier_call_chain(&group->udata.notifier, >>> + VFIO_GROUP_NOTIFY_ATTACH_KVM, kvm); >> >> As this is a callback before KVM releases its last refcount, i do not >> think vendor driver need to get additional KVM refcount. > > The *group* driver doesn't need it indeed. The mdev vendor driver > however does, so it will use kvm_get_kvm under its own mutex. That is: Yes, own mutex is definitely can work.:) It is vendor driver internal operation and it depends on the internal implementation. My idea is that we can make sure the KVM instance is valid before calling DETACH callback. So if we can properly release all the resource associated with the kvm instance in this callback, it is okay without additional kvm ref. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/11/2016 15:00, Xiao Guangrong wrote: >> >> The *group* driver doesn't need it indeed. The mdev vendor driver >> however does, so it will use kvm_get_kvm under its own mutex. That is: > > Yes, own mutex is definitely can work.:) It is vendor driver internal > operation and it depends on the internal implementation. > > My idea is that we can make sure the KVM instance is valid before calling > DETACH callback. So if we can properly release all the resource associated > with the kvm instance in this callback, it is okay without additional kvm > ref. That should work if they have their own mutex, but please add a comment. :) Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 09 Nov 2016 20:49:32 +0800 Jike Song <jike.song@intel.com> wrote: > On 11/08/2016 04:45 AM, Paolo Bonzini wrote: > > On 07/11/2016 19:28, Alex Williamson wrote: > >>>> Can the reference become invalid? > >>> > >>> No, this is guaranteed by virt/kvm/vfio.c + the udata.lock mutex (which > >>> probably should be renamed...). > >> > >> The caller gets a reference to kvm, but there's no guarantee that the > >> association of that kvm reference to the group stays valid. Once we're > >> outside of that mutex, we might as well consider that kvm:group > >> association stale. > >> > >>>> The caller may still hold > >>>> a kvm references, but couldn't the group be detached from one kvm > >>>> instance and re-attached to another? > >>> > >>> Can this be handled by the vendor driver? Does it get a callback when > >>> it's detached from a KVM instance? > >> > >> The only release callback through vfio is when the user closes the > >> device, the code in this series is the full extent of vfio awareness of > >> kvm. Thanks, > > > > Maybe there should be an mdev callback at the point of association and > > deassociation between VFIO and KVM. Then the vendor driver can just use > > the same mutex for association, deassociation and usage. I'm not even > > sure that these patches are necessary once you have that callback. > > Hi Alex & Paolo, > > So I cooked another draft version of this, there is no kvm pointer saved > in vfio_group in this version, and notifier will be called on attach/detach, > please kindly have a look :-) > > > -- > Thanks, > Jike > > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index ed2361e4..20b5da9 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -34,6 +34,7 @@ > #include <linux/uaccess.h> > #include <linux/vfio.h> > #include <linux/wait.h> > +#include <linux/kvm_host.h> > > #define DRIVER_VERSION "0.3" > #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>" > @@ -86,6 +87,10 @@ struct vfio_group { > struct mutex unbound_lock; > atomic_t opened; > bool noiommu; > + struct { > + struct mutex lock; > + struct blocking_notifier_head notifier; > + } udata; > }; > > struct vfio_device { > @@ -333,6 +338,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group) > mutex_init(&group->device_lock); > INIT_LIST_HEAD(&group->unbound_list); > mutex_init(&group->unbound_lock); > + mutex_init(&group->udata.lock); > atomic_set(&group->container_users, 0); > atomic_set(&group->opened, 0); > group->iommu_group = iommu_group; > @@ -414,10 +420,11 @@ static void vfio_group_release(struct kref *kref) > iommu_group_put(iommu_group); > } > > -static void vfio_group_put(struct vfio_group *group) > +void vfio_group_put(struct vfio_group *group) > { > kref_put_mutex(&group->kref, vfio_group_release, &vfio.group_lock); > } > +EXPORT_SYMBOL_GPL(vfio_group_put); > > /* Assume group_lock or group reference is held */ > static void vfio_group_get(struct vfio_group *group) > @@ -480,7 +487,7 @@ static struct vfio_group *vfio_group_get_from_minor(int minor) > return group; > } > > -static struct vfio_group *vfio_group_get_from_dev(struct device *dev) > +struct vfio_group *vfio_group_get_from_dev(struct device *dev) > { > struct iommu_group *iommu_group; > struct vfio_group *group; > @@ -494,6 +501,7 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev) > > return group; > } > +EXPORT_SYMBOL_GPL(vfio_group_get_from_dev); > > /** > * Device objects - create, release, get, put, search > @@ -1745,6 +1753,44 @@ long vfio_external_check_extension(struct vfio_group *group, unsigned long arg) > } > EXPORT_SYMBOL_GPL(vfio_external_check_extension); > > +int vfio_group_register_notifier(struct vfio_group *group, struct notifier_block *nb) > +{ > + return blocking_notifier_chain_register(&group->udata.notifier, nb); > +} > +EXPORT_SYMBOL_GPL(vfio_group_register_notifier); > + > +int vfio_group_unregister_notifier(struct vfio_group *group, struct notifier_block *nb) > +{ > + return blocking_notifier_chain_unregister(&group->udata.notifier, nb); > +} > +EXPORT_SYMBOL_GPL(vfio_group_unregister_notifier); Kirti is already adding vfio_register_notifier & vfio_unregister_notifier, these are not exclusive to the iommu, I clarified that in my question that IOVA range invalidation is just one aspect of what that notifier might be used for. The mdev framework also automatically registers and unregisters that notifier around open/release. So, I don't think we want a new notifier, we just want vfio.c to also consume that notifier. So I think this patch needs a few components that build on what Kirti has, 1) we add a blocking_notifier_head per vfio_group and have vfio_{un}regsiter_notifier add and remove that notifier to the group chain, 2) we create a vfio_group_notify() function that the kvm-vfio pseudo device can call via symbol_get, 3) Have kvm-vfio call vfio_group_notify() with VFIO_GROUP_NOTIFY_SET_KVM where the data is a pointer to the struct kvm (or NULL to unset, we don't need separate set vs unset notifiers). Does that work? Thanks, Alex > + > +void vfio_group_attach_kvm(struct vfio_group *group, struct kvm *kvm, > + void (*fn)(struct kvm *)) > +{ > + mutex_lock(&group->udata.lock); > + > + fn(kvm); > + blocking_notifier_call_chain(&group->udata.notifier, > + VFIO_GROUP_NOTIFY_ATTACH_KVM, kvm); > + > + mutex_unlock(&group->udata.lock); > +} > +EXPORT_SYMBOL_GPL(vfio_group_attach_kvm); > + > +void vfio_group_detach_kvm(struct vfio_group *group, struct kvm *kvm, > + void (*fn)(struct kvm *)) > +{ > + mutex_lock(&group->udata.lock); > + > + blocking_notifier_call_chain(&group->udata.notifier, > + VFIO_GROUP_NOTIFY_DETACH_KVM, kvm); > + fn(kvm); > + > + mutex_unlock(&group->udata.lock); > +} > +EXPORT_SYMBOL_GPL(vfio_group_detach_kvm); > + > /** > * Sub-module support > */ > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > index 87c9afe..4819a45 100644 > --- a/include/linux/vfio.h > +++ b/include/linux/vfio.h > @@ -102,6 +102,21 @@ extern void vfio_unregister_iommu_driver( > extern long vfio_external_check_extension(struct vfio_group *group, > unsigned long arg); > > +extern struct vfio_group *vfio_group_get_from_dev(struct device *dev); > +extern void vfio_group_put(struct vfio_group *group); > + > +#define VFIO_GROUP_NOTIFY_ATTACH_KVM 1 > +#define VFIO_GROUP_NOTIFY_DETACH_KVM 2 > +struct kvm; > +extern void vfio_group_attach_kvm(struct vfio_group *group, struct kvm *kvm, > + void (*fn)(struct kvm *)); > +extern void vfio_group_detach_kvm(struct vfio_group *group, struct kvm *kvm, > + void (*fn)(struct kvm *)); > +extern int vfio_group_register_notifier(struct vfio_group *group, > + struct notifier_block *nb); > +extern int vfio_group_unregister_notifier(struct vfio_group *group, > + struct notifier_block *nb); > + > /* > * Sub-module helpers > */ > diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c > index 1dd087d..d889b56 100644 > --- a/virt/kvm/vfio.c > +++ b/virt/kvm/vfio.c > @@ -60,6 +60,32 @@ static void kvm_vfio_group_put_external_user(struct vfio_group *vfio_group) > symbol_put(vfio_group_put_external_user); > } > > +static void kvm_vfio_group_attach_kvm(struct vfio_group *group, struct kvm_device *dev) > +{ > + void (*fn)(struct vfio_group *, struct kvm *, void (*fn)(struct kvm *)); > + > + fn = symbol_get(vfio_group_attach_kvm); > + if (!fn) > + return; > + > + fn(group, dev->kvm, kvm_get_kvm); > + > + symbol_put(vfio_group_attach_kvm); > +} > + > +static void kvm_vfio_group_detach_kvm(struct vfio_group *group, struct kvm *kvm) > +{ > + void (*fn)(struct vfio_group *, struct kvm *, void (*fn)(struct kvm *)); > + > + fn = symbol_get(vfio_group_detach_kvm); > + if (!fn) > + return; > + > + fn(group, kvm, kvm_put_kvm); > + > + symbol_put(vfio_group_detach_kvm); > +} > + > static bool kvm_vfio_group_is_coherent(struct vfio_group *vfio_group) > { > long (*fn)(struct vfio_group *, unsigned long); > @@ -155,6 +181,8 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg) > list_add_tail(&kvg->node, &kv->group_list); > kvg->vfio_group = vfio_group; > > + kvm_vfio_group_attach_kvm(vfio_group, dev); > + > kvm_arch_start_assignment(dev->kvm); > > mutex_unlock(&kv->lock); > @@ -196,6 +224,8 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg) > > mutex_unlock(&kv->lock); > > + kvm_vfio_group_detach_kvm(vfio_group, dev->kvm); > + > kvm_vfio_group_put_external_user(vfio_group); > > kvm_vfio_update_coherency(dev); > @@ -240,6 +270,7 @@ static void kvm_vfio_destroy(struct kvm_device *dev) > struct kvm_vfio_group *kvg, *tmp; > > list_for_each_entry_safe(kvg, tmp, &kv->group_list, node) { > + kvm_vfio_group_detach_kvm(kvg->vfio_group, dev->kvm); > kvm_vfio_group_put_external_user(kvg->vfio_group); > list_del(&kvg->node); > kfree(kvg); -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/10/2016 01:53 AM, Alex Williamson wrote: > On Wed, 09 Nov 2016 20:49:32 +0800 > Jike Song <jike.song@intel.com> wrote: > >> On 11/08/2016 04:45 AM, Paolo Bonzini wrote: >>> On 07/11/2016 19:28, Alex Williamson wrote: >>>>>> Can the reference become invalid? >>>>> >>>>> No, this is guaranteed by virt/kvm/vfio.c + the udata.lock mutex (which >>>>> probably should be renamed...). >>>> >>>> The caller gets a reference to kvm, but there's no guarantee that the >>>> association of that kvm reference to the group stays valid. Once we're >>>> outside of that mutex, we might as well consider that kvm:group >>>> association stale. >>>> >>>>>> The caller may still hold >>>>>> a kvm references, but couldn't the group be detached from one kvm >>>>>> instance and re-attached to another? >>>>> >>>>> Can this be handled by the vendor driver? Does it get a callback when >>>>> it's detached from a KVM instance? >>>> >>>> The only release callback through vfio is when the user closes the >>>> device, the code in this series is the full extent of vfio awareness of >>>> kvm. Thanks, >>> >>> Maybe there should be an mdev callback at the point of association and >>> deassociation between VFIO and KVM. Then the vendor driver can just use >>> the same mutex for association, deassociation and usage. I'm not even >>> sure that these patches are necessary once you have that callback. >> >> Hi Alex & Paolo, >> >> So I cooked another draft version of this, there is no kvm pointer saved >> in vfio_group in this version, and notifier will be called on attach/detach, >> please kindly have a look :-) >> >> >> -- >> Thanks, >> Jike >> >> >> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c >> index ed2361e4..20b5da9 100644 >> --- a/drivers/vfio/vfio.c >> +++ b/drivers/vfio/vfio.c >> @@ -34,6 +34,7 @@ >> #include <linux/uaccess.h> >> #include <linux/vfio.h> >> #include <linux/wait.h> >> +#include <linux/kvm_host.h> >> >> #define DRIVER_VERSION "0.3" >> #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>" >> @@ -86,6 +87,10 @@ struct vfio_group { >> struct mutex unbound_lock; >> atomic_t opened; >> bool noiommu; >> + struct { >> + struct mutex lock; >> + struct blocking_notifier_head notifier; >> + } udata; >> }; >> >> struct vfio_device { >> @@ -333,6 +338,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group) >> mutex_init(&group->device_lock); >> INIT_LIST_HEAD(&group->unbound_list); >> mutex_init(&group->unbound_lock); >> + mutex_init(&group->udata.lock); >> atomic_set(&group->container_users, 0); >> atomic_set(&group->opened, 0); >> group->iommu_group = iommu_group; >> @@ -414,10 +420,11 @@ static void vfio_group_release(struct kref *kref) >> iommu_group_put(iommu_group); >> } >> >> -static void vfio_group_put(struct vfio_group *group) >> +void vfio_group_put(struct vfio_group *group) >> { >> kref_put_mutex(&group->kref, vfio_group_release, &vfio.group_lock); >> } >> +EXPORT_SYMBOL_GPL(vfio_group_put); >> >> /* Assume group_lock or group reference is held */ >> static void vfio_group_get(struct vfio_group *group) >> @@ -480,7 +487,7 @@ static struct vfio_group *vfio_group_get_from_minor(int minor) >> return group; >> } >> >> -static struct vfio_group *vfio_group_get_from_dev(struct device *dev) >> +struct vfio_group *vfio_group_get_from_dev(struct device *dev) >> { >> struct iommu_group *iommu_group; >> struct vfio_group *group; >> @@ -494,6 +501,7 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev) >> >> return group; >> } >> +EXPORT_SYMBOL_GPL(vfio_group_get_from_dev); >> >> /** >> * Device objects - create, release, get, put, search >> @@ -1745,6 +1753,44 @@ long vfio_external_check_extension(struct vfio_group *group, unsigned long arg) >> } >> EXPORT_SYMBOL_GPL(vfio_external_check_extension); >> >> +int vfio_group_register_notifier(struct vfio_group *group, struct notifier_block *nb) >> +{ >> + return blocking_notifier_chain_register(&group->udata.notifier, nb); >> +} >> +EXPORT_SYMBOL_GPL(vfio_group_register_notifier); >> + >> +int vfio_group_unregister_notifier(struct vfio_group *group, struct notifier_block *nb) >> +{ >> + return blocking_notifier_chain_unregister(&group->udata.notifier, nb); >> +} >> +EXPORT_SYMBOL_GPL(vfio_group_unregister_notifier); > > Kirti is already adding vfio_register_notifier & > vfio_unregister_notifier, these are not exclusive to the iommu, I > clarified that in my question that IOVA range invalidation is just one > aspect of what that notifier might be used for. The mdev framework > also automatically registers and unregisters that notifier around > open/release. So, I don't think we want a new notifier, we just want > vfio.c to also consume that notifier. Unfortunately the kvm:group attaching happens before device opening, so registering the notifier in open() is not functional: the event has disappeared before we start watching it. A possible workaround is, register the notifier in create() instead of open(). That should be functional, but will cause another issue: being able to register a notifier means we have a vfio-group reference, when to put that reference? putting it in remove() is not a good idea since a device might be open/release multiple times between create/remove, holding the ref until removal breaks it; putting it in release() is obviously not a good idea neither. IOW, having the notifiers there must be some dirty work in vendor driver to work around the issue above :( > So I think this patch needs a few components that build on what Kirti > has, 1) we add a blocking_notifier_head per vfio_group and have > vfio_{un}regsiter_notifier add and remove that notifier to the group > chain, 2) we create a vfio_group_notify() function that the kvm-vfio > pseudo device can call via symbol_get, 3) Have kvm-vfio call > vfio_group_notify() with VFIO_GROUP_NOTIFY_SET_KVM where the data is a > pointer to the struct kvm (or NULL to unset, we don't need separate set > vs unset notifiers). Does that work? Thanks, Yes, it works better than the original form of below patch. vfio side doesn't store any data, nor introduce any lock, only a callback for kvm to use. -- Thanks, Jike -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/09/2016 10:00 PM, Xiao Guangrong wrote: > On 11/09/2016 09:31 PM, Paolo Bonzini wrote: >> On 09/11/2016 14:06, Xiao Guangrong wrote: >>> On 11/09/2016 08:49 PM, Jike Song wrote: >>> >>>> +void vfio_group_attach_kvm(struct vfio_group *group, struct kvm *kvm, >>>> + void (*fn)(struct kvm *)) >>>> +{ >>>> + mutex_lock(&group->udata.lock); >>> >>> This lock is needed, please see below. >> >> *not* needed I guess. > > Yes, indeed. Sorry for the typo. :( > >> >>>> + >>>> + fn(kvm); >>>> + blocking_notifier_call_chain(&group->udata.notifier, >>>> + VFIO_GROUP_NOTIFY_ATTACH_KVM, kvm); >>> >>> As this is a callback before KVM releases its last refcount, i do not >>> think vendor driver need to get additional KVM refcount. >> >> The *group* driver doesn't need it indeed. The mdev vendor driver >> however does, so it will use kvm_get_kvm under its own mutex. That is: > > Yes, own mutex is definitely can work.:) It is vendor driver internal > operation and it depends on the internal implementation. > > My idea is that we can make sure the KVM instance is valid before calling > DETACH callback. So if we can properly release all the resource associated > with the kvm instance in this callback, it is okay without additional kvm > ref. Yes, the mutex in vfio_group is actually pointless, now I understand and agree. Thanks Guangrong and Polao :) -- Thanks, Jike -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/10/2016 12:10 PM, Jike Song wrote: > On 11/10/2016 01:53 AM, Alex Williamson wrote: >> On Wed, 09 Nov 2016 20:49:32 +0800 >> Jike Song <jike.song@intel.com> wrote: >> >>> On 11/08/2016 04:45 AM, Paolo Bonzini wrote: >>>> On 07/11/2016 19:28, Alex Williamson wrote: >>>>>>> Can the reference become invalid? >>>>>> >>>>>> No, this is guaranteed by virt/kvm/vfio.c + the udata.lock mutex (which >>>>>> probably should be renamed...). >>>>> >>>>> The caller gets a reference to kvm, but there's no guarantee that the >>>>> association of that kvm reference to the group stays valid. Once we're >>>>> outside of that mutex, we might as well consider that kvm:group >>>>> association stale. >>>>> >>>>>>> The caller may still hold >>>>>>> a kvm references, but couldn't the group be detached from one kvm >>>>>>> instance and re-attached to another? >>>>>> >>>>>> Can this be handled by the vendor driver? Does it get a callback when >>>>>> it's detached from a KVM instance? >>>>> >>>>> The only release callback through vfio is when the user closes the >>>>> device, the code in this series is the full extent of vfio awareness of >>>>> kvm. Thanks, >>>> >>>> Maybe there should be an mdev callback at the point of association and >>>> deassociation between VFIO and KVM. Then the vendor driver can just use >>>> the same mutex for association, deassociation and usage. I'm not even >>>> sure that these patches are necessary once you have that callback. >>> >>> Hi Alex & Paolo, >>> >>> So I cooked another draft version of this, there is no kvm pointer saved >>> in vfio_group in this version, and notifier will be called on attach/detach, >>> please kindly have a look :-) >>> >>> >>> -- >>> Thanks, >>> Jike >>> >>> >>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c >>> index ed2361e4..20b5da9 100644 >>> --- a/drivers/vfio/vfio.c >>> +++ b/drivers/vfio/vfio.c >>> @@ -34,6 +34,7 @@ >>> #include <linux/uaccess.h> >>> #include <linux/vfio.h> >>> #include <linux/wait.h> >>> +#include <linux/kvm_host.h> >>> >>> #define DRIVER_VERSION "0.3" >>> #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>" >>> @@ -86,6 +87,10 @@ struct vfio_group { >>> struct mutex unbound_lock; >>> atomic_t opened; >>> bool noiommu; >>> + struct { >>> + struct mutex lock; >>> + struct blocking_notifier_head notifier; >>> + } udata; >>> }; >>> >>> struct vfio_device { >>> @@ -333,6 +338,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group) >>> mutex_init(&group->device_lock); >>> INIT_LIST_HEAD(&group->unbound_list); >>> mutex_init(&group->unbound_lock); >>> + mutex_init(&group->udata.lock); >>> atomic_set(&group->container_users, 0); >>> atomic_set(&group->opened, 0); >>> group->iommu_group = iommu_group; >>> @@ -414,10 +420,11 @@ static void vfio_group_release(struct kref *kref) >>> iommu_group_put(iommu_group); >>> } >>> >>> -static void vfio_group_put(struct vfio_group *group) >>> +void vfio_group_put(struct vfio_group *group) >>> { >>> kref_put_mutex(&group->kref, vfio_group_release, &vfio.group_lock); >>> } >>> +EXPORT_SYMBOL_GPL(vfio_group_put); >>> >>> /* Assume group_lock or group reference is held */ >>> static void vfio_group_get(struct vfio_group *group) >>> @@ -480,7 +487,7 @@ static struct vfio_group *vfio_group_get_from_minor(int minor) >>> return group; >>> } >>> >>> -static struct vfio_group *vfio_group_get_from_dev(struct device *dev) >>> +struct vfio_group *vfio_group_get_from_dev(struct device *dev) >>> { >>> struct iommu_group *iommu_group; >>> struct vfio_group *group; >>> @@ -494,6 +501,7 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev) >>> >>> return group; >>> } >>> +EXPORT_SYMBOL_GPL(vfio_group_get_from_dev); >>> >>> /** >>> * Device objects - create, release, get, put, search >>> @@ -1745,6 +1753,44 @@ long vfio_external_check_extension(struct vfio_group *group, unsigned long arg) >>> } >>> EXPORT_SYMBOL_GPL(vfio_external_check_extension); >>> >>> +int vfio_group_register_notifier(struct vfio_group *group, struct notifier_block *nb) >>> +{ >>> + return blocking_notifier_chain_register(&group->udata.notifier, nb); >>> +} >>> +EXPORT_SYMBOL_GPL(vfio_group_register_notifier); >>> + >>> +int vfio_group_unregister_notifier(struct vfio_group *group, struct notifier_block *nb) >>> +{ >>> + return blocking_notifier_chain_unregister(&group->udata.notifier, nb); >>> +} >>> +EXPORT_SYMBOL_GPL(vfio_group_unregister_notifier); >> >> Kirti is already adding vfio_register_notifier & >> vfio_unregister_notifier, these are not exclusive to the iommu, I >> clarified that in my question that IOVA range invalidation is just one >> aspect of what that notifier might be used for. The mdev framework >> also automatically registers and unregisters that notifier around >> open/release. So, I don't think we want a new notifier, we just want >> vfio.c to also consume that notifier. > > Unfortunately the kvm:group attaching happens before device opening, > so registering the notifier in open() is not functional: the event > has disappeared before we start watching it. > > A possible workaround is, register the notifier in create() instead of > open(). That should be functional, but will cause another issue: being able > to register a notifier means we have a vfio-group reference, when to put > that reference? putting it in remove() is not a good idea since a device > might be open/release multiple times between create/remove, holding the ref > until removal breaks it; putting it in release() is obviously not a > good idea neither. > > IOW, having the notifiers there must be some dirty work in vendor > driver to work around the issue above :( > >> So I think this patch needs a few components that build on what Kirti >> has, 1) we add a blocking_notifier_head per vfio_group and have >> vfio_{un}regsiter_notifier add and remove that notifier to the group >> chain, 2) we create a vfio_group_notify() function that the kvm-vfio >> pseudo device can call via symbol_get, 3) Have kvm-vfio call >> vfio_group_notify() with VFIO_GROUP_NOTIFY_SET_KVM where the data is a >> pointer to the struct kvm (or NULL to unset, we don't need separate set >> vs unset notifiers). Does that work? Thanks, > > Yes, it works better than the original form of below patch. > vfio side doesn't store any data, nor introduce any lock, only a callback > for kvm to use. > To make my reply clearer: the notifier can work without two separate set/unset, can be combined with Kirti's iommu notifier, however, the problem of being too late to register from open() still exists, and I still find it difficult to work around. -- Thanks, Jike -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 10 Nov 2016 14:04:19 +0800 Jike Song <jike.song@intel.com> wrote: > On 11/10/2016 12:10 PM, Jike Song wrote: > > On 11/10/2016 01:53 AM, Alex Williamson wrote: > >> On Wed, 09 Nov 2016 20:49:32 +0800 > >> Jike Song <jike.song@intel.com> wrote: > >> > >>> On 11/08/2016 04:45 AM, Paolo Bonzini wrote: > >>>> On 07/11/2016 19:28, Alex Williamson wrote: > >>>>>>> Can the reference become invalid? > >>>>>> > >>>>>> No, this is guaranteed by virt/kvm/vfio.c + the udata.lock mutex (which > >>>>>> probably should be renamed...). > >>>>> > >>>>> The caller gets a reference to kvm, but there's no guarantee that the > >>>>> association of that kvm reference to the group stays valid. Once we're > >>>>> outside of that mutex, we might as well consider that kvm:group > >>>>> association stale. > >>>>> > >>>>>>> The caller may still hold > >>>>>>> a kvm references, but couldn't the group be detached from one kvm > >>>>>>> instance and re-attached to another? > >>>>>> > >>>>>> Can this be handled by the vendor driver? Does it get a callback when > >>>>>> it's detached from a KVM instance? > >>>>> > >>>>> The only release callback through vfio is when the user closes the > >>>>> device, the code in this series is the full extent of vfio awareness of > >>>>> kvm. Thanks, > >>>> > >>>> Maybe there should be an mdev callback at the point of association and > >>>> deassociation between VFIO and KVM. Then the vendor driver can just use > >>>> the same mutex for association, deassociation and usage. I'm not even > >>>> sure that these patches are necessary once you have that callback. > >>> > >>> Hi Alex & Paolo, > >>> > >>> So I cooked another draft version of this, there is no kvm pointer saved > >>> in vfio_group in this version, and notifier will be called on attach/detach, > >>> please kindly have a look :-) > >>> > >>> > >>> -- > >>> Thanks, > >>> Jike > >>> > >>> > >>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > >>> index ed2361e4..20b5da9 100644 > >>> --- a/drivers/vfio/vfio.c > >>> +++ b/drivers/vfio/vfio.c > >>> @@ -34,6 +34,7 @@ > >>> #include <linux/uaccess.h> > >>> #include <linux/vfio.h> > >>> #include <linux/wait.h> > >>> +#include <linux/kvm_host.h> > >>> > >>> #define DRIVER_VERSION "0.3" > >>> #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>" > >>> @@ -86,6 +87,10 @@ struct vfio_group { > >>> struct mutex unbound_lock; > >>> atomic_t opened; > >>> bool noiommu; > >>> + struct { > >>> + struct mutex lock; > >>> + struct blocking_notifier_head notifier; > >>> + } udata; > >>> }; > >>> > >>> struct vfio_device { > >>> @@ -333,6 +338,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group) > >>> mutex_init(&group->device_lock); > >>> INIT_LIST_HEAD(&group->unbound_list); > >>> mutex_init(&group->unbound_lock); > >>> + mutex_init(&group->udata.lock); > >>> atomic_set(&group->container_users, 0); > >>> atomic_set(&group->opened, 0); > >>> group->iommu_group = iommu_group; > >>> @@ -414,10 +420,11 @@ static void vfio_group_release(struct kref *kref) > >>> iommu_group_put(iommu_group); > >>> } > >>> > >>> -static void vfio_group_put(struct vfio_group *group) > >>> +void vfio_group_put(struct vfio_group *group) > >>> { > >>> kref_put_mutex(&group->kref, vfio_group_release, &vfio.group_lock); > >>> } > >>> +EXPORT_SYMBOL_GPL(vfio_group_put); > >>> > >>> /* Assume group_lock or group reference is held */ > >>> static void vfio_group_get(struct vfio_group *group) > >>> @@ -480,7 +487,7 @@ static struct vfio_group *vfio_group_get_from_minor(int minor) > >>> return group; > >>> } > >>> > >>> -static struct vfio_group *vfio_group_get_from_dev(struct device *dev) > >>> +struct vfio_group *vfio_group_get_from_dev(struct device *dev) > >>> { > >>> struct iommu_group *iommu_group; > >>> struct vfio_group *group; > >>> @@ -494,6 +501,7 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev) > >>> > >>> return group; > >>> } > >>> +EXPORT_SYMBOL_GPL(vfio_group_get_from_dev); > >>> > >>> /** > >>> * Device objects - create, release, get, put, search > >>> @@ -1745,6 +1753,44 @@ long vfio_external_check_extension(struct vfio_group *group, unsigned long arg) > >>> } > >>> EXPORT_SYMBOL_GPL(vfio_external_check_extension); > >>> > >>> +int vfio_group_register_notifier(struct vfio_group *group, struct notifier_block *nb) > >>> +{ > >>> + return blocking_notifier_chain_register(&group->udata.notifier, nb); > >>> +} > >>> +EXPORT_SYMBOL_GPL(vfio_group_register_notifier); > >>> + > >>> +int vfio_group_unregister_notifier(struct vfio_group *group, struct notifier_block *nb) > >>> +{ > >>> + return blocking_notifier_chain_unregister(&group->udata.notifier, nb); > >>> +} > >>> +EXPORT_SYMBOL_GPL(vfio_group_unregister_notifier); > >> > >> Kirti is already adding vfio_register_notifier & > >> vfio_unregister_notifier, these are not exclusive to the iommu, I > >> clarified that in my question that IOVA range invalidation is just one > >> aspect of what that notifier might be used for. The mdev framework > >> also automatically registers and unregisters that notifier around > >> open/release. So, I don't think we want a new notifier, we just want > >> vfio.c to also consume that notifier. > > > > Unfortunately the kvm:group attaching happens before device opening, > > so registering the notifier in open() is not functional: the event > > has disappeared before we start watching it. > > > > A possible workaround is, register the notifier in create() instead of > > open(). That should be functional, but will cause another issue: being able > > to register a notifier means we have a vfio-group reference, when to put > > that reference? putting it in remove() is not a good idea since a device > > might be open/release multiple times between create/remove, holding the ref > > until removal breaks it; putting it in release() is obviously not a > > good idea neither. > > > > IOW, having the notifiers there must be some dirty work in vendor > > driver to work around the issue above :( > > > >> So I think this patch needs a few components that build on what Kirti > >> has, 1) we add a blocking_notifier_head per vfio_group and have > >> vfio_{un}regsiter_notifier add and remove that notifier to the group > >> chain, 2) we create a vfio_group_notify() function that the kvm-vfio > >> pseudo device can call via symbol_get, 3) Have kvm-vfio call > >> vfio_group_notify() with VFIO_GROUP_NOTIFY_SET_KVM where the data is a > >> pointer to the struct kvm (or NULL to unset, we don't need separate set > >> vs unset notifiers). Does that work? Thanks, > > > > Yes, it works better than the original form of below patch. > > vfio side doesn't store any data, nor introduce any lock, only a callback > > for kvm to use. > > > > To make my reply clearer: the notifier can work without two separate > set/unset, can be combined with Kirti's iommu notifier, however, the problem > of being too late to register from open() still exists, and I still find it > difficult to work around. Ok, so it's a little bit ugly, but kvm-vfio can tell vfio about struct kvm with a vfio_group_set_kvm() callback that it uses via symbol get, using the real struct kvm pointer or NULL to unset. vfio caches this on the vfio_group. When a notifier is registered, it replays the event through the notifier. Any updates while a notifier is connected are both cached and immediately replayed through the notifier. So the vendor driver to vfio communication channel is the same, but the vfio to kvm-vfio involves some buffering. Does that work? Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/10/2016 11:37 PM, Alex Williamson wrote: > On Thu, 10 Nov 2016 14:04:19 +0800 > Jike Song <jike.song@intel.com> wrote: > >> On 11/10/2016 12:10 PM, Jike Song wrote: >>> On 11/10/2016 01:53 AM, Alex Williamson wrote: >>>> On Wed, 09 Nov 2016 20:49:32 +0800 >>>> Jike Song <jike.song@intel.com> wrote: >>>> >>>>> On 11/08/2016 04:45 AM, Paolo Bonzini wrote: >>>>>> On 07/11/2016 19:28, Alex Williamson wrote: >>>>>>>>> Can the reference become invalid? >>>>>>>> >>>>>>>> No, this is guaranteed by virt/kvm/vfio.c + the udata.lock mutex (which >>>>>>>> probably should be renamed...). >>>>>>> >>>>>>> The caller gets a reference to kvm, but there's no guarantee that the >>>>>>> association of that kvm reference to the group stays valid. Once we're >>>>>>> outside of that mutex, we might as well consider that kvm:group >>>>>>> association stale. >>>>>>> >>>>>>>>> The caller may still hold >>>>>>>>> a kvm references, but couldn't the group be detached from one kvm >>>>>>>>> instance and re-attached to another? >>>>>>>> >>>>>>>> Can this be handled by the vendor driver? Does it get a callback when >>>>>>>> it's detached from a KVM instance? >>>>>>> >>>>>>> The only release callback through vfio is when the user closes the >>>>>>> device, the code in this series is the full extent of vfio awareness of >>>>>>> kvm. Thanks, >>>>>> >>>>>> Maybe there should be an mdev callback at the point of association and >>>>>> deassociation between VFIO and KVM. Then the vendor driver can just use >>>>>> the same mutex for association, deassociation and usage. I'm not even >>>>>> sure that these patches are necessary once you have that callback. >>>>> >>>>> Hi Alex & Paolo, >>>>> >>>>> So I cooked another draft version of this, there is no kvm pointer saved >>>>> in vfio_group in this version, and notifier will be called on attach/detach, >>>>> please kindly have a look :-) >>>>> >>>>> >>>>> -- >>>>> Thanks, >>>>> Jike >>>>> >>>>> >>>>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c >>>>> index ed2361e4..20b5da9 100644 >>>>> --- a/drivers/vfio/vfio.c >>>>> +++ b/drivers/vfio/vfio.c >>>>> @@ -34,6 +34,7 @@ >>>>> #include <linux/uaccess.h> >>>>> #include <linux/vfio.h> >>>>> #include <linux/wait.h> >>>>> +#include <linux/kvm_host.h> >>>>> >>>>> #define DRIVER_VERSION "0.3" >>>>> #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>" >>>>> @@ -86,6 +87,10 @@ struct vfio_group { >>>>> struct mutex unbound_lock; >>>>> atomic_t opened; >>>>> bool noiommu; >>>>> + struct { >>>>> + struct mutex lock; >>>>> + struct blocking_notifier_head notifier; >>>>> + } udata; >>>>> }; >>>>> >>>>> struct vfio_device { >>>>> @@ -333,6 +338,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group) >>>>> mutex_init(&group->device_lock); >>>>> INIT_LIST_HEAD(&group->unbound_list); >>>>> mutex_init(&group->unbound_lock); >>>>> + mutex_init(&group->udata.lock); >>>>> atomic_set(&group->container_users, 0); >>>>> atomic_set(&group->opened, 0); >>>>> group->iommu_group = iommu_group; >>>>> @@ -414,10 +420,11 @@ static void vfio_group_release(struct kref *kref) >>>>> iommu_group_put(iommu_group); >>>>> } >>>>> >>>>> -static void vfio_group_put(struct vfio_group *group) >>>>> +void vfio_group_put(struct vfio_group *group) >>>>> { >>>>> kref_put_mutex(&group->kref, vfio_group_release, &vfio.group_lock); >>>>> } >>>>> +EXPORT_SYMBOL_GPL(vfio_group_put); >>>>> >>>>> /* Assume group_lock or group reference is held */ >>>>> static void vfio_group_get(struct vfio_group *group) >>>>> @@ -480,7 +487,7 @@ static struct vfio_group *vfio_group_get_from_minor(int minor) >>>>> return group; >>>>> } >>>>> >>>>> -static struct vfio_group *vfio_group_get_from_dev(struct device *dev) >>>>> +struct vfio_group *vfio_group_get_from_dev(struct device *dev) >>>>> { >>>>> struct iommu_group *iommu_group; >>>>> struct vfio_group *group; >>>>> @@ -494,6 +501,7 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev) >>>>> >>>>> return group; >>>>> } >>>>> +EXPORT_SYMBOL_GPL(vfio_group_get_from_dev); >>>>> >>>>> /** >>>>> * Device objects - create, release, get, put, search >>>>> @@ -1745,6 +1753,44 @@ long vfio_external_check_extension(struct vfio_group *group, unsigned long arg) >>>>> } >>>>> EXPORT_SYMBOL_GPL(vfio_external_check_extension); >>>>> >>>>> +int vfio_group_register_notifier(struct vfio_group *group, struct notifier_block *nb) >>>>> +{ >>>>> + return blocking_notifier_chain_register(&group->udata.notifier, nb); >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(vfio_group_register_notifier); >>>>> + >>>>> +int vfio_group_unregister_notifier(struct vfio_group *group, struct notifier_block *nb) >>>>> +{ >>>>> + return blocking_notifier_chain_unregister(&group->udata.notifier, nb); >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(vfio_group_unregister_notifier); >>>> >>>> Kirti is already adding vfio_register_notifier & >>>> vfio_unregister_notifier, these are not exclusive to the iommu, I >>>> clarified that in my question that IOVA range invalidation is just one >>>> aspect of what that notifier might be used for. The mdev framework >>>> also automatically registers and unregisters that notifier around >>>> open/release. So, I don't think we want a new notifier, we just want >>>> vfio.c to also consume that notifier. >>> >>> Unfortunately the kvm:group attaching happens before device opening, >>> so registering the notifier in open() is not functional: the event >>> has disappeared before we start watching it. >>> >>> A possible workaround is, register the notifier in create() instead of >>> open(). That should be functional, but will cause another issue: being able >>> to register a notifier means we have a vfio-group reference, when to put >>> that reference? putting it in remove() is not a good idea since a device >>> might be open/release multiple times between create/remove, holding the ref >>> until removal breaks it; putting it in release() is obviously not a >>> good idea neither. >>> >>> IOW, having the notifiers there must be some dirty work in vendor >>> driver to work around the issue above :( >>> >>>> So I think this patch needs a few components that build on what Kirti >>>> has, 1) we add a blocking_notifier_head per vfio_group and have >>>> vfio_{un}regsiter_notifier add and remove that notifier to the group >>>> chain, 2) we create a vfio_group_notify() function that the kvm-vfio >>>> pseudo device can call via symbol_get, 3) Have kvm-vfio call >>>> vfio_group_notify() with VFIO_GROUP_NOTIFY_SET_KVM where the data is a >>>> pointer to the struct kvm (or NULL to unset, we don't need separate set >>>> vs unset notifiers). Does that work? Thanks, >>> >>> Yes, it works better than the original form of below patch. >>> vfio side doesn't store any data, nor introduce any lock, only a callback >>> for kvm to use. >>> >> >> To make my reply clearer: the notifier can work without two separate >> set/unset, can be combined with Kirti's iommu notifier, however, the problem >> of being too late to register from open() still exists, and I still find it >> difficult to work around. > > Ok, so it's a little bit ugly, but kvm-vfio can tell vfio about struct > kvm with a vfio_group_set_kvm() callback that it uses via symbol get, > using the real struct kvm pointer or NULL to unset. vfio caches this > on the vfio_group. When a notifier is registered, it replays the event > through the notifier. Any updates while a notifier is connected are > both cached and immediately replayed through the notifier. So the > vendor driver to vfio communication channel is the same, but the vfio > to kvm-vfio involves some buffering. Does that work? Thanks, Yes it works, thanks for the suggestion, I'll post a v4 series. -- Thanks, Jike -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/10/2016 01:53 AM, Alex Williamson wrote: > On Wed, 09 Nov 2016 20:49:32 +0800 > Jike Song <jike.song@intel.com> wrote: > >> On 11/08/2016 04:45 AM, Paolo Bonzini wrote: >>> On 07/11/2016 19:28, Alex Williamson wrote: >>>>>> Can the reference become invalid? >>>>> >>>>> No, this is guaranteed by virt/kvm/vfio.c + the udata.lock mutex (which >>>>> probably should be renamed...). >>>> >>>> The caller gets a reference to kvm, but there's no guarantee that the >>>> association of that kvm reference to the group stays valid. Once we're >>>> outside of that mutex, we might as well consider that kvm:group >>>> association stale. >>>> >>>>>> The caller may still hold >>>>>> a kvm references, but couldn't the group be detached from one kvm >>>>>> instance and re-attached to another? >>>>> >>>>> Can this be handled by the vendor driver? Does it get a callback when >>>>> it's detached from a KVM instance? >>>> >>>> The only release callback through vfio is when the user closes the >>>> device, the code in this series is the full extent of vfio awareness of >>>> kvm. Thanks, >>> >>> Maybe there should be an mdev callback at the point of association and >>> deassociation between VFIO and KVM. Then the vendor driver can just use >>> the same mutex for association, deassociation and usage. I'm not even >>> sure that these patches are necessary once you have that callback. >> >> Hi Alex & Paolo, >> >> So I cooked another draft version of this, there is no kvm pointer saved >> in vfio_group in this version, and notifier will be called on attach/detach, >> please kindly have a look :-) >> >> >> -- >> Thanks, >> Jike >> >> >> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c >> index ed2361e4..20b5da9 100644 >> --- a/drivers/vfio/vfio.c >> +++ b/drivers/vfio/vfio.c >> @@ -34,6 +34,7 @@ >> #include <linux/uaccess.h> >> #include <linux/vfio.h> >> #include <linux/wait.h> >> +#include <linux/kvm_host.h> >> >> #define DRIVER_VERSION "0.3" >> #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>" >> @@ -86,6 +87,10 @@ struct vfio_group { >> struct mutex unbound_lock; >> atomic_t opened; >> bool noiommu; >> + struct { >> + struct mutex lock; >> + struct blocking_notifier_head notifier; >> + } udata; >> }; >> >> struct vfio_device { >> @@ -333,6 +338,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group) >> mutex_init(&group->device_lock); >> INIT_LIST_HEAD(&group->unbound_list); >> mutex_init(&group->unbound_lock); >> + mutex_init(&group->udata.lock); >> atomic_set(&group->container_users, 0); >> atomic_set(&group->opened, 0); >> group->iommu_group = iommu_group; >> @@ -414,10 +420,11 @@ static void vfio_group_release(struct kref *kref) >> iommu_group_put(iommu_group); >> } >> >> -static void vfio_group_put(struct vfio_group *group) >> +void vfio_group_put(struct vfio_group *group) >> { >> kref_put_mutex(&group->kref, vfio_group_release, &vfio.group_lock); >> } >> +EXPORT_SYMBOL_GPL(vfio_group_put); >> >> /* Assume group_lock or group reference is held */ >> static void vfio_group_get(struct vfio_group *group) >> @@ -480,7 +487,7 @@ static struct vfio_group *vfio_group_get_from_minor(int minor) >> return group; >> } >> >> -static struct vfio_group *vfio_group_get_from_dev(struct device *dev) >> +struct vfio_group *vfio_group_get_from_dev(struct device *dev) >> { >> struct iommu_group *iommu_group; >> struct vfio_group *group; >> @@ -494,6 +501,7 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev) >> >> return group; >> } >> +EXPORT_SYMBOL_GPL(vfio_group_get_from_dev); >> >> /** >> * Device objects - create, release, get, put, search >> @@ -1745,6 +1753,44 @@ long vfio_external_check_extension(struct vfio_group *group, unsigned long arg) >> } >> EXPORT_SYMBOL_GPL(vfio_external_check_extension); >> >> +int vfio_group_register_notifier(struct vfio_group *group, struct notifier_block *nb) >> +{ >> + return blocking_notifier_chain_register(&group->udata.notifier, nb); >> +} >> +EXPORT_SYMBOL_GPL(vfio_group_register_notifier); >> + >> +int vfio_group_unregister_notifier(struct vfio_group *group, struct notifier_block *nb) >> +{ >> + return blocking_notifier_chain_unregister(&group->udata.notifier, nb); >> +} >> +EXPORT_SYMBOL_GPL(vfio_group_unregister_notifier); > > Kirti is already adding vfio_register_notifier & > vfio_unregister_notifier, these are not exclusive to the iommu, I > clarified that in my question that IOVA range invalidation is just one > aspect of what that notifier might be used for. The mdev framework > also automatically registers and unregisters that notifier around > open/release. So, I don't think we want a new notifier, we just want > vfio.c to also consume that notifier. > Hi Alex, Sorry, I have one more question: does combining Kirti's iommu notifier and my group notifier mean there should only one blocking_notifier_head? If so, where should it be? vfio_container, vfio_group or vfio_iommu? -- Thanks, Jike > So I think this patch needs a few components that build on what Kirti > has, 1) we add a blocking_notifier_head per vfio_group and have > vfio_{un}regsiter_notifier add and remove that notifier to the group > chain, 2) we create a vfio_group_notify() function that the kvm-vfio > pseudo device can call via symbol_get, 3) Have kvm-vfio call > vfio_group_notify() with VFIO_GROUP_NOTIFY_SET_KVM where the data is a > pointer to the struct kvm (or NULL to unset, we don't need separate set > vs unset notifiers). Does that work? Thanks, > > Alex > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 14 Nov 2016 18:19:34 +0800 Jike Song <jike.song@intel.com> wrote: > On 11/10/2016 01:53 AM, Alex Williamson wrote: > > On Wed, 09 Nov 2016 20:49:32 +0800 > > Jike Song <jike.song@intel.com> wrote: > > > >> On 11/08/2016 04:45 AM, Paolo Bonzini wrote: > >>> On 07/11/2016 19:28, Alex Williamson wrote: > >>>>>> Can the reference become invalid? > >>>>> > >>>>> No, this is guaranteed by virt/kvm/vfio.c + the udata.lock mutex (which > >>>>> probably should be renamed...). > >>>> > >>>> The caller gets a reference to kvm, but there's no guarantee that the > >>>> association of that kvm reference to the group stays valid. Once we're > >>>> outside of that mutex, we might as well consider that kvm:group > >>>> association stale. > >>>> > >>>>>> The caller may still hold > >>>>>> a kvm references, but couldn't the group be detached from one kvm > >>>>>> instance and re-attached to another? > >>>>> > >>>>> Can this be handled by the vendor driver? Does it get a callback when > >>>>> it's detached from a KVM instance? > >>>> > >>>> The only release callback through vfio is when the user closes the > >>>> device, the code in this series is the full extent of vfio awareness of > >>>> kvm. Thanks, > >>> > >>> Maybe there should be an mdev callback at the point of association and > >>> deassociation between VFIO and KVM. Then the vendor driver can just use > >>> the same mutex for association, deassociation and usage. I'm not even > >>> sure that these patches are necessary once you have that callback. > >> > >> Hi Alex & Paolo, > >> > >> So I cooked another draft version of this, there is no kvm pointer saved > >> in vfio_group in this version, and notifier will be called on attach/detach, > >> please kindly have a look :-) > >> > >> > >> -- > >> Thanks, > >> Jike > >> > >> > >> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > >> index ed2361e4..20b5da9 100644 > >> --- a/drivers/vfio/vfio.c > >> +++ b/drivers/vfio/vfio.c > >> @@ -34,6 +34,7 @@ > >> #include <linux/uaccess.h> > >> #include <linux/vfio.h> > >> #include <linux/wait.h> > >> +#include <linux/kvm_host.h> > >> > >> #define DRIVER_VERSION "0.3" > >> #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>" > >> @@ -86,6 +87,10 @@ struct vfio_group { > >> struct mutex unbound_lock; > >> atomic_t opened; > >> bool noiommu; > >> + struct { > >> + struct mutex lock; > >> + struct blocking_notifier_head notifier; > >> + } udata; > >> }; > >> > >> struct vfio_device { > >> @@ -333,6 +338,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group) > >> mutex_init(&group->device_lock); > >> INIT_LIST_HEAD(&group->unbound_list); > >> mutex_init(&group->unbound_lock); > >> + mutex_init(&group->udata.lock); > >> atomic_set(&group->container_users, 0); > >> atomic_set(&group->opened, 0); > >> group->iommu_group = iommu_group; > >> @@ -414,10 +420,11 @@ static void vfio_group_release(struct kref *kref) > >> iommu_group_put(iommu_group); > >> } > >> > >> -static void vfio_group_put(struct vfio_group *group) > >> +void vfio_group_put(struct vfio_group *group) > >> { > >> kref_put_mutex(&group->kref, vfio_group_release, &vfio.group_lock); > >> } > >> +EXPORT_SYMBOL_GPL(vfio_group_put); > >> > >> /* Assume group_lock or group reference is held */ > >> static void vfio_group_get(struct vfio_group *group) > >> @@ -480,7 +487,7 @@ static struct vfio_group *vfio_group_get_from_minor(int minor) > >> return group; > >> } > >> > >> -static struct vfio_group *vfio_group_get_from_dev(struct device *dev) > >> +struct vfio_group *vfio_group_get_from_dev(struct device *dev) > >> { > >> struct iommu_group *iommu_group; > >> struct vfio_group *group; > >> @@ -494,6 +501,7 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev) > >> > >> return group; > >> } > >> +EXPORT_SYMBOL_GPL(vfio_group_get_from_dev); > >> > >> /** > >> * Device objects - create, release, get, put, search > >> @@ -1745,6 +1753,44 @@ long vfio_external_check_extension(struct vfio_group *group, unsigned long arg) > >> } > >> EXPORT_SYMBOL_GPL(vfio_external_check_extension); > >> > >> +int vfio_group_register_notifier(struct vfio_group *group, struct notifier_block *nb) > >> +{ > >> + return blocking_notifier_chain_register(&group->udata.notifier, nb); > >> +} > >> +EXPORT_SYMBOL_GPL(vfio_group_register_notifier); > >> + > >> +int vfio_group_unregister_notifier(struct vfio_group *group, struct notifier_block *nb) > >> +{ > >> + return blocking_notifier_chain_unregister(&group->udata.notifier, nb); > >> +} > >> +EXPORT_SYMBOL_GPL(vfio_group_unregister_notifier); > > > > Kirti is already adding vfio_register_notifier & > > vfio_unregister_notifier, these are not exclusive to the iommu, I > > clarified that in my question that IOVA range invalidation is just one > > aspect of what that notifier might be used for. The mdev framework > > also automatically registers and unregisters that notifier around > > open/release. So, I don't think we want a new notifier, we just want > > vfio.c to also consume that notifier. > > > > Hi Alex, > > Sorry, I have one more question: does combining Kirti's iommu notifier > and my group notifier mean there should only one blocking_notifier_head? > If so, where should it be? vfio_container, vfio_group or vfio_iommu? I suspect the most straightforward approach is to place a blocking_notifier_head on the vfio_group in addition to the one that Kirti has placed on the vfio_iommu. Both will include the same notifier_block from the vendor driver and call the notifier chain independently. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c index ed2361e4..20b5da9 100644 --- a/drivers/vfio/vfio.c +++ b/drivers/vfio/vfio.c @@ -34,6 +34,7 @@ #include <linux/uaccess.h> #include <linux/vfio.h> #include <linux/wait.h> +#include <linux/kvm_host.h> #define DRIVER_VERSION "0.3" #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>" @@ -86,6 +87,10 @@ struct vfio_group { struct mutex unbound_lock; atomic_t opened; bool noiommu; + struct { + struct mutex lock; + struct blocking_notifier_head notifier; + } udata; }; struct vfio_device { @@ -333,6 +338,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group) mutex_init(&group->device_lock); INIT_LIST_HEAD(&group->unbound_list); mutex_init(&group->unbound_lock); + mutex_init(&group->udata.lock); atomic_set(&group->container_users, 0); atomic_set(&group->opened, 0); group->iommu_group = iommu_group; @@ -414,10 +420,11 @@ static void vfio_group_release(struct kref *kref) iommu_group_put(iommu_group); } -static void vfio_group_put(struct vfio_group *group) +void vfio_group_put(struct vfio_group *group) { kref_put_mutex(&group->kref, vfio_group_release, &vfio.group_lock); } +EXPORT_SYMBOL_GPL(vfio_group_put); /* Assume group_lock or group reference is held */ static void vfio_group_get(struct vfio_group *group) @@ -480,7 +487,7 @@ static struct vfio_group *vfio_group_get_from_minor(int minor) return group; } -static struct vfio_group *vfio_group_get_from_dev(struct device *dev) +struct vfio_group *vfio_group_get_from_dev(struct device *dev) { struct iommu_group *iommu_group; struct vfio_group *group; @@ -494,6 +501,7 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev) return group; } +EXPORT_SYMBOL_GPL(vfio_group_get_from_dev); /** * Device objects - create, release, get, put, search @@ -1745,6 +1753,44 @@ long vfio_external_check_extension(struct vfio_group *group, unsigned long arg) } EXPORT_SYMBOL_GPL(vfio_external_check_extension); +int vfio_group_register_notifier(struct vfio_group *group, struct notifier_block *nb) +{ + return blocking_notifier_chain_register(&group->udata.notifier, nb); +} +EXPORT_SYMBOL_GPL(vfio_group_register_notifier); + +int vfio_group_unregister_notifier(struct vfio_group *group, struct notifier_block *nb) +{ + return blocking_notifier_chain_unregister(&group->udata.notifier, nb); +} +EXPORT_SYMBOL_GPL(vfio_group_unregister_notifier); + +void vfio_group_attach_kvm(struct vfio_group *group, struct kvm *kvm, + void (*fn)(struct kvm *)) +{ + mutex_lock(&group->udata.lock); + + fn(kvm); + blocking_notifier_call_chain(&group->udata.notifier, + VFIO_GROUP_NOTIFY_ATTACH_KVM, kvm); + + mutex_unlock(&group->udata.lock); +} +EXPORT_SYMBOL_GPL(vfio_group_attach_kvm); + +void vfio_group_detach_kvm(struct vfio_group *group, struct kvm *kvm, + void (*fn)(struct kvm *)) +{ + mutex_lock(&group->udata.lock); + + blocking_notifier_call_chain(&group->udata.notifier, + VFIO_GROUP_NOTIFY_DETACH_KVM, kvm); + fn(kvm); + + mutex_unlock(&group->udata.lock); +} +EXPORT_SYMBOL_GPL(vfio_group_detach_kvm); + /** * Sub-module support */ diff --git a/include/linux/vfio.h b/include/linux/vfio.h index 87c9afe..4819a45 100644 --- a/include/linux/vfio.h +++ b/include/linux/vfio.h @@ -102,6 +102,21 @@ extern void vfio_unregister_iommu_driver( extern long vfio_external_check_extension(struct vfio_group *group, unsigned long arg); +extern struct vfio_group *vfio_group_get_from_dev(struct device *dev); +extern void vfio_group_put(struct vfio_group *group); + +#define VFIO_GROUP_NOTIFY_ATTACH_KVM 1 +#define VFIO_GROUP_NOTIFY_DETACH_KVM 2 +struct kvm; +extern void vfio_group_attach_kvm(struct vfio_group *group, struct kvm *kvm, + void (*fn)(struct kvm *)); +extern void vfio_group_detach_kvm(struct vfio_group *group, struct kvm *kvm, + void (*fn)(struct kvm *)); +extern int vfio_group_register_notifier(struct vfio_group *group, + struct notifier_block *nb); +extern int vfio_group_unregister_notifier(struct vfio_group *group, + struct notifier_block *nb); + /* * Sub-module helpers */ diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c index 1dd087d..d889b56 100644 --- a/virt/kvm/vfio.c +++ b/virt/kvm/vfio.c @@ -60,6 +60,32 @@ static void kvm_vfio_group_put_external_user(struct vfio_group *vfio_group) symbol_put(vfio_group_put_external_user); } +static void kvm_vfio_group_attach_kvm(struct vfio_group *group, struct kvm_device *dev) +{ + void (*fn)(struct vfio_group *, struct kvm *, void (*fn)(struct kvm *)); + + fn = symbol_get(vfio_group_attach_kvm); + if (!fn) + return; + + fn(group, dev->kvm, kvm_get_kvm); + + symbol_put(vfio_group_attach_kvm); +} + +static void kvm_vfio_group_detach_kvm(struct vfio_group *group, struct kvm *kvm) +{ + void (*fn)(struct vfio_group *, struct kvm *, void (*fn)(struct kvm *)); + + fn = symbol_get(vfio_group_detach_kvm); + if (!fn) + return; + + fn(group, kvm, kvm_put_kvm); + + symbol_put(vfio_group_detach_kvm); +} + static bool kvm_vfio_group_is_coherent(struct vfio_group *vfio_group) { long (*fn)(struct vfio_group *, unsigned long); @@ -155,6 +181,8 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg) list_add_tail(&kvg->node, &kv->group_list); kvg->vfio_group = vfio_group; + kvm_vfio_group_attach_kvm(vfio_group, dev); + kvm_arch_start_assignment(dev->kvm); mutex_unlock(&kv->lock); @@ -196,6 +224,8 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg) mutex_unlock(&kv->lock); + kvm_vfio_group_detach_kvm(vfio_group, dev->kvm); + kvm_vfio_group_put_external_user(vfio_group); kvm_vfio_update_coherency(dev); @@ -240,6 +270,7 @@ static void kvm_vfio_destroy(struct kvm_device *dev) struct kvm_vfio_group *kvg, *tmp; list_for_each_entry_safe(kvg, tmp, &kv->group_list, node) { + kvm_vfio_group_detach_kvm(kvg->vfio_group, dev->kvm); kvm_vfio_group_put_external_user(kvg->vfio_group); list_del(&kvg->node); kfree(kvg);