[Qemu-devel] [PATCH 0/3] hw/arm/virt: Add another UART

[Qemu-devel] [PATCH 0/3] hw/arm/virt: Add another UART

[Peter Maydell]

This patchset adds a second UART to the ARM virt board, which
can be useful in some cases where you'd like a second comms
channel but don't want have to deal with PCI to get it.

Patch 3 attempts to add it to the ACPI tables, but this is
entirely untested and pure guesswork, since I don't really
know anything about ACPI. That patch needs review from
somebody who does know ACPI :-)

thanks
-- PMM

Peter Maydell (3):
  hw/arm/virt: Add virt-2.12 machine type
  hw/arm/virt: Add another UART to the virt board
  hw/arm/virt-acpi-build: Add second UART to ACPI tables

 include/hw/arm/virt.h    |  2 ++
 include/hw/compat.h      |  3 +++
 hw/arm/virt-acpi-build.c |  5 +++++
 hw/arm/virt.c            | 38 +++++++++++++++++++++++++++++++++-----
 4 files changed, 43 insertions(+), 5 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/3] hw/arm/virt: Add virt-2.12 machine type

[Peter Maydell]

Add virt-2.12 machine type.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/compat.h |  3 +++
 hw/arm/virt.c       | 19 +++++++++++++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/include/hw/compat.h b/include/hw/compat.h
index cf389b4..263de97 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -1,6 +1,9 @@
 #ifndef HW_COMPAT_H
 #define HW_COMPAT_H
 
+#define HW_COMPAT_2_11 \
+    /* empty */
+
 #define HW_COMPAT_2_10 \
     {\
         .driver   = "virtio-mouse-device",\
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 151592b..543f9bd 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1618,7 +1618,7 @@ static void machvirt_machine_init(void)
 }
 type_init(machvirt_machine_init);
 
-static void virt_2_11_instance_init(Object *obj)
+static void virt_2_12_instance_init(Object *obj)
 {
     VirtMachineState *vms = VIRT_MACHINE(obj);
     VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
@@ -1678,10 +1678,25 @@ static void virt_2_11_instance_init(Object *obj)
     vms->irqmap = a15irqmap;
 }
 
+static void virt_machine_2_12_options(MachineClass *mc)
+{
+}
+DEFINE_VIRT_MACHINE_AS_LATEST(2, 12)
+
+#define VIRT_COMPAT_2_11 \
+    HW_COMPAT_2_11
+
+static void virt_2_11_instance_init(Object *obj)
+{
+    virt_2_12_instance_init(obj);
+}
+
 static void virt_machine_2_11_options(MachineClass *mc)
 {
+    virt_machine_2_12_options(mc);
+    SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_11);
 }
-DEFINE_VIRT_MACHINE_AS_LATEST(2, 11)
+DEFINE_VIRT_MACHINE(2, 11)
 
 #define VIRT_COMPAT_2_10 \
     HW_COMPAT_2_10
-- 
2.7.4

[Qemu-devel] [PATCH 2/3] hw/arm/virt: Add another UART to the virt board

[Peter Maydell]

Currently we only provide one non-secure UART on the virt
board. This is OK for most purposes, but there are some
use cases where having a second UART would be useful (like
bare-metal testing where you don't really want to have to
probe and set up a PCI device just to have a second comms
channel).

