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(-)
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 <yfliu2008@qq.com>
---
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 },
[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 */
--
2.34.1On 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 <yfliu2008@qq.com>
> ---
> 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 <alistair.francis@wdc.com>
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 */
Hi Alistair,
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 <yfliu2008@qq.com>
> > ---
> > 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.
Actually I used 0x10000100 at the begining, but soon realized it is in
the same 4K page of UART0, thus the hypervisor refuses to assign it to
another VM for isolation purpose. Thus current address was chosen to
be flexible.
Regards,
Yanfeng
>
> Otherwise:
>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>
> 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 */
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 <yfliu2008@qq.com>
> > ---
> > 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 <alistair.francis@wdc.com>
>
> 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 */
Hi,
On 14/7/26 05:40, Yanfeng Liu wrote:
> 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?
(please do not top-post on technical mailing lists)
It would have be clearer if you precised that ...
>
> 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
... here [*] ...
>>>
>>> Signed-off-by: Yanfeng Liu <yfliu2008@qq.com>
>>> ---
>>> docs/system/riscv/virt.rst | 5 ++++-
>>> hw/riscv/virt-acpi-build.c | 12 ++++++++----
>>> hw/riscv/virt.c | 39 ++++++++++++++++++++++++++++------
>>> 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
... and added a hint comment about alignment here.
>>> @@ -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 <alistair.francis@wdc.com>
>>
>> Alistair
>>
>>> [VIRT_FW_CFG] = { 0x10100000, 0x18 },
>>> [VIRT_FLASH] = { 0x20000000, 0x4000000 },
>>> [VIRT_IMSIC_M] = { 0x24000000, VIRT_IMSIC_MAX_SIZE },
Hi,
On 3/7/26 06:24, 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 <yfliu2008@qq.com>
> ---
> 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(-)
> @@ -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)) {
Checking for chardev backend connected to map a peripheral block
is an anti-pattern; the block exists on HW regardless.
> + 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;
Thus we want to always instantiate all UART peripherals.
If you drop the uart1_present field then the patch LGTM.
> + }
> +
> 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;
Philippe,
Thank you!
Actually v1 creates UART1 carelessly, then Peter suggested following
5d8a250f9068 (openrisc) and e7100972f2 (arm) to reduce breakage of
existing guests.
I looked around and noticed the `if (serial_hd())` pattern is still
used in arm/virt, arm/msf2-soc, arm/kzm, xrtensa/sim, and ppc/e500 etc.
Guess I need consensus before moving ahead.
Regards,
Yanfeng
On Fri, 2026-07-03 at 11:31 +0200, Philippe Mathieu-Daudé wrote:
> Hi,
>
> On 3/7/26 06:24, 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 <yfliu2008@qq.com>
> > ---
> > 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(-)
>
>
> > @@ -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)) {
>
> Checking for chardev backend connected to map a peripheral block
> is an anti-pattern; the block exists on HW regardless.
>
> > + 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;
>
> Thus we want to always instantiate all UART peripherals.
>
> If you drop the uart1_present field then the patch LGTM.
>
> > + }
> > +
> > 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;
Resending below as I haven't seen this in mail list.
On Sat, 2026-07-04 at 09:51 +0800, Yanfeng Liu wrote:
>
> Philippe,
>
> Thank you!
>
> Actually v1 creates UART1 carelessly, then Peter suggested following
> 5d8a250f9068 (openrisc) and e7100972f2 (arm) to reduce breakage of
> existing guests.
>
> I looked around and noticed the `if (serial_hd())` pattern is still
> used in arm/virt, arm/msf2-soc, arm/kzm, xrtensa/sim, and ppc/e500
> etc.
>
> Guess I need consensus before moving ahead.
>
> Regards,
> Yanfeng
>
>
> On Fri, 2026-07-03 at 11:31 +0200, Philippe Mathieu-Daudé wrote:
> > Hi,
> >
> > On 3/7/26 06:24, 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 <yfliu2008@qq.com>
> > > ---
> > > 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(-)
> >
> >
> > > @@ -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)) {
> >
> > Checking for chardev backend connected to map a peripheral block
> > is an anti-pattern; the block exists on HW regardless.
> >
> > > + 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;
> >
> > Thus we want to always instantiate all UART peripherals.
> >
> > If you drop the uart1_present field then the patch LGTM.
> >
> > > + }
> > > +
> > > 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;
Philippe,
The v1 always creates UART1 carelessly, then Peter suggested following
5d8a250f9068 (openrisc) and e7100972f2 (arm) to reduce guest breakage.
I looked around and noticed the pattern is used in arm/virt, arm/msf2-
soc, arm/kzm, xrtensa/sim, and ppc/e500 etc.
Please give me clear direction here.
Regards,
Yanfeng
On Fri, 2026-07-03 at 11:31 +0200, Philippe Mathieu-Daudé wrote:
> Hi,
>
> On 3/7/26 06:24, 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 <yfliu2008@qq.com>
> > ---
> > 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(-)
>
>
> > @@ -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)) {
>
> Checking for chardev backend connected to map a peripheral block
> is an anti-pattern; the block exists on HW regardless.
>
> > + 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;
>
> Thus we want to always instantiate all UART peripherals.
>
> If you drop the uart1_present field then the patch LGTM.
>
> > + }
> > +
> > 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;
On 4/7/26 03:41, Yanfeng Liu wrote:
>
> Philippe,
>
> The v1 always creates UART1 carelessly, then Peter suggested following
> 5d8a250f9068 (openrisc) and e7100972f2 (arm) to reduce guest breakage.
Oh I missed that. I'm confused because I thought not checking for
backend use of a chardev (in particular calling serial_hd) was part
of Peter's recommendations, since he did that cleanup years ago. I'll
let him finish his review of your series then; simply ignore my
comments.
Regards,
Phil.
>
> I looked around and noticed the pattern is used in arm/virt, arm/msf2-
> soc, arm/kzm, xrtensa/sim, and ppc/e500 etc.
>
> Please give me clear direction here.
>
> Regards,
> Yanfeng
>
>
> On Fri, 2026-07-03 at 11:31 +0200, Philippe Mathieu-Daudé wrote:
>> Hi,
>>
>> On 3/7/26 06:24, 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 <yfliu2008@qq.com>
>>> ---
>>> 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(-)
>>
>>
>>> @@ -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)) {
>>
>> Checking for chardev backend connected to map a peripheral block
>> is an anti-pattern; the block exists on HW regardless.
>>
>>> + 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;
>>
>> Thus we want to always instantiate all UART peripherals.
>>
>> If you drop the uart1_present field then the patch LGTM.
>>
>>> + }
>>> +
>>> 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;
>
>
On Sun, 5 Jul 2026 at 23:05, Philippe Mathieu-Daudé <philmd@oss.qualcomm.com> wrote: > > On 4/7/26 03:41, Yanfeng Liu wrote: > > > > Philippe, > > > > The v1 always creates UART1 carelessly, then Peter suggested following > > 5d8a250f9068 (openrisc) and e7100972f2 (arm) to reduce guest breakage. > > Oh I missed that. I'm confused because I thought not checking for > backend use of a chardev (in particular calling serial_hd) was part > of Peter's recommendations, since he did that cleanup years ago. Where we're modelling a real piece of hardware that always has, say, two UARTs, we want to always create both UARTs (because they're always there in the real hardware, whether there's a serial cable plugged into them or not). Having the chardev code handle "backend is NULL" as equivalent to "this is the do-nothing backend" lets us avoid unnecessary conditionals in the board code. For "virtual" board types where it's QEMU defining what they are, the situation is a little different. Ideally we'd do the simple thing, i.e. "just create all the devices", but where there's existing guest software that makes assumptions about how older versions of QEMU used to behave and the number of users of the board type is big enough that we care about not breaking that, we sometimes need to do something else. The "only create the second UART if the user puts another -serial option on the command line" was the best idea I could come up with for letting the user opt into the extra functionality if they needed it. (If we have another future architecture that creates a "virt" board type we should make sure we have it create at least two UARTs right from the beginning, because we've now made this mistake twice :-)) -- PMM
On 14/7/26 10:52, Peter Maydell wrote: > On Sun, 5 Jul 2026 at 23:05, Philippe Mathieu-Daudé > <philmd@oss.qualcomm.com> wrote: >> >> On 4/7/26 03:41, Yanfeng Liu wrote: >>> >>> Philippe, >>> >>> The v1 always creates UART1 carelessly, then Peter suggested following >>> 5d8a250f9068 (openrisc) and e7100972f2 (arm) to reduce guest breakage. >> >> Oh I missed that. I'm confused because I thought not checking for >> backend use of a chardev (in particular calling serial_hd) was part >> of Peter's recommendations, since he did that cleanup years ago. > > Where we're modelling a real piece of hardware that always has, > say, two UARTs, we want to always create both UARTs (because they're > always there in the real hardware, whether there's a serial cable > plugged into them or not). Having the chardev code handle "backend > is NULL" as equivalent to "this is the do-nothing backend" lets us > avoid unnecessary conditionals in the board code. > > For "virtual" board types where it's QEMU defining what they are, > the situation is a little different. Ideally we'd do the simple > thing, i.e. "just create all the devices", but where there's > existing guest software that makes assumptions about how older > versions of QEMU used to behave and the number of users of the > board type is big enough that we care about not breaking that, > we sometimes need to do something else. The "only create the > second UART if the user puts another -serial option on the > command line" was the best idea I could come up with for letting > the user opt into the extra functionality if they needed it. > > (If we have another future architecture that creates a "virt" > board type we should make sure we have it create at least two > UARTs right from the beginning, because we've now made this > mistake twice :-)) Thanks for the clarification, I missed this is a built up virt board where QEMU itself announces the hw available, not some real world model. Regards, Phil.
© 2016 - 2026 Red Hat, Inc.