[Qemu-devel] address order of virtio-mmio devices

[Laszlo Ersek <lersek@redhat.com>]

Hi,

I find that virtio-mmio devices are assigned highest-to-lowest addresses
as they are found on the command line. IOW, this code (from commit
f5fdcd6e):

    /* Note that we have to create the transports in forwards order
     * so that command line devices are inserted lowest address first,
     * and then add dtb nodes in reverse order so that they appear in
     * the finished device tree lowest address first.
     */
    for (i = 0; i < NUM_VIRTIO_TRANSPORTS; i++) {
        int irq = vbi->irqmap[VIRT_MMIO] + i;
        hwaddr base = vbi->memmap[VIRT_MMIO].base + i * size;

        sysbus_create_simple("virtio-mmio", base, pic[irq]);
    }

works exactly in reverse.

The reason is that qbus_realize() reverses the order of child buses:

    if (bus->parent) {
        QLIST_INSERT_HEAD(&bus->parent->child_bus, bus, sibling);

ie. it inserts at the head, not at the tail.

Then, when the -device options are processed, qbus_find_recursive()
locates free buses "forwards".

According to info qtree, for the command line options

  -device virtio-net-device,netdev=netdev0,bootindex=2
  -device virtio-blk-device,drive=hd0,bootindex=0
  -device virtio-scsi-device,id=scsi0

(in this order), we end up with the following (partial) tree:

  dev: virtio-mmio, id ""
    gpio-out "sysbus-irq" 1
    indirect_desc = true
    event_idx = true
    mmio 000000000a003e00/0000000000000200
    bus: virtio-mmio-bus.31
      type virtio-mmio-bus
      dev: virtio-net-device, id ""
        mac = "52:54:00:12:34:56"
        vlan = <null>
        netdev = "netdev0"
        x-txtimer = 150000 (0x249f0)
        x-txburst = 256 (0x100)
        tx = ""
  dev: virtio-mmio, id ""
    gpio-out "sysbus-irq" 1
    indirect_desc = true
    event_idx = true
    mmio 000000000a003c00/0000000000000200
    bus: virtio-mmio-bus.30
      type virtio-mmio-bus
      dev: virtio-blk-device, id ""
        drive = "hd0"
        logical_block_size = 512 (0x200)
        physical_block_size = 512 (0x200)
        min_io_size = 0 (0x0)
        opt_io_size = 0 (0x0)
        discard_granularity = 4294967295 (0xffffffff)
        cyls = 16383 (0x3fff)
        heads = 16 (0x10)
        secs = 63 (0x3f)
        serial = ""
        config-wce = true
        scsi = true
        x-data-plane = false
  dev: virtio-mmio, id ""
    gpio-out "sysbus-irq" 1
    indirect_desc = true
    event_idx = true
    mmio 000000000a003a00/0000000000000200
    bus: virtio-mmio-bus.29
      type virtio-mmio-bus
      dev: virtio-scsi-device, id "scsi0"
        num_queues = 1 (0x1)
        max_sectors = 65535 (0xffff)
        cmd_per_lun = 128 (0x80)
        bus: scsi0.0
          type SCSI
          [snip]

Notice that the first device listed ends at 0xa000000 + 0x20 * 0x200 =
0xa004000, and further devices are located at decreasing addresses.

So, my questions are:
- has anyone actually tested the placement?
- would you take a patch that reverses the loop?

----------------
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 2353440..705e623 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -441,23 +441,17 @@ static void create_virtio_devices(const
VirtBoardInfo *vbi, qemu_irq *pic)
     int i;
     hwaddr size = vbi->memmap[VIRT_MMIO].size;

-    /* Note that we have to create the transports in forwards order
+    /* Note that we have to create the transports in backwards order
      * so that command line devices are inserted lowest address first,
-     * and then add dtb nodes in reverse order so that they appear in
+     * and then add dtb nodes in reverse order too so that they appear in
      * the finished device tree lowest address first.
      */
-    for (i = 0; i < NUM_VIRTIO_TRANSPORTS; i++) {
-        int irq = vbi->irqmap[VIRT_MMIO] + i;
-        hwaddr base = vbi->memmap[VIRT_MMIO].base + i * size;
-
-        sysbus_create_simple("virtio-mmio", base, pic[irq]);
-    }
-
     for (i = NUM_VIRTIO_TRANSPORTS - 1; i >= 0; i--) {
         char *nodename;
         int irq = vbi->irqmap[VIRT_MMIO] + i;
         hwaddr base = vbi->memmap[VIRT_MMIO].base + i * size;

+        sysbus_create_simple("virtio-mmio", base, pic[irq]);
         nodename = g_strdup_printf("/virtio_mmio@%" PRIx64, base);
         qemu_fdt_add_subnode(vbi->fdt, nodename);
         qemu_fdt_setprop_string(vbi->fdt, nodename,
----------------

With the patch, the info qtree output is as follows:

  dev: virtio-mmio, id ""
    gpio-out "sysbus-irq" 1
    indirect_desc = true
    event_idx = true
    mmio 000000000a000000/0000000000000200
    bus: virtio-mmio-bus.31
      type virtio-mmio-bus
      dev: virtio-net-device, id ""
        mac = "52:54:00:12:34:56"
        vlan = <null>
        netdev = "netdev0"
        x-txtimer = 150000 (0x249f0)
        x-txburst = 256 (0x100)
        tx = ""
  dev: virtio-mmio, id ""
    gpio-out "sysbus-irq" 1
    indirect_desc = true
    event_idx = true
    mmio 000000000a000200/0000000000000200
    bus: virtio-mmio-bus.30
      type virtio-mmio-bus
      dev: virtio-blk-device, id ""
        drive = "hd0"
        logical_block_size = 512 (0x200)
        physical_block_size = 512 (0x200)
        min_io_size = 0 (0x0)
        opt_io_size = 0 (0x0)
        discard_granularity = 4294967295 (0xffffffff)
        cyls = 16383 (0x3fff)
        heads = 16 (0x10)
        secs = 63 (0x3f)
        serial = ""
        config-wce = true
        scsi = true
        x-data-plane = false
  dev: virtio-mmio, id ""
    gpio-out "sysbus-irq" 1
    indirect_desc = true
    event_idx = true
    mmio 000000000a000400/0000000000000200
    bus: virtio-mmio-bus.29
      type virtio-mmio-bus
      dev: virtio-scsi-device, id "scsi0"
        num_queues = 1 (0x1)
        max_sectors = 65535 (0xffff)
        cmd_per_lun = 128 (0x80)
        bus: scsi0.0
          type SCSI
          [snip]

There's nothing to do about the decreasing "virtio-mmio-bus.N" naming --
see the "automatic_ids++" in qbus_realize(), which, paired with
QLIST_INSERT_HEAD() just below it, leads to the decrasing IDs. However,
the addresses do reflect the command line order.

Another possibility would be to replace QLIST_INSERT_HEAD() with
QTAILQ_INSERT_TAIL() in qbus_realize(), but compared to the virt board
code, that could very easily regress stuff.

Thanks
Laszlo