thr3ads.net - Linux Virtualization - [RFC PATCH 3/4] virtio: introudce a mdev based transport [Sep 2019]

If this information is useful, please help other people find it:
Share via:

Michael S. Tsirkin

2019-Sep-10 13:52 UTC

[RFC PATCH 3/4] virtio: introudce a mdev based transport

On Tue, Sep 10, 2019 at 09:13:02PM +0800, Jason Wang
wrote:> 
> On 2019/9/10 ??6:01, Michael S. Tsirkin wrote:
> > > +#ifndef _LINUX_VIRTIO_MDEV_H
> > > +#define _LINUX_VIRTIO_MDEV_H
> > > +
> > > +#include <linux/interrupt.h>
> > > +#include <linux/vringh.h>
> > > +#include <uapi/linux/virtio_net.h>
> > > +
> > > +/*
> > > + * Ioctls
> > > + */
> > Pls add a bit more content here. It's redundant to state these
> > are ioctls. Much better to document what does each one do.
> 
> 
> Ok.
> 
> 
> > 
> > > +
> > > +struct virtio_mdev_callback {
> > > +	irqreturn_t (*callback)(void *);
> > > +	void *private;
> > > +};
> > > +
> > > +#define VIRTIO_MDEV 0xAF
> > > +#define VIRTIO_MDEV_SET_VQ_CALLBACK _IOW(VIRTIO_MDEV, 0x00, \
> > > +					 struct virtio_mdev_callback)
> > > +#define VIRTIO_MDEV_SET_CONFIG_CALLBACK _IOW(VIRTIO_MDEV, 0x01,
\
> > > +					struct virtio_mdev_callback)
> > Function pointer in an ioctl parameter? How does this ever make sense?
> 
> 
> I admit this is hacky (casting).
> 
> 
> > And can't we use a couple of registers for this, and avoid ioctls?
> 
> 
> Yes, how about something like interrupt numbers for each virtqueue and
> config?
Should we just reuse VIRTIO_PCI_COMMON_Q_XXX then?

