Re: [PATCH v3 03/47] hw/arm/xlnx-versal: uart: refactor creation

["Edgar E. Iglesias" <edgar.iglesias@amd.com>]

On Thu, Aug 21, 2025 at 03:03:02PM +0200, Luc Michel wrote:
> Refactor the UARTs creations. The VersalMap struct is now used to
> describe the SoC and its peripherals. For now it contains the two UARTs
> mapping information. The creation function now embeds the FDT creation
> logic as well. The devices are now created dynamically using qdev_new
> and (qdev|sysbus)_realize_and_unref.
> 
> This will allow to rely entirely on the VersalMap structure to create
> the SoC and allow easy addition of new SoCs of the same family (like
> versal2 coming with next commits).
> 
> Note that the connection to the CRL is removed for now and will be
> re-added by next commits.


Hi Luc,

I see the following on this patch:
Starting program: /home/edgar/src/c/qemu/patchew/build/qemu-system-aarch64 -M xlnx-versal-virt -m 2g -serial stdio -display none -net nic,model=cadence_gem,netdev=n0 -netdev user,id=n0 -kernel Image -device virtio-blk-device,drive=d0 -drive format=qcow2,if=none,file=rootfs.qcow2,id=d0 -append root=/dev/vda1\ console=ttyAMA0
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
[New Thread 0x7ffff5a4c640 (LWP 130095)]
[New Thread 0x7ffff4889640 (LWP 130096)]

Thread 1 "qemu-system-aar" received signal SIGSEGV, Segmentation fault.
arm_cpu_mp_affinity (cpu=cpu@entry=0x0) at ../qemu/target/arm/cpu.c:1446
1446        return cpu->mp_affinity;
(gdb) bt
#0  arm_cpu_mp_affinity (cpu=cpu@entry=0x0) at ../qemu/target/arm/cpu.c:1446
#1  0x0000555555d8517d in fdt_add_cpu_nodes (psci_conduit=1, s=0x7ffff4a7b010)
    at ../qemu/hw/arm/xlnx-versal-virt.c:114
#2  versal_virt_init (machine=0x7ffff4a7b010) at ../qemu/hw/arm/xlnx-versal-virt.c:682
#3  0x00005555559b766d in machine_run_board_init
    (machine=<optimized out>, mem_path=<optimized out>, errp=<optimized out>,
    errp@entry=0x555557602858 <error_fatal>) at ../qemu/hw/core/machine.c:1694
#4  0x0000555555bfda6c in qemu_init_board () at ../qemu/system/vl.c:2710
#5  qmp_x_exit_preconfig (errp=0x555557602858 <error_fatal>) at ../qemu/system/vl.c:2804
#6  0x0000555555c013de in qemu_init (argc=<optimized out>, argv=<optimized out>) at ../qemu/system/vl.c:3840
#7  0x000055555590253d in main (argc=<optimized out>, argv=<optimized out>) at ../qemu/system/main.c:71
(gdb)



