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

[Sebastian Huber <sebastian.huber@embedded-brains.de>]

Hello Peter,

thanks for the review.

On 20.05.24 15:58, Peter Maydell wrote:
> 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.

I used the hw/arm/realview.c as a template for this change. I will try 
to implement the suggested changes.

> 
>>       /* 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.

Ok.

> 
>> +        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.

Yes, indeed. I originally used a Qemu version from Xilinx. They have a 
huge set of patches which is not integrated in Qemu.

> 
>> +    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
> orhttps://www.qemu.org/docs/master/system/arm/realview.html
> is an example of a more minimal board documentation page.

Ok, I add this to my TODO list or try to delegate this, but it will take 
some time.

-- 
embedded brains GmbH & Co. KG
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/