Add a second NS UART to the virt board. This will be the
second serial device if 'secure=no' (the default), and the
third serial device if 'secure=yes'.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/arm/virt.h |  2 ++
 hw/arm/virt.c         | 19 ++++++++++++++++---
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 33b0ff3..685009a 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -72,6 +72,7 @@ enum {
     VIRT_GPIO,
     VIRT_SECURE_UART,
     VIRT_SECURE_MEM,
+    VIRT_UART_2,
 };
 
 typedef struct MemMapEntry {
@@ -85,6 +86,7 @@ typedef struct {
     bool no_its;
     bool no_pmu;
     bool claim_edge_triggered_timers;
+    bool no_second_uart;
 } VirtMachineClass;
 
 typedef struct {
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 543f9bd..e234f55 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -139,6 +139,7 @@ static const MemMapEntry a15memmap[] = {
     [VIRT_FW_CFG] =             { 0x09020000, 0x00000018 },
     [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
     [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
+    [VIRT_UART_2] =             { 0x09050000, 0x00001000 },
     [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
@@ -157,6 +158,7 @@ static const int a15irqmap[] = {
     [VIRT_PCIE] = 3, /* ... to 6 */
     [VIRT_GPIO] = 7,
     [VIRT_SECURE_UART] = 8,
+    [VIRT_UART_2] = 9,
     [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
     [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */
     [VIRT_PLATFORM_BUS] = 112, /* ...to 112 + PLATFORM_BUS_NUM_IRQS -1 */
@@ -676,7 +678,7 @@ 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 {
+    } else if (uart == VIRT_SECURE_UART) {
         /* 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");
@@ -1260,6 +1262,7 @@ static void machvirt_init(MachineState *machine)
     int n, virt_max_cpus;
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
+    int uart_count = 0;
 
     /* We can probe only here because during property set
      * KVM is not available yet
@@ -1419,11 +1422,16 @@ static void machvirt_init(MachineState *machine)
 
     fdt_add_pmu_nodes(vms);
 
-    create_uart(vms, pic, VIRT_UART, sysmem, serial_hds[0]);
+    create_uart(vms, pic, VIRT_UART, sysmem, serial_hds[uart_count++]);
 
     if (vms->secure) {
         create_secure_ram(vms, secure_sysmem);
-        create_uart(vms, pic, VIRT_SECURE_UART, secure_sysmem, serial_hds[1]);
+        create_uart(vms, pic, VIRT_SECURE_UART, secure_sysmem,
+                    serial_hds[uart_count++]);
+    }
+
+    if (!vmc->no_second_uart) {
+        create_uart(vms, pic, VIRT_UART_2, sysmem, serial_hds[uart_count++]);
     }
 
     create_rtc(vms, pic);
@@ -1693,8 +1701,13 @@ static void virt_2_11_instance_init(Object *obj)
 
 static void virt_machine_2_11_options(MachineClass *mc)
 {
+    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
+
     virt_machine_2_12_options(mc);
     SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_11);
+
+    /* The second NS UART was added in 2.12 */
+    vmc->no_second_uart = true;
 }
 DEFINE_VIRT_MACHINE(2, 11)
 
-- 
2.7.4

[Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Peter Maydell]

Add the second UART to the ACPI tables.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Pure guesswork, as I don't have a UEFI setup to hand and
am not familiar with ACPI table formats either...
---
 hw/arm/virt-acpi-build.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 3d78ff6..a38287b 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -689,6 +689,7 @@ static void build_fadt(GArray *table_data, BIOSLinker *linker,
 static void
 build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 {
+    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
     Aml *scope, *dsdt;
     const MemMapEntry *memmap = vms->memmap;
     const int *irqmap = vms->irqmap;
@@ -706,6 +707,10 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     acpi_dsdt_add_cpus(scope, vms->smp_cpus);
     acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
                        (irqmap[VIRT_UART] + ARM_SPI_BASE));
+    if (!vmc->no_second_uart) {
+        acpi_dsdt_add_uart(scope, &memmap[VIRT_UART_2],
+                           (irqmap[VIRT_UART_2] + ARM_SPI_BASE));
+    }
     acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
     acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
     acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/3] hw/arm/virt: Add another UART

[Peter Maydell]

On 8 December 2017 at 15:02, Peter Maydell <peter.maydell@linaro.org> wrote:
> This patchset adds a second UART to the ARM virt board, which
> can be useful in some cases where you'd like a second comms
> channel but don't want have to deal with PCI to get it.
>
> Patch 3 attempts to add it to the ACPI tables, but this is
> entirely untested and pure guesswork, since I don't really
> know anything about ACPI. That patch needs review from
> somebody who does know ACPI :-)
>
> thanks
> -- PMM
>
> Peter Maydell (3):
>   hw/arm/virt: Add virt-2.12 machine type
>   hw/arm/virt: Add another UART to the virt board
>   hw/arm/virt-acpi-build: Add second UART to ACPI tables

Oops, Shannon's linaro email in MAINTAINERS bounces; re-cc'ing
with the huawei one.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Shannon Zhao]

On 2017/12/8 23:02, Peter Maydell wrote:
> Add the second UART to the ACPI tables.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Pure guesswork, as I don't have a UEFI setup to hand and
> am not familiar with ACPI table formats either...
> ---
>  hw/arm/virt-acpi-build.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 3d78ff6..a38287b 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -689,6 +689,7 @@ static void build_fadt(GArray *table_data, BIOSLinker *linker,
>  static void
>  build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>  {
> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>      Aml *scope, *dsdt;
>      const MemMapEntry *memmap = vms->memmap;
>      const int *irqmap = vms->irqmap;
> @@ -706,6 +707,10 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      acpi_dsdt_add_cpus(scope, vms->smp_cpus);
>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
> +    if (!vmc->no_second_uart) {
> +        acpi_dsdt_add_uart(scope, &memmap[VIRT_UART_2],
> +                           (irqmap[VIRT_UART_2] + ARM_SPI_BASE));
> +    }
>      acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
>      acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
>      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
> 
Reviewed-by: Shannon Zhao <zhaoshenglong@huawei.com>

-- 
Shannon

Re: [Qemu-devel] [PATCH 2/3] hw/arm/virt: Add another UART to the virt board

[Shannon Zhao]

On 2017/12/8 23:02, Peter Maydell wrote:
> Currently we only provide one non-secure UART on the virt
> board. This is OK for most purposes, but there are some
> use cases where having a second UART would be useful (like
> bare-metal testing where you don't really want to have to
> probe and set up a PCI device just to have a second comms
> channel).
> 
> Add a second NS UART to the virt board. This will be the
> second serial device if 'secure=no' (the default), and the
> third serial device if 'secure=yes'.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/arm/virt.h |  2 ++
>  hw/arm/virt.c         | 19 ++++++++++++++++---
>  2 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 33b0ff3..685009a 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -72,6 +72,7 @@ enum {
>      VIRT_GPIO,
>      VIRT_SECURE_UART,
>      VIRT_SECURE_MEM,
> +    VIRT_UART_2,
>  };
>  
>  typedef struct MemMapEntry {
> @@ -85,6 +86,7 @@ typedef struct {
>      bool no_its;
>      bool no_pmu;
>      bool claim_edge_triggered_timers;
> +    bool no_second_uart;
>  } VirtMachineClass;
>  
>  typedef struct {
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 543f9bd..e234f55 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -139,6 +139,7 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_FW_CFG] =             { 0x09020000, 0x00000018 },
>      [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
>      [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
> +    [VIRT_UART_2] =             { 0x09050000, 0x00001000 },
>      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> @@ -157,6 +158,7 @@ static const int a15irqmap[] = {
>      [VIRT_PCIE] = 3, /* ... to 6 */
>      [VIRT_GPIO] = 7,
>      [VIRT_SECURE_UART] = 8,
> +    [VIRT_UART_2] = 9,
>      [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
>      [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */
>      [VIRT_PLATFORM_BUS] = 112, /* ...to 112 + PLATFORM_BUS_NUM_IRQS -1 */
> @@ -676,7 +678,7 @@ 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 {
> +    } else if (uart == VIRT_SECURE_UART) {
>          /* 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");
> @@ -1260,6 +1262,7 @@ static void machvirt_init(MachineState *machine)
>      int n, virt_max_cpus;
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
>      bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
> +    int uart_count = 0;
>  
>      /* We can probe only here because during property set
>       * KVM is not available yet
> @@ -1419,11 +1422,16 @@ static void machvirt_init(MachineState *machine)
>  
>      fdt_add_pmu_nodes(vms);
>  
> -    create_uart(vms, pic, VIRT_UART, sysmem, serial_hds[0]);
> +    create_uart(vms, pic, VIRT_UART, sysmem, serial_hds[uart_count++]);
>  
>      if (vms->secure) {
>          create_secure_ram(vms, secure_sysmem);
> -        create_uart(vms, pic, VIRT_SECURE_UART, secure_sysmem, serial_hds[1]);
> +        create_uart(vms, pic, VIRT_SECURE_UART, secure_sysmem,
> +                    serial_hds[uart_count++]);
> +    }
> +
> +    if (!vmc->no_second_uart) {
> +        create_uart(vms, pic, VIRT_UART_2, sysmem, serial_hds[uart_count++]);
>      }
>  
>      create_rtc(vms, pic);
> @@ -1693,8 +1701,13 @@ static void virt_2_11_instance_init(Object *obj)
>  
>  static void virt_machine_2_11_options(MachineClass *mc)
>  {
> +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> +
>      virt_machine_2_12_options(mc);
>      SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_11);
> +
> +    /* The second NS UART was added in 2.12 */
> +    vmc->no_second_uart = true;
>  }
>  DEFINE_VIRT_MACHINE(2, 11)
>  
> 
I'm wondering if it need to provide a machine option for user to choose
whether adding the second uart or not.

Thanks,
-- 
Shannon

Re: [Qemu-devel] [PATCH 2/3] hw/arm/virt: Add another UART to the virt board

[Peter Maydell]

On 12 December 2017 at 05:55, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
> On 2017/12/8 23:02, Peter Maydell wrote:
>> Currently we only provide one non-secure UART on the virt
>> board. This is OK for most purposes, but there are some
>> use cases where having a second UART would be useful (like
>> bare-metal testing where you don't really want to have to
>> probe and set up a PCI device just to have a second comms
>> channel).

> I'm wondering if it need to provide a machine option for user to choose
> whether adding the second uart or not.

It seems harmless enough to me to provide it always.
It doesn't increase the attack surface for security
vulnerabilities because it's the same device as the
first UART the guest already has access to.

Do you have a scenario in mind where it would be bad
to provide the second uart?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Laszlo Ersek]

On 12/12/17 06:53, Shannon Zhao wrote:
> 
> 
> On 2017/12/8 23:02, Peter Maydell wrote:
>> Add the second UART to the ACPI tables.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> Pure guesswork, as I don't have a UEFI setup to hand and
>> am not familiar with ACPI table formats either...
>> ---
>>  hw/arm/virt-acpi-build.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 3d78ff6..a38287b 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -689,6 +689,7 @@ static void build_fadt(GArray *table_data, BIOSLinker *linker,
>>  static void
>>  build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>  {
>> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>>      Aml *scope, *dsdt;
>>      const MemMapEntry *memmap = vms->memmap;
>>      const int *irqmap = vms->irqmap;
>> @@ -706,6 +707,10 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>      acpi_dsdt_add_cpus(scope, vms->smp_cpus);
>>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
>>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
>> +    if (!vmc->no_second_uart) {
>> +        acpi_dsdt_add_uart(scope, &memmap[VIRT_UART_2],
>> +                           (irqmap[VIRT_UART_2] + ARM_SPI_BASE));
>> +    }
>>      acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
>>      acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
>>      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
>>
> Reviewed-by: Shannon Zhao <zhaoshenglong@huawei.com>
> 

(Responding here so I don't have to manually update Shannon's email
address while replying to patch 3/3 directly.)

acpi_dsdt_add_uart() does:

    Aml *dev = aml_device("COM0");
    aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0011")));
    aml_append(dev, aml_name_decl("_UID", aml_int(0)));

The device name should be unique. Plus, within the same _HID, the _UID
should be unique as well. I suggest adding another unsigned parameter to
acpi_dsdt_add_uart(), and hooking it into COMx and _UID.


BTW, has anyone tested this with the ArmVirtQemu firmware? As far as I
can see from the firmware code, the firmware will use the PL011 whose
description comes first in the DTB (and ignore the other PL011), in an
fdt_next_node() traversal. Is that OK for the intended use case?
(Perhaps I should have asked this under the second patch, which updates
the DTB generator.)

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Peter Maydell]

On 12 December 2017 at 11:06, Laszlo Ersek <lersek@redhat.com> wrote:
> BTW, has anyone tested this with the ArmVirtQemu firmware? As far as I
> can see from the firmware code, the firmware will use the PL011 whose
> description comes first in the DTB (and ignore the other PL011), in an
> fdt_next_node() traversal. Is that OK for the intended use case?
> (Perhaps I should have asked this under the second patch, which updates
> the DTB generator.)

I haven't tested, since I don't have a working setup for that to hand.
(Do you have instructions somewhere for getting it working?)

The behaviour we would want would be for the firmware to keep using
the PL011 at 0x09000000. (In an ideal world the firmware would
prefer the UART marked in the 'stdout-path' in the DTB /chosen node,
as the kernel does, I guess.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Laszlo Ersek]

On 12/12/17 12:11, Peter Maydell wrote:
> On 12 December 2017 at 11:06, Laszlo Ersek <lersek@redhat.com> wrote:
>> BTW, has anyone tested this with the ArmVirtQemu firmware? As far as I
>> can see from the firmware code, the firmware will use the PL011 whose
>> description comes first in the DTB (and ignore the other PL011), in an
>> fdt_next_node() traversal. Is that OK for the intended use case?
>> (Perhaps I should have asked this under the second patch, which updates
>> the DTB generator.)
> 
> I haven't tested, since I don't have a working setup for that to hand.
> (Do you have instructions somewhere for getting it working?)

The Wiki page I most frequently refer to (for a libvirt-less description
anyway) is Ard's:

  https://wiki.linaro.org/LEG/UEFIforQEMU

There's also:

  https://www.kraxel.org/repos/
  https://fedoraproject.org/wiki/Architectures/AArch64/Install_with_QEMU

> The behaviour we would want would be for the firmware to keep using
> the PL011 at 0x09000000.

With these QEMU patches, I reckon that's going to happen, yes.

> (In an ideal world the firmware would
> prefer the UART marked in the 'stdout-path' in the DTB /chosen node,
> as the kernel does, I guess.)

Hmmm, I recall that we used to have some code related to the /chosen
node... We have a helper function for locating that
(GetOrInsertChosenNode), but we no longer use it, it seems? The last
(only?) use was apparently removed in:

https://github.com/tianocore/edk2/commit/29589acf1010

I'll let Ard comment too.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Ard Biesheuvel]

On 12 December 2017 at 11:33, Laszlo Ersek <lersek@redhat.com> wrote:
> On 12/12/17 12:11, Peter Maydell wrote:
>> On 12 December 2017 at 11:06, Laszlo Ersek <lersek@redhat.com> wrote:
>>> BTW, has anyone tested this with the ArmVirtQemu firmware? As far as I
>>> can see from the firmware code, the firmware will use the PL011 whose
>>> description comes first in the DTB (and ignore the other PL011), in an
>>> fdt_next_node() traversal. Is that OK for the intended use case?
>>> (Perhaps I should have asked this under the second patch, which updates
>>> the DTB generator.)
>>
>> I haven't tested, since I don't have a working setup for that to hand.
>> (Do you have instructions somewhere for getting it working?)
>
> The Wiki page I most frequently refer to (for a libvirt-less description
> anyway) is Ard's:
>
>   https://wiki.linaro.org/LEG/UEFIforQEMU
>
> There's also:
>
>   https://www.kraxel.org/repos/
>   https://fedoraproject.org/wiki/Architectures/AArch64/Install_with_QEMU
>
>> The behaviour we would want would be for the firmware to keep using
>> the PL011 at 0x09000000.
>
> With these QEMU patches, I reckon that's going to happen, yes.
>

IIRC the DT nodes appear in the opposite order as they are added, so
adding the second UART after adding the original one is probably going
to cause trouble.

We do check the 'status' though, so if backward compatibility with
older firmwares is a concern, we could set status=disabled, and let
newer firmware builds enable it on the fly.

>> (In an ideal world the firmware would
>> prefer the UART marked in the 'stdout-path' in the DTB /chosen node,
>> as the kernel does, I guess.)
>
> Hmmm, I recall that we used to have some code related to the /chosen
> node... We have a helper function for locating that
> (GetOrInsertChosenNode), but we no longer use it, it seems? The last
> (only?) use was apparently removed in:
>
> https://github.com/tianocore/edk2/commit/29589acf1010
>
> I'll let Ard comment too.
>

That would require us to implement some non-trivial parsing and alias
resolution, given that stdout-path is simply a string that may refer
to a UART node via an alias.

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Laszlo Ersek]

On 12/12/17 12:44, Ard Biesheuvel wrote:
> On 12 December 2017 at 11:33, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 12/12/17 12:11, Peter Maydell wrote:
>>> On 12 December 2017 at 11:06, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> BTW, has anyone tested this with the ArmVirtQemu firmware? As far as I
>>>> can see from the firmware code, the firmware will use the PL011 whose
>>>> description comes first in the DTB (and ignore the other PL011), in an
>>>> fdt_next_node() traversal. Is that OK for the intended use case?
>>>> (Perhaps I should have asked this under the second patch, which updates
>>>> the DTB generator.)
>>>
>>> I haven't tested, since I don't have a working setup for that to hand.
>>> (Do you have instructions somewhere for getting it working?)
>>
>> The Wiki page I most frequently refer to (for a libvirt-less description
>> anyway) is Ard's:
>>
>>   https://wiki.linaro.org/LEG/UEFIforQEMU
>>
>> There's also:
>>
>>   https://www.kraxel.org/repos/
>>   https://fedoraproject.org/wiki/Architectures/AArch64/Install_with_QEMU
>>
>>> The behaviour we would want would be for the firmware to keep using
>>> the PL011 at 0x09000000.
>>
>> With these QEMU patches, I reckon that's going to happen, yes.
>>
> 
> IIRC the DT nodes appear in the opposite order as they are added,

Oh! Thanks for catching that!

> so
> adding the second UART after adding the original one is probably going
> to cause trouble.
> 
> We do check the 'status' though, so if backward compatibility with
> older firmwares is a concern, we could set status=disabled, and let
> newer firmware builds enable it on the fly.

Peter, what is the intended use of the second UART?

(In the firmware, a second PL011 could be useful: it might make it
possible to separate the debug output (debug messages) from interactive
serial console IO. (On x86, in the default OVMF build, debug messages
are written to the QEMU debug IO port, and the serial port is used only
for interactive console IO.) I don't remember how important we deemed
this separation for aarch64, last time it surfaced.)

>>> (In an ideal world the firmware would
>>> prefer the UART marked in the 'stdout-path' in the DTB /chosen node,
>>> as the kernel does, I guess.)
>>
>> Hmmm, I recall that we used to have some code related to the /chosen
>> node... We have a helper function for locating that
>> (GetOrInsertChosenNode), but we no longer use it, it seems? The last
>> (only?) use was apparently removed in:
>>
>> https://github.com/tianocore/edk2/commit/29589acf1010
>>
>> I'll let Ard comment too.
>>
> 
> That would require us to implement some non-trivial parsing and alias
> resolution, given that stdout-path is simply a string that may refer
> to a UART node via an alias.

Let's not do that please...

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Peter Maydell]

On 12 December 2017 at 11:59, Laszlo Ersek <lersek@redhat.com> wrote:
> Peter, what is the intended use of the second UART?

Per the commit message:
# there are some
# use cases where having a second UART would be useful (like
# bare-metal testing where you don't really want to have to
# probe and set up a PCI device just to have a second comms
# channel).

>From QEMU's point of view this is just "some people would like
more than one UART, x86 provides more than one UART, it's
easy to provide an extra UART and it's up to the guest what
it would like to do with it". If there's utility for the
OVMF firmware in having 2 UARTs that's doubly good reason
to have it.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Laszlo Ersek]

On 12/12/17 13:07, Peter Maydell wrote:
> On 12 December 2017 at 11:59, Laszlo Ersek <lersek@redhat.com> wrote:
>> Peter, what is the intended use of the second UART?
> 
> Per the commit message:
> # there are some
> # use cases where having a second UART would be useful (like
> # bare-metal testing where you don't really want to have to
> # probe and set up a PCI device just to have a second comms
> # channel).
> 
> From QEMU's point of view this is just "some people would like
> more than one UART, x86 provides more than one UART, it's
> easy to provide an extra UART and it's up to the guest what
> it would like to do with it". If there's utility for the
> OVMF firmware in having 2 UARTs that's doubly good reason
> to have it.

I agree. There's one user-visible complication: while with one UART,
it's not possible to mix things up, with two UARTs, users will
inevitably want to assign inverse roles to them, relative to what QEMU /
the firmware assigns originally.

E.g. if one PL011 is redirected to a regular file (debug messages) and
the other one to stdio (console), there will be complaints that "I
wanted it the other way around". The redirection happens on the backend
(chardev) level, but the firmware won't see the difference on the
frontend (PL011) level.

Is it possible to add a new property to the UART nodes in the DTB, like
"purpose"?

Or can we make a virt-arm policy that "first -serial is always ...,
second -serial is always ..."?

(
Technically the latter could work, because:

- QEMU_OPTION_serial / add_device_config() keeps the command line order,
- the traversal of DEV_SERIAL with serial_parse() also keeps that order,
- machvirt_init() uses the same order via serial_hds[]
- create_uart() always prepends the new node to the DTB, via
  qemu_fdt_add_subnode() (if I understood Ard right).

I'm not sure though whether libvirt would like the ordering of -serial
options to carry any meaning.
)

Sorry if these questions don't belong on the QEMU level.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Peter Maydell]

On 12 December 2017 at 12:41, Laszlo Ersek <lersek@redhat.com> wrote:
> I agree. There's one user-visible complication: while with one UART,
> it's not possible to mix things up, with two UARTs, users will
> inevitably want to assign inverse roles to them, relative to what QEMU /
> the firmware assigns originally.
>
> E.g. if one PL011 is redirected to a regular file (debug messages) and
> the other one to stdio (console), there will be complaints that "I
> wanted it the other way around". The redirection happens on the backend
> (chardev) level, but the firmware won't see the difference on the
> frontend (PL011) level.
>
> Is it possible to add a new property to the UART nodes in the DTB, like
> "purpose"?

This is what the "chosen" stdout-path node in the DTB is for,
but you said you didn't want to look at that :-) (it means
"device to be used for boot console output".)

> Or can we make a virt-arm policy that "first -serial is always ...,
> second -serial is always ..."?

Well, the first -serial will always be the 0x09000000 one
that we have today, and the second -serial will be the other
one (unless you have -machine secure=yes, in which case
second -serial is the secure-only UART and third -serial is
the second NS UART).

Is this any different to the x86 case, where there are two
UARTs at different IO port/IRQ locations?

I think the only thing users really expect is that if you're
using just the one serial port for anything then it's the
first one.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Laszlo Ersek]

On 12/12/17 13:47, Peter Maydell wrote:
> On 12 December 2017 at 12:41, Laszlo Ersek <lersek@redhat.com> wrote:
>> I agree. There's one user-visible complication: while with one UART,
>> it's not possible to mix things up, with two UARTs, users will
>> inevitably want to assign inverse roles to them, relative to what QEMU /
>> the firmware assigns originally.
>>
>> E.g. if one PL011 is redirected to a regular file (debug messages) and
>> the other one to stdio (console), there will be complaints that "I
>> wanted it the other way around". The redirection happens on the backend
>> (chardev) level, but the firmware won't see the difference on the
>> frontend (PL011) level.
>>
>> Is it possible to add a new property to the UART nodes in the DTB, like
>> "purpose"?
> 
> This is what the "chosen" stdout-path node in the DTB is for,
> but you said you didn't want to look at that :-) (it means
> "device to be used for boot console output".)

:(

Ard is right that we really shouldn't do that kind of parsing magic in
very early UEFI stuff.


>> Or can we make a virt-arm policy that "first -serial is always ...,
>> second -serial is always ..."?
> 
> Well, the first -serial will always be the 0x09000000 one
> that we have today, and the second -serial will be the other
> one (unless you have -machine secure=yes, in which case
> second -serial is the secure-only UART and third -serial is
> the second NS UART).
> 
> Is this any different to the x86 case, where there are two
> UARTs at different IO port/IRQ locations?

OVMF (x86) uses two distinct devices for the two purposes discussed.

* It uses the "debug console device" for debug message output, at
hard-coded IO port 0x402; so if you'd like to capture those messages,
then you have to add:

  -chardev file,id=debugfile,path=debug.log \
  -device isa-debugcon,iobase=0x402,chardev=debugfile \

(or the more traditional

  -debugcon file:debug.log \
  -global isa-debugcon.iobase=0x402 \
)

* For console I/O, it uses the first serial port. (I think I have once
investigated what it takes to use other serial ports for console I/O --
I'm no longer sure if "it happens to be impossible in OVMF", or just
"nobody ever does that".)


The important distinction (on the UEFI level anyway) is that the debug
messages need to go to a super dumb device, while console I/O can use a
much higher-level serial IO abstraction ("protocol").


> I think the only thing users really expect is that if you're
> using just the one serial port for anything then it's the
> first one.

You are right -- we've never had two (non-secure) UARTs on virt-arm, so
we can invent whatever assignment, as long as:

- the single UART case doesn't break (keeps receiving both debug output
and console IO),

- whatever we invent for the two UARTs case now, it remains consistent
over time. I think this implies we can rely on the FDT node ordering in
the firmware (if we want to use the 2nd UART at all, that is), and then
QEMU shouldn't change the serial_hds[] <-> FDT node mapping in the
future, in machvirt_init() and the callees thereof.

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Ard Biesheuvel]

On 12 December 2017 at 13:28, Laszlo Ersek <lersek@redhat.com> wrote:
> On 12/12/17 13:47, Peter Maydell wrote:
>> On 12 December 2017 at 12:41, Laszlo Ersek <lersek@redhat.com> wrote:
>>> I agree. There's one user-visible complication: while with one UART,
>>> it's not possible to mix things up, with two UARTs, users will
>>> inevitably want to assign inverse roles to them, relative to what QEMU /
>>> the firmware assigns originally.
>>>
>>> E.g. if one PL011 is redirected to a regular file (debug messages) and
>>> the other one to stdio (console), there will be complaints that "I
>>> wanted it the other way around". The redirection happens on the backend
>>> (chardev) level, but the firmware won't see the difference on the
>>> frontend (PL011) level.
>>>
>>> Is it possible to add a new property to the UART nodes in the DTB, like
>>> "purpose"?
>>
>> This is what the "chosen" stdout-path node in the DTB is for,
>> but you said you didn't want to look at that :-) (it means
>> "device to be used for boot console output".)
>
> :(
>
> Ard is right that we really shouldn't do that kind of parsing magic in
> very early UEFI stuff.
>
>
>>> Or can we make a virt-arm policy that "first -serial is always ...,
>>> second -serial is always ..."?
>>
>> Well, the first -serial will always be the 0x09000000 one
>> that we have today, and the second -serial will be the other
>> one (unless you have -machine secure=yes, in which case
>> second -serial is the secure-only UART and third -serial is
>> the second NS UART).
>>
>> Is this any different to the x86 case, where there are two
>> UARTs at different IO port/IRQ locations?
>
> OVMF (x86) uses two distinct devices for the two purposes discussed.
>
> * It uses the "debug console device" for debug message output, at
> hard-coded IO port 0x402; so if you'd like to capture those messages,
> then you have to add:
>
>   -chardev file,id=debugfile,path=debug.log \
>   -device isa-debugcon,iobase=0x402,chardev=debugfile \
>
> (or the more traditional
>
>   -debugcon file:debug.log \
>   -global isa-debugcon.iobase=0x402 \
> )
>
> * For console I/O, it uses the first serial port. (I think I have once
> investigated what it takes to use other serial ports for console I/O --
> I'm no longer sure if "it happens to be impossible in OVMF", or just
> "nobody ever does that".)
>
>
> The important distinction (on the UEFI level anyway) is that the debug
> messages need to go to a super dumb device, while console I/O can use a
> much higher-level serial IO abstraction ("protocol").
>
>
>> I think the only thing users really expect is that if you're
>> using just the one serial port for anything then it's the
>> first one.
>
> You are right -- we've never had two (non-secure) UARTs on virt-arm, so
> we can invent whatever assignment, as long as:
>
> - the single UART case doesn't break (keeps receiving both debug output
> and console IO),
>
> - whatever we invent for the two UARTs case now, it remains consistent
> over time. I think this implies we can rely on the FDT node ordering in
> the firmware (if we want to use the 2nd UART at all, that is), and then
> QEMU shouldn't change the serial_hds[] <-> FDT node mapping in the
> future, in machvirt_init() and the callees thereof.
>

Given that the DEBUG output is only produced on DEBUG builds, couldn't
we simply stipulate that DEBUG output appears on the PL011 with the
lowest physical address? That keeps the current status quo, and is
probably sufficient for 99% of the use cases of people using the DEBUG
builds.

Then, we can introduce some code to decode stdout-path in FdtClientDxe
(a higher level DT parsing driver) so that the actual serial console
gets installed onto whichever UART is described as the system console.
Also, given that ArmVirtQemu is tightly coupled to QEMU/mach-virt, we
could decide to only use node names in stdout-path (as we do
currently) to simplify the parsing logic.

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Andrew Jones]

On Tue, Dec 12, 2017 at 02:28:51PM +0100, Laszlo Ersek wrote:
> On 12/12/17 13:47, Peter Maydell wrote:
> > On 12 December 2017 at 12:41, Laszlo Ersek <lersek@redhat.com> wrote:
> >> I agree. There's one user-visible complication: while with one UART,
> >> it's not possible to mix things up, with two UARTs, users will
> >> inevitably want to assign inverse roles to them, relative to what QEMU /
> >> the firmware assigns originally.
> >>
> >> E.g. if one PL011 is redirected to a regular file (debug messages) and
> >> the other one to stdio (console), there will be complaints that "I
> >> wanted it the other way around". The redirection happens on the backend
> >> (chardev) level, but the firmware won't see the difference on the
> >> frontend (PL011) level.
> >>
> >> Is it possible to add a new property to the UART nodes in the DTB, like
> >> "purpose"?
> > 
> > This is what the "chosen" stdout-path node in the DTB is for,
> > but you said you didn't want to look at that :-) (it means
> > "device to be used for boot console output".)
> 
> :(
> 
> Ard is right that we really shouldn't do that kind of parsing magic in
> very early UEFI stuff.
> 
> 
> >> Or can we make a virt-arm policy that "first -serial is always ...,
> >> second -serial is always ..."?
> > 
> > Well, the first -serial will always be the 0x09000000 one
> > that we have today, and the second -serial will be the other
> > one (unless you have -machine secure=yes, in which case
> > second -serial is the secure-only UART and third -serial is
> > the second NS UART).
> > 
> > Is this any different to the x86 case, where there are two
> > UARTs at different IO port/IRQ locations?
> 
> OVMF (x86) uses two distinct devices for the two purposes discussed.
> 
> * It uses the "debug console device" for debug message output, at
> hard-coded IO port 0x402; so if you'd like to capture those messages,
> then you have to add:
> 
>   -chardev file,id=debugfile,path=debug.log \
>   -device isa-debugcon,iobase=0x402,chardev=debugfile \
> 
> (or the more traditional
> 
>   -debugcon file:debug.log \
>   -global isa-debugcon.iobase=0x402 \
> )
> 
> * For console I/O, it uses the first serial port. (I think I have once

If "first" here means the first '-serial' argument on the command line,
then mach-virt has the same requirement. The console must be the first
'-serial' argument. The secure UART, provided when the 'secure' property
is set, gets the next one. So

 -chardev file,path=secure-uart.log,id=s -serial stdio -serial chardev:s

works as expected, but

 -chardev file,path=secure-uart.log,id=s -serial chardev:s -serial stdio

does not.

This series keeps that requirement and adds that the second NS UART's
'-serial' argument must come after the secure UART, when the machine
has the 'secure' property set.

Command line ordering requirements aren't awesome, so it might be nice
to add a serial option 'console', allowing

 -chardev file,path=secure-uart.log,id=s -serial chardev:s -serial stdio,console

to work too.

> investigated what it takes to use other serial ports for console I/O --
> I'm no longer sure if "it happens to be impossible in OVMF", or just
> "nobody ever does that".)
> 
> 
> The important distinction (on the UEFI level anyway) is that the debug
> messages need to go to a super dumb device, while console I/O can use a
> much higher-level serial IO abstraction ("protocol").
> 
> 
> > I think the only thing users really expect is that if you're
> > using just the one serial port for anything then it's the
> > first one.
> 
> You are right -- we've never had two (non-secure) UARTs on virt-arm, so
> we can invent whatever assignment, as long as:
> 
> - the single UART case doesn't break (keeps receiving both debug output
> and console IO),
> 
> - whatever we invent for the two UARTs case now, it remains consistent
> over time. I think this implies we can rely on the FDT node ordering in
> the firmware (if we want to use the 2nd UART at all, that is), and then
> QEMU shouldn't change the serial_hds[] <-> FDT node mapping in the
> future, in machvirt_init() and the callees thereof.
> 
> Thanks!
> Laszlo
> 

Personally I think AAVMF should learn how to parse stdout-path, but as
an interim solution it could just hard code the console address
(0x9000000), like it does the base of memory (which I think it should
eventually learn to get from the DT, pointed to by register x0, too :-)

Always generating the second UART's FDT node with status = "disabled",
as Ard suggests, even when the user has provided a '-serial' command
line option for it, i.e. serial_hds[i] != NULL, i > 0, doesn't seem like
what we'd want to do. Wouldn't Linux assume (when boot directly without
AAVMF) that the UART should not be used in that case?

Thanks,
drew

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Peter Maydell]

On 12 December 2017 at 13:56, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Given that the DEBUG output is only produced on DEBUG builds, couldn't
> we simply stipulate that DEBUG output appears on the PL011 with the
> lowest physical address? That keeps the current status quo, and is
> probably sufficient for 99% of the use cases of people using the DEBUG
> builds.
>
> Then, we can introduce some code to decode stdout-path in FdtClientDxe
> (a higher level DT parsing driver) so that the actual serial console
> gets installed onto whichever UART is described as the system console.
> Also, given that ArmVirtQemu is tightly coupled to QEMU/mach-virt, we
> could decide to only use node names in stdout-path (as we do
> currently) to simplify the parsing logic.

That doesn't actually usefully separate debug output from the
console, though, because stdout-path is also going to point
to the UART with the lowest physical address...

-- PMM

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Ard Biesheuvel]

On 12 December 2017 at 14:10, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 December 2017 at 13:56, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> Given that the DEBUG output is only produced on DEBUG builds, couldn't
>> we simply stipulate that DEBUG output appears on the PL011 with the
>> lowest physical address? That keeps the current status quo, and is
>> probably sufficient for 99% of the use cases of people using the DEBUG
>> builds.
>>
>> Then, we can introduce some code to decode stdout-path in FdtClientDxe
>> (a higher level DT parsing driver) so that the actual serial console
>> gets installed onto whichever UART is described as the system console.
>> Also, given that ArmVirtQemu is tightly coupled to QEMU/mach-virt, we
>> could decide to only use node names in stdout-path (as we do
>> currently) to simplify the parsing logic.
>
> That doesn't actually usefully separate debug output from the
> console, though, because stdout-path is also going to point
> to the UART with the lowest physical address...
>

By default, yes, just like is currently the case. But I would assume
this new serial port could be appointed 'console' and put into
stdout-path by QEMU, based on the command line options?

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Peter Maydell]

On 12 December 2017 at 14:12, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 12 December 2017 at 14:10, Peter Maydell <peter.maydell@linaro.org> wrote:
>> That doesn't actually usefully separate debug output from the
>> console, though, because stdout-path is also going to point
>> to the UART with the lowest physical address...
>>
>
> By default, yes, just like is currently the case. But I would assume
> this new serial port could be appointed 'console' and put into
> stdout-path by QEMU, based on the command line options?

We don't have any command line options for doing that, and I'm
generally reluctant to introduce new command line UI, especially
new command line UI that's specific to Arm and not also needed
for x86.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Ard Biesheuvel]

On 12 December 2017 at 14:13, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 December 2017 at 14:12, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 12 December 2017 at 14:10, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> That doesn't actually usefully separate debug output from the
>>> console, though, because stdout-path is also going to point
>>> to the UART with the lowest physical address...
>>>
>>
>> By default, yes, just like is currently the case. But I would assume
>> this new serial port could be appointed 'console' and put into
>> stdout-path by QEMU, based on the command line options?
>
> We don't have any command line options for doing that, and I'm
> generally reluctant to introduce new command line UI, especially
> new command line UI that's specific to Arm and not also needed
> for x86.
>

If stdout-path is always going to point to pl011@0x9000000, why would
we need to parse it?

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Peter Maydell]

