diff mbox series

[2/3] hw/arm/virt: Add another UART to the virt board

Message ID 1512745328-5109-3-git-send-email-peter.maydell@linaro.org
State Superseded
Headers show
Series hw/arm/virt: Add another UART | expand

Commit Message

Peter Maydell Dec. 8, 2017, 3:02 p.m. UTC
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(-)

-- 
2.7.4

Comments

Shannon Zhao Dec. 12, 2017, 5:55 a.m. UTC | #1
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
Peter Maydell Dec. 12, 2017, 10:45 a.m. UTC | #2
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
Jason A. Donenfeld Dec. 12, 2017, 2:25 p.m. UTC | #3
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.
Andrew Jones Dec. 12, 2017, 2:45 p.m. UTC | #4
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

> 

>
Peter Maydell Dec. 12, 2017, 2:50 p.m. UTC | #5
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
Andrew Jones Dec. 12, 2017, 3:16 p.m. UTC | #6
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

>
Andrew Jones Dec. 12, 2017, 3:16 p.m. UTC | #7
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

> 

>
Peter Maydell Dec. 12, 2017, 3:51 p.m. UTC | #8
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
Jason A. Donenfeld May 31, 2018, 2:12 a.m. UTC | #9
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
Peter Maydell May 31, 2018, 8:32 a.m. UTC | #10
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
Jason A. Donenfeld May 31, 2018, 11:48 a.m. UTC | #11
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?
Peter Maydell May 31, 2018, 11:57 a.m. UTC | #12
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
diff mbox series

Patch

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)