Re: [PATCH V3 4/7] mdev: introduce device specific ops
On 2019/10/17 下午4:45, Cornelia Huck wrote:
On Thu, 17 Oct 2019 16:30:43 +0800
Jason Wang wrote:
On 2019/10/17 上午12:53, Alex Williamson wrote:
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
I will add a BUG_ON() when class_id has already set.
Probably better a WARN_ON()?
Right.
Thanks
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH V3 4/7] mdev: introduce device specific ops
On Thu, 17 Oct 2019 16:30:43 +0800
Jason Wang wrote:
> On 2019/10/17 上午12:53, Alex Williamson wrote:
> >>> 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
>
>
> I will add a BUG_ON() when class_id has already set.
Probably better a WARN_ON()?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH V3 4/7] mdev: introduce device specific ops
On 2019/10/17 上午12:53, Alex Williamson wrote:
On Wed, 16 Oct 2019 15:31:25 +
Parav Pandit wrote:
-Original Message-
From: Cornelia Huck
Sent: Wednesday, October 16, 2019 3:53 AM
To: Parav Pandit
Cc: Alex Williamson ; Jason Wang
; k...@vger.kernel.org; linux-s...@vger.kernel.org;
linux-ker...@vger.kernel.org; dri-devel@lists.freedesktop.org; intel-
g...@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org;
kwankh...@nvidia.com; m...@redhat.com; tiwei@intel.com;
virtualizat...@lists.linux-foundation.org; net...@vger.kernel.org;
maxime.coque...@redhat.com; cunming.li...@intel.com;
zhihong.w...@intel.com; rob.mil...@broadcom.com; xiao.w.w...@intel.com;
haotian.w...@sifive.com; zhen...@linux.intel.com; zhi.a.w...@intel.com;
jani.nik...@linux.intel.com; joonas.lahti...@linux.intel.com;
rodrigo.v...@intel.com; airl...@linux.ie; dan...@ffwll.ch;
far...@linux.ibm.com; pa...@linux.ibm.com; seb...@linux.ibm.com;
ober...@linux.ibm.com; heiko.carst...@de.ibm.com; g...@linux.ibm.com;
borntrae...@de.ibm.com; akrow...@linux.ibm.com; fre...@linux.ibm.com;
lingshan@intel.com; Ido Shamay ;
epere...@redhat.com; l...@redhat.com; christophe.de.dinec...@gmail.com;
kevin.t...@intel.com
Subject: Re: [PATCH V3 4/7] mdev: introduce device specific ops
On Wed, 16 Oct 2019 05:50:08 +
Parav Pandit wrote:
Hi Alex,
-Original Message-
From: Alex Williamson
Sent: Tuesday, October 15, 2019 12:27 PM
To: Jason Wang
Cc: Cornelia Huck ; k...@vger.kernel.org; linux-
s...@vger.kernel.org; linux-ker...@vger.kernel.org; dri-
de...@lists.freedesktop.org; intel-...@lists.freedesktop.org;
intel-gvt- d...@lists.freedesktop.org; kwankh...@nvidia.com;
m...@redhat.com; tiwei@intel.com;
virtualizat...@lists.linux-foundation.org;
net...@vger.kernel.org; maxime.coque...@redhat.com;
cunming.li...@intel.com; zhihong.w...@intel.com;
rob.mil...@broadcom.com; xiao.w.w...@intel.com;
haotian.w...@sifive.com; zhen...@linux.intel.com;
zhi.a.w...@intel.com; jani.nik...@linux.intel.com;
joonas.lahti...@linux.intel.com; rodrigo.v...@intel.com;
airl...@linux.ie; dan...@ffwll.ch; far...@linux.ibm.com;
pa...@linux.ibm.com; seb...@linux.ibm.com; ober...@linux.ibm.com;
heiko.carst...@de.ibm.com; g...@linux.ibm.com;
borntrae...@de.ibm.com; akrow...@linux.ibm.com;
fre...@linux.ibm.com; lingshan@intel.com; Ido Shamay
; epere...@redhat.com; l...@redhat.com; Parav
Pandit ; christophe.de.dinec...@gmail.com;
kevin.t...@intel.com
Subject: Re: [PATCH V3 4/7] mdev: introduce device specific ops
On Tue, 15 Oct 2019 20:17:01 +0800
Jason Wang wrote:
On 2019/10/15 下午6:41, Cornelia Huck wrote:
On Fri, 11 Oct 2019 16:15:54 +0800 Jason Wang
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 >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 p
Re: [PATCH V3 4/7] mdev: introduce device specific ops
On Wed, 16 Oct 2019 20:48:06 +
Parav Pandit wrote:
> > From: Alex Williamson
> > On Wed, 16 Oct 2019 15:31:25 +
> > Parav Pandit wrote:
> > > > From: Cornelia Huck
> > > > Parav Pandit wrote:
> > > > > > From: Alex Williamson
> > > > > > On Tue, 15 Oct 2019 20:17:01 +0800 Jason Wang
> > > > > > wrote:
> > > > > >
> > > > > > > On 2019/10/15 下午6:41, Cornelia Huck wrote:
> > > > > > > > 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 >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?
> > > >
> > > 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.
> We should have driver who intent to use different device ops for each
> device with single parent that supports this claim.
Why? Are we not allowed to identify restrictions implied by a given
proposal if we don't yet have a user? I can't subscribe to that.
> For instance, for a vfio-mdev we
> > already support the concept of an iommu backing device which makes
> > the
Re: [PATCH V3 4/7] mdev: introduce device specific ops
On Wed, 16 Oct 2019 15:31:25 + Parav Pandit wrote: > > -Original Message- > > From: Cornelia Huck > > Sent: Wednesday, October 16, 2019 3:53 AM > > To: Parav Pandit > > Cc: Alex Williamson ; Jason Wang > > ; k...@vger.kernel.org; linux-s...@vger.kernel.org; > > linux-ker...@vger.kernel.org; dri-devel@lists.freedesktop.org; intel- > > g...@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org; > > kwankh...@nvidia.com; m...@redhat.com; tiwei@intel.com; > > virtualizat...@lists.linux-foundation.org; net...@vger.kernel.org; > > maxime.coque...@redhat.com; cunming.li...@intel.com; > > zhihong.w...@intel.com; rob.mil...@broadcom.com; xiao.w.w...@intel.com; > > haotian.w...@sifive.com; zhen...@linux.intel.com; zhi.a.w...@intel.com; > > jani.nik...@linux.intel.com; joonas.lahti...@linux.intel.com; > > rodrigo.v...@intel.com; airl...@linux.ie; dan...@ffwll.ch; > > far...@linux.ibm.com; pa...@linux.ibm.com; seb...@linux.ibm.com; > > ober...@linux.ibm.com; heiko.carst...@de.ibm.com; g...@linux.ibm.com; > > borntrae...@de.ibm.com; akrow...@linux.ibm.com; fre...@linux.ibm.com; > > lingshan@intel.com; Ido Shamay ; > > epere...@redhat.com; l...@redhat.com; christophe.de.dinec...@gmail.com; > > kevin.t...@intel.com > > Subject: Re: [PATCH V3 4/7] mdev: introduce device specific ops > > > > On Wed, 16 Oct 2019 05:50:08 + > > Parav Pandit wrote: > > > > > Hi Alex, > > > > > > > -Original Message- > > > > From: Alex Williamson > > > > Sent: Tuesday, October 15, 2019 12:27 PM > > > > To: Jason Wang > > > > Cc: Cornelia Huck ; k...@vger.kernel.org; linux- > > > > s...@vger.kernel.org; linux-ker...@vger.kernel.org; dri- > > > > de...@lists.freedesktop.org; intel-...@lists.freedesktop.org; > > > > intel-gvt- d...@lists.freedesktop.org; kwankh...@nvidia.com; > > > > m...@redhat.com; tiwei@intel.com; > > > > virtualizat...@lists.linux-foundation.org; > > > > net...@vger.kernel.org; maxime.coque...@redhat.com; > > > > cunming.li...@intel.com; zhihong.w...@intel.com; > > > > rob.mil...@broadcom.com; xiao.w.w...@intel.com; > > > > haotian.w...@sifive.com; zhen...@linux.intel.com; > > > > zhi.a.w...@intel.com; jani.nik...@linux.intel.com; > > > > joonas.lahti...@linux.intel.com; rodrigo.v...@intel.com; > > > > airl...@linux.ie; dan...@ffwll.ch; far...@linux.ibm.com; > > > > pa...@linux.ibm.com; seb...@linux.ibm.com; ober...@linux.ibm.com; > > > > heiko.carst...@de.ibm.com; g...@linux.ibm.com; > > > > borntrae...@de.ibm.com; akrow...@linux.ibm.com; > > > > fre...@linux.ibm.com; lingshan@intel.com; Ido Shamay > > > > ; epere...@redhat.com; l...@redhat.com; Parav > > > > Pandit ; christophe.de.dinec...@gmail.com; > > > > kevin.t...@intel.com > > > > Subject: Re: [PATCH V3 4/7] mdev: introduce device specific ops > > > > > > > > On Tue, 15 Oct 2019 20:17:01 +0800 > > > > Jason Wang wrote: > > > > > > > > > On 2019/10/15 下午6:41, Cornelia Huck wrote: > > > > > > On Fri, 11 Oct 2019 16:15:54 +0800 Jason Wang > > > > > > 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 sin
Re: [PATCH V3 4/7] mdev: introduce device specific ops
On Wed, 16 Oct 2019 05:50:08 +
Parav Pandit wrote:
> Hi Alex,
>
> > -Original Message-
> > From: Alex Williamson
> > Sent: Tuesday, October 15, 2019 12:27 PM
> > To: Jason Wang
> > Cc: Cornelia Huck ; k...@vger.kernel.org; linux-
> > s...@vger.kernel.org; linux-ker...@vger.kernel.org; dri-
> > de...@lists.freedesktop.org; intel-...@lists.freedesktop.org; intel-gvt-
> > d...@lists.freedesktop.org; kwankh...@nvidia.com; m...@redhat.com;
> > tiwei@intel.com; virtualizat...@lists.linux-foundation.org;
> > net...@vger.kernel.org; maxime.coque...@redhat.com;
> > cunming.li...@intel.com; zhihong.w...@intel.com;
> > rob.mil...@broadcom.com; xiao.w.w...@intel.com;
> > haotian.w...@sifive.com; zhen...@linux.intel.com; zhi.a.w...@intel.com;
> > jani.nik...@linux.intel.com; joonas.lahti...@linux.intel.com;
> > rodrigo.v...@intel.com; airl...@linux.ie; dan...@ffwll.ch;
> > far...@linux.ibm.com; pa...@linux.ibm.com; seb...@linux.ibm.com;
> > ober...@linux.ibm.com; heiko.carst...@de.ibm.com; g...@linux.ibm.com;
> > borntrae...@de.ibm.com; akrow...@linux.ibm.com; fre...@linux.ibm.com;
> > lingshan@intel.com; Ido Shamay ;
> > epere...@redhat.com; l...@redhat.com; Parav Pandit
> > ; christophe.de.dinec...@gmail.com;
> > kevin.t...@intel.com
> > Subject: Re: [PATCH V3 4/7] mdev: introduce device specific ops
> >
> > On Tue, 15 Oct 2019 20:17:01 +0800
> > Jason Wang wrote:
> >
> > > On 2019/10/15 下午6:41, Cornelia Huck wrote:
> > > > On Fri, 11 Oct 2019 16:15:54 +0800
> > > > Jason Wang 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 >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 md
RE: [PATCH V3 4/7] mdev: introduce device specific ops
Hi Alex, > -Original Message- > From: Alex Williamson > Sent: Tuesday, October 15, 2019 12:27 PM > To: Jason Wang > Cc: Cornelia Huck ; k...@vger.kernel.org; linux- > s...@vger.kernel.org; linux-ker...@vger.kernel.org; dri- > de...@lists.freedesktop.org; intel-...@lists.freedesktop.org; intel-gvt- > d...@lists.freedesktop.org; kwankh...@nvidia.com; m...@redhat.com; > tiwei@intel.com; virtualizat...@lists.linux-foundation.org; > net...@vger.kernel.org; maxime.coque...@redhat.com; > cunming.li...@intel.com; zhihong.w...@intel.com; > rob.mil...@broadcom.com; xiao.w.w...@intel.com; > haotian.w...@sifive.com; zhen...@linux.intel.com; zhi.a.w...@intel.com; > jani.nik...@linux.intel.com; joonas.lahti...@linux.intel.com; > rodrigo.v...@intel.com; airl...@linux.ie; dan...@ffwll.ch; > far...@linux.ibm.com; pa...@linux.ibm.com; seb...@linux.ibm.com; > ober...@linux.ibm.com; heiko.carst...@de.ibm.com; g...@linux.ibm.com; > borntrae...@de.ibm.com; akrow...@linux.ibm.com; fre...@linux.ibm.com; > lingshan@intel.com; Ido Shamay ; > epere...@redhat.com; l...@redhat.com; Parav Pandit > ; christophe.de.dinec...@gmail.com; > kevin.t...@intel.com > Subject: Re: [PATCH V3 4/7] mdev: introduce device specific ops > > On Tue, 15 Oct 2019 20:17:01 +0800 > Jason Wang wrote: > > > On 2019/10/15 下午6:41, Cornelia Huck wrote: > > > On Fri, 11 Oct 2019 16:15:54 +0800 > > > Jason Wang wrote: > > > > > >> Currently, except for the create and remove, the rest of > > >> mdev_parent_ops is designed for vfio-mdev driver only and may not > > >> help for kernel mdev driver. With the help of class id, this patch > > >> introduces device specific callbacks inside mdev_device structure. > > >> This allows different set of callback to be used by vfio-mdev and > > >> virtio-mdev. > > >> > > >> Signed-off-by: Jason Wang > > >> --- > > >> .../driver-api/vfio-mediated-device.rst | 22 +--- > > >> MAINTAINERS | 1 + > > >> drivers/gpu/drm/i915/gvt/kvmgt.c | 18 --- > > >> drivers/s390/cio/vfio_ccw_ops.c | 18 --- > > >> drivers/s390/crypto/vfio_ap_ops.c | 14 +++-- > > >> drivers/vfio/mdev/mdev_core.c | 9 +++- > > >> drivers/vfio/mdev/mdev_private.h | 1 + > > >> drivers/vfio/mdev/vfio_mdev.c | 37 ++--- > > >> include/linux/mdev.h | 42 +++ > > >> include/linux/vfio_mdev.h | 52 +++ > > >> samples/vfio-mdev/mbochs.c| 20 --- > > >> samples/vfio-mdev/mdpy.c | 21 +--- > > >> samples/vfio-mdev/mtty.c | 18 --- > > >> 13 files changed, 177 insertions(+), 96 deletions(-) > > >> create mode 100644 include/linux/vfio_mdev.h > > >> > > >> diff --git a/Documentation/driver-api/vfio-mediated-device.rst > > >> b/Documentation/driver-api/vfio-mediated-device.rst > > >> index 2035e48da7b2..553574ebba73 100644 > > >> --- a/Documentation/driver-api/vfio-mediated-device.rst > > >> +++ b/Documentation/driver-api/vfio-mediated-device.rst > > >> @@ -152,11 +152,20 @@ callbacks per mdev parent device, per mdev > type, or any other categorization. > > >> Vendor drivers are expected to be fully asynchronous in this respect or > > >> provide their own internal resource protection.) > > >> > > >> -The callbacks in the mdev_parent_ops structure are as follows: > > >> +In order to support multiple types of device/driver, device needs > > >> +to provide both class_id and device_ops through: > > > "As multiple types of mediated devices may be supported, the device > > > needs to set up the class id and the device specific callbacks via:" > > > > > > ? > > > > > >> > > >> -* open: open callback of mediated device > > >> -* close: close callback of mediated device > > >> -* ioctl: ioctl callback of mediated device > > >> +void mdev_set_class(struct mdev_device *mdev, u16 id, const > > >> + void *ops); > > >> + > > >> +The class_id is used to be paired with ids in id_table in > > >> +mdev_driver structure for probing the correct driver. > > > "The class id (specified in id) is used to match a device with an >
Re: [PATCH V3 4/7] mdev: introduce device specific ops
On Tue, 15 Oct 2019 20:17:01 +0800 Jason Wang wrote: > On 2019/10/15 下午6:41, Cornelia Huck wrote: > > On Fri, 11 Oct 2019 16:15:54 +0800 > > Jason Wang wrote: > > > >> Currently, except for the create and remove, the rest of > >> mdev_parent_ops is designed for vfio-mdev driver only and may not help > >> for kernel mdev driver. With the help of class id, this patch > >> introduces device specific callbacks inside mdev_device > >> structure. This allows different set of callback to be used by > >> vfio-mdev and virtio-mdev. > >> > >> Signed-off-by: Jason Wang > >> --- > >> .../driver-api/vfio-mediated-device.rst | 22 +--- > >> MAINTAINERS | 1 + > >> drivers/gpu/drm/i915/gvt/kvmgt.c | 18 --- > >> drivers/s390/cio/vfio_ccw_ops.c | 18 --- > >> drivers/s390/crypto/vfio_ap_ops.c | 14 +++-- > >> drivers/vfio/mdev/mdev_core.c | 9 +++- > >> drivers/vfio/mdev/mdev_private.h | 1 + > >> drivers/vfio/mdev/vfio_mdev.c | 37 ++--- > >> include/linux/mdev.h | 42 +++ > >> include/linux/vfio_mdev.h | 52 +++ > >> samples/vfio-mdev/mbochs.c| 20 --- > >> samples/vfio-mdev/mdpy.c | 21 +--- > >> samples/vfio-mdev/mtty.c | 18 --- > >> 13 files changed, 177 insertions(+), 96 deletions(-) > >> create mode 100644 include/linux/vfio_mdev.h > >> > >> diff --git a/Documentation/driver-api/vfio-mediated-device.rst > >> b/Documentation/driver-api/vfio-mediated-device.rst > >> index 2035e48da7b2..553574ebba73 100644 > >> --- a/Documentation/driver-api/vfio-mediated-device.rst > >> +++ b/Documentation/driver-api/vfio-mediated-device.rst > >> @@ -152,11 +152,20 @@ callbacks per mdev parent device, per mdev type, or > >> any other categorization. > >> Vendor drivers are expected to be fully asynchronous in this respect or > >> provide their own internal resource protection.) > >> > >> -The callbacks in the mdev_parent_ops structure are as follows: > >> +In order to support multiple types of device/driver, device needs to > >> +provide both class_id and device_ops through: > > "As multiple types of mediated devices may be supported, the device > > needs to set up the class id and the device specific callbacks via:" > > > > ? > > > >> > >> -* open: open callback of mediated device > >> -* close: close callback of mediated device > >> -* ioctl: ioctl callback of mediated device > >> +void mdev_set_class(struct mdev_device *mdev, u16 id, const void > >> *ops); > >> + > >> +The class_id is used to be paired with ids in id_table in mdev_driver > >> +structure for probing the correct driver. > > "The class id (specified in id) is used to match a device with an mdev > > driver via its id table." > > > > ? > > > >> The device_ops is device > >> +specific callbacks which can be get through mdev_get_dev_ops() > >> +function by mdev bus driver. > > "The device specific callbacks (specified in *ops) are obtainable via > > mdev_get_dev_ops() (for use by the mdev bus driver.)" > > > > ? > > > >> For vfio-mdev device, its device specific > >> +ops are as follows: > > "A vfio-mdev device (class id MDEV_ID_VFIO) uses the following > > device-specific ops:" > > > > ? > > > All you propose is better than what I wrote, will change the docs. > > > > > >> + > >> +* open: open callback of vfio mediated device > >> +* close: close callback of vfio mediated device > >> +* ioctl: ioctl callback of vfio mediated device > >> * read : read emulation callback > >> * write: write emulation callback > >> * mmap: mmap emulation callback > >> @@ -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
Re: [PATCH V3 4/7] mdev: introduce device specific ops
On 2019/10/15 下午6:41, Cornelia Huck wrote: On Fri, 11 Oct 2019 16:15:54 +0800 Jason Wang wrote: Currently, except for the create and remove, the rest of mdev_parent_ops is designed for vfio-mdev driver only and may not help for kernel mdev driver. With the help of class id, this patch introduces device specific callbacks inside mdev_device structure. This allows different set of callback to be used by vfio-mdev and virtio-mdev. Signed-off-by: Jason Wang --- .../driver-api/vfio-mediated-device.rst | 22 +--- MAINTAINERS | 1 + drivers/gpu/drm/i915/gvt/kvmgt.c | 18 --- drivers/s390/cio/vfio_ccw_ops.c | 18 --- drivers/s390/crypto/vfio_ap_ops.c | 14 +++-- drivers/vfio/mdev/mdev_core.c | 9 +++- drivers/vfio/mdev/mdev_private.h | 1 + drivers/vfio/mdev/vfio_mdev.c | 37 ++--- include/linux/mdev.h | 42 +++ include/linux/vfio_mdev.h | 52 +++ samples/vfio-mdev/mbochs.c| 20 --- samples/vfio-mdev/mdpy.c | 21 +--- samples/vfio-mdev/mtty.c | 18 --- 13 files changed, 177 insertions(+), 96 deletions(-) create mode 100644 include/linux/vfio_mdev.h diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst index 2035e48da7b2..553574ebba73 100644 --- a/Documentation/driver-api/vfio-mediated-device.rst +++ b/Documentation/driver-api/vfio-mediated-device.rst @@ -152,11 +152,20 @@ callbacks per mdev parent device, per mdev type, or any other categorization. Vendor drivers are expected to be fully asynchronous in this respect or provide their own internal resource protection.) -The callbacks in the mdev_parent_ops structure are as follows: +In order to support multiple types of device/driver, device needs to +provide both class_id and device_ops through: "As multiple types of mediated devices may be supported, the device needs to set up the class id and the device specific callbacks via:" ? -* open: open callback of mediated device -* close: close callback of mediated device -* ioctl: ioctl callback of mediated device +void mdev_set_class(struct mdev_device *mdev, u16 id, const void *ops); + +The class_id is used to be paired with ids in id_table in mdev_driver +structure for probing the correct driver. "The class id (specified in id) is used to match a device with an mdev driver via its id table." ? The device_ops is device +specific callbacks which can be get through mdev_get_dev_ops() +function by mdev bus driver. "The device specific callbacks (specified in *ops) are obtainable via mdev_get_dev_ops() (for use by the mdev bus driver.)" ? For vfio-mdev device, its device specific +ops are as follows: "A vfio-mdev device (class id MDEV_ID_VFIO) uses the following device-specific ops:" ? All you propose is better than what I wrote, will change the docs. + +* open: open callback of vfio mediated device +* close: close callback of vfio mediated device +* ioctl: ioctl callback of vfio mediated device * read : read emulation callback * write: write emulation callback * mmap: mmap emulation callback @@ -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. Thanks If the former, would it make sense to first register the ops for an id (once), and then have the ->create callback only set the class id (with the core doing the lookup of the ops)? However, the mdev_parent_ops structure is not required in the function call that a driver should use to unregister itself with the mdev core driver::
Re: [PATCH V3 4/7] mdev: introduce device specific ops
On Fri, 11 Oct 2019 16:15:54 +0800 Jason Wang wrote: > Currently, except for the create and remove, the rest of > mdev_parent_ops is designed for vfio-mdev driver only and may not help > for kernel mdev driver. With the help of class id, this patch > introduces device specific callbacks inside mdev_device > structure. This allows different set of callback to be used by > vfio-mdev and virtio-mdev. > > Signed-off-by: Jason Wang > --- > .../driver-api/vfio-mediated-device.rst | 22 +--- > MAINTAINERS | 1 + > drivers/gpu/drm/i915/gvt/kvmgt.c | 18 --- > drivers/s390/cio/vfio_ccw_ops.c | 18 --- > drivers/s390/crypto/vfio_ap_ops.c | 14 +++-- > drivers/vfio/mdev/mdev_core.c | 9 +++- > drivers/vfio/mdev/mdev_private.h | 1 + > drivers/vfio/mdev/vfio_mdev.c | 37 ++--- > include/linux/mdev.h | 42 +++ > include/linux/vfio_mdev.h | 52 +++ > samples/vfio-mdev/mbochs.c| 20 --- > samples/vfio-mdev/mdpy.c | 21 +--- > samples/vfio-mdev/mtty.c | 18 --- > 13 files changed, 177 insertions(+), 96 deletions(-) > create mode 100644 include/linux/vfio_mdev.h > > diff --git a/Documentation/driver-api/vfio-mediated-device.rst > b/Documentation/driver-api/vfio-mediated-device.rst > index 2035e48da7b2..553574ebba73 100644 > --- a/Documentation/driver-api/vfio-mediated-device.rst > +++ b/Documentation/driver-api/vfio-mediated-device.rst > @@ -152,11 +152,20 @@ callbacks per mdev parent device, per mdev type, or any > other categorization. > Vendor drivers are expected to be fully asynchronous in this respect or > provide their own internal resource protection.) > > -The callbacks in the mdev_parent_ops structure are as follows: > +In order to support multiple types of device/driver, device needs to > +provide both class_id and device_ops through: "As multiple types of mediated devices may be supported, the device needs to set up the class id and the device specific callbacks via:" ? > > -* open: open callback of mediated device > -* close: close callback of mediated device > -* ioctl: ioctl callback of mediated device > +void mdev_set_class(struct mdev_device *mdev, u16 id, const void *ops); > + > +The class_id is used to be paired with ids in id_table in mdev_driver > +structure for probing the correct driver. "The class id (specified in id) is used to match a device with an mdev driver via its id table." ? > The device_ops is device > +specific callbacks which can be get through mdev_get_dev_ops() > +function by mdev bus driver. "The device specific callbacks (specified in *ops) are obtainable via mdev_get_dev_ops() (for use by the mdev bus driver.)" ? > For vfio-mdev device, its device specific > +ops are as follows: "A vfio-mdev device (class id MDEV_ID_VFIO) uses the following device-specific ops:" ? > + > +* open: open callback of vfio mediated device > +* close: close callback of vfio mediated device > +* ioctl: ioctl callback of vfio mediated device > * read : read emulation callback > * write: write emulation callback > * mmap: mmap emulation callback > @@ -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? If the former, would it make sense to first register the ops for an id (once), and then have the ->create callback only set the class id (with the core doing the lookup of the ops)? > > However, the mdev_parent_ops structure is not required in the function call > that a driver should use to unregister itself with the mdev core driver:: ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH V3 4/7] mdev: introduce device specific ops
Currently, except for the create and remove, the rest of
mdev_parent_ops is designed for vfio-mdev driver only and may not help
for kernel mdev driver. With the help of class id, this patch
introduces device specific callbacks inside mdev_device
structure. This allows different set of callback to be used by
vfio-mdev and virtio-mdev.
Signed-off-by: Jason Wang
---
.../driver-api/vfio-mediated-device.rst | 22 +---
MAINTAINERS | 1 +
drivers/gpu/drm/i915/gvt/kvmgt.c | 18 ---
drivers/s390/cio/vfio_ccw_ops.c | 18 ---
drivers/s390/crypto/vfio_ap_ops.c | 14 +++--
drivers/vfio/mdev/mdev_core.c | 9 +++-
drivers/vfio/mdev/mdev_private.h | 1 +
drivers/vfio/mdev/vfio_mdev.c | 37 ++---
include/linux/mdev.h | 42 +++
include/linux/vfio_mdev.h | 52 +++
samples/vfio-mdev/mbochs.c| 20 ---
samples/vfio-mdev/mdpy.c | 21 +---
samples/vfio-mdev/mtty.c | 18 ---
13 files changed, 177 insertions(+), 96 deletions(-)
create mode 100644 include/linux/vfio_mdev.h
diff --git a/Documentation/driver-api/vfio-mediated-device.rst
b/Documentation/driver-api/vfio-mediated-device.rst
index 2035e48da7b2..553574ebba73 100644
--- a/Documentation/driver-api/vfio-mediated-device.rst
+++ b/Documentation/driver-api/vfio-mediated-device.rst
@@ -152,11 +152,20 @@ callbacks per mdev parent device, per mdev type, or any
other categorization.
Vendor drivers are expected to be fully asynchronous in this respect or
provide their own internal resource protection.)
-The callbacks in the mdev_parent_ops structure are as follows:
+In order to support multiple types of device/driver, device needs to
+provide both class_id and device_ops through:
-* open: open callback of mediated device
-* close: close callback of mediated device
-* ioctl: ioctl callback of mediated device
+void mdev_set_class(struct mdev_device *mdev, u16 id, const void *ops);
+
+The class_id is used to be paired with ids in id_table in mdev_driver
+structure for probing the correct driver. The device_ops is device
+specific callbacks which can be get through mdev_get_dev_ops()
+function by mdev bus driver. For vfio-mdev device, its device specific
+ops are as follows:
+
+* open: open callback of vfio mediated device
+* close: close callback of vfio mediated device
+* ioctl: ioctl callback of vfio mediated device
* read : read emulation callback
* write: write emulation callback
* mmap: mmap emulation callback
@@ -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);
However, the mdev_parent_ops structure is not required in the function call
that a driver should use to unregister itself with the mdev core driver::
diff --git a/MAINTAINERS b/MAINTAINERS
index 8824f61cd2c0..3d196a023b5e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17127,6 +17127,7 @@ S: Maintained
F: Documentation/driver-api/vfio-mediated-device.rst
F: drivers/vfio/mdev/
F: include/linux/mdev.h
+F: include/linux/vfio_mdev.h
F: samples/vfio-mdev/
VFIO PLATFORM DRIVER
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 17e9d4634c84..7e2b720074fd 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -42,6 +42,7 @@
#include
#include
#include
+#include
#include
#include
@@ -643,6 +644,8 @@ static void kvmgt_put_vfio_device(void *vgpu)
vfio_device_put(((struct intel_vgpu *)vgpu)->vdev.vfio_device);
}
+static const struct vfio_mdev_device_ops intel_vfio_vgpu_dev_ops;
+
static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev)
{
struct intel_vgpu *vgpu = NULL;
@@ -678,7 +681,7 @@ static int intel_vgpu_create(struct kobject *kobj, struct
mdev_device *mdev)
dev_name(mdev_dev(mdev)));
ret = 0;
- mdev_set_class(mdev, MDEV_ID_VFIO);
+ mdev_set_class(mdev, MDEV_ID_VFIO, _vfio_vgpu_dev_ops);
out:
return ret;
}
@@ -1599,20 +1602,21 @@ static const struct attribute_group
*intel_vgpu_groups[] = {
NULL,
};
-static struct mdev_parent_ops intel_vgpu_ops = {
- .mdev_attr_groups = intel_vgpu_groups,
- .create = intel_vgpu_create,
- .remove = intel_vgpu_remove,
-
+static const struct vfio_mdev_device_ops
