Linux Virtualization - Oct 2019 - [PATCH V4 4/6] mdev: introduce virtio device and its device ops

On Thu, 17 Oct 2019 18:48:34 +0800
Jason Wang <jasowang at redhat.com> wrote:
> This patch implements basic support for mdev driver that supports
> virtio transport for kernel virtio driver.
> 
> Signed-off-by: Jason Wang <jasowang at redhat.com>
> ---
>  drivers/vfio/mdev/mdev_core.c |  12 +++
>  include/linux/mdev.h          |   4 +
>  include/linux/virtio_mdev.h   | 151 ++++++++++++++++++++++++++++++++++
>  3 files changed, 167 insertions(+)
>  create mode 100644 include/linux/virtio_mdev.h
> 
> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> index d0f3113c8071..5834f6b7c7a5 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -57,6 +57,18 @@ void mdev_set_vfio_ops(struct mdev_device *mdev,
>  }
>  EXPORT_SYMBOL(mdev_set_vfio_ops);
>  
> +/* Specify the virtio device ops for the mdev device, this
> + * must be called during create() callback for virtio mdev device.
> + */
Change this as for the vfio comment (last patch?)
> +void mdev_set_virtio_ops(struct mdev_device *mdev,
> +			 const struct virtio_mdev_device_ops *virtio_ops)
> +{
> +	BUG_ON(mdev->class_id);
s/BUG_ON/WARN_ON/
> +	mdev->class_id = MDEV_CLASS_ID_VIRTIO;
> +	mdev->device_ops = virtio_ops;
> +}
> +EXPORT_SYMBOL(mdev_set_virtio_ops);
> +
>  const void *mdev_get_dev_ops(struct mdev_device *mdev)
>  {
>  	return mdev->device_ops;
(...)
> diff --git a/include/linux/virtio_mdev.h b/include/linux/virtio_mdev.h
> new file mode 100644
> index 000000000000..b965b50f9b24
> --- /dev/null
> +++ b/include/linux/virtio_mdev.h
> @@ -0,0 +1,151 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Virtio mediated device driver
> + *
> + * Copyright 2019, Red Hat Corp.
> + *     Author: Jason Wang <jasowang at redhat.com>
> + */
> +#ifndef _LINUX_VIRTIO_MDEV_H
> +#define _LINUX_VIRTIO_MDEV_H
> +
> +#include <linux/interrupt.h>
> +#include <linux/mdev.h>
> +#include <uapi/linux/vhost.h>
> +
> +#define VIRTIO_MDEV_DEVICE_API_STRING		"virtio-mdev"
> +#define VIRTIO_MDEV_F_VERSION_1 0x1
> +
> +struct virtio_mdev_callback {
> +	irqreturn_t (*callback)(void *data);
> +	void *private;
> +};
> +
> +/**
> + * struct vfio_mdev_device_ops - Structure to be registered for each
> + * mdev device to register the device to virtio-mdev module.
> + *
> + * @set_vq_address:		Set the address of virtqueue
> + *				@mdev: mediated device
> + *				@idx: virtqueue index
> + *				@desc_area: address of desc area
> + *				@driver_area: address of driver area
> + *				@device_area: address of device area
> + *				Returns integer: success (0) or error (< 0)
Probably dumb question (have not read the next patches yet): Is this
agnostic regarding split or packed queues?
> + * @set_vq_num:		Set the size of virtqueue
> + *				@mdev: mediated device
> + *				@idx: virtqueue index
> + *				@num: the size of virtqueue
> + * @kick_vq:			Kick the virtqueue
> + *				@mdev: mediated device
> + *				@idx: virtqueue index
> + * @set_vq_cb:			Set the interrupt callback function for
> + *				a virtqueue
> + *				@mdev: mediated device
> + *				@idx: virtqueue index
> + *				@cb: virtio-mdev interrupt callback structure
> + * @set_vq_ready:		Set ready status for a virtqueue
> + *				@mdev: mediated device
> + *				@idx: virtqueue index
> + *				@ready: ready (true) not ready(false)
> + * @get_vq_ready:		Get ready status for a virtqueue
> + *				@mdev: mediated device
> + *				@idx: virtqueue index
> + *				Returns boolean: ready (true) or not (false)
> + * @set_vq_state:		Set the state for a virtqueue
> + *				@mdev: mediated device
> + *				@idx: virtqueue index
> + *				@state: virtqueue state (last_avail_idx)
> + *				Returns integer: success (0) or error (< 0)
> + * @get_vq_state:		Get the state for a virtqueue
> + *				@mdev: mediated device
> + *				@idx: virtqueue index
> + *				Returns virtqueue state (last_avail_idx)
> + * @get_vq_align:		Get the virtqueue align requirement
> + *				for the device
> + *				@mdev: mediated device
> + *				Returns virtqueue algin requirement
> + * @get_features:		Get virtio features supported by the device
> + *				@mdev: mediated device
> + *				Returns the virtio features support by the
> + *				device
> + * @get_features:		Set virtio features supported by the driver
s/get_features/set_features/
> + *				@mdev: mediated device
> + *				@features: feature support by the driver
> + *				Returns integer: success (0) or error (< 0)
> + * @set_config_cb:		Set the config interrupt callback
> + *				@mdev: mediated device
> + *				@cb: virtio-mdev interrupt callback structure
> + * @get_vq_num_max:		Get the max size of virtqueue
> + *				@mdev: mediated device
> + *				Returns u16: max size of virtqueue
> + * @get_device_id:		Get virtio device id
> + *				@mdev: mediated device
> + *				Returns u32: virtio device id
> + * @get_vendor_id:		Get virtio vendor id
> + *				@mdev: mediated device
> + *				Returns u32: virtio vendor id
How is the vendor id defined? As for normal virtio-pci devices?
> + * @get_status: 		Get the device status
> + *				@mdev: mediated device
> + *				Returns u8: virtio device status
> + * @set_status: 		Set the device status
> + *				@mdev: mediated device
> + *				@status: virtio device status
> + * @get_config: 		Read from device specific configuration space
> + *				@mdev: mediated device
> + *				@offset: offset from the beginning of
> + *				configuration space
> + *				@buf: buffer used to read to
> + *				@len: the length to read from
> + *				configration space
> + * @set_config: 		Write to device specific configuration space
> + *				@mdev: mediated device
> + *				@offset: offset from the beginning of
> + *				configuration space
> + *				@buf: buffer used to write from
> + *				@len: the length to write to
> + *				configration space
> + * @get_mdev_features:		Get the feature of virtio mdev device
> + *				@mdev: mediated device
> + *				Returns the mdev features (API) support by
> + *				the device.
What kind of 'features' are supposed to go in there? Are these bits,
like you defined for VIRTIO_MDEV_F_VERSION_1 above?
> + * @get_generation:		Get device generaton
> + *				@mdev: mediated device
> + *				Returns u32: device generation
Is that callback mandatory?
> + */
> +struct virtio_mdev_device_ops {
> +	/* Virtqueue ops */
> +	int (*set_vq_address)(struct mdev_device *mdev,
> +			      u16 idx, u64 desc_area, u64 driver_area,
> +			      u64 device_area);
> +	void (*set_vq_num)(struct mdev_device *mdev, u16 idx, u32 num);
> +	void (*kick_vq)(struct mdev_device *mdev, u16 idx);
> +	void (*set_vq_cb)(struct mdev_device *mdev, u16 idx,
> +			  struct virtio_mdev_callback *cb);
> +	void (*set_vq_ready)(struct mdev_device *mdev, u16 idx, bool ready);
> +	bool (*get_vq_ready)(struct mdev_device *mdev, u16 idx);
> +	int (*set_vq_state)(struct mdev_device *mdev, u16 idx, u64 state);
> +	u64 (*get_vq_state)(struct mdev_device *mdev, u16 idx);
> +
> +	/* Device ops */
> +	u16 (*get_vq_align)(struct mdev_device *mdev);
> +	u64 (*get_features)(struct mdev_device *mdev);
> +	int (*set_features)(struct mdev_device *mdev, u64 features);
> +	void (*set_config_cb)(struct mdev_device *mdev,
> +			      struct virtio_mdev_callback *cb);
> +	u16 (*get_vq_num_max)(struct mdev_device *mdev);
> +	u32 (*get_device_id)(struct mdev_device *mdev);
> +	u32 (*get_vendor_id)(struct mdev_device *mdev);
> +	u8 (*get_status)(struct mdev_device *mdev);
> +	void (*set_status)(struct mdev_device *mdev, u8 status);
> +	void (*get_config)(struct mdev_device *mdev, unsigned int offset,
> +			   void *buf, unsigned int len);
> +	void (*set_config)(struct mdev_device *mdev, unsigned int offset,
> +			   const void *buf, unsigned int len);
> +	u64 (*get_mdev_features)(struct mdev_device *mdev);
> +	u32 (*get_generation)(struct mdev_device *mdev);
> +};
> +
> +void mdev_set_virtio_ops(struct mdev_device *mdev,
> +			 const struct virtio_mdev_device_ops *virtio_ops);
> +
> +#endif

On 2019/10/18 ??5:46, Cornelia Huck wrote:
> On Thu, 17 Oct 2019 18:48:34 +0800
> Jason Wang <jasowang at redhat.com> wrote:
>
>> This patch implements basic support for mdev driver that supports
>> virtio transport for kernel virtio driver.
>>
>> Signed-off-by: Jason Wang <jasowang at redhat.com>
>> ---
>>   drivers/vfio/mdev/mdev_core.c |  12 +++
>>   include/linux/mdev.h          |   4 +
>>   include/linux/virtio_mdev.h   | 151
++++++++++++++++++++++++++++++++++
>>   3 files changed, 167 insertions(+)
>>   create mode 100644 include/linux/virtio_mdev.h
>>
>> diff --git a/drivers/vfio/mdev/mdev_core.c
b/drivers/vfio/mdev/mdev_core.c
>> index d0f3113c8071..5834f6b7c7a5 100644
>> --- a/drivers/vfio/mdev/mdev_core.c
>> +++ b/drivers/vfio/mdev/mdev_core.c
>> @@ -57,6 +57,18 @@ void mdev_set_vfio_ops(struct mdev_device *mdev,
>>   }
>>   EXPORT_SYMBOL(mdev_set_vfio_ops);
>>   
>> +/* Specify the virtio device ops for the mdev device, this
>> + * must be called during create() callback for virtio mdev device.
>> + */
> Change this as for the vfio comment (last patch?)

Ok.

>
>> +void mdev_set_virtio_ops(struct mdev_device *mdev,
>> +			 const struct virtio_mdev_device_ops *virtio_ops)
>> +{
>> +	BUG_ON(mdev->class_id);
> s/BUG_ON/WARN_ON/

Yes.

>
>> +	mdev->class_id = MDEV_CLASS_ID_VIRTIO;
>> +	mdev->device_ops = virtio_ops;
>> +}
>> +EXPORT_SYMBOL(mdev_set_virtio_ops);
>> +
>>   const void *mdev_get_dev_ops(struct mdev_device *mdev)
>>   {
>>   	return mdev->device_ops;
> (...)
>
>> diff --git a/include/linux/virtio_mdev.h b/include/linux/virtio_mdev.h
>> new file mode 100644
>> index 000000000000..b965b50f9b24
>> --- /dev/null
>> +++ b/include/linux/virtio_mdev.h
>> @@ -0,0 +1,151 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Virtio mediated device driver
>> + *
>> + * Copyright 2019, Red Hat Corp.
>> + *     Author: Jason Wang <jasowang at redhat.com>
>> + */
>> +#ifndef _LINUX_VIRTIO_MDEV_H
>> +#define _LINUX_VIRTIO_MDEV_H
>> +
>> +#include <linux/interrupt.h>
>> +#include <linux/mdev.h>
>> +#include <uapi/linux/vhost.h>
>> +
>> +#define VIRTIO_MDEV_DEVICE_API_STRING		"virtio-mdev"
>> +#define VIRTIO_MDEV_F_VERSION_1 0x1
>> +
>> +struct virtio_mdev_callback {
>> +	irqreturn_t (*callback)(void *data);
>> +	void *private;
>> +};
>> +
>> +/**
>> + * struct vfio_mdev_device_ops - Structure to be registered for each
>> + * mdev device to register the device to virtio-mdev module.
>> + *
>> + * @set_vq_address:		Set the address of virtqueue
>> + *				@mdev: mediated device
>> + *				@idx: virtqueue index
>> + *				@desc_area: address of desc area
>> + *				@driver_area: address of driver area
>> + *				@device_area: address of device area
>> + *				Returns integer: success (0) or error (< 0)
> Probably dumb question (have not read the next patches yet): Is this
> agnostic regarding split or packed queues?

Yes, it is to and to be more obvious, I use the terminology from recent 
spec.

>
>> + * @set_vq_num:		Set the size of virtqueue
>> + *				@mdev: mediated device
>> + *				@idx: virtqueue index
>> + *				@num: the size of virtqueue
>> + * @kick_vq:			Kick the virtqueue
>> + *				@mdev: mediated device
>> + *				@idx: virtqueue index
>> + * @set_vq_cb:			Set the interrupt callback function for
>> + *				a virtqueue
>> + *				@mdev: mediated device
>> + *				@idx: virtqueue index
>> + *				@cb: virtio-mdev interrupt callback structure
>> + * @set_vq_ready:		Set ready status for a virtqueue
>> + *				@mdev: mediated device
>> + *				@idx: virtqueue index
>> + *				@ready: ready (true) not ready(false)
>> + * @get_vq_ready:		Get ready status for a virtqueue
>> + *				@mdev: mediated device
>> + *				@idx: virtqueue index
>> + *				Returns boolean: ready (true) or not (false)
>> + * @set_vq_state:		Set the state for a virtqueue
>> + *				@mdev: mediated device
>> + *				@idx: virtqueue index
>> + *				@state: virtqueue state (last_avail_idx)
>> + *				Returns integer: success (0) or error (< 0)
>> + * @get_vq_state:		Get the state for a virtqueue
>> + *				@mdev: mediated device
>> + *				@idx: virtqueue index
>> + *				Returns virtqueue state (last_avail_idx)
>> + * @get_vq_align:		Get the virtqueue align requirement
>> + *				for the device
>> + *				@mdev: mediated device
>> + *				Returns virtqueue algin requirement
>> + * @get_features:		Get virtio features supported by the device
>> + *				@mdev: mediated device
>> + *				Returns the virtio features support by the
>> + *				device
>> + * @get_features:		Set virtio features supported by the driver
> s/get_features/set_features/

Will fix.

>
>> + *				@mdev: mediated device
>> + *				@features: feature support by the driver
>> + *				Returns integer: success (0) or error (< 0)
>> + * @set_config_cb:		Set the config interrupt callback
>> + *				@mdev: mediated device
>> + *				@cb: virtio-mdev interrupt callback structure
>> + * @get_vq_num_max:		Get the max size of virtqueue
>> + *				@mdev: mediated device
>> + *				Returns u16: max size of virtqueue
>> + * @get_device_id:		Get virtio device id
>> + *				@mdev: mediated device
>> + *				Returns u32: virtio device id
>> + * @get_vendor_id:		Get virtio vendor id
>> + *				@mdev: mediated device
>> + *				Returns u32: virtio vendor id
> How is the vendor id defined? As for normal virtio-pci devices?

The vendor that provides this device. So something like this

I notice that MMIO also had this so it looks to me it's not pci specific.

>
>> + * @get_status: 		Get the device status
>> + *				@mdev: mediated device
>> + *				Returns u8: virtio device status
>> + * @set_status: 		Set the device status
>> + *				@mdev: mediated device
>> + *				@status: virtio device status
>> + * @get_config: 		Read from device specific configuration space
>> + *				@mdev: mediated device
>> + *				@offset: offset from the beginning of
>> + *				configuration space
>> + *				@buf: buffer used to read to
>> + *				@len: the length to read from
>> + *				configration space
>> + * @set_config: 		Write to device specific configuration space
>> + *				@mdev: mediated device
>> + *				@offset: offset from the beginning of
>> + *				configuration space
>> + *				@buf: buffer used to write from
>> + *				@len: the length to write to
>> + *				configration space
>> + * @get_mdev_features:		Get the feature of virtio mdev device
>> + *				@mdev: mediated device
>> + *				Returns the mdev features (API) support by
>> + *				the device.
> What kind of 'features' are supposed to go in there? Are these
bits,
> like you defined for VIRTIO_MDEV_F_VERSION_1 above?

It's the API or mdev features other than virtio features. It could be 
used by driver to determine the capability of the mdev device. Besides 
_F_VERSION_1, we may add dirty page tracking etc which means we need new 
device ops.

>
>> + * @get_generation:		Get device generaton
>> + *				@mdev: mediated device
>> + *				Returns u32: device generation
> Is that callback mandatory?

I think so, it's hard to emulate that completely in virtio-mdev transport.

Thanks

>
>> + */
>> +struct virtio_mdev_device_ops {
>> +	/* Virtqueue ops */
>> +	int (*set_vq_address)(struct mdev_device *mdev,
>> +			      u16 idx, u64 desc_area, u64 driver_area,
>> +			      u64 device_area);
>> +	void (*set_vq_num)(struct mdev_device *mdev, u16 idx, u32 num);
>> +	void (*kick_vq)(struct mdev_device *mdev, u16 idx);
>> +	void (*set_vq_cb)(struct mdev_device *mdev, u16 idx,
>> +			  struct virtio_mdev_callback *cb);
>> +	void (*set_vq_ready)(struct mdev_device *mdev, u16 idx, bool ready);
>> +	bool (*get_vq_ready)(struct mdev_device *mdev, u16 idx);
>> +	int (*set_vq_state)(struct mdev_device *mdev, u16 idx, u64 state);
>> +	u64 (*get_vq_state)(struct mdev_device *mdev, u16 idx);
>> +
>> +	/* Device ops */
>> +	u16 (*get_vq_align)(struct mdev_device *mdev);
>> +	u64 (*get_features)(struct mdev_device *mdev);
>> +	int (*set_features)(struct mdev_device *mdev, u64 features);
>> +	void (*set_config_cb)(struct mdev_device *mdev,
>> +			      struct virtio_mdev_callback *cb);
>> +	u16 (*get_vq_num_max)(struct mdev_device *mdev);
>> +	u32 (*get_device_id)(struct mdev_device *mdev);
>> +	u32 (*get_vendor_id)(struct mdev_device *mdev);
>> +	u8 (*get_status)(struct mdev_device *mdev);
>> +	void (*set_status)(struct mdev_device *mdev, u8 status);
>> +	void (*get_config)(struct mdev_device *mdev, unsigned int offset,
>> +			   void *buf, unsigned int len);
>> +	void (*set_config)(struct mdev_device *mdev, unsigned int offset,
>> +			   const void *buf, unsigned int len);
>> +	u64 (*get_mdev_features)(struct mdev_device *mdev);
>> +	u32 (*get_generation)(struct mdev_device *mdev);
>> +};
>> +
>> +void mdev_set_virtio_ops(struct mdev_device *mdev,
>> +			 const struct virtio_mdev_device_ops *virtio_ops);
>> +
>> +#endif

On Fri, 18 Oct 2019 18:55:02 +0800
Jason Wang <jasowang at redhat.com> wrote:
> On 2019/10/18 ??5:46, Cornelia Huck wrote:
> > On Thu, 17 Oct 2019 18:48:34 +0800
> > Jason Wang <jasowang at redhat.com> wrote:
> >> + * @get_vendor_id:		Get virtio vendor id
> >> + *				@mdev: mediated device
> >> + *				Returns u32: virtio vendor id  
> > How is the vendor id defined? As for normal virtio-pci devices?  
> 
> 
> The vendor that provides this device. So something like this
> 
> I notice that MMIO also had this so it looks to me it's not pci
specific.
Ok. Would be good to specify this more explicitly.
> 
> 
> >  
> >> + * @get_status: 		Get the device status
> >> + *				@mdev: mediated device
> >> + *				Returns u8: virtio device status
> >> + * @set_status: 		Set the device status
> >> + *				@mdev: mediated device
> >> + *				@status: virtio device status
> >> + * @get_config: 		Read from device specific configuration space
> >> + *				@mdev: mediated device
> >> + *				@offset: offset from the beginning of
> >> + *				configuration space
> >> + *				@buf: buffer used to read to
> >> + *				@len: the length to read from
> >> + *				configration space
> >> + * @set_config: 		Write to device specific configuration space
> >> + *				@mdev: mediated device
> >> + *				@offset: offset from the beginning of
> >> + *				configuration space
> >> + *				@buf: buffer used to write from
> >> + *				@len: the length to write to
> >> + *				configration space
> >> + * @get_mdev_features:		Get the feature of virtio mdev device
> >> + *				@mdev: mediated device
> >> + *				Returns the mdev features (API) support by
> >> + *				the device.  
> > What kind of 'features' are supposed to go in there? Are these
bits,
> > like you defined for VIRTIO_MDEV_F_VERSION_1 above?  
> 
> 
> It's the API or mdev features other than virtio features. It could be 
> used by driver to determine the capability of the mdev device. Besides 
> _F_VERSION_1, we may add dirty page tracking etc which means we need new 
> device ops.
Ok, so that's supposed to be distinct bits that can be or'ed together?
Makes sense, but probably needs some more documentation somewhere.
> 
> 
> >  
> >> + * @get_generation:		Get device generaton
> >> + *				@mdev: mediated device
> >> + *				Returns u32: device generation  
> > Is that callback mandatory?  
> 
> 
> I think so, it's hard to emulate that completely in virtio-mdev
transport.
IIRC, the generation stuff is not mandatory in the current version of
virtio, as not all transports have that concept.

Generally, are any of the callbacks optional, or are all of them
mandatory? From what I understand, you plan to add new things that
depend on features... would that mean non-mandatory callbacks?