> 
> > 
> > > +
> > > +#define VIRTIO_MDEV_DEVICE_API_STRING		"virtio-mdev"
> > > +
> > > +/*
> > > + * Control registers
> > > + */
> > > +
> > > +/* Magic value ("virt" string) - Read Only */
> > > +#define VIRTIO_MDEV_MAGIC_VALUE		0x000
> > > +
> > > +/* Virtio device version - Read Only */
> > > +#define VIRTIO_MDEV_VERSION		0x004
> > > +
> > > +/* Virtio device ID - Read Only */
> > > +#define VIRTIO_MDEV_DEVICE_ID		0x008
> > > +
> > > +/* Virtio vendor ID - Read Only */
> > > +#define VIRTIO_MDEV_VENDOR_ID		0x00c
> > > +
> > > +/* Bitmask of the features supported by the device (host)
> > > + * (32 bits per set) - Read Only */
> > > +#define VIRTIO_MDEV_DEVICE_FEATURES	0x010
> > > +
> > > +/* Device (host) features set selector - Write Only */
> > > +#define VIRTIO_MDEV_DEVICE_FEATURES_SEL	0x014
> > > +
> > > +/* Bitmask of features activated by the driver (guest)
> > > + * (32 bits per set) - Write Only */
> > > +#define VIRTIO_MDEV_DRIVER_FEATURES	0x020
> > > +
> > > +/* Activated features set selector - Write Only */
> > > +#define VIRTIO_MDEV_DRIVER_FEATURES_SEL	0x024
> > > +
> > > +/* Queue selector - Write Only */
> > > +#define VIRTIO_MDEV_QUEUE_SEL		0x030
> > > +
> > > +/* Maximum size of the currently selected queue - Read Only */
> > > +#define VIRTIO_MDEV_QUEUE_NUM_MAX	0x034
> > > +
> > > +/* Queue size for the currently selected queue - Write Only */
> > > +#define VIRTIO_MDEV_QUEUE_NUM		0x038
> > > +
> > > +/* Ready bit for the currently selected queue - Read Write */
> > > +#define VIRTIO_MDEV_QUEUE_READY		0x044
> > Is this same as started?
> 
> 
> Do you mean "status"?
I really meant "enabled", didn't remember the correct name.
As in:  VIRTIO_PCI_COMMON_Q_ENABLE
> 
> > 
> > > +
> > > +/* Alignment of virtqueue - Read Only */
> > > +#define VIRTIO_MDEV_QUEUE_ALIGN		0x048
> > > +
> > > +/* Queue notifier - Write Only */
> > > +#define VIRTIO_MDEV_QUEUE_NOTIFY	0x050
> > > +
> > > +/* Device status register - Read Write */
> > > +#define VIRTIO_MDEV_STATUS		0x060
> > > +
> > > +/* Selected queue's Descriptor Table address, 64 bits in two
halves */
> > > +#define VIRTIO_MDEV_QUEUE_DESC_LOW	0x080
> > > +#define VIRTIO_MDEV_QUEUE_DESC_HIGH	0x084
> > > +
> > > +/* Selected queue's Available Ring address, 64 bits in two
halves */
> > > +#define VIRTIO_MDEV_QUEUE_AVAIL_LOW	0x090
> > > +#define VIRTIO_MDEV_QUEUE_AVAIL_HIGH	0x094
> > > +
> > > +/* Selected queue's Used Ring address, 64 bits in two halves
*/
> > > +#define VIRTIO_MDEV_QUEUE_USED_LOW	0x0a0
> > > +#define VIRTIO_MDEV_QUEUE_USED_HIGH	0x0a4
> > > +
> > > +/* Configuration atomicity value */
> > > +#define VIRTIO_MDEV_CONFIG_GENERATION	0x0fc
> > > +
> > > +/* The config space is defined by each driver as
> > > + * the per-driver configuration space - Read Write */
> > > +#define VIRTIO_MDEV_CONFIG		0x100
> > Mixing device and generic config space is what virtio pci did,
> > caused lots of problems with extensions.
> > It would be better to reserve much more space.
> 
> 
> I see, will do this.
> 
> Thanks
> 
> 
> > 
> > 
> > > +
> > > +#endif
> > > +
> > > +
> > > +/* Ready bit for the currently selected queue - Read Write */
> > > -- 
> > > 2.19.1

Jason Wang

2019-Sep-11 02:38 UTC

head link

[RFC PATCH 3/4] virtio: introudce a mdev based transport

On 2019/9/10 ??9:52, Michael S. Tsirkin wrote:> On Tue, Sep 10, 2019 at 09:13:02PM +0800, Jason Wang wrote:
>> On 2019/9/10 ??6:01, Michael S. Tsirkin wrote:
>>>> +#ifndef _LINUX_VIRTIO_MDEV_H
>>>> +#define _LINUX_VIRTIO_MDEV_H
>>>> +
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/vringh.h>
>>>> +#include <uapi/linux/virtio_net.h>
>>>> +
>>>> +/*
>>>> + * Ioctls
>>>> + */
>>> Pls add a bit more content here. It's redundant to state these
>>> are ioctls. Much better to document what does each one do.
>>
>> Ok.
>>
>>
>>>> +
>>>> +struct virtio_mdev_callback {
>>>> +	irqreturn_t (*callback)(void *);
>>>> +	void *private;
>>>> +};
>>>> +
>>>> +#define VIRTIO_MDEV 0xAF
>>>> +#define VIRTIO_MDEV_SET_VQ_CALLBACK _IOW(VIRTIO_MDEV, 0x00, \
>>>> +					 struct virtio_mdev_callback)
>>>> +#define VIRTIO_MDEV_SET_CONFIG_CALLBACK _IOW(VIRTIO_MDEV,
0x01, \
>>>> +					struct virtio_mdev_callback)
>>> Function pointer in an ioctl parameter? How does this ever make
sense?
>>
>> I admit this is hacky (casting).
>>
>>
>>> And can't we use a couple of registers for this, and avoid
ioctls?
>>
>> Yes, how about something like interrupt numbers for each virtqueue and
>> config?
> Should we just reuse VIRTIO_PCI_COMMON_Q_XXX then?

You mean something like VIRTIO_PCI_COMMON_Q_MSIX? Then it becomes a PCI 
transport in fact. And using either MSIX or irq number is actually 
another layer of indirection. So I think we can just write callback 
function and parameter through registers.

