qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] hw/arm/virt: Second uart for normal-world


From: Daniel Thompson
Subject: Re: [PATCH] hw/arm/virt: Second uart for normal-world
Date: Mon, 9 Dec 2019 17:08:27 +0000

On Mon, Dec 09, 2019 at 03:36:17PM +0000, Peter Maydell wrote:
> On Mon, 9 Dec 2019 at 15:25, Daniel Thompson <address@hidden> 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 <address@hidden>
> > ---
> >
> > 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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]