lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 25 Sep 2019 20:04:54 +0800 From: Jason Wang <jasowang@...hat.com> To: Alex Williamson <alex.williamson@...hat.com> Cc: kvm@...r.kernel.org, linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org, intel-gfx@...ts.freedesktop.org, intel-gvt-dev@...ts.freedesktop.org, kwankhede@...dia.com, mst@...hat.com, tiwei.bie@...el.com, virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org, cohuck@...hat.com, maxime.coquelin@...hat.com, cunming.liang@...el.com, zhihong.wang@...el.com, rob.miller@...adcom.com, xiao.w.wang@...el.com, haotian.wang@...ive.com, zhenyuw@...ux.intel.com, zhi.a.wang@...el.com, jani.nikula@...ux.intel.com, joonas.lahtinen@...ux.intel.com, rodrigo.vivi@...el.com, airlied@...ux.ie, daniel@...ll.ch, farman@...ux.ibm.com, pasic@...ux.ibm.com, sebott@...ux.ibm.com, oberpar@...ux.ibm.com, heiko.carstens@...ibm.com, gor@...ux.ibm.com, borntraeger@...ibm.com, akrowiak@...ux.ibm.com, freude@...ux.ibm.com, lingshan.zhu@...el.com, idos@...lanox.com, eperezma@...hat.com, lulu@...hat.com, parav@...lanox.com, christophe.de.dinechin@...il.com, kevin.tian@...el.com Subject: Re: [PATCH V2 5/8] mdev: introduce device specific ops On 2019/9/25 上午7:06, Alex Williamson wrote: > On Tue, 24 Sep 2019 21:53:29 +0800 > Jason Wang <jasowang@...hat.com> 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 <jasowang@...hat.com> >> --- >> .../driver-api/vfio-mediated-device.rst | 4 +- >> MAINTAINERS | 1 + >> drivers/gpu/drm/i915/gvt/kvmgt.c | 17 +++--- >> drivers/s390/cio/vfio_ccw_ops.c | 17 ++++-- >> drivers/s390/crypto/vfio_ap_ops.c | 13 +++-- >> drivers/vfio/mdev/mdev_core.c | 12 +++++ >> 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 | 19 ++++--- >> samples/vfio-mdev/mdpy.c | 19 ++++--- >> samples/vfio-mdev/mtty.c | 17 ++++-- >> 13 files changed, 168 insertions(+), 83 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 a5bdc60d62a1..d50425b368bb 100644 >> --- a/Documentation/driver-api/vfio-mediated-device.rst >> +++ b/Documentation/driver-api/vfio-mediated-device.rst >> @@ -152,7 +152,9 @@ 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: >> +The device specific callbacks are referred through device_ops pointer >> +in mdev_parent_ops. For vfio-mdev device, its callbacks in device_ops >> +are as follows: > This is not accurate. device_ops is now on the mdev_device and is an > mdev bus driver specific structure of callbacks that must be registered > for each mdev device by the parent driver during the create callback. > There's a one to one mapping of class_id to mdev_device_ops callbacks. Yes. > > That also suggests to me that we could be more clever in registering > both of these with mdev-core. Can we embed the class_id in the ops > structure in a common way so that the core can extract it and the bus > drivers can access their specific callbacks? That seems much cleaner, let me try to do that in V3. > >> * open: open callback of mediated device >> * close: close callback of mediated device >> diff --git a/MAINTAINERS b/MAINTAINERS >> index b2326dece28e..89832b316500 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -17075,6 +17075,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 f793252a3d2a..b274f5ee481f 100644 >> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c >> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c >> @@ -42,6 +42,7 @@ >> #include <linux/kvm_host.h> >> #include <linux/vfio.h> >> #include <linux/mdev.h> >> +#include <linux/vfio_mdev.h> >> #include <linux/debugfs.h> >> >> #include <linux/nospec.h> >> @@ -643,6 +644,8 @@ static void kvmgt_put_vfio_device(void *vgpu) >> vfio_device_put(((struct intel_vgpu *)vgpu)->vdev.vfio_device); >> } >> >> +static 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; >> @@ -679,6 +682,7 @@ static int intel_vgpu_create(struct kobject *kobj, struct mdev_device *mdev) >> ret = 0; >> >> mdev_set_class_id(mdev, MDEV_ID_VFIO); >> + mdev_set_dev_ops(mdev, &intel_vfio_vgpu_dev_ops); > This seems rather unrefined. We're registering interdependent data in > separate calls. All drivers need to make both of these calls. I'm not > sure if this is a good idea, but what if we had: > > static const struct vfio_mdev_device_ops intel_vfio_vgpu_dev_ops = { > .id = MDEV_ID_VFIO, > .open = intel_vgpu_open, > .release = intel_vgpu_release, > ... > > And the set function passed &intel_vfio_vgpu_dev_ops.id and the mdev > bus drivers used container_of to get to their callbacks? Yes, with the check of mdev_device_create() if nothing is set by the device. > >> out: >> return ret; >> } >> @@ -1601,20 +1605,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 struct vfio_mdev_device_ops intel_vfio_vgpu_dev_ops = { >> .open = intel_vgpu_open, >> .release = intel_vgpu_release, >> - >> .read = intel_vgpu_read, >> .write = intel_vgpu_write, >> .mmap = intel_vgpu_mmap, >> .ioctl = intel_vgpu_ioctl, >> }; >> >> +static struct mdev_parent_ops intel_vgpu_ops = { > These could maybe be made const at the same time. Thanks, > > Alex Ok, let me fix. Thanks
Powered by blists - more mailing lists