[PATCH 0/4] xlnx-zynqmp: add support to boot on RPUs

[PATCH 0/4] xlnx-zynqmp: add support to boot on RPUs

[Clément Chigot]

This series enhances Xilinx ZynqMP support to allow booting on RPUs and
ease gdb connections to them.

It was validated with home-made binaries. FreeRTOS was tested but without
success: outputs/IRQ seems broken. AFAICT, FreeRTOS is expecting Xilinx's
QEMU thus I didn't investigate further, but I'd still like advice
on the 3rd patch ("wire a second GIC") since it could be related to it.

Clément Chigot (2):
  hw/arm: make cpu targeted by arm_load_kernel the primary CPU.
  hw/arm/xlnx-zynqmp: adapt cluster-id based on the boot cpu

Frederic Konrad (2):
  hw/intc/arm_gic: introduce a first-cpu-index property
  hw/arm/xlnx-zynqmp: wire a second GIC for the Cortex-R5

 hw/arm/boot.c                    |  15 +++--
 hw/arm/xlnx-zynqmp.c             | 104 ++++++++++++++++++++++++++++---
 hw/intc/arm_gic.c                |   2 +-
 hw/intc/arm_gic_common.c         |   1 +
 include/hw/arm/boot.h            |   3 +
 include/hw/arm/xlnx-zynqmp.h     |   6 ++
 include/hw/intc/arm_gic_common.h |   2 +
 7 files changed, 115 insertions(+), 18 deletions(-)

-- 
2.34.1

[PATCH 1/4] hw/arm: make cpu targeted by arm_load_kernel the primary CPU.

[Clément Chigot]

Currently, arm booting processus assumes that the first_cpu is the CPU
that will boot: `arm_load_kernel` is powering off all but the `first_cpu`;
`do_cpu_reset` is setting the loader address only for this `first_cpu`.

For most of the boards, this isn't an issue as the kernel is loaded and
booted on the first CPU anyway. However, for zynqmp, the option
"boot-cpu" allows to choose any CPUs.

Create a new arm_boot_info entry `primary_cpu` recording which CPU will
be boot first. This one is set when `arm_boot_kernel` is called.

Signed-off-by: Clément Chigot <chigot@adacore.com>
---
 hw/arm/boot.c         | 15 +++++++--------
 include/hw/arm/boot.h |  3 +++
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index f94b940bc3..8da4c67fa9 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -743,7 +743,7 @@ static void do_cpu_reset(void *opaque)
             } else {
                 if (arm_feature(env, ARM_FEATURE_EL3) &&
                     (info->secure_boot ||
-                     (info->secure_board_setup && cs == first_cpu))) {
+                     (info->secure_board_setup && cpu == info->primary_cpu))) {
                     /* Start this CPU in Secure SVC */
                     target_el = 3;
                 }
@@ -751,7 +751,7 @@ static void do_cpu_reset(void *opaque)
 
             arm_emulate_firmware_reset(cs, target_el);
 
