[PATCH 0/3] hw/arm: Create second NonSecure UART for virt board

[PATCH 0/3] hw/arm: Create second NonSecure UART for virt board

[Peter Maydell]

For some use-cases, it is helpful to have more than one UART available
to the guest, but the Arm 'virt' board only creates one.  If the
second UART slot is not already used for a TrustZone Secure-World-only
UART, create it as a NonSecure UART if the user provides a serial
backend for it (e.g. via a second -serial command line option).

We've wanted this for literally years; my first attempt at it
was this series in 2017:
https://lore.kernel.org/all/1512745328-5109-1-git-send-email-peter.maydell@linaro.org/
More recently  Axel Heider revived the idea with a patchset last year:
https://lore.kernel.org/qemu-devel/166990501232.22022.16582561244534011083-0@git.sr.ht/

However it has previously foundered on the problem that EDK2 does
odd things when presented with multiple UARTs in the DTB. (Specifically,
it will send the guest GRUB bootloader output to UART1, debug output
to both UARTs 0 and 1 depending on how far through boot it is, and the
guest kernel will use UART0 since that's what the ACPI tables say.)

Several things here I think mean we can finally get over this issue:
 * I learnt about the device tree "aliases" node; this allows us to
   explicitly say "this node is the first UART and this node is the
   second UART". So guests like Linux that follow this part of the
   DTB spec will always get the UART order correct; and if there are
   obscure guests that turn out to misbehave, we can point at the
   spec and say "this is how you should fix this on your end"...
 * This patch, like Axel's patch, only creates the second UART if
   the user asks for it on the command line, so any pre-existing
   command lines will not change behaviour.
 * Laszlo Ersek has kindly written some EDK2 patches that rationalise
   what it does when it finds more than one UART. This means that
   we can tell any users who do want to use two UARTs with EDK2
   "you should upgrade your EDK2 blobs to version NNN if you want to
   do that".

I haven't tagged this patchset RFC because I think it is good for
review as it stands. However I think we should ideally wait for the
EDK2 changes to go upstream first, so that we can fill in the TODO
comment in patch 3 and mention in our release notes what the new
EDK2 that correctly handles multiple UARTs would be. So I'm not
targeting this at 8.2.

Review from ACPI experts to confirm that I got the ACPI table
changes right in patch 3 would be appreciated :-)

thanks
-- PMM

Peter Maydell (3):
  hw/arm/virt: Add serial aliases in DTB
  hw/arm/virt: Rename VIRT_UART and VIRT_SECURE_UART to VIRT_UART[01]
  hw/arm/virt: allow creation of a second NonSecure UART

 docs/system/arm/virt.rst |  6 ++++-
 include/hw/arm/virt.h    |  5 ++--
 hw/arm/virt-acpi-build.c | 22 ++++++++++-------
 hw/arm/virt.c            | 52 +++++++++++++++++++++++++++++++++-------
 4 files changed, 65 insertions(+), 20 deletions(-)

-- 
2.34.1

[PATCH 1/3] hw/arm/virt: Add serial aliases in DTB

[Peter Maydell]

If there is more than one UART in the DTB, then there is no guarantee
on which order a guest is supposed to initialise them.  The standard
solution to this is "serialN" entries in the "/aliases" node of the
dtb which give the nodename of the UARTs.

At the moment we only have two UARTs in the DTB when one is for
the Secure world and one for the Non-Secure world, so this isn't
really a problem. However if we want to add a second NS UART we'll
need the aliases to ensure guests pick the right one.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 529f1c089c0..b714ff1c03d 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -280,6 +280,8 @@ static void create_fdt(VirtMachineState *vms)
         }
     }
 