On 12 December 2017 at 14:16, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 12 December 2017 at 14:13, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 12 December 2017 at 14:12, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>> On 12 December 2017 at 14:10, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> That doesn't actually usefully separate debug output from the
>>>> console, though, because stdout-path is also going to point
>>>> to the UART with the lowest physical address...
>>>>
>>>
>>> By default, yes, just like is currently the case. But I would assume
>>> this new serial port could be appointed 'console' and put into
>>> stdout-path by QEMU, based on the command line options?
>>
>> We don't have any command line options for doing that, and I'm
>> generally reluctant to introduce new command line UI, especially
>> new command line UI that's specific to Arm and not also needed
>> for x86.
>>
>
> If stdout-path is always going to point to pl011@0x9000000, why would
> we need to parse it?

If you had always parsed stdout-path, we wouldn't need to worry
about the order in which the UART nodes appear in the dtb...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Andrew Jones]

On Tue, Dec 12, 2017 at 01:56:44PM +0000, Ard Biesheuvel wrote:
> On 12 December 2017 at 13:28, Laszlo Ersek <lersek@redhat.com> wrote:
> > On 12/12/17 13:47, Peter Maydell wrote:
> >> On 12 December 2017 at 12:41, Laszlo Ersek <lersek@redhat.com> wrote:
> >>> I agree. There's one user-visible complication: while with one UART,
> >>> it's not possible to mix things up, with two UARTs, users will
> >>> inevitably want to assign inverse roles to them, relative to what QEMU /
> >>> the firmware assigns originally.
> >>>
> >>> E.g. if one PL011 is redirected to a regular file (debug messages) and
> >>> the other one to stdio (console), there will be complaints that "I
> >>> wanted it the other way around". The redirection happens on the backend
> >>> (chardev) level, but the firmware won't see the difference on the
> >>> frontend (PL011) level.
> >>>
> >>> Is it possible to add a new property to the UART nodes in the DTB, like
> >>> "purpose"?
> >>
> >> This is what the "chosen" stdout-path node in the DTB is for,
> >> but you said you didn't want to look at that :-) (it means
> >> "device to be used for boot console output".)
> >
> > :(
> >
> > Ard is right that we really shouldn't do that kind of parsing magic in
> > very early UEFI stuff.
> >
> >
> >>> Or can we make a virt-arm policy that "first -serial is always ...,
> >>> second -serial is always ..."?
> >>
> >> Well, the first -serial will always be the 0x09000000 one
> >> that we have today, and the second -serial will be the other
> >> one (unless you have -machine secure=yes, in which case
> >> second -serial is the secure-only UART and third -serial is
> >> the second NS UART).
> >>
> >> Is this any different to the x86 case, where there are two
> >> UARTs at different IO port/IRQ locations?
> >
> > OVMF (x86) uses two distinct devices for the two purposes discussed.
> >
> > * It uses the "debug console device" for debug message output, at
> > hard-coded IO port 0x402; so if you'd like to capture those messages,
> > then you have to add:
> >
> >   -chardev file,id=debugfile,path=debug.log \
> >   -device isa-debugcon,iobase=0x402,chardev=debugfile \
> >
> > (or the more traditional
> >
> >   -debugcon file:debug.log \
> >   -global isa-debugcon.iobase=0x402 \
> > )
> >
> > * For console I/O, it uses the first serial port. (I think I have once
> > investigated what it takes to use other serial ports for console I/O --
> > I'm no longer sure if "it happens to be impossible in OVMF", or just
> > "nobody ever does that".)
> >
> >
> > The important distinction (on the UEFI level anyway) is that the debug
> > messages need to go to a super dumb device, while console I/O can use a
> > much higher-level serial IO abstraction ("protocol").
> >
> >
> >> I think the only thing users really expect is that if you're
> >> using just the one serial port for anything then it's the
> >> first one.
> >
> > You are right -- we've never had two (non-secure) UARTs on virt-arm, so
> > we can invent whatever assignment, as long as:
> >
> > - the single UART case doesn't break (keeps receiving both debug output
> > and console IO),
> >
> > - whatever we invent for the two UARTs case now, it remains consistent
> > over time. I think this implies we can rely on the FDT node ordering in
> > the firmware (if we want to use the 2nd UART at all, that is), and then
> > QEMU shouldn't change the serial_hds[] <-> FDT node mapping in the
> > future, in machvirt_init() and the callees thereof.
> >
> 
> Given that the DEBUG output is only produced on DEBUG builds, couldn't
> we simply stipulate that DEBUG output appears on the PL011 with the
> lowest physical address? That keeps the current status quo, and is
> probably sufficient for 99% of the use cases of people using the DEBUG
> builds.

