Re: [Qemu-devel] [PATCH v7 1/4] vfio: Mediated device Core driver

[Neo Jia]

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! :-)

Unfortunately, this is something I definitely can't agree with you.

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.

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.

> 
> 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.

> 
>       -       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.

> 
>       -       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?

> 
>       -       Maintainability.
>               This is pretty obvious :)

Definitely not, the burden is moving to the vendor driver side.

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.

Thanks,
Neo

> 
> > +
> > +/*
> > + * 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