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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.