| Message ID | 20191209152456.977399-1-daniel.thompson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm/virt: Second uart for normal-world | expand |
On Mon, 9 Dec 2019 at 15:25, Daniel Thompson <daniel.thompson@linaro.org> wrote: > > The virt machine can have two UARTs but the second UART is only > registered when secure-mode support is enabled. Change the machine so > this UART is always registered bringing the behaviour of the virt > machine closer to x86 land, where VMs can be expected to support two > UARTs. This approach is also similar to how a TZPC would typically > make a UART inaccessible to normal world on physical hardware. > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> > --- > > Notes: > It is difficult to add a UART without some kind of odd difference of > behaviour somewhere. As far as I could tell the choices are: > > 1. Move the secure UART from UART1 to UART2. This is a > not-backward-compatible difference of behaviour (will likely break > the command lines for existing users of the secure UART). > > 2. We tack the new UART on at the end, meaning UART1 will re-enumerates > as UART2 when secure mode is enabled/disabled. This is rather > surprising for users. > > 3. UART1 is registered and inaccessible when secure mode is not enabled > (e.g. user must provide a dummy -serial argument to skip the missing > UART) > > 4. Normal world can only use the second UART if there is no secure mode > support. > > 5. Don't support an extra UART ;-) > > Of these I concluded that #4 was least worst! Ultimately it is should be > unsurprising for users because it is how most physical hardware works > (e.g. a trustzone controller is used to make an existing UART > inaccessible to normal world). This change looks simple but it will break booting of UEFI in the guest. Unfortunately UEFI enumerates UARTs in the guest in the opposite order to the Linux kernel, so whichever way round you put the extra UART something will get it wrong and stop producing output where the user expects. I think the conclusion I came to was that the only way to avoid breaking existing command lines would be to only create the second UART if the user explicitly asked for it somehow. (Possibly just looking at "if there really is a 2nd serial on the command line" with "if (serial_hd(1)" would suffice, or perhaps not.) You also need to do something to add the 2nd UART to the ACPI tables. (Very out of date and broken patchset from last time I looked at this: https://lists.gnu.org/archive/html/qemu-arm/2017-12/msg00063.html ) thanks -- PMM
On Mon, Dec 09, 2019 at 03:36:17PM +0000, Peter Maydell wrote: > On Mon, 9 Dec 2019 at 15:25, Daniel Thompson <daniel.thompson@linaro.org> wrote: > > > > The virt machine can have two UARTs but the second UART is only > > registered when secure-mode support is enabled. Change the machine so > > this UART is always registered bringing the behaviour of the virt > > machine closer to x86 land, where VMs can be expected to support two > > UARTs. This approach is also similar to how a TZPC would typically > > make a UART inaccessible to normal world on physical hardware. > > > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> > > --- > > > > Notes: > > It is difficult to add a UART without some kind of odd difference of > > behaviour somewhere. As far as I could tell the choices are: > > > > 1. Move the secure UART from UART1 to UART2. This is a > > not-backward-compatible difference of behaviour (will likely break > > the command lines for existing users of the secure UART). > > > > 2. We tack the new UART on at the end, meaning UART1 will re-enumerates > > as UART2 when secure mode is enabled/disabled. This is rather > > surprising for users. > > > > 3. UART1 is registered and inaccessible when secure mode is not enabled > > (e.g. user must provide a dummy -serial argument to skip the missing > > UART) > > > > 4. Normal world can only use the second UART if there is no secure mode > > support. > > > > 5. Don't support an extra UART ;-) > > > > Of these I concluded that #4 was least worst! Ultimately it is should be > > unsurprising for users because it is how most physical hardware works > > (e.g. a trustzone controller is used to make an existing UART > > inaccessible to normal world). > > This change looks simple but it will break booting of UEFI > in the guest. Unfortunately UEFI enumerates UARTs in the guest > in the opposite order to the Linux kernel, so whichever way > round you put the extra UART something will get it wrong and > stop producing output where the user expects. > > I think the conclusion I came to was that the only way to > avoid breaking existing command lines would be to only > create the second UART if the user explicitly asked for > it somehow. (Possibly just looking at "if there really is > a 2nd serial on the command line" with "if (serial_hd(1)" > would suffice, or perhaps not.) I don't object to making it command line dependant (it is certainly lower risk) but, out of interest, has using /aliases to force the kernel to enumerate the serial nodes in the existing order been ruled out for any reason. With something like the patch below we can give /dev/ttyAMA0 with a stable device number regardless of the order of the UARTs within the DT: diff --git a/hw/arm/virt.c b/hw/arm/virt.c index a5cca04dba7f..7078cfc0c106 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -225,8 +225,12 @@ static void create_fdt(VirtMachineState *vms) qemu_fdt_setprop_cell(fdt, "/", "#address-cells", 0x2); qemu_fdt_setprop_cell(fdt, "/", "#size-cells", 0x2); - /* /chosen must exist for load_dtb to fill in necessary properties later */ + /* + * /chosen and /aliases must exist for load_dtb to fill in + * necessary properties later + */ qemu_fdt_add_subnode(fdt, "/chosen"); + 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 @@ -754,6 +758,9 @@ static void create_uart(const VirtMachineState *vms, qemu_irq *pic, int uart, if (uart == VIRT_UART) { qemu_fdt_setprop_string(vms->fdt, "/chosen", "stdout-path", nodename); + qemu_fdt_setprop_string(vms->fdt, "/aliases", "serial0", nodename); + } else if (!vms->secure) { + qemu_fdt_setprop_string(vms->fdt, "/aliases", "serial1", nodename); } else { /* Mark as not usable by the normal world */ qemu_fdt_setprop_string(vms->fdt, nodename, "status", "disabled"); > You also need to do something to add the 2nd UART to the ACPI tables. > > (Very out of date and broken patchset from last time I looked at this: > https://lists.gnu.org/archive/html/qemu-arm/2017-12/msg00063.html > ) Thanks for the heads up. I'll have to do a bit of reading/testing before I get back to you on that! Daniel.
On Mon, 9 Dec 2019 at 17:08, Daniel Thompson <daniel.thompson@linaro.org> wrote: > I don't object to making it command line dependant (it is certainly > lower risk) but, out of interest, has using /aliases to force the > kernel to enumerate the serial nodes in the existing order been ruled > out for any reason. No, I don't think anybody's investigated that (I wasn't aware that you could do something like that). Bear in mind that the kernel is not the only consumer of the DT, though -- you need to use a mechanism that all DT consumers will handle correctly. thanks -- PMM
On Mon, Dec 09, 2019 at 05:10:30PM +0000, Peter Maydell wrote: > On Mon, 9 Dec 2019 at 17:08, Daniel Thompson <daniel.thompson@linaro.org> wrote: > > I don't object to making it command line dependant (it is certainly > > lower risk) but, out of interest, has using /aliases to force the > > kernel to enumerate the serial nodes in the existing order been ruled > > out for any reason. > > No, I don't think anybody's investigated that (I wasn't aware > that you could do something like that). Bear in mind that the > kernel is not the only consumer of the DT, though -- you need > to use a mechanism that all DT consumers will handle correctly. The syntax for /aliases is standardized (in the DT documentation) but AFAIK the exact semantic meaning of an alias relies somewhat on idiom. It is true that the DT binding documentation for some serial drivers does include details of /aliases but sadly PL011 is not amoung them. I took a fairly detailed look at FreeBSD. I don't think /aliases is used to control enumeration order but that appears to be because aliases are handled in a different way to Linux. For example FreeBSD allows a custom console to be selected using FDT syntax (hw.fdt.console=serial0 or hw.fdt.console=/path/to/fdt-uart ) which means the Linux-like approach (such as console=ttyAMA0) need not be used. In summary I think that support for /aliases can and should be added since it the best way to help DT systems figure out how to match qemu uart numbering to its own naming. However I agree we still need a way to create systems with only a single UART even if I have not yet been able to come up with a test case that proves /aliases is insufficient ;-) Daniel.
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index d4bedc260712..a5cca04dba7f 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -1721,6 +1721,12 @@ static void machvirt_init(MachineState *machine) if (vms->secure) { create_secure_ram(vms, secure_sysmem); create_uart(vms, pic, VIRT_SECURE_UART, secure_sysmem, serial_hd(1)); + } else { + /* + * If secure mode is disabled then let's setup the "secure" + * UART so that normal world can use it. + */ + create_uart(vms, pic, VIRT_SECURE_UART, sysmem, serial_hd(1)); } vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64);
The virt machine can have two UARTs but the second UART is only registered when secure-mode support is enabled. Change the machine so this UART is always registered bringing the behaviour of the virt machine closer to x86 land, where VMs can be expected to support two UARTs. This approach is also similar to how a TZPC would typically make a UART inaccessible to normal world on physical hardware. Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> --- Notes: It is difficult to add a UART without some kind of odd difference of behaviour somewhere. As far as I could tell the choices are: 1. Move the secure UART from UART1 to UART2. This is a not-backward-compatible difference of behaviour (will likely break the command lines for existing users of the secure UART). 2. We tack the new UART on at the end, meaning UART1 will re-enumerates as UART2 when secure mode is enabled/disabled. This is rather surprising for users. 3. UART1 is registered and inaccessible when secure mode is not enabled (e.g. user must provide a dummy -serial argument to skip the missing UART) 4. Normal world can only use the second UART if there is no secure mode support. 5. Don't support an extra UART ;-) Of these I concluded that #4 was least worst! Ultimately it is should be unsurprising for users because it is how most physical hardware works (e.g. a trustzone controller is used to make an existing UART inaccessible to normal world). hw/arm/virt.c | 6 ++++++ 1 file changed, 6 insertions(+) base-commit: 8350b17be015bb872f28268bdeba1bac6c380efc -- 2.23.0