I was thinking that if AAVMF learned to use a second UART for debug
output, that the debug builds could go away, as they have for x86 (IIUC).
If a user wires up a second UART, then AAVMF could unconditionally
output debug messages to it. Not wiring it up would lose the messages,
which is no different than the non-verbose build we have today. It
would be nice if could tell AAVMF not to use a second UART for debug
messages too, though, as the debug messages make AAVMF quite slow, and
the user may want to provide the guest a second UART.

> 
> Then, we can introduce some code to decode stdout-path in FdtClientDxe
> (a higher level DT parsing driver) so that the actual serial console
> gets installed onto whichever UART is described as the system console.
> Also, given that ArmVirtQemu is tightly coupled to QEMU/mach-virt, we
> could decide to only use node names in stdout-path (as we do
> currently) to simplify the parsing logic.
>

That sounds reasonable and would only require adding a comment above
the setting of stdout-path in the QEMU code to formalize it.

Thanks,
drew

Re: [Qemu-devel] [PATCH 2/3] hw/arm/virt: Add another UART to the virt board

[Jason A. Donenfeld]

In case it's of direct interest in this thread, the ARM results on
https://www.wireguard.com/build-status/ are running on a QEMU built
with this disgusting cludge: https://א.cc/roJ0gMw3

The patch of this thread is the actually correct way of accomplishing
what that cludge does.

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Ard Biesheuvel]

