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

[Laszlo Ersek]

On 01/29/15 20:01, Peter Maydell wrote:
> On 29 January 2015 at 18:29, Laszlo Ersek <address@hidden> wrote:
>> 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.
> 
> I see what you mean now.
> 
> Does the ordering we have result in the guest kernel (usually)
> allocating vda to the first command line device, vdb to the
> second, and so on? I'm pretty sure I recall testing at the
> time and that that's why we have the funny ordering.

That's exactly what we're seeing. The first virtio-blk-device on the
qemu command line ends up getting the highest /dev/vdX in the guest, and
the last one on the qemu cmdline becomes /dev/vda in the guest.
Libguestfs expects the last cmdline option to become the highest /dev/vdX.

> (Looking back through the archives, when we last looked at
> this I think it turned out that the kernel probes the mmio
> devices in one order and then assigns them vda/vdb/etc
> in the opposite order.)

Ugh! That would be the perfect explanation for the current qemu code.

> 
>> 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.
> 
> Regardless, we can't change the ordering now without breaking
> existing command lines (I'm sure there are people out there
> who aren't using UUIDs).
> 
> We could fix the comment, though.

If the guest kernel changed its "assignment strategy" at some point, but
earlier it used to match the comment (and the code), then whichever way
we shape the comment will be wrong for the other kernel strategy. :) So,
in that case this code is probably best left undisturbed.

I'll try to dig out some kernel commit for more evidence.

Thanks!
Laszlo