+    qemu_fdt_add_subnode(fdt, "/aliases");
+
     /* Clock node, for the benefit of the UART. The kernel device tree
      * binding documentation claims the PL011 node clock properties are
      * optional but in practice if you omit them the kernel refuses to
@@ -889,7 +891,9 @@ static void create_uart(const VirtMachineState *vms, int uart,
 
     if (uart == VIRT_UART) {
         qemu_fdt_setprop_string(ms->fdt, "/chosen", "stdout-path", nodename);
+        qemu_fdt_setprop_string(ms->fdt, "/aliases", "serial0", nodename);
     } else {
+        qemu_fdt_setprop_string(ms->fdt, "/aliases", "serial1", nodename);
         /* Mark as not usable by the normal world */
         qemu_fdt_setprop_string(ms->fdt, nodename, "status", "disabled");
         qemu_fdt_setprop_string(ms->fdt, nodename, "secure-status", "okay");
-- 
2.34.1

[PATCH 2/3] hw/arm/virt: Rename VIRT_UART and VIRT_SECURE_UART to VIRT_UART[01]

[Peter Maydell]

We're going to make the second UART not always a secure-only device.
Rename the constants VIRT_UART and VIRT_SECURE_UART to VIRT_UART0
and VIRT_UART1 accordingly.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/arm/virt.h    |  4 ++--
 hw/arm/virt-acpi-build.c | 12 ++++++------
 hw/arm/virt.c            | 14 +++++++-------
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index f69239850e6..0de58328b2f 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -59,7 +59,7 @@ enum {
     VIRT_GIC_ITS,
     VIRT_GIC_REDIST,
     VIRT_SMMU,
-    VIRT_UART,
+    VIRT_UART0,
     VIRT_MMIO,
     VIRT_RTC,
     VIRT_FW_CFG,
@@ -69,7 +69,7 @@ enum {
     VIRT_PCIE_ECAM,
     VIRT_PLATFORM_BUS,
     VIRT_GPIO,
-    VIRT_SECURE_UART,
+    VIRT_UART1,
     VIRT_SECURE_MEM,
     VIRT_SECURE_GPIO,
     VIRT_PCDIMM_ACPI,
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 9ce136cd88c..54f26640982 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -483,14 +483,14 @@ build_spcr(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     build_append_int_noprefix(table_data, 0, 3); /* Reserved */
     /* Base Address */
     build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 8, 0, 1,
-                     vms->memmap[VIRT_UART].base);
+                     vms->memmap[VIRT_UART0].base);
     /* Interrupt Type */
     build_append_int_noprefix(table_data,
         (1 << 3) /* Bit[3] ARMH GIC interrupt */, 1);
     build_append_int_noprefix(table_data, 0, 1); /* IRQ */
     /* Global System Interrupt */
     build_append_int_noprefix(table_data,
-                              vms->irqmap[VIRT_UART] + ARM_SPI_BASE, 4);
+                              vms->irqmap[VIRT_UART0] + ARM_SPI_BASE, 4);
     build_append_int_noprefix(table_data, 3 /* 9600 */, 1); /* Baud Rate */
     build_append_int_noprefix(table_data, 0 /* No Parity */, 1); /* Parity */
     /* Stop Bits */
@@ -674,11 +674,11 @@ build_dbg2(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 
     /* BaseAddressRegister[] */
     build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 8, 0, 1,
-                     vms->memmap[VIRT_UART].base);
+                     vms->memmap[VIRT_UART0].base);
 
     /* AddressSize[] */
     build_append_int_noprefix(table_data,
-                              vms->memmap[VIRT_UART].size, 4);
+                              vms->memmap[VIRT_UART0].size, 4);
 
     /* NamespaceString[] */
     g_array_append_vals(table_data, name, namespace_length);
@@ -859,8 +859,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
      */
     scope = aml_scope("\\_SB");
     acpi_dsdt_add_cpus(scope, vms);