On 12 December 2017 at 14:17, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 December 2017 at 14:16, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 12 December 2017 at 14:13, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 12 December 2017 at 14:12, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>> On 12 December 2017 at 14:10, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>> That doesn't actually usefully separate debug output from the
>>>>> console, though, because stdout-path is also going to point
>>>>> to the UART with the lowest physical address...
>>>>>
>>>>
>>>> By default, yes, just like is currently the case. But I would assume
>>>> this new serial port could be appointed 'console' and put into
>>>> stdout-path by QEMU, based on the command line options?
>>>
>>> We don't have any command line options for doing that, and I'm
>>> generally reluctant to introduce new command line UI, especially
>>> new command line UI that's specific to Arm and not also needed
>>> for x86.
>>>
>>
>> If stdout-path is always going to point to pl011@0x9000000, why would
>> we need to parse it?
>
> If you had always parsed stdout-path, we wouldn't need to worry
> about the order in which the UART nodes appear in the dtb...
>

That's a fair point. But it still does not justify introducing added
complexity in the firmware, if the conclusion is always going to be
that the console will be on pl011@9000000.

To Drew's point re DEBUG builds, I don't think we should generally
enable DEBUG and send the output to nowhereland if the user does not
wire it up. That's a MMIO trap and two world switches for each
character written, if I am not mistaken, and the DEBUG builds are very
noisy.

But still, given that stdout-path is essentially written in stone
anyway, could we also decide that it will always refer to
pl011@9000000 by its nodename? That way, we can easily implement the
early DEBUG logic by using any non-disabled non-secure PL011 that is
not referenced in stdout-path, and falling back to the one that is in
stdout-path, which will essentially work as it does today. And for the
more elaborate DXE driver, we can find the nodename from stdout-path.

Re: [Qemu-devel] [PATCH 1/3] hw/arm/virt: Add virt-2.12 machine type

[Andrew Jones]

On Fri, Dec 08, 2017 at 03:02:06PM +0000, Peter Maydell wrote:
> Add virt-2.12 machine type.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/compat.h |  3 +++
>  hw/arm/virt.c       | 19 +++++++++++++++++--
>  2 files changed, 20 insertions(+), 2 deletions(-)


Reviewed-by: Andrew Jones <drjones@redhat.com>

