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;