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

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

[~axelheider]

This is a follow-up on older attempts to make a second UART
available for the arm-virt machine in normal world. The use case
is, that this give a simple I/O channel in addition to stdout, as this
simplifies various test scenarios. Especially for non-Linux operating
systems (e.g. seL4) where arm-virt is handy as a generic machine
for testing purposes.

There are existing discussions about this topic at:
- https://lists.gnu.org/archive/html/qemu-arm/2017-12/msg00063.html
- https://lists.gnu.org/archive/html/qemu-discuss/2018-11/msg00001.html
- https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg01613.html

This patch tries to address the concerns raised there and also
avoid breaking compatibility with existing setups.

v2 also updates the documentation in docs/system/arm/virt.rst

Axel Heider (1):
  hw/arm/virt: make second UART available

 docs/system/arm/virt.rst |  5 ++--
 hw/arm/virt-acpi-build.c | 12 ++++-----
 hw/arm/virt.c            | 57 ++++++++++++++++++++++++++++++----------
 include/hw/arm/virt.h    |  4 +--
 4 files changed, 54 insertions(+), 24 deletions(-)

-- 
2.34.5

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

[~axelheider]

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>
---
 docs/system/arm/virt.rst |  5 ++--
 hw/arm/virt-acpi-build.c | 12 ++++-----
 hw/arm/virt.c            | 57 ++++++++++++++++++++++++++++++----------
 include/hw/arm/virt.h    |  4 +--
 4 files changed, 54 insertions(+), 24 deletions(-)

diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst
index 20442ea2c1..0d9de6bb2e 100644
--- a/docs/system/arm/virt.rst
+++ b/docs/system/arm/virt.rst
@@ -26,7 +26,8 @@ The virt board supports:
 
 - PCI/PCIe devices
 - Flash memory
-- One PL011 UART
+- Two PL011 UARTs. The second UART only exists if a backend is configured
+  explicitly or TrustZone is enabled.
 - An RTC
 - The fw_cfg device that allows a guest to obtain data from QEMU
 - A PL061 GPIO controller
@@ -42,7 +43,7 @@ The virt board supports:
 - many CPUs (up to 512 if using a GICv3 and highmem)
 - Secure-World-only devices if the CPU has TrustZone:
 
-  - A second PL011 UART
+  - The second PL011 UART
   - A second PL061 GPIO controller, with GPIO lines for triggering
     a system reset or system poweroff
   - A secure flash memory
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 4156111d49..3e1852a1b9 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -482,14 +482,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 */
@@ -673,11 +673,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);
@@ -858,8 +858,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 b871350856..92f9401b23 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -143,11 +143,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 }, /* secure UART */
     [VIRT_SMMU] =               { 0x09050000, 0x00020000 },
     [VIRT_PCDIMM_ACPI] =        { 0x09070000, MEMORY_HOTPLUG_IO_LEN },
     [VIRT_ACPI_GED] =           { 0x09080000, ACPI_GED_EVT_SEL_LEN },
@@ -184,11 +184,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 */
@@ -843,6 +843,27 @@ static void create_uart(const VirtMachineState *vms, int uart,
                         MemoryRegion *mem, Chardev *chr)
 {
     char *nodename;
+    /*
+     * 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.
+     */
+    switch(uart) {
+    case VIRT_UART0:
+        break;
+    case VIRT_UART1:
+        if (!vms->secure && !chr) {
+            return;
+        }
+        break;
+    default:
+        error_report("unsupported UART ID %d", uart);
+        exit(1);
+    }
+
     hwaddr base = vms->memmap[uart].base;
     hwaddr size = vms->memmap[uart].size;
     int irq = vms->irqmap[uart];
@@ -854,6 +875,7 @@ static void create_uart(const VirtMachineState *vms, int uart,
 
     qdev_prop_set_chr(dev, "chardev", chr);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    /* if security extensions are enabled, 'mem' is 'secure_sysmem' for UART1 */
     memory_region_add_subregion(mem, base,
                                 sysbus_mmio_get_region(s, 0));
     sysbus_connect_irq(s, 0, qdev_get_gpio_in(vms->gic, irq));
@@ -873,15 +895,21 @@ static void create_uart(const VirtMachineState *vms, int uart,
     qemu_fdt_setprop(ms->fdt, nodename, "clock-names",
                          clocknames, sizeof(clocknames));
 
-    if (uart == VIRT_UART) {
+    switch(uart) {
+    case VIRT_UART0:
         qemu_fdt_setprop_string(ms->fdt, "/chosen", "stdout-path", nodename);
-    } else {
-        /* 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");
-
-        qemu_fdt_setprop_string(ms->fdt, "/secure-chosen", "stdout-path",
-                                nodename);
+        break;
+    case VIRT_UART1:
+        if (vms->secure) {
+            qemu_fdt_setprop_string(ms->fdt, nodename, "status", "disabled");
+            qemu_fdt_setprop_string(ms->fdt, nodename, "secure-status", "okay");
+            qemu_fdt_setprop_string(ms->fdt, "/secure-chosen", "stdout-path",
+                                    nodename);
+        }
+        break;
+    default:
+        /* noting special */
+        break;
     }
 
     g_free(nodename);
@@ -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));
 
     if (vms->secure) {
         create_secure_ram(vms, secure_sysmem, secure_tag_sysmem);
-        create_uart(vms, VIRT_SECURE_UART, secure_sysmem, serial_hd(1));
     }
 
     if (tag_sysmem) {
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 6ec479ca2b..90563c132b 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -69,7 +69,7 @@ enum {
     VIRT_GIC_ITS,
     VIRT_GIC_REDIST,
     VIRT_SMMU,
-    VIRT_UART,
+    VIRT_UART0,
     VIRT_MMIO,
     VIRT_RTC,
     VIRT_FW_CFG,
@@ -79,7 +79,7 @@ enum {
     VIRT_PCIE_ECAM,
     VIRT_PLATFORM_BUS,
     VIRT_GPIO,
-    VIRT_SECURE_UART,
+    VIRT_UART1, /* secure UART if vms->secure */
     VIRT_SECURE_MEM,
     VIRT_SECURE_GPIO,
     VIRT_PCDIMM_ACPI,
-- 
2.34.5

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