From: Kirti Wankhede <kwankhede@nvidia.com> To: "Tian, Kevin" <kevin.tian@intel.com>, "alex.williamson@redhat.com" <alex.williamson@redhat.com>, "pbonzini@redhat.com" <pbonzini@redhat.com>, "kraxel@redhat.com" <kraxel@redhat.com>, "cjia@nvidia.com" <cjia@nvidia.com> Cc: "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>, "kvm@vger.kernel.org" <kvm@vger.kernel.org>, "Song, Jike" <jike.song@intel.com>, "bjsdjshi@linux.vnet.ibm.com" <bjsdjshi@linux.vnet.ibm.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v9 01/12] vfio: Mediated device Core driver Date: Wed, 26 Oct 2016 20:28:16 +0530 [thread overview] Message-ID: <813a9246-f3cc-7f53-08a6-6a0673a273e6@nvidia.com> (raw) In-Reply-To: <AADFC41AFE54684AB9EE6CBC0274A5D18DFE31F9@SHSMSX101.ccr.corp.intel.com> >> Medisted bus driver is responsible to add/delete mediated devices to/from > > Medisted -> Mediated > Thanks for pointing out the typeo. Correcting it. >> VFIO group when devices are bound and unbound to the driver. >> >> 2. Physical device driver interface >> This interface provides vendor driver the set APIs to manage physical >> device related work in its driver. APIs are : >> >> * dev_attr_groups: attributes of the parent device. >> * mdev_attr_groups: attributes of the mediated device. >> * supported_type_groups: attributes to define supported type. This is >> mandatory field. >> * create: to allocate basic resources in driver for a mediated device. > > in 'which driver'? it should be clear to remove 'in driver' here > >> * remove: to free resources in driver when mediated device is destroyed. >> * open: open callback of mediated device >> * release: release callback of mediated device >> * read : read emulation callback. >> * write: write emulation callback. >> * mmap: mmap emulation callback. >> * ioctl: ioctl callback. > > You only highlight 'mandatory field' for supported_type_groups. What > about other fields? Are all of them optional? Please clarify and also > stay consistent to later code comment. > 'create' and 'remove' are mandatory. Updating the description here. Rest all are not cross-checked in mdev core driver, like 'create' and 'remove' but yes rest are optional. If vendor driver don't want to support emulated region they don't need read/write callbacks. Similarly if vendor driver don't want to support mmap region, they don't need mmap callback. Code comments are consistent with this description. ... >> + >> +config VFIO_MDEV >> + tristate "Mediated device driver framework" >> + depends on VFIO >> + default n >> + help >> + Provides a framework to virtualize devices which don't have SR_IOV >> + capability built-in. > > This statement is not accurate. A device can support SR-IOV, but in the same > time using this mediated technology w/ SR-IOV capability disabled. > If SR-IOV is supported why would user use this framework? SR-IOV would give better performance. ... >> + >> +static struct mdev_device *__find_mdev_device(struct parent_device *parent, >> + uuid_le uuid) > > parent_find_mdev_device? > This function search for mdev device with given UUID, so I think its consistent what we have below for parent, __find_parent_device(). ... >> +static int mdev_device_remove_ops(struct mdev_device *mdev, bool force_remove) >> +{ > > > Can you add some comment here about when force_remove may be expected > here, which would help others understand immediately instead of walking through > the whole patch set? > mdev_device_remove_ops gets called from sysfs's 'remove' and when parent device is being unregistered from mdev device framework. - 'force_remove' is set to 'false' when called from sysfs's 'remove' which indicates that if the mdev device is active, used by VMM or userspace application, vendor driver could return error then don't remove the device. - 'force_remove' is set to 'true' when called from mdev_unregister_device() which indicate that parent device is being removed from mdev device framework so remove mdev device forcefully. > >> + struct parent_device *parent = mdev->parent; >> + int ret; >> + >> + /* >> + * Vendor driver can return error if VMM or userspace application is >> + * using this mdev device. >> + */ >> + ret = parent->ops->remove(mdev); > > what about passing force_remove flag to remove callback, so vendor driver > can decide whether any force cleanup required? > 'remove' getting called from sysfs is asynchronous, so vendor driver can retrun failure in that case if vendor driver finds that mdev device is being actively used. mdev_unregister_device() is going to be called from vendor driver itself when device is being unbound or driver is being unloaded. In this case vendor driver can identify itself that its in its own teardown path. So I feel there is no need to pass force_remove flag to 'remove' callback. >> +int mdev_create_sysfs_files(struct device *dev, struct mdev_type *type) >> +{ >> + int ret; >> + >> + ret = sysfs_create_files(&dev->kobj, mdev_device_attrs); >> + if (ret) { >> + pr_err("Failed to create remove sysfs entry\n"); >> + return ret; >> + } >> + >> + ret = sysfs_create_link(type->devices_kobj, &dev->kobj, dev_name(dev)); >> + if (ret) { >> + pr_err("Failed to create symlink in types\n"); > > looks wrong place... > No, this is correct. Above function creates symlink in mdev_supported_types/<type>/devices directory. >> + goto device_link_failed; >> + } >> + >> + ret = sysfs_create_link(&dev->kobj, &type->kobj, "mdev_type"); >> + if (ret) { >> + pr_err("Failed to create symlink in device directory\n"); > > exchange with above. > Again this is also correct. Above creates 'mdev_type' symlink in mdev device's directory. Although these are correct, removing these error prints. You can find it from its return type or if sysfs functions through warnings. Kirti.
WARNING: multiple messages have this Message-ID (diff)
From: Kirti Wankhede <kwankhede@nvidia.com> To: "Tian, Kevin" <kevin.tian@intel.com>, "alex.williamson@redhat.com" <alex.williamson@redhat.com>, "pbonzini@redhat.com" <pbonzini@redhat.com>, "kraxel@redhat.com" <kraxel@redhat.com>, "cjia@nvidia.com" <cjia@nvidia.com> Cc: "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>, "kvm@vger.kernel.org" <kvm@vger.kernel.org>, "Song, Jike" <jike.song@intel.com>, "bjsdjshi@linux.vnet.ibm.com" <bjsdjshi@linux.vnet.ibm.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> Subject: Re: [Qemu-devel] [PATCH v9 01/12] vfio: Mediated device Core driver Date: Wed, 26 Oct 2016 20:28:16 +0530 [thread overview] Message-ID: <813a9246-f3cc-7f53-08a6-6a0673a273e6@nvidia.com> (raw) In-Reply-To: <AADFC41AFE54684AB9EE6CBC0274A5D18DFE31F9@SHSMSX101.ccr.corp.intel.com> >> Medisted bus driver is responsible to add/delete mediated devices to/from > > Medisted -> Mediated > Thanks for pointing out the typeo. Correcting it. >> VFIO group when devices are bound and unbound to the driver. >> >> 2. Physical device driver interface >> This interface provides vendor driver the set APIs to manage physical >> device related work in its driver. APIs are : >> >> * dev_attr_groups: attributes of the parent device. >> * mdev_attr_groups: attributes of the mediated device. >> * supported_type_groups: attributes to define supported type. This is >> mandatory field. >> * create: to allocate basic resources in driver for a mediated device. > > in 'which driver'? it should be clear to remove 'in driver' here > >> * remove: to free resources in driver when mediated device is destroyed. >> * open: open callback of mediated device >> * release: release callback of mediated device >> * read : read emulation callback. >> * write: write emulation callback. >> * mmap: mmap emulation callback. >> * ioctl: ioctl callback. > > You only highlight 'mandatory field' for supported_type_groups. What > about other fields? Are all of them optional? Please clarify and also > stay consistent to later code comment. > 'create' and 'remove' are mandatory. Updating the description here. Rest all are not cross-checked in mdev core driver, like 'create' and 'remove' but yes rest are optional. If vendor driver don't want to support emulated region they don't need read/write callbacks. Similarly if vendor driver don't want to support mmap region, they don't need mmap callback. Code comments are consistent with this description. ... >> + >> +config VFIO_MDEV >> + tristate "Mediated device driver framework" >> + depends on VFIO >> + default n >> + help >> + Provides a framework to virtualize devices which don't have SR_IOV >> + capability built-in. > > This statement is not accurate. A device can support SR-IOV, but in the same > time using this mediated technology w/ SR-IOV capability disabled. > If SR-IOV is supported why would user use this framework? SR-IOV would give better performance. ... >> + >> +static struct mdev_device *__find_mdev_device(struct parent_device *parent, >> + uuid_le uuid) > > parent_find_mdev_device? > This function search for mdev device with given UUID, so I think its consistent what we have below for parent, __find_parent_device(). ... >> +static int mdev_device_remove_ops(struct mdev_device *mdev, bool force_remove) >> +{ > > > Can you add some comment here about when force_remove may be expected > here, which would help others understand immediately instead of walking through > the whole patch set? > mdev_device_remove_ops gets called from sysfs's 'remove' and when parent device is being unregistered from mdev device framework. - 'force_remove' is set to 'false' when called from sysfs's 'remove' which indicates that if the mdev device is active, used by VMM or userspace application, vendor driver could return error then don't remove the device. - 'force_remove' is set to 'true' when called from mdev_unregister_device() which indicate that parent device is being removed from mdev device framework so remove mdev device forcefully. > >> + struct parent_device *parent = mdev->parent; >> + int ret; >> + >> + /* >> + * Vendor driver can return error if VMM or userspace application is >> + * using this mdev device. >> + */ >> + ret = parent->ops->remove(mdev); > > what about passing force_remove flag to remove callback, so vendor driver > can decide whether any force cleanup required? > 'remove' getting called from sysfs is asynchronous, so vendor driver can retrun failure in that case if vendor driver finds that mdev device is being actively used. mdev_unregister_device() is going to be called from vendor driver itself when device is being unbound or driver is being unloaded. In this case vendor driver can identify itself that its in its own teardown path. So I feel there is no need to pass force_remove flag to 'remove' callback. >> +int mdev_create_sysfs_files(struct device *dev, struct mdev_type *type) >> +{ >> + int ret; >> + >> + ret = sysfs_create_files(&dev->kobj, mdev_device_attrs); >> + if (ret) { >> + pr_err("Failed to create remove sysfs entry\n"); >> + return ret; >> + } >> + >> + ret = sysfs_create_link(type->devices_kobj, &dev->kobj, dev_name(dev)); >> + if (ret) { >> + pr_err("Failed to create symlink in types\n"); > > looks wrong place... > No, this is correct. Above function creates symlink in mdev_supported_types/<type>/devices directory. >> + goto device_link_failed; >> + } >> + >> + ret = sysfs_create_link(&dev->kobj, &type->kobj, "mdev_type"); >> + if (ret) { >> + pr_err("Failed to create symlink in device directory\n"); > > exchange with above. > Again this is also correct. Above creates 'mdev_type' symlink in mdev device's directory. Although these are correct, removing these error prints. You can find it from its return type or if sysfs functions through warnings. Kirti.
next prev parent reply other threads:[~2016-10-26 14:58 UTC|newest] Thread overview: 148+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-10-17 21:22 [PATCH v9 00/12] Add Mediated device support Kirti Wankhede 2016-10-17 21:22 ` [Qemu-devel] " Kirti Wankhede 2016-10-17 21:22 ` [PATCH v9 01/12] vfio: Mediated device Core driver Kirti Wankhede 2016-10-17 21:22 ` [Qemu-devel] " Kirti Wankhede 2016-10-18 23:16 ` Alex Williamson 2016-10-18 23:16 ` [Qemu-devel] " Alex Williamson 2016-10-19 19:16 ` Kirti Wankhede 2016-10-19 19:16 ` [Qemu-devel] " Kirti Wankhede 2016-10-19 22:20 ` Alex Williamson 2016-10-19 22:20 ` [Qemu-devel] " Alex Williamson 2016-10-19 22:20 ` Alex Williamson 2016-10-20 7:23 ` Jike Song 2016-10-20 7:23 ` [Qemu-devel] " Jike Song 2016-10-20 17:12 ` Alex Williamson 2016-10-20 17:12 ` [Qemu-devel] " Alex Williamson 2016-10-21 2:41 ` Jike Song 2016-10-21 2:41 ` [Qemu-devel] " Jike Song 2016-10-27 5:56 ` Jike Song 2016-10-27 5:56 ` [Qemu-devel] " Jike Song 2016-10-26 6:52 ` Tian, Kevin 2016-10-26 6:52 ` [Qemu-devel] " Tian, Kevin 2016-10-26 14:58 ` Kirti Wankhede [this message] 2016-10-26 14:58 ` Kirti Wankhede 2016-10-17 21:22 ` [PATCH v9 02/12] vfio: VFIO based driver for Mediated devices Kirti Wankhede 2016-10-17 21:22 ` [Qemu-devel] " Kirti Wankhede 2016-10-26 6:57 ` Tian, Kevin 2016-10-26 6:57 ` [Qemu-devel] " Tian, Kevin 2016-10-26 15:01 ` Kirti Wankhede 2016-10-26 15:01 ` [Qemu-devel] " Kirti Wankhede 2016-10-17 21:22 ` [PATCH v9 03/12] vfio: Rearrange functions to get vfio_group from dev Kirti Wankhede 2016-10-17 21:22 ` [Qemu-devel] " Kirti Wankhede 2016-10-19 17:26 ` Alex Williamson 2016-10-19 17:26 ` [Qemu-devel] " Alex Williamson 2016-10-17 21:22 ` [PATCH v9 04/12] vfio iommu: Add support for mediated devices Kirti Wankhede 2016-10-17 21:22 ` [Qemu-devel] " Kirti Wankhede 2016-10-17 21:22 ` Kirti Wankhede 2016-10-19 21:02 ` Alex Williamson 2016-10-19 21:02 ` [Qemu-devel] " Alex Williamson 2016-10-20 20:17 ` Kirti Wankhede 2016-10-20 20:17 ` [Qemu-devel] " Kirti Wankhede 2016-10-24 2:32 ` Alex Williamson 2016-10-24 2:32 ` [Qemu-devel] " Alex Williamson 2016-10-26 7:19 ` Tian, Kevin 2016-10-26 7:19 ` [Qemu-devel] " Tian, Kevin 2016-10-26 15:06 ` Kirti Wankhede 2016-10-26 15:06 ` [Qemu-devel] " Kirti Wankhede 2016-10-26 7:53 ` Tian, Kevin 2016-10-26 7:53 ` [Qemu-devel] " Tian, Kevin 2016-10-26 15:16 ` Alex Williamson 2016-10-26 15:16 ` [Qemu-devel] " Alex Williamson 2016-10-26 7:54 ` Tian, Kevin 2016-10-26 7:54 ` [Qemu-devel] " Tian, Kevin 2016-10-26 15:19 ` Alex Williamson 2016-10-26 15:19 ` [Qemu-devel] " Alex Williamson 2016-10-21 7:49 ` Jike Song 2016-10-21 7:49 ` [Qemu-devel] " Jike Song 2016-10-21 14:36 ` Alex Williamson 2016-10-21 14:36 ` [Qemu-devel] " Alex Williamson 2016-10-24 10:35 ` Kirti Wankhede 2016-10-24 10:35 ` [Qemu-devel] " Kirti Wankhede 2016-10-27 7:20 ` Alexey Kardashevskiy 2016-10-27 12:31 ` Kirti Wankhede 2016-10-27 12:31 ` Kirti Wankhede 2016-10-27 12:31 ` Kirti Wankhede 2016-10-27 14:30 ` [Qemu-devel] " Alex Williamson 2016-10-27 14:30 ` Alex Williamson 2016-10-27 14:30 ` Alex Williamson 2016-10-27 15:59 ` [Qemu-devel] " Kirti Wankhede 2016-10-27 15:59 ` Kirti Wankhede 2016-10-28 2:18 ` Alexey Kardashevskiy 2016-11-01 14:01 ` Kirti Wankhede 2016-11-01 14:01 ` Kirti Wankhede 2016-11-02 1:24 ` Alexey Kardashevskiy 2016-11-02 3:29 ` Kirti Wankhede 2016-11-02 3:29 ` Kirti Wankhede 2016-11-02 4:09 ` Alexey Kardashevskiy 2016-11-02 12:21 ` Jike Song 2016-11-02 12:21 ` Jike Song 2016-11-02 12:41 ` [Qemu-devel] " Kirti Wankhede 2016-11-02 12:41 ` Kirti Wankhede 2016-11-02 13:00 ` Jike Song 2016-11-02 13:18 ` Kirti Wankhede 2016-11-02 13:18 ` Kirti Wankhede 2016-11-02 13:35 ` Jike Song 2016-11-02 13:35 ` Jike Song 2016-11-03 4:29 ` [Qemu-devel] " Alexey Kardashevskiy 2016-11-03 4:29 ` Alexey Kardashevskiy 2016-10-17 21:22 ` [PATCH v9 05/12] vfio: Introduce common function to add capabilities Kirti Wankhede 2016-10-17 21:22 ` [Qemu-devel] " Kirti Wankhede 2016-10-20 19:24 ` Alex Williamson 2016-10-20 19:24 ` [Qemu-devel] " Alex Williamson 2016-10-24 21:27 ` Kirti Wankhede 2016-10-24 21:27 ` [Qemu-devel] " Kirti Wankhede 2016-10-24 21:39 ` Alex Williamson 2016-10-24 21:39 ` [Qemu-devel] " Alex Williamson 2016-10-17 21:22 ` [PATCH v9 06/12] vfio_pci: Update vfio_pci to use vfio_info_add_capability() Kirti Wankhede 2016-10-17 21:22 ` [Qemu-devel] " Kirti Wankhede 2016-10-20 19:24 ` Alex Williamson 2016-10-20 19:24 ` [Qemu-devel] " Alex Williamson 2016-10-24 21:22 ` Kirti Wankhede 2016-10-24 21:22 ` [Qemu-devel] " Kirti Wankhede 2016-10-24 21:37 ` Alex Williamson 2016-10-24 21:37 ` [Qemu-devel] " Alex Williamson 2016-10-17 21:22 ` [PATCH v9 07/12] vfio: Introduce vfio_set_irqs_validate_and_prepare() Kirti Wankhede 2016-10-17 21:22 ` [Qemu-devel] " Kirti Wankhede 2016-10-17 21:22 ` [PATCH v9 08/12] vfio_pci: Updated to use vfio_set_irqs_validate_and_prepare() Kirti Wankhede 2016-10-17 21:22 ` [Qemu-devel] " Kirti Wankhede 2016-10-17 21:22 ` [PATCH v9 09/12] vfio_platform: " Kirti Wankhede 2016-10-17 21:22 ` [Qemu-devel] " Kirti Wankhede 2016-10-17 21:22 ` [PATCH v9 10/12] vfio: Add function to get device_api string from vfio_device_info.flags Kirti Wankhede 2016-10-17 21:22 ` [Qemu-devel] " Kirti Wankhede 2016-10-20 19:34 ` Alex Williamson 2016-10-20 19:34 ` [Qemu-devel] " Alex Williamson 2016-10-20 20:29 ` Kirti Wankhede 2016-10-20 20:29 ` [Qemu-devel] " Kirti Wankhede 2016-10-20 21:05 ` Alex Williamson 2016-10-20 21:05 ` [Qemu-devel] " Alex Williamson 2016-10-20 21:14 ` Kirti Wankhede 2016-10-20 21:14 ` [Qemu-devel] " Kirti Wankhede 2016-10-20 21:22 ` Alex Williamson 2016-10-20 21:22 ` [Qemu-devel] " Alex Williamson 2016-10-21 3:00 ` Kirti Wankhede 2016-10-21 3:00 ` [Qemu-devel] " Kirti Wankhede 2016-10-21 3:20 ` Alex Williamson 2016-10-21 3:20 ` [Qemu-devel] " Alex Williamson 2016-10-17 21:22 ` [PATCH v9 11/12] docs: Add Documentation for Mediated devices Kirti Wankhede 2016-10-17 21:22 ` [Qemu-devel] " Kirti Wankhede 2016-10-25 16:17 ` Alex Williamson 2016-10-25 16:17 ` [Qemu-devel] " Alex Williamson 2016-10-17 21:22 ` [PATCH v9 12/12] docs: Sample driver to demonstrate how to use Mediated device framework Kirti Wankhede 2016-10-17 21:22 ` [Qemu-devel] " Kirti Wankhede 2016-10-18 2:54 ` Dong Jia Shi 2016-10-18 17:17 ` Alex Williamson 2016-10-18 17:17 ` [Qemu-devel] " Alex Williamson 2016-10-19 19:19 ` Kirti Wankhede 2016-10-19 19:19 ` [Qemu-devel] " Kirti Wankhede 2016-10-18 2:54 ` Dong Jia Shi 2016-10-17 21:41 ` [PATCH v9 00/12] Add Mediated device support Alex Williamson 2016-10-17 21:41 ` [Qemu-devel] " Alex Williamson 2016-10-24 7:07 ` Jike Song 2016-10-24 7:07 ` [Qemu-devel] " Jike Song 2016-12-05 17:44 ` Gerd Hoffmann 2016-12-05 17:44 ` [Qemu-devel] " Gerd Hoffmann 2016-12-05 17:44 ` Gerd Hoffmann 2016-12-06 2:24 ` Jike Song 2016-12-06 2:24 ` [Qemu-devel] " Jike Song 2016-12-07 14:40 ` Gerd Hoffmann 2016-12-07 14:40 ` [Qemu-devel] " Gerd Hoffmann
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=813a9246-f3cc-7f53-08a6-6a0673a273e6@nvidia.com \ --to=kwankhede@nvidia.com \ --cc=alex.williamson@redhat.com \ --cc=bjsdjshi@linux.vnet.ibm.com \ --cc=cjia@nvidia.com \ --cc=jike.song@intel.com \ --cc=kevin.tian@intel.com \ --cc=kraxel@redhat.com \ --cc=kvm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=pbonzini@redhat.com \ --cc=qemu-devel@nongnu.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.