[Qemu-devel] [RFC] arm/virt: add one more uart for UEFI runtime debug

[Qemu-devel] [RFC] arm/virt: add one more uart for UEFI runtime debug

[Heyi Guo]

This patch is based on the discussion in TianoCore edk2-devel mailing
list:
https://lists.01.org/pipermail/edk2-devel/2019-March/037986.html

The conclusion is that we need an individual UART for UEFI runtime
services to print debug message at runtime, to avoid conflicting with
the system UART. We cannot use the secure UART either, for we may both
have Trusted Firmware and UEFI runtime services running on the VM, and
it is not easy to keep synchronization between the two components.

To keep backward compatibility, UEFI UART is put behind the secure
UART when secure world is enabled.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Heyi Guo <guoheyi@huawei.com>
---
 hw/arm/virt.c         | 29 +++++++++++++++++++++++------
 include/hw/arm/virt.h |  1 +
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index ce2664a..cbc5a66 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -131,6 +131,7 @@ static const MemMapEntry base_memmap[] = {
     [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
     [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
     [VIRT_SMMU] =               { 0x09050000, 0x00020000 },
+    [VIRT_UEFI_UART] =          { 0x09070000, 0x00001000 },
     [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
@@ -166,6 +167,7 @@ static const int a15irqmap[] = {
     [VIRT_PCIE] = 3, /* ... to 6 */
     [VIRT_GPIO] = 7,
     [VIRT_SECURE_UART] = 8,
+    [VIRT_UEFI_UART] = 9,
     [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
     [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */
     [VIRT_SMMU] = 74,    /* ...to 74 + NUM_SMMU_IRQS - 1 */
@@ -713,13 +715,23 @@ 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);
     } else {
+        const char *status_string;
+
+        if (uart == VIRT_SECURE_UART) {
+            status_string = "secure-status";
+        } else {
+            status_string = "uefi-status";
+        }
+
         /* Mark as not usable by the normal world */
         qemu_fdt_setprop_string(vms->fdt, nodename, "status", "disabled");
-        qemu_fdt_setprop_string(vms->fdt, nodename, "secure-status", "okay");
+        qemu_fdt_setprop_string(vms->fdt, nodename, status_string, "okay");
 
-        qemu_fdt_add_subnode(vms->fdt, "/secure-chosen");
-        qemu_fdt_setprop_string(vms->fdt, "/secure-chosen", "stdout-path",
-                                nodename);
+        if (uart == VIRT_SECURE_UART) {
+            qemu_fdt_add_subnode(vms->fdt, "/secure-chosen");
+            qemu_fdt_setprop_string(vms->fdt, "/secure-chosen", "stdout-path",
+                                    nodename);
+        }
     }
 
     g_free(nodename);
@@ -1423,6 +1435,7 @@ static void machvirt_init(MachineState *machine)
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
     bool aarch64 = true;
+    int uart_index = 0;
 
     /*
      * In accelerated mode, the memory map is computed earlier in kvm_type()
@@ -1616,13 +1629,17 @@ static void machvirt_init(MachineState *machine)
 
     fdt_add_pmu_nodes(vms);
 
-    create_uart(vms, pic, VIRT_UART, sysmem, serial_hd(0));
+    create_uart(vms, pic, VIRT_UART, sysmem, serial_hd(uart_index++));
 
     if (vms->secure) {
         create_secure_ram(vms, secure_sysmem);
-        create_uart(vms, pic, VIRT_SECURE_UART, secure_sysmem, serial_hd(1));
+        create_uart(vms, pic, VIRT_SECURE_UART, secure_sysmem,
+                    serial_hd(uart_index++));
     }
 
+    /* Create UART for UEFI runtime services debug */
+    create_uart(vms, pic, VIRT_UEFI_UART, sysmem, serial_hd(uart_index));
+
     vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64);
 
     create_rtc(vms, pic);
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 507517c..565769f 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -77,6 +77,7 @@ enum {
     VIRT_GPIO,
     VIRT_SECURE_UART,
     VIRT_SECURE_MEM,
+    VIRT_UEFI_UART,
     VIRT_LOWMEMMAP_LAST,
 };
 
-- 
1.8.3.1

[Qemu-devel] [RFC] arm/virt: add one more uart for UEFI runtime debug

[Heyi Guo]

This patch is based on the discussion in TianoCore edk2-devel mailing
list:
https://lists.01.org/pipermail/edk2-devel/2019-March/037986.html

The conclusion is that we need an individual UART for UEFI runtime
services to print debug message at runtime, to avoid conflicting with
the system UART. We cannot use the secure UART either, for we may both
have Trusted Firmware and UEFI runtime services running on the VM, and
it is not easy to keep synchronization between the two components.

To keep backward compatibility, UEFI UART is put behind the secure
UART when secure world is enabled.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Heyi Guo <guoheyi@huawei.com>
---
 hw/arm/virt.c         | 29 +++++++++++++++++++++++------
 include/hw/arm/virt.h |  1 +
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index ce2664a..cbc5a66 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -131,6 +131,7 @@ static const MemMapEntry base_memmap[] = {
     [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
     [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
     [VIRT_SMMU] =               { 0x09050000, 0x00020000 },
+    [VIRT_UEFI_UART] =          { 0x09070000, 0x00001000 },
     [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
@@ -166,6 +167,7 @@ static const int a15irqmap[] = {
     [VIRT_PCIE] = 3, /* ... to 6 */
     [VIRT_GPIO] = 7,
     [VIRT_SECURE_UART] = 8,
+    [VIRT_UEFI_UART] = 9,
     [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
     [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */
     [VIRT_SMMU] = 74,    /* ...to 74 + NUM_SMMU_IRQS - 1 */
@@ -713,13 +715,23 @@ 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);
     } else {
+        const char *status_string;
+
+        if (uart == VIRT_SECURE_UART) {
+            status_string = "secure-status";
+        } else {
+            status_string = "uefi-status";
+        }
+
         /* Mark as not usable by the normal world */
         qemu_fdt_setprop_string(vms->fdt, nodename, "status", "disabled");
-        qemu_fdt_setprop_string(vms->fdt, nodename, "secure-status", "okay");
+        qemu_fdt_setprop_string(vms->fdt, nodename, status_string, "okay");
 
-        qemu_fdt_add_subnode(vms->fdt, "/secure-chosen");
-        qemu_fdt_setprop_string(vms->fdt, "/secure-chosen", "stdout-path",
-                                nodename);
+        if (uart == VIRT_SECURE_UART) {
+            qemu_fdt_add_subnode(vms->fdt, "/secure-chosen");
+            qemu_fdt_setprop_string(vms->fdt, "/secure-chosen", "stdout-path",
+                                    nodename);
+        }
     }
 
     g_free(nodename);
@@ -1423,6 +1435,7 @@ static void machvirt_init(MachineState *machine)
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
     bool aarch64 = true;
+    int uart_index = 0;
 
     /*
      * In accelerated mode, the memory map is computed earlier in kvm_type()
@@ -1616,13 +1629,17 @@ static void machvirt_init(MachineState *machine)
 
     fdt_add_pmu_nodes(vms);
 
-    create_uart(vms, pic, VIRT_UART, sysmem, serial_hd(0));
+    create_uart(vms, pic, VIRT_UART, sysmem, serial_hd(uart_index++));
 
     if (vms->secure) {
         create_secure_ram(vms, secure_sysmem);
-        create_uart(vms, pic, VIRT_SECURE_UART, secure_sysmem, serial_hd(1));
+        create_uart(vms, pic, VIRT_SECURE_UART, secure_sysmem,
+                    serial_hd(uart_index++));
     }
 
+    /* Create UART for UEFI runtime services debug */
+    create_uart(vms, pic, VIRT_UEFI_UART, sysmem, serial_hd(uart_index));
+
     vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64);
 
     create_rtc(vms, pic);
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 507517c..565769f 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -77,6 +77,7 @@ enum {
     VIRT_GPIO,
     VIRT_SECURE_UART,
     VIRT_SECURE_MEM,
+    VIRT_UEFI_UART,
     VIRT_LOWMEMMAP_LAST,
 };
 
-- 
1.8.3.1

Re: [Qemu-devel] [RFC] arm/virt: add one more uart for UEFI runtime debug

[Peter Maydell]

On Sun, 7 Apr 2019 at 09:19, Heyi Guo <guoheyi@huawei.com> wrote:
>
> This patch is based on the discussion in TianoCore edk2-devel mailing
> list:
> https://lists.01.org/pipermail/edk2-devel/2019-March/037986.html
>
> The conclusion is that we need an individual UART for UEFI runtime
> services to print debug message at runtime, to avoid conflicting with
> the system UART. We cannot use the secure UART either, for we may both
> have Trusted Firmware and UEFI runtime services running on the VM, and
> it is not easy to keep synchronization between the two components.

I don't think it makes much sense to keep adding UARTs for every
bit of the system software stack. We don't have an infinite
supply of UARTs on real hardware either -- how is this handled
there ?

Also adding new UARTs to this board is awkward because of
the weird choices various bits of software have made in
reading the dtb and picking a UART if there's more than one
listed.

> @@ -713,13 +715,23 @@ 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);
>      } else {
> +        const char *status_string;
> +
> +        if (uart == VIRT_SECURE_UART) {
> +            status_string = "secure-status";
> +        } else {
> +            status_string = "uefi-status";

Where does this status string come from? The "secure-status" one
is documented in the device tree spec and has a well defined
meaning.

thanks
-- PMM

Re: [Qemu-devel] [RFC] arm/virt: add one more uart for UEFI runtime debug

[Peter Maydell]

On Sun, 7 Apr 2019 at 09:19, Heyi Guo <guoheyi@huawei.com> wrote:
>
> This patch is based on the discussion in TianoCore edk2-devel mailing
> list:
> https://lists.01.org/pipermail/edk2-devel/2019-March/037986.html
>
> The conclusion is that we need an individual UART for UEFI runtime
> services to print debug message at runtime, to avoid conflicting with
> the system UART. We cannot use the secure UART either, for we may both
> have Trusted Firmware and UEFI runtime services running on the VM, and
> it is not easy to keep synchronization between the two components.

I don't think it makes much sense to keep adding UARTs for every
bit of the system software stack. We don't have an infinite
supply of UARTs on real hardware either -- how is this handled
there ?

Also adding new UARTs to this board is awkward because of
the weird choices various bits of software have made in
reading the dtb and picking a UART if there's more than one
listed.

> @@ -713,13 +715,23 @@ 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);
>      } else {
> +        const char *status_string;
> +
> +        if (uart == VIRT_SECURE_UART) {
> +            status_string = "secure-status";
> +        } else {
> +            status_string = "uefi-status";

Where does this status string come from? The "secure-status" one
is documented in the device tree spec and has a well defined
meaning.

thanks
-- PMM

Re: [Qemu-devel] [RFC] arm/virt: add one more uart for UEFI runtime debug

[Heyi Guo]

On 2019/4/7 21:16, Peter Maydell wrote:
> On Sun, 7 Apr 2019 at 09:19, Heyi Guo <guoheyi@huawei.com> wrote:
>> This patch is based on the discussion in TianoCore edk2-devel mailing
>> list:
>> https://lists.01.org/pipermail/edk2-devel/2019-March/037986.html
>>
>> The conclusion is that we need an individual UART for UEFI runtime
>> services to print debug message at runtime, to avoid conflicting with
>> the system UART. We cannot use the secure UART either, for we may both
>> have Trusted Firmware and UEFI runtime services running on the VM, and
>> it is not easy to keep synchronization between the two components.
> I don't think it makes much sense to keep adding UARTs for every
> bit of the system software stack. We don't have an infinite
> supply of UARTs on real hardware either -- how is this handled
> there ?
As you were in the edk2 mail thread, I supposed you also agreed to add one more UART. What did you mean by "I do still have a todo list item to add a 2nd UART"? How shall we do that?

On our real hardware platforms, we don't have a separate serial port for UEFI runtime services; that's the reason for why I had wanted to have an adaptable approach for platform without one more serial port...

>
> Also adding new UARTs to this board is awkward because of
> the weird choices various bits of software have made in
> reading the dtb and picking a UART if there's more than one
> listed.
>
>> @@ -713,13 +715,23 @@ 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);
>>       } else {
>> +        const char *status_string;
>> +
>> +        if (uart == VIRT_SECURE_UART) {
>> +            status_string = "secure-status";
>> +        } else {
>> +            status_string = "uefi-status";
> Where does this status string come from? The "secure-status" one
> is documented in the device tree spec and has a well defined
> meaning.
Sorry this came from my imagination, so I sent it as a "RFC" :)

Thanks,
Heyi

>
> thanks
> -- PMM
>
> .
>

Re: [Qemu-devel] [RFC] arm/virt: add one more uart for UEFI runtime debug

[Heyi Guo]

On 2019/4/7 21:16, Peter Maydell wrote:
> On Sun, 7 Apr 2019 at 09:19, Heyi Guo <guoheyi@huawei.com> wrote:
>> This patch is based on the discussion in TianoCore edk2-devel mailing
>> list:
>> https://lists.01.org/pipermail/edk2-devel/2019-March/037986.html
>>
>> The conclusion is that we need an individual UART for UEFI runtime
>> services to print debug message at runtime, to avoid conflicting with
>> the system UART. We cannot use the secure UART either, for we may both
>> have Trusted Firmware and UEFI runtime services running on the VM, and
>> it is not easy to keep synchronization between the two components.
> I don't think it makes much sense to keep adding UARTs for every
> bit of the system software stack. We don't have an infinite
> supply of UARTs on real hardware either -- how is this handled
> there ?
As you were in the edk2 mail thread, I supposed you also agreed to add one more UART. What did you mean by "I do still have a todo list item to add a 2nd UART"? How shall we do that?

On our real hardware platforms, we don't have a separate serial port for UEFI runtime services; that's the reason for why I had wanted to have an adaptable approach for platform without one more serial port...

>
> Also adding new UARTs to this board is awkward because of
> the weird choices various bits of software have made in
> reading the dtb and picking a UART if there's more than one
> listed.
>
>> @@ -713,13 +715,23 @@ 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);
>>       } else {
>> +        const char *status_string;
>> +
>> +        if (uart == VIRT_SECURE_UART) {
>> +            status_string = "secure-status";
>> +        } else {
>> +            status_string = "uefi-status";
> Where does this status string come from? The "secure-status" one
> is documented in the device tree spec and has a well defined
> meaning.
Sorry this came from my imagination, so I sent it as a "RFC" :)

Thanks,
Heyi

>
> thanks
> -- PMM
>
> .
>

Re: [Qemu-devel] [RFC] arm/virt: add one more uart for UEFI runtime debug

[Peter Maydell]

On Mon, 8 Apr 2019 at 02:45, Heyi Guo <guoheyi@huawei.com> wrote:
> On 2019/4/7 21:16, Peter Maydell wrote:
> > I don't think it makes much sense to keep adding UARTs for every
> > bit of the system software stack. We don't have an infinite
> > supply of UARTs on real hardware either -- how is this handled
> > there ?
> As you were in the edk2 mail thread, I supposed you also agreed to add one more UART. What did you mean by "I do still have a todo list item to add a 2nd UART"? How shall we do that?

That would be a simple second UART available to both secure
and non-secure and advertised in the device tree. It would
only be created if the user specifically asked for it on
the QEMU command line. I still need to investigate exactly
how that ought to work, though.

thanks
-- PMM

Re: [Qemu-devel] [RFC] arm/virt: add one more uart for UEFI runtime debug

[Peter Maydell]

On Mon, 8 Apr 2019 at 02:45, Heyi Guo <guoheyi@huawei.com> wrote:
> On 2019/4/7 21:16, Peter Maydell wrote:
> > I don't think it makes much sense to keep adding UARTs for every
> > bit of the system software stack. We don't have an infinite
> > supply of UARTs on real hardware either -- how is this handled
> > there ?
> As you were in the edk2 mail thread, I supposed you also agreed to add one more UART. What did you mean by "I do still have a todo list item to add a 2nd UART"? How shall we do that?

That would be a simple second UART available to both secure
and non-secure and advertised in the device tree. It would
only be created if the user specifically asked for it on
the QEMU command line. I still need to investigate exactly
how that ought to work, though.

thanks
-- PMM