From: Alex Williamson <alex.williamson@redhat.com> To: Kirti Wankhede <kwankhede@nvidia.com> Cc: shuai.ruan@intel.com, kevin.tian@intel.com, cjia@nvidia.com, kvm@vger.kernel.org, qemu-devel@nongnu.org, jike.song@intel.com, kraxel@redhat.com, pbonzini@redhat.com, bjsdjshi@linux.vnet.ibm.com, zhiyuan.lv@intel.com Subject: Re: [RFC PATCH v4 1/3] Mediated device Core driver Date: Thu, 26 May 2016 08:06:07 -0600 [thread overview] Message-ID: <20160526080607.58281d58@ul30vt.home> (raw) In-Reply-To: <c736badf-274c-f111-06d9-c1a9446991b3@nvidia.com> On Thu, 26 May 2016 14:33:39 +0530 Kirti Wankhede <kwankhede@nvidia.com> wrote: > Thanks Alex. > > I'll consider all the nits and fix those in next version of patch. > > More below: > > On 5/26/2016 4:09 AM, Alex Williamson wrote: > > On Wed, 25 May 2016 01:28:15 +0530 > > Kirti Wankhede <kwankhede@nvidia.com> wrote: > > > > ... > > >> + > >> +config MDEV > >> + tristate "Mediated device driver framework" > >> + depends on VFIO > >> + default n > >> + help > >> + MDEV provides a framework to virtualize device without > SR-IOV cap > >> + See Documentation/mdev.txt for more details. > > > > I don't see that file anywhere in this series. > > Yes, missed this file in this patch. I'll add it in next version of patch. > Since mdev module is moved in vfio directory, should I place this file > in vfio directory, Documentation/vfio/mdev.txt? or keep documentation of > mdev module within vfio.txt itself? Maybe just call it vfio-mediated-device.txt > >> + if (phy_dev) { > >> + mutex_lock(&phy_devices.list_lock); > >> + > >> + /* > >> + * If vendor driver doesn't return success that means vendor > >> + * driver doesn't support hot-unplug > >> + */ > >> + if (phy_dev->ops->destroy) { > >> + if (phy_dev->ops->destroy(phy_dev->dev, mdevice->uuid, > >> + mdevice->instance)) { > >> + mutex_unlock(&phy_devices.list_lock); > >> + return; > >> + } > >> + } > >> + > >> + mdev_remove_attribute_group(&mdevice->dev, > >> + phy_dev->ops->mdev_attr_groups); > >> + mdevice->phy_dev = NULL; > >> + mutex_unlock(&phy_devices.list_lock); > > > > Locking here appears arbitrary, how does the above code interact with > > phy_devices.dev_list? > > > > Sorry for not being clear about phy_devices.list_lock, probably I > shouldn't have named it 'list_lock'. This lock is also to synchronize > register_device & unregister_device and physical device specific > callbacks: supported_config, create, destroy, start and shutdown. > Although supported_config, create and destroy are per phy_device > specific callbacks while start and shutdown could refer to multiple > phy_devices indirectly when there are multiple mdev devices of same type > on different physical devices. There could be race condition in start > callback and destroy & unregister_device. I'm revisiting this lock again > and will see to use per phy device lock for phy_device specific callbacks. > > > >> +struct mdev_device { > >> + struct kref kref; > >> + struct device dev; > >> + struct phy_device *phy_dev; > >> + struct iommu_group *group; > >> + void *iommu_data; > >> + uuid_le uuid; > >> + uint32_t instance; > >> + void *driver_data; > >> + struct mutex ops_lock; > >> + struct list_head next; > >> +}; > > > > Could this be in the private header? Seems like this should be opaque > > outside of mdev core. > > > > No, this structure is used in mediated device call back functions to > vendor driver so that vendor driver could identify mdev device, similar > to pci_dev structure in pci bus subsystem. (I'll remove kref which is > not being used at all.) Personally I'd prefer to see more use of reference counting and less locking, especially since the locking is mostly ineffective in this version. > >> + * @read: Read emulation callback > >> + * @mdev: mediated device structure > >> + * @buf: read buffer > >> + * @count: number bytes to read > >> + * @address_space: specifies for which address > >> + * space the request is: pci_config_space, IO > >> + * register space or MMIO space. > > > > Seems like I asked before and it's no more clear in the code, how do we > > handle multiple spaces for various types? ie. a device might have > > multiple MMIO spaces. > > > >> + * @pos: offset from base address. > > Sorry, updated the code but missed to update comment here. > pos = base_address + offset > (its not 'pos' anymore, will rename it to addr) > > so vendor driver is aware about base addresses of multiple MMIO spaces > and its size, they can identify MMIO space based on addr. Why not let the vendor driver provide vfio_region_info directly, including the offset within the device file descriptor thedn the mediated device core simply pass read/write through without caring what the address space is? Thanks, Alex > >> +/* > >> + * Physical Device > >> + */ > >> +struct phy_device { > >> + struct device *dev; > >> + const struct phy_device_ops *ops; > >> + struct list_head next; > >> +}; > > > > I would really like to be able to use the mediated device interface to > > create a purely virtual device, is the expectation that my physical > > device interface would create a virtual struct device which would > > become the parent and control point in sysfs for creating all the mdev > > devices? Should we be calling this a host_device or mdev_parent_dev in > > that case since there's really no requirement that it be a physical > > device? > > Makes sense. I'll rename it to parent_device. > > Thanks, > Kirti. >
WARNING: multiple messages have this Message-ID (diff)
From: Alex Williamson <alex.williamson@redhat.com> To: Kirti Wankhede <kwankhede@nvidia.com> Cc: pbonzini@redhat.com, kraxel@redhat.com, cjia@nvidia.com, qemu-devel@nongnu.org, kvm@vger.kernel.org, kevin.tian@intel.com, shuai.ruan@intel.com, jike.song@intel.com, zhiyuan.lv@intel.com, bjsdjshi@linux.vnet.ibm.com Subject: Re: [Qemu-devel] [RFC PATCH v4 1/3] Mediated device Core driver Date: Thu, 26 May 2016 08:06:07 -0600 [thread overview] Message-ID: <20160526080607.58281d58@ul30vt.home> (raw) In-Reply-To: <c736badf-274c-f111-06d9-c1a9446991b3@nvidia.com> On Thu, 26 May 2016 14:33:39 +0530 Kirti Wankhede <kwankhede@nvidia.com> wrote: > Thanks Alex. > > I'll consider all the nits and fix those in next version of patch. > > More below: > > On 5/26/2016 4:09 AM, Alex Williamson wrote: > > On Wed, 25 May 2016 01:28:15 +0530 > > Kirti Wankhede <kwankhede@nvidia.com> wrote: > > > > ... > > >> + > >> +config MDEV > >> + tristate "Mediated device driver framework" > >> + depends on VFIO > >> + default n > >> + help > >> + MDEV provides a framework to virtualize device without > SR-IOV cap > >> + See Documentation/mdev.txt for more details. > > > > I don't see that file anywhere in this series. > > Yes, missed this file in this patch. I'll add it in next version of patch. > Since mdev module is moved in vfio directory, should I place this file > in vfio directory, Documentation/vfio/mdev.txt? or keep documentation of > mdev module within vfio.txt itself? Maybe just call it vfio-mediated-device.txt > >> + if (phy_dev) { > >> + mutex_lock(&phy_devices.list_lock); > >> + > >> + /* > >> + * If vendor driver doesn't return success that means vendor > >> + * driver doesn't support hot-unplug > >> + */ > >> + if (phy_dev->ops->destroy) { > >> + if (phy_dev->ops->destroy(phy_dev->dev, mdevice->uuid, > >> + mdevice->instance)) { > >> + mutex_unlock(&phy_devices.list_lock); > >> + return; > >> + } > >> + } > >> + > >> + mdev_remove_attribute_group(&mdevice->dev, > >> + phy_dev->ops->mdev_attr_groups); > >> + mdevice->phy_dev = NULL; > >> + mutex_unlock(&phy_devices.list_lock); > > > > Locking here appears arbitrary, how does the above code interact with > > phy_devices.dev_list? > > > > Sorry for not being clear about phy_devices.list_lock, probably I > shouldn't have named it 'list_lock'. This lock is also to synchronize > register_device & unregister_device and physical device specific > callbacks: supported_config, create, destroy, start and shutdown. > Although supported_config, create and destroy are per phy_device > specific callbacks while start and shutdown could refer to multiple > phy_devices indirectly when there are multiple mdev devices of same type > on different physical devices. There could be race condition in start > callback and destroy & unregister_device. I'm revisiting this lock again > and will see to use per phy device lock for phy_device specific callbacks. > > > >> +struct mdev_device { > >> + struct kref kref; > >> + struct device dev; > >> + struct phy_device *phy_dev; > >> + struct iommu_group *group; > >> + void *iommu_data; > >> + uuid_le uuid; > >> + uint32_t instance; > >> + void *driver_data; > >> + struct mutex ops_lock; > >> + struct list_head next; > >> +}; > > > > Could this be in the private header? Seems like this should be opaque > > outside of mdev core. > > > > No, this structure is used in mediated device call back functions to > vendor driver so that vendor driver could identify mdev device, similar > to pci_dev structure in pci bus subsystem. (I'll remove kref which is > not being used at all.) Personally I'd prefer to see more use of reference counting and less locking, especially since the locking is mostly ineffective in this version. > >> + * @read: Read emulation callback > >> + * @mdev: mediated device structure > >> + * @buf: read buffer > >> + * @count: number bytes to read > >> + * @address_space: specifies for which address > >> + * space the request is: pci_config_space, IO > >> + * register space or MMIO space. > > > > Seems like I asked before and it's no more clear in the code, how do we > > handle multiple spaces for various types? ie. a device might have > > multiple MMIO spaces. > > > >> + * @pos: offset from base address. > > Sorry, updated the code but missed to update comment here. > pos = base_address + offset > (its not 'pos' anymore, will rename it to addr) > > so vendor driver is aware about base addresses of multiple MMIO spaces > and its size, they can identify MMIO space based on addr. Why not let the vendor driver provide vfio_region_info directly, including the offset within the device file descriptor thedn the mediated device core simply pass read/write through without caring what the address space is? Thanks, Alex > >> +/* > >> + * Physical Device > >> + */ > >> +struct phy_device { > >> + struct device *dev; > >> + const struct phy_device_ops *ops; > >> + struct list_head next; > >> +}; > > > > I would really like to be able to use the mediated device interface to > > create a purely virtual device, is the expectation that my physical > > device interface would create a virtual struct device which would > > become the parent and control point in sysfs for creating all the mdev > > devices? Should we be calling this a host_device or mdev_parent_dev in > > that case since there's really no requirement that it be a physical > > device? > > Makes sense. I'll rename it to parent_device. > > Thanks, > Kirti. >
next prev parent reply other threads:[~2016-05-26 14:06 UTC|newest] Thread overview: 92+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-05-24 19:58 [RFC PATCH v4 0/3] Add Mediated device support[was: Add vGPU support] Kirti Wankhede 2016-05-24 19:58 ` [Qemu-devel] " Kirti Wankhede 2016-05-24 19:58 ` [RFC PATCH v4 1/3] Mediated device Core driver Kirti Wankhede 2016-05-24 19:58 ` [Qemu-devel] " Kirti Wankhede 2016-05-25 7:55 ` Tian, Kevin 2016-05-25 7:55 ` [Qemu-devel] " Tian, Kevin 2016-05-25 14:47 ` Kirti Wankhede 2016-05-25 14:47 ` [Qemu-devel] " Kirti Wankhede 2016-05-27 9:00 ` Tian, Kevin 2016-05-27 9:00 ` [Qemu-devel] " Tian, Kevin 2016-05-25 22:39 ` Alex Williamson 2016-05-25 22:39 ` [Qemu-devel] " Alex Williamson 2016-05-26 9:03 ` Kirti Wankhede 2016-05-26 9:03 ` [Qemu-devel] " Kirti Wankhede 2016-05-26 14:06 ` Alex Williamson [this message] 2016-05-26 14:06 ` Alex Williamson 2016-06-03 8:57 ` Dong Jia 2016-06-03 8:57 ` [Qemu-devel] " Dong Jia 2016-06-03 9:40 ` Tian, Kevin 2016-06-03 9:40 ` [Qemu-devel] " Tian, Kevin 2016-06-06 2:24 ` Dong Jia 2016-06-06 2:24 ` [Qemu-devel] " Dong Jia 2016-06-06 5:27 ` Kirti Wankhede 2016-06-06 5:27 ` [Qemu-devel] " Kirti Wankhede 2016-06-06 6:01 ` Dong Jia 2016-06-06 6:01 ` [Qemu-devel] " Dong Jia 2016-06-06 6:27 ` Neo Jia 2016-06-06 6:27 ` [Qemu-devel] " Neo Jia 2016-06-06 8:29 ` Dong Jia 2016-06-06 8:29 ` [Qemu-devel] " Dong Jia 2016-06-06 17:44 ` Neo Jia 2016-06-06 17:44 ` [Qemu-devel] " Neo Jia 2016-06-06 19:31 ` Alex Williamson 2016-06-06 19:31 ` [Qemu-devel] " Alex Williamson 2016-06-07 3:03 ` Tian, Kevin 2016-06-07 3:03 ` [Qemu-devel] " Tian, Kevin 2016-06-07 22:42 ` Alex Williamson 2016-06-07 22:42 ` [Qemu-devel] " Alex Williamson 2016-06-08 1:18 ` Tian, Kevin 2016-06-08 1:18 ` [Qemu-devel] " Tian, Kevin 2016-06-08 1:39 ` Alex Williamson 2016-06-08 1:39 ` [Qemu-devel] " Alex Williamson 2016-06-08 3:18 ` Dong Jia 2016-06-08 3:18 ` [Qemu-devel] " Dong Jia 2016-06-08 3:48 ` Neo Jia 2016-06-08 3:48 ` [Qemu-devel] " Neo Jia 2016-06-08 6:13 ` Dong Jia 2016-06-08 6:13 ` [Qemu-devel] " Dong Jia 2016-06-08 6:22 ` Neo Jia 2016-06-08 6:22 ` [Qemu-devel] " Neo Jia 2016-06-08 4:29 ` Alex Williamson 2016-06-08 4:29 ` [Qemu-devel] " Alex Williamson 2016-06-15 6:37 ` Dong Jia 2016-06-15 6:37 ` [Qemu-devel] " Dong Jia 2016-05-24 19:58 ` [RFC PATCH v4 2/3] VFIO driver for mediated PCI device Kirti Wankhede 2016-05-24 19:58 ` [Qemu-devel] " Kirti Wankhede 2016-05-25 8:15 ` Tian, Kevin 2016-05-25 8:15 ` [Qemu-devel] " Tian, Kevin 2016-05-25 13:04 ` Kirti Wankhede 2016-05-25 13:04 ` [Qemu-devel] " Kirti Wankhede 2016-05-27 10:03 ` Tian, Kevin 2016-05-27 10:03 ` [Qemu-devel] " Tian, Kevin 2016-05-27 15:13 ` Alex Williamson 2016-05-27 15:13 ` [Qemu-devel] " Alex Williamson 2016-05-24 19:58 ` [RFC PATCH v4 3/3] VFIO Type1 IOMMU: Add support for mediated devices Kirti Wankhede 2016-05-24 19:58 ` [Qemu-devel] " Kirti Wankhede 2016-06-01 8:40 ` Dong Jia 2016-06-01 8:40 ` [Qemu-devel] " Dong Jia 2016-06-02 7:56 ` Neo Jia 2016-06-02 7:56 ` [Qemu-devel] " Neo Jia 2016-06-03 8:32 ` Dong Jia 2016-06-03 8:32 ` [Qemu-devel] " Dong Jia 2016-06-03 8:37 ` Tian, Kevin 2016-06-03 8:37 ` [Qemu-devel] " Tian, Kevin 2016-05-25 7:13 ` [RFC PATCH v4 0/3] Add Mediated device support[was: Add vGPU support] Tian, Kevin 2016-05-25 7:13 ` [Qemu-devel] " Tian, Kevin 2016-05-25 13:43 ` Alex Williamson 2016-05-25 13:43 ` [Qemu-devel] " Alex Williamson 2016-05-27 11:02 ` Tian, Kevin 2016-05-27 11:02 ` [Qemu-devel] " Tian, Kevin 2016-05-27 14:54 ` Alex Williamson 2016-05-27 14:54 ` [Qemu-devel] " Alex Williamson 2016-05-27 22:43 ` Tian, Kevin 2016-05-27 22:43 ` [Qemu-devel] " Tian, Kevin 2016-05-28 14:56 ` Alex Williamson 2016-05-28 14:56 ` [Qemu-devel] " Alex Williamson 2016-05-31 2:29 ` Jike Song 2016-05-31 2:29 ` [Qemu-devel] " Jike Song 2016-05-31 14:29 ` Alex Williamson 2016-05-31 14:29 ` [Qemu-devel] " Alex Williamson 2016-06-02 2:11 ` Jike Song 2016-06-02 2:11 ` [Qemu-devel] " Jike Song
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=20160526080607.58281d58@ul30vt.home \ --to=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=kwankhede@nvidia.com \ --cc=pbonzini@redhat.com \ --cc=qemu-devel@nongnu.org \ --cc=shuai.ruan@intel.com \ --cc=zhiyuan.lv@intel.com \ /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.