New subject: [PATCH V3 4/7] mdev: introduce device specific ops

16 Oct 2019

      On Wed, 16 Oct 2019 15:31:25 +0000
Parav Pandit parav@mellanox.com wrote:
...
...
-----Original Message-----
From: Cornelia Huck cohuck@redhat.com
Sent: Wednesday, October 16, 2019 3:53 AM
To: Parav Pandit parav@mellanox.com
Cc: Alex Williamson alex.williamson@redhat.com; Jason Wang
jasowang@redhat.com; kvm@vger.kernel.org; linux-s390@vger.kernel.org;
linux-kernel@vger.kernel.org; dri-devel@lists.freedesktop.org; intel-
gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org;
kwankhede@nvidia.com; mst@redhat.com; tiwei.bie@intel.com;
virtualization@lists.linux-foundation.org; netdev@vger.kernel.org;
maxime.coquelin@redhat.com; cunming.liang@intel.com;
zhihong.wang@intel.com; rob.miller@broadcom.com; xiao.w.wang@intel.com;
haotian.wang@sifive.com; zhenyuw@linux.intel.com; zhi.a.wang@intel.com;
jani.nikula@linux.intel.com; joonas.lahtinen@linux.intel.com;
rodrigo.vivi@intel.com; airlied@linux.ie; daniel@ffwll.ch;
farman@linux.ibm.com; pasic@linux.ibm.com; sebott@linux.ibm.com;
oberpar@linux.ibm.com; heiko.carstens@de.ibm.com; gor@linux.ibm.com;
borntraeger@de.ibm.com; akrowiak@linux.ibm.com; freude@linux.ibm.com;
lingshan.zhu@intel.com; Ido Shamay idos@mellanox.com;
eperezma@redhat.com; lulu@redhat.com; christophe.de.dinechin@gmail.com;
kevin.tian@intel.com
Subject: Re: [PATCH V3 4/7] mdev: introduce device specific ops
On Wed, 16 Oct 2019 05:50:08 +0000
Parav Pandit parav@mellanox.com wrote:
...
Hi Alex,
...
-----Original Message-----
From: Alex Williamson alex.williamson@redhat.com
Sent: Tuesday, October 15, 2019 12:27 PM
To: Jason Wang jasowang@redhat.com
Cc: Cornelia Huck cohuck@redhat.com; kvm@vger.kernel.org; linux-
s390@vger.kernel.org; linux-kernel@vger.kernel.org; dri-
devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org;
intel-gvt- dev@lists.freedesktop.org; kwankhede@nvidia.com;
mst@redhat.com; tiwei.bie@intel.com;
virtualization@lists.linux-foundation.org;
netdev@vger.kernel.org; maxime.coquelin@redhat.com;
cunming.liang@intel.com; zhihong.wang@intel.com;
rob.miller@broadcom.com; xiao.w.wang@intel.com;
haotian.wang@sifive.com; zhenyuw@linux.intel.com;
zhi.a.wang@intel.com; jani.nikula@linux.intel.com;
joonas.lahtinen@linux.intel.com; rodrigo.vivi@intel.com;
airlied@linux.ie; daniel@ffwll.ch; farman@linux.ibm.com;
pasic@linux.ibm.com; sebott@linux.ibm.com; oberpar@linux.ibm.com;
heiko.carstens@de.ibm.com; gor@linux.ibm.com;
borntraeger@de.ibm.com; akrowiak@linux.ibm.com;
freude@linux.ibm.com; lingshan.zhu@intel.com; Ido Shamay
idos@mellanox.com; eperezma@redhat.com; lulu@redhat.com; Parav
Pandit parav@mellanox.com; christophe.de.dinechin@gmail.com;
kevin.tian@intel.com
Subject: Re: [PATCH V3 4/7] mdev: introduce device specific ops
On Tue, 15 Oct 2019 20:17:01 +0800
Jason Wang jasowang@redhat.com wrote:
...
On 2019/10/15 下午6:41, Cornelia Huck wrote:
...
On Fri, 11 Oct 2019 16:15:54 +0800 Jason Wang
jasowang@redhat.com 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.

current mdev binding to vfio_mdev
mdev binding to virtio
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?
How many are we expecting? I envisioned handful of them.
It carries because parent is few, mdevs are several hundreds.
It makes sense to keep few copies, instead of several hundred copies
and it doesn't need to setup on every mdev creation.
It does need setup on every mdev creation, it's just a matter of the
scope, 'id and ops' vs 'id only' vs 'ops with implicit id'.  The other
argument is assuming a space vs time trade-off that I'm having a hard
time judging is necessarily the correct approach.  We potentially have
better data locality in the mdev device structure vs the parent.  The
caching of the ops structure itself is separate from how we get to it.
We might have hundreds of pointers to those ops structure, but the
space trade-off might we worth it if they're on the same cacheline as
the mdev device itself vs the indirection via the parent.
I see a couple other drawbacks to the parent hosted ops pointers as
well.  First, it imposes that per parent there can only be one device
ops structure per class id, but who's to say that different types of
mdev devices for a given parent all make the same callbacks into the
parent.  For instance, for a vfio-mdev we already support the concept
of an iommu backing device which makes the type1 iommu code behave a
little differently.  Those differences might be sufficient that the
parent driver would register a different device ops structure for an
iommu backed mdev vs a non-iommu backed device.  The other drawback is
that it implies a binary difference in all mdev parent drivers to add
any new device ids.  I know we don't guarantee binary compatibility,
but it's rather ugly.
Overall, I guess I tend to prefer Connie's proposal, the class id and
structure are tied together and the parent driver is only responsible
for one of them, the class id is hidden away in mdev-core and the mdev
driver itself.
...
...
...
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;
}
One further step towards making this hard to use incorrectly might be
to return an error if class_id is already set.  Thanks,
Alex