Re: [PATCH 2/2] hw/arm/xilinx_zynq: Support up to two CPU cores

[Peter Maydell <peter.maydell@linaro.org>]

On Tue, 7 May 2024 at 14:04, Sebastian Huber
<sebastian.huber@embedded-brains.de> wrote:
>
> The Zynq 7000 SoCs contain two Arm Cortex-A9 MPCore (the Zynq 7000S have only
> one core).  Add support for up to two simulated cores.
>
> Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
> ---
>  hw/arm/xilinx_zynq.c | 42 +++++++++++++++++++++++++++---------------
>  1 file changed, 27 insertions(+), 15 deletions(-)
>
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 078abd77bd..3b858e3e9a 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -184,6 +184,8 @@ static void zynq_init(MachineState *machine)
>      SysBusDevice *busdev;
>      qemu_irq pic[64];
>      int n;
> +    unsigned int smp_cpus = machine->smp.cpus;
> +    qemu_irq cpu_irq[2];

We prefer not to have arrays of qemu_irq like this that are
just passing qemu_irqs from one place to another. Instead
at the point where you want the ARM_CPU_IRQ of a particular
CPU, call qdev_get_gpio_in() on the CPU object there.

I suggest dropping the "ARMCPU *cpu" local from this function
and instead adding an "ARMCPU *cpu[ZYNQ_MAX_CPUS]" array to
the ZynqMachineState struct.

>      /* max 2GB ram */
>      if (machine->ram_size > 2 * GiB) {
> @@ -191,21 +193,27 @@ static void zynq_init(MachineState *machine)
>          exit(EXIT_FAILURE);
>      }
>
> -    cpu = ARM_CPU(object_new(machine->cpu_type));
> +    for (n = 0; n < smp_cpus; n++) {
> +        Object *cpuobj = object_new(machine->cpu_type);
>
> -    /* By default A9 CPUs have EL3 enabled.  This board does not
> -     * currently support EL3 so the CPU EL3 property is disabled before
> -     * realization.
> -     */
> -    if (object_property_find(OBJECT(cpu), "has_el3")) {
> -        object_property_set_bool(OBJECT(cpu), "has_el3", false, &error_fatal);
> -    }
> +        /* By default A9 CPUs have EL3 enabled.  This board does not
> +         * currently support EL3 so the CPU EL3 property is disabled before
> +         * realization.
> +         */

If you're moving comment text around checkpatch will suggest that
you fix it up to our current coding standard, which is that
a multiline comment has the "/*" on a line of its own.

> +        if (object_property_find(cpuobj, "has_el3")) {
> +            object_property_set_bool(cpuobj, "has_el3", false, &error_fatal);
> +        }
> +
> +        object_property_set_int(cpuobj, "midr", ZYNQ_BOARD_MIDR,
> +                                &error_fatal);
> +        object_property_set_int(cpuobj, "reset-cbar", MPCORE_PERIPHBASE,
> +                                &error_fatal);
>
> -    object_property_set_int(OBJECT(cpu), "midr", ZYNQ_BOARD_MIDR,
> -                            &error_fatal);
> -    object_property_set_int(OBJECT(cpu), "reset-cbar", MPCORE_PERIPHBASE,
> -                            &error_fatal);
> -    qdev_realize(DEVICE(cpu), NULL, &error_fatal);
> +        qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
> +
> +        cpu_irq[n] = qdev_get_gpio_in(DEVICE(cpuobj), ARM_CPU_IRQ);
> +    }
> +    cpu = ARM_CPU(first_cpu);
>
>      /* DDR remapped to address zero.  */
>      memory_region_add_subregion(address_space_mem, 0, machine->ram);
> @@ -238,10 +246,14 @@ static void zynq_init(MachineState *machine)
>      sysbus_mmio_map(SYS_BUS_DEVICE(slcr), 0, 0xF8000000);
>
>      dev = qdev_new(TYPE_A9MPCORE_PRIV);
> -    qdev_prop_set_uint32(dev, "num-cpu", 1);
> +    qdev_prop_set_uint32(dev, "num-cpu", smp_cpus);
>      busdev = SYS_BUS_DEVICE(dev);
>      sysbus_realize_and_unref(busdev, &error_fatal);
>      sysbus_mmio_map(busdev, 0, MPCORE_PERIPHBASE);
> +    for (n = 0; n < smp_cpus; n++) {
> +        sysbus_connect_irq(busdev, n, cpu_irq[n]);
> +    }

Looks like you have based this on a version of QEMU which doesn't
have commit 68a5827b80117973 which wires up the FIQ line of the
A9MPCORE_PRIV device to the CPUs.

> +    zynq_binfo.gic_cpu_if_addr = MPCORE_PERIPHBASE + 0x100;
>      sysbus_create_varargs("l2x0", MPCORE_PERIPHBASE + 0x2000, NULL);
>      sysbus_connect_irq(busdev, 0,
>                         qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ));
> @@ -357,7 +369,7 @@ static void zynq_machine_class_init(ObjectClass *oc, void *data)
>      MachineClass *mc = MACHINE_CLASS(oc);
>      mc->desc = "Xilinx Zynq Platform Baseboard for Cortex-A9";
>      mc->init = zynq_init;
> -    mc->max_cpus = 1;
> +    mc->max_cpus = 2;
>      mc->no_sdcard = 1;
>      mc->ignore_memory_transaction_failures = true;
>      mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a9");
> --

I'm not making this a condition for accepting this patch, but
since you're working on this board model would you consider
writing up some documentation for it? It's one of the boards
we do not currently have documented at all. This doesn't have to
be very extensive: a few paragraphs describing what the board
type is, maybe linking to a reference to the hardware, listing
what is and isn't implemented, and (if there are some convenient
examples available) perhaps listing some examples of use.
docs/system/arm/xlnx-versal-virt.rst is the docs for the
Xilinx Versal board; the rendered version of that is:
https://www.qemu.org/docs/master/system/arm/xlnx-versal-virt.html
or https://www.qemu.org/docs/master/system/arm/realview.html
is an example of a more minimal board documentation page.

thanks
-- PMM