Message-ID: <20191024144449.626d560b@x1.home>
Date:   Thu, 24 Oct 2019 14:44:49 -0600
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Jason Wang <jasowang@...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,
        stefanha@...hat.com
Subject: Re: [PATCH V5 4/6] mdev: introduce virtio device and its device ops

On Thu, 24 Oct 2019 11:51:35 +0800
Jason Wang <jasowang@...hat.com> wrote:

> On 2019/10/24 上午5:57, Alex Williamson wrote:
> > On Wed, 23 Oct 2019 21:07:50 +0800
> > Jason Wang <jasowang@...hat.com> wrote:
> >  
> >> This patch implements basic support for mdev driver that supports
> >> virtio transport for kernel virtio driver.
> >>
> >> Signed-off-by: Jason Wang <jasowang@...hat.com>
> >> ---
> >>   drivers/vfio/mdev/mdev_core.c    |  20 ++++
> >>   drivers/vfio/mdev/mdev_private.h |   2 +
> >>   include/linux/mdev.h             |   6 ++
> >>   include/linux/virtio_mdev_ops.h  | 159 +++++++++++++++++++++++++++++++
> >>   4 files changed, 187 insertions(+)
> >>   create mode 100644 include/linux/virtio_mdev_ops.h
> >>
> >> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> >> index 555bd61d8c38..9b00c3513120 100644
> >> --- a/drivers/vfio/mdev/mdev_core.c
> >> +++ b/drivers/vfio/mdev/mdev_core.c
> >> @@ -76,6 +76,26 @@ const struct vfio_mdev_device_ops *mdev_get_vfio_ops(struct mdev_device *mdev)
> >>   }
> >>   EXPORT_SYMBOL(mdev_get_vfio_ops);
> >>   
> >> +/* Specify the virtio device ops for the mdev device, this
> >> + * must be called during create() callback for virtio mdev device.
> >> + */
> >> +void mdev_set_virtio_ops(struct mdev_device *mdev,
> >> +			 const struct virtio_mdev_device_ops *virtio_ops)
> >> +{
> >> +	mdev_set_class(mdev, MDEV_CLASS_ID_VIRTIO);
> >> +	mdev->virtio_ops = virtio_ops;
> >> +}
> >> +EXPORT_SYMBOL(mdev_set_virtio_ops);
> >> +
> >> +/* Get the virtio device ops for the mdev device. */
> >> +const struct virtio_mdev_device_ops *
> >> +mdev_get_virtio_ops(struct mdev_device *mdev)
> >> +{
> >> +	WARN_ON(mdev->class_id != MDEV_CLASS_ID_VIRTIO);
> >> +	return mdev->virtio_ops;
> >> +}
> >> +EXPORT_SYMBOL(mdev_get_virtio_ops);
> >> +
> >>   struct device *mdev_dev(struct mdev_device *mdev)
> >>   {
> >>   	return &mdev->dev;
> >> diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h
> >> index 0770410ded2a..7b47890c34e7 100644
> >> --- a/drivers/vfio/mdev/mdev_private.h
> >> +++ b/drivers/vfio/mdev/mdev_private.h
> >> @@ -11,6 +11,7 @@
> >>   #define MDEV_PRIVATE_H
> >>   
> >>   #include <linux/vfio_mdev_ops.h>
> >> +#include <linux/virtio_mdev_ops.h>
> >>   
> >>   int  mdev_bus_register(void);
> >>   void mdev_bus_unregister(void);
> >> @@ -38,6 +39,7 @@ struct mdev_device {
> >>   	u16 class_id;
> >>   	union {
> >>   		const struct vfio_mdev_device_ops *vfio_ops;
> >> +		const struct virtio_mdev_device_ops *virtio_ops;
> >>   	};
> >>   };
> >>   
> >> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> >> index 4625f1a11014..9b69b0bbebfd 100644
> >> --- a/include/linux/mdev.h
> >> +++ b/include/linux/mdev.h
> >> @@ -17,6 +17,7 @@
> >>   
> >>   struct mdev_device;
> >>   struct vfio_mdev_device_ops;
> >> +struct virtio_mdev_device_ops;
> >>   
> >>   /*
> >>    * Called by the parent device driver to set the device which represents
> >> @@ -112,6 +113,10 @@ void mdev_set_class(struct mdev_device *mdev, u16 id);
> >>   void mdev_set_vfio_ops(struct mdev_device *mdev,
> >>   		       const struct vfio_mdev_device_ops *vfio_ops);
> >>   const struct vfio_mdev_device_ops *mdev_get_vfio_ops(struct mdev_device *mdev);
> >> +void mdev_set_virtio_ops(struct mdev_device *mdev,
> >> +			 const struct virtio_mdev_device_ops *virtio_ops);
> >> +const struct virtio_mdev_device_ops *
> >> +mdev_get_virtio_ops(struct mdev_device *mdev);
> >>   
> >>   extern struct bus_type mdev_bus_type;
> >>   
> >> @@ -127,6 +132,7 @@ struct mdev_device *mdev_from_dev(struct device *dev);
> >>   
> >>   enum {
> >>   	MDEV_CLASS_ID_VFIO = 1,
> >> +	MDEV_CLASS_ID_VIRTIO = 2,
> >>   	/* New entries must be added here */
> >>   };
> >>   
> >> diff --git a/include/linux/virtio_mdev_ops.h b/include/linux/virtio_mdev_ops.h
> >> new file mode 100644
> >> index 000000000000..d417b41f2845
> >> --- /dev/null
> >> +++ b/include/linux/virtio_mdev_ops.h
> >> @@ -0,0 +1,159 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-only */
> >> +/*
> >> + * Virtio mediated device driver
> >> + *
> >> + * Copyright 2019, Red Hat Corp.
> >> + *     Author: Jason Wang <jasowang@...hat.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 for virtio/vhost drivers.
> >> + *
> >> + * The device ops that is supported by VIRTIO_MDEV_F_VERSION_1, the
> >> + * callbacks are mandatory unless explicity mentioned.  
> > If the version of the callbacks is returned by a callback within the
> > structure defined by the version... isn't that a bit circular?  This
> > seems redundant to me versus the class id.  The fact that the parent
> > driver defines the device as MDEV_CLASS_ID_VIRTIO should tell us this
> > already.  If it was incremented, we'd need an MDEV_CLASS_ID_VIRTIOv2,
> > which the virtio-mdev bus driver could add to its id table and handle
> > differently.  
> 
> 
> My understanding is versions are only allowed to increase monotonically, 
> this seems less flexible than features. E.g we have features A, B, C, 
> mdev device can choose to support only a subset. E.g when mdev device 
> can support dirty page logging, it can add a new feature bit for driver 
> to know that it support new device ops. MDEV_CLASS_ID_VIRTIOv2 may only 
> be useful when we will invent a complete new API.

But this interface rather conflates features and versions by returning
a version as a feature.  If we simply want to say that there are no
additional features, then get_mdev_features() should return an empty
set.  If dirty page logging is a feature, then I'd expect a bit in the
get_mdev_features() return value to identify that feature.

However, I've been under the impression (perhaps wrongly) that the
class-id has a 1:1 correlation to the device-ops exposed to the bus
driver, so if dirty page logging requires extra callbacks, that would
imply a new device-ops, which requires a new class-id.  In that case
virtio-mdev would claim both class-ids and would need some way to
differentiate them.  But I also see that such a solution can become
unmanageable as the set of class-ids would need to encompass every
combination of features.

So I think what's suggested by this is that the initial struct
virtio_mdev_device_ops is a base set of callbacks which would be
extended via features?  But then why does get_generation() not make use
of this?  And if we can define get_generation() as optional and simply
test if it's implemented, then it seems we don't need any feature bits
to extend the structure (unless we're looking at binary compatibility).
So maybe get_mdev_features() is meant to expose underlying features
unrelated to the callbacks?  Which is not in line with the description?
Hopefully you can see my confusion in what we're trying to do here.
Thanks,

Alex

> >> + *
> >> + * @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)
> >> + * @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/g/s/
> >
> > Thanks,
> > Alex  
> 
> 
> Will fix.
> 
> Thanks
> 
> 
> >  
> >> + *				@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 id for the vendor that provides this device
> >> + *				@mdev: mediated device
> >> + *				Returns u32: virtio vendor id
> >> + * @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 a set of bits that demonstrate
> >> + *				thecapability of the mdev device. New
> >> + *				features bits must be added when
> >> + *				introducing new device ops.
> >> + *				@mdev: mediated device
> >> + *				Returns the mdev features (API) support by
> >> + *				the device.
> >> + * @get_generation:		Get device config generaton (optionally)
> >> + *				@mdev: mediated device
> >> + *				Returns u32: device generation
> >> + */
> >> +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);
> >> +
> >> +	/* Virtio 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);
> >> +	u32 (*get_generation)(struct mdev_device *mdev);
> >> +
> >> +	/* Mdev device ops */
> >> +	u64 (*get_mdev_features)(struct mdev_device *mdev);
> >> +};
> >> +
> >> +void mdev_set_virtio_ops(struct mdev_device *mdev,
> >> +			 const struct virtio_mdev_device_ops *virtio_ops);
> >> +
> >> +#endif  

Powered by blists - more mailing lists