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

[Jason Wang]

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?


> > +
> > +#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"?


> > +
> > +/* 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