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

[Laszlo Ersek]

On 01/29/15 19:15, Peter Maydell wrote:
> On 29 January 2015 at 17:25, Laszlo Ersek <address@hidden> wrote:
>> 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.
> 
> Note that you can't rely on device ordering anyway. Guests
> should be using UUIDs or similar to ensure they mount the
> right filesystems in the right places, because the kernel
> makes no guarantee that it will enumerate devices in the
> device tree in any particular order.

Understood. Then we'll probably have to address this in libguestfs.

> 
>> 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
> 
> What tree is this a dump of? It doesn't look like a
> device tree dump.

No, it is the output of "info qtree", as I said above.

> Dumping the dtb produces results in
> the expected order:
> 
> [run qemu with -machine dumpdtb=/tmp/dump.dtb]
> dtc -I dtb -O dts /tmp/dump.dtb |less
> 
> [...]
>         address@hidden {
>                 interrupts = <0x0 0x10 0x1>;
>                 reg = <0x0 0xa000000 0x0 0x200>;
>                 compatible = "virtio,mmio";
>         };
> 
>         address@hidden {
>                 interrupts = <0x0 0x11 0x1>;
>                 reg = <0x0 0xa000200 0x0 0x200>;
>                 compatible = "virtio,mmio";
>         };
> [etc]

Yes. The DTB is 100% fine. However, we're talking two independent
mappings here. The lower level mapping is the DTB, and that's completely
fine. It's handled by the second loop in create_virtio_devices(). But,
that mapping in the DTB doesn't know about actual virtio devices at all,
it only describes virtio-mmio transports.

The bug is in the other mapping, ie. how devices specified with the
-device options are mapped onto the (then already existing) virtio-mmio
transports. The processing of -device is fine, it goes forward. The
issue is that qbus_find_recursive(), in response to each -device option,
finds the next free transport in decreasing address order, *because*
qbus_realize() *prepended* each transport, not appended it, when
create_virtio_devices() called sysbus_create_simple() in increasing
address order at startup.

> Device tree nodes appear in the tree lowest address
> first, which is exactly what the code above is
> claiming to do.

The comment makes two statements, and two loops follow it.

The first statement in the comment is wrong:

     /* Note that we have to create the transports in forwards order
      * so that command line devices are inserted lowest address first,

The first loop matches the first statement in the comment, hence the
first loop is also wrong.

The second statement in the comment is correct:

      * and then add dtb nodes in reverse order so that they appear in
      * the finished device tree lowest address first.

hence the second loop (which matches the second statement) is also correct.

Thanks
Laszlo