>
>
>>>> +
>>>> +#define VIRTIO_MDEV_DEVICE_API_STRING		"virtio-mdev"
>>>> +
>>>> +/*
>>>> + * Control registers
>>>> + */
>>>> +
>>>> +/* Magic value ("virt" string) - Read Only */
>>>> +#define VIRTIO_MDEV_MAGIC_VALUE		0x000
>>>> +
>>>> +/* Virtio device version - Read Only */
>>>> +#define VIRTIO_MDEV_VERSION		0x004
>>>> +
>>>> +/* Virtio device ID - Read Only */
>>>> +#define VIRTIO_MDEV_DEVICE_ID		0x008
>>>> +
>>>> +/* Virtio vendor ID - Read Only */
>>>> +#define VIRTIO_MDEV_VENDOR_ID		0x00c
>>>> +
>>>> +/* Bitmask of the features supported by the device (host)
>>>> + * (32 bits per set) - Read Only */
>>>> +#define VIRTIO_MDEV_DEVICE_FEATURES	0x010
>>>> +
>>>> +/* Device (host) features set selector - Write Only */
>>>> +#define VIRTIO_MDEV_DEVICE_FEATURES_SEL	0x014
>>>> +
>>>> +/* Bitmask of features activated by the driver (guest)
>>>> + * (32 bits per set) - Write Only */
>>>> +#define VIRTIO_MDEV_DRIVER_FEATURES	0x020
>>>> +
>>>> +/* Activated features set selector - Write Only */
>>>> +#define VIRTIO_MDEV_DRIVER_FEATURES_SEL	0x024
>>>> +
>>>> +/* Queue selector - Write Only */
>>>> +#define VIRTIO_MDEV_QUEUE_SEL		0x030
>>>> +
>>>> +/* Maximum size of the currently selected queue - Read Only */
>>>> +#define VIRTIO_MDEV_QUEUE_NUM_MAX	0x034
>>>> +
>>>> +/* Queue size for the currently selected queue - Write Only */
>>>> +#define VIRTIO_MDEV_QUEUE_NUM		0x038
>>>> +
>>>> +/* Ready bit for the currently selected queue - Read Write */
>>>> +#define VIRTIO_MDEV_QUEUE_READY		0x044
>>> Is this same as started?
>>
>> Do you mean "status"?
> I really meant "enabled", didn't remember the correct name.
> As in:  VIRTIO_PCI_COMMON_Q_ENABLE

Yes, it's the same.

Thanks

>
>>>> +
>>>> +/* Alignment of virtqueue - Read Only */
>>>> +#define VIRTIO_MDEV_QUEUE_ALIGN		0x048
>>>> +
>>>> +/* Queue notifier - Write Only */
>>>> +#define VIRTIO_MDEV_QUEUE_NOTIFY	0x050
>>>> +
>>>> +/* Device status register - Read Write */
>>>> +#define VIRTIO_MDEV_STATUS		0x060
>>>> +
>>>> +/* Selected queue's Descriptor Table address, 64 bits in
two halves */
>>>> +#define VIRTIO_MDEV_QUEUE_DESC_LOW	0x080
>>>> +#define VIRTIO_MDEV_QUEUE_DESC_HIGH	0x084
>>>> +
>>>> +/* Selected queue's Available Ring address, 64 bits in two
halves */
>>>> +#define VIRTIO_MDEV_QUEUE_AVAIL_LOW	0x090
>>>> +#define VIRTIO_MDEV_QUEUE_AVAIL_HIGH	0x094
>>>> +
>>>> +/* Selected queue's Used Ring address, 64 bits in two
halves */
>>>> +#define VIRTIO_MDEV_QUEUE_USED_LOW	0x0a0
>>>> +#define VIRTIO_MDEV_QUEUE_USED_HIGH	0x0a4
>>>> +
>>>> +/* Configuration atomicity value */
>>>> +#define VIRTIO_MDEV_CONFIG_GENERATION	0x0fc
>>>> +
>>>> +/* The config space is defined by each driver as
>>>> + * the per-driver configuration space - Read Write */
>>>> +#define VIRTIO_MDEV_CONFIG		0x100
>>> Mixing device and generic config space is what virtio pci did,
>>> caused lots of problems with extensions.
>>> It would be better to reserve much more space.
>>
>> I see, will do this.
>>
>> Thanks
>>
>>
>>>
>>>> +
>>>> +#endif
>>>> +
>>>> +
>>>> +/* Ready bit for the currently selected queue - Read Write */
>>>> -- 
>>>> 2.19.1