-    acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
-                       (irqmap[VIRT_UART] + ARM_SPI_BASE));
+    acpi_dsdt_add_uart(scope, &memmap[VIRT_UART0],
+                       (irqmap[VIRT_UART0] + ARM_SPI_BASE));
     if (vmc->acpi_expose_flash) {
         acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
     }
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index b714ff1c03d..fd524aed6b6 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -144,11 +144,11 @@ static const MemMapEntry base_memmap[] = {
     [VIRT_GIC_ITS] =            { 0x08080000, 0x00020000 },
     /* This redistributor space allows up to 2*64kB*123 CPUs */
     [VIRT_GIC_REDIST] =         { 0x080A0000, 0x00F60000 },
-    [VIRT_UART] =               { 0x09000000, 0x00001000 },
+    [VIRT_UART0] =              { 0x09000000, 0x00001000 },
     [VIRT_RTC] =                { 0x09010000, 0x00001000 },
     [VIRT_FW_CFG] =             { 0x09020000, 0x00000018 },
     [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
-    [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
+    [VIRT_UART1] =              { 0x09040000, 0x00001000 },
     [VIRT_SMMU] =               { 0x09050000, 0x00020000 },
     [VIRT_PCDIMM_ACPI] =        { 0x09070000, MEMORY_HOTPLUG_IO_LEN },
     [VIRT_ACPI_GED] =           { 0x09080000, ACPI_GED_EVT_SEL_LEN },
@@ -191,11 +191,11 @@ static MemMapEntry extended_memmap[] = {
 };
 
 static const int a15irqmap[] = {
-    [VIRT_UART] = 1,
+    [VIRT_UART0] = 1,
     [VIRT_RTC] = 2,
     [VIRT_PCIE] = 3, /* ... to 6 */
     [VIRT_GPIO] = 7,
-    [VIRT_SECURE_UART] = 8,
+    [VIRT_UART1] = 8,
     [VIRT_ACPI_GED] = 9,
     [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
     [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */
@@ -889,7 +889,7 @@ static void create_uart(const VirtMachineState *vms, int uart,
     qemu_fdt_setprop(ms->fdt, nodename, "clock-names",
                          clocknames, sizeof(clocknames));
 
-    if (uart == VIRT_UART) {
+    if (uart == VIRT_UART0) {
         qemu_fdt_setprop_string(ms->fdt, "/chosen", "stdout-path", nodename);
         qemu_fdt_setprop_string(ms->fdt, "/aliases", "serial0", nodename);
     } else {
@@ -2269,11 +2269,11 @@ 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));
 
     if (vms->secure) {
         create_secure_ram(vms, secure_sysmem, secure_tag_sysmem);
-        create_uart(vms, VIRT_SECURE_UART, secure_sysmem, serial_hd(1));
+        create_uart(vms, VIRT_UART1, secure_sysmem, serial_hd(1));
     }
 
     if (tag_sysmem) {
-- 
2.34.1

[PATCH 3/3] hw/arm/virt: allow creation of a second NonSecure UART

[Peter Maydell]

For some use-cases, it is helpful to have more than one UART
available to the guest. If the second UART slot is not already
used for a TrustZone Secure-World-only UART, create it as a
NonSecure UART only when the user provides a serial backend
(e.g. via a second -serial command line option).

This avoids problems where existing guest software only expects
a single UART, and gets confused by the second UART in the DTB.
The major example of this is older EDK2 firmware, which will
send the GRUB bootloader output to UART1 and the guest
serial output to UART0. Users who want to use both UARTs
with a guest setup including EDK2 are advised to update
to a newer EDK2.

TODO: give specifics of which EDK2 version has this fix,
once the patches which fix EDK2 are upstream.

Inspired-by: Axel Heider <axel.heider@hensoldt.net>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This patch was originally based on the one from Axel Heider
that aimed to do the same thing:
https://lore.kernel.org/qemu-devel/166990501232.22022.16582561244534011083-1@git.sr.ht/
but by the time I had added the ACPI support and dealt with
the EDK2 compatibility awkwardness, I found I had pretty
much rewritten it. So this combination of author and tags
seemed to me the most appropriate, but I'm happy to adjust
if people (esp. Axel!) would prefer otherwise.

It is in theory possible to slightly work around the
incorrect behaviour of old EDK2 binaries by listing the
two UARTs in the opposite order in the DTB. However since
old EDK2 ends up using the two UARTs in different orders
depending on which phase of boot it is in (and in particular
with EDK2 debug builds debug messages go to a mix of both
UARTs) this doesn't seem worthwhile. I think most users
who are interested in the second UART are likely to be
using a bare-metal or direct Linux boot anyway.
---
 docs/system/arm/virt.rst |  6 +++++-
 include/hw/arm/virt.h    |  1 +
 hw/arm/virt-acpi-build.c | 12 ++++++++----
 hw/arm/virt.c            | 38 +++++++++++++++++++++++++++++++++++---
 4 files changed, 49 insertions(+), 8 deletions(-)

diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst
index e1697ac8f48..028d2416d5b 100644
--- a/docs/system/arm/virt.rst
+++ b/docs/system/arm/virt.rst
@@ -26,7 +26,7 @@ The virt board supports:
 
 - PCI/PCIe devices
 - Flash memory
-- One PL011 UART
+- Either one or two PL011 UARTs for the NonSecure World
 - An RTC
 - The fw_cfg device that allows a guest to obtain data from QEMU
 - A PL061 GPIO controller
@@ -48,6 +48,10 @@ The virt board supports:
   - A secure flash memory
   - 16MB of secure RAM
 
+The second NonSecure UART only exists if a backend is configured
+explicitly (e.g. with a second -serial command line option) and
+TrustZone emulation is not enabled.
+
 Supported guest CPU types:
 
 - ``cortex-a7`` (32-bit)
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 0de58328b2f..da15eb342bd 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -150,6 +150,7 @@ struct VirtMachineState {
     bool ras;
     bool mte;
     bool dtb_randomness;
+    bool second_ns_uart_present;
     OnOffAuto acpi;
     VirtGICType gic_version;
     VirtIOMMUType iommu;
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 54f26640982..b812f33c929 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -77,11 +77,11 @@ static void acpi_dsdt_add_cpus(Aml *scope, VirtMachineState *vms)
 }
 
 static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap,
-                                           uint32_t uart_irq)
+                               uint32_t uart_irq, int uartidx)
 {
-    Aml *dev = aml_device("COM0");
+    Aml *dev = aml_device("COM%d", uartidx);
     aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0011")));
-    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
+    aml_append(dev, aml_name_decl("_UID", aml_int(uartidx)));
 
     Aml *crs = aml_resource_template();
     aml_append(crs, aml_memory32_fixed(uart_memmap->base,
@@ -860,7 +860,11 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     scope = aml_scope("\\_SB");
     acpi_dsdt_add_cpus(scope, vms);
     acpi_dsdt_add_uart(scope, &memmap[VIRT_UART0],
-                       (irqmap[VIRT_UART0] + ARM_SPI_BASE));
+                       (irqmap[VIRT_UART0] + ARM_SPI_BASE), 0);
+    if (vms->second_ns_uart_present) {
+        acpi_dsdt_add_uart(scope, &memmap[VIRT_UART1],
+                           (irqmap[VIRT_UART1] + ARM_SPI_BASE), 1);
+    }
     if (vmc->acpi_expose_flash) {
         acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
     }
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index fd524aed6b6..7f60df7d7b2 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -856,7 +856,7 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem)
 }
 
 static void create_uart(const VirtMachineState *vms, int uart,
-                        MemoryRegion *mem, Chardev *chr)
+                        MemoryRegion *mem, Chardev *chr, bool secure)
 {
     char *nodename;
     hwaddr base = vms->memmap[uart].base;
@@ -894,6 +894,8 @@ static void create_uart(const VirtMachineState *vms, int uart,
         qemu_fdt_setprop_string(ms->fdt, "/aliases", "serial0", nodename);
     } else {
         qemu_fdt_setprop_string(ms->fdt, "/aliases", "serial1", nodename);
+    }
+    if (secure) {
         /* Mark as not usable by the normal world */
         qemu_fdt_setprop_string(ms->fdt, nodename, "status", "disabled");
         qemu_fdt_setprop_string(ms->fdt, nodename, "secure-status", "okay");
@@ -2269,11 +2271,41 @@ static void machvirt_init(MachineState *machine)
 
     fdt_add_pmu_nodes(vms);
 
-    create_uart(vms, VIRT_UART0, sysmem, serial_hd(0));
+    /*
+     * The first UART 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>'.
+     * This avoids potentially breaking existing user setups that expect
+     * only one NonSecure UART to be present (for instance, older EDK2
+     * binaries).
+     *
+     * The nodes end up in the DTB in reverse order of creation, so we must
+     * create UART0 last to ensure it appears as the first node in the DTB,
+     * for compatibility with guest software that just iterates through the
+     * DTB to find the first UART, as older versions of EDK2 do.
+     * DTB readers that follow the spec, as Linux does, should honour the
+     * aliases node information and /chosen/stdout-path regardless of
+     * the order that nodes appear in the DTB.
+     *
+     * For similar back-compatibility reasons, if UART1 is the secure UART
+     * we create it second (and so it appears first in the DTB), because
+     * that's what QEMU has always done.
+     */
+    if (!vms->secure) {
+        Chardev *serial1 = serial_hd(1);
+
+        if (serial1) {
+            vms->second_ns_uart_present = true;
+            create_uart(vms, VIRT_UART1, sysmem, serial1, false);
+        }
+    }
+    create_uart(vms, VIRT_UART0, sysmem, serial_hd(0), false);
+    if (vms->secure) {
+        create_uart(vms, VIRT_UART1, secure_sysmem, serial_hd(1), true);
+    }
 
     if (vms->secure) {
         create_secure_ram(vms, secure_sysmem, secure_tag_sysmem);
-        create_uart(vms, VIRT_UART1, secure_sysmem, serial_hd(1));
     }
 
     if (tag_sysmem) {
-- 
2.34.1

Re: [PATCH 2/3] hw/arm/virt: Rename VIRT_UART and VIRT_SECURE_UART to VIRT_UART[01]

[Philippe Mathieu-Daudé]

On 23/10/23 18:15, Peter Maydell wrote:
> We're going to make the second UART not always a secure-only device.
> Rename the constants VIRT_UART and VIRT_SECURE_UART to VIRT_UART0
> and VIRT_UART1 accordingly.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/hw/arm/virt.h    |  4 ++--
>   hw/arm/virt-acpi-build.c | 12 ++++++------
>   hw/arm/virt.c            | 14 +++++++-------
>   3 files changed, 15 insertions(+), 15 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 1/3] hw/arm/virt: Add serial aliases in DTB

[Philippe Mathieu-Daudé]

On 23/10/23 18:15, Peter Maydell wrote:
> If there is more than one UART in the DTB, then there is no guarantee
> on which order a guest is supposed to initialise them.  The standard
> solution to this is "serialN" entries in the "/aliases" node of the
> dtb which give the nodename of the UARTs.
> 
> At the moment we only have two UARTs in the DTB when one is for
> the Secure world and one for the Non-Secure world, so this isn't
> really a problem. However if we want to add a second NS UART we'll
> need the aliases to ensure guests pick the right one.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/arm/virt.c | 4 ++++
>   1 file changed, 4 insertions(+)


> @@ -889,7 +891,9 @@ static void create_uart(const VirtMachineState *vms, int uart,
>   
>       if (uart == VIRT_UART) {

I'd have put this patch after the rename (#2 <-> #1). Anyhow,

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>           qemu_fdt_setprop_string(ms->fdt, "/chosen", "stdout-path", nodename);
> +        qemu_fdt_setprop_string(ms->fdt, "/aliases", "serial0", nodename);
>       } else {
> +        qemu_fdt_setprop_string(ms->fdt, "/aliases", "serial1", nodename);
>           /* Mark as not usable by the normal world */
>           qemu_fdt_setprop_string(ms->fdt, nodename, "status", "disabled");
>           qemu_fdt_setprop_string(ms->fdt, nodename, "secure-status", "okay");

Re: [PATCH 3/3] hw/arm/virt: allow creation of a second NonSecure UART

[Laszlo Ersek]

On 10/23/23 18:15, Peter Maydell wrote:
> For some use-cases, it is helpful to have more than one UART
> available to the guest. If the second UART slot is not already
> used for a TrustZone Secure-World-only UART, create it as a
> NonSecure UART only when the user provides a serial backend
> (e.g. via a second -serial command line option).
> 
> This avoids problems where existing guest software only expects
> a single UART, and gets confused by the second UART in the DTB.
> The major example of this is older EDK2 firmware, which will
> send the GRUB bootloader output to UART1 and the guest
> serial output to UART0. Users who want to use both UARTs
> with a guest setup including EDK2 are advised to update
> to a newer EDK2.
> 
> TODO: give specifics of which EDK2 version has this fix,
> once the patches which fix EDK2 are upstream.

The patches should hopefully land in edk2-stable202311 (i.e., in the
November release).

The new ArmVirtQemu behavior is as follows:

- just one UART: same as before

- two UARTs: the UEFI console is on the "chosen" UART, and the edk2
DEBUG log is on the "first non-chosen" UART (i.e., on the "other" UART,
in practice).

series
Tested-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo


> 
> Inspired-by: Axel Heider <axel.heider@hensoldt.net>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This patch was originally based on the one from Axel Heider
> that aimed to do the same thing:
> https://lore.kernel.org/qemu-devel/166990501232.22022.16582561244534011083-1@git.sr.ht/
> but by the time I had added the ACPI support and dealt with
> the EDK2 compatibility awkwardness, I found I had pretty
> much rewritten it. So this combination of author and tags
> seemed to me the most appropriate, but I'm happy to adjust
> if people (esp. Axel!) would prefer otherwise.
> 
> It is in theory possible to slightly work around the
> incorrect behaviour of old EDK2 binaries by listing the
> two UARTs in the opposite order in the DTB. However since
> old EDK2 ends up using the two UARTs in different orders
> depending on which phase of boot it is in (and in particular
> with EDK2 debug builds debug messages go to a mix of both
> UARTs) this doesn't seem worthwhile. I think most users
> who are interested in the second UART are likely to be
> using a bare-metal or direct Linux boot anyway.
> ---
>  docs/system/arm/virt.rst |  6 +++++-
>  include/hw/arm/virt.h    |  1 +
>  hw/arm/virt-acpi-build.c | 12 ++++++++----
>  hw/arm/virt.c            | 38 +++++++++++++++++++++++++++++++++++---
>  4 files changed, 49 insertions(+), 8 deletions(-)
> 
> diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst
> index e1697ac8f48..028d2416d5b 100644
> --- a/docs/system/arm/virt.rst
> +++ b/docs/system/arm/virt.rst
> @@ -26,7 +26,7 @@ The virt board supports:
>  
>  - PCI/PCIe devices
>  - Flash memory
> -- One PL011 UART
> +- Either one or two PL011 UARTs for the NonSecure World
>  - An RTC
>  - The fw_cfg device that allows a guest to obtain data from QEMU
>  - A PL061 GPIO controller
> @@ -48,6 +48,10 @@ The virt board supports:
>    - A secure flash memory
>    - 16MB of secure RAM
>  
> +The second NonSecure UART only exists if a backend is configured
> +explicitly (e.g. with a second -serial command line option) and
> +TrustZone emulation is not enabled.
> +
>  Supported guest CPU types:
>  
>  - ``cortex-a7`` (32-bit)
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 0de58328b2f..da15eb342bd 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -150,6 +150,7 @@ struct VirtMachineState {
>      bool ras;
>      bool mte;
>      bool dtb_randomness;
> +    bool second_ns_uart_present;
>      OnOffAuto acpi;
>      VirtGICType gic_version;
>      VirtIOMMUType iommu;
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 54f26640982..b812f33c929 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -77,11 +77,11 @@ static void acpi_dsdt_add_cpus(Aml *scope, VirtMachineState *vms)
>  }
>  
>  static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap,
> -                                           uint32_t uart_irq)
> +                               uint32_t uart_irq, int uartidx)
>  {
> -    Aml *dev = aml_device("COM0");
> +    Aml *dev = aml_device("COM%d", uartidx);
>      aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0011")));
> -    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> +    aml_append(dev, aml_name_decl("_UID", aml_int(uartidx)));
>  
>      Aml *crs = aml_resource_template();
>      aml_append(crs, aml_memory32_fixed(uart_memmap->base,
> @@ -860,7 +860,11 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      scope = aml_scope("\\_SB");
>      acpi_dsdt_add_cpus(scope, vms);
>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART0],
> -                       (irqmap[VIRT_UART0] + ARM_SPI_BASE));
> +                       (irqmap[VIRT_UART0] + ARM_SPI_BASE), 0);
> +    if (vms->second_ns_uart_present) {
> +        acpi_dsdt_add_uart(scope, &memmap[VIRT_UART1],
> +                           (irqmap[VIRT_UART1] + ARM_SPI_BASE), 1);
> +    }
>      if (vmc->acpi_expose_flash) {
>          acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
>      }
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index fd524aed6b6..7f60df7d7b2 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -856,7 +856,7 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem)
>  }
>  
>  static void create_uart(const VirtMachineState *vms, int uart,
> -                        MemoryRegion *mem, Chardev *chr)
> +                        MemoryRegion *mem, Chardev *chr, bool secure)
>  {
>      char *nodename;
>      hwaddr base = vms->memmap[uart].base;
> @@ -894,6 +894,8 @@ static void create_uart(const VirtMachineState *vms, int uart,
>          qemu_fdt_setprop_string(ms->fdt, "/aliases", "serial0", nodename);
>      } else {
>          qemu_fdt_setprop_string(ms->fdt, "/aliases", "serial1", nodename);
> +    }
> +    if (secure) {
>          /* Mark as not usable by the normal world */
>          qemu_fdt_setprop_string(ms->fdt, nodename, "status", "disabled");
>          qemu_fdt_setprop_string(ms->fdt, nodename, "secure-status", "okay");
> @@ -2269,11 +2271,41 @@ static void machvirt_init(MachineState *machine)
>  
>      fdt_add_pmu_nodes(vms);
>  
> -    create_uart(vms, VIRT_UART0, sysmem, serial_hd(0));
> +    /*
> +     * The first UART 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>'.
> +     * This avoids potentially breaking existing user setups that expect
> +     * only one NonSecure UART to be present (for instance, older EDK2
> +     * binaries).
> +     *
> +     * The nodes end up in the DTB in reverse order of creation, so we must
> +     * create UART0 last to ensure it appears as the first node in the DTB,
> +     * for compatibility with guest software that just iterates through the
> +     * DTB to find the first UART, as older versions of EDK2 do.
> +     * DTB readers that follow the spec, as Linux does, should honour the
> +     * aliases node information and /chosen/stdout-path regardless of
> +     * the order that nodes appear in the DTB.
> +     *
> +     * For similar back-compatibility reasons, if UART1 is the secure UART
> +     * we create it second (and so it appears first in the DTB), because
> +     * that's what QEMU has always done.
> +     */
> +    if (!vms->secure) {
> +        Chardev *serial1 = serial_hd(1);
> +
> +        if (serial1) {
> +            vms->second_ns_uart_present = true;
> +            create_uart(vms, VIRT_UART1, sysmem, serial1, false);
> +        }
> +    }
> +    create_uart(vms, VIRT_UART0, sysmem, serial_hd(0), false);
> +    if (vms->secure) {
> +        create_uart(vms, VIRT_UART1, secure_sysmem, serial_hd(1), true);
> +    }
>  
>      if (vms->secure) {
>          create_secure_ram(vms, secure_sysmem, secure_tag_sysmem);
> -        create_uart(vms, VIRT_UART1, secure_sysmem, serial_hd(1));
>      }
>  
>      if (tag_sysmem) {