From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jike Song Subject: Re: [PATCH v7 1/4] vfio: Mediated device Core driver Date: Fri, 09 Sep 2016 14:26:24 +0800 Message-ID: <57D25610.8000801@intel.com> References: <1472097235-6332-1-git-send-email-kwankhede@nvidia.com> <1472097235-6332-2-git-send-email-kwankhede@nvidia.com> <57D11CC3.6010605@intel.com> <20160908093824.GA23397@nvidia.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: Kirti Wankhede , alex.williamson@redhat.com, pbonzini@redhat.com, kraxel@redhat.com, qemu-devel@nongnu.org, kvm@vger.kernel.org, kevin.tian@intel.com, bjsdjshi@linux.vnet.ibm.com To: Neo Jia Return-path: Received: from mga07.intel.com ([134.134.136.100]:29335 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750839AbcIIG2P (ORCPT ); Fri, 9 Sep 2016 02:28:15 -0400 In-Reply-To: <20160908093824.GA23397@nvidia.com> Sender: kvm-owner@vger.kernel.org List-ID: On 09/08/2016 05:38 PM, Neo Jia wrote: > On Thu, Sep 08, 2016 at 04:09:39PM +0800, Jike Song wrote: >> On 08/25/2016 11:53 AM, Kirti Wankhede wrote: >>> + >>> +/** >>> + * struct parent_ops - Structure to be registered for each parent device to >>> + * register the device to mdev module. >>> + * >>> + * @owner: The module owner. >>> + * @dev_attr_groups: Default attributes of the parent device. >>> + * @mdev_attr_groups: Default attributes of the mediated device. >>> + * @supported_config: Called to get information about supported types. >>> + * @dev : device structure of parent device. >>> + * @config: should return string listing supported config >>> + * Returns integer: success (0) or error (< 0) >>> + * @create: Called to allocate basic resources in parent device's >>> + * driver for a particular mediated device. It is >>> + * mandatory to provide create ops. >>> + * @mdev: mdev_device structure on of mediated device >>> + * that is being created >>> + * @mdev_params: extra parameters required by parent >>> + * device's driver. >>> + * Returns integer: success (0) or error (< 0) >>> + * @destroy: Called to free resources in parent device's driver for a >>> + * a mediated device. It is mandatory to provide destroy >>> + * ops. >>> + * @mdev: mdev_device device structure which is being >>> + * destroyed >>> + * Returns integer: success (0) or error (< 0) >>> + * If VMM is running and destroy() is called that means the >>> + * mdev is being hotunpluged. Return error if VMM is >>> + * running and driver doesn't support mediated device >>> + * hotplug. >>> + * @reset: Called to reset mediated device. >>> + * @mdev: mdev_device device structure. >>> + * Returns integer: success (0) or error (< 0) >>> + * @set_online_status: Called to change to status of mediated device. >>> + * @mdev: mediated device. >>> + * @online: set true or false to make mdev device online or >>> + * offline. >>> + * Returns integer: success (0) or error (< 0) >>> + * @get_online_status: Called to get online/offline status of mediated device >>> + * @mdev: mediated device. >>> + * @online: Returns status of mediated device. >>> + * Returns integer: success (0) or error (< 0) >>> + * @read: Read emulation callback >>> + * @mdev: mediated device structure >>> + * @buf: read buffer >>> + * @count: number of bytes to read >>> + * @pos: address. >>> + * Retuns number on bytes read on success or error. >>> + * @write: Write emulation callback >>> + * @mdev: mediated device structure >>> + * @buf: write buffer >>> + * @count: number of bytes to be written >>> + * @pos: address. >>> + * Retuns number on bytes written on success or error. >>> + * @get_irq_info: Called to retrieve information about mediated device IRQ >>> + * @mdev: mediated device structure >>> + * @irq_info: VFIO IRQ flags and count. >>> + * Returns integer: success (0) or error (< 0) >>> + * @set_irqs: Called to send about interrupts configuration >>> + * information that VMM sets. >>> + * @mdev: mediated device structure >>> + * @flags, index, start, count and *data : same as that of >>> + * struct vfio_irq_set of VFIO_DEVICE_SET_IRQS API. >>> + * @get_device_info: Called to get VFIO device information for a mediated >>> + * device. >>> + * @vfio_device_info: VFIO device info. >>> + * Returns integer: success (0) or error (< 0) >>> + * @get_region_info: Called to get VFIO region size and flags of mediated >>> + * device. >>> + * @mdev: mediated device structure >>> + * @region_info: output, returns size and flags of >>> + * requested region. >>> + * @cap_type_id: returns id of capability. >>> + * @cap_type: returns pointer to capability structure >>> + * corresponding to capability id. >>> + * Returns integer: success (0) or error (< 0) >>> + * >>> + * Parent device that support mediated device should be registered with mdev >>> + * module with parent_ops structure. >>> + */ >>> + >>> +struct parent_ops { >>> + struct module *owner; >>> + const struct attribute_group **dev_attr_groups; >>> + const struct attribute_group **mdev_attr_groups; >>> + >>> + int (*supported_config)(struct device *dev, char *config); >>> + int (*create)(struct mdev_device *mdev, char *mdev_params); >>> + int (*destroy)(struct mdev_device *mdev); >>> + int (*reset)(struct mdev_device *mdev); >>> + int (*set_online_status)(struct mdev_device *mdev, bool online); >>> + int (*get_online_status)(struct mdev_device *mdev, bool *online); >>> + ssize_t (*read)(struct mdev_device *mdev, char *buf, size_t count, >>> + loff_t pos); >>> + ssize_t (*write)(struct mdev_device *mdev, char *buf, size_t count, >>> + loff_t pos); >>> + int (*mmap)(struct mdev_device *mdev, struct vm_area_struct *vma); >>> + int (*get_irq_info)(struct mdev_device *mdev, >>> + struct vfio_irq_info *irq_info); >>> + int (*set_irqs)(struct mdev_device *mdev, uint32_t flags, >>> + unsigned int index, unsigned int start, >>> + unsigned int count, void *data); >>> + int (*get_device_info)(struct mdev_device *mdev, >>> + struct vfio_device_info *dev_info); >>> + int (*get_region_info)(struct mdev_device *mdev, >>> + struct vfio_region_info *region_info, >>> + u16 *cap_type_id, void **cap_type); >>> +}; >> >> I have a strong objection here to such low-level interfaces, the interfaces >> between vfio-mdev and vendor drivers should be as thin as possible, not imposing >> any limitation to vendor drivers. > > Hi Jike, > > Welcome! :-) Aha, thanks! :) > > Unfortunately, this is something I definitely can't agree with you. > Glad to see your opinion! > We would like to capture the common code as much as possible without losing > flexibilities, so each vendor driver writers won't have to duplicate them and we > have something can be maintained publicly. > Yeah it is good to reduce the duplications among different vendor drivers, but what do you think about the duplication between here and other bus drivers like vfio-pci? > If you are running into specific limitation with above callback interfaces, > please show us the scenarios and we are very happy to look into that. > Though we don't actually test it upon this series (using high-level implementations instead), I personally don't think there is a problem. However, this doesn't necessarily mean it's sufficient. >> >> I saw that validate_map_request was removed from the ops and mmap was added. >> That is pretty nice. Furthermore, if you add an ioctl here, you can also remove >> get_device_info, get_irq_info, set_irqs, and get_region_info (and even "reset"). >> There are several benefits by doing this: > > The decision of moving validate_map_request is mainly because we are adding a lot of > advanced logic which most vendor drivers don't require, since we are the only consumer > of such logic, no need to put it in the public/shared module. > >> >> - Balanced interfaces. >> Like I replied in another mail, you won't have unbalanced interfaces. >> You already have read, write and mmap in the ops, why not ioctl? > > Sorry, don't think "balanced" interface is a design criteria especially when > simply pursuing the sake of "balanced or full-set" interface ends up lots > duplicated code for vendor driver writers. > Please kindly have a look at my comment on patch 2/4, about how to check the validity of "count". >> >> - Scalability. >> You are intercepting vfio optional capabilities in the framework, but >> how if vfio.ko, or vfio-pci.ko add a few new capabilities in the future? > > Exactly my point about the code sharing. > > If new cap is added inside vfio.ko or vfio-pci.ko, we can just add it into > vfio_mdev.ko. > > Adding the code in one place is better to duplicate in multiple vendor drivers. So after adding that, how many places will you have? >> >> - Abstraction. >> Even placing common codes here can avoid code duplication, you still >> have code duplicate with vfio-pci. Better to move common logic out of >> vfio-pci and call them from mdev vendor drivers. > > Are you saying to avoid the code duplications between vfio-pci and vfio-mdev? > Exactly. I haven't check other bus-driver like vfio-platform, but even if only having vfio-pci considered, there will be duplications. >> >> - Maintainability. >> This is pretty obvious :) > > Definitely not, the burden is moving to the vendor driver side. > Moving to vendor side is not the target, as said above, this will probably cause more abstraction and refactoring of existing vfio code. > Again, Jike, I really want to enable you with the mediated framework we have been > doing here. So it is probably easier for us to accommodate your need if you could > follow the interfaces we have introduced and let us know if you have any specific > issues. I won't read this as that one is not welcome to comment as long as he met no actual issue :) -- Thanks, Jike >>> + >>> +/* >>> + * Parent Device >>> + */ >>> + >>> +struct parent_device { >>> + struct device *dev; >>> + const struct parent_ops *ops; >>> + >>> + /* internal */ >>> + struct kref ref; >>> + struct list_head next; >>> + struct list_head mdev_list; >>> + struct mutex mdev_list_lock; >>> + wait_queue_head_t release_done; >>> +}; >>> + >>> +/** >>> + * struct mdev_driver - Mediated device driver >>> + * @name: driver name >>> + * @probe: called when new device created >>> + * @remove: called when device removed >>> + * @driver: device driver structure >>> + * >>> + **/ >>> +struct mdev_driver { >>> + const char *name; >>> + int (*probe)(struct device *dev); >>> + void (*remove)(struct device *dev); >>> + struct device_driver driver; >>> +}; >>> + >>> +static inline struct mdev_driver *to_mdev_driver(struct device_driver *drv) >>> +{ >>> + return drv ? container_of(drv, struct mdev_driver, driver) : NULL; >>> +} >>> + >>> +static inline struct mdev_device *to_mdev_device(struct device *dev) >>> +{ >>> + return dev ? container_of(dev, struct mdev_device, dev) : NULL; >>> +} >> >> These can be macros, like pci ones. >> >>> + >>> +static inline void *mdev_get_drvdata(struct mdev_device *mdev) >>> +{ >>> + return mdev->driver_data; >>> +} >>> + >>> +static inline void mdev_set_drvdata(struct mdev_device *mdev, void *data) >>> +{ >>> + mdev->driver_data = data; >>> +} >>> + >>> +extern struct bus_type mdev_bus_type; >>> + >>> +#define dev_is_mdev(d) ((d)->bus == &mdev_bus_type) >>> + >>> +extern int mdev_register_device(struct device *dev, >>> + const struct parent_ops *ops); >>> +extern void mdev_unregister_device(struct device *dev); >>> + >>> +extern int mdev_register_driver(struct mdev_driver *drv, struct module *owner); >>> +extern void mdev_unregister_driver(struct mdev_driver *drv); >>> + >>> +extern struct mdev_device *mdev_get_device(struct mdev_device *mdev); >>> +extern void mdev_put_device(struct mdev_device *mdev); >>> + >>> +extern struct mdev_device *mdev_get_device_by_group(struct iommu_group *group); >>> + >>> +#endif /* MDEV_H */ >>> >> >> -- >> Thanks, >> Jike From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58890) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1biFIQ-0002cN-J0 for qemu-devel@nongnu.org; Fri, 09 Sep 2016 02:28:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1biFIL-0005tP-Hl for qemu-devel@nongnu.org; Fri, 09 Sep 2016 02:28:18 -0400 Received: from mga05.intel.com ([192.55.52.43]:31593) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1biFIL-0005sa-5Q for qemu-devel@nongnu.org; Fri, 09 Sep 2016 02:28:13 -0400 Message-ID: <57D25610.8000801@intel.com> Date: Fri, 09 Sep 2016 14:26:24 +0800 From: Jike Song MIME-Version: 1.0 References: <1472097235-6332-1-git-send-email-kwankhede@nvidia.com> <1472097235-6332-2-git-send-email-kwankhede@nvidia.com> <57D11CC3.6010605@intel.com> <20160908093824.GA23397@nvidia.com> In-Reply-To: <20160908093824.GA23397@nvidia.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v7 1/4] vfio: Mediated device Core driver List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Neo Jia Cc: Kirti Wankhede , alex.williamson@redhat.com, pbonzini@redhat.com, kraxel@redhat.com, qemu-devel@nongnu.org, kvm@vger.kernel.org, kevin.tian@intel.com, bjsdjshi@linux.vnet.ibm.com On 09/08/2016 05:38 PM, Neo Jia wrote: > On Thu, Sep 08, 2016 at 04:09:39PM +0800, Jike Song wrote: >> On 08/25/2016 11:53 AM, Kirti Wankhede wrote: >>> + >>> +/** >>> + * struct parent_ops - Structure to be registered for each parent device to >>> + * register the device to mdev module. >>> + * >>> + * @owner: The module owner. >>> + * @dev_attr_groups: Default attributes of the parent device. >>> + * @mdev_attr_groups: Default attributes of the mediated device. >>> + * @supported_config: Called to get information about supported types. >>> + * @dev : device structure of parent device. >>> + * @config: should return string listing supported config >>> + * Returns integer: success (0) or error (< 0) >>> + * @create: Called to allocate basic resources in parent device's >>> + * driver for a particular mediated device. It is >>> + * mandatory to provide create ops. >>> + * @mdev: mdev_device structure on of mediated device >>> + * that is being created >>> + * @mdev_params: extra parameters required by parent >>> + * device's driver. >>> + * Returns integer: success (0) or error (< 0) >>> + * @destroy: Called to free resources in parent device's driver for a >>> + * a mediated device. It is mandatory to provide destroy >>> + * ops. >>> + * @mdev: mdev_device device structure which is being >>> + * destroyed >>> + * Returns integer: success (0) or error (< 0) >>> + * If VMM is running and destroy() is called that means the >>> + * mdev is being hotunpluged. Return error if VMM is >>> + * running and driver doesn't support mediated device >>> + * hotplug. >>> + * @reset: Called to reset mediated device. >>> + * @mdev: mdev_device device structure. >>> + * Returns integer: success (0) or error (< 0) >>> + * @set_online_status: Called to change to status of mediated device. >>> + * @mdev: mediated device. >>> + * @online: set true or false to make mdev device online or >>> + * offline. >>> + * Returns integer: success (0) or error (< 0) >>> + * @get_online_status: Called to get online/offline status of mediated device >>> + * @mdev: mediated device. >>> + * @online: Returns status of mediated device. >>> + * Returns integer: success (0) or error (< 0) >>> + * @read: Read emulation callback >>> + * @mdev: mediated device structure >>> + * @buf: read buffer >>> + * @count: number of bytes to read >>> + * @pos: address. >>> + * Retuns number on bytes read on success or error. >>> + * @write: Write emulation callback >>> + * @mdev: mediated device structure >>> + * @buf: write buffer >>> + * @count: number of bytes to be written >>> + * @pos: address. >>> + * Retuns number on bytes written on success or error. >>> + * @get_irq_info: Called to retrieve information about mediated device IRQ >>> + * @mdev: mediated device structure >>> + * @irq_info: VFIO IRQ flags and count. >>> + * Returns integer: success (0) or error (< 0) >>> + * @set_irqs: Called to send about interrupts configuration >>> + * information that VMM sets. >>> + * @mdev: mediated device structure >>> + * @flags, index, start, count and *data : same as that of >>> + * struct vfio_irq_set of VFIO_DEVICE_SET_IRQS API. >>> + * @get_device_info: Called to get VFIO device information for a mediated >>> + * device. >>> + * @vfio_device_info: VFIO device info. >>> + * Returns integer: success (0) or error (< 0) >>> + * @get_region_info: Called to get VFIO region size and flags of mediated >>> + * device. >>> + * @mdev: mediated device structure >>> + * @region_info: output, returns size and flags of >>> + * requested region. >>> + * @cap_type_id: returns id of capability. >>> + * @cap_type: returns pointer to capability structure >>> + * corresponding to capability id. >>> + * Returns integer: success (0) or error (< 0) >>> + * >>> + * Parent device that support mediated device should be registered with mdev >>> + * module with parent_ops structure. >>> + */ >>> + >>> +struct parent_ops { >>> + struct module *owner; >>> + const struct attribute_group **dev_attr_groups; >>> + const struct attribute_group **mdev_attr_groups; >>> + >>> + int (*supported_config)(struct device *dev, char *config); >>> + int (*create)(struct mdev_device *mdev, char *mdev_params); >>> + int (*destroy)(struct mdev_device *mdev); >>> + int (*reset)(struct mdev_device *mdev); >>> + int (*set_online_status)(struct mdev_device *mdev, bool online); >>> + int (*get_online_status)(struct mdev_device *mdev, bool *online); >>> + ssize_t (*read)(struct mdev_device *mdev, char *buf, size_t count, >>> + loff_t pos); >>> + ssize_t (*write)(struct mdev_device *mdev, char *buf, size_t count, >>> + loff_t pos); >>> + int (*mmap)(struct mdev_device *mdev, struct vm_area_struct *vma); >>> + int (*get_irq_info)(struct mdev_device *mdev, >>> + struct vfio_irq_info *irq_info); >>> + int (*set_irqs)(struct mdev_device *mdev, uint32_t flags, >>> + unsigned int index, unsigned int start, >>> + unsigned int count, void *data); >>> + int (*get_device_info)(struct mdev_device *mdev, >>> + struct vfio_device_info *dev_info); >>> + int (*get_region_info)(struct mdev_device *mdev, >>> + struct vfio_region_info *region_info, >>> + u16 *cap_type_id, void **cap_type); >>> +}; >> >> I have a strong objection here to such low-level interfaces, the interfaces >> between vfio-mdev and vendor drivers should be as thin as possible, not imposing >> any limitation to vendor drivers. > > Hi Jike, > > Welcome! :-) Aha, thanks! :) > > Unfortunately, this is something I definitely can't agree with you. > Glad to see your opinion! > We would like to capture the common code as much as possible without losing > flexibilities, so each vendor driver writers won't have to duplicate them and we > have something can be maintained publicly. > Yeah it is good to reduce the duplications among different vendor drivers, but what do you think about the duplication between here and other bus drivers like vfio-pci? > If you are running into specific limitation with above callback interfaces, > please show us the scenarios and we are very happy to look into that. > Though we don't actually test it upon this series (using high-level implementations instead), I personally don't think there is a problem. However, this doesn't necessarily mean it's sufficient. >> >> I saw that validate_map_request was removed from the ops and mmap was added. >> That is pretty nice. Furthermore, if you add an ioctl here, you can also remove >> get_device_info, get_irq_info, set_irqs, and get_region_info (and even "reset"). >> There are several benefits by doing this: > > The decision of moving validate_map_request is mainly because we are adding a lot of > advanced logic which most vendor drivers don't require, since we are the only consumer > of such logic, no need to put it in the public/shared module. > >> >> - Balanced interfaces. >> Like I replied in another mail, you won't have unbalanced interfaces. >> You already have read, write and mmap in the ops, why not ioctl? > > Sorry, don't think "balanced" interface is a design criteria especially when > simply pursuing the sake of "balanced or full-set" interface ends up lots > duplicated code for vendor driver writers. > Please kindly have a look at my comment on patch 2/4, about how to check the validity of "count". >> >> - Scalability. >> You are intercepting vfio optional capabilities in the framework, but >> how if vfio.ko, or vfio-pci.ko add a few new capabilities in the future? > > Exactly my point about the code sharing. > > If new cap is added inside vfio.ko or vfio-pci.ko, we can just add it into > vfio_mdev.ko. > > Adding the code in one place is better to duplicate in multiple vendor drivers. So after adding that, how many places will you have? >> >> - Abstraction. >> Even placing common codes here can avoid code duplication, you still >> have code duplicate with vfio-pci. Better to move common logic out of >> vfio-pci and call them from mdev vendor drivers. > > Are you saying to avoid the code duplications between vfio-pci and vfio-mdev? > Exactly. I haven't check other bus-driver like vfio-platform, but even if only having vfio-pci considered, there will be duplications. >> >> - Maintainability. >> This is pretty obvious :) > > Definitely not, the burden is moving to the vendor driver side. > Moving to vendor side is not the target, as said above, this will probably cause more abstraction and refactoring of existing vfio code. > Again, Jike, I really want to enable you with the mediated framework we have been > doing here. So it is probably easier for us to accommodate your need if you could > follow the interfaces we have introduced and let us know if you have any specific > issues. I won't read this as that one is not welcome to comment as long as he met no actual issue :) -- Thanks, Jike >>> + >>> +/* >>> + * Parent Device >>> + */ >>> + >>> +struct parent_device { >>> + struct device *dev; >>> + const struct parent_ops *ops; >>> + >>> + /* internal */ >>> + struct kref ref; >>> + struct list_head next; >>> + struct list_head mdev_list; >>> + struct mutex mdev_list_lock; >>> + wait_queue_head_t release_done; >>> +}; >>> + >>> +/** >>> + * struct mdev_driver - Mediated device driver >>> + * @name: driver name >>> + * @probe: called when new device created >>> + * @remove: called when device removed >>> + * @driver: device driver structure >>> + * >>> + **/ >>> +struct mdev_driver { >>> + const char *name; >>> + int (*probe)(struct device *dev); >>> + void (*remove)(struct device *dev); >>> + struct device_driver driver; >>> +}; >>> + >>> +static inline struct mdev_driver *to_mdev_driver(struct device_driver *drv) >>> +{ >>> + return drv ? container_of(drv, struct mdev_driver, driver) : NULL; >>> +} >>> + >>> +static inline struct mdev_device *to_mdev_device(struct device *dev) >>> +{ >>> + return dev ? container_of(dev, struct mdev_device, dev) : NULL; >>> +} >> >> These can be macros, like pci ones. >> >>> + >>> +static inline void *mdev_get_drvdata(struct mdev_device *mdev) >>> +{ >>> + return mdev->driver_data; >>> +} >>> + >>> +static inline void mdev_set_drvdata(struct mdev_device *mdev, void *data) >>> +{ >>> + mdev->driver_data = data; >>> +} >>> + >>> +extern struct bus_type mdev_bus_type; >>> + >>> +#define dev_is_mdev(d) ((d)->bus == &mdev_bus_type) >>> + >>> +extern int mdev_register_device(struct device *dev, >>> + const struct parent_ops *ops); >>> +extern void mdev_unregister_device(struct device *dev); >>> + >>> +extern int mdev_register_driver(struct mdev_driver *drv, struct module *owner); >>> +extern void mdev_unregister_driver(struct mdev_driver *drv); >>> + >>> +extern struct mdev_device *mdev_get_device(struct mdev_device *mdev); >>> +extern void mdev_put_device(struct mdev_device *mdev); >>> + >>> +extern struct mdev_device *mdev_get_device_by_group(struct iommu_group *group); >>> + >>> +#endif /* MDEV_H */ >>> >> >> -- >> Thanks, >> Jike