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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
