[PATCH] Change virtio_pci to use a shared memory area for config

[PATCH] Change virtio_pci to use a shared memory area for config

[Anthony Liguori]

This patch changes virtio_pci to use a shared memory area for virtio config
info instead of using the PCI configuration space.  This is closer semantically
to what the virtio API exposes and is it a lot easier to implement on both
ends.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index eb9a8e0..7e6e453 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -39,6 +39,12 @@ struct virtio_pci_device
 	/* the IO mapping for the PCI config space */
 	void *ioaddr;
 
+	/* the shared device configuration */
+	void *config;
+
+	/* the size of the shared device configuration */
+	u32 config_len;
+
 	/* a list of queues so we can dispatch IRQs */
 	spinlock_t lock;
 	struct list_head virtqueues;
@@ -111,47 +117,12 @@ static void vp_get(struct virtio_device *vdev, unsigned offset,
 		   void *buf, unsigned len)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	void *ioaddr = vp_dev->ioaddr + VIRTIO_PCI_CONFIG + offset;
-
-	/* We translate appropriately sized get requests into more natural
-	 * IO operations.  These functions also take care of endianness
-	 * conversion. */
-	switch (len) {
-	case 1: {
-		u8 val;
-		val = ioread8(ioaddr);
-		memcpy(buf, &val, sizeof(val));
-		break;
-	}
-	case 2: {
-		u16 val;
-		val = ioread16(ioaddr);
-		memcpy(buf, &val, sizeof(val));
-		break;
-	}
-	case 4: {
-		u32 val;
-		val = ioread32(ioaddr);
-		memcpy(buf, &val, sizeof(val));
-		break;
-	}
-	case 8: {
-		u64 val;
-		val = (u64)ioread32(ioaddr) << 32;
-		val |= ioread32(ioaddr + 4);
-		memcpy(buf, &val, sizeof(val));
-		break;
-	}
 
-	default: {
-		uint8_t *ptr = buf;
-		int i;
+	BUG_ON(vp_dev->config == NULL);
 
-		for (i = 0; i < len; i++)
-			ptr[i] = ioread8(ioaddr + i);
-		break;
-	}
-	}
+	BUG_ON(vp_dev->config_len < (offset + len));
+
+	memcpy(buf, vp_dev->config + offset, len);
 }
 
 /* the config->set() implementation.  it's symmetric to the config->get()
@@ -160,43 +131,10 @@ static void vp_set(struct virtio_device *vdev, unsigned offset,
 		   const void *buf, unsigned len)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	void *ioaddr = vp_dev->ioaddr + VIRTIO_PCI_CONFIG + offset;
-
-	switch (len) {
-	case 1: {
-		u8 val;
-		memcpy(&val, buf, sizeof(val));
-		iowrite8(val, ioaddr);
-		break;
-	}
-	case 2: {
-		u16 val;
-		memcpy(&val, buf, sizeof(val));
-		iowrite16(val, ioaddr);
-		break;
-	}
-	case 4: {
-		u32 val;
-		memcpy(&val, buf, sizeof(val));
-		iowrite32(val, ioaddr);
-		break;
-	}
-	case 8: {
-		u64 val;
-		memcpy(&val, buf, sizeof(val));
-		iowrite32(val >> 32, ioaddr);
-		iowrite32(val, ioaddr + 4);
-		break;
-	}
-	default: {
-		const uint8_t *ptr = buf;
-		int i;
 
-		for (i = 0; i < len; i++)
-			iowrite8(ptr[i], ioaddr + i);
-		break;
-	}
-	}
+	BUG_ON(vp_dev->config_len < (offset + len));
+
+	memcpy(vp_dev->config + offset, buf, len);
 }
 
 /* config->{get,set}_status() implementations */