> 
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index cf389b4..263de97 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -1,6 +1,9 @@
>  #ifndef HW_COMPAT_H
>  #define HW_COMPAT_H
>  
> +#define HW_COMPAT_2_11 \
> +    /* empty */
> +
>  #define HW_COMPAT_2_10 \
>      {\
>          .driver   = "virtio-mouse-device",\
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 151592b..543f9bd 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1618,7 +1618,7 @@ static void machvirt_machine_init(void)
>  }
>  type_init(machvirt_machine_init);
>  
> -static void virt_2_11_instance_init(Object *obj)
> +static void virt_2_12_instance_init(Object *obj)
>  {
>      VirtMachineState *vms = VIRT_MACHINE(obj);
>      VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> @@ -1678,10 +1678,25 @@ static void virt_2_11_instance_init(Object *obj)
>      vms->irqmap = a15irqmap;
>  }
>  
> +static void virt_machine_2_12_options(MachineClass *mc)
> +{
> +}
> +DEFINE_VIRT_MACHINE_AS_LATEST(2, 12)
> +
> +#define VIRT_COMPAT_2_11 \
> +    HW_COMPAT_2_11
> +
> +static void virt_2_11_instance_init(Object *obj)
> +{
> +    virt_2_12_instance_init(obj);
> +}
> +
>  static void virt_machine_2_11_options(MachineClass *mc)
>  {
> +    virt_machine_2_12_options(mc);
> +    SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_11);
>  }
> -DEFINE_VIRT_MACHINE_AS_LATEST(2, 11)
> +DEFINE_VIRT_MACHINE(2, 11)
>  
>  #define VIRT_COMPAT_2_10 \
>      HW_COMPAT_2_10
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH 2/3] hw/arm/virt: Add another UART to the virt board

[Andrew Jones]

