On Wed, 16 Oct 2019 05:50:08 +0000 Parav Pandit <[email protected]> wrote:
> Hi Alex, > > > -----Original Message----- > > From: Alex Williamson <[email protected]> > > Sent: Tuesday, October 15, 2019 12:27 PM > > To: Jason Wang <[email protected]> > > Cc: Cornelia Huck <[email protected]>; [email protected]; linux- > > [email protected]; [email protected]; dri- > > [email protected]; [email protected]; intel-gvt- > > [email protected]; [email protected]; [email protected]; > > [email protected]; [email protected]; > > [email protected]; [email protected]; > > [email protected]; [email protected]; > > [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected]; > > [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected]; > > [email protected]; Ido Shamay <[email protected]>; > > [email protected]; [email protected]; Parav Pandit > > <[email protected]>; [email protected]; > > [email protected] > > Subject: Re: [PATCH V3 4/7] mdev: introduce device specific ops > > > > On Tue, 15 Oct 2019 20:17:01 +0800 > > Jason Wang <[email protected]> wrote: > > > > > On 2019/10/15 下午6:41, Cornelia Huck wrote: > > > > On Fri, 11 Oct 2019 16:15:54 +0800 > > > > Jason Wang <[email protected]> wrote: > > > >> @@ -167,9 +176,10 @@ register itself with the mdev core driver:: > > > >> extern int mdev_register_device(struct device *dev, > > > >> const struct mdev_parent_ops > > > >> *ops); > > > >> > > > >> -It is also required to specify the class_id through:: > > > >> +It is also required to specify the class_id and device specific ops > > through:: > > > >> > > > >> - extern int mdev_set_class(struct device *dev, u16 id); > > > >> + extern int mdev_set_class(struct device *dev, u16 id, > > > >> + const void *ops); > > > > Apologies if that has already been discussed, but do we want a 1:1 > > > > relationship between id and ops, or can different devices with the > > > > same id register different ops? > > > > > > > > > I think we have a N:1 mapping between id and ops, e.g we want both > > > virtio-mdev and vhost-mdev use a single set of device ops. > > > > The contents of the ops structure is essentially defined by the id, which is > > why I was leaning towards them being defined together. They are effectively > > interlocked, the id defines which mdev "endpoint" > > driver is loaded and that driver requires mdev_get_dev_ops() to return the > > structure required by the driver. I wish there was a way we could > > incorporate type checking here. We toyed with the idea of having the class > > in the same structure as the ops, but I think this approach was chosen for > > simplicity. We could still do something like: > > > > int mdev_set_class_struct(struct device *dev, const struct mdev_class_struct > > *class); > > > > struct mdev_class_struct { > > u16 id; > > union { > > struct vfio_mdev_ops vfio_ops; > > struct virtio_mdev_ops virtio_ops; > > }; > > }; > > > > Maybe even: > > > > struct vfio_mdev_ops *mdev_get_vfio_ops(struct mdev_device *mdev) { > > BUG_ON(mdev->class.id != MDEV_ID_VFIO); > > return &mdev->class.vfio_ops; > > } > > > > The match callback would of course just use the mdev->class.id value. > > Functionally equivalent, but maybe better type characteristics. Thanks, > > > > Alex > > We have 3 use cases of mdev. > 1. current mdev binding to vfio_mdev > 2. mdev binding to virtio > 3. mdev binding to mlx5_core without dev_ops > > Also > (a) a given parent may serve multiple types of classes in future. > (b) number of classes may not likely explode, they will be handful of them. > (vfio_mdev, virtio) > > So, instead of making copies of this dev_ops pointer in each mdev, it is > better to keep const multiple ops in their parent device. > Something like below, > > struct mdev_parent { > [..] > struct mdev_parent_ops *parent_ops; /* create, remove */ > struct vfio_mdev_ops *vfio_ops; /* read,write, ioctl etc */ > struct virtio_mdev_ops *virtio_ops; /* virtio ops */ > }; That feels a bit odd. Why should the parent carry pointers to every possible version of ops? > > const struct vfio_mdev_ops *mdev_get_vfio_ops(struct mdev_parent *parent); > const struct virtio_mdev_ops *mdev_get_virtio_ops(struct mdev_parent *parent); > > This way, > (a) we have strong type check support > (b) ops pointer is not duplicated across several hundred mdev devices, and > don't have to set on every mdev creation > (c) all 3 classes of mdev are supported > (d) one extra symbol table entry used per ops type, but there are not > expected to grow a lot. > (e) multiple classes per single parent is still supported > (f) still extendible for multiple classes (well defined classes = vfio, > virtio, and vendor class) Yet another suggestion: have the class id derive from the function you use to set up the ops. void mdev_set_vfio_ops(struct mdev_device *mdev, const struct vfio_mdev_ops *vfio_ops) { mdev->device_ops = vfio_ops; mdev->class_id = MDEV_ID_VFIO; } void mdev_set_virtio_ops(struct mdev_device *mdev, const struct virtio_mdev_ops *virtio_ops) { mdev->device_ops = virtio_ops; mdev->class_id = MDEV_ID_VIRTIO; } void mdev_set_vhost_ops(struct mdev_device *mdev, const struct virtio_mdev_ops *virtio_ops) { mdev->device_ops = virtio_ops; mdev->class_id = MDEV_ID_VHOST; } void mdev_set_vendor_ops(struct mdev_device *mdev) /* no ops */ { mdev->class_id = MDEV_ID_VENDOR; } _______________________________________________ Virtualization mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/virtualization