@@ -281,7 +219,7 @@ static struct virtqueue *vp_find_vq(struct virtio_device *vdev, unsigned index,
 	info->queue_index = index;
 	info->num = num;
 
-	info->queue = kmalloc(vring_size(num), GFP_KERNEL | __GFP_ZERO);
+	info->queue = kzalloc(PAGE_ALIGN(vring_size(num)), GFP_KERNEL);
 	if (info->queue == NULL) {
 		err = -ENOMEM;
 		goto out_info;
@@ -390,11 +328,25 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
 	vp_dev->vdev.id.vendor = pci_dev->subsystem_vendor;
 	vp_dev->vdev.id.device = pci_dev->subsystem_device;
 
+	vp_dev->config_len = ioread32(vp_dev->ioaddr + VIRTIO_PCI_CONFIG_LEN);
+	if (vp_dev->config_len) {
+		/* round up to nearest page size to ensure proper alignment */
+		vp_dev->config = kzalloc(PAGE_ALIGN(vp_dev->config_len),
+					 GFP_KERNEL);
+		if (vp_dev->config == NULL)
+			goto out_set_drvdata;
+
+		/* tell the host about our config space */
+		iowrite32(virt_to_phys(vp_dev->config) >> PAGE_SHIFT,
+			  vp_dev->ioaddr + VIRTIO_PCI_CONFIG_PFN);
+	} else
+		vp_dev->config = NULL;
+
 	/* register a handler for the queue with the PCI device's interrupt */
 	err = request_irq(vp_dev->pci_dev->irq, vp_interrupt, IRQF_SHARED,
 			  vp_dev->vdev.dev.bus_id, vp_dev);
 	if (err)
-		goto out_set_drvdata;
+		goto out_alloc_config;
 
 	/* finally register the virtio device */
 	err = register_virtio_device(&vp_dev->vdev);
@@ -405,6 +357,8 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
 
 out_req_irq:
 	free_irq(pci_dev->irq, vp_dev);
+out_alloc_config:
+	kfree(vp_dev->config);
 out_set_drvdata:
 	pci_set_drvdata(pci_dev, NULL);
 	pci_iounmap(pci_dev, vp_dev->ioaddr);
@@ -422,6 +376,7 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
 	struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
 
 	free_irq(pci_dev->irq, vp_dev);
+	kfree(vp_dev->config);
 	pci_set_drvdata(pci_dev, NULL);
 	pci_iounmap(pci_dev, vp_dev->ioaddr);
 	pci_release_regions(pci_dev);
diff --git a/include/linux/virtio_pci.h b/include/linux/virtio_pci.h
index 4b28409..79e6641 100644
--- a/include/linux/virtio_pci.h
+++ b/include/linux/virtio_pci.h
@@ -45,8 +45,12 @@
  * a read-and-acknowledge. */
 #define VIRTIO_PCI_ISR			19
 
-/* The remaining space is defined by each driver as the per-driver
- * configuration space */
-#define VIRTIO_PCI_CONFIG		20
+/* A 32-bit r/o configuration size.  This is the amount of memory required
+ * to be allocated for VIRTIO_PCI_CONFIG_PFN. */
+#define VIRTIO_PCI_CONFIG_LEN		20
+
+/* A 32-bit r/w PFN for the shared configuration information.  The PA written
+ * by the host must point to at least VIRTIO_PCI_CONFIG_LEN bytes */
+#define VIRTIO_PCI_CONFIG_PFN		24
 
 #endif

Re: [PATCH] Change virtio_pci to use a shared memory area for config

[Rusty Russell]

On Sunday 11 November 2007 10:39:25 Anthony Liguori wrote:
> This patch changes virtio_pci to use a shared memory area for virtio config
> info instead of using the PCI configuration space.  This is closer
> semantically to what the virtio API exposes and is it a lot easier to
> implement on both ends.

No it's not!

Does this help illuminate your path?

Cheers,
Rusty.
BTW: Am switching back to a patchqueue... it's just easier.
==
Simplify virtio configuration further: use structs

Instead of using constants for offsets, and documenting sizes, we can
actually expost them as a structure representation.  This is clearer,
and easier for the host to implement.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff -r 07f7ae8105c8 Documentation/lguest/lguest.c
--- a/Documentation/lguest/lguest.c	Mon Nov 12 17:44:21 2007 +1100
+++ b/Documentation/lguest/lguest.c	Mon Nov 12 18:47:48 2007 +1100
@@ -35,6 +35,7 @@
 #include <assert.h>
 #include <sched.h>
 #include <limits.h>
+#include <stddef.h>
 #include "linux/lguest_launcher.h"
 #include "linux/virtio_config.h"
 #include "linux/virtio_net.h"
@@ -184,7 +185,7 @@ static void *_convert(struct iovec *iov,
 #define cpu_to_le64(v64) (v64)
 #define le16_to_cpu(v16) (v16)
 #define le32_to_cpu(v32) (v32)
-#define le64_to_cpu(v32) (v64)
+#define le64_to_cpu(v64) (v64)
 
 /*L:100 The Launcher code itself takes us out into userspace, that scary place
  * where pointers run wild and free!  Unfortunately, like most userspace
@@ -985,7 +986,7 @@ static void handle_input(int fd)
  * number of virtqueue descriptors, then two sets of feature bits, then an
  * array of configuration bytes.  This routine returns the configuration
  * pointer. */
-static void *device_config(const struct device *dev)
+static u8 *device_config(const struct device *dev)
 {
 	return (void *)(dev->desc + 1)
 		+ dev->desc->num_vq * sizeof(struct lguest_vqconfig)
@@ -1081,24 +1082,18 @@ static void add_feature(struct device *d
 	features[bit / CHAR_BIT] |= (1 << (bit % CHAR_BIT));
 }
 
-/* This routine adds a new configuration field to an existing device's
+/* This routine sets the configuration fields for an existing device's
  * descriptor.  It only works for the last device, but that's OK because that's
  * how we use it. */
-static void add_desc_field(struct device *dev, unsigned off, unsigned len,
-			   const void *c)
-{
-	u8 *config = device_config(dev);
-
-	/* Extend the length of the device's config space if needed. */
-	if (off + len > dev->desc->config_len)
-		dev->desc->config_len = off + len;
-
+static void set_config(struct device *dev, unsigned len, const void *conf)
+{
 	/* Check we haven't overflowed our single page. */
-	if (config + dev->desc->config_len > devices.descpage + getpagesize())
+	if (device_config(dev) + len > devices.descpage + getpagesize())
 		errx(1, "Too many devices");
 
-	/* Copy in the config information. */
-	memcpy(config + off, c, len);
+	/* Copy in the config information, and store the length. */
+	memcpy(device_config(dev), conf, len);
+	dev->desc->config_len = len;
 }
 
 /* This routine does all the creation and setup of a new device, including
@@ -1252,7 +1247,7 @@ static void setup_tun_net(const char *ar
 	int netfd, ipfd;
 	u32 ip;
 	const char *br_name = NULL;
-	u8 hwaddr[6];
+	struct virtio_net_config conf;
 
 	/* We open the /dev/net/tun device and tell it we want a tap device.  A
 	 * tap device is like a tun device, only somehow different.  To tell
@@ -1291,11 +1286,11 @@ static void setup_tun_net(const char *ar
 		ip = str2ip(arg);
 
 	/* Set up the tun device, and get the mac address for the interface. */
-	configure_device(ipfd, ifr.ifr_name, ip, hwaddr);
+	configure_device(ipfd, ifr.ifr_name, ip, conf.mac);
 
 	/* Tell Guest what MAC address to use. */
 	add_feature(dev, VIRTIO_NET_F_MAC);
-	add_desc_field(dev, VIRTIO_CONFIG_NET_MAC_F, sizeof(hwaddr), hwaddr);
+	set_config(dev, sizeof(conf), &conf);
 
 	/* We don't need the socket any more; setup is done. */
 	close(ipfd);
@@ -1485,8 +1480,7 @@ static void setup_block_file(const char 
 	struct device *dev;
 	struct vblk_info *vblk;
 	void *stack;
-	u64 cap;
-	unsigned int val;
+	struct virtio_blk_config conf;
 
 	/* This is the pipe the I/O thread will use to tell us I/O is done. */
 	pipe(p);
@@ -1504,19 +1498,18 @@ static void setup_block_file(const char 
 	vblk->fd = open_or_die(filename, O_RDWR|O_LARGEFILE);
 	vblk->len = lseek64(vblk->fd, 0, SEEK_END);
 
-	/* We're going to specify the maximum number of segments, and we
-	 * support barriers. */
-	add_feature(dev, VIRTIO_BLK_F_SEG_MAX);
+	/* We support barriers. */
 	add_feature(dev, VIRTIO_BLK_F_BARRIER);
 
 	/* Tell Guest how many sectors this device has. */
-	cap = cpu_to_le64(vblk->len / 512);
-	add_desc_field(dev, VIRTIO_CONFIG_BLK_F_CAPACITY, sizeof(cap), &cap);
+	conf.capacity = cpu_to_le64(vblk->len / 512);
 
 	/* Tell Guest not to put in too many descriptors at once: two are used
 	 * for the in and out elements. */
-	val = cpu_to_le32(VIRTQUEUE_NUM - 2);
-	add_desc_field(dev, VIRTIO_CONFIG_BLK_F_SEG_MAX, sizeof(val), &val);
+	add_feature(dev, VIRTIO_BLK_F_SEG_MAX);
+	conf.seg_max = cpu_to_le32(VIRTQUEUE_NUM - 2);
+
+	set_config(dev, sizeof(conf), &conf);
 
 	/* The I/O thread writes to this end of the pipe when done. */
 	vblk->done_fd = p[1];
@@ -1535,7 +1528,7 @@ static void setup_block_file(const char 
 	close(vblk->workpipe[0]);
 
 	verbose("device %u: virtblock %llu sectors\n",
-		devices.device_num, cap);
+		devices.device_num, le64_to_cpu(conf.capacity));
 }
 /* That's the end of device setup. */
 
diff -r 07f7ae8105c8 drivers/block/virtio_blk.c
--- a/drivers/block/virtio_blk.c	Mon Nov 12 17:44:21 2007 +1100
+++ b/drivers/block/virtio_blk.c	Mon Nov 12 18:47:48 2007 +1100
@@ -218,7 +218,8 @@ static int virtblk_probe(struct virtio_d
 		blk_queue_ordered(vblk->disk->queue, QUEUE_ORDERED_TAG, NULL);
 
 	/* Host must always specify the capacity. */
-	__virtio_config_val(vdev, VIRTIO_CONFIG_BLK_F_CAPACITY, &cap);
+	__virtio_config_val(vdev, offsetof(struct virtio_blk_config, capacity),
+			    &cap);
 
 	/* If capacity is too big, truncate with warning. */
 	if ((sector_t)cap != cap) {
@@ -231,12 +232,14 @@ static int virtblk_probe(struct virtio_d
 	/* Host can optionally specify maximum segment size and number of
 	 * segments. */
 	err = virtio_config_val(vdev, VIRTIO_BLK_F_SIZE_MAX,
-				VIRTIO_CONFIG_BLK_F_SIZE_MAX, &v);
+				offsetof(struct virtio_blk_config, size_max),
+				&v);
 	if (!err)
 		blk_queue_max_segment_size(vblk->disk->queue, v);
 
 	err = virtio_config_val(vdev, VIRTIO_BLK_F_SEG_MAX,
-				VIRTIO_CONFIG_BLK_F_SEG_MAX, &v);
+				offsetof(struct virtio_blk_config, seg_max),
+				&v);
 	if (!err)
 		blk_queue_max_hw_segments(vblk->disk->queue, v);
 
diff -r 07f7ae8105c8 drivers/net/virtio_net.c
--- a/drivers/net/virtio_net.c	Mon Nov 12 17:44:21 2007 +1100
+++ b/drivers/net/virtio_net.c	Mon Nov 12 18:47:48 2007 +1100
@@ -352,8 +352,9 @@ static int virtnet_probe(struct virtio_d
 
 	/* Configuration may specify what MAC to use.  Otherwise random. */
 	if (vdev->config->feature(vdev, VIRTIO_NET_F_MAC)) {
-		vdev->config->get(vdev, VIRTIO_CONFIG_NET_MAC_F, dev->dev_addr,
-				  dev->addr_len);
+		vdev->config->get(vdev,
+				  offsetof(struct virtio_net_config, mac),
+				  dev->dev_addr, dev->addr_len);
 	} else
 		random_ether_addr(dev->dev_addr);
 
diff -r 07f7ae8105c8 include/linux/virtio_blk.h
--- a/include/linux/virtio_blk.h	Mon Nov 12 17:44:21 2007 +1100
+++ b/include/linux/virtio_blk.h	Mon Nov 12 18:47:48 2007 +1100
@@ -10,12 +10,15 @@
 #define VIRTIO_BLK_F_SIZE_MAX	1	/* Indicates maximum segment size */
 #define VIRTIO_BLK_F_SEG_MAX	2	/* Indicates maximum # of segments */
 
-/* The capacity (in 512-byte sectors).  8 bytes. */
-#define VIRTIO_CONFIG_BLK_F_CAPACITY	0
-/* The maximum segment size. 4 bytes. */
-#define VIRTIO_CONFIG_BLK_F_SIZE_MAX	0x08
-/* The maximum number of segments.  4 bytes. */
-#define VIRTIO_CONFIG_BLK_F_SEG_MAX	0x0A
+struct virtio_blk_config
+{
+	/* The capacity (in 512-byte sectors). */
+	__le64 capacity;
+	/* The maximum segment size (if VIRTIO_BLK_F_SIZE_MAX) */
+	__le32 size_max;
+	/* The maximum number of segments (if VIRTIO_BLK_F_SEG_MAX) */
+	__le32 seg_max;
+} __attribute__((packed));
 
 /* These two define direction. */
 #define VIRTIO_BLK_T_IN		0
diff -r 07f7ae8105c8 include/linux/virtio_net.h
--- a/include/linux/virtio_net.h	Mon Nov 12 17:44:21 2007 +1100
+++ b/include/linux/virtio_net.h	Mon Nov 12 18:47:48 2007 +1100
@@ -13,8 +13,11 @@
 #define VIRTIO_NET_F_TSO6	4
 #define VIRTIO_NET_F_MAC	5
 
-/* The config defining mac address (6 bytes) */
-#define VIRTIO_CONFIG_NET_MAC_F	0
+struct virtio_net_config
+{
+	/* The config defining mac address (if VIRTIO_NET_F_MAC) */
+	__u8 mac[6];
+} __attribute__((packed));
 
 /* This is the first element of the scatter-gather list.  If you don't
  * specify GSO or CSUM features, you can simply ignore the header. */

[PATCH] Change virtio_pci to use a shared memory area for config

[Anthony Liguori]

This patch changes virtio_pci to treat the PCI config space as opaque memory.
This is closer semantically to what the virtio API exposes and is it a lot
easier to implement on the guest side.  It also doesn't preclude a more
PCI-like treatment of the ABI.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index eb9a8e0..36e86eb 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -112,46 +112,11 @@ static void vp_get(struct virtio_device *vdev, unsigned offset,
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	void *ioaddr = vp_dev->ioaddr + VIRTIO_PCI_CONFIG + offset;
+	u8 *ptr = buf;
+	int i;
 
-	/* We translate appropriately sized get requests into more natural
-	 * IO operations.  These functions also take care of endianness
-	 * conversion. */
-	switch (len) {
-	case 1: {
-		u8 val;
-		val = ioread8(ioaddr);
-		memcpy(buf, &val, sizeof(val));
-		break;
-	}
-	case 2: {
-		u16 val;
-		val = ioread16(ioaddr);
-		memcpy(buf, &val, sizeof(val));
-		break;
-	}
-	case 4: {
-		u32 val;
-		val = ioread32(ioaddr);
-		memcpy(buf, &val, sizeof(val));
-		break;
-	}
-	case 8: {
-		u64 val;
-		val = (u64)ioread32(ioaddr) << 32;
-		val |= ioread32(ioaddr + 4);
-		memcpy(buf, &val, sizeof(val));
-		break;
-	}
-
-	default: {
-		uint8_t *ptr = buf;
-		int i;
-
-		for (i = 0; i < len; i++)
-			ptr[i] = ioread8(ioaddr + i);
-		break;
-	}
-	}
+	for (i = 0; i < len; i++)
+		ptr[i] = ioread8(ioaddr + i);
 }
 
 /* the config->set() implementation.  it's symmetric to the config->get()
@@ -161,42 +126,11 @@ static void vp_set(struct virtio_device *vdev, unsigned offset,
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	void *ioaddr = vp_dev->ioaddr + VIRTIO_PCI_CONFIG + offset;
+	const u8 *ptr = buf;
+	int i;
 
-	switch (len) {
-	case 1: {
-		u8 val;
-		memcpy(&val, buf, sizeof(val));
-		iowrite8(val, ioaddr);
-		break;
-	}
-	case 2: {
-		u16 val;
-		memcpy(&val, buf, sizeof(val));
-		iowrite16(val, ioaddr);
-		break;
-	}
-	case 4: {
-		u32 val;
-		memcpy(&val, buf, sizeof(val));
-		iowrite32(val, ioaddr);
-		break;
-	}
-	case 8: {
-		u64 val;
-		memcpy(&val, buf, sizeof(val));
-		iowrite32(val >> 32, ioaddr);
-		iowrite32(val, ioaddr + 4);
-		break;
-	}
-	default: {
-		const uint8_t *ptr = buf;
-		int i;
-
-		for (i = 0; i < len; i++)
-			iowrite8(ptr[i], ioaddr + i);
-		break;
-	}
-	}
+	for (i = 0; i < len; i++)
+		iowrite8(ptr[i], ioaddr + i);
 }
 
 /* config->{get,set}_status() implementations */
@@ -281,7 +215,7 @@ static struct virtqueue *vp_find_vq(struct virtio_device *vdev, unsigned index,
 	info->queue_index = index;
 	info->num = num;
 
-	info->queue = kmalloc(vring_size(num), GFP_KERNEL | __GFP_ZERO);
+	info->queue = kzalloc(PAGE_ALIGN(vring_size(num)), GFP_KERNEL);
 	if (info->queue == NULL) {
 		err = -ENOMEM;
 		goto out_info;