[Qemu-devel] [PATCH] hw/arm/virt: explain device-to-transport mapping in create_virtio_devices()

[Qemu-devel] [PATCH] hw/arm/virt: explain device-to-transport mapping in create_virtio_devices()

[Laszlo Ersek]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/arm/virt.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 2353440..091e5ee 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -441,10 +441,27 @@ 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
-     * 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.
+    /* We create the transports in forwards order. Since qbus_realize()
+     * prepends (not appends) new child buses, the incrementing loop below will
+     * create a list of virtio-mmio buses with decreasing base addresses.
+     *
+     * When a -device option is processed from the command line,
+     * qbus_find_recursive() picks the next free virtio-mmio bus in forwards
+     * order. The upshot is that -device options in increasing command line
+     * order are mapped to virtio-mmio buses with decreasing base addresses.
+     *
+     * When this code was originally written, that arrangement ensured that the
+     * guest Linux kernel would give the lowest "name" (/dev/vda, eth0, etc) to
+     * the first -device on the command line. (The end-to-end order is a
+     * function of this loop, qbus_realize(), qbus_find_recursive(), and the
+     * guest kernel's name-to-address assignment strategy.)
+     *
+     * Meanwhile, the kernel's traversal seems to have been reserved; see eg.
+     * the message, if not necessarily the code, of commit 70161ff336.
+     * Therefore the loop now establishes the inverse of the original intent.
+     *
+     * Unfortunately, we can't counteract the kernel change by reversing the
+     * loop; it would break existing command lines.
      */
     for (i = 0; i < NUM_VIRTIO_TRANSPORTS; i++) {
         int irq = vbi->irqmap[VIRT_MMIO] + i;
@@ -453,6 +470,13 @@ static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic)
         sysbus_create_simple("virtio-mmio", base, pic[irq]);
     }
 
+    /* We add dtb nodes in reverse order so that they appear in the finished
+     * device tree lowest address first.
+     *
+     * Note that this mapping is independent of the loop above. The previous
+     * loop influences virtio device to virtio transport assignment, whereas
+     * this loop controls how virtio transports are laid out in the dtb.
+     */
     for (i = NUM_VIRTIO_TRANSPORTS - 1; i >= 0; i--) {
         char *nodename;
         int irq = vbi->irqmap[VIRT_MMIO] + i;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] hw/arm/virt: explain device-to-transport mapping in create_virtio_devices()

[Laszlo Ersek]

Peter,

