Anthony Liguori
2007-Nov-10 15:39 UTC
[PATCH] Change virtio_pci to use a shared memory area for config
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
Rusty Russell
2007-Nov-11 23:51 UTC
[PATCH] Change virtio_pci to use a shared memory area for config
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. */
Anthony Liguori
2007-Nov-12 19:30 UTC
[PATCH] Change virtio_pci to use a shared memory area for config
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;