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 <ler...@redhat.com> Thanks Laszlo > > Inspired-by: Axel Heider <axel.hei...@hensoldt.net> > Signed-off-by: Peter Maydell <peter.mayd...@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.1658256124453401108...@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) {