[Intel-gfx] [PATCH V13 4/6] mdev: introduce mediated virtio bus

[Jason Wang]

On 2019/11/20 下午9:49, Jason Gunthorpe wrote:
> On Wed, Nov 20, 2019 at 10:14:26AM +0800, Jason Wang wrote:
>
>>>> I don't quite get the question here.
>>> In the driver model the bus_type and foo_device are closely
>>> linked.
>> I don't get the definition of "closely linked" here. Do you think the bus
>> and device implement virtual bus series are closely linked? If yes, how did
>> they achieve that?
> I mean if you have a 'foo_device' then it should be on a 'foo_bus' and
> not on some 'bar_bus', as that is how the driver core generally works.


I fully agree with you here. But isn't that just what this patch did? We 
had "mdev_virtio" device on "mdev_virtio" bus not "mdev_vfio" bus.


>   
>>>    Creating 'mdev_device' instances and overriding the bus_type
>>> is a very abusive thing to do.
>> Ok, mdev_device (without this series) had:
>>
>> struct mdev_device {
>>      struct device dev;
>>      struct mdev_parent *parent;
>>      guid_t uuid;
>>      void *driver_data;
>>      struct list_head next;
>>      struct kobject *type_kobj;
>>      struct device *iommu_device;
>>      bool active;
>> };
>>
>> So it's nothing bus or VFIO specific. And what virtual bus had is:
> What do mean? 'struct mdev_parent *parent' is the VFIO specific
> stuff. I haven't figured out what the confusing mdev_parent is
> supposed to be,


struct mdev_parent_ops {
     struct module   *owner;
     const struct attribute_group **dev_attr_groups;
     const struct attribute_group **mdev_attr_groups;
     struct attribute_group **supported_type_groups;

     int     (*create)(struct kobject *kobj, struct mdev_device *mdev);
     int     (*remove)(struct mdev_device *mdev);
     int     (*open)(struct mdev_device *mdev);
     void    (*release)(struct mdev_device *mdev);
     ssize_t (*read)(struct mdev_device *mdev, char __user *buf,
             size_t count, loff_t *ppos);
     ssize_t (*write)(struct mdev_device *mdev, const char __user *buf,
              size_t count, loff_t *ppos);
     long    (*ioctl)(struct mdev_device *mdev, unsigned int cmd,
              unsigned long arg);
     int    (*mmap)(struct mdev_device *mdev, struct vm_area_struct *vma);
};

You can see that nothing is really VFIO specific here..


> or whhy the VFIO ops are linked to the parent or not
> the device..


I guess the answer the mdev_devices belongs to the same parent are 
expected to have same ops.


> Honestly the whole mdev thing has a very strange take on
> how to use the driver core.


Suggestions are welcomed.


>
>>> Abusing it for other things is not appropriate. ie creating an
>>> instance and not filling in most of the vfio focused ops is an abusive
>>> thing to do.
>> Well, it's only half of the mdev_parent_ops in mdev_parent, various methods
>> could be done do be more generic to avoid duplication of codes. No?
> There are many ways to avoid duplicating code.
>
> Taking something well defined, and bolting on something unrelated just
> to share a bit of code is a very poor way to avoid code duplication.


We can have make the code better...


>> I'm sure you will need to handle other issues besides GUID which had been
>> settled by mdev e.g IOMMU and types when starting to write a real hardware
>> driver.
> The iommu framework already handles that, the mdev stuff contributes
> very little from what I can see.


Yes, but if we start from beginning to invent a new infrastructure and 
we still need GUID, IOMMU, types. So it will be very similar to mdev 
looks right now. So why not improve mdev?


>
>>> Most likely, at least for virtio-net, everyone else will be able to
>>> use devlink as well, making it much less clear if that GUID lifecycle
>>> stuff is a good idea or not.
>> This assumption is wrong, we hard already had at least two concrete examples
>> of vDPA device that doesn't use devlink:
>>
>> - Intel IFC where virtio is done at VF level
>> - Ali Cloud ECS instance where virtio is done at PF level
> Again, you don't explain why they couldn't use devlink.


Yes, they could, but of course for many reasons they won't use devlink. 
Not only devlink, even netlink is not used or implemented in all type of 
network devices.


>
> Or, why do we need GUID lifecycle stuff when these PCI devices can
> only create a single virtio and can just go ahead and do that as soon
> as they are probed.
>
> The GUID stuff was invented for slicing, which you say is not
> happening in these cases.


I think that's all about a consistent management interface, "slicing by 
one" is still compatible.

Thanks


>
> Jason