| Message ID | 1479138156-28905-13-git-send-email-kwankhede@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/14/2016 11:42 PM, Kirti Wankhede wrote: > Add a notifier calback to parent's ops structure of mdev device so that per > device notifer for vfio module is registered through vfio_mdev module. > > Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com> > Signed-off-by: Neo Jia <cjia@nvidia.com> > Change-Id: Iafa6f1721aecdd6e50eb93b153b5621e6d29b637 > --- > drivers/vfio/mdev/vfio_mdev.c | 19 +++++++++++++++++++ > include/linux/mdev.h | 9 +++++++++ > 2 files changed, 28 insertions(+) > > diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c > index ffc36758cb84..1694b1635607 100644 > --- a/drivers/vfio/mdev/vfio_mdev.c > +++ b/drivers/vfio/mdev/vfio_mdev.c > @@ -24,6 +24,15 @@ > #define DRIVER_AUTHOR "NVIDIA Corporation" > #define DRIVER_DESC "VFIO based driver for Mediated device" > > +static int vfio_mdev_notifier(struct notifier_block *nb, unsigned long action, > + void *data) > +{ > + struct mdev_device *mdev = container_of(nb, struct mdev_device, nb); > + struct parent_device *parent = mdev->parent; > + > + return parent->ops->notifier(mdev, action, data); > +} > + > static int vfio_mdev_open(void *device_data) > { > struct mdev_device *mdev = device_data; > @@ -40,6 +49,11 @@ static int vfio_mdev_open(void *device_data) > if (ret) > module_put(THIS_MODULE); > > + if (likely(parent->ops->notifier)) { > + mdev->nb.notifier_call = vfio_mdev_notifier; > + if (vfio_register_notifier(&mdev->dev, &mdev->nb)) > + pr_err("Failed to register notifier for mdev\n"); > + } Hi Kirti, Could you please move the notifier registration before parent->ops->open()? as you might know, I'm extending your vfio_register_notifier to also include the attaching/detaching events of vfio_group and kvm. Basically if vfio_group not attached to any kvm instance, the parent->ops->open() should return -ENODEV to indicate the failure, but to know whether kvm is available in open(), the notifier registration should be earlier. Of course I can call vfio_register_notifier() from an earlier place to workaround it, but it doesn't seem a canonical way. -- Thanks, Jike > return ret; > } > > @@ -48,6 +62,11 @@ static void vfio_mdev_release(void *device_data) > struct mdev_device *mdev = device_data; > struct parent_device *parent = mdev->parent; > > + if (likely(parent->ops->notifier)) { > + if (vfio_unregister_notifier(&mdev->dev, &mdev->nb)) > + pr_err("Failed to unregister notifier for mdev\n"); > + } > + > if (likely(parent->ops->release)) > parent->ops->release(mdev); > > diff --git a/include/linux/mdev.h b/include/linux/mdev.h > index 4900cc472364..665afe0a4c31 100644 > --- a/include/linux/mdev.h > +++ b/include/linux/mdev.h > @@ -37,6 +37,7 @@ struct mdev_device { > struct kref ref; > struct list_head next; > struct kobject *type_kobj; > + struct notifier_block nb; > }; > > /** > @@ -85,6 +86,12 @@ struct mdev_device { > * @mmap: mmap callback > * @mdev: mediated device structure > * @vma: vma structure > + * @notifer: Notifier callback, currently only for > + * VFIO_IOMMU_NOTIFY_DMA_UNMAP action notified duing > + * DMA_UNMAP call on mapped iova range. > + * @mdev: mediated device structure > + * @action: Action for which notifier is called > + * @data: Data associated with the notifier > * Parent device that support mediated device should be registered with mdev > * module with parent_ops structure. > **/ > @@ -106,6 +113,8 @@ struct parent_ops { > ssize_t (*ioctl)(struct mdev_device *mdev, unsigned int cmd, > unsigned long arg); > int (*mmap)(struct mdev_device *mdev, struct vm_area_struct *vma); > + int (*notifier)(struct mdev_device *mdev, unsigned long action, > + void *data); > }; > > /* interface for exporting mdev supported type attributes */ > -- 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/15/2016 12:15 PM, Jike Song wrote: > On 11/14/2016 11:42 PM, Kirti Wankhede wrote: >> Add a notifier calback to parent's ops structure of mdev device so that per >> device notifer for vfio module is registered through vfio_mdev module. >> >> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com> >> Signed-off-by: Neo Jia <cjia@nvidia.com> >> Change-Id: Iafa6f1721aecdd6e50eb93b153b5621e6d29b637 >> --- >> drivers/vfio/mdev/vfio_mdev.c | 19 +++++++++++++++++++ >> include/linux/mdev.h | 9 +++++++++ >> 2 files changed, 28 insertions(+) >> >> diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c >> index ffc36758cb84..1694b1635607 100644 >> --- a/drivers/vfio/mdev/vfio_mdev.c >> +++ b/drivers/vfio/mdev/vfio_mdev.c >> @@ -24,6 +24,15 @@ >> #define DRIVER_AUTHOR "NVIDIA Corporation" >> #define DRIVER_DESC "VFIO based driver for Mediated device" >> >> +static int vfio_mdev_notifier(struct notifier_block *nb, unsigned long action, >> + void *data) >> +{ >> + struct mdev_device *mdev = container_of(nb, struct mdev_device, nb); >> + struct parent_device *parent = mdev->parent; >> + >> + return parent->ops->notifier(mdev, action, data); >> +} >> + >> static int vfio_mdev_open(void *device_data) >> { >> struct mdev_device *mdev = device_data; >> @@ -40,6 +49,11 @@ static int vfio_mdev_open(void *device_data) >> if (ret) >> module_put(THIS_MODULE); >> >> + if (likely(parent->ops->notifier)) { >> + mdev->nb.notifier_call = vfio_mdev_notifier; >> + if (vfio_register_notifier(&mdev->dev, &mdev->nb)) >> + pr_err("Failed to register notifier for mdev\n"); >> + } > > Hi Kirti, > > Could you please move the notifier registration before parent->ops->open()? > as you might know, I'm extending your vfio_register_notifier to also include > the attaching/detaching events of vfio_group and kvm. Basically if vfio_group > not attached to any kvm instance, the parent->ops->open() should return -ENODEV > to indicate the failure, but to know whether kvm is available in open(), the > notifier registration should be earlier. > Ok. That seem fine to me. Thanks, Kirti > Of course I can call vfio_register_notifier() from an earlier place to > workaround it, but it doesn't seem a canonical way. > > -- > Thanks, > Jike > >> return ret; >> } >> >> @@ -48,6 +62,11 @@ static void vfio_mdev_release(void *device_data) >> struct mdev_device *mdev = device_data; >> struct parent_device *parent = mdev->parent; >> >> + if (likely(parent->ops->notifier)) { >> + if (vfio_unregister_notifier(&mdev->dev, &mdev->nb)) >> + pr_err("Failed to unregister notifier for mdev\n"); >> + } >> + >> if (likely(parent->ops->release)) >> parent->ops->release(mdev); >> >> diff --git a/include/linux/mdev.h b/include/linux/mdev.h >> index 4900cc472364..665afe0a4c31 100644 >> --- a/include/linux/mdev.h >> +++ b/include/linux/mdev.h >> @@ -37,6 +37,7 @@ struct mdev_device { >> struct kref ref; >> struct list_head next; >> struct kobject *type_kobj; >> + struct notifier_block nb; >> }; >> >> /** >> @@ -85,6 +86,12 @@ struct mdev_device { >> * @mmap: mmap callback >> * @mdev: mediated device structure >> * @vma: vma structure >> + * @notifer: Notifier callback, currently only for >> + * VFIO_IOMMU_NOTIFY_DMA_UNMAP action notified duing >> + * DMA_UNMAP call on mapped iova range. >> + * @mdev: mediated device structure >> + * @action: Action for which notifier is called >> + * @data: Data associated with the notifier >> * Parent device that support mediated device should be registered with mdev >> * module with parent_ops structure. >> **/ >> @@ -106,6 +113,8 @@ struct parent_ops { >> ssize_t (*ioctl)(struct mdev_device *mdev, unsigned int cmd, >> unsigned long arg); >> int (*mmap)(struct mdev_device *mdev, struct vm_area_struct *vma); >> + int (*notifier)(struct mdev_device *mdev, unsigned long action, >> + void *data); >> }; >> >> /* interface for exporting mdev supported type attributes */ >> -- 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/15/2016 04:11 PM, Kirti Wankhede wrote: > > > On 11/15/2016 12:15 PM, Jike Song wrote: >> On 11/14/2016 11:42 PM, Kirti Wankhede wrote: >>> Add a notifier calback to parent's ops structure of mdev device so that per >>> device notifer for vfio module is registered through vfio_mdev module. >>> >>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com> >>> Signed-off-by: Neo Jia <cjia@nvidia.com> >>> Change-Id: Iafa6f1721aecdd6e50eb93b153b5621e6d29b637 >>> --- >>> drivers/vfio/mdev/vfio_mdev.c | 19 +++++++++++++++++++ >>> include/linux/mdev.h | 9 +++++++++ >>> 2 files changed, 28 insertions(+) >>> >>> diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c >>> index ffc36758cb84..1694b1635607 100644 >>> --- a/drivers/vfio/mdev/vfio_mdev.c >>> +++ b/drivers/vfio/mdev/vfio_mdev.c >>> @@ -24,6 +24,15 @@ >>> #define DRIVER_AUTHOR "NVIDIA Corporation" >>> #define DRIVER_DESC "VFIO based driver for Mediated device" >>> >>> +static int vfio_mdev_notifier(struct notifier_block *nb, unsigned long action, >>> + void *data) >>> +{ >>> + struct mdev_device *mdev = container_of(nb, struct mdev_device, nb); >>> + struct parent_device *parent = mdev->parent; >>> + >>> + return parent->ops->notifier(mdev, action, data); >>> +} >>> + >>> static int vfio_mdev_open(void *device_data) >>> { >>> struct mdev_device *mdev = device_data; >>> @@ -40,6 +49,11 @@ static int vfio_mdev_open(void *device_data) >>> if (ret) >>> module_put(THIS_MODULE); >>> >>> + if (likely(parent->ops->notifier)) { >>> + mdev->nb.notifier_call = vfio_mdev_notifier; >>> + if (vfio_register_notifier(&mdev->dev, &mdev->nb)) >>> + pr_err("Failed to register notifier for mdev\n"); >>> + } >> >> Hi Kirti, >> >> Could you please move the notifier registration before parent->ops->open()? >> as you might know, I'm extending your vfio_register_notifier to also include >> the attaching/detaching events of vfio_group and kvm. Basically if vfio_group >> not attached to any kvm instance, the parent->ops->open() should return -ENODEV >> to indicate the failure, but to know whether kvm is available in open(), the >> notifier registration should be earlier. >> > > Ok. That seem fine to me. > Thanks - and I guess it's also good to move unregister after ->release(), so that a sequence of register-open-release-unregister guaranteed :) -- 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 Tue, 15 Nov 2016 14:45:42 +0800 Jike Song <jike.song@intel.com> wrote: > On 11/14/2016 11:42 PM, Kirti Wankhede wrote: > > Add a notifier calback to parent's ops structure of mdev device so that per > > device notifer for vfio module is registered through vfio_mdev module. > > > > Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com> > > Signed-off-by: Neo Jia <cjia@nvidia.com> > > Change-Id: Iafa6f1721aecdd6e50eb93b153b5621e6d29b637 > > --- > > drivers/vfio/mdev/vfio_mdev.c | 19 +++++++++++++++++++ > > include/linux/mdev.h | 9 +++++++++ > > 2 files changed, 28 insertions(+) > > > > diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c > > index ffc36758cb84..1694b1635607 100644 > > --- a/drivers/vfio/mdev/vfio_mdev.c > > +++ b/drivers/vfio/mdev/vfio_mdev.c > > @@ -24,6 +24,15 @@ > > #define DRIVER_AUTHOR "NVIDIA Corporation" > > #define DRIVER_DESC "VFIO based driver for Mediated device" > > > > +static int vfio_mdev_notifier(struct notifier_block *nb, unsigned long action, > > + void *data) > > +{ > > + struct mdev_device *mdev = container_of(nb, struct mdev_device, nb); > > + struct parent_device *parent = mdev->parent; > > + > > + return parent->ops->notifier(mdev, action, data); > > +} > > + > > static int vfio_mdev_open(void *device_data) > > { > > struct mdev_device *mdev = device_data; > > @@ -40,6 +49,11 @@ static int vfio_mdev_open(void *device_data) > > if (ret) > > module_put(THIS_MODULE); > > > > + if (likely(parent->ops->notifier)) { > > + mdev->nb.notifier_call = vfio_mdev_notifier; > > + if (vfio_register_notifier(&mdev->dev, &mdev->nb)) > > + pr_err("Failed to register notifier for mdev\n"); > > + } > > Hi Kirti, > > Could you please move the notifier registration before parent->ops->open()? > as you might know, I'm extending your vfio_register_notifier to also include > the attaching/detaching events of vfio_group and kvm. Basically if vfio_group > not attached to any kvm instance, the parent->ops->open() should return -ENODEV > to indicate the failure, but to know whether kvm is available in open(), the > notifier registration should be earlier. It seems like you're giving general guidance for how a vendor driver open() function should work, yet a hard dependency on KVM should be discouraged. You're making a choice for your vendor driver alone. I would also be very cautious about the coherency of signaling the KVM association relative to the user of the group. Is it possible that the association of one KVM instance by a user of the group can leak to the next user? Does vfio need to seen a gratuitous un-set of the KVM association on group close()? etc. Thanks, Alex > Of course I can call vfio_register_notifier() from an earlier place to > workaround it, but it doesn't seem a canonical way. > > -- > Thanks, > Jike > > > return ret; > > } > > > > @@ -48,6 +62,11 @@ static void vfio_mdev_release(void *device_data) > > struct mdev_device *mdev = device_data; > > struct parent_device *parent = mdev->parent; > > > > + if (likely(parent->ops->notifier)) { > > + if (vfio_unregister_notifier(&mdev->dev, &mdev->nb)) > > + pr_err("Failed to unregister notifier for mdev\n"); > > + } > > + > > if (likely(parent->ops->release)) > > parent->ops->release(mdev); > > > > diff --git a/include/linux/mdev.h b/include/linux/mdev.h > > index 4900cc472364..665afe0a4c31 100644 > > --- a/include/linux/mdev.h > > +++ b/include/linux/mdev.h > > @@ -37,6 +37,7 @@ struct mdev_device { > > struct kref ref; > > struct list_head next; > > struct kobject *type_kobj; > > + struct notifier_block nb; > > }; > > > > /** > > @@ -85,6 +86,12 @@ struct mdev_device { > > * @mmap: mmap callback > > * @mdev: mediated device structure > > * @vma: vma structure > > + * @notifer: Notifier callback, currently only for > > + * VFIO_IOMMU_NOTIFY_DMA_UNMAP action notified duing > > + * DMA_UNMAP call on mapped iova range. > > + * @mdev: mediated device structure > > + * @action: Action for which notifier is called > > + * @data: Data associated with the notifier > > * Parent device that support mediated device should be registered with mdev > > * module with parent_ops structure. > > **/ > > @@ -106,6 +113,8 @@ struct parent_ops { > > ssize_t (*ioctl)(struct mdev_device *mdev, unsigned int cmd, > > unsigned long arg); > > int (*mmap)(struct mdev_device *mdev, struct vm_area_struct *vma); > > + int (*notifier)(struct mdev_device *mdev, unsigned long action, > > + void *data); > > }; > > > > /* interface for exporting mdev supported type attributes */ > > -- 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/15/2016 11:19 PM, Alex Williamson wrote: > On Tue, 15 Nov 2016 14:45:42 +0800 > Jike Song <jike.song@intel.com> wrote: > >> On 11/14/2016 11:42 PM, Kirti Wankhede wrote: >>> Add a notifier calback to parent's ops structure of mdev device so that per >>> device notifer for vfio module is registered through vfio_mdev module. >>> >>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com> >>> Signed-off-by: Neo Jia <cjia@nvidia.com> >>> Change-Id: Iafa6f1721aecdd6e50eb93b153b5621e6d29b637 >>> --- >>> drivers/vfio/mdev/vfio_mdev.c | 19 +++++++++++++++++++ >>> include/linux/mdev.h | 9 +++++++++ >>> 2 files changed, 28 insertions(+) >>> >>> diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c >>> index ffc36758cb84..1694b1635607 100644 >>> --- a/drivers/vfio/mdev/vfio_mdev.c >>> +++ b/drivers/vfio/mdev/vfio_mdev.c >>> @@ -24,6 +24,15 @@ >>> #define DRIVER_AUTHOR "NVIDIA Corporation" >>> #define DRIVER_DESC "VFIO based driver for Mediated device" >>> >>> +static int vfio_mdev_notifier(struct notifier_block *nb, unsigned long action, >>> + void *data) >>> +{ >>> + struct mdev_device *mdev = container_of(nb, struct mdev_device, nb); >>> + struct parent_device *parent = mdev->parent; >>> + >>> + return parent->ops->notifier(mdev, action, data); >>> +} >>> + >>> static int vfio_mdev_open(void *device_data) >>> { >>> struct mdev_device *mdev = device_data; >>> @@ -40,6 +49,11 @@ static int vfio_mdev_open(void *device_data) >>> if (ret) >>> module_put(THIS_MODULE); >>> >>> + if (likely(parent->ops->notifier)) { >>> + mdev->nb.notifier_call = vfio_mdev_notifier; >>> + if (vfio_register_notifier(&mdev->dev, &mdev->nb)) >>> + pr_err("Failed to register notifier for mdev\n"); >>> + } >> >> Hi Kirti, >> >> Could you please move the notifier registration before parent->ops->open()? >> as you might know, I'm extending your vfio_register_notifier to also include >> the attaching/detaching events of vfio_group and kvm. Basically if vfio_group >> not attached to any kvm instance, the parent->ops->open() should return -ENODEV >> to indicate the failure, but to know whether kvm is available in open(), the >> notifier registration should be earlier. > > It seems like you're giving general guidance for how a vendor driver > open() function should work, yet a hard dependency on KVM should be > discouraged. You're making a choice for your vendor driver alone. I apologize for any confusion, but all I meant here was, if the real world requires a vendor driver to indicate errors instead of false success, it has to know some information before making the choice. > I would also be very cautious about the coherency of signaling the KVM > association relative to the user of the group. Is it possible that the > association of one KVM instance by a user of the group can leak to the > next user? Does vfio need to seen a gratuitous un-set of the KVM > association on group close()? etc. Thanks, I failed to see how this is possible, per my understanding the vfio_group_set_kvm gets called twice (once with kvm, another with NULL) during kvm's holding the group reference. Would you elaborate a bit more? -- 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
diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c index ffc36758cb84..1694b1635607 100644 --- a/drivers/vfio/mdev/vfio_mdev.c +++ b/drivers/vfio/mdev/vfio_mdev.c @@ -24,6 +24,15 @@ #define DRIVER_AUTHOR "NVIDIA Corporation" #define DRIVER_DESC "VFIO based driver for Mediated device" +static int vfio_mdev_notifier(struct notifier_block *nb, unsigned long action, + void *data) +{ + struct mdev_device *mdev = container_of(nb, struct mdev_device, nb); + struct parent_device *parent = mdev->parent; + + return parent->ops->notifier(mdev, action, data); +} + static int vfio_mdev_open(void *device_data) { struct mdev_device *mdev = device_data; @@ -40,6 +49,11 @@ static int vfio_mdev_open(void *device_data) if (ret) module_put(THIS_MODULE); + if (likely(parent->ops->notifier)) { + mdev->nb.notifier_call = vfio_mdev_notifier; + if (vfio_register_notifier(&mdev->dev, &mdev->nb)) + pr_err("Failed to register notifier for mdev\n"); + } return ret; } @@ -48,6 +62,11 @@ static void vfio_mdev_release(void *device_data) struct mdev_device *mdev = device_data; struct parent_device *parent = mdev->parent; + if (likely(parent->ops->notifier)) { + if (vfio_unregister_notifier(&mdev->dev, &mdev->nb)) + pr_err("Failed to unregister notifier for mdev\n"); + } + if (likely(parent->ops->release)) parent->ops->release(mdev); diff --git a/include/linux/mdev.h b/include/linux/mdev.h index 4900cc472364..665afe0a4c31 100644 --- a/include/linux/mdev.h +++ b/include/linux/mdev.h @@ -37,6 +37,7 @@ struct mdev_device { struct kref ref; struct list_head next; struct kobject *type_kobj; + struct notifier_block nb; }; /** @@ -85,6 +86,12 @@ struct mdev_device { * @mmap: mmap callback * @mdev: mediated device structure * @vma: vma structure + * @notifer: Notifier callback, currently only for + * VFIO_IOMMU_NOTIFY_DMA_UNMAP action notified duing + * DMA_UNMAP call on mapped iova range. + * @mdev: mediated device structure + * @action: Action for which notifier is called + * @data: Data associated with the notifier * Parent device that support mediated device should be registered with mdev * module with parent_ops structure. **/ @@ -106,6 +113,8 @@ struct parent_ops { ssize_t (*ioctl)(struct mdev_device *mdev, unsigned int cmd, unsigned long arg); int (*mmap)(struct mdev_device *mdev, struct vm_area_struct *vma); + int (*notifier)(struct mdev_device *mdev, unsigned long action, + void *data); }; /* interface for exporting mdev supported type attributes */