Michael S. Tsirkin

2019-Sep-11 09:36 UTC

head link

[RFC PATCH 3/4] virtio: introudce a mdev based transport

On Wed, Sep 11, 2019 at 10:38:39AM +0800, Jason Wang
wrote:> 
> On 2019/9/10 ??9:52, Michael S. Tsirkin wrote:
> > On Tue, Sep 10, 2019 at 09:13:02PM +0800, Jason Wang wrote:
> > > On 2019/9/10 ??6:01, Michael S. Tsirkin wrote:
> > > > > +#ifndef _LINUX_VIRTIO_MDEV_H
> > > > > +#define _LINUX_VIRTIO_MDEV_H
> > > > > +
> > > > > +#include <linux/interrupt.h>
> > > > > +#include <linux/vringh.h>
> > > > > +#include <uapi/linux/virtio_net.h>
> > > > > +
> > > > > +/*
> > > > > + * Ioctls
> > > > > + */
> > > > Pls add a bit more content here. It's redundant to state
these
> > > > are ioctls. Much better to document what does each one do.
> > > 
> > > Ok.
> > > 
> > > 
> > > > > +
> > > > > +struct virtio_mdev_callback {
> > > > > +	irqreturn_t (*callback)(void *);
> > > > > +	void *private;
> > > > > +};
> > > > > +
> > > > > +#define VIRTIO_MDEV 0xAF
> > > > > +#define VIRTIO_MDEV_SET_VQ_CALLBACK _IOW(VIRTIO_MDEV,
0x00, \
> > > > > +					 struct virtio_mdev_callback)
> > > > > +#define VIRTIO_MDEV_SET_CONFIG_CALLBACK
_IOW(VIRTIO_MDEV, 0x01, \
> > > > > +					struct virtio_mdev_callback)
> > > > Function pointer in an ioctl parameter? How does this ever
make sense?
> > > 
> > > I admit this is hacky (casting).
> > > 
> > > 
> > > > And can't we use a couple of registers for this, and
avoid ioctls?
> > > 
> > > Yes, how about something like interrupt numbers for each
virtqueue and
> > > config?
> > Should we just reuse VIRTIO_PCI_COMMON_Q_XXX then?
> 
> 
> You mean something like VIRTIO_PCI_COMMON_Q_MSIX? Then it becomes a PCI
> transport in fact. And using either MSIX or irq number is actually another
> layer of indirection. So I think we can just write callback function and
> parameter through registers.
I just realized, all these registers are just encoded so you
can pass stuff through read/write. But it can instead be
just a normal C function call with no messy encoding.
So why do we want to do this encoding?