On 01/30/15 05:31, Laszlo Ersek wrote:
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  hw/arm/virt.c | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 2353440..091e5ee 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -441,10 +441,27 @@ 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
> -     * 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.
> +    /* We create the transports in forwards order. Since qbus_realize()
> +     * prepends (not appends) new child buses, the incrementing loop below will
> +     * create a list of virtio-mmio buses with decreasing base addresses.
> +     *
> +     * When a -device option is processed from the command line,
> +     * qbus_find_recursive() picks the next free virtio-mmio bus in forwards
> +     * order. The upshot is that -device options in increasing command line
> +     * order are mapped to virtio-mmio buses with decreasing base addresses.
> +     *
> +     * When this code was originally written, that arrangement ensured that the
> +     * guest Linux kernel would give the lowest "name" (/dev/vda, eth0, etc) to
> +     * the first -device on the command line. (The end-to-end order is a
> +     * function of this loop, qbus_realize(), qbus_find_recursive(), and the
> +     * guest kernel's name-to-address assignment strategy.)
> +     *
> +     * Meanwhile, the kernel's traversal seems to have been reserved; see eg.

can you please s/reserved/reversed/?

Result of over-editing, sorry.

Thanks

> +     * the message, if not necessarily the code, of commit 70161ff336.
> +     * Therefore the loop now establishes the inverse of the original intent.
> +     *
> +     * Unfortunately, we can't counteract the kernel change by reversing the
> +     * loop; it would break existing command lines.
>       */
>      for (i = 0; i < NUM_VIRTIO_TRANSPORTS; i++) {
>          int irq = vbi->irqmap[VIRT_MMIO] + i;
> @@ -453,6 +470,13 @@ static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic)
>          sysbus_create_simple("virtio-mmio", base, pic[irq]);
>      }
>  
> +    /* We add dtb nodes in reverse order so that they appear in the finished
> +     * device tree lowest address first.
> +     *
> +     * Note that this mapping is independent of the loop above. The previous
> +     * loop influences virtio device to virtio transport assignment, whereas
> +     * this loop controls how virtio transports are laid out in the dtb.
> +     */
>      for (i = NUM_VIRTIO_TRANSPORTS - 1; i >= 0; i--) {
>          char *nodename;
>          int irq = vbi->irqmap[VIRT_MMIO] + i;
> 

Re: [Qemu-devel] [PATCH] hw/arm/virt: explain device-to-transport mapping in create_virtio_devices()

[Peter Maydell]

On 30 January 2015 at 04:34, Laszlo Ersek <lersek@redhat.com> wrote:
> Peter,
>
> On 01/30/15 05:31, Laszlo Ersek wrote:
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  hw/arm/virt.c | 32 ++++++++++++++++++++++++++++----
>>  1 file changed, 28 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 2353440..091e5ee 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -441,10 +441,27 @@ 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
>> -     * 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.
>> +    /* We create the transports in forwards order. Since qbus_realize()
>> +     * prepends (not appends) new child buses, the incrementing loop below will
>> +     * create a list of virtio-mmio buses with decreasing base addresses.
>> +     *
>> +     * When a -device option is processed from the command line,
>> +     * qbus_find_recursive() picks the next free virtio-mmio bus in forwards
>> +     * order. The upshot is that -device options in increasing command line
>> +     * order are mapped to virtio-mmio buses with decreasing base addresses.
>> +     *
>> +     * When this code was originally written, that arrangement ensured that the
>> +     * guest Linux kernel would give the lowest "name" (/dev/vda, eth0, etc) to
>> +     * the first -device on the command line. (The end-to-end order is a
>> +     * function of this loop, qbus_realize(), qbus_find_recursive(), and the
>> +     * guest kernel's name-to-address assignment strategy.)
>> +     *
>> +     * Meanwhile, the kernel's traversal seems to have been reserved; see eg.
>
> can you please s/reserved/reversed/?
>
> Result of over-editing, sorry.

Sure, no problem. I also suggest I add this para:
 *
 * In any case, the kernel makes no guarantee about the stability of
 * enumeration order of virtio devices (as demonstrated by it changing
 * between kernel versions). For reliable and stable identification
 * of disks users must use UUIDs or similar mechanisms.

-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/virt: explain device-to-transport mapping in create_virtio_devices()

[Laszlo Ersek]

On 02/03/15 12:51, Peter Maydell wrote:
> On 30 January 2015 at 04:34, Laszlo Ersek <lersek@redhat.com> wrote:
>> Peter,
>>
>> On 01/30/15 05:31, Laszlo Ersek wrote:
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>  hw/arm/virt.c | 32 ++++++++++++++++++++++++++++----
>>>  1 file changed, 28 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index 2353440..091e5ee 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -441,10 +441,27 @@ 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
>>> -     * 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.
>>> +    /* We create the transports in forwards order. Since qbus_realize()
>>> +     * prepends (not appends) new child buses, the incrementing loop below will
>>> +     * create a list of virtio-mmio buses with decreasing base addresses.
>>> +     *
>>> +     * When a -device option is processed from the command line,
>>> +     * qbus_find_recursive() picks the next free virtio-mmio bus in forwards
>>> +     * order. The upshot is that -device options in increasing command line
>>> +     * order are mapped to virtio-mmio buses with decreasing base addresses.
>>> +     *
>>> +     * When this code was originally written, that arrangement ensured that the
>>> +     * guest Linux kernel would give the lowest "name" (/dev/vda, eth0, etc) to
>>> +     * the first -device on the command line. (The end-to-end order is a
>>> +     * function of this loop, qbus_realize(), qbus_find_recursive(), and the
>>> +     * guest kernel's name-to-address assignment strategy.)
>>> +     *
>>> +     * Meanwhile, the kernel's traversal seems to have been reserved; see eg.
>>
>> can you please s/reserved/reversed/?
>>
>> Result of over-editing, sorry.
> 
> Sure, no problem. I also suggest I add this para:
>  *
>  * In any case, the kernel makes no guarantee about the stability of
>  * enumeration order of virtio devices (as demonstrated by it changing
>  * between kernel versions). For reliable and stable identification
>  * of disks users must use UUIDs or similar mechanisms.

Thanks. Do you want me to resubmit, or can you just squash it and
observe that fact in a [peter.maydell@linaro.org: note about UUIDs]
betwixt two other tags?

Thanks
Laszlo
PS: yes, I love "betwixt". Whether I used it correctly above is
secondary. ;)

Re: [Qemu-devel] [PATCH] hw/arm/virt: explain device-to-transport mapping in create_virtio_devices()

[Peter Maydell]

On 3 February 2015 at 12:04, Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/03/15 12:51, Peter Maydell wrote:
>> Sure, no problem. I also suggest I add this para:
>>  *
>>  * In any case, the kernel makes no guarantee about the stability of
>>  * enumeration order of virtio devices (as demonstrated by it changing
>>  * between kernel versions). For reliable and stable identification
>>  * of disks users must use UUIDs or similar mechanisms.
>
> Thanks. Do you want me to resubmit, or can you just squash it and
> observe that fact in a [peter.maydell@linaro.org: note about UUIDs]
> betwixt two other tags?

No, I'll just fix it up as I queue the patch.

-- PMM