Linux Virtualization - Oct 2019 - [PATCH V4 5/6] virtio: introduce a mdev based transport

On Thu, 17 Oct 2019 18:48:35 +0800
Jason Wang <jasowang at redhat.com> 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 <jasowang at redhat.com>
> ---
>  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 = &virtio_mdev_config_ops;
> +	vm_dev->mdev = mdev;
> +	INIT_LIST_HEAD(&vm_dev->virtqueues);
> +	spin_lock_init(&vm_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(&vm_dev->vdev);
> +	if (rc)
> +		put_device(dev);
> +	else
> +		dev_set_drvdata(dev, vm_dev);
> +
> +	return rc;
> +}
(...)

On 2019/10/18 ??10:20, Cornelia Huck wrote:
> On Thu, 17 Oct 2019 18:48:35 +0800
> Jason Wang <jasowang at redhat.com> 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 <jasowang at redhat.com>
>> ---
>>   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 = &virtio_mdev_config_ops;
>> +	vm_dev->mdev = mdev;
>> +	INIT_LIST_HEAD(&vm_dev->virtqueues);
>> +	spin_lock_init(&vm_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(&vm_dev->vdev);
>> +	if (rc)
>> +		put_device(dev);
>> +	else
>> +		dev_set_drvdata(dev, vm_dev);
>> +
>> +	return rc;
>> +}
> (...)

On Mon, 21 Oct 2019 13:59:23 +0800
Jason Wang <jasowang at redhat.com> wrote:
> On 2019/10/18 ??10:20, Cornelia Huck wrote:
> > On Thu, 17 Oct 2019 18:48:35 +0800
> > Jason Wang <jasowang at redhat.com> 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 <jasowang at redhat.com>
> >> ---
> >>   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 = &virtio_mdev_config_ops;
> >> +	vm_dev->mdev = mdev;
> >> +	INIT_LIST_HEAD(&vm_dev->virtqueues);
> >> +	spin_lock_init(&vm_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.