virtio config_ops refactoring
Subsystems (8)
Hi,
The current virtio config_ops do not map very well to the PCI config space. Here they are:
Quoted 13 lines
struct
virtio_config_ops{ void *(*find)(struct virtio_device *vdev, u8 type, unsigned *len); void (*get)(struct virtio_device *vdev, void *token, void *buf, unsigned len); void (*set)(struct virtio_device *vdev, void *token, const void *buf, unsigned len); u8 (*get_status)(struct virtio_device *vdev); void (*set_status)(struct virtio_device *vdev, u8 status); struct virtqueue *(*find_vq)(struct virtio_device *vdev, bool (*callback)(struct virtqueue *)); void (*del_vq)(struct virtqueue *vq); };
Semantically, find requires that a field have both a type and a length. With the exception of the VIRTQUEUE field used internally by lguest, type is always a unique identifier. Since virtqueue information is not a required part of the config space, it seems to me that type really should be treated as a unique identifier.
find_vq also is curious in that it is stateful in it's enumeration. This adds seemingly unnecessary complexity. The result is that in my PCI virtio implementation, I have to use an opaque blob that's self describing and variable length in the PCI config space. This is not very natural at all for a PCI device!
Here is how I propose changing the config_ops:
struct virtio_config_ops
{
bool (*get)(struct virtio_device *vdev, unsigned id, void *buf,
unsigned len);
bool (*set)(struct virtio_device *vdev, unsigned id, const void
*buf, unsigned len);
u8 (*get_status)(struct virtio_device *vdev);
void (*set_status)(struct virtio_device *vdev, u8 status); struct virtqueue *(*get_vq)(struct virtio_device *vdev, unsigned
index, bool (*callback)(struct virtqueue *));
void (*del_vq)(struct virtqueue *vq);
};
config_ops->get() returns false if the id is invalid as does ->set(). get_vq() returns NULL if the index is not a valid virtqueue (and we'll mandate that all virtqueues are in order starting at 0).
The id space is defined by the driver itself but is guaranteed to be non-overlapping and starting from 0. For instance, the block device would have:
/* The capacity (in 512-byte sectors). */
#define VIRTIO_CONFIG_BLK_F_CAPACITY 0x00
/* The maximum segment size. */
#define VIRTIO_CONFIG_BLK_F_SIZE_MAX 0x08
/* The maximum number of segments. */
#define VIRTIO_CONFIG_BLK_F_SEG_MAX 0x12
This maps very well to the PCI config space. For lguest, the actual config items would simply be packed in a single page. Since lguest uses the config space for virtqueues, lguest_device_desc will have to be changed to also contain an array of virtqueue infos. It doesn't prevent an implementation from using the id's as opaque keys though (as one might do for Xen since presumably the configuration data would be represented within xenbus).
If there's agreement that this approach is better, I'll start submitting patches.
Regards,
Anthony Liguori
Quoted 5 lines · On Wednesday 07 November 2007 04:48:35 Anthony Liguori wrote:
Semantically, find requires that a field have both a type and a length. With the exception of the VIRTQUEUE field used internally by lguest, type is always a unique identifier. Since virtqueue information is not a required part of the config space, it seems to me that type really should be treated as a unique identifier.
Hi Anthony,
Not sure I get this. It is a unique identifier. You need the length
to handle unknown fields.
Quoted 1 line
find_vqalso is curious in that it is stateful in it's enumeration.
Well, they're *all* stateful. This gives a simple method of knowing what fields the guest understands: it marks the fields as it finds them. Then it sets the status, which allows the host to know when it's completed configuration reads.
I like enumerating the virtqueues: it's not necessary but it's clearer.
Quoted 1 line
This adds seemingly unnecessary complexity.
I'd be happy for a simpler mechanism...
Cheers,
Rusty.
Quoted 5 lines · On Wednesday 07 November 2007 04:48:35 Anthony Liguori wrote:
Semantically, find requires that a field have both a type and a length. With the exception of the VIRTQUEUE field used internally by lguest, type is always a unique identifier. Since virtqueue information is not a required part of the config space, it seems to me that type really should be treated as a unique identifier.
Hi Anthony,
Not sure I get this. It is a unique identifier. You need the length
to handle unknown fields.
Quoted 1 line
find_vqalso is curious in that it is stateful in it's enumeration.
Well, they're *all* stateful. This gives a simple method of knowing what fields the guest understands: it marks the fields as it finds them. Then it sets the status, which allows the host to know when it's completed configuration reads.
I like enumerating the virtqueues: it's not necessary but it's clearer.
Quoted 1 line
This adds seemingly unnecessary complexity.
I'd be happy for a simpler mechanism...
Cheers,
Rusty.
Quoted 11 lines · Rusty Russell
Quoted 5 lines · On Wednesday 07 November 2007 04:48:35 Anthony Liguori wrote:
Semantically, find requires that a field have both a type and a length. With the exception of the VIRTQUEUE field used internally by lguest, type is always a unique identifier. Since virtqueue information is not a required part of the config space, it seems to me that type really should be treated as a unique identifier.
Hi Anthony,
Not sure I get this. It is a unique identifier. You need the length
to handle unknown fields.
It's not a unique identifier since it can be used for multiple items (like it is for virtqueues configs).
Quoted 3 lines
Quoted 1 line
find_vqalso is curious in that it is stateful in it's enumeration.Well, they're *all* stateful. This gives a simple method of knowing what fields the guest understands: it marks the fields as it finds them. Then it sets the status, which allows the host to know when it's completed configuration reads.
But PCI device configuration is not stateful. If you care about letting the host know what features a guest understands, I think something more explicit and stateful should be used. For instance, a feature register that stores a bitmap.
Otherwise, the host has to infer based on what fields that guest has read what features the guest actually supports. That seems error prone to me.
Quoted 5 lines
I like enumerating the virtqueues: it's not necessary but it's clearer.
Quoted 1 line
This adds seemingly unnecessary complexity.
I'd be happy for a simpler mechanism...
What do you think of what I proposed? It seems simpler to me.
Regards,
Anthony Liguori
Quoted 2 lines
Cheers,
Rusty.
Quoted 11 lines · Rusty Russell
Quoted 5 lines · On Wednesday 07 November 2007 04:48:35 Anthony Liguori wrote:
Semantically, find requires that a field have both a type and a length. With the exception of the VIRTQUEUE field used internally by lguest, type is always a unique identifier. Since virtqueue information is not a required part of the config space, it seems to me that type really should be treated as a unique identifier.
Hi Anthony,
Not sure I get this. It is a unique identifier. You need the length
to handle unknown fields.
It's not a unique identifier since it can be used for multiple items (like it is for virtqueues configs).
Quoted 3 lines
Quoted 1 line
find_vqalso is curious in that it is stateful in it's enumeration.Well, they're *all* stateful. This gives a simple method of knowing what fields the guest understands: it marks the fields as it finds them. Then it sets the status, which allows the host to know when it's completed configuration reads.
But PCI device configuration is not stateful. If you care about letting the host know what features a guest understands, I think something more explicit and stateful should be used. For instance, a feature register that stores a bitmap.
Otherwise, the host has to infer based on what fields that guest has read what features the guest actually supports. That seems error prone to me.
Quoted 5 lines
I like enumerating the virtqueues: it's not necessary but it's clearer.
Quoted 1 line
This adds seemingly unnecessary complexity.
I'd be happy for a simpler mechanism...
What do you think of what I proposed? It seems simpler to me.
Regards,
Anthony Liguori
Quoted 2 lines
Cheers,
Rusty.
PATCH 1/1 virtio config_ops refactoring
In reply to Anthony LiguoriAfter discussion with Anthony, the virtio config has been simplified. We lose some minor features (the virtio_net address must now be 6 bytes) but it turns out to be a wash in terms of complexity, while simplifying PCI.
This can be found in the new virtio git tree, in the "patches/1" branch (new branches will be created as I rebase to keep up with Linus).
git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux-2.6-virtio.git
I've also posted below, for easy commentry.
Cheers,
Rusty.
===
Previously we used a type/len pair within the config space, but this seems overkill. We now simply use an agreed offset within the config space and assume everyone knows the size of any entry it is interested in (the config space can now only be extended at the end).
The main driver-visible change is that we indicate what fields are present with an explicit feature bit.
Documentation/lguest/lguest.c +101 -60
@@ -34,6 +34,7 @@#include <zlib.h> #include <assert.h> #include <sched.h> +#include <limits.h> #include "linux/lguest_launcher.h" #include "linux/virtio_config.h" #include "linux/virtio_net.h"
@@ -96,13 +97,11 @@ struct device_list/* The descriptor page for the devices. */ u8 *descpage; -/* The tail of the last descriptor. */ -unsigned int desc_used; -/* A single linked list of devices. */ struct device *dev; -/* ... And an end pointer so we can easily append new devices */ -struct device **lastdev; +/* And a pointer to the last device for easy append and also for +* configuration appending. */ +struct device *lastdev; };/* The list of Guest devices, based on command line arguments. */
@@ -980,54 +979,44 @@ static void handle_input(int fd)* * All devices need a descriptor so the Guest knows it exists, and a "struct * device" so the Launcher can keep track of it. We have common helper -* routines to allocate them. -* -* This routine allocates a new "struct lguest_device_desc" from descriptor -* table just above the Guest's normal memory. It returns a pointer to that -* descriptor. */ -static struct lguest_device_desc *new_dev_desc (u16 type) -{ -struct lguest_device_desc *d; - -/* We only have one page for all the descriptors. */ -if (devices.desc_used +sizeof (*d) >getpagesize ()) -errx (1 ,"Too many devices" ); - -/* We don't need to set config_len or status: page is 0 already. */ - d = (void *)devices.descpage + devices.desc_used; - d->type = type; - devices.desc_used +=sizeof (*d); +* routines to allocate and manage them. */ -return d; +/* The layout of the device page is a "struct lguest_device_desc" followed by a +* 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) +{ +return (void *)(dev->desc +1 ) + + dev->desc->num_vq *sizeof (struct lguest_vqconfig ) + + dev->desc->feature_len *2 ; } -/* Each device descriptor is followed by some configuration information. -* Each configuration field looks like: u8 type, u8 len, [... len bytes...]. -* -* This routine adds a new field to 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,u8 type,u8 len,const void *c) +/* This routine allocates a new "struct lguest_device_desc" from descriptor +* table page just above the Guest's normal memory. It returns a pointer to +* that descriptor. */ +static struct lguest_device_desc *new_dev_desc (u16 type) { -/* This is the last descriptor, right? */ -assert (devices.descpage + devices.desc_used - == (u8 *)(dev->desc +1 ) + dev->desc->config_len); +struct lguest_device_desc d = { .type = type }; +void *p; -/* We only have one page of device descriptions. */ -if (devices.desc_used +2 + len >getpagesize ()) -errx (1 ,"Too many devices" ); +/* Figure out where the next device config is, based on the last one. */ +if (devices.lastdev) + p =device_config (devices.lastdev) + + devices.lastdev->desc->config_len; +else + p = devices.descpage; -/* Copy in the new config header: type then length. */ - devices.descpage[devices.desc_used++] = type; - devices.descpage[devices.desc_used++] = len; -memcpy (devices.descpage + devices.desc_used, c, len); - devices.desc_used += len; +/* We only have one page for all the descriptors. */ +if (p +sizeof (d) > (void *)devices.descpage +getpagesize ()) +errx (1 ,"Too many devices" ); -/* Update the device descriptor length: two byte head then data. */ - dev->desc->config_len +=2 + len; +/* p might not be aligned, so we memcpy in. */ +return memcpy (p, &d,sizeof (d)); } -/* This routine adds a virtqueue to a device. We specify how many descriptors -* the virtqueue is to have. */ +/* Each device descriptor is followed by the description of its virtqueues. We +* specify how many descriptors the virtqueue is to have. */ static void add_virtqueue (struct device *dev,unsigned int num_descs,void (*handle_output)(int fd,struct virtqueue *me)) {
@@ -1047,9 +1036,15 @@ static void add_virtqueue(struct device *dev, unsigned int num_descs,/* Initialize the vring. */ vring_init (&vq->vring, num_descs, p); -/* Add the configuration information to this device's descriptor. */ -add_desc_field (dev,VIRTIO_CONFIG_F_VIRTQUEUE , -sizeof (vq->config), &vq->config); +/* Append virtqueue to this device's descriptor. We use +* device_config() to get the end of the device's current virtqueues; +* we check that we haven't added any config or feature information +* yet, otherwise we'd be overwriting them. */ +assert (dev->desc->config_len ==0 && dev->desc->feature_len ==0 ); +memcpy (device_config (dev), &vq->config,sizeof (vq->config)); + dev->desc->num_vq++; + +verbose ("Virtqueue page %#lx\n" ,to_guest_phys (p));/* Add to tail of list, so dev->vq is first vq, dev->vq->next is * second. */
@@ -1068,6 +1063,43 @@ static void add_virtqueue(struct device *dev, unsigned int num_descs, vq->vring.used->flags =VRING_USED_F_NO_NOTIFY ; } +/* The virtqueue descriptors are followed by feature bytes. */ +static void add_feature (struct device *dev,unsigned bit) +{ +u8 *features; + +/* We can't extend the feature bits once we've added config bytes */ +if (dev->desc->feature_len <= bit /CHAR_BIT ) { +assert (dev->desc->config_len ==0 ); + dev->desc->feature_len = (bit /CHAR_BIT ) +1 ; + } + + features = (u8 *)(dev->desc +1 ) + + dev->desc->num_vq *sizeof (struct lguest_vqconfig ); + + features[bit /CHAR_BIT ] |= (1 << (bit %CHAR_BIT )); +} + +/* This routine adds a new configuration field to 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; + +/* Check we haven't overflowed our single page. */ +if (config + dev->desc->config_len > devices.descpage +getpagesize ()) +errx (1 ,"Too many devices" ); + +/* Copy in the config information. */ +memcpy (config + off, c, len); +} +/* This routine does all the creation and setup of a new device, including * calling new_dev_desc() to allocate the descriptor and device memory. */ static struct device *new_device(const char *name, u16 type, int fd,
@@ -1075,14 +1107,6 @@ static struct device *new_device(const char *name, u16 type, int fd, {struct device *dev =malloc (sizeof (*dev)); -/* Append to device list. Prepending to a single-linked list is -* easier, but the user expects the devices to be arranged on the bus -* in command-line order. The first network device on the command line -* is eth0, the first block device /dev/vda, etc. */ - *devices.lastdev = dev; - dev->next =NULL ; - devices.lastdev = &dev->next; -/* Now we populate the fields one at a time. */ dev->fd = fd;/* If we have an input handler for this file descriptor, then we add it
@@ -1092,6 +1116,17 @@ static struct device *new_device(const char *name, u16 type, int fd, dev->desc =new_dev_desc (type); dev->handle_input = handle_input; dev->name = name; + +/* Append to device list. Prepending to a single-linked list is +* easier, but the user expects the devices to be arranged on the bus +* in command-line order. The first network device on the command line +* is eth0, the first block device /dev/vda, etc. */ +if (devices.lastdev) + devices.lastdev->next = dev; +else + devices.dev = dev; + devices.lastdev = dev; +return dev; }
@@ -1258,9 +1293,10 @@ static void setup_tun_net(const char *arg)configure_device (ipfd, ifr.ifr_name, ip, hwaddr);/* 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); -/* We don't seed the socket any more; setup is done. */ +/* We don't need the socket any more; setup is done. */ close (ipfd); verbose("device %u: tun net %u.%u.%u.%u\n",
@@ -1467,6 +1503,11 @@ static void setup_block_file(const char *filename) 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 ); +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);
@@ -1570,12 +1623,12 @@ int main(int argc, char *argv[])/* First we initialize the device list. Since console and network * device receive input from a file descriptor, we keep an fdset * (infds) and the maximum fd number (max_infd) with the head of the -* list. We also keep a pointer to the last device, for easy appending -* to the list. Finally, we keep the next interrupt number to hand out -* (1: remember that 0 is used by the timer). */ +* list. We also keep a pointer to the last device. Finally, we keep +* the next interrupt number to hand out (1: remember that 0 is used by +* the timer). */ FD_ZERO (&devices.infds); devices.max_infd =-1 ; - devices.lastdev = &devices.dev; + devices.lastdev =NULL ; devices.next_irq =1 ;/* We need to know how much memory so we can set up the device
drivers/block/virtio_blk.c +10 -20
@@ -162,8 +162,6 @@ static int virtblk_probe(struct virtio_device *vdev) {struct virtio_blk *vblk;int err, major; -void *token; -unsigned int len;u64 cap;u32 v;
@@ -178,7 +176,7 @@ static int virtblk_probe(struct virtio_device *vdev) vblk->vdev = vdev;/* We expect one virtqueue, for output. */ - vblk->vq = vdev->config->find_vq (vdev, blk_done); + vblk->vq = vdev->config->find_vq (vdev,0 , blk_done);if (IS_ERR (vblk->vq)) { err =PTR_ERR (vblk->vq); goto out_free_vblk;
@@ -216,16 +214,12 @@ static int virtblk_probe(struct virtio_device *vdev) vblk->disk->fops = &virtblk_fops;/* If barriers are supported, tell block layer that queue is ordered */ - token = vdev->config->find (vdev,VIRTIO_CONFIG_BLK_F , &len); -if (virtio_use_bit (vdev, token, len,VIRTIO_BLK_F_BARRIER )) +if (vdev->config->feature (vdev,VIRTIO_BLK_F_BARRIER ))blk_queue_ordered (vblk->disk->queue,QUEUE_ORDERED_TAG ,NULL ); - err =virtio_config_val (vdev,VIRTIO_CONFIG_BLK_F_CAPACITY , &cap); -if (err) { -dev_err (&vdev->dev,"Bad/missing capacity in config\n" ); - goto out_put_disk; - } +/* Host must always specify the capacity. */ +__virtio_config_val (vdev,VIRTIO_CONFIG_BLK_F_CAPACITY , &cap);/* If capacity is too big, truncate with warning. */ if ((sector_t )cap != cap) { dev_warn(&vdev->dev, "Capacity %llu too large: truncating\n",
@@ -234,21 +229,17 @@ static int virtblk_probe(struct virtio_device *vdev) }set_capacity (vblk->disk, cap); - err =virtio_config_val (vdev,VIRTIO_CONFIG_BLK_F_SIZE_MAX , &v); +/* 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);if (!err)blk_queue_max_segment_size (vblk->disk->queue, v); -else if (err != -ENOENT ) { -dev_err (&vdev->dev,"Bad SIZE_MAX in config\n" ); - goto out_put_disk; - } - err =virtio_config_val (vdev,VIRTIO_CONFIG_BLK_F_SEG_MAX , &v); + err =virtio_config_val (vdev,VIRTIO_BLK_F_SEG_MAX , +VIRTIO_CONFIG_BLK_F_SEG_MAX , &v);if (!err)blk_queue_max_hw_segments (vblk->disk->queue, v); -else if (err != -ENOENT ) { -dev_err (&vdev->dev,"Bad SEG_MAX in config\n" ); - goto out_put_disk; - }add_disk (vblk->disk);return 0 ;
drivers/char/virtio_console.c +2 -2
@@ -158,13 +158,13 @@ static int __devinit virtcons_probe(struct virtio_device *dev)/* Find the input queue. */ /* FIXME: This is why we want to wean off hvc: we do nothing * when input comes in. */ - in_vq = vdev->config->find_vq (vdev,NULL ); + in_vq = vdev->config->find_vq (vdev,0 ,NULL );if (IS_ERR (in_vq)) { err =PTR_ERR (in_vq); goto free; } - out_vq = vdev->config->find_vq (vdev,NULL ); + out_vq = vdev->config->find_vq (vdev,1 ,NULL );if (IS_ERR (out_vq)) { err =PTR_ERR (out_vq); goto free_in_vq;
drivers/lguest/lguest_device.c +76 -58
192 lines not on this page · raw message
drivers/net/virtio_net.c +10 -14
@@ -320,10 +320,8 @@ static int virtnet_close(struct net_device *dev)static int virtnet_probe (struct virtio_device *vdev) {int err; -unsigned int len;struct net_device *dev;struct virtnet_info *vi; -void *token;/* Allocate ourselves a network device with room for our info */ dev =alloc_etherdev (sizeof (struct virtnet_info ));
@@ -339,25 +337,23 @@ static int virtnet_probe(struct virtio_device *vdev)SET_NETDEV_DEV (dev, &vdev->dev);/* Do we support "hardware" checksums? */ - token = vdev->config->find (vdev,VIRTIO_CONFIG_NET_F , &len); -if (virtio_use_bit (vdev, token, len,VIRTIO_NET_F_NO_CSUM )) { +if (vdev->config->feature (vdev,VIRTIO_NET_F_NO_CSUM )) {/* This opens up the world of extra features. */ dev->features |=NETIF_F_HW_CSUM |NETIF_F_SG |NETIF_F_FRAGLIST ; -if (virtio_use_bit (vdev, token, len,VIRTIO_NET_F_TSO4 )) +if (vdev->config->feature (vdev,VIRTIO_NET_F_TSO4 )) dev->features |=NETIF_F_TSO ; -if (virtio_use_bit (vdev, token, len,VIRTIO_NET_F_UFO )) +if (vdev->config->feature (vdev,VIRTIO_NET_F_UFO )) dev->features |=NETIF_F_UFO ; -if (virtio_use_bit (vdev, token, len,VIRTIO_NET_F_TSO4_ECN )) +if (vdev->config->feature (vdev,VIRTIO_NET_F_TSO4_ECN )) dev->features |=NETIF_F_TSO_ECN ; -if (virtio_use_bit (vdev, token, len,VIRTIO_NET_F_TSO6 )) +if (vdev->config->feature (vdev,VIRTIO_NET_F_TSO6 )) dev->features |=NETIF_F_TSO6 ; }/* Configuration may specify what MAC to use. Otherwise random. */ - token = vdev->config->find (vdev,VIRTIO_CONFIG_NET_MAC_F , &len); -if (token) { - dev->addr_len = len; - vdev->config->get (vdev, token, dev->dev_addr, len); +if (vdev->config->feature (vdev,VIRTIO_NET_F_MAC )) { + vdev->config->get (vdev,VIRTIO_CONFIG_NET_MAC_F , dev->dev_addr, + dev->addr_len); }else random_ether_addr (dev->dev_addr);
@@ -368,13 +364,13 @@ static int virtnet_probe(struct virtio_device *vdev) vi->vdev = vdev;/* We expect two virtqueues, receive then send. */ - vi->rvq = vdev->config->find_vq (vdev, skb_recv_done); + vi->rvq = vdev->config->find_vq (vdev,0 , skb_recv_done);if (IS_ERR (vi->rvq)) { err =PTR_ERR (vi->rvq); goto free; } - vi->svq = vdev->config->find_vq (vdev, skb_xmit_done); + vi->svq = vdev->config->find_vq (vdev,1 , skb_xmit_done);if (IS_ERR (vi->svq)) { err =PTR_ERR (vi->svq); goto free_recv;
drivers/virtio/virtio.c +0 -45
51 lines not on this page · raw message
include/linux/lguest_launcher.h +7 -2
@@ -23,7 +23,12 @@struct lguest_device_desc {/* The device type: console, network, disk etc. Type 0 terminates. */ __u8 type; -/* The number of bytes of the config array. */ +/* The number of virtqueues (first in config array) */ +__u8 num_vq; +/* The number of bytes of feature bits. Multiply by 2: one for host +* features and one for guest acknowledgements. */ +__u8 feature_len; +/* The number of bytes of the config array after virtqueues. */ __u8 config_len;/* A status byte, written by the Guest. */ __u8 status;
@@ -31,7 +36,7 @@ struct lguest_device_desc { };/*D:135 This is how we expect the device configuration field for a virtqueue -* (type VIRTIO_CONFIG_F_VIRTQUEUE) to be laid out: */ +* to be laid out in config space. */ struct lguest_vqconfig {/* The number of entries in the virtio_ring */ __u16 num;
include/linux/virtio_blk.h +10 -9
@@ -6,15 +6,16 @@#define VIRTIO_ID_BLOCK 2/* Feature bits */ -#define VIRTIO_CONFIG_BLK_F 0x40 -#define VIRTIO_BLK_F_BARRIER 1/* Does host support barriers? */ - -/* The capacity (in 512-byte sectors). */ -#define VIRTIO_CONFIG_BLK_F_CAPACITY 0x41 -/* The maximum segment size. */ -#define VIRTIO_CONFIG_BLK_F_SIZE_MAX 0x42 -/* The maximum number of segments. */ -#define VIRTIO_CONFIG_BLK_F_SEG_MAX 0x43 +#define VIRTIO_BLK_F_BARRIER 0/* Does host support barriers? */ +#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/* These two define direction. */ #define VIRTIO_BLK_T_IN 0
include/linux/virtio_config.h +46 -52
149 lines not on this page · raw message
include/linux/virtio_net.h +4 -4
@@ -5,16 +5,16 @@/* The ID for virtio_net */ #define VIRTIO_ID_NET 1 -/* The bitmap of config for virtio net */ -#define VIRTIO_CONFIG_NET_F 0x40 +/* The feature bitmap for virtio net */ #define VIRTIO_NET_F_NO_CSUM 0#define VIRTIO_NET_F_TSO4 1#define VIRTIO_NET_F_UFO 2#define VIRTIO_NET_F_TSO4_ECN 3#define VIRTIO_NET_F_TSO6 4 +#define VIRTIO_NET_F_MAC 5 -/* The config defining mac address. */ -#define VIRTIO_CONFIG_NET_MAC_F 0x41 +/* The config defining mac address (6 bytes) */ +#define VIRTIO_CONFIG_NET_MAC_F 0/* 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 1/1 virtio config_ops refactoring
In reply to Anthony LiguoriAfter discussion with Anthony, the virtio config has been simplified. We lose some minor features (the virtio_net address must now be 6 bytes) but it turns out to be a wash in terms of complexity, while simplifying PCI.
This can be found in the new virtio git tree, in the "patches/1" branch (new branches will be created as I rebase to keep up with Linus).
git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux-2.6-virtio.git
I've also posted below, for easy commentry.
Cheers,
Rusty.
===
Previously we used a type/len pair within the config space, but this seems overkill. We now simply use an agreed offset within the config space and assume everyone knows the size of any entry it is interested in (the config space can now only be extended at the end).
The main driver-visible change is that we indicate what fields are present with an explicit feature bit.
Documentation/lguest/lguest.c +101 -60
@@ -34,6 +34,7 @@#include <zlib.h> #include <assert.h> #include <sched.h> +#include <limits.h> #include "linux/lguest_launcher.h" #include "linux/virtio_config.h" #include "linux/virtio_net.h"
@@ -96,13 +97,11 @@ struct device_list/* The descriptor page for the devices. */ u8 *descpage; -/* The tail of the last descriptor. */ -unsigned int desc_used; -/* A single linked list of devices. */ struct device *dev; -/* ... And an end pointer so we can easily append new devices */ -struct device **lastdev; +/* And a pointer to the last device for easy append and also for +* configuration appending. */ +struct device *lastdev; };/* The list of Guest devices, based on command line arguments. */
@@ -980,54 +979,44 @@ static void handle_input(int fd)* * All devices need a descriptor so the Guest knows it exists, and a "struct * device" so the Launcher can keep track of it. We have common helper -* routines to allocate them. -* -* This routine allocates a new "struct lguest_device_desc" from descriptor -* table just above the Guest's normal memory. It returns a pointer to that -* descriptor. */ -static struct lguest_device_desc *new_dev_desc (u16 type) -{ -struct lguest_device_desc *d; - -/* We only have one page for all the descriptors. */ -if (devices.desc_used +sizeof (*d) >getpagesize ()) -errx (1 ,"Too many devices" ); - -/* We don't need to set config_len or status: page is 0 already. */ - d = (void *)devices.descpage + devices.desc_used; - d->type = type; - devices.desc_used +=sizeof (*d); +* routines to allocate and manage them. */ -return d; +/* The layout of the device page is a "struct lguest_device_desc" followed by a +* 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) +{ +return (void *)(dev->desc +1 ) + + dev->desc->num_vq *sizeof (struct lguest_vqconfig ) + + dev->desc->feature_len *2 ; } -/* Each device descriptor is followed by some configuration information. -* Each configuration field looks like: u8 type, u8 len, [... len bytes...]. -* -* This routine adds a new field to 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,u8 type,u8 len,const void *c) +/* This routine allocates a new "struct lguest_device_desc" from descriptor +* table page just above the Guest's normal memory. It returns a pointer to +* that descriptor. */ +static struct lguest_device_desc *new_dev_desc (u16 type) { -/* This is the last descriptor, right? */ -assert (devices.descpage + devices.desc_used - == (u8 *)(dev->desc +1 ) + dev->desc->config_len); +struct lguest_device_desc d = { .type = type }; +void *p; -/* We only have one page of device descriptions. */ -if (devices.desc_used +2 + len >getpagesize ()) -errx (1 ,"Too many devices" ); +/* Figure out where the next device config is, based on the last one. */ +if (devices.lastdev) + p =device_config (devices.lastdev) + + devices.lastdev->desc->config_len; +else + p = devices.descpage; -/* Copy in the new config header: type then length. */ - devices.descpage[devices.desc_used++] = type; - devices.descpage[devices.desc_used++] = len; -memcpy (devices.descpage + devices.desc_used, c, len); - devices.desc_used += len; +/* We only have one page for all the descriptors. */ +if (p +sizeof (d) > (void *)devices.descpage +getpagesize ()) +errx (1 ,"Too many devices" ); -/* Update the device descriptor length: two byte head then data. */ - dev->desc->config_len +=2 + len; +/* p might not be aligned, so we memcpy in. */ +return memcpy (p, &d,sizeof (d)); } -/* This routine adds a virtqueue to a device. We specify how many descriptors -* the virtqueue is to have. */ +/* Each device descriptor is followed by the description of its virtqueues. We +* specify how many descriptors the virtqueue is to have. */ static void add_virtqueue (struct device *dev,unsigned int num_descs,void (*handle_output)(int fd,struct virtqueue *me)) {
@@ -1047,9 +1036,15 @@ static void add_virtqueue(struct device *dev, unsigned int num_descs,/* Initialize the vring. */ vring_init (&vq->vring, num_descs, p); -/* Add the configuration information to this device's descriptor. */ -add_desc_field (dev,VIRTIO_CONFIG_F_VIRTQUEUE , -sizeof (vq->config), &vq->config); +/* Append virtqueue to this device's descriptor. We use +* device_config() to get the end of the device's current virtqueues; +* we check that we haven't added any config or feature information +* yet, otherwise we'd be overwriting them. */ +assert (dev->desc->config_len ==0 && dev->desc->feature_len ==0 ); +memcpy (device_config (dev), &vq->config,sizeof (vq->config)); + dev->desc->num_vq++; + +verbose ("Virtqueue page %#lx\n" ,to_guest_phys (p));/* Add to tail of list, so dev->vq is first vq, dev->vq->next is * second. */
@@ -1068,6 +1063,43 @@ static void add_virtqueue(struct device *dev, unsigned int num_descs, vq->vring.used->flags =VRING_USED_F_NO_NOTIFY ; } +/* The virtqueue descriptors are followed by feature bytes. */ +static void add_feature (struct device *dev,unsigned bit) +{ +u8 *features; + +/* We can't extend the feature bits once we've added config bytes */ +if (dev->desc->feature_len <= bit /CHAR_BIT ) { +assert (dev->desc->config_len ==0 ); + dev->desc->feature_len = (bit /CHAR_BIT ) +1 ; + } + + features = (u8 *)(dev->desc +1 ) + + dev->desc->num_vq *sizeof (struct lguest_vqconfig ); + + features[bit /CHAR_BIT ] |= (1 << (bit %CHAR_BIT )); +} + +/* This routine adds a new configuration field to 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; + +/* Check we haven't overflowed our single page. */ +if (config + dev->desc->config_len > devices.descpage +getpagesize ()) +errx (1 ,"Too many devices" ); + +/* Copy in the config information. */ +memcpy (config + off, c, len); +} +/* This routine does all the creation and setup of a new device, including * calling new_dev_desc() to allocate the descriptor and device memory. */ static struct device *new_device(const char *name, u16 type, int fd,
@@ -1075,14 +1107,6 @@ static struct device *new_device(const char *name, u16 type, int fd, {struct device *dev =malloc (sizeof (*dev)); -/* Append to device list. Prepending to a single-linked list is -* easier, but the user expects the devices to be arranged on the bus -* in command-line order. The first network device on the command line -* is eth0, the first block device /dev/vda, etc. */ - *devices.lastdev = dev; - dev->next =NULL ; - devices.lastdev = &dev->next; -/* Now we populate the fields one at a time. */ dev->fd = fd;/* If we have an input handler for this file descriptor, then we add it
@@ -1092,6 +1116,17 @@ static struct device *new_device(const char *name, u16 type, int fd, dev->desc =new_dev_desc (type); dev->handle_input = handle_input; dev->name = name; + +/* Append to device list. Prepending to a single-linked list is +* easier, but the user expects the devices to be arranged on the bus +* in command-line order. The first network device on the command line +* is eth0, the first block device /dev/vda, etc. */ +if (devices.lastdev) + devices.lastdev->next = dev; +else + devices.dev = dev; + devices.lastdev = dev; +return dev; }
@@ -1258,9 +1293,10 @@ static void setup_tun_net(const char *arg)configure_device (ipfd, ifr.ifr_name, ip, hwaddr);/* 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); -/* We don't seed the socket any more; setup is done. */ +/* We don't need the socket any more; setup is done. */ close (ipfd); verbose("device %u: tun net %u.%u.%u.%u\n",
@@ -1467,6 +1503,11 @@ static void setup_block_file(const char *filename) 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 ); +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);
@@ -1570,12 +1623,12 @@ int main(int argc, char *argv[])/* First we initialize the device list. Since console and network * device receive input from a file descriptor, we keep an fdset * (infds) and the maximum fd number (max_infd) with the head of the -* list. We also keep a pointer to the last device, for easy appending -* to the list. Finally, we keep the next interrupt number to hand out -* (1: remember that 0 is used by the timer). */ +* list. We also keep a pointer to the last device. Finally, we keep +* the next interrupt number to hand out (1: remember that 0 is used by +* the timer). */ FD_ZERO (&devices.infds); devices.max_infd =-1 ; - devices.lastdev = &devices.dev; + devices.lastdev =NULL ; devices.next_irq =1 ;/* We need to know how much memory so we can set up the device
drivers/block/virtio_blk.c +10 -20
@@ -162,8 +162,6 @@ static int virtblk_probe(struct virtio_device *vdev) {struct virtio_blk *vblk;int err, major; -void *token; -unsigned int len;u64 cap;u32 v;
@@ -178,7 +176,7 @@ static int virtblk_probe(struct virtio_device *vdev) vblk->vdev = vdev;/* We expect one virtqueue, for output. */ - vblk->vq = vdev->config->find_vq (vdev, blk_done); + vblk->vq = vdev->config->find_vq (vdev,0 , blk_done);if (IS_ERR (vblk->vq)) { err =PTR_ERR (vblk->vq); goto out_free_vblk;
@@ -216,16 +214,12 @@ static int virtblk_probe(struct virtio_device *vdev) vblk->disk->fops = &virtblk_fops;/* If barriers are supported, tell block layer that queue is ordered */ - token = vdev->config->find (vdev,VIRTIO_CONFIG_BLK_F , &len); -if (virtio_use_bit (vdev, token, len,VIRTIO_BLK_F_BARRIER )) +if (vdev->config->feature (vdev,VIRTIO_BLK_F_BARRIER ))blk_queue_ordered (vblk->disk->queue,QUEUE_ORDERED_TAG ,NULL ); - err =virtio_config_val (vdev,VIRTIO_CONFIG_BLK_F_CAPACITY , &cap); -if (err) { -dev_err (&vdev->dev,"Bad/missing capacity in config\n" ); - goto out_put_disk; - } +/* Host must always specify the capacity. */ +__virtio_config_val (vdev,VIRTIO_CONFIG_BLK_F_CAPACITY , &cap);/* If capacity is too big, truncate with warning. */ if ((sector_t )cap != cap) { dev_warn(&vdev->dev, "Capacity %llu too large: truncating\n",
@@ -234,21 +229,17 @@ static int virtblk_probe(struct virtio_device *vdev) }set_capacity (vblk->disk, cap); - err =virtio_config_val (vdev,VIRTIO_CONFIG_BLK_F_SIZE_MAX , &v); +/* 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);if (!err)blk_queue_max_segment_size (vblk->disk->queue, v); -else if (err != -ENOENT ) { -dev_err (&vdev->dev,"Bad SIZE_MAX in config\n" ); - goto out_put_disk; - } - err =virtio_config_val (vdev,VIRTIO_CONFIG_BLK_F_SEG_MAX , &v); + err =virtio_config_val (vdev,VIRTIO_BLK_F_SEG_MAX , +VIRTIO_CONFIG_BLK_F_SEG_MAX , &v);if (!err)blk_queue_max_hw_segments (vblk->disk->queue, v); -else if (err != -ENOENT ) { -dev_err (&vdev->dev,"Bad SEG_MAX in config\n" ); - goto out_put_disk; - }add_disk (vblk->disk);return 0 ;
drivers/char/virtio_console.c +2 -2
@@ -158,13 +158,13 @@ static int __devinit virtcons_probe(struct virtio_device *dev)/* Find the input queue. */ /* FIXME: This is why we want to wean off hvc: we do nothing * when input comes in. */ - in_vq = vdev->config->find_vq (vdev,NULL ); + in_vq = vdev->config->find_vq (vdev,0 ,NULL );if (IS_ERR (in_vq)) { err =PTR_ERR (in_vq); goto free; } - out_vq = vdev->config->find_vq (vdev,NULL ); + out_vq = vdev->config->find_vq (vdev,1 ,NULL );if (IS_ERR (out_vq)) { err =PTR_ERR (out_vq); goto free_in_vq;
drivers/lguest/lguest_device.c +76 -58
192 lines not on this page · raw message
drivers/net/virtio_net.c +10 -14
@@ -320,10 +320,8 @@ static int virtnet_close(struct net_device *dev)static int virtnet_probe (struct virtio_device *vdev) {int err; -unsigned int len;struct net_device *dev;struct virtnet_info *vi; -void *token;/* Allocate ourselves a network device with room for our info */ dev =alloc_etherdev (sizeof (struct virtnet_info ));
@@ -339,25 +337,23 @@ static int virtnet_probe(struct virtio_device *vdev)SET_NETDEV_DEV (dev, &vdev->dev);/* Do we support "hardware" checksums? */ - token = vdev->config->find (vdev,VIRTIO_CONFIG_NET_F , &len); -if (virtio_use_bit (vdev, token, len,VIRTIO_NET_F_NO_CSUM )) { +if (vdev->config->feature (vdev,VIRTIO_NET_F_NO_CSUM )) {/* This opens up the world of extra features. */ dev->features |=NETIF_F_HW_CSUM |NETIF_F_SG |NETIF_F_FRAGLIST ; -if (virtio_use_bit (vdev, token, len,VIRTIO_NET_F_TSO4 )) +if (vdev->config->feature (vdev,VIRTIO_NET_F_TSO4 )) dev->features |=NETIF_F_TSO ; -if (virtio_use_bit (vdev, token, len,VIRTIO_NET_F_UFO )) +if (vdev->config->feature (vdev,VIRTIO_NET_F_UFO )) dev->features |=NETIF_F_UFO ; -if (virtio_use_bit (vdev, token, len,VIRTIO_NET_F_TSO4_ECN )) +if (vdev->config->feature (vdev,VIRTIO_NET_F_TSO4_ECN )) dev->features |=NETIF_F_TSO_ECN ; -if (virtio_use_bit (vdev, token, len,VIRTIO_NET_F_TSO6 )) +if (vdev->config->feature (vdev,VIRTIO_NET_F_TSO6 )) dev->features |=NETIF_F_TSO6 ; }/* Configuration may specify what MAC to use. Otherwise random. */ - token = vdev->config->find (vdev,VIRTIO_CONFIG_NET_MAC_F , &len); -if (token) { - dev->addr_len = len; - vdev->config->get (vdev, token, dev->dev_addr, len); +if (vdev->config->feature (vdev,VIRTIO_NET_F_MAC )) { + vdev->config->get (vdev,VIRTIO_CONFIG_NET_MAC_F , dev->dev_addr, + dev->addr_len); }else random_ether_addr (dev->dev_addr);
@@ -368,13 +364,13 @@ static int virtnet_probe(struct virtio_device *vdev) vi->vdev = vdev;/* We expect two virtqueues, receive then send. */ - vi->rvq = vdev->config->find_vq (vdev, skb_recv_done); + vi->rvq = vdev->config->find_vq (vdev,0 , skb_recv_done);if (IS_ERR (vi->rvq)) { err =PTR_ERR (vi->rvq); goto free; } - vi->svq = vdev->config->find_vq (vdev, skb_xmit_done); + vi->svq = vdev->config->find_vq (vdev,1 , skb_xmit_done);if (IS_ERR (vi->svq)) { err =PTR_ERR (vi->svq); goto free_recv;
drivers/virtio/virtio.c +0 -45
51 lines not on this page · raw message
include/linux/lguest_launcher.h +7 -2
@@ -23,7 +23,12 @@struct lguest_device_desc {/* The device type: console, network, disk etc. Type 0 terminates. */ __u8 type; -/* The number of bytes of the config array. */ +/* The number of virtqueues (first in config array) */ +__u8 num_vq; +/* The number of bytes of feature bits. Multiply by 2: one for host +* features and one for guest acknowledgements. */ +__u8 feature_len; +/* The number of bytes of the config array after virtqueues. */ __u8 config_len;/* A status byte, written by the Guest. */ __u8 status;
@@ -31,7 +36,7 @@ struct lguest_device_desc { };/*D:135 This is how we expect the device configuration field for a virtqueue -* (type VIRTIO_CONFIG_F_VIRTQUEUE) to be laid out: */ +* to be laid out in config space. */ struct lguest_vqconfig {/* The number of entries in the virtio_ring */ __u16 num;
include/linux/virtio_blk.h +10 -9
@@ -6,15 +6,16 @@#define VIRTIO_ID_BLOCK 2/* Feature bits */ -#define VIRTIO_CONFIG_BLK_F 0x40 -#define VIRTIO_BLK_F_BARRIER 1/* Does host support barriers? */ - -/* The capacity (in 512-byte sectors). */ -#define VIRTIO_CONFIG_BLK_F_CAPACITY 0x41 -/* The maximum segment size. */ -#define VIRTIO_CONFIG_BLK_F_SIZE_MAX 0x42 -/* The maximum number of segments. */ -#define VIRTIO_CONFIG_BLK_F_SEG_MAX 0x43 +#define VIRTIO_BLK_F_BARRIER 0/* Does host support barriers? */ +#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/* These two define direction. */ #define VIRTIO_BLK_T_IN 0
include/linux/virtio_config.h +46 -52
149 lines not on this page · raw message
include/linux/virtio_net.h +4 -4
@@ -5,16 +5,16 @@/* The ID for virtio_net */ #define VIRTIO_ID_NET 1 -/* The bitmap of config for virtio net */ -#define VIRTIO_CONFIG_NET_F 0x40 +/* The feature bitmap for virtio net */ #define VIRTIO_NET_F_NO_CSUM 0#define VIRTIO_NET_F_TSO4 1#define VIRTIO_NET_F_UFO 2#define VIRTIO_NET_F_TSO4_ECN 3#define VIRTIO_NET_F_TSO6 4 +#define VIRTIO_NET_F_MAC 5 -/* The config defining mac address. */ -#define VIRTIO_CONFIG_NET_MAC_F 0x41 +/* The config defining mac address (6 bytes) */ +#define VIRTIO_CONFIG_NET_MAC_F 0/* 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. */
Quoted 4 lines · Rusty Russell
After discussion with Anthony, the virtio config has been simplified. We lose some minor features (the
virtio_netaddress must now be 6 bytes) but it turns out to be a wash in terms of complexity, while simplifying PCI.
Hi Rusty,
Thanks for posting this! It's really simplified things for me.
Quoted 18 lines
include/linux/virtio_config.h +5
-10-/** -* virtio_use_bit - helper to use a feature bit in a bitfield value. -* @dev: the virtio device -* @token: the token as returned from vdev->config->find(). -* @len: the length of the field. -* @bitnum: the bit to test. +* __virtio_config_val - get a single virtio config without feature check. +* @vdev: the virtio device +* @offset: the type to search for. +* @val: a pointer to the value to fill in. * -* If handed a NULL token, it returns false, otherwise returns bit status. -* If it's one, it sets the mirroring acknowledgement bit. */ -int virtio_use_bit (struct virtio_device *vdev, -void *token,unsigned int len,unsigned int bitnum); +* The value is endian-corrected and returned in v. */
include/linux/virtio_config.h struct virtio_device; · line 103
Quoted 10 lines
include/linux/virtio_config.h +10
-0+#define __virtio_config_val (vdev, offset, v) do { \ + BUILD_BUG_ON(sizeof(*(v)) != 1 && sizeof(*(v)) != 2 \ + && sizeof(*(v)) != 4 && sizeof(*(v)) != 8); \ + (vdev)->config->get((vdev), (offset), (v), sizeof(*(v))); \ + switch (sizeof(*(v))) { \ + case 2: le16_to_cpus((__u16 *) v); break; \ + case 4: le32_to_cpus((__u32 *) v); break; \ + case 8: le64_to_cpus((__u64 *) v); break; \ + } \ +} while(0)
I would prefer that the virtio API not expose a little endian standard. I'm currently converting config->get() ops to ioreadXX depending on the size which already does the endianness conversion for me so this just messes things up. I think it's better to let the backend deal with endianness since it's trivial to handle for both the PCI backend and the lguest backend (lguest doesn't need to do any endianness conversion).
Regards,
Anthony Liguori
Quoted 28 lines
#endif /*
__KERNEL__*/
#endif /*_LINUX_VIRTIO_CONFIG_H*/include/linux/virtio_net.h +4
-4@@ -5,16 +5,16 @@/* The ID for virtio_net */ #define VIRTIO_ID_NET 1 -/* The bitmap of config for virtio net */ -#define VIRTIO_CONFIG_NET_F 0x40 +/* The feature bitmap for virtio net */ #define VIRTIO_NET_F_NO_CSUM 0#define VIRTIO_NET_F_TSO4 1#define VIRTIO_NET_F_UFO 2#define VIRTIO_NET_F_TSO4_ECN 3#define VIRTIO_NET_F_TSO6 4 +#define VIRTIO_NET_F_MAC 5 -/* The config defining mac address. */ -#define VIRTIO_CONFIG_NET_MAC_F 0x41 +/* The config defining mac address (6 bytes) */ +#define VIRTIO_CONFIG_NET_MAC_F 0/* 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. */
Quoted 4 lines · Rusty Russell
After discussion with Anthony, the virtio config has been simplified. We lose some minor features (the
virtio_netaddress must now be 6 bytes) but it turns out to be a wash in terms of complexity, while simplifying PCI.
Hi Rusty,
Thanks for posting this! It's really simplified things for me.
Quoted 18 lines
include/linux/virtio_config.h +5
-10-/** -* virtio_use_bit - helper to use a feature bit in a bitfield value. -* @dev: the virtio device -* @token: the token as returned from vdev->config->find(). -* @len: the length of the field. -* @bitnum: the bit to test. +* __virtio_config_val - get a single virtio config without feature check. +* @vdev: the virtio device +* @offset: the type to search for. +* @val: a pointer to the value to fill in. * -* If handed a NULL token, it returns false, otherwise returns bit status. -* If it's one, it sets the mirroring acknowledgement bit. */ -int virtio_use_bit (struct virtio_device *vdev, -void *token,unsigned int len,unsigned int bitnum); +* The value is endian-corrected and returned in v. */
include/linux/virtio_config.h struct virtio_device; · line 103
Quoted 10 lines
include/linux/virtio_config.h +10
-0+#define __virtio_config_val (vdev, offset, v) do { \ + BUILD_BUG_ON(sizeof(*(v)) != 1 && sizeof(*(v)) != 2 \ + && sizeof(*(v)) != 4 && sizeof(*(v)) != 8); \ + (vdev)->config->get((vdev), (offset), (v), sizeof(*(v))); \ + switch (sizeof(*(v))) { \ + case 2: le16_to_cpus((__u16 *) v); break; \ + case 4: le32_to_cpus((__u32 *) v); break; \ + case 8: le64_to_cpus((__u64 *) v); break; \ + } \ +} while(0)
I would prefer that the virtio API not expose a little endian standard. I'm currently converting config->get() ops to ioreadXX depending on the size which already does the endianness conversion for me so this just messes things up. I think it's better to let the backend deal with endianness since it's trivial to handle for both the PCI backend and the lguest backend (lguest doesn't need to do any endianness conversion).
Regards,
Anthony Liguori
Quoted 28 lines
#endif /*
__KERNEL__*/
#endif /*_LINUX_VIRTIO_CONFIG_H*/include/linux/virtio_net.h +4
-4@@ -5,16 +5,16 @@/* The ID for virtio_net */ #define VIRTIO_ID_NET 1 -/* The bitmap of config for virtio net */ -#define VIRTIO_CONFIG_NET_F 0x40 +/* The feature bitmap for virtio net */ #define VIRTIO_NET_F_NO_CSUM 0#define VIRTIO_NET_F_TSO4 1#define VIRTIO_NET_F_UFO 2#define VIRTIO_NET_F_TSO4_ECN 3#define VIRTIO_NET_F_TSO6 4 +#define VIRTIO_NET_F_MAC 5 -/* The config defining mac address. */ -#define VIRTIO_CONFIG_NET_MAC_F 0x41 +/* The config defining mac address (6 bytes) */ +#define VIRTIO_CONFIG_NET_MAC_F 0/* 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. */
Quoted 6 lines · On Thursday 08 November 2007 04:30:50 Anthony Liguori wrote:
I would prefer that the virtio API not expose a little endian standard. I'm currently converting
config->get()ops to ioreadXX depending on the size which already does the endianness conversion for me so this just messes things up. I think it's better to let the backend deal with endianness since it's trivial to handle for both the PCI backend and the lguest backend (lguest doesn't need to do any endianness conversion).
-ETOOMUCHMAGIC. We should either expose all the XX interfaces (but this isn't a high-speed interface, so let's not) or not "sometimes" convert endianness. Getting surprises because a field happens to be packed into 4 bytes is counter-intuitive.
Since your most trivial implementation is to do a byte at a time, I don't think you have a good argument on that basis either.
Cheers,
Rusty.
Quoted 6 lines · On Thursday 08 November 2007 04:30:50 Anthony Liguori wrote:
I would prefer that the virtio API not expose a little endian standard. I'm currently converting
config->get()ops to ioreadXX depending on the size which already does the endianness conversion for me so this just messes things up. I think it's better to let the backend deal with endianness since it's trivial to handle for both the PCI backend and the lguest backend (lguest doesn't need to do any endianness conversion).
-ETOOMUCHMAGIC. We should either expose all the XX interfaces (but this isn't a high-speed interface, so let's not) or not "sometimes" convert endianness. Getting surprises because a field happens to be packed into 4 bytes is counter-intuitive.
Since your most trivial implementation is to do a byte at a time, I don't think you have a good argument on that basis either.
Cheers,
Rusty.
Quoted 10 lines · Rusty Russell
Quoted 6 lines · On Thursday 08 November 2007 04:30:50 Anthony Liguori wrote:
I would prefer that the virtio API not expose a little endian standard. I'm currently converting
config->get()ops to ioreadXX depending on the size which already does the endianness conversion for me so this just messes things up. I think it's better to let the backend deal with endianness since it's trivial to handle for both the PCI backend and the lguest backend (lguest doesn't need to do any endianness conversion).
-ETOOMUCHMAGIC. We should either expose all the XX interfaces (but this isn't
a high-speed interface, so let's not) or not "sometimes" convert endianness. Getting surprises because a field happens to be packed into 4 bytes is counter-intuitive.
Then I think it's necessary to expose the XX interfaces. Otherwise, the backend has to deal with doing all register operations at a per-byte granularity which adds a whole lot of complexity on a per-device basis (as opposed to a little complexity once in the transport layer).
You really want to be able to rely on multi-byte atomic operations too when setting values. Otherwise, you need another register to just to signal when it's okay for the device to examine any given register.
Regards,
Anthony Liguori
Quoted 4 lines
Since your most trivial implementation is to do a byte at a time, I don't think you have a good argument on that basis either.
Cheers,
Rusty.
Quoted 10 lines · Rusty Russell
Quoted 6 lines · On Thursday 08 November 2007 04:30:50 Anthony Liguori wrote:
I would prefer that the virtio API not expose a little endian standard. I'm currently converting
config->get()ops to ioreadXX depending on the size which already does the endianness conversion for me so this just messes things up. I think it's better to let the backend deal with endianness since it's trivial to handle for both the PCI backend and the lguest backend (lguest doesn't need to do any endianness conversion).
-ETOOMUCHMAGIC. We should either expose all the XX interfaces (but this isn't
a high-speed interface, so let's not) or not "sometimes" convert endianness. Getting surprises because a field happens to be packed into 4 bytes is counter-intuitive.
Then I think it's necessary to expose the XX interfaces. Otherwise, the backend has to deal with doing all register operations at a per-byte granularity which adds a whole lot of complexity on a per-device basis (as opposed to a little complexity once in the transport layer).
You really want to be able to rely on multi-byte atomic operations too when setting values. Otherwise, you need another register to just to signal when it's okay for the device to examine any given register.
Regards,
Anthony Liguori
Quoted 4 lines
Since your most trivial implementation is to do a byte at a time, I don't think you have a good argument on that basis either.
Cheers,
Rusty.
Quoted 14 lines · On Thursday 08 November 2007 13:41:16 Anthony Liguori wrote:
Quoted 8 lines · Rusty Russell
Quoted 6 lines · On Thursday 08 November 2007 04:30:50 Anthony Liguori wrote:
I would prefer that the virtio API not expose a little endian standard. I'm currently converting
config->get()ops to ioreadXX depending on the size which already does the endianness conversion for me so this just messes things up. I think it's better to let the backend deal with endianness since it's trivial to handle for both the PCI backend and the lguest backend (lguest doesn't need to do any endianness conversion).Quoted 4 lines
-ETOOMUCHMAGIC. We should either expose all the XX interfaces (but this isn't a high-speed interface, so let's not) or not "sometimes" convert endianness. Getting surprises because a field happens to be packed into 4 bytes is counter-intuitive.
Quoted 4 lines
Then I think it's necessary to expose the XX interfaces. Otherwise, the backend has to deal with doing all register operations at a per-byte granularity which adds a whole lot of complexity on a per-device basis (as opposed to a little complexity once in the transport layer).
Huh? Take a look at the drivers, this simply isn't true. Do you have evidence that it will be true later?
Plus your code will be smaller doing a single writeb/readb loop than trying to do a switch statement.
Quoted 3 lines
You really want to be able to rely on multi-byte atomic operations too when setting values. Otherwise, you need another register to just to signal when it's okay for the device to examine any given register.
You already do; the status register fills this role. For example, you can't tell what features a guest understands until it updates the status register.
Hope that clarifies,
Rusty.
Quoted 14 lines · On Thursday 08 November 2007 13:41:16 Anthony Liguori wrote:
Quoted 8 lines · Rusty Russell
Quoted 6 lines · On Thursday 08 November 2007 04:30:50 Anthony Liguori wrote:
I would prefer that the virtio API not expose a little endian standard. I'm currently converting
config->get()ops to ioreadXX depending on the size which already does the endianness conversion for me so this just messes things up. I think it's better to let the backend deal with endianness since it's trivial to handle for both the PCI backend and the lguest backend (lguest doesn't need to do any endianness conversion).Quoted 4 lines
-ETOOMUCHMAGIC. We should either expose all the XX interfaces (but this isn't a high-speed interface, so let's not) or not "sometimes" convert endianness. Getting surprises because a field happens to be packed into 4 bytes is counter-intuitive.
Quoted 4 lines
Then I think it's necessary to expose the XX interfaces. Otherwise, the backend has to deal with doing all register operations at a per-byte granularity which adds a whole lot of complexity on a per-device basis (as opposed to a little complexity once in the transport layer).
Huh? Take a look at the drivers, this simply isn't true. Do you have evidence that it will be true later?
Plus your code will be smaller doing a single writeb/readb loop than trying to do a switch statement.
Quoted 3 lines
You really want to be able to rely on multi-byte atomic operations too when setting values. Otherwise, you need another register to just to signal when it's okay for the device to examine any given register.
You already do; the status register fills this role. For example, you can't tell what features a guest understands until it updates the status register.
Hope that clarifies,
Rusty.
Quoted 18 lines · Rusty Russell
Quoted 16 lines · On Thursday 08 November 2007 13:41:16 Anthony Liguori wrote:
Quoted 14 lines · Rusty Russell
Quoted 7 lines · On Thursday 08 November 2007 04:30:50 Anthony Liguori wrote:
I would prefer that the virtio API not expose a little endian standard. I'm currently converting
config->get()ops to ioreadXX depending on the size which already does the endianness conversion for me so this just messes things up. I think it's better to let the backend deal with endianness since it's trivial to handle for both the PCI backend and the lguest backend (lguest doesn't need to do any endianness conversion).
-ETOOMUCHMAGIC. We should either expose all the XX interfaces (but this isn't a high-speed interface, so let's not) or not "sometimes" convert endianness. Getting surprises because a field happens to be packed into 4 bytes is counter-intuitive.
Quoted 8 lines
Quoted 5 lines
Then I think it's necessary to expose the XX interfaces. Otherwise, the backend has to deal with doing all register operations at a per-byte granularity which adds a whole lot of complexity on a per-device basis (as opposed to a little complexity once in the transport layer).
Huh? Take a look at the drivers, this simply isn't true. Do you have evidence that it will be true later?
I'm a bit confused. So right now, the higher level virtio functions do endianness conversion. I really want to make sure that if a guest tries to read a 4-byte PCI config field, that it does so using an "outl" instruction so that in my QEMU backend, I don't have to deal with a guest reading/writing a single byte within a 4-byte configuration field. It's the difference between having in the PIO handler:
switch (addr) {
case VIRTIO_BLK_CONFIG_MAX_SEG:
return vdev->max_seg;
case VIRTIO_BLK_CONFIG_MAX_SIZE:
return vdev->max_size;
....
}
and:
switch (addr) {
case VIRTIO_BLK_CONFIG_MAX_SEG:
return vdev->max_seg & 0xFF;
case VIRTIO_BLK_CONFIG_MAX_SEG + 1:
return (vdev->max_seg >> 8) & 0xFF;
case VIRTIO_BLK_CONFIG_MAX_SEG + 2:
return (vdev->max_seg >> 16) & 0xFF;
case VIRTIO_BLK_CONFIG_MAX_SEG + 3:
return (vdev->max_seg >> 24) & 0xFF;
case VIRTIO_BLK_CONFIG_MAX_SIZE:
return vdev->max_size & 0xFF;
case VIRTIO_BLK_CONFIG_MAX_SIZE + 1:
return (vdev->max_size >> 8) & 0xFF;
case VIRTIO_BLK_CONFIG_MAX_SIZE + 2:
return (vdev->max_size >> 16) & 0xFF;
case VIRTIO_BLK_CONFIG_MAX_SIZE + 3:
return (vdev->max_size >> 24) & 0xFF;
...
}
It's the host-side code I'm concerned about, not the guest-side code. I'm happy to just ignore the whole endianness conversion thing and always pass values through in the CPU bitness but it's very important to me that the PCI config registers are accessed with their natural sized instructions (just as they would with a real PCI device).
Regards,
Anthony Liguori
Quoted 14 lines
Plus your code will be smaller doing a single writeb/readb loop than trying to do a switch statement.
Quoted 4 lines
You really want to be able to rely on multi-byte atomic operations too when setting values. Otherwise, you need another register to just to signal when it's okay for the device to examine any given register.
You already do; the status register fills this role. For example, you can't tell what features a guest understands until it updates the status register.
Hope that clarifies,
Rusty.
Quoted 18 lines · Rusty Russell
Quoted 16 lines · On Thursday 08 November 2007 13:41:16 Anthony Liguori wrote:
Quoted 14 lines · Rusty Russell
Quoted 7 lines · On Thursday 08 November 2007 04:30:50 Anthony Liguori wrote:
I would prefer that the virtio API not expose a little endian standard. I'm currently converting
config->get()ops to ioreadXX depending on the size which already does the endianness conversion for me so this just messes things up. I think it's better to let the backend deal with endianness since it's trivial to handle for both the PCI backend and the lguest backend (lguest doesn't need to do any endianness conversion).
-ETOOMUCHMAGIC. We should either expose all the XX interfaces (but this isn't a high-speed interface, so let's not) or not "sometimes" convert endianness. Getting surprises because a field happens to be packed into 4 bytes is counter-intuitive.
Quoted 8 lines
Quoted 5 lines
Then I think it's necessary to expose the XX interfaces. Otherwise, the backend has to deal with doing all register operations at a per-byte granularity which adds a whole lot of complexity on a per-device basis (as opposed to a little complexity once in the transport layer).
Huh? Take a look at the drivers, this simply isn't true. Do you have evidence that it will be true later?
I'm a bit confused. So right now, the higher level virtio functions do endianness conversion. I really want to make sure that if a guest tries to read a 4-byte PCI config field, that it does so using an "outl" instruction so that in my QEMU backend, I don't have to deal with a guest reading/writing a single byte within a 4-byte configuration field. It's the difference between having in the PIO handler:
switch (addr) {
case VIRTIO_BLK_CONFIG_MAX_SEG:
return vdev->max_seg;
case VIRTIO_BLK_CONFIG_MAX_SIZE:
return vdev->max_size;
....
}
and:
switch (addr) {
case VIRTIO_BLK_CONFIG_MAX_SEG:
return vdev->max_seg & 0xFF;
case VIRTIO_BLK_CONFIG_MAX_SEG + 1:
return (vdev->max_seg >> 8) & 0xFF;
case VIRTIO_BLK_CONFIG_MAX_SEG + 2:
return (vdev->max_seg >> 16) & 0xFF;
case VIRTIO_BLK_CONFIG_MAX_SEG + 3:
return (vdev->max_seg >> 24) & 0xFF;
case VIRTIO_BLK_CONFIG_MAX_SIZE:
return vdev->max_size & 0xFF;
case VIRTIO_BLK_CONFIG_MAX_SIZE + 1:
return (vdev->max_size >> 8) & 0xFF;
case VIRTIO_BLK_CONFIG_MAX_SIZE + 2:
return (vdev->max_size >> 16) & 0xFF;
case VIRTIO_BLK_CONFIG_MAX_SIZE + 3:
return (vdev->max_size >> 24) & 0xFF;
...
}
It's the host-side code I'm concerned about, not the guest-side code. I'm happy to just ignore the whole endianness conversion thing and always pass values through in the CPU bitness but it's very important to me that the PCI config registers are accessed with their natural sized instructions (just as they would with a real PCI device).
Regards,
Anthony Liguori
Quoted 14 lines
Plus your code will be smaller doing a single writeb/readb loop than trying to do a switch statement.
Quoted 4 lines
You really want to be able to rely on multi-byte atomic operations too when setting values. Otherwise, you need another register to just to signal when it's okay for the device to examine any given register.
You already do; the status register fills this role. For example, you can't tell what features a guest understands until it updates the status register.
Hope that clarifies,
Rusty.
Hi, I'm sorry, I've been stuck on other things (NFS RDMA anyone?) and missed part of this discussion.
Is it really the case that operations on virtio devices will involve outl/inl etc.?
Apologies in advance for my failure to pay attention.
thanks
ron
Hi, I'm sorry, I've been stuck on other things (NFS RDMA anyone?) and missed part of this discussion.
Is it really the case that operations on virtio devices will involve outl/inl etc.?
Apologies in advance for my failure to pay attention.
thanks
ron
Quoted 6 lines · ron minnich
Hi, I'm sorry, I've been stuck on other things (NFS RDMA anyone?) and missed part of this discussion.
Is it really the case that operations on virtio devices will involve outl/inl etc.?
No, this is just for the PCI virtio transport. lguest's virtio transport uses hypercalls and shared memory.
Regards,
Anthony Liguori
Quoted 6 lines
Apologies in advance for my failure to pay attention.
thanks
ron
Quoted 7 lines · ron minnich
Hi, I'm sorry, I've been stuck on other things (NFS RDMA anyone?) and missed part of this discussion.
Is it really the case that operations on virtio devices will involve outl/inl etc.?
What's the problem with them?
Except for the kick event it's not performance critical and the difference between pio vmexit and hypercall exit is very small.
I don't know about problems in other architectures, maybe mmio is better?
Dor.
Quoted 4 lines
Apologies in advance for my failure to pay attention.
Quoted 8 lines
thanks
ron
_______________________________________________
Lguest mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/lguest
_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Quoted 11 lines · Dor Laor
Quoted 7 lines · ron minnich
Hi, I'm sorry, I've been stuck on other things (NFS RDMA anyone?) and missed part of this discussion.
Is it really the case that operations on virtio devices will involve outl/inl etc.?
What's the problem with them?
Except for the kick event it's not performance critical and the difference between pio vmexit and hypercall exit is very small.
If you're a nutty guy who's interesting in creating the most absolute minimal VMM to run exotic paravirtual guests on massive clusters, then requiring PIO implies that you have to have an instruction decoder which is goes against the earlier goal ;-)
Regards,
Anthony Liguori
Quoted 15 lines
I don't know about problems in other architectures, maybe mmio is better? Dor.
Quoted 12 lines
Apologies in advance for my failure to pay attention.
thanks
ron
_______________________________________________
Lguest mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/lguest
Quoted 11 lines · Dor Laor
Quoted 7 lines · ron minnich
Hi, I'm sorry, I've been stuck on other things (NFS RDMA anyone?) and missed part of this discussion.
Is it really the case that operations on virtio devices will involve outl/inl etc.?
What's the problem with them?
Except for the kick event it's not performance critical and the difference between pio vmexit and hypercall exit is very small.
If you're a nutty guy who's interesting in creating the most absolute minimal VMM to run exotic paravirtual guests on massive clusters, then requiring PIO implies that you have to have an instruction decoder which is goes against the earlier goal ;-)
Regards,
Anthony Liguori
Quoted 15 lines
I don't know about problems in other architectures, maybe mmio is better? Dor.
Quoted 12 lines
Apologies in advance for my failure to pay attention.
thanks
ron
_______________________________________________
Lguest mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/lguest
Quoted 3 lines · On Friday 09 November 2007 09:33:04 Anthony Liguori wrote:
I really want to make sure that if a guest tries
to read a 4-byte PCI config field, that it does so using an "outl" instruction so that in my QEMU backend
So you want to enforce PCI requirements onto virtio config accesses. This doesn't seem very nice: the fact that PCI accesses use different namespaces for different sizes makes sense from a primitive hardware point of view, but sucks for software. Fortunately, if you insist on byte-at-a-time they're the same.
Quoted 17 lines
switch (addr) {
caseVIRTIO_BLK_CONFIG_MAX_SEG:return vdev->max_seg & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SEG + 1: return (vdev->max_seg >> 8) & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SEG + 2: return (vdev->max_seg >> 16) & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SEG + 3: return (vdev->max_seg >> 24) & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SIZE: return vdev->max_size & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SIZE + 1: return (vdev->max_size >> 8) & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SIZE + 2: return (vdev->max_size >> 16) & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SIZE + 3: return (vdev->max_size >> 24) & 0xFF;
Quoted 1 line
...
struct virtio_blk_config
{
uint32_t max_seg, max_size;
};
...
struct virtio_blk_config conf = { vdev->max_seg, vdev->max_size };
return ((unsigned char *)&conf)[addr];(Which strongly implies our headers should expose that nominal struct, rather than numerical constants).
Rusty.
Quoted 3 lines · On Friday 09 November 2007 09:33:04 Anthony Liguori wrote:
I really want to make sure that if a guest tries
to read a 4-byte PCI config field, that it does so using an "outl" instruction so that in my QEMU backend
So you want to enforce PCI requirements onto virtio config accesses. This doesn't seem very nice: the fact that PCI accesses use different namespaces for different sizes makes sense from a primitive hardware point of view, but sucks for software. Fortunately, if you insist on byte-at-a-time they're the same.
Quoted 17 lines
switch (addr) {
caseVIRTIO_BLK_CONFIG_MAX_SEG:return vdev->max_seg & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SEG + 1: return (vdev->max_seg >> 8) & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SEG + 2: return (vdev->max_seg >> 16) & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SEG + 3: return (vdev->max_seg >> 24) & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SIZE: return vdev->max_size & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SIZE + 1: return (vdev->max_size >> 8) & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SIZE + 2: return (vdev->max_size >> 16) & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SIZE + 3: return (vdev->max_size >> 24) & 0xFF;
Quoted 1 line
...
struct virtio_blk_config
{
uint32_t max_seg, max_size;
};
...
struct virtio_blk_config conf = { vdev->max_seg, vdev->max_size };
return ((unsigned char *)&conf)[addr];(Which strongly implies our headers should expose that nominal struct, rather than numerical constants).
Rusty.
Quoted 32 lines · Rusty Russell
Quoted 17 lines · On Friday 09 November 2007 09:33:04 Anthony Liguori wrote:
switch (addr) {
caseVIRTIO_BLK_CONFIG_MAX_SEG:return vdev->max_seg & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SEG + 1: return (vdev->max_seg >> 8) & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SEG + 2: return (vdev->max_seg >> 16) & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SEG + 3: return (vdev->max_seg >> 24) & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SIZE: return vdev->max_size & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SIZE + 1: return (vdev->max_size >> 8) & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SIZE + 2: return (vdev->max_size >> 16) & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SIZE + 3: return (vdev->max_size >> 24) & 0xFF;Quoted 2 lines
...
struct virtio_blk_config { uint32_t max_seg, max_size; }; ... struct virtio_blk_config conf = { vdev->max_seg, vdev->max_size }; return ((unsigned char *)&conf)[addr];
Quoted 2 lines
(Which strongly implies our headers should expose that nominal struct, rather than numerical constants).
The problem is the ABI. We can either require that PCI configuration values are accessed with natural instructions, or it makes very little sense to use the PCI configuration space for virtio configuration information. If we really can't find a way to do this (and I think my current implementation is the best compromise since it hides this from everything else), then I think I'll switch over to just writing a PFN into a PCI configuration slot and then have that page store the virtio configuration information (much like is done with lguest).
Either virtio config looks like a shared memory area (as lguest currently implements it), or it looks like hardware registers (like virtio-pci implements it). After thinking about it for a while, I don't think the two can be reconciled. There are subtle differences between the two that can't be hidden in the virtio interface. For instance, in the PCI model, you get notified when values are read/written whereas in the lguest model, you don't and need explicit status bits.
If you're very against the switch() magic, then I'll switch over to just using a shared memory area.
Regards,
Anthony Liguori
Quoted 3 lines
Rusty.
Quoted 32 lines · Rusty Russell
Quoted 17 lines · On Friday 09 November 2007 09:33:04 Anthony Liguori wrote:
switch (addr) {
caseVIRTIO_BLK_CONFIG_MAX_SEG:return vdev->max_seg & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SEG + 1: return (vdev->max_seg >> 8) & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SEG + 2: return (vdev->max_seg >> 16) & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SEG + 3: return (vdev->max_seg >> 24) & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SIZE: return vdev->max_size & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SIZE + 1: return (vdev->max_size >> 8) & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SIZE + 2: return (vdev->max_size >> 16) & 0xFF; case VIRTIO_BLK_CONFIG_MAX_SIZE + 3: return (vdev->max_size >> 24) & 0xFF;Quoted 2 lines
...
struct virtio_blk_config { uint32_t max_seg, max_size; }; ... struct virtio_blk_config conf = { vdev->max_seg, vdev->max_size }; return ((unsigned char *)&conf)[addr];
Quoted 2 lines
(Which strongly implies our headers should expose that nominal struct, rather than numerical constants).
The problem is the ABI. We can either require that PCI configuration values are accessed with natural instructions, or it makes very little sense to use the PCI configuration space for virtio configuration information. If we really can't find a way to do this (and I think my current implementation is the best compromise since it hides this from everything else), then I think I'll switch over to just writing a PFN into a PCI configuration slot and then have that page store the virtio configuration information (much like is done with lguest).
Either virtio config looks like a shared memory area (as lguest currently implements it), or it looks like hardware registers (like virtio-pci implements it). After thinking about it for a while, I don't think the two can be reconciled. There are subtle differences between the two that can't be hidden in the virtio interface. For instance, in the PCI model, you get notified when values are read/written whereas in the lguest model, you don't and need explicit status bits.
If you're very against the switch() magic, then I'll switch over to just using a shared memory area.
Regards,
Anthony Liguori
Quoted 3 lines
Rusty.
Quoted 4 lines · On Saturday 10 November 2007 10:45:38 Anthony Liguori wrote:
The problem is the ABI. We can either require that PCI configuration values are accessed with natural instructions, or it makes very little sense to use the PCI configuration space for virtio configuration information.
To me it seems logical and simplest to allow any accesses, lay out your structure and emulate it in the obvious way.
You can put the configuration information somewhere else, but the PCI config space seems the logical place.
Quoted 7 lines
Either virtio config looks like a shared memory area (as lguest currently implements it), or it looks like hardware registers (like virtio-pci implements it). After thinking about it for a while, I don't think the two can be reconciled. There are subtle differences between the two that can't be hidden in the virtio interface. For instance, in the PCI model, you get notified when values are read/written whereas in the lguest model, you don't and need explicit status bits.
No. You need those status bits even if you have your register model, otherwise you can't tell when the configuration as a whole is stable. Consider the feature bits. Worse, consider extending the feature bits beyond 32 bits.
(We will have to deal with dynamic configuration changes in future; I was planning on using some status bits. But it's pretty clear that it's going to require an explicit ack of some form.)
Hope that clarifies,
Rusty.
Quoted 4 lines · On Saturday 10 November 2007 10:45:38 Anthony Liguori wrote:
The problem is the ABI. We can either require that PCI configuration values are accessed with natural instructions, or it makes very little sense to use the PCI configuration space for virtio configuration information.
To me it seems logical and simplest to allow any accesses, lay out your structure and emulate it in the obvious way.
You can put the configuration information somewhere else, but the PCI config space seems the logical place.
Quoted 7 lines
Either virtio config looks like a shared memory area (as lguest currently implements it), or it looks like hardware registers (like virtio-pci implements it). After thinking about it for a while, I don't think the two can be reconciled. There are subtle differences between the two that can't be hidden in the virtio interface. For instance, in the PCI model, you get notified when values are read/written whereas in the lguest model, you don't and need explicit status bits.
No. You need those status bits even if you have your register model, otherwise you can't tell when the configuration as a whole is stable. Consider the feature bits. Worse, consider extending the feature bits beyond 32 bits.
(We will have to deal with dynamic configuration changes in future; I was planning on using some status bits. But it's pretty clear that it's going to require an explicit ack of some form.)
Hope that clarifies,
Rusty.
Quoted 10 lines · Rusty Russell
Quoted 5 lines · On Saturday 10 November 2007 10:45:38 Anthony Liguori wrote:
The problem is the ABI. We can either require that PCI configuration values are accessed with natural instructions, or it makes very little sense to use the PCI configuration space for virtio configuration information.
To me it seems logical and simplest to allow any accesses, lay out your structure and emulate it in the obvious way.
Okay, I've got some updates that I'm going to send out now to the PCI virtio driver but I'll also change it to switch over to a memory layout. It's not the best PCI ABI but I can certainly live with it.
Quoted 2 lines
You can put the configuration information somewhere else, but the PCI config space seems the logical place.
If we're treating the PCI config space as an opaque memory blob, instead of as distinct registers, I think it makes more sense to just put it in memory. In the backend, I have to treat it as a memory blob anyway and using the PCI config space just means that I have to write all the various PIO handlers for the different access sizes. It's much more elegant in my mind just to have the driver provide some memory that the host fills out.
Thanks for all the review,
Anthony Liguori
Quoted 17 lines
Quoted 8 lines
Either virtio config looks like a shared memory area (as lguest currently implements it), or it looks like hardware registers (like virtio-pci implements it). After thinking about it for a while, I don't think the two can be reconciled. There are subtle differences between the two that can't be hidden in the virtio interface. For instance, in the PCI model, you get notified when values are read/written whereas in the lguest model, you don't and need explicit status bits.
No. You need those status bits even if you have your register model, otherwise you can't tell when the configuration as a whole is stable. Consider the feature bits. Worse, consider extending the feature bits beyond 32 bits.
(We will have to deal with dynamic configuration changes in future; I was planning on using some status bits. But it's pretty clear that it's going to require an explicit ack of some form.)
Hope that clarifies,
Rusty.
Quoted 10 lines · Rusty Russell
Quoted 5 lines · On Saturday 10 November 2007 10:45:38 Anthony Liguori wrote:
The problem is the ABI. We can either require that PCI configuration values are accessed with natural instructions, or it makes very little sense to use the PCI configuration space for virtio configuration information.
To me it seems logical and simplest to allow any accesses, lay out your structure and emulate it in the obvious way.
Okay, I've got some updates that I'm going to send out now to the PCI virtio driver but I'll also change it to switch over to a memory layout. It's not the best PCI ABI but I can certainly live with it.
Quoted 2 lines
You can put the configuration information somewhere else, but the PCI config space seems the logical place.
If we're treating the PCI config space as an opaque memory blob, instead of as distinct registers, I think it makes more sense to just put it in memory. In the backend, I have to treat it as a memory blob anyway and using the PCI config space just means that I have to write all the various PIO handlers for the different access sizes. It's much more elegant in my mind just to have the driver provide some memory that the host fills out.
Thanks for all the review,
Anthony Liguori
Quoted 17 lines
Quoted 8 lines
Either virtio config looks like a shared memory area (as lguest currently implements it), or it looks like hardware registers (like virtio-pci implements it). After thinking about it for a while, I don't think the two can be reconciled. There are subtle differences between the two that can't be hidden in the virtio interface. For instance, in the PCI model, you get notified when values are read/written whereas in the lguest model, you don't and need explicit status bits.
No. You need those status bits even if you have your register model, otherwise you can't tell when the configuration as a whole is stable. Consider the feature bits. Worse, consider extending the feature bits beyond 32 bits.
(We will have to deal with dynamic configuration changes in future; I was planning on using some status bits. But it's pretty clear that it's going to require an explicit ack of some form.)
Hope that clarifies,
Rusty.