| Message ID | 20231023161532.2729084-4-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/arm: Create second NonSecure UART for virt board | expand |
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) {
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) {
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(-)