> 
> Signed-off-by: Luc Michel <luc.michel@amd.com>
> Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>
> ---
>  include/hw/arm/xlnx-versal.h |   2 -
>  hw/arm/xlnx-versal-virt.c    |  36 +--------
>  hw/arm/xlnx-versal.c         | 142 ++++++++++++++++++++++++++++-------
>  3 files changed, 117 insertions(+), 63 deletions(-)
> 
> diff --git a/include/hw/arm/xlnx-versal.h b/include/hw/arm/xlnx-versal.h
> index f2a62b43552..b01ddeb1423 100644
> --- a/include/hw/arm/xlnx-versal.h
> +++ b/include/hw/arm/xlnx-versal.h
> @@ -16,11 +16,10 @@
>  #include "hw/sysbus.h"
>  #include "hw/cpu/cluster.h"
>  #include "hw/or-irq.h"
>  #include "hw/sd/sdhci.h"
>  #include "hw/intc/arm_gicv3.h"
> -#include "hw/char/pl011.h"
>  #include "hw/dma/xlnx-zdma.h"
>  #include "hw/net/cadence_gem.h"
>  #include "hw/rtc/xlnx-zynqmp-rtc.h"
>  #include "qom/object.h"
>  #include "hw/usb/xlnx-usb-subsystem.h"
> @@ -78,11 +77,10 @@ struct Versal {
>  
>      struct {
>          MemoryRegion mr_ocm;
>  
>          struct {
> -            PL011State uart[XLNX_VERSAL_NR_UARTS];
>              CadenceGEMState gem[XLNX_VERSAL_NR_GEMS];
>              OrIRQState gem_irq_orgate[XLNX_VERSAL_NR_GEMS];
>              XlnxZDMA adma[XLNX_VERSAL_NR_ADMAS];
>              VersalUsb2 usb;
>              CanBusState *canbus[XLNX_VERSAL_NR_CANFD];
> diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
> index 69f3bb401b9..06cc5bae8b0 100644
> --- a/hw/arm/xlnx-versal-virt.c
> +++ b/hw/arm/xlnx-versal-virt.c
> @@ -75,10 +75,11 @@ static void fdt_create(VersalVirt *s)
>  
>      s->phandle.usb = qemu_fdt_alloc_phandle(s->fdt);
>      s->phandle.dwc = qemu_fdt_alloc_phandle(s->fdt);
>      /* Create /chosen node for load_dtb.  */
>      qemu_fdt_add_subnode(s->fdt, "/chosen");
> +    qemu_fdt_add_subnode(s->fdt, "/aliases");
>  
>      /* Header */
>      qemu_fdt_setprop_cell(s->fdt, "/", "interrupt-parent", s->phandle.gic);
>      qemu_fdt_setprop_cell(s->fdt, "/", "#size-cells", 0x2);
>      qemu_fdt_setprop_cell(s->fdt, "/", "#address-cells", 0x2);
> @@ -206,44 +207,10 @@ static void fdt_add_usb_xhci_nodes(VersalVirt *s)
>      qemu_fdt_setprop_cell(s->fdt, name, "phandle", s->phandle.dwc);
>      qemu_fdt_setprop_string(s->fdt, name, "maximum-speed", "high-speed");
>      g_free(name);
>  }
>  
> -static void fdt_add_uart_nodes(VersalVirt *s)
> -{
> -    uint64_t addrs[] = { MM_UART1, MM_UART0 };
> -    unsigned int irqs[] = { VERSAL_UART1_IRQ_0, VERSAL_UART0_IRQ_0 };
> -    const char compat[] = "arm,pl011\0arm,sbsa-uart";
> -    const char clocknames[] = "uartclk\0apb_pclk";
> -    int i;
> -
> -    for (i = 0; i < ARRAY_SIZE(addrs); i++) {
> -        char *name = g_strdup_printf("/uart@%" PRIx64, addrs[i]);
> -        qemu_fdt_add_subnode(s->fdt, name);
> -        qemu_fdt_setprop_cell(s->fdt, name, "current-speed", 115200);
> -        qemu_fdt_setprop_cells(s->fdt, name, "clocks",
> -                               s->phandle.clk_125Mhz, s->phandle.clk_125Mhz);
> -        qemu_fdt_setprop(s->fdt, name, "clock-names",
> -                         clocknames, sizeof(clocknames));
> -
> -        qemu_fdt_setprop_cells(s->fdt, name, "interrupts",
> -                               GIC_FDT_IRQ_TYPE_SPI, irqs[i],
> -                               GIC_FDT_IRQ_FLAGS_LEVEL_HI);
> -        qemu_fdt_setprop_sized_cells(s->fdt, name, "reg",
> -                                     2, addrs[i], 2, 0x1000);
> -        qemu_fdt_setprop(s->fdt, name, "compatible",
> -                         compat, sizeof(compat));
> -        qemu_fdt_setprop(s->fdt, name, "u-boot,dm-pre-reloc", NULL, 0);
> -
> -        if (addrs[i] == MM_UART0) {
> -            /* Select UART0.  */
> -            qemu_fdt_setprop_string(s->fdt, "/chosen", "stdout-path", name);
> -        }
> -        g_free(name);
> -    }
> -}
> -
>  static void fdt_add_canfd_nodes(VersalVirt *s)
>  {
>      uint64_t addrs[] = { MM_CANFD1, MM_CANFD0 };
>      uint32_t size[] = { MM_CANFD1_SIZE, MM_CANFD0_SIZE };
>      unsigned int irqs[] = { VERSAL_CANFD1_IRQ_0, VERSAL_CANFD0_IRQ_0 };
> @@ -700,11 +667,10 @@ static void versal_virt_init(MachineState *machine)
>                               &error_abort);
>  
>      fdt_create(s);
>      versal_set_fdt(&s->soc, s->fdt);
>      fdt_add_gem_nodes(s);
> -    fdt_add_uart_nodes(s);
>      fdt_add_canfd_nodes(s);
>      fdt_add_gic_nodes(s);
>      fdt_add_timer_nodes(s);
>      fdt_add_zdma_nodes(s);
>      fdt_add_usb_xhci_nodes(s);
> diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
> index 7bb55751e5c..87468cbc291 100644
> --- a/hw/arm/xlnx-versal.c
> +++ b/hw/arm/xlnx-versal.c
> @@ -24,18 +24,96 @@
>  #include "qemu/log.h"
>  #include "target/arm/cpu-qom.h"
>  #include "target/arm/gtimer.h"
>  #include "system/device_tree.h"
>  #include "hw/arm/fdt.h"
> +#include "hw/char/pl011.h"
>  
>  #define XLNX_VERSAL_ACPU_TYPE ARM_CPU_TYPE_NAME("cortex-a72")
>  #define XLNX_VERSAL_RCPU_TYPE ARM_CPU_TYPE_NAME("cortex-r5f")
>  #define GEM_REVISION        0x40070106
>  
>  #define VERSAL_NUM_PMC_APB_IRQS 18
>  #define NUM_OSPI_IRQ_LINES 3
>  
> +typedef struct VersalSimplePeriphMap {
> +    uint64_t addr;
> +    int irq;
> +} VersalSimplePeriphMap;
> +
> +typedef struct VersalMap {
> +    VersalSimplePeriphMap uart[2];
> +    size_t num_uart;
> +} VersalMap;
> +
> +static const VersalMap VERSAL_MAP = {
> +    .uart[0] = { 0xff000000, 18 },
> +    .uart[1] = { 0xff010000, 19 },
> +    .num_uart = 2,
> +};
> +
> +static const VersalMap *VERSION_TO_MAP[] = {
> +    [VERSAL_VER_VERSAL] = &VERSAL_MAP,
> +};
> +
> +static inline VersalVersion versal_get_version(Versal *s)
> +{
> +    return XLNX_VERSAL_BASE_GET_CLASS(s)->version;
> +}
> +
> +static inline const VersalMap *versal_get_map(Versal *s)
> +{
> +    return VERSION_TO_MAP[versal_get_version(s)];
> +}
> +
> +
> +static qemu_irq versal_get_irq(Versal *s, int irq_idx)
> +{
> +    return qdev_get_gpio_in(DEVICE(&s->fpd.apu.gic), irq_idx);
> +}
> +
> +static void versal_sysbus_connect_irq(Versal *s, SysBusDevice *sbd,
> +                                      int sbd_idx, int irq_idx)
> +{
> +    qemu_irq irq = versal_get_irq(s, irq_idx);
> +
> +    if (irq == NULL) {
> +        return;
> +    }
> +
> +    sysbus_connect_irq(sbd, sbd_idx, irq);
> +}
> +
> +static inline char *versal_fdt_add_subnode(Versal *s, const char *path,
> +                                           uint64_t at, const char *compat,
> +                                           size_t compat_sz)
> +{
> +    char *p;
> +
> +    p = g_strdup_printf("%s@%" PRIx64, path, at);
> +    qemu_fdt_add_subnode(s->cfg.fdt, p);
> +
> +    if (!strncmp(compat, "memory", compat_sz)) {
> +        qemu_fdt_setprop(s->cfg.fdt, p, "device_type", compat, compat_sz);
> +    } else {
> +        qemu_fdt_setprop(s->cfg.fdt, p, "compatible", compat, compat_sz);
> +    }
> +
> +    return p;
> +}
> +
> +static inline char *versal_fdt_add_simple_subnode(Versal *s, const char *path,
> +                                                  uint64_t addr, uint64_t len,
> +                                                  const char *compat,
> +                                                  size_t compat_sz)
> +{
> +    char *p = versal_fdt_add_subnode(s, path, addr, compat, compat_sz);
> +
> +    qemu_fdt_setprop_sized_cells(s->cfg.fdt, p, "reg", 2, addr, 2, len);
> +    return p;
> +}
> +
>  static void versal_create_apu_cpus(Versal *s)
>  {
>      int i;
>  
>      object_initialize_child(OBJECT(s), "apu-cluster", &s->fpd.apu.cluster,
> @@ -165,32 +243,48 @@ static void versal_create_rpu_cpus(Versal *s)
>      }
>  
>      qdev_realize(DEVICE(&s->lpd.rpu.cluster), NULL, &error_fatal);
>  }
>  
> -static void versal_create_uarts(Versal *s, qemu_irq *pic)
> +static void versal_create_uart(Versal *s,
> +                               const VersalSimplePeriphMap *map,
> +                               int chardev_idx)
>  {
> -    int i;
> +    DeviceState *dev;
> +    MemoryRegion *mr;
> +    g_autofree char *node;
> +    g_autofree char *alias;
> +    const char compatible[] = "arm,pl011\0arm,sbsa-uart";
> +    const char clocknames[] = "uartclk\0apb_pclk";
>  
> -    for (i = 0; i < ARRAY_SIZE(s->lpd.iou.uart); i++) {
> -        static const int irqs[] = { VERSAL_UART0_IRQ_0, VERSAL_UART1_IRQ_0};
> -        static const uint64_t addrs[] = { MM_UART0, MM_UART1 };
> -        char *name = g_strdup_printf("uart%d", i);
> -        DeviceState *dev;
> -        MemoryRegion *mr;
> +    dev = qdev_new(TYPE_PL011);
> +    object_property_add_child(OBJECT(s), "uart[*]", OBJECT(dev));
> +    qdev_prop_set_chr(dev, "chardev", serial_hd(chardev_idx));
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>  
> -        object_initialize_child(OBJECT(s), name, &s->lpd.iou.uart[i],
> -                                TYPE_PL011);
> -        dev = DEVICE(&s->lpd.iou.uart[i]);
> -        qdev_prop_set_chr(dev, "chardev", serial_hd(i));
> -        sysbus_realize(SYS_BUS_DEVICE(dev), &error_fatal);
> +    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
> +    memory_region_add_subregion(&s->mr_ps, map->addr, mr);
>  
> -        mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
> -        memory_region_add_subregion(&s->mr_ps, addrs[i], mr);
> +    versal_sysbus_connect_irq(s, SYS_BUS_DEVICE(dev), 0, map->irq);
>  
> -        sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[irqs[i]]);
> -        g_free(name);
> +    node = versal_fdt_add_simple_subnode(s, "/uart", map->addr, 0x1000,
> +                                         compatible, sizeof(compatible));
> +    qemu_fdt_setprop_cell(s->cfg.fdt, node, "current-speed", 115200);
> +    qemu_fdt_setprop_cells(s->cfg.fdt, node, "clocks",
> +                           s->phandle.clk_125mhz, s->phandle.clk_125mhz);
> +    qemu_fdt_setprop(s->cfg.fdt, node, "clock-names", clocknames,
> +                     sizeof(clocknames));
> +    qemu_fdt_setprop_cells(s->cfg.fdt, node, "interrupts",
> +                           GIC_FDT_IRQ_TYPE_SPI, map->irq,
> +                           GIC_FDT_IRQ_FLAGS_LEVEL_HI);
> +    qemu_fdt_setprop(s->cfg.fdt, node, "u-boot,dm-pre-reloc", NULL, 0);
> +
> +    alias = g_strdup_printf("serial%d", chardev_idx);
> +    qemu_fdt_setprop_string(s->cfg.fdt, "/aliases", alias, node);
> +
> +    if (chardev_idx == 0) {
> +        qemu_fdt_setprop_string(s->cfg.fdt, "/chosen", "stdout-path", node);
>      }
>  }
>  
>  static void versal_create_canfds(Versal *s, qemu_irq *pic)
>  {
> @@ -781,18 +875,10 @@ static void versal_create_crl(Versal *s, qemu_irq *pic)
>          object_property_set_link(OBJECT(&s->lpd.crl),
>                                   name, OBJECT(&s->lpd.iou.adma[i]),
>                                   &error_abort);
>      }
>  
> -    for (i = 0; i < ARRAY_SIZE(s->lpd.iou.uart); i++) {
> -        g_autofree gchar *name = g_strdup_printf("uart[%d]", i);
> -
> -        object_property_set_link(OBJECT(&s->lpd.crl),
> -                                 name, OBJECT(&s->lpd.iou.uart[i]),
> -                                 &error_abort);
> -    }
> -
>      object_property_set_link(OBJECT(&s->lpd.crl),
>                               "usb", OBJECT(&s->lpd.iou.usb),
>                               &error_abort);
>  
>      sysbus_realize(sbd, &error_fatal);
> @@ -955,11 +1041,15 @@ static void versal_realize(DeviceState *dev, Error **errp)
>      s->phandle.clk_125mhz = fdt_add_clk_node(s, "/clk125", 125 * 1000 * 1000);
>  
>      versal_create_apu_cpus(s);
>      versal_create_apu_gic(s, pic);
>      versal_create_rpu_cpus(s);
> -    versal_create_uarts(s, pic);
> +
> +    for (i = 0; i < map->num_uart; i++) {
> +        versal_create_uart(s, &map->uart[i], i);
> +    }
> +
>      versal_create_canfds(s, pic);
>      versal_create_usbs(s, pic);
>      versal_create_gems(s, pic);
>      versal_create_admas(s, pic);
>      versal_create_sds(s, pic);
> -- 
> 2.50.1
>