From d39a6785f40af658224bc3ff3d4c4a5a2f7c9eda Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 13 Feb 2015 17:13:43 +1030 Subject: tools/lguest: more documentation and checking of virtio 1.0 compliance. This is from all the non-PCI parts of the spec. Signed-off-by: Rusty Russell --- tools/lguest/lguest.c | 307 +++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 293 insertions(+), 14 deletions(-) (limited to 'tools') diff --git a/tools/lguest/lguest.c b/tools/lguest/lguest.c index 4c7c2aa66c89..bc444aff2333 100644 --- a/tools/lguest/lguest.c +++ b/tools/lguest/lguest.c @@ -170,6 +170,9 @@ struct device { /* Is it operational */ bool running; + /* Has it written FEATURES_OK but not re-checked it? */ + bool wrote_features_ok; + /* PCI configuration */ union { struct pci_config config; @@ -668,7 +671,26 @@ static void trigger_irq(struct virtqueue *vq) return; vq->pending_used = 0; - /* If they don't want an interrupt, don't send one... */ + /* + * 2.4.7.1: + * + * If the VIRTIO_F_EVENT_IDX feature bit is not negotiated: + * The driver MUST set flags to 0 or 1. + */ + if (vq->vring.avail->flags > 1) + errx(1, "%s: avail->flags = %u\n", + vq->dev->name, vq->vring.avail->flags); + + /* + * 2.4.7.2: + * + * If the VIRTIO_F_EVENT_IDX feature bit is not negotiated: + * + * - The device MUST ignore the used_event value. + * - After the device writes a descriptor index into the used ring: + * - If flags is 1, the device SHOULD NOT send an interrupt. + * - If flags is 0, the device MUST send an interrupt. + */ if (vq->vring.avail->flags & VRING_AVAIL_F_NO_INTERRUPT) { return; } @@ -703,6 +725,14 @@ static unsigned wait_for_vq_desc(struct virtqueue *vq, struct vring_desc *desc; u16 last_avail = lg_last_avail(vq); + /* + * 2.4.7.1: + * + * The driver MUST handle spurious interrupts from the device. + * + * That's why this is a while loop. + */ + /* There's nothing available? */ while (last_avail == vq->vring.avail->idx) { u64 event; @@ -776,12 +806,62 @@ static unsigned wait_for_vq_desc(struct virtqueue *vq, * descriptor chain. */ if (desc[i].flags & VRING_DESC_F_INDIRECT) { + /* 2.4.5.3.1: + * + * The driver MUST NOT set the VIRTQ_DESC_F_INDIRECT + * flag unless the VIRTIO_F_INDIRECT_DESC feature was + * negotiated. + */ + if (!(vq->dev->features_accepted & + (1<dev->name); + + /* + * 2.4.5.3.1: + * + * The driver MUST NOT set the VIRTQ_DESC_F_INDIRECT + * flag within an indirect descriptor (ie. only one + * table per descriptor). + */ + if (desc != vq->vring.desc) + errx(1, "%s: Indirect within indirect", + vq->dev->name); + + /* + * Proposed update VIRTIO-134 spells this out: + * + * A driver MUST NOT set both VIRTQ_DESC_F_INDIRECT + * and VIRTQ_DESC_F_NEXT in flags. + */ + if (desc[i].flags & VRING_DESC_F_NEXT) + errx(1, "%s: indirect and next together", + vq->dev->name); + if (desc[i].len % sizeof(struct vring_desc)) errx(1, "Invalid size for indirect buffer table"); + /* + * 2.4.5.3.2: + * + * The device MUST ignore the write-only flag + * (flags&VIRTQ_DESC_F_WRITE) in the descriptor that + * refers to an indirect table. + * + * We ignore it here: :) + */ max = desc[i].len / sizeof(struct vring_desc); desc = check_pointer(desc[i].addr, desc[i].len); i = 0; + + /* 2.4.5.3.1: + * + * A driver MUST NOT create a descriptor chain longer + * than the Queue Size of the device. + */ + if (max > vq->pci_config.queue_size) + errx(1, "%s: indirect has too many entries", + vq->dev->name); } /* Grab the first descriptor, and check it's OK. */ @@ -1082,6 +1162,7 @@ static void reset_device(struct device *dev) } } dev->running = false; + dev->wrote_features_ok = false; /* Now we care if threads die. */ signal(SIGCHLD, (void *)kill_launcher); @@ -1703,6 +1784,18 @@ static void check_virtqueue(struct device *d, struct virtqueue *vq) || vq->pci_config.queue_used_hi) errx(1, "%s: invalid 64-bit queue address", d->name); + /* + * 2.4.1: + * + * The driver MUST ensure that the physical address of the first byte + * of each virtqueue part is a multiple of the specified alignment + * value in the above table. + */ + if (vq->pci_config.queue_desc_lo % 16 + || vq->pci_config.queue_avail_lo % 2 + || vq->pci_config.queue_used_lo % 4) + errx(1, "%s: invalid alignment in queue addresses", d->name); + /* Initialize the virtqueue and check they're all in range. */ vq->vring.num = vq->pci_config.queue_size; vq->vring.desc = check_pointer(vq->pci_config.queue_desc_lo, @@ -1715,6 +1808,16 @@ static void check_virtqueue(struct device *d, struct virtqueue *vq) sizeof(*vq->vring.used) + (sizeof(vq->vring.used->ring[0]) * vq->vring.num)); + + /* + * 2.4.9.1: + * + * The driver MUST initialize flags in the used ring to 0 + * when allocating the used ring. + */ + if (vq->vring.used->flags != 0) + errx(1, "%s: invalid initial used.flags %#x", + d->name, vq->vring.used->flags); } static void start_virtqueue(struct virtqueue *vq) @@ -1768,12 +1871,12 @@ static void emulate_mmio_write(struct device *d, u32 off, u32 val, u32 mask) d->mmio->cfg.device_feature = (d->features >> 32); else d->mmio->cfg.device_feature = 0; - goto write_through32; + goto feature_write_through32; case offsetof(struct virtio_pci_mmio, cfg.guest_feature_select): if (val > 1) errx(1, "%s: Unexpected driver select %u", d->name, val); - goto write_through32; + goto feature_write_through32; case offsetof(struct virtio_pci_mmio, cfg.guest_feature): if (d->mmio->cfg.guest_feature_select == 0) { d->features_accepted &= ~((u64)0xFFFFFFFF); @@ -1783,11 +1886,19 @@ static void emulate_mmio_write(struct device *d, u32 off, u32 val, u32 mask) d->features_accepted &= 0xFFFFFFFF; d->features_accepted |= ((u64)val) << 32; } + /* + * 2.2.1: + * + * The driver MUST NOT accept a feature which the device did + * not offer + */ if (d->features_accepted & ~d->features) errx(1, "%s: over-accepted features %#llx of %#llx", d->name, d->features_accepted, d->features); - goto write_through32; - case offsetof(struct virtio_pci_mmio, cfg.device_status): + goto feature_write_through32; + case offsetof(struct virtio_pci_mmio, cfg.device_status): { + u8 prev; + verbose("%s: device status -> %#x\n", d->name, val); /* * 4.1.4.3.1: @@ -1795,8 +1906,15 @@ static void emulate_mmio_write(struct device *d, u32 off, u32 val, u32 mask) * The device MUST reset when 0 is written to device_status, * and present a 0 in device_status once that is done. */ - if (val == 0) + if (val == 0) { reset_device(d); + goto write_through8; + } + + /* 2.1.1: The driver MUST NOT clear a device status bit. */ + if (d->mmio->cfg.device_status & ~val) + errx(1, "%s: unset of device status bit %#x -> %#x", + d->name, d->mmio->cfg.device_status, val); /* * 2.1.2: @@ -1808,7 +1926,67 @@ static void emulate_mmio_write(struct device *d, u32 off, u32 val, u32 mask) && !(d->mmio->cfg.device_status & VIRTIO_CONFIG_S_DRIVER_OK)) start_virtqueues(d); + /* + * 3.1.1: + * + * The driver MUST follow this sequence to initialize a device: + * - Reset the device. + * - Set the ACKNOWLEDGE status bit: the guest OS has + * notice the device. + * - Set the DRIVER status bit: the guest OS knows how + * to drive the device. + * - Read device feature bits, and write the subset + * of feature bits understood by the OS and driver + * to the device. During this step the driver MAY + * read (but MUST NOT write) the device-specific + * configuration fields to check that it can + * support the device before accepting it. + * - Set the FEATURES_OK status bit. The driver + * MUST not accept new feature bits after this + * step. + * - Re-read device status to ensure the FEATURES_OK + * bit is still set: otherwise, the device does + * not support our subset of features and the + * device is unusable. + * - Perform device-specific setup, including + * discovery of virtqueues for the device, + * optional per-bus setup, reading and possibly + * writing the device’s virtio configuration + * space, and population of virtqueues. + * - Set the DRIVER_OK status bit. At this point the + * device is “live”. + */ + prev = 0; + switch (val & ~d->mmio->cfg.device_status) { + case VIRTIO_CONFIG_S_DRIVER_OK: + prev |= VIRTIO_CONFIG_S_FEATURES_OK; /* fall thru */ + case VIRTIO_CONFIG_S_FEATURES_OK: + prev |= VIRTIO_CONFIG_S_DRIVER; /* fall thru */ + case VIRTIO_CONFIG_S_DRIVER: + prev |= VIRTIO_CONFIG_S_ACKNOWLEDGE; /* fall thru */ + case VIRTIO_CONFIG_S_ACKNOWLEDGE: + break; + default: + errx(1, "%s: unknown device status bit %#x -> %#x", + d->name, d->mmio->cfg.device_status, val); + } + if (d->mmio->cfg.device_status != prev) + errx(1, "%s: unexpected status transition %#x -> %#x", + d->name, d->mmio->cfg.device_status, val); + + /* If they just wrote FEATURES_OK, we make sure they read */ + switch (val & ~d->mmio->cfg.device_status) { + case VIRTIO_CONFIG_S_FEATURES_OK: + d->wrote_features_ok = true; + break; + case VIRTIO_CONFIG_S_DRIVER_OK: + if (d->wrote_features_ok) + errx(1, "%s: did not re-read FEATURES_OK", + d->name); + break; + } goto write_through8; + } case offsetof(struct virtio_pci_mmio, cfg.queue_select): vq = vq_by_num(d, val); /* @@ -1844,7 +2022,9 @@ static void emulate_mmio_write(struct device *d, u32 off, u32 val, u32 mask) case offsetof(struct virtio_pci_mmio, cfg.queue_msix_vector): errx(1, "%s: attempt to set MSIX vector to %u", d->name, val); - case offsetof(struct virtio_pci_mmio, cfg.queue_enable): + case offsetof(struct virtio_pci_mmio, cfg.queue_enable): { + struct virtqueue *vq = vq_by_num(d, d->mmio->cfg.queue_select); + /* * 4.1.4.3.2: * @@ -1852,17 +2032,27 @@ static void emulate_mmio_write(struct device *d, u32 off, u32 val, u32 mask) */ if (val != 1) errx(1, "%s: setting queue_enable to %u", d->name, val); - d->mmio->cfg.queue_enable = val; - save_vq_config(&d->mmio->cfg, - vq_by_num(d, d->mmio->cfg.queue_select)); + /* - * 4.1.4.3.2: + * 3.1.1: * - * The driver MUST configure the other virtqueue fields before - * enabling the virtqueue with queue_enable. + * 7. Perform device-specific setup, including discovery of + * virtqueues for the device, optional per-bus setup, + * reading and possibly writing the device’s virtio + * configuration space, and population of virtqueues. + * 8. Set the DRIVER_OK status bit. + * + * All our devices require all virtqueues to be enabled, so + * they should have done that before setting DRIVER_OK. */ - check_virtqueue(d, vq_by_num(d, d->mmio->cfg.queue_select)); + if (d->mmio->cfg.device_status & VIRTIO_CONFIG_S_DRIVER_OK) + errx(1, "%s: enabling vs after DRIVER_OK", d->name); + + d->mmio->cfg.queue_enable = val; + save_vq_config(&d->mmio->cfg, vq); + check_virtqueue(d, vq); goto write_through16; + } case offsetof(struct virtio_pci_mmio, cfg.queue_notify_off): errx(1, "%s: attempt to write to queue_notify_off", d->name); case offsetof(struct virtio_pci_mmio, cfg.queue_desc_lo): @@ -1880,6 +2070,26 @@ static void emulate_mmio_write(struct device *d, u32 off, u32 val, u32 mask) if (d->mmio->cfg.queue_enable) errx(1, "%s: changing queue on live device", d->name); + + /* + * 3.1.1: + * + * The driver MUST follow this sequence to initialize a device: + *... + * 5. Set the FEATURES_OK status bit. The driver MUST not + * accept new feature bits after this step. + */ + if (!(d->mmio->cfg.device_status & VIRTIO_CONFIG_S_FEATURES_OK)) + errx(1, "%s: enabling vs before FEATURES_OK", d->name); + + /* + * 6. Re-read device status to ensure the FEATURES_OK bit is + * still set... + */ + if (d->wrote_features_ok) + errx(1, "%s: didn't re-read FEATURES_OK before setup", + d->name); + goto write_through32; case offsetof(struct virtio_pci_mmio, notify): vq = vq_by_num(d, val); @@ -1909,6 +2119,27 @@ static void emulate_mmio_write(struct device *d, u32 off, u32 val, u32 mask) errx(1, "%s: Unexpected write to offset %u", d->name, off); } +feature_write_through32: + /* + * 3.1.1: + * + * The driver MUST follow this sequence to initialize a device: + *... + * - Set the DRIVER status bit: the guest OS knows how + * to drive the device. + * - Read device feature bits, and write the subset + * of feature bits understood by the OS and driver + * to the device. + *... + * - Set the FEATURES_OK status bit. The driver MUST not + * accept new feature bits after this step. + */ + if (!(d->mmio->cfg.device_status & VIRTIO_CONFIG_S_DRIVER)) + errx(1, "%s: feature write before VIRTIO_CONFIG_S_DRIVER", + d->name); + if (d->mmio->cfg.device_status & VIRTIO_CONFIG_S_FEATURES_OK) + errx(1, "%s: feature write after VIRTIO_CONFIG_S_FEATURES_OK", + d->name); /* * 4.1.3.1: @@ -1951,12 +2182,29 @@ static u32 emulate_mmio_read(struct device *d, u32 off, u32 mask) case offsetof(struct virtio_pci_mmio, cfg.device_feature): case offsetof(struct virtio_pci_mmio, cfg.guest_feature_select): case offsetof(struct virtio_pci_mmio, cfg.guest_feature): + /* + * 3.1.1: + * + * The driver MUST follow this sequence to initialize a device: + *... + * - Set the DRIVER status bit: the guest OS knows how + * to drive the device. + * - Read device feature bits, and write the subset + * of feature bits understood by the OS and driver + * to the device. + */ + if (!(d->mmio->cfg.device_status & VIRTIO_CONFIG_S_DRIVER)) + errx(1, "%s: feature read before VIRTIO_CONFIG_S_DRIVER", + d->name); goto read_through32; case offsetof(struct virtio_pci_mmio, cfg.msix_config): errx(1, "%s: read of msix_config", d->name); case offsetof(struct virtio_pci_mmio, cfg.num_queues): goto read_through16; case offsetof(struct virtio_pci_mmio, cfg.device_status): + /* As they did read, any write of FEATURES_OK is now fine. */ + d->wrote_features_ok = false; + goto read_through8; case offsetof(struct virtio_pci_mmio, cfg.config_generation): /* * 4.1.4.3.1: @@ -1971,6 +2219,15 @@ static u32 emulate_mmio_read(struct device *d, u32 off, u32 mask) */ goto read_through8; case offsetof(struct virtio_pci_mmio, notify): + /* + * 3.1.1: + * + * The driver MUST NOT notify the device before setting + * DRIVER_OK. + */ + if (!(d->mmio->cfg.device_status & VIRTIO_CONFIG_S_DRIVER_OK)) + errx(1, "%s: notify before VIRTIO_CONFIG_S_DRIVER_OK", + d->name); goto read_through16; case offsetof(struct virtio_pci_mmio, isr): if (mask != 0xFF) @@ -1992,6 +2249,23 @@ static u32 emulate_mmio_read(struct device *d, u32 off, u32 mask) if (off > d->mmio_size - 4) errx(1, "%s: read past end (%#x)", d->name, getreg(eip)); + + /* + * 3.1.1: + * The driver MUST follow this sequence to initialize a device: + *... + * 3. Set the DRIVER status bit: the guest OS knows how to + * drive the device. + * 4. Read device feature bits, and write the subset of + * feature bits understood by the OS and driver to the + * device. During this step the driver MAY read (but MUST NOT + * write) the device-specific configuration fields to check + * that it can support the device before accepting it. + */ + if (!(d->mmio->cfg.device_status & VIRTIO_CONFIG_S_DRIVER)) + errx(1, "%s: config read before VIRTIO_CONFIG_S_DRIVER", + d->name); + if (mask == 0xFFFFFFFF) goto read_through32; else if (mask == 0xFFFF) @@ -2200,6 +2474,10 @@ static void init_pci_config(struct pci_config *pci, u16 type, * * The device MUST either present notify_off_multiplier as an even * power of 2, or present notify_off_multiplier as 0. + * + * 2.1.2: + * + * The device MUST initialize device status to 0 upon reset. */ memset(pci, 0, sizeof(*pci)); @@ -2340,6 +2618,7 @@ static struct device *new_pci_device(const char *name, u16 type, dev->name = name; dev->vq = NULL; dev->running = false; + dev->wrote_features_ok = false; dev->mmio_size = sizeof(struct virtio_pci_mmio); dev->mmio = calloc(1, dev->mmio_size); dev->features = (u64)1 << VIRTIO_F_VERSION_1; -- cgit v1.2.3-59-g8ed1b