Re: [PATCH v2] riscv/virt: Add optional UART1

Alistair,


I was using  0x10000100 at first but soon noticed it is in the same MMU
page as UART0, which is difficult for pass-through hypervisor guests.
So current approach seems a little more flexible?

Regards,
yanfeng


On Tue, 2026-07-14 at 01:13 +0000, Alistair Francis wrote:
> On Fri, 2026-07-03 at 12:24 +0800, Yanfeng Liu wrote:
> > This adds optional UART1 to RiscV virt board if required at
> > runtime to simplify multicore development.
> > 
> > Note that UART0 remains default serial_hd(0) and it is:
> > 
> > - the lowest address UART
> > - first serial in DTB
> > - behind /aliases/serial0 in DTB
> > - the /chosen/stdout-path in DTB
> > 
> > Signed-off-by: Yanfeng Liu <[email protected]>
> > ---
> >  docs/system/riscv/virt.rst |  5 ++++-
> >  hw/riscv/virt-acpi-build.c | 12 ++++++++----
> >  hw/riscv/virt.c            | 39 ++++++++++++++++++++++++++++------
> > --
> > --
> >  include/hw/riscv/virt.h    |  3 +++
> >  4 files changed, 44 insertions(+), 15 deletions(-)
> > 
> > diff --git a/docs/system/riscv/virt.rst
> > b/docs/system/riscv/virt.rst
> > index 60850970ce..45ac63ac77 100644
> > --- a/docs/system/riscv/virt.rst
> > +++ b/docs/system/riscv/virt.rst
> > @@ -16,7 +16,7 @@ The ``virt`` machine supports the following
> > devices:
> >  * Core Local Interruptor (CLINT)
> >  * Platform-Level Interrupt Controller (PLIC)
> >  * CFI parallel NOR flash memory
> > -* 1 NS16550 compatible UART
> > +* Either 1 or 2 NS16550 compatible UARTs
> >  * 1 Google Goldfish RTC
> >  * 1 SiFive Test device
> >  * 8 virtio-mmio transport devices
> > @@ -27,6 +27,9 @@ The hypervisor extension has been enabled for the
> > default CPU, so virtual
> >  machines with hypervisor extension can simply be used without
> > explicitly
> >  declaring.
> >  
> > +The second UART only exists if a backend is configured explicitly
> > (e.g.
> > +with a second `-serial` command line option).
> > +
> >  Hardware configuration information
> >  ----------------------------------
> >  
> > diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-
> > build.c
> > index 413d47d70e..597751c84a 100644
> > --- a/hw/riscv/virt-acpi-build.c
> > +++ b/hw/riscv/virt-acpi-build.c
> > @@ -169,11 +169,11 @@ static void acpi_dsdt_add_plic_aplic(Aml
> > *scope, uint8_t socket_count,
> >  
> >  static void
> >  acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap,
> > -                    uint32_t uart_irq)
> > +                    uint32_t uart_irq, int uartidx)
> >  {
> > -    Aml *dev = aml_device("COM0");
> > +    Aml *dev = aml_device("COM%d", uartidx);
> >      aml_append(dev, aml_name_decl("_HID",
> > aml_string("RSCV0003")));
> > -    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> > +    aml_append(dev, aml_name_decl("_UID", aml_int(uartidx)));
> >  
> >      Aml *crs = aml_resource_template();
> >      aml_append(crs, aml_memory32_fixed(uart_memmap->base,
> > @@ -479,7 +479,11 @@ static void build_dsdt(GArray *table_data,
> >                                   memmap[VIRT_APLIC_S].size,
> > "RSCV0002");
> >      }
> >  
> > -    acpi_dsdt_add_uart(scope, &memmap[VIRT_UART0], UART0_IRQ);
> > +    acpi_dsdt_add_uart(scope, &memmap[VIRT_UART0], UART0_IRQ, 0);
> > +    if (s->uart1_present) {
> > +        acpi_dsdt_add_uart(scope, &memmap[VIRT_UART1], UART1_IRQ,
> > 1);
> > +    }
> > +
> >      if (virt_is_iommu_sys_enabled(s)) {
> >          acpi_dsdt_add_iommu_sys(scope, &memmap[VIRT_IOMMU_SYS],
> > IOMMU_SYS_IRQ);
> >      }
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index 33775a61fd..9a112ccdef 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -96,6 +96,7 @@ static const MemMapEntry virt_memmap[] = {
> >      [VIRT_APLIC_S] =      {  0xd000000, APLIC_SIZE(VIRT_CPUS_MAX)
> > },
> >      [VIRT_UART0] =        { 0x10000000,         0x100 },
> >      [VIRT_VIRTIO] =       { 0x10001000,        0x1000 },
> > +    [VIRT_UART1] =        { 0x1000a000,         0x100 },
> 
> This could be at address 0x10000100 instead and squish up against
> UART0.
> 
> Otherwise:
> 
> Reviewed-by: Alistair Francis <[email protected]>
> 
> Alistair
> 
> >      [VIRT_FW_CFG] =       { 0x10100000,          0x18 },
> >      [VIRT_FLASH] =        { 0x20000000,     0x4000000 },
> >      [VIRT_IMSIC_M] =      { 0x24000000, VIRT_IMSIC_MAX_SIZE },
> > @@ -186,7 +187,8 @@ static void create_pcie_irq_map(RISCVVirtState
> > *s, void *fdt, char *nodename,
> >                            FDT_MAX_INT_MAP_WIDTH] = {};
> >      uint32_t *irq_map = full_irq_map;
> >  
> > -    /* This code creates a standard swizzle of interrupts such
> > that
> > +    /*
> > +     * This code creates a standard swizzle of interrupts such
> > that
> >       * each device's first interrupt is based on it's PCI_SLOT
> > number.
> >       * (See pci_swizzle_map_irq_fn())
> >       *
> > @@ -832,28 +834,38 @@ static void create_fdt_reset(RISCVVirtState
> > *s,
> > uint32_t *phandle)
> >  }
> >  
> >  static void create_fdt_uart(RISCVVirtState *s,
> > -                            uint32_t irq_mmio_phandle)
> > +                            uint32_t irq_mmio_phandle, int memId,
> > int irqNo)
> >  {
> >      g_autofree char *name = NULL;
> >      MachineState *ms = MACHINE(s);
> >  
> >      name = g_strdup_printf("/soc/serial@%"HWADDR_PRIx,
> > -                           s->memmap[VIRT_UART0].base);
> > +                           s->memmap[memId].base);
> >      qemu_fdt_add_subnode(ms->fdt, name);
> >      qemu_fdt_setprop_string(ms->fdt, name, "compatible",
> > "ns16550a");
> >      qemu_fdt_setprop_sized_cells(ms->fdt, name, "reg",
> > -                                 2, s->memmap[VIRT_UART0].base,
> > -                                 2, s->memmap[VIRT_UART0].size);
> > +                                 2, s->memmap[memId].base,
> > +                                 2, s->memmap[memId].size);
> >      qemu_fdt_setprop_cell(ms->fdt, name, "clock-frequency",
> > 3686400);
> >      qemu_fdt_setprop_cell(ms->fdt, name, "interrupt-parent",
> > irq_mmio_phandle);
> >      if (s->aia_type == VIRT_AIA_TYPE_NONE) {
> > -        qemu_fdt_setprop_cell(ms->fdt, name, "interrupts",
> > UART0_IRQ);
> > +        qemu_fdt_setprop_cell(ms->fdt, name, "interrupts", irqNo);
> >      } else {
> > -        qemu_fdt_setprop_cells(ms->fdt, name, "interrupts",
> > UART0_IRQ, 0x4);
> > +        qemu_fdt_setprop_cells(ms->fdt, name, "interrupts", irqNo,
> > 0x4);
> > +    }
> > +
> > +    if (VIRT_UART0 == memId) {
> > +        qemu_fdt_setprop_string(ms->fdt, "/chosen", "stdout-path",
> > name);
> > +        qemu_fdt_setprop_string(ms->fdt, "/aliases", "serial0",
> > name);
> >      }
> > +}
> >  
> > -    qemu_fdt_setprop_string(ms->fdt, "/chosen", "stdout-path",
> > name);
> > -    qemu_fdt_setprop_string(ms->fdt, "/aliases", "serial0", name);
> > +static void create_fdt_uarts(RISCVVirtState *s, uint32_t
> > irq_mmio_phandle)
> > +{
> > +    if (s->uart1_present) {
> > +        create_fdt_uart(s, irq_mmio_phandle, VIRT_UART1,
> > UART1_IRQ);
> > +    }
> > +    create_fdt_uart(s, irq_mmio_phandle, VIRT_UART0, UART0_IRQ);
> >  }
> >  
> >  static void create_fdt_rtc(RISCVVirtState *s,
> > @@ -1023,7 +1035,7 @@ static void finalize_fdt(RISCVVirtState *s)
> >  
> >      create_fdt_reset(s, &phandle);
> >  
> > -    create_fdt_uart(s, irq_mmio_phandle);
> > +    create_fdt_uarts(s, irq_mmio_phandle);
> >  
> >      create_fdt_rtc(s, irq_mmio_phandle);
> >  }
> > @@ -1567,6 +1579,13 @@ static void virt_machine_init(MachineState
> > *machine)
> >          0, qdev_get_gpio_in(mmio_irqchip, UART0_IRQ), 399193,
> >          serial_hd(0), DEVICE_LITTLE_ENDIAN);
> >  
> > +    if (serial_hd(1)) {
> > +        serial_mm_init(system_memory, s->memmap[VIRT_UART1].base,
> > +            0, qdev_get_gpio_in(mmio_irqchip, UART1_IRQ), 399193,
> > +            serial_hd(1), DEVICE_LITTLE_ENDIAN);
> > +        s->uart1_present = true;
> > +    }
> > +
> >      sysbus_create_simple("goldfish_rtc", s->memmap[VIRT_RTC].base,
> >          qdev_get_gpio_in(mmio_irqchip, RTC_IRQ));
> >  
> > diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> > index 18a2a323a3..7d57c8292e 100644
> > --- a/include/hw/riscv/virt.h
> > +++ b/include/hw/riscv/virt.h
> > @@ -59,6 +59,7 @@ struct RISCVVirtState {
> >      int aia_guests;
> >      char *oem_id;
> >      char *oem_table_id;
> > +    bool uart1_present;
> >      OnOffAuto acpi;
> >      const MemMapEntry *memmap;
> >      struct GPEXHost *gpex_host;
> > @@ -78,6 +79,7 @@ enum {
> >      VIRT_APLIC_S,
> >      VIRT_UART0,
> >      VIRT_VIRTIO,
> > +    VIRT_UART1,
> >      VIRT_FW_CFG,
> >      VIRT_IMSIC_M,
> >      VIRT_IMSIC_S,
> > @@ -93,6 +95,7 @@ enum {
> >  enum {
> >      UART0_IRQ = 10,
> >      RTC_IRQ = 11,
> > +    UART1_IRQ = 12,
> >      VIRTIO_IRQ = 1, /* 1 to 8 */
> >      VIRTIO_COUNT = 8,
> >      PCIE_IRQ = 0x20, /* 32 to 35 */