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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
