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

[Jike Song <jike.song@intel.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