hw/arm/virt: Second uart for normal-world
diff mbox series

Message ID 20191209152456.977399-1-daniel.thompson@linaro.org
State New
Headers show
Series
  • hw/arm/virt: Second uart for normal-world
Related show

Commit Message

Daniel Thompson Dec. 9, 2019, 3:24 p.m. UTC
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

Comments

Peter Maydell Dec. 9, 2019, 3:36 p.m. UTC | #1
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
Daniel Thompson Dec. 9, 2019, 5:08 p.m. UTC | #2
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.
Peter Maydell Dec. 9, 2019, 5:10 p.m. UTC | #3
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
Daniel Thompson Dec. 10, 2019, 4:53 p.m. UTC | #4
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.

Patch
diff mbox series

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