> 
> > 
> > 
> > > > > +
> > > > > +#define VIRTIO_MDEV_DEVICE_API_STRING	
"virtio-mdev"
> > > > > +
> > > > > +/*
> > > > > + * Control registers
> > > > > + */
> > > > > +
> > > > > +/* Magic value ("virt" string) - Read Only
*/
> > > > > +#define VIRTIO_MDEV_MAGIC_VALUE		0x000
> > > > > +
> > > > > +/* Virtio device version - Read Only */
> > > > > +#define VIRTIO_MDEV_VERSION		0x004
> > > > > +
> > > > > +/* Virtio device ID - Read Only */
> > > > > +#define VIRTIO_MDEV_DEVICE_ID		0x008
> > > > > +
> > > > > +/* Virtio vendor ID - Read Only */
> > > > > +#define VIRTIO_MDEV_VENDOR_ID		0x00c
> > > > > +
> > > > > +/* Bitmask of the features supported by the device
(host)
> > > > > + * (32 bits per set) - Read Only */
> > > > > +#define VIRTIO_MDEV_DEVICE_FEATURES	0x010
> > > > > +
> > > > > +/* Device (host) features set selector - Write Only */
> > > > > +#define VIRTIO_MDEV_DEVICE_FEATURES_SEL	0x014
> > > > > +
> > > > > +/* Bitmask of features activated by the driver (guest)
> > > > > + * (32 bits per set) - Write Only */
> > > > > +#define VIRTIO_MDEV_DRIVER_FEATURES	0x020
> > > > > +
> > > > > +/* Activated features set selector - Write Only */
> > > > > +#define VIRTIO_MDEV_DRIVER_FEATURES_SEL	0x024
> > > > > +
> > > > > +/* Queue selector - Write Only */
> > > > > +#define VIRTIO_MDEV_QUEUE_SEL		0x030
> > > > > +
> > > > > +/* Maximum size of the currently selected queue - Read
Only */
> > > > > +#define VIRTIO_MDEV_QUEUE_NUM_MAX	0x034
> > > > > +
> > > > > +/* Queue size for the currently selected queue - Write
Only */
> > > > > +#define VIRTIO_MDEV_QUEUE_NUM		0x038
> > > > > +
> > > > > +/* Ready bit for the currently selected queue - Read
Write */
> > > > > +#define VIRTIO_MDEV_QUEUE_READY		0x044
> > > > Is this same as started?
> > > 
> > > Do you mean "status"?
> > I really meant "enabled", didn't remember the correct
name.
> > As in:  VIRTIO_PCI_COMMON_Q_ENABLE
> 
> 
> Yes, it's the same.
> 
> Thanks
> 
> 
> > 
> > > > > +
> > > > > +/* Alignment of virtqueue - Read Only */
> > > > > +#define VIRTIO_MDEV_QUEUE_ALIGN		0x048
> > > > > +
> > > > > +/* Queue notifier - Write Only */
> > > > > +#define VIRTIO_MDEV_QUEUE_NOTIFY	0x050
> > > > > +
> > > > > +/* Device status register - Read Write */
> > > > > +#define VIRTIO_MDEV_STATUS		0x060
> > > > > +
> > > > > +/* Selected queue's Descriptor Table address, 64
bits in two halves */
> > > > > +#define VIRTIO_MDEV_QUEUE_DESC_LOW	0x080
> > > > > +#define VIRTIO_MDEV_QUEUE_DESC_HIGH	0x084
> > > > > +
> > > > > +/* Selected queue's Available Ring address, 64
bits in two halves */
> > > > > +#define VIRTIO_MDEV_QUEUE_AVAIL_LOW	0x090
> > > > > +#define VIRTIO_MDEV_QUEUE_AVAIL_HIGH	0x094
> > > > > +
> > > > > +/* Selected queue's Used Ring address, 64 bits in
two halves */
> > > > > +#define VIRTIO_MDEV_QUEUE_USED_LOW	0x0a0
> > > > > +#define VIRTIO_MDEV_QUEUE_USED_HIGH	0x0a4
> > > > > +
> > > > > +/* Configuration atomicity value */
> > > > > +#define VIRTIO_MDEV_CONFIG_GENERATION	0x0fc
> > > > > +
> > > > > +/* The config space is defined by each driver as
> > > > > + * the per-driver configuration space - Read Write */
> > > > > +#define VIRTIO_MDEV_CONFIG		0x100
> > > > Mixing device and generic config space is what virtio pci
did,
> > > > caused lots of problems with extensions.
> > > > It would be better to reserve much more space.
> > > 
> > > I see, will do this.
> > > 
> > > Thanks
> > > 
> > > 
> > > > 
> > > > > +
> > > > > +#endif
> > > > > +
> > > > > +
> > > > > +/* Ready bit for the currently selected queue - Read
Write */
> > > > > -- 
> > > > > 2.19.1

Reasonably Related Threads

Search for more maybe matching threads

Linux Virtualization - Sep 2019 - [RFC PATCH 3/4] virtio: introudce a mdev based transport

[RFC PATCH 3/4] virtio: introudce a mdev based transport

[RFC PATCH 3/4] virtio: introudce a mdev based transport

[RFC PATCH 3/4] virtio: introudce a mdev based transport

Reasonably Related Threads