Re: [PATCH V4 5/6] virtio: introduce a mdev based transport
On 2019/10/21 下午5:36, Cornelia Huck wrote: On Mon, 21 Oct 2019 13:59:23 +0800 Jason Wang wrote: On 2019/10/18 下午10:20, Cornelia Huck wrote: On Thu, 17 Oct 2019 18:48:35 +0800 Jason Wang wrote: This patch introduces a new mdev transport for virtio. This is used to use kernel virtio driver to drive the mediated device that is capable of populating virtqueue directly. A new virtio-mdev driver will be registered to the mdev bus, when a new virtio-mdev device is probed, it will register the device with mdev based config ops. This means it is a software transport between mdev driver and mdev device. The transport was implemented through device specific ops which is a part of mdev_parent_ops now. Signed-off-by: Jason Wang --- drivers/virtio/Kconfig | 7 + drivers/virtio/Makefile | 1 + drivers/virtio/virtio_mdev.c | 409 +++ 3 files changed, 417 insertions(+) (...) +static int virtio_mdev_probe(struct device *dev) +{ + struct mdev_device *mdev = mdev_from_dev(dev); + const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev); + struct virtio_mdev_device *vm_dev; + int rc; + + vm_dev = devm_kzalloc(dev, sizeof(*vm_dev), GFP_KERNEL); + if (!vm_dev) + return -ENOMEM; + + vm_dev->vdev.dev.parent = dev; + vm_dev->vdev.dev.release = virtio_mdev_release_dev; + vm_dev->vdev.config = _mdev_config_ops; + vm_dev->mdev = mdev; + INIT_LIST_HEAD(_dev->virtqueues); + spin_lock_init(_dev->lock); + + vm_dev->version = ops->get_mdev_features(mdev); + if (vm_dev->version != VIRTIO_MDEV_F_VERSION_1) { + dev_err(dev, "VIRTIO_MDEV_F_VERSION_1 is mandatory\n"); + return -ENXIO; + } Hm, so how is that mdev features interface supposed to work? If VIRTIO_MDEV_F_VERSION_1 is a bit, I would expect this code to test for its presence, and not for identity. This should be used by driver to detect the which sets of functions and their semantics that could be provided by the device. E.g when driver support both version 2 and version 1 but device only support version 1, driver can switch to use version 1. Btw, Is there a easy way for to test its presence or do you mean doing sanity testing on existence of the mandatory ops that provided by the device? What I meant was something like: features = ops->get_mdev_features(mdev); if (features & VIRTIO_MDEV_F_VERSION_1) vm_dev->version = 1; else //moan about missing support for version 1 Can there be class id specific extra features, or is this only for core features? If the latter, maybe also do something like supported_features = ORED_LIST_OF_FEATURES; if (features & ~supported_features) //moan about extra feature bits Consider driver can claim to support a list of ids, so I this it's former. Will do as what you proposed. Thanks What will happen if we come up with a version 2? If this is backwards compatible, will both version 2 and version 1 be set? Yes, I think so, and version 2 should be considered as some extensions of version 1. If it's completely, it should use a new class id. Ok, that makes sense. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V4 5/6] virtio: introduce a mdev based transport
On Mon, 21 Oct 2019 13:59:23 +0800 Jason Wang wrote: > On 2019/10/18 下午10:20, Cornelia Huck wrote: > > On Thu, 17 Oct 2019 18:48:35 +0800 > > Jason Wang wrote: > > > >> This patch introduces a new mdev transport for virtio. This is used to > >> use kernel virtio driver to drive the mediated device that is capable > >> of populating virtqueue directly. > >> > >> A new virtio-mdev driver will be registered to the mdev bus, when a > >> new virtio-mdev device is probed, it will register the device with > >> mdev based config ops. This means it is a software transport between > >> mdev driver and mdev device. The transport was implemented through > >> device specific ops which is a part of mdev_parent_ops now. > >> > >> Signed-off-by: Jason Wang > >> --- > >> drivers/virtio/Kconfig | 7 + > >> drivers/virtio/Makefile | 1 + > >> drivers/virtio/virtio_mdev.c | 409 +++ > >> 3 files changed, 417 insertions(+) > > (...) > > > >> +static int virtio_mdev_probe(struct device *dev) > >> +{ > >> + struct mdev_device *mdev = mdev_from_dev(dev); > >> + const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev); > >> + struct virtio_mdev_device *vm_dev; > >> + int rc; > >> + > >> + vm_dev = devm_kzalloc(dev, sizeof(*vm_dev), GFP_KERNEL); > >> + if (!vm_dev) > >> + return -ENOMEM; > >> + > >> + vm_dev->vdev.dev.parent = dev; > >> + vm_dev->vdev.dev.release = virtio_mdev_release_dev; > >> + vm_dev->vdev.config = _mdev_config_ops; > >> + vm_dev->mdev = mdev; > >> + INIT_LIST_HEAD(_dev->virtqueues); > >> + spin_lock_init(_dev->lock); > >> + > >> + vm_dev->version = ops->get_mdev_features(mdev); > >> + if (vm_dev->version != VIRTIO_MDEV_F_VERSION_1) { > >> + dev_err(dev, "VIRTIO_MDEV_F_VERSION_1 is mandatory\n"); > >> + return -ENXIO; > >> + } > > Hm, so how is that mdev features interface supposed to work? If > > VIRTIO_MDEV_F_VERSION_1 is a bit, I would expect this code to test for > > its presence, and not for identity. > > > This should be used by driver to detect the which sets of functions and > their semantics that could be provided by the device. E.g when driver > support both version 2 and version 1 but device only support version 1, > driver can switch to use version 1. Btw, Is there a easy way for to test > its presence or do you mean doing sanity testing on existence of the > mandatory ops that provided by the device? What I meant was something like: features = ops->get_mdev_features(mdev); if (features & VIRTIO_MDEV_F_VERSION_1) vm_dev->version = 1; else //moan about missing support for version 1 Can there be class id specific extra features, or is this only for core features? If the latter, maybe also do something like supported_features = ORED_LIST_OF_FEATURES; if (features & ~supported_features) //moan about extra feature bits > > > > > > What will happen if we come up with a version 2? If this is backwards > > compatible, will both version 2 and version 1 be set? > > > Yes, I think so, and version 2 should be considered as some extensions > of version 1. If it's completely, it should use a new class id. Ok, that makes sense. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V4 5/6] virtio: introduce a mdev based transport
On 2019/10/18 下午10:20, Cornelia Huck wrote: On Thu, 17 Oct 2019 18:48:35 +0800 Jason Wang wrote: This patch introduces a new mdev transport for virtio. This is used to use kernel virtio driver to drive the mediated device that is capable of populating virtqueue directly. A new virtio-mdev driver will be registered to the mdev bus, when a new virtio-mdev device is probed, it will register the device with mdev based config ops. This means it is a software transport between mdev driver and mdev device. The transport was implemented through device specific ops which is a part of mdev_parent_ops now. Signed-off-by: Jason Wang --- drivers/virtio/Kconfig | 7 + drivers/virtio/Makefile | 1 + drivers/virtio/virtio_mdev.c | 409 +++ 3 files changed, 417 insertions(+) (...) +static int virtio_mdev_probe(struct device *dev) +{ + struct mdev_device *mdev = mdev_from_dev(dev); + const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev); + struct virtio_mdev_device *vm_dev; + int rc; + + vm_dev = devm_kzalloc(dev, sizeof(*vm_dev), GFP_KERNEL); + if (!vm_dev) + return -ENOMEM; + + vm_dev->vdev.dev.parent = dev; + vm_dev->vdev.dev.release = virtio_mdev_release_dev; + vm_dev->vdev.config = _mdev_config_ops; + vm_dev->mdev = mdev; + INIT_LIST_HEAD(_dev->virtqueues); + spin_lock_init(_dev->lock); + + vm_dev->version = ops->get_mdev_features(mdev); + if (vm_dev->version != VIRTIO_MDEV_F_VERSION_1) { + dev_err(dev, "VIRTIO_MDEV_F_VERSION_1 is mandatory\n"); + return -ENXIO; + } Hm, so how is that mdev features interface supposed to work? If VIRTIO_MDEV_F_VERSION_1 is a bit, I would expect this code to test for its presence, and not for identity. This should be used by driver to detect the which sets of functions and their semantics that could be provided by the device. E.g when driver support both version 2 and version 1 but device only support version 1, driver can switch to use version 1. Btw, Is there a easy way for to test its presence or do you mean doing sanity testing on existence of the mandatory ops that provided by the device? What will happen if we come up with a version 2? If this is backwards compatible, will both version 2 and version 1 be set? Yes, I think so, and version 2 should be considered as some extensions of version 1. If it's completely, it should use a new class id. Thanks + + vm_dev->vdev.id.device = ops->get_device_id(mdev); + if (vm_dev->vdev.id.device == 0) + return -ENODEV; + + vm_dev->vdev.id.vendor = ops->get_vendor_id(mdev); + rc = register_virtio_device(_dev->vdev); + if (rc) + put_device(dev); + else + dev_set_drvdata(dev, vm_dev); + + return rc; +} (...) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V4 5/6] virtio: introduce a mdev based transport
On Thu, 17 Oct 2019 18:48:35 +0800 Jason Wang wrote: > This patch introduces a new mdev transport for virtio. This is used to > use kernel virtio driver to drive the mediated device that is capable > of populating virtqueue directly. > > A new virtio-mdev driver will be registered to the mdev bus, when a > new virtio-mdev device is probed, it will register the device with > mdev based config ops. This means it is a software transport between > mdev driver and mdev device. The transport was implemented through > device specific ops which is a part of mdev_parent_ops now. > > Signed-off-by: Jason Wang > --- > drivers/virtio/Kconfig | 7 + > drivers/virtio/Makefile | 1 + > drivers/virtio/virtio_mdev.c | 409 +++ > 3 files changed, 417 insertions(+) (...) > +static int virtio_mdev_probe(struct device *dev) > +{ > + struct mdev_device *mdev = mdev_from_dev(dev); > + const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev); > + struct virtio_mdev_device *vm_dev; > + int rc; > + > + vm_dev = devm_kzalloc(dev, sizeof(*vm_dev), GFP_KERNEL); > + if (!vm_dev) > + return -ENOMEM; > + > + vm_dev->vdev.dev.parent = dev; > + vm_dev->vdev.dev.release = virtio_mdev_release_dev; > + vm_dev->vdev.config = _mdev_config_ops; > + vm_dev->mdev = mdev; > + INIT_LIST_HEAD(_dev->virtqueues); > + spin_lock_init(_dev->lock); > + > + vm_dev->version = ops->get_mdev_features(mdev); > + if (vm_dev->version != VIRTIO_MDEV_F_VERSION_1) { > + dev_err(dev, "VIRTIO_MDEV_F_VERSION_1 is mandatory\n"); > + return -ENXIO; > + } Hm, so how is that mdev features interface supposed to work? If VIRTIO_MDEV_F_VERSION_1 is a bit, I would expect this code to test for its presence, and not for identity. What will happen if we come up with a version 2? If this is backwards compatible, will both version 2 and version 1 be set? > + > + vm_dev->vdev.id.device = ops->get_device_id(mdev); > + if (vm_dev->vdev.id.device == 0) > + return -ENODEV; > + > + vm_dev->vdev.id.vendor = ops->get_vendor_id(mdev); > + rc = register_virtio_device(_dev->vdev); > + if (rc) > + put_device(dev); > + else > + dev_set_drvdata(dev, vm_dev); > + > + return rc; > +} (...) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH V4 5/6] virtio: introduce a mdev based transport
This patch introduces a new mdev transport for virtio. This is used to use kernel virtio driver to drive the mediated device that is capable of populating virtqueue directly. A new virtio-mdev driver will be registered to the mdev bus, when a new virtio-mdev device is probed, it will register the device with mdev based config ops. This means it is a software transport between mdev driver and mdev device. The transport was implemented through device specific ops which is a part of mdev_parent_ops now. Signed-off-by: Jason Wang --- drivers/virtio/Kconfig | 7 + drivers/virtio/Makefile | 1 + drivers/virtio/virtio_mdev.c | 409 +++ 3 files changed, 417 insertions(+) create mode 100644 drivers/virtio/virtio_mdev.c diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig index 078615cf2afc..8d18722ab572 100644 --- a/drivers/virtio/Kconfig +++ b/drivers/virtio/Kconfig @@ -43,6 +43,13 @@ config VIRTIO_PCI_LEGACY If unsure, say Y. +config VIRTIO_MDEV_DEVICE + tristate "VIRTIO driver for Mediated devices" + depends on VFIO_MDEV && VIRTIO + default n + help + VIRTIO based driver for Mediated devices. + config VIRTIO_PMEM tristate "Support for virtio pmem driver" depends on VIRTIO diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile index 3a2b5c5dcf46..ebc7fa15ae82 100644 --- a/drivers/virtio/Makefile +++ b/drivers/virtio/Makefile @@ -6,3 +6,4 @@ virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o +obj-$(CONFIG_VIRTIO_MDEV_DEVICE) += virtio_mdev.o diff --git a/drivers/virtio/virtio_mdev.c b/drivers/virtio/virtio_mdev.c new file mode 100644 index ..24c0df5ffe77 --- /dev/null +++ b/drivers/virtio/virtio_mdev.c @@ -0,0 +1,409 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * VIRTIO based driver for Mediated device + * + * Copyright (c) 2019, Red Hat. All rights reserved. + * Author: Jason Wang + * + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define DRIVER_VERSION "0.1" +#define DRIVER_AUTHOR "Red Hat Corporation" +#define DRIVER_DESC "VIRTIO based driver for Mediated device" + +#define to_virtio_mdev_device(dev) \ + container_of(dev, struct virtio_mdev_device, vdev) + +struct virtio_mdev_device { + struct virtio_device vdev; + struct mdev_device *mdev; + unsigned long version; + + /* The lock to protect virtqueue list */ + spinlock_t lock; + /* List of virtio_mdev_vq_info */ + struct list_head virtqueues; +}; + +struct virtio_mdev_vq_info { + /* the actual virtqueue */ + struct virtqueue *vq; + + /* the list node for the virtqueues list */ + struct list_head node; +}; + +static struct mdev_device *vm_get_mdev(struct virtio_device *vdev) +{ + struct virtio_mdev_device *vm_dev = to_virtio_mdev_device(vdev); + struct mdev_device *mdev = vm_dev->mdev; + + return mdev; +} + +static void virtio_mdev_get(struct virtio_device *vdev, unsigned offset, + void *buf, unsigned len) +{ + struct mdev_device *mdev = vm_get_mdev(vdev); + const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev); + + ops->get_config(mdev, offset, buf, len); +} + +static void virtio_mdev_set(struct virtio_device *vdev, unsigned offset, + const void *buf, unsigned len) +{ + struct mdev_device *mdev = vm_get_mdev(vdev); + const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev); + + ops->set_config(mdev, offset, buf, len); +} + +static u32 virtio_mdev_generation(struct virtio_device *vdev) +{ + struct mdev_device *mdev = vm_get_mdev(vdev); + const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev); + + return ops->get_generation(mdev); +} + +static u8 virtio_mdev_get_status(struct virtio_device *vdev) +{ + struct mdev_device *mdev = vm_get_mdev(vdev); + const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev); + + return ops->get_status(mdev); +} + +static void virtio_mdev_set_status(struct virtio_device *vdev, u8 status) +{ + struct mdev_device *mdev = vm_get_mdev(vdev); + const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev); + + return ops->set_status(mdev, status); +} + +static void virtio_mdev_reset(struct virtio_device *vdev) +{ + struct mdev_device *mdev = vm_get_mdev(vdev); + const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev); + + return ops->set_status(mdev, 0); +} + +static bool virtio_mdev_notify(struct virtqueue *vq) +{ + struct mdev_device *mdev = vm_get_mdev(vq->vdev); + const struct virtio_mdev_device_ops *ops =