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

[Jike Song]

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