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) {