On Fri, Dec 08, 2017 at 03:02:07PM +0000, Peter Maydell wrote:
> Currently we only provide one non-secure UART on the virt
> board. This is OK for most purposes, but there are some
> use cases where having a second UART would be useful (like
> bare-metal testing where you don't really want to have to
> probe and set up a PCI device just to have a second comms
> channel).
> 
> Add a second NS UART to the virt board. This will be the
> second serial device if 'secure=no' (the default), and the
> third serial device if 'secure=yes'.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/arm/virt.h |  2 ++
>  hw/arm/virt.c         | 19 ++++++++++++++++---
>  2 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 33b0ff3..685009a 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -72,6 +72,7 @@ enum {
>      VIRT_GPIO,
>      VIRT_SECURE_UART,
>      VIRT_SECURE_MEM,
> +    VIRT_UART_2,
>  };
>  
>  typedef struct MemMapEntry {
> @@ -85,6 +86,7 @@ typedef struct {
>      bool no_its;
>      bool no_pmu;
>      bool claim_edge_triggered_timers;
> +    bool no_second_uart;
>  } VirtMachineClass;
>  
>  typedef struct {
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 543f9bd..e234f55 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -139,6 +139,7 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_FW_CFG] =             { 0x09020000, 0x00000018 },
>      [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
>      [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
> +    [VIRT_UART_2] =             { 0x09050000, 0x00001000 },
>      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> @@ -157,6 +158,7 @@ static const int a15irqmap[] = {
>      [VIRT_PCIE] = 3, /* ... to 6 */
>      [VIRT_GPIO] = 7,
>      [VIRT_SECURE_UART] = 8,
> +    [VIRT_UART_2] = 9,
>      [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
>      [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */
>      [VIRT_PLATFORM_BUS] = 112, /* ...to 112 + PLATFORM_BUS_NUM_IRQS -1 */
> @@ -676,7 +678,7 @@ 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 {
> +    } else if (uart == VIRT_SECURE_UART) {
>          /* 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");
> @@ -1260,6 +1262,7 @@ static void machvirt_init(MachineState *machine)
>      int n, virt_max_cpus;
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
>      bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
> +    int uart_count = 0;
>  
>      /* We can probe only here because during property set
>       * KVM is not available yet
> @@ -1419,11 +1422,16 @@ static void machvirt_init(MachineState *machine)
>  
>      fdt_add_pmu_nodes(vms);
>  
> -    create_uart(vms, pic, VIRT_UART, sysmem, serial_hds[0]);
> +    create_uart(vms, pic, VIRT_UART, sysmem, serial_hds[uart_count++]);
>  
>      if (vms->secure) {
>          create_secure_ram(vms, secure_sysmem);
> -        create_uart(vms, pic, VIRT_SECURE_UART, secure_sysmem, serial_hds[1]);
> +        create_uart(vms, pic, VIRT_SECURE_UART, secure_sysmem,
> +                    serial_hds[uart_count++]);
> +    }
> +
> +    if (!vmc->no_second_uart) {

Should this be

    if (!vmc->no_second_uart && serial_hds[uart_count] != NULL) {

A bit late now, but maybe the other UART resource allocations should have
been conditional on actually being wired up too?

Thanks,
drew

> +        create_uart(vms, pic, VIRT_UART_2, sysmem, serial_hds[uart_count++]);
>      }
>  
>      create_rtc(vms, pic);
> @@ -1693,8 +1701,13 @@ static void virt_2_11_instance_init(Object *obj)
>  
>  static void virt_machine_2_11_options(MachineClass *mc)
>  {
> +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> +
>      virt_machine_2_12_options(mc);
>      SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_11);
> +
> +    /* The second NS UART was added in 2.12 */
> +    vmc->no_second_uart = true;
>  }
>  DEFINE_VIRT_MACHINE(2, 11)
>  
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH 2/3] hw/arm/virt: Add another UART to the virt board

[Peter Maydell]

On 12 December 2017 at 14:45, Andrew Jones <drjones@redhat.com> wrote:
> Should this be
>
>     if (!vmc->no_second_uart && serial_hds[uart_count] != NULL) {
>
> A bit late now, but maybe the other UART resource allocations should have
> been conditional on actually being wired up too?

What does x86 do?

I think the conclusion we came to about UARTs in general was that
boards should always create the devices, and the UART models
should cope with being passed a NULL chardev backend. (This
is like real hardware, where the 2nd UART always exists even
if you haven't plugged a cable into the 2nd serial port on the
back of the box.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Andrew Jones]

On Tue, Dec 12, 2017 at 02:31:05PM +0000, Ard Biesheuvel wrote:
> On 12 December 2017 at 14:17, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 12 December 2017 at 14:16, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >> On 12 December 2017 at 14:13, Peter Maydell <peter.maydell@linaro.org> wrote:
> >>> On 12 December 2017 at 14:12, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >>>> On 12 December 2017 at 14:10, Peter Maydell <peter.maydell@linaro.org> wrote:
> >>>>> That doesn't actually usefully separate debug output from the
> >>>>> console, though, because stdout-path is also going to point
> >>>>> to the UART with the lowest physical address...
> >>>>>
> >>>>
> >>>> By default, yes, just like is currently the case. But I would assume
> >>>> this new serial port could be appointed 'console' and put into
> >>>> stdout-path by QEMU, based on the command line options?
> >>>
> >>> We don't have any command line options for doing that, and I'm
> >>> generally reluctant to introduce new command line UI, especially
> >>> new command line UI that's specific to Arm and not also needed
> >>> for x86.
> >>>
> >>
> >> If stdout-path is always going to point to pl011@0x9000000, why would
> >> we need to parse it?
> >
> > If you had always parsed stdout-path, we wouldn't need to worry
> > about the order in which the UART nodes appear in the dtb...
> >
> 
> That's a fair point. But it still does not justify introducing added
> complexity in the firmware, if the conclusion is always going to be
> that the console will be on pl011@9000000.
> 
> To Drew's point re DEBUG builds, I don't think we should generally
> enable DEBUG and send the output to nowhereland if the user does not
> wire it up. That's a MMIO trap and two world switches for each
> character written, if I am not mistaken, and the DEBUG builds are very
> noisy.

100% agree, which is why I said "If a user wires up a second UART...",
but I guess my "Not wiring it up would lose the messages..." sentence
was ambiguous. I didn't mean they'd output to no where, I meant those
messages wouldn't be output. I.e. some sort of debug_printf() function
would be written that only actually does a write when there's a debug
UART on the other end to write to.

I just commented on patch 2/3 of this series asking if we shouldn't
just not allocate the second UART's MMIO region and IRQ, when the user
doesn't provide a '-serial' option for it. If we didn't, then AAVMF
would just not output debug messages when only one UART (the console)
is found in the DT.

Thanks,
drew

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Andrew Jones]

On Tue, Dec 12, 2017 at 12:06:39PM +0100, Laszlo Ersek wrote:
> On 12/12/17 06:53, Shannon Zhao wrote:
> > 
> > 
> > On 2017/12/8 23:02, Peter Maydell wrote:
> >> Add the second UART to the ACPI tables.
> >>
> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> >> ---
> >> Pure guesswork, as I don't have a UEFI setup to hand and
> >> am not familiar with ACPI table formats either...
> >> ---
> >>  hw/arm/virt-acpi-build.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> >> index 3d78ff6..a38287b 100644
> >> --- a/hw/arm/virt-acpi-build.c
> >> +++ b/hw/arm/virt-acpi-build.c
> >> @@ -689,6 +689,7 @@ static void build_fadt(GArray *table_data, BIOSLinker *linker,
> >>  static void
> >>  build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> >>  {
> >> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> >>      Aml *scope, *dsdt;
> >>      const MemMapEntry *memmap = vms->memmap;
> >>      const int *irqmap = vms->irqmap;
> >> @@ -706,6 +707,10 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> >>      acpi_dsdt_add_cpus(scope, vms->smp_cpus);
> >>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
> >>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
> >> +    if (!vmc->no_second_uart) {
> >> +        acpi_dsdt_add_uart(scope, &memmap[VIRT_UART_2],
> >> +                           (irqmap[VIRT_UART_2] + ARM_SPI_BASE));
> >> +    }
> >>      acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> >>      acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
> >>      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
> >>
> > Reviewed-by: Shannon Zhao <zhaoshenglong@huawei.com>
> > 
> 
> (Responding here so I don't have to manually update Shannon's email
> address while replying to patch 3/3 directly.)
> 
> acpi_dsdt_add_uart() does:
> 
>     Aml *dev = aml_device("COM0");
>     aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0011")));
>     aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> 
> The device name should be unique. Plus, within the same _HID, the _UID
> should be unique as well. I suggest adding another unsigned parameter to
> acpi_dsdt_add_uart(), and hooking it into COMx and _UID.

Yes, I agree with Laszlo here (and confirmed with Igor). Also, if memory
serves me, the _ADR part of acpi_dsdt_add_uart() is only there to deal
with early Linux ACPI support not finding the console UART correctly.
I believe it does now, meaning the _ADR could even be removed for COM0,
and we certainly don't want it for COM1 because it's point is to find
COM0, so the new unsigned parameter should also be used as condition for
it. A later patch could remove it altogether.

Thanks,
drew

Re: [Qemu-devel] [PATCH 2/3] hw/arm/virt: Add another UART to the virt board

[Andrew Jones]

On Tue, Dec 12, 2017 at 02:50:50PM +0000, Peter Maydell wrote:
> On 12 December 2017 at 14:45, Andrew Jones <drjones@redhat.com> wrote:
> > Should this be
> >
> >     if (!vmc->no_second_uart && serial_hds[uart_count] != NULL) {
> >
> > A bit late now, but maybe the other UART resource allocations should have
> > been conditional on actually being wired up too?
> 
> What does x86 do?

Looks like they provide the io-ports unconditionally.

> 
> I think the conclusion we came to about UARTs in general was that
> boards should always create the devices, and the UART models
> should cope with being passed a NULL chardev backend. (This
> is like real hardware, where the 2nd UART always exists even
> if you haven't plugged a cable into the 2nd serial port on the
> back of the box.)

Yes, I guess the real hardware analogy provides the justification
for doing it this way.

OK, so this patch is fine, but it kills my suggestion for AAVMF to
only output debug messages to a debug port when the debug port is
actually useful. Now it won't be able to tell the difference unless
we can inform it some other way.

Thanks,
drew

> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH 2/3] hw/arm/virt: Add another UART to the virt board

[Andrew Jones]

On Fri, Dec 08, 2017 at 03:02:07PM +0000, Peter Maydell wrote:
> Currently we only provide one non-secure UART on the virt
> board. This is OK for most purposes, but there are some
> use cases where having a second UART would be useful (like
> bare-metal testing where you don't really want to have to
> probe and set up a PCI device just to have a second comms
> channel).
> 
> Add a second NS UART to the virt board. This will be the
> second serial device if 'secure=no' (the default), and the
> third serial device if 'secure=yes'.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/arm/virt.h |  2 ++
>  hw/arm/virt.c         | 19 ++++++++++++++++---
>  2 files changed, 18 insertions(+), 3 deletions(-)


Reviewed-by: Andrew Jones <drjones@redhat.com>

> 
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 33b0ff3..685009a 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -72,6 +72,7 @@ enum {
>      VIRT_GPIO,
>      VIRT_SECURE_UART,
>      VIRT_SECURE_MEM,
> +    VIRT_UART_2,
>  };
>  
>  typedef struct MemMapEntry {
> @@ -85,6 +86,7 @@ typedef struct {
>      bool no_its;
>      bool no_pmu;
>      bool claim_edge_triggered_timers;
> +    bool no_second_uart;
>  } VirtMachineClass;
>  
>  typedef struct {
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 543f9bd..e234f55 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -139,6 +139,7 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_FW_CFG] =             { 0x09020000, 0x00000018 },
>      [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
>      [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
> +    [VIRT_UART_2] =             { 0x09050000, 0x00001000 },
>      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> @@ -157,6 +158,7 @@ static const int a15irqmap[] = {
>      [VIRT_PCIE] = 3, /* ... to 6 */
>      [VIRT_GPIO] = 7,
>      [VIRT_SECURE_UART] = 8,
> +    [VIRT_UART_2] = 9,
>      [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
>      [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */
>      [VIRT_PLATFORM_BUS] = 112, /* ...to 112 + PLATFORM_BUS_NUM_IRQS -1 */
> @@ -676,7 +678,7 @@ 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 {
> +    } else if (uart == VIRT_SECURE_UART) {
>          /* 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");
> @@ -1260,6 +1262,7 @@ static void machvirt_init(MachineState *machine)
>      int n, virt_max_cpus;
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
>      bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
> +    int uart_count = 0;
>  
>      /* We can probe only here because during property set
>       * KVM is not available yet
> @@ -1419,11 +1422,16 @@ static void machvirt_init(MachineState *machine)
>  
>      fdt_add_pmu_nodes(vms);
>  
> -    create_uart(vms, pic, VIRT_UART, sysmem, serial_hds[0]);
> +    create_uart(vms, pic, VIRT_UART, sysmem, serial_hds[uart_count++]);
>  
>      if (vms->secure) {
>          create_secure_ram(vms, secure_sysmem);
> -        create_uart(vms, pic, VIRT_SECURE_UART, secure_sysmem, serial_hds[1]);
> +        create_uart(vms, pic, VIRT_SECURE_UART, secure_sysmem,
> +                    serial_hds[uart_count++]);
> +    }
> +
> +    if (!vmc->no_second_uart) {
> +        create_uart(vms, pic, VIRT_UART_2, sysmem, serial_hds[uart_count++]);
>      }
>  
>      create_rtc(vms, pic);
> @@ -1693,8 +1701,13 @@ static void virt_2_11_instance_init(Object *obj)
>  
>  static void virt_machine_2_11_options(MachineClass *mc)
>  {
> +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> +
>      virt_machine_2_12_options(mc);
>      SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_11);
> +
> +    /* The second NS UART was added in 2.12 */
> +    vmc->no_second_uart = true;
>  }
>  DEFINE_VIRT_MACHINE(2, 11)
>  
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH 2/3] hw/arm/virt: Add another UART to the virt board

[Peter Maydell]

On 12 December 2017 at 15:16, Andrew Jones <drjones@redhat.com> wrote:
> OK, so this patch is fine, but it kills my suggestion for AAVMF to
> only output debug messages to a debug port when the debug port is
> actually useful. Now it won't be able to tell the difference unless
> we can inform it some other way.

Yeah, and that might be a nice thing to do. I'm just a little
reluctant to have Arm start doing something that we don't
already do for x86, or plan to do on x86 in the same way
at the same time.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Laszlo Ersek]

On 12/12/17 15:51, Andrew Jones wrote:
> On Tue, Dec 12, 2017 at 02:31:05PM +0000, Ard Biesheuvel wrote:
>> On 12 December 2017 at 14:17, Peter Maydell
>> <peter.maydell@linaro.org> wrote:
>>> On 12 December 2017 at 14:16, Ard Biesheuvel
>>> <ard.biesheuvel@linaro.org> wrote:
>>>> On 12 December 2017 at 14:13, Peter Maydell
>>>> <peter.maydell@linaro.org> wrote:
>>>>> On 12 December 2017 at 14:12, Ard Biesheuvel
>>>>> <ard.biesheuvel@linaro.org> wrote:
>>>>>> On 12 December 2017 at 14:10, Peter Maydell
>>>>>> <peter.maydell@linaro.org> wrote:
>>>>>>> That doesn't actually usefully separate debug output from the
>>>>>>> console, though, because stdout-path is also going to point
>>>>>>> to the UART with the lowest physical address...
>>>>>>>
>>>>>>
>>>>>> By default, yes, just like is currently the case. But I would
>>>>>> assume this new serial port could be appointed 'console' and put
>>>>>> into stdout-path by QEMU, based on the command line options?
>>>>>
>>>>> We don't have any command line options for doing that, and I'm
>>>>> generally reluctant to introduce new command line UI, especially
>>>>> new command line UI that's specific to Arm and not also needed
>>>>> for x86.
>>>>>
>>>>
>>>> If stdout-path is always going to point to pl011@0x9000000, why
>>>> would we need to parse it?
>>>
>>> If you had always parsed stdout-path, we wouldn't need to worry
>>> about the order in which the UART nodes appear in the dtb...
>>>
>>
>> That's a fair point. But it still does not justify introducing added
>> complexity in the firmware, if the conclusion is always going to be
>> that the console will be on pl011@9000000.
>>
>> To Drew's point re DEBUG builds, I don't think we should generally
>> enable DEBUG and send the output to nowhereland if the user does not
>> wire it up. That's a MMIO trap and two world switches for each
>> character written, if I am not mistaken, and the DEBUG builds are
>> very noisy.
>
> 100% agree, which is why I said "If a user wires up a second UART...",
> but I guess my "Not wiring it up would lose the messages..." sentence
> was ambiguous. I didn't mean they'd output to no where, I meant those
> messages wouldn't be output. I.e. some sort of debug_printf() function
> would be written that only actually does a write when there's a debug
> UART on the other end to write to.

Right; this can be implemented based on the DTB. Please see below.


> I just commented on patch 2/3 of this series asking if we shouldn't
> just not allocate the second UART's MMIO region and IRQ, when the user
> doesn't provide a '-serial' option for it. If we didn't, then AAVMF
> would just not output debug messages when only one UART (the console)
> is found in the DT.

This is what we ultimately want down-stream, yes; but up-stream, it
would be an incompatible change -- DEBUG builds used with one UART only
would suddenly not produce debug messages.


I don't think I can continue discussing this without going into
ArmVirtQemu specifics. We have the following layers of drivers and
library classes / instances (more abstract higher up on the diagram).

The top left node stands for debug messages written by any modules, and
the top right node stands for the abstraction (EFI_SERIAL_IO_PROTOCOL)
through which interactive console / terminal IO occurs ultimately (it
requires a few more layers on top but those don't matter for now).


[any driver that writes debug messages]    SerialDxe
  |  |                                     (DXE driver that produces
  |  |                                     EFI_SERIAL_IO_PROTOCOL;
  |  DebugLib:BaseDebugLibNull  +-----+----uses DebugLib for one
  |  (used for RELEASE builds; /     /     ASSERT_EFI_ERROR();
  |  DEBUG messages are dropped     /      uses SerialPortLib for
  |  indiscriminately)             /       producing
  |                               /        EFI_SERIAL_IO_PROTOCOL)
  |                              /           |
  DebugLib:BaseDebugLibSerialPort            |
  (for DEBUG / NOOPT builds;                 |
  debug messages are written to              |
  the serial port)                           |
    |  |                                     |
    |  |                                     |
    |  SerialPortLib: FdtPL011SerialPortLib  |
    |  (used by DXE and later modules, the UART
    |  base need not be parsed repeatedly from the
    |  DTB)                                       \
    |                                              \
    |                                               \
    SerialPortLib: EarlyFdtPL011SerialPortLib        \
    (used for early (pre-DXE) modules; the   \        +
    DTB is parsed on every serial port        \       |
    action for the UART base address)          \      |
                                                \     |
                                                 \    |
                                                  PL011UartLib
                                                  (actual hw access)

This is super messy, because in a DEBUG build, debug messages (such as
an assertion failure report) in SerialDxe itself would have to be logged
to "one" serial port, but the exact same driver should use the "other"
serial port for implementing EFI_SERIAL_IO_PROTOCOL. Currently both
"paths" go through FdtPL011SerialPortLib, which -- i.e., the
SerialPortLib *class* itself -- cannot handle more than one serial port
(in the same UEFI executable).

So, for upstream,

(1) we need two new DebugLib instances (pre-DXE, and DXE-or-later), for
    the ArmVirtQemu platform, such that they use PL011UartLib directly,
    without going through the SerialPortLib class.

    These DebugLib instances will have to parse the DTB for the UART
    base address (on all calls, or initially). If there is only one
    UART, use that, otherwise, use the one that is found second in the
    DTB.

(2) In RELEASE builds, BaseDebugLibNull should remain OK (DEBUG messages
    are dropped statically).

(3) The current SerialPortLib instances need not be changed; they can
    continue using the first UART found in the DTB.


For downstream, the DebugLib instances from (1) can be customized so
that debug messages are dropped if only one UART is found in the DTB.
(I.e., never write a DEBUG message unless you can write it separate from
whatever port is used by FdtPL011SerialPortLib.)

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Peter Maydell]

On 12 December 2017 at 11:06, Laszlo Ersek <lersek@redhat.com> wrote:
> BTW, has anyone tested this with the ArmVirtQemu firmware? As far as I
> can see from the firmware code, the firmware will use the PL011 whose
> description comes first in the DTB (and ignore the other PL011), in an
> fdt_next_node() traversal. Is that OK for the intended use case?

I have now tested this, and annoyingly UEFI and the kernel seem
to disagree about enumeration order. That is, if QEMU creates
them in the code in the order 0x09050000 (uart 2), 0x09000000 (uart 1),
then they appear in the dtb with uart 1 first, and the kernel enumerates
them as ttyAMA0 being uart 1 and ttyAMA1 being uart 2, but UEFI
outputs to uart 2...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Laszlo Ersek]

On 12/13/17 14:56, Peter Maydell wrote:
> On 12 December 2017 at 11:06, Laszlo Ersek <lersek@redhat.com> wrote:
>> BTW, has anyone tested this with the ArmVirtQemu firmware? As far as I
>> can see from the firmware code, the firmware will use the PL011 whose
>> description comes first in the DTB (and ignore the other PL011), in an
>> fdt_next_node() traversal. Is that OK for the intended use case?
> 
> I have now tested this,

Thank you!

> and annoyingly UEFI and the kernel seem
> to disagree about enumeration order. That is, if QEMU creates
> them in the code in the order 0x09050000 (uart 2), 0x09000000 (uart 1),
> then they appear in the dtb with uart 1 first, and the kernel enumerates
> them as ttyAMA0 being uart 1 and ttyAMA1 being uart 2, but UEFI
> outputs to uart 2...

Ouch. This reminds me (remotely) of QEMU commit 587078f0ed63
("hw/arm/virt: explain device-to-transport mapping in
create_virtio_devices()", 2015-02-05).

I'd still like to avoid the "sophisticated" /chosen lookup (the lookup
itself is not too complex, but evaluating whatever we find there against
each of the scanned UART nodes appears difficult, if I'm to understand
Ard's earlier point correctly). I hope that we can match the kernel's
logic with simple modifications to our scanning loops, e.g. we could
simply pick the last UART rather than the first, or else do a maximum
(or minimum) search for the UART base, and stick with the maximum (or
minimum) found.

However, for that, we first have to understand what the kernel does. Can
someone explain that? (I tried taking a look, but it's turtles all the
way down.)

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Ard Biesheuvel]

On 13 December 2017 at 16:01, Laszlo Ersek <lersek@redhat.com> wrote:
> On 12/13/17 14:56, Peter Maydell wrote:
>> On 12 December 2017 at 11:06, Laszlo Ersek <lersek@redhat.com> wrote:
>>> BTW, has anyone tested this with the ArmVirtQemu firmware? As far as I
>>> can see from the firmware code, the firmware will use the PL011 whose
>>> description comes first in the DTB (and ignore the other PL011), in an
>>> fdt_next_node() traversal. Is that OK for the intended use case?
>>
>> I have now tested this,
>
> Thank you!
>
>> and annoyingly UEFI and the kernel seem
>> to disagree about enumeration order. That is, if QEMU creates
>> them in the code in the order 0x09050000 (uart 2), 0x09000000 (uart 1),
>> then they appear in the dtb with uart 1 first, and the kernel enumerates
>> them as ttyAMA0 being uart 1 and ttyAMA1 being uart 2, but UEFI
>> outputs to uart 2...
>
> Ouch. This reminds me (remotely) of QEMU commit 587078f0ed63
> ("hw/arm/virt: explain device-to-transport mapping in
> create_virtio_devices()", 2015-02-05).
>
> I'd still like to avoid the "sophisticated" /chosen lookup (the lookup
> itself is not too complex, but evaluating whatever we find there against
> each of the scanned UART nodes appears difficult, if I'm to understand
> Ard's earlier point correctly).

This may be a lot simpler than I originally thought. stdout-path
contains a string of the from

<nodename-or-alias>[:<baudrate[<parity>[<data-bits>[<flow-control>]]]]

where nodename-or-alias can be resolved into a path via fdt_get_alias
(), or used directly as a path otherwise.

> I hope that we can match the kernel's
> logic with simple modifications to our scanning loops, e.g. we could
> simply pick the last UART rather than the first, or else do a maximum
> (or minimum) search for the UART base, and stick with the maximum (or
> minimum) found.
>
> However, for that, we first have to understand what the kernel does. Can
> someone explain that? (I tried taking a look, but it's turtles all the
> way down.)
>

The kernel uses the contents of the SPCR table or /chosen/stdout-path
as the console unless overridden on the kernel command line. This is
independent of enumeration order.

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

[Peter Maydell]

On 13 December 2017 at 16:46, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> The kernel uses the contents of the SPCR table or /chosen/stdout-path
> as the console unless overridden on the kernel command line. This is
> independent of enumeration order.

True, but we don't want to assign ttyAMA0 and ttyAMA1 to the
UARTs arbitrarily -- ttyAMA0 must continue to be the one
at address 0x09000000, or we break commandlines.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/3] hw/arm/virt: Add virt-2.12 machine type

[Peter Maydell]

On 12 December 2017 at 14:39, Andrew Jones <drjones@redhat.com> wrote:
> On Fri, Dec 08, 2017 at 03:02:06PM +0000, Peter Maydell wrote:
>> Add virt-2.12 machine type.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  include/hw/compat.h |  3 +++
>>  hw/arm/virt.c       | 19 +++++++++++++++++--
>>  2 files changed, 20 insertions(+), 2 deletions(-)
>
>
> Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks. The rest of this series has issues but I'm going
to apply this patch to target-arm.next (minus the compat.h
change which has already gone into master via another commit).

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/3] hw/arm/virt: Add another UART to the virt board

[Jason A. Donenfeld]

Hi Peter,

What ever became of this? I still really could use a second UART in
the standard configuration. I'm still building with this cludge --
https://א.cc/RXI3ssWV -- in order to run the test suite on
build.wireguard.com.

Thanks,
Jason

Re: [Qemu-devel] [PATCH 2/3] hw/arm/virt: Add another UART to the virt board

[Peter Maydell]

On 31 May 2018 at 03:12, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> Hi Peter,
>
> What ever became of this? I still really could use a second UART in
> the standard configuration. I'm still building with this cludge --
> https://א.cc/RXI3ssWV -- in order to run the test suite on
> build.wireguard.com.

It stalled on the fact that adding the second UART breaks
UEFI images (annoyingly, UEFI iterates through UARTs in
the DTB in the opposite direction to Linux, so if you add
a second UART then it picks that one to use rather than
the first one, IIRC). So it would have to be an awkward
thing with a new machine option to request the extra UART.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/3] hw/arm/virt: Add another UART to the virt board

[Jason A. Donenfeld]

On Thu, May 31, 2018 at 10:32 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> It stalled on the fact that adding the second UART breaks
> UEFI images (annoyingly, UEFI iterates through UARTs in
> the DTB in the opposite direction to Linux, so if you add
> a second UART then it picks that one to use rather than
> the first one, IIRC). So it would have to be an awkward
> thing with a new machine option to request the extra UART.

Can you just add them in the opposite order for UEFI then?

Re: [Qemu-devel] [PATCH 2/3] hw/arm/virt: Add another UART to the virt board

[Peter Maydell]

On 31 May 2018 at 12:48, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On Thu, May 31, 2018 at 10:32 AM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> It stalled on the fact that adding the second UART breaks
>> UEFI images (annoyingly, UEFI iterates through UARTs in
>> the DTB in the opposite direction to Linux, so if you add
>> a second UART then it picks that one to use rather than
>> the first one, IIRC). So it would have to be an awkward
>> thing with a new machine option to request the extra UART.
>
> Can you just add them in the opposite order for UEFI then?

No, because that would break Linux guests. We don't
know what guest software we're running (and indeed
UEFI typically proceeds to boot Linux).

thanks
-- PMM