From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kirti Wankhede Subject: Re: [RFC PATCH v4 1/3] Mediated device Core driver Date: Thu, 26 May 2016 14:33:39 +0530 Message-ID: References: <1464119897-10844-1-git-send-email-kwankhede@nvidia.com> <1464119897-10844-2-git-send-email-kwankhede@nvidia.com> <20160525163932.266850d4@ul30vt.home> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Cc: , , , , , , , , , To: Alex Williamson Return-path: Received: from hqemgate15.nvidia.com ([216.228.121.64]:5091 "EHLO hqemgate15.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750977AbcEZJDw (ORCPT ); Thu, 26 May 2016 05:03:52 -0400 In-Reply-To: <20160525163932.266850d4@ul30vt.home> Sender: kvm-owner@vger.kernel.org List-ID: 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 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? >> + 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.) >> + * @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. >> +/* >> + * 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. From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52531) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b5rCw-0005ok-8g for qemu-devel@nongnu.org; Thu, 26 May 2016 05:03:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b5rCr-0002aN-TS for qemu-devel@nongnu.org; Thu, 26 May 2016 05:03:58 -0400 Received: from hqemgate15.nvidia.com ([216.228.121.64]:5093) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b5rCr-0002a7-LO for qemu-devel@nongnu.org; Thu, 26 May 2016 05:03:53 -0400 References: <1464119897-10844-1-git-send-email-kwankhede@nvidia.com> <1464119897-10844-2-git-send-email-kwankhede@nvidia.com> <20160525163932.266850d4@ul30vt.home> From: Kirti Wankhede Message-ID: Date: Thu, 26 May 2016 14:33:39 +0530 MIME-Version: 1.0 In-Reply-To: <20160525163932.266850d4@ul30vt.home> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH v4 1/3] Mediated device Core driver List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson 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 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 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? >> + 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.) >> + * @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. >> +/* >> + * 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.