1. Patchew
  2. QEMU
  3. [PATCH 0/4] xlnx-zynqmp: add support to boot on RPUs
  4. View patch

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

Clément Chigot posted 4 patches 6 months ago
Maintainers: Peter Maydell <peter.maydell@linaro.org>, Alistair Francis <alistair@alistair23.me>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
There is a newer version of this series
[PATCH 2/4] hw/intc/arm_gic: introduce a first-cpu-index property
Posted by Clément Chigot 6 months ago 
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
Posted by Philippe Mathieu-Daudé 6 months ago 
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
Posted by Peter Maydell 6 months ago 
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
Posted by Edgar E. Iglesias 5 months, 4 weeks ago 
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
Re: [PATCH 2/4] hw/intc/arm_gic: introduce a first-cpu-index property
Posted by Clément Chigot 6 months ago 
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
Posted by Philippe Mathieu-Daudé 6 months ago 
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.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.