Re: [PATCH qemu.git v2 1/1] hw/arm/virt: make second UART available

[Peter Maydell]

On Thu, 1 Dec 2022 at 14:30, ~axelheider <axelheider@git.sr.ht> wrote:
>
> From: Axel Heider <axel.heider@hensoldt.net>
>
> The first UART always always exists. If the security extensions are
> enabled, the second UART also always exists. Otherwise, it only exists
> if a backend is configured explicitly via '-serial <backend>', where
> 'null' creates a dummy backend. This allows enabling the second UART
> explicitly on demand and does not interfere with any existing setup
> that just expect one (or two if security extensions are enabled)
> UARTs.
>
> Signed-off-by: Axel Heider <axel.heider@hensoldt.net>

Hi; just wanted to say that this is still on my to-review list.
As a preliminary note:

> @@ -2222,11 +2250,12 @@ static void machvirt_init(MachineState *machine)
>
>      fdt_add_pmu_nodes(vms);
>
> -    create_uart(vms, VIRT_UART, sysmem, serial_hd(0));
> +    create_uart(vms, VIRT_UART0, sysmem, serial_hd(0));
> +    create_uart(vms, VIRT_UART1, vms->secure ? secure_sysmem : sysmem,
> +                serial_hd(1));

Creating the UARTs in this order in the code results in
them appearing in the DTB in reverse order. (I forget why;
I think dtb nodes put on a list in one order and then
put into the dtb proper in the other, or something.)
The result is that if you add an extra '-serial foo'
argument then Linux decides that UART0 is ttyAMA1 and
UART1 is ttyAMA0, which is a bit counter-intuitive.

This can be avoided by something like:

@@ -2289,9 +2289,20 @@ static void machvirt_init(MachineState *machine)

     fdt_add_pmu_nodes(vms);

+    /*
+     * These end up in the DTB in reverse order of creation, so
+     * we must create UART0 last to ensure it appears as the
+     * first UART, ttyAMA0, for Linux.
+     * For back-compatibility with older QEMU versions, if UART1 is
+     * the secure UART and thus always created, we create it second.
+     */
+    if (!vms->secure) {
+        create_uart(vms, VIRT_UART1, sysmem, serial_hd(1));
+    }
     create_uart(vms, VIRT_UART0, sysmem, serial_hd(0));
-    create_uart(vms, VIRT_UART1, vms->secure ? secure_sysmem : sysmem,
-                serial_hd(1));
+    if (vms->secure) {
+        create_uart(vms, VIRT_UART1, secure_sysmem, serial_hd(1));
+    }

     if (vms->secure) {
         create_secure_ram(vms, secure_sysmem, secure_tag_sysmem);

I still need to:
 * look at what UEFI does if presented with this DTB
   (may involve filing a bug report to see if they will
   change the enumeration order if it's still silly)
 * check what happens when we boot Linux passing it the
   h/w info via ACPI

thanks
-- PMM