-            if (cs == first_cpu) {
+            if (cpu == info->primary_cpu) {
                 AddressSpace *as = arm_boot_address_space(cpu, info);
 
                 cpu_set_pc(cs, info->loader_start);
@@ -1238,6 +1238,9 @@ void arm_load_kernel(ARMCPU *cpu, MachineState *ms, struct arm_boot_info *info)
     info->dtb_filename = ms->dtb;
     info->dtb_limit = 0;
 
+    /* We assume the CPU passed as argument is the primary CPU.  */
+    info->primary_cpu = cpu;
+
     /* Load the kernel.  */
     if (!info->kernel_filename || info->firmware_loaded) {
         arm_setup_firmware_boot(cpu, info);
@@ -1287,12 +1290,8 @@ void arm_load_kernel(ARMCPU *cpu, MachineState *ms, struct arm_boot_info *info)
 
             object_property_set_int(cpuobj, "psci-conduit", info->psci_conduit,
                                     &error_abort);
-            /*
-             * Secondary CPUs start in PSCI powered-down state. Like the
-             * code in do_cpu_reset(), we assume first_cpu is the primary
-             * CPU.
-             */
-            if (cs != first_cpu) {
+            /* Secondary CPUs start in PSCI powered-down state.  */
+            if (ARM_CPU(cs) != info->primary_cpu) {
                 object_property_set_bool(cpuobj, "start-powered-off", true,
                                          &error_abort);
             }
diff --git a/include/hw/arm/boot.h b/include/hw/arm/boot.h
index b12bf61ca8..a2e22bda8a 100644
--- a/include/hw/arm/boot.h
+++ b/include/hw/arm/boot.h
@@ -132,6 +132,9 @@ struct arm_boot_info {
     bool secure_board_setup;
 
     arm_endianness endianness;
+
+    /* CPU having load the kernel and that should be the first to boot.  */
+    ARMCPU *primary_cpu;
 };
 
 /**
-- 
2.34.1

Re: [PATCH 1/4] hw/arm: make cpu targeted by arm_load_kernel the primary CPU.

[Peter Maydell]

On Tue, 13 May 2025 at 15:15, Clément Chigot <chigot@adacore.com> wrote:
>
> Currently, arm booting processus assumes that the first_cpu is the CPU

"the arm booting process"

> that will boot: `arm_load_kernel` is powering off all but the `first_cpu`;
> `do_cpu_reset` is setting the loader address only for this `first_cpu`.
>
> For most of the boards, this isn't an issue as the kernel is loaded and
> booted on the first CPU anyway. However, for zynqmp, the option
> "boot-cpu" allows to choose any CPUs.
>
> Create a new arm_boot_info entry `primary_cpu` recording which CPU will
> be boot first. This one is set when `arm_boot_kernel` is called.
>
> Signed-off-by: Clément Chigot <chigot@adacore.com>

Yeah, this makes sense, and every board currently passes
in the first CPU object so this isn't a behaviour change.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 2/4] hw/intc/arm_gic: introduce a first-cpu-index property

[Clément Chigot]

From: Frederic Konrad <konrad.frederic@yahoo.fr>

This introduces a first-cpu-index property to the arm-gic, as some SOCs
could have two separate GIC (ie: the zynqmp).

Signed-off-by: Clément Chigot <chigot@adacore.com>
---
 hw/intc/arm_gic.c                | 2 +-
 hw/intc/arm_gic_common.c         | 1 +
 include/hw/intc/arm_gic_common.h | 2 ++
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index d18bef40fc..899f133363 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -59,7 +59,7 @@ static const uint8_t gic_id_gicv2[] = {
 static inline int gic_get_current_cpu(GICState *s)
 {
     if (!qtest_enabled() && s->num_cpu > 1) {
-        return current_cpu->cpu_index;
+        return current_cpu->cpu_index - s->first_cpu_index;
     }
     return 0;
 }
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index 0f0c48d89a..ed5be05645 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -350,6 +350,7 @@ static void arm_gic_common_linux_init(ARMLinuxBootIf *obj,
 
 static const Property arm_gic_common_properties[] = {
     DEFINE_PROP_UINT32("num-cpu", GICState, num_cpu, 1),
+    DEFINE_PROP_UINT32("first-cpu-index", GICState, first_cpu_index, 0),
     DEFINE_PROP_UINT32("num-irq", GICState, num_irq, 32),
     /* Revision can be 1 or 2 for GIC architecture specification
      * versions 1 or 2, or 0 to indicate the legacy 11MPCore GIC.
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index 97fea4102d..93a3cc2bf8 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -129,6 +129,8 @@ struct GICState {
     uint32_t num_lrs;
 
     uint32_t num_cpu;
+    /* cpu_index of the first CPU, attached to this GIC.  */
+    uint32_t first_cpu_index;
 
     MemoryRegion iomem; /* Distributor */
     /* This is just so we can have an opaque pointer which identifies
-- 
2.34.1

Re: [PATCH 2/4] hw/intc/arm_gic: introduce a first-cpu-index property

[Philippe Mathieu-Daudé]

On 13/5/25 16:14, Clément Chigot wrote:
> From: Frederic Konrad <konrad.frederic@yahoo.fr>
> 
> This introduces a first-cpu-index property to the arm-gic, as some SOCs
> could have two separate GIC (ie: the zynqmp).
> 
> Signed-off-by: Clément Chigot <chigot@adacore.com>
> ---
>   hw/intc/arm_gic.c                | 2 +-
>   hw/intc/arm_gic_common.c         | 1 +
>   include/hw/intc/arm_gic_common.h | 2 ++
>   3 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index d18bef40fc..899f133363 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -59,7 +59,7 @@ static const uint8_t gic_id_gicv2[] = {
>   static inline int gic_get_current_cpu(GICState *s)
>   {
>       if (!qtest_enabled() && s->num_cpu > 1) {
> -        return current_cpu->cpu_index;
> +        return current_cpu->cpu_index - s->first_cpu_index;

Note, CPUState::cpu_index is meant for accelerators code and shouldn't
be used in hw/ (in particular because it vary when using hotplug).

>       }
>       return 0;
>   }
> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> index 0f0c48d89a..ed5be05645 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -350,6 +350,7 @@ static void arm_gic_common_linux_init(ARMLinuxBootIf *obj,
>   
>   static const Property arm_gic_common_properties[] = {
>       DEFINE_PROP_UINT32("num-cpu", GICState, num_cpu, 1),
> +    DEFINE_PROP_UINT32("first-cpu-index", GICState, first_cpu_index, 0),
>       DEFINE_PROP_UINT32("num-irq", GICState, num_irq, 32),
>       /* Revision can be 1 or 2 for GIC architecture specification
>        * versions 1 or 2, or 0 to indicate the legacy 11MPCore GIC.
> diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
> index 97fea4102d..93a3cc2bf8 100644
> --- a/include/hw/intc/arm_gic_common.h
> +++ b/include/hw/intc/arm_gic_common.h
> @@ -129,6 +129,8 @@ struct GICState {
>       uint32_t num_lrs;
>   
>       uint32_t num_cpu;
> +    /* cpu_index of the first CPU, attached to this GIC.  */
> +    uint32_t first_cpu_index;
>   
>       MemoryRegion iomem; /* Distributor */
>       /* This is just so we can have an opaque pointer which identifies

Alternative series motivated to remove &first_cpu / qemu_get_cpu():
https://lore.kernel.org/qemu-devel/20231212162935.42910-1-philmd@linaro.org/

Re: [PATCH 2/4] hw/intc/arm_gic: introduce a first-cpu-index property

[Clément Chigot]

On Tue, May 13, 2025 at 5:39 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> On 13/5/25 16:14, Clément Chigot wrote:
> > From: Frederic Konrad <konrad.frederic@yahoo.fr>
> >
> > This introduces a first-cpu-index property to the arm-gic, as some SOCs
> > could have two separate GIC (ie: the zynqmp).
> >
> > Signed-off-by: Clément Chigot <chigot@adacore.com>
> > ---
> >   hw/intc/arm_gic.c                | 2 +-
> >   hw/intc/arm_gic_common.c         | 1 +
> >   include/hw/intc/arm_gic_common.h | 2 ++
> >   3 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> > index d18bef40fc..899f133363 100644
> > --- a/hw/intc/arm_gic.c
> > +++ b/hw/intc/arm_gic.c
> > @@ -59,7 +59,7 @@ static const uint8_t gic_id_gicv2[] = {
> >   static inline int gic_get_current_cpu(GICState *s)
> >   {
> >       if (!qtest_enabled() && s->num_cpu > 1) {
> > -        return current_cpu->cpu_index;
> > +        return current_cpu->cpu_index - s->first_cpu_index;
>
> Note, CPUState::cpu_index is meant for accelerators code and shouldn't
> be used in hw/ (in particular because it vary when using hotplug).

Is there another way to perform that then ? As you can see `cpu_index`
is already present prior to my patch. I don't mind improving it as a
prerequisite for that series though.

> >       }
> >       return 0;
> >   }
> > diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> > index 0f0c48d89a..ed5be05645 100644
> > --- a/hw/intc/arm_gic_common.c
> > +++ b/hw/intc/arm_gic_common.c
> > @@ -350,6 +350,7 @@ static void arm_gic_common_linux_init(ARMLinuxBootIf *obj,
> >
> >   static const Property arm_gic_common_properties[] = {
> >       DEFINE_PROP_UINT32("num-cpu", GICState, num_cpu, 1),
> > +    DEFINE_PROP_UINT32("first-cpu-index", GICState, first_cpu_index, 0),
> >       DEFINE_PROP_UINT32("num-irq", GICState, num_irq, 32),
> >       /* Revision can be 1 or 2 for GIC architecture specification
> >        * versions 1 or 2, or 0 to indicate the legacy 11MPCore GIC.
> > diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
> > index 97fea4102d..93a3cc2bf8 100644
> > --- a/include/hw/intc/arm_gic_common.h
> > +++ b/include/hw/intc/arm_gic_common.h
> > @@ -129,6 +129,8 @@ struct GICState {
> >       uint32_t num_lrs;
> >
> >       uint32_t num_cpu;
> > +    /* cpu_index of the first CPU, attached to this GIC.  */
> > +    uint32_t first_cpu_index;
> >
> >       MemoryRegion iomem; /* Distributor */
> >       /* This is just so we can have an opaque pointer which identifies
>
> Alternative series motivated to remove &first_cpu / qemu_get_cpu():
> https://lore.kernel.org/qemu-devel/20231212162935.42910-1-philmd@linaro.org/

Re: [PATCH 2/4] hw/intc/arm_gic: introduce a first-cpu-index property

[Philippe Mathieu-Daudé]

Hi Clément,

On 14/5/25 14:41, Clément Chigot wrote:
> On Tue, May 13, 2025 at 5:39 PM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
>>
>> On 13/5/25 16:14, Clément Chigot wrote:
>>> From: Frederic Konrad <konrad.frederic@yahoo.fr>
>>>
>>> This introduces a first-cpu-index property to the arm-gic, as some SOCs
>>> could have two separate GIC (ie: the zynqmp).
>>>
>>> Signed-off-by: Clément Chigot <chigot@adacore.com>
>>> ---
>>>    hw/intc/arm_gic.c                | 2 +-
>>>    hw/intc/arm_gic_common.c         | 1 +
>>>    include/hw/intc/arm_gic_common.h | 2 ++
>>>    3 files changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>>> index d18bef40fc..899f133363 100644
>>> --- a/hw/intc/arm_gic.c
>>> +++ b/hw/intc/arm_gic.c
>>> @@ -59,7 +59,7 @@ static const uint8_t gic_id_gicv2[] = {
>>>    static inline int gic_get_current_cpu(GICState *s)
>>>    {
>>>        if (!qtest_enabled() && s->num_cpu > 1) {
>>> -        return current_cpu->cpu_index;
>>> +        return current_cpu->cpu_index - s->first_cpu_index;
>>
>> Note, CPUState::cpu_index is meant for accelerators code and shouldn't
>> be used in hw/ (in particular because it vary when using hotplug).
> 
> Is there another way to perform that then ? As you can see `cpu_index`
> is already present prior to my patch. I don't mind improving it as a
> prerequisite for that series though.

Yeah it is a pre-existing design issue, I was just thinking loudly,
no need to worry for your use: if we ever clean it, we'll also
clean here.

> 
>>>        }
>>>        return 0;
>>>    }
>>> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
>>> index 0f0c48d89a..ed5be05645 100644
>>> --- a/hw/intc/arm_gic_common.c
>>> +++ b/hw/intc/arm_gic_common.c
>>> @@ -350,6 +350,7 @@ static void arm_gic_common_linux_init(ARMLinuxBootIf *obj,
>>>
>>>    static const Property arm_gic_common_properties[] = {
>>>        DEFINE_PROP_UINT32("num-cpu", GICState, num_cpu, 1),
>>> +    DEFINE_PROP_UINT32("first-cpu-index", GICState, first_cpu_index, 0),
>>>        DEFINE_PROP_UINT32("num-irq", GICState, num_irq, 32),
>>>        /* Revision can be 1 or 2 for GIC architecture specification
>>>         * versions 1 or 2, or 0 to indicate the legacy 11MPCore GIC.
>>> diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
>>> index 97fea4102d..93a3cc2bf8 100644
>>> --- a/include/hw/intc/arm_gic_common.h
>>> +++ b/include/hw/intc/arm_gic_common.h
>>> @@ -129,6 +129,8 @@ struct GICState {
>>>        uint32_t num_lrs;
>>>
>>>        uint32_t num_cpu;
>>> +    /* cpu_index of the first CPU, attached to this GIC.  */
>>> +    uint32_t first_cpu_index;
>>>
>>>        MemoryRegion iomem; /* Distributor */
>>>        /* This is just so we can have an opaque pointer which identifies
>>
>> Alternative series motivated to remove &first_cpu / qemu_get_cpu():
>> https://lore.kernel.org/qemu-devel/20231212162935.42910-1-philmd@linaro.org/

(Just an observation, not an objection to this simple work-around).

Regards,

Phil.

Re: [PATCH 2/4] hw/intc/arm_gic: introduce a first-cpu-index property

[Peter Maydell]

On Tue, 13 May 2025 at 16:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 13/5/25 16:14, Clément Chigot wrote:
> > From: Frederic Konrad <konrad.frederic@yahoo.fr>
> >
> > This introduces a first-cpu-index property to the arm-gic, as some SOCs
> > could have two separate GIC (ie: the zynqmp).
> >
> > Signed-off-by: Clément Chigot <chigot@adacore.com>
> > ---
> >   hw/intc/arm_gic.c                | 2 +-
> >   hw/intc/arm_gic_common.c         | 1 +
> >   include/hw/intc/arm_gic_common.h | 2 ++
> >   3 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> > index d18bef40fc..899f133363 100644
> > --- a/hw/intc/arm_gic.c
> > +++ b/hw/intc/arm_gic.c
> > @@ -59,7 +59,7 @@ static const uint8_t gic_id_gicv2[] = {
> >   static inline int gic_get_current_cpu(GICState *s)
> >   {
> >       if (!qtest_enabled() && s->num_cpu > 1) {
> > -        return current_cpu->cpu_index;
> > +        return current_cpu->cpu_index - s->first_cpu_index;
>
> Note, CPUState::cpu_index is meant for accelerators code and shouldn't
> be used in hw/ (in particular because it vary when using hotplug).

Mmm. I think my ideal solution for that would be that we
put the CPU index into the MemTxAttrs (requester_id, perhaps
with some extra flag that it's not a PCI requester ID).
Then every device that's looking at current_cpu to figure
out which CPU actually called it would be able to stop
doing that. We'd still have the "OK, so what number does
the GIC think this CPU is?" question, though.

For telling the GIC which CPUs it has connected to it, my
instinct is to say that ideally we'd have the GIC have
something like an array of link properties, and the board
or SoC code passes the GIC a pointer to each CPU in the
order it wants them to be.

But that would be quite a bit of fiddling around to achieve,
so I think I'm OK with the approach in this patch, especially
since the GICv2 is pretty well obsolete at this point.

(GICv3 also assumes "starting from zero", in several places
where it loops from 0 to s->num_cpu calling qemu_get_cpu().
Link properties is probably what I'm going to end up doing with
the GICv5 design.)

One note: if you add a new property to the GIC, please
also add documentation of it to the "QEMU interface" comment
in include/hw/intc/arm_gic.h.

thanks
-- PMM

Re: [PATCH 2/4] hw/intc/arm_gic: introduce a first-cpu-index property

[Edgar E. Iglesias]

[-- Attachment #1: Type: text/plain, Size: 3047 bytes --]

On Mon, May 19, 2025 at 5:26 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Tue, 13 May 2025 at 16:39, Philippe Mathieu-Daudé <philmd@linaro.org>
> wrote:
> >
> > On 13/5/25 16:14, Clément Chigot wrote:
> > > From: Frederic Konrad <konrad.frederic@yahoo.fr>
> > >
> > > This introduces a first-cpu-index property to the arm-gic, as some SOCs
> > > could have two separate GIC (ie: the zynqmp).
> > >
> > > Signed-off-by: Clément Chigot <chigot@adacore.com>
> > > ---
> > >   hw/intc/arm_gic.c                | 2 +-
> > >   hw/intc/arm_gic_common.c         | 1 +
> > >   include/hw/intc/arm_gic_common.h | 2 ++
> > >   3 files changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> > > index d18bef40fc..899f133363 100644
> > > --- a/hw/intc/arm_gic.c
> > > +++ b/hw/intc/arm_gic.c
> > > @@ -59,7 +59,7 @@ static const uint8_t gic_id_gicv2[] = {
> > >   static inline int gic_get_current_cpu(GICState *s)
> > >   {
> > >       if (!qtest_enabled() && s->num_cpu > 1) {
> > > -        return current_cpu->cpu_index;
> > > +        return current_cpu->cpu_index - s->first_cpu_index;
> >
> > Note, CPUState::cpu_index is meant for accelerators code and shouldn't
> > be used in hw/ (in particular because it vary when using hotplug).
>
> Mmm. I think my ideal solution for that would be that we
> put the CPU index into the MemTxAttrs (requester_id, perhaps
> with some extra flag that it's not a PCI requester ID).
> Then every device that's looking at current_cpu to figure
> out which CPU actually called it would be able to stop
> doing that. We'd still have the "OK, so what number does
> the GIC think this CPU is?" question, though.
>
> For telling the GIC which CPUs it has connected to it, my
> instinct is to say that ideally we'd have the GIC have
> something like an array of link properties, and the board
> or SoC code passes the GIC a pointer to each CPU in the
> order it wants them to be.
>
> But that would be quite a bit of fiddling around to achieve,
> so I think I'm OK with the approach in this patch, especially
> since the GICv2 is pretty well obsolete at this point.
>
> (GICv3 also assumes "starting from zero", in several places
> where it loops from 0 to s->num_cpu calling qemu_get_cpu().
> Link properties is probably what I'm going to end up doing with
> the GICv5 design.)
>
> One note: if you add a new property to the GIC, please
> also add documentation of it to the "QEMU interface" comment
> in include/hw/intc/arm_gic.h.
>
>
>
Yes, I agree with Peter.

Another option is to have the GIC expose a per-cpu MR for the GICC
interfaces.
We would need to map these onto a per-cpu address-space. I think this is
the way
it works on real HW GICv2 with a per-cpu AMBA port. A problem with this
approach
is that it (in QEMU) doesn't scale very well to other GIC's with large
numbers of cores.
So the side-band signals with CPU index in memtxattrs sounds good to me!

Cheers,
Edgar

[-- Attachment #2: Type: text/html, Size: 4095 bytes --]

[PATCH 3/4] hw/arm/xlnx-zynqmp: wire a second GIC for the Cortex-R5

[Clément Chigot]

From: Frederic Konrad <konrad.frederic@yahoo.fr>

This wires a second GIC for the Cortex-R5, all the IRQs are split when there
is an RPU instanciated.

Signed-off-by: Clément Chigot <chigot@adacore.com>
---
 hw/arm/xlnx-zynqmp.c         | 88 +++++++++++++++++++++++++++++++++---
 include/hw/arm/xlnx-zynqmp.h |  6 +++
 2 files changed, 87 insertions(+), 7 deletions(-)

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index ec96a46eec..be33669f87 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -26,8 +26,6 @@
 #include "target/arm/cpu-qom.h"
 #include "target/arm/gtimer.h"
 
-#define GIC_NUM_SPI_INTR 160
-
 #define ARM_PHYS_TIMER_PPI  30
 #define ARM_VIRT_TIMER_PPI  27
 #define ARM_HYP_TIMER_PPI   26
@@ -206,7 +204,7 @@ static const XlnxZynqMPGICRegion xlnx_zynqmp_gic_regions[] = {
 
 static inline int arm_gic_ppi_index(int cpu_nr, int ppi_index)
 {
-    return GIC_NUM_SPI_INTR + cpu_nr * GIC_INTERNAL + ppi_index;
+    return XLXN_ZYNQMP_GIC_NUM_SPI_INTR + cpu_nr * GIC_INTERNAL + ppi_index;
 }
 
 static void xlnx_zynqmp_create_rpu(MachineState *ms, XlnxZynqMPState *s,
@@ -377,6 +375,8 @@ static void xlnx_zynqmp_init(Object *obj)
     XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
     int i;
     int num_apus = MIN(ms->smp.cpus, XLNX_ZYNQMP_NUM_APU_CPUS);
+    int num_rpus = MIN((int)(ms->smp.cpus - XLNX_ZYNQMP_NUM_APU_CPUS),
+                       XLNX_ZYNQMP_NUM_RPU_CPUS);
 
     object_initialize_child(obj, "apu-cluster", &s->apu_cluster,
                             TYPE_CPU_CLUSTER);
@@ -390,6 +390,12 @@ static void xlnx_zynqmp_init(Object *obj)
 
     object_initialize_child(obj, "gic", &s->gic, gic_class_name());
 
+    if (num_rpus > 0) {
+        /* Do not create the rpu_gic in case we don't have rpus..  */
+        object_initialize_child(obj, "rpu_gic", &s->rpu_gic,
+                                gic_class_name());
+    }
+
     for (i = 0; i < XLNX_ZYNQMP_NUM_GEMS; i++) {
         object_initialize_child(obj, "gem[*]", &s->gem[i], TYPE_CADENCE_GEM);
         object_initialize_child(obj, "gem-irq-orgate[*]",
@@ -439,6 +445,13 @@ static void xlnx_zynqmp_init(Object *obj)
     object_initialize_child(obj, "qspi-irq-orgate",
                             &s->qspi_irq_orgate, TYPE_OR_IRQ);
 
+    for (i = 0; i < ARRAY_SIZE(s->splitter); i++) {
+        g_autofree char *name = g_strdup_printf("irq-splitter%d", i);
+        object_initialize_child(obj, name, &s->splitter[i], TYPE_SPLIT_IRQ);
+    }
+
+
+
     for (i = 0; i < XLNX_ZYNQMP_NUM_USB; i++) {
         object_initialize_child(obj, "usb[*]", &s->usb[i], TYPE_USB_DWC3);
     }
@@ -452,10 +465,13 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
     uint8_t i;
     uint64_t ram_size;
     int num_apus = MIN(ms->smp.cpus, XLNX_ZYNQMP_NUM_APU_CPUS);
+    int num_rpus = MIN((int)(ms->smp.cpus - XLNX_ZYNQMP_NUM_APU_CPUS),
+                       XLNX_ZYNQMP_NUM_RPU_CPUS);
     const char *boot_cpu = s->boot_cpu ? s->boot_cpu : "apu-cpu[0]";
     ram_addr_t ddr_low_size, ddr_high_size;
-    qemu_irq gic_spi[GIC_NUM_SPI_INTR];
+    qemu_irq gic_spi[XLXN_ZYNQMP_GIC_NUM_SPI_INTR];
     Error *err = NULL;
+    DeviceState *splitter;
 
     ram_size = memory_region_size(s->ddr_ram);
 
@@ -502,13 +518,21 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
         g_free(ocm_name);
     }
 
-    qdev_prop_set_uint32(DEVICE(&s->gic), "num-irq", GIC_NUM_SPI_INTR + 32);
+    qdev_prop_set_uint32(DEVICE(&s->gic), "num-irq", XLXN_ZYNQMP_GIC_NUM_SPI_INTR + 32);
     qdev_prop_set_uint32(DEVICE(&s->gic), "revision", 2);
     qdev_prop_set_uint32(DEVICE(&s->gic), "num-cpu", num_apus);
     qdev_prop_set_bit(DEVICE(&s->gic), "has-security-extensions", s->secure);
     qdev_prop_set_bit(DEVICE(&s->gic),
                       "has-virtualization-extensions", s->virt);
 
+    if (num_rpus > 0) {
+        qdev_prop_set_uint32(DEVICE(&s->rpu_gic), "num-irq",
+                             XLXN_ZYNQMP_GIC_NUM_SPI_INTR + 32);
+        qdev_prop_set_uint32(DEVICE(&s->rpu_gic), "revision", 1);
+        qdev_prop_set_uint32(DEVICE(&s->rpu_gic), "num-cpu", num_rpus);
+        qdev_prop_set_uint32(DEVICE(&s->rpu_gic), "first-cpu-index", 4);
+    }
+
     qdev_realize(DEVICE(&s->apu_cluster), NULL, &error_fatal);
 
     /* Realize APUs before realizing the GIC. KVM requires this.  */
@@ -608,13 +632,63 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (num_rpus > 0) {
+        if (!sysbus_realize(SYS_BUS_DEVICE(&s->rpu_gic), errp)) {
+            return;
+        }
+
+        for (i = 0; i < num_rpus; i++) {
+            qemu_irq irq;
+
+            sysbus_mmio_map(SYS_BUS_DEVICE(&s->rpu_gic), i + 1,
+                            GIC_BASE_ADDR + i * 0x1000);
+            sysbus_connect_irq(SYS_BUS_DEVICE(&s->rpu_gic), i,
+                               qdev_get_gpio_in(DEVICE(&s->rpu_cpu[i]),
+                                                ARM_CPU_IRQ));
+            sysbus_connect_irq(SYS_BUS_DEVICE(&s->rpu_gic), i + num_rpus,
+                               qdev_get_gpio_in(DEVICE(&s->rpu_cpu[i]),
+                                                ARM_CPU_FIQ));
+            sysbus_connect_irq(SYS_BUS_DEVICE(&s->rpu_gic), i + num_rpus * 2,
+                               qdev_get_gpio_in(DEVICE(&s->rpu_cpu[i]),
+                                                ARM_CPU_VIRQ));
+            sysbus_connect_irq(SYS_BUS_DEVICE(&s->rpu_gic), i + num_rpus * 3,
+                               qdev_get_gpio_in(DEVICE(&s->rpu_cpu[i]),
+                                                ARM_CPU_VFIQ));
+            irq = qdev_get_gpio_in(DEVICE(&s->rpu_gic),
+                                   arm_gic_ppi_index(i, ARM_PHYS_TIMER_PPI));
+            qdev_connect_gpio_out(DEVICE(&s->rpu_cpu[i]), GTIMER_PHYS, irq);
+            irq = qdev_get_gpio_in(DEVICE(&s->rpu_gic),
+                                   arm_gic_ppi_index(i, ARM_VIRT_TIMER_PPI));
+            qdev_connect_gpio_out(DEVICE(&s->rpu_cpu[i]), GTIMER_VIRT, irq);
+            irq = qdev_get_gpio_in(DEVICE(&s->rpu_gic),
+                                   arm_gic_ppi_index(i, ARM_HYP_TIMER_PPI));
+            qdev_connect_gpio_out(DEVICE(&s->rpu_cpu[i]), GTIMER_HYP, irq);
+            irq = qdev_get_gpio_in(DEVICE(&s->rpu_gic),
+                                   arm_gic_ppi_index(i, ARM_SEC_TIMER_PPI));
+            qdev_connect_gpio_out(DEVICE(&s->rpu_cpu[i]), GTIMER_SEC, irq);
+        }
+
+        sysbus_mmio_map(SYS_BUS_DEVICE(&s->rpu_gic), 0, GIC_BASE_ADDR);
+    }
+
     if (!s->boot_cpu_ptr) {
         error_setg(errp, "ZynqMP Boot cpu %s not found", boot_cpu);
         return;
     }
 
-    for (i = 0; i < GIC_NUM_SPI_INTR; i++) {
-        gic_spi[i] = qdev_get_gpio_in(DEVICE(&s->gic), i);
+    for (i = 0; i < XLXN_ZYNQMP_GIC_NUM_SPI_INTR; i++) {
+        splitter = DEVICE(&s->splitter[i]);
+        qdev_prop_set_uint16(splitter, "num-lines", 2);
+        qdev_realize(splitter, NULL, &error_abort);
+        if (num_rpus > 0) {
+            gic_spi[i] = qdev_get_gpio_in(splitter, 0);
+            qdev_connect_gpio_out(splitter, 0,
+                                  qdev_get_gpio_in(DEVICE(&s->gic), i));
+            qdev_connect_gpio_out(splitter, 1,
+                                  qdev_get_gpio_in(DEVICE(&s->rpu_gic), i));
+        } else {
+            gic_spi[i] = qdev_get_gpio_in(DEVICE(&s->gic), i);
+        }
     }
 
     for (i = 0; i < XLNX_ZYNQMP_NUM_GEMS; i++) {
diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
index c137ac59e8..a69953650d 100644
--- a/include/hw/arm/xlnx-zynqmp.h
+++ b/include/hw/arm/xlnx-zynqmp.h
@@ -42,6 +42,7 @@
 #include "hw/misc/xlnx-zynqmp-crf.h"
 #include "hw/timer/cadence_ttc.h"
 #include "hw/usb/hcd-dwc3.h"
+#include "hw/core/split-irq.h"
 
 #define TYPE_XLNX_ZYNQMP "xlnx-zynqmp"
 OBJECT_DECLARE_SIMPLE_TYPE(XlnxZynqMPState, XLNX_ZYNQMP)
@@ -87,12 +88,14 @@ OBJECT_DECLARE_SIMPLE_TYPE(XlnxZynqMPState, XLNX_ZYNQMP)
                                   XLNX_ZYNQMP_MAX_HIGH_RAM_SIZE)
 
 #define XLNX_ZYNQMP_NUM_TTC 4
+#define XLXN_ZYNQMP_GIC_NUM_SPI_INTR 160
 
 /*
  * Unimplemented mmio regions needed to boot some images.
  */
 #define XLNX_ZYNQMP_NUM_UNIMP_AREAS 1
 
+
 struct XlnxZynqMPState {
     /*< private >*/
     DeviceState parent_obj;
@@ -105,6 +108,9 @@ struct XlnxZynqMPState {
     GICState gic;
     MemoryRegion gic_mr[XLNX_ZYNQMP_GIC_REGIONS][XLNX_ZYNQMP_GIC_ALIASES];
 
+    GICState rpu_gic;
+    SplitIRQ splitter[XLXN_ZYNQMP_GIC_NUM_SPI_INTR];
+
     MemoryRegion ocm_ram[XLNX_ZYNQMP_NUM_OCM_BANKS];
 
     MemoryRegion *ddr_ram;
-- 
2.34.1

[PATCH 4/4] hw/arm/xlnx-zynqmp: adapt cluster-id based on the boot cpu

[Clément Chigot]

When gdb is being connected to QEmu, it will be attached to the first
CPU cluster. However, the ZynqMP board has two clusters, those being of
two different architectures.
Therefore, when gdb is connecting to the ZynqMP, it receives the target
descriptor of the first CPU cluster. Up to now, it was always the APU
cluster, which is AARCH64.

When booting on a RPU, gdb will still connect to the APU. If gdb is
supporting only ARM32, it will receive the APU target descriptor,
resulting in:
  | (gdb) target remote :1234
  | warning: while parsing target description (at line 1): Target
  | description specified unknown architecture "aarch64"

Adjust the cluster-id based on the boot cpu will resolve the above
issue; allowing a pure ARM32 toolchain to debug programs running on
RPUs.

Signed-off-by: Clément Chigot <chigot@adacore.com>
---
 hw/arm/xlnx-zynqmp.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index be33669f87..f23ace6446 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -221,7 +221,13 @@ static void xlnx_zynqmp_create_rpu(MachineState *ms, XlnxZynqMPState *s,
 
     object_initialize_child(OBJECT(s), "rpu-cluster", &s->rpu_cluster,
                             TYPE_CPU_CLUSTER);
-    qdev_prop_set_uint32(DEVICE(&s->rpu_cluster), "cluster-id", 1);
+
+    /* In order to connect gdb to the boot cpu, adjust the cluster-id.  */
+    if (!strncmp(boot_cpu, "rpu-cpu", 7)) {
+        qdev_prop_set_uint32(DEVICE(&s->rpu_cluster), "cluster-id", 0);
+    } else {
+        qdev_prop_set_uint32(DEVICE(&s->rpu_cluster), "cluster-id", 1);
+    }
 
     for (i = 0; i < num_rpus; i++) {
         const char *name;
@@ -380,7 +386,6 @@ static void xlnx_zynqmp_init(Object *obj)
 
     object_initialize_child(obj, "apu-cluster", &s->apu_cluster,
                             TYPE_CPU_CLUSTER);
-    qdev_prop_set_uint32(DEVICE(&s->apu_cluster), "cluster-id", 0);
 
     for (i = 0; i < num_apus; i++) {
         object_initialize_child(OBJECT(&s->apu_cluster), "apu-cpu[*]",
@@ -475,6 +480,13 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
 
     ram_size = memory_region_size(s->ddr_ram);
 
+    /* In order to connect gdb to the boot cpu, adjust the cluster-id.  */
+    if (!strncmp(boot_cpu, "apu-cpu", 7)) {
+        qdev_prop_set_uint32(DEVICE(&s->apu_cluster), "cluster-id", 0);
+    } else {
+        qdev_prop_set_uint32(DEVICE(&s->apu_cluster), "cluster-id", 1);
+    }
+
     /*
      * Create the DDR Memory Regions. User friendly checks should happen at
      * the board level
-- 
2.34.1

Re: [PATCH 4/4] hw/arm/xlnx-zynqmp: adapt cluster-id based on the boot cpu

[Peter Maydell]

On Tue, 13 May 2025 at 15:15, Clément Chigot <chigot@adacore.com> wrote:
>
> When gdb is being connected to QEmu, it will be attached to the first

(QEMU is all-caps, by the way)

> CPU cluster. However, the ZynqMP board has two clusters, those being of
> two different architectures.
> Therefore, when gdb is connecting to the ZynqMP, it receives the target
> descriptor of the first CPU cluster. Up to now, it was always the APU
> cluster, which is AARCH64.
>
> When booting on a RPU, gdb will still connect to the APU. If gdb is
> supporting only ARM32, it will receive the APU target descriptor,
> resulting in:
>   | (gdb) target remote :1234
>   | warning: while parsing target description (at line 1): Target
>   | description specified unknown architecture "aarch64"
>
> Adjust the cluster-id based on the boot cpu will resolve the above
> issue; allowing a pure ARM32 toolchain to debug programs running on
> RPUs.

I'm not really enthusiastic about renumbering the clusters
like this. I think you should be able to get gdb to connect
to the second cluster via the multiple-inferior support:

https://www.qemu.org/docs/master/system/gdb.html#debugging-multicore-machines

thanks
-- PMM

Re: [PATCH 4/4] hw/arm/xlnx-zynqmp: adapt cluster-id based on the boot cpu

[Clément Chigot]

On Mon, May 19, 2025 at 5:38 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 13 May 2025 at 15:15, Clément Chigot <chigot@adacore.com> wrote:
> >
> > When gdb is being connected to QEmu, it will be attached to the first
>
> (QEMU is all-caps, by the way)
>
> > CPU cluster. However, the ZynqMP board has two clusters, those being of
> > two different architectures.
> > Therefore, when gdb is connecting to the ZynqMP, it receives the target
> > descriptor of the first CPU cluster. Up to now, it was always the APU
> > cluster, which is AARCH64.
> >
> > When booting on a RPU, gdb will still connect to the APU. If gdb is
> > supporting only ARM32, it will receive the APU target descriptor,
> > resulting in:
> >   | (gdb) target remote :1234
> >   | warning: while parsing target description (at line 1): Target
> >   | description specified unknown architecture "aarch64"
> >
> > Adjust the cluster-id based on the boot cpu will resolve the above
> > issue; allowing a pure ARM32 toolchain to debug programs running on
> > RPUs.
>
> I'm not really enthusiastic about renumbering the clusters
> like this. I think you should be able to get gdb to connect
> to the second cluster via the multiple-inferior support:
>
> https://www.qemu.org/docs/master/system/gdb.html#debugging-multicore-machines

Sadly, that doesn't work. `attach 2` requires the connection to be
already established. But it's being aborted as soon as gdb can't
decode the XML provided.
  | $ arm-elf-gdb helloworld
  | (gdb) target extended-remote localhost:1234
  | Remote debugging using localhost:1234
  | warning: while parsing target description (at line 1): Target
description specified unknown architecture "aarch64"
  | warning: Could not load XML target description; ignoring
  | Remote 'g' packet reply is too long (expected 168 bytes, got 268 bytes): ...
  | (gdb) add-inferior
  | [New inferior 2]
  | Added inferior 2
  | (gdb) inferior 2
  | [Switching to inferior 2 [<null>] (<noexec>)]
  | (gdb) attach 2
  | Don't know how to attach.  Try "help target".
  | (gdb) info connection
  | No connections.

FTR, I tried to find a way to adjust gdbstub.c to retrieve the
boot-cpu or have a different "first cpu" according to an option. But
this was far more complex than just that patch.
That being said, I understand that this solution is far from perfect
and I'm fine keeping it as a local patch if you think it would bring
more pain overall. There is a simple workaround/solution: have a gdb
understanding both aarch64 and arm32 when debugging ZynqMP programs
(something we cannot do that on our side for various reasons
though...).

Clément

> thanks
> -- PMM