Re: [PATCH v3 1/2] ppc/spapr: Introduce SPAPR_IRQ_NR_IPIS to refer IRQ range for CPU IPIs.

[Kowshik Jois B S <kowsjois@linux.vnet.ibm.com>]

On 24/11/23 13:39, Cédric Le Goater wrote:
> On 11/24/23 09:01, Harsh Prateek Bora wrote:
>>
>>
>> On 11/23/23 19:42, Cédric Le Goater wrote:
>>> On 11/23/23 10:31, Harsh Prateek Bora wrote:
>>>>
>>>>
>>>> On 11/23/23 14:20, CI've applied these patches and verified on the 
>>>> latest upstream qemu. The code is working as expected.
>>>>
>>>> Tested-by: Kowshik Jois<kowsjois@linux.ibm.com>édric Le Goater wrote:
>>>>> On 11/23/23 06:57, Harsh Prateek Bora wrote:
>>>>>> spapr_irq_init currently uses existing macro SPAPR_XIRQ_BASE to 
>>>>>> refer to
>>>>>> the range of CPU IPIs during initialization of nr-irqs property.
>>>>>> It is more appropriate to have its own define which can be further
>>>>>> reused as appropriate for correct interpretation.
>>>>>>
>>>>>> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>>>>>> Suggested-by: Cedric Le Goater <clg@kaod.org>
>>>>>
>>>>> One comment below
>>>>>
>>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>>>>
>>>>
>>>> Thanks, responding below ..
>>>>
>>>>>> ---
>>>>>>   include/hw/ppc/spapr_irq.h | 14 +++++++++++++-
>>>>>>   hw/ppc/spapr_irq.c         |  6 ++++--
>>>>>>   2 files changed, 17 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
>>>>>> index c22a72c9e2..4fd2d5853d 100644
>>>>>> --- a/include/hw/ppc/spapr_irq.h
>>>>>> +++ b/include/hw/ppc/spapr_irq.h
>>>>>> @@ -14,9 +14,21 @@
>>>>>>   #include "qom/object.h"
>>>>>>   /*
>>>>>> - * IRQ range offsets per device type
>>>>>> + * The XIVE IRQ backend uses the same layout as the XICS backend 
>>>>>> but
>>>>>> + * covers the full range of the IRQ number space. The IRQ 
>>>>>> numbers for
>>>>>> + * the CPU IPIs are allocated at the bottom of this space, below 
>>>>>> 4K,
>>>>>> + * to preserve compatibility with XICS which does not use that 
>>>>>> range.
>>>>>> + */
>>>>>> +
>>>>>> +/*
>>>>>> + * CPU IPI range (XIVE only)
>>>>>>    */
>>>>>>   #define SPAPR_IRQ_IPI        0x0
>>>>>> +#define SPAPR_IRQ_NR_IPIS    0x1000
>>>>>> +
>>>>>> +/*
>>>>>> + * IRQ range offsets per device type
>>>>>> + */
>>>>>>   #define SPAPR_XIRQ_BASE      XICS_IRQ_BASE /* 0x1000 */
>>>>>>   #define SPAPR_IRQ_EPOW       (SPAPR_XIRQ_BASE + 0x0000)
>>>>>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>>>>>> index a0d1e1298e..97b2fc42ab 100644
>>>>>> --- a/hw/ppc/spapr_irq.c
>>>>>> +++ b/hw/ppc/spapr_irq.c
>>>>>> @@ -23,6 +23,8 @@
>>>>>>   #include "trace.h"
>>>>>> +QEMU_BUILD_BUG_ON(SPAPR_IRQ_NR_IPIS > SPAPR_XIRQ_BASE);
>>>>>> +
>>>>>
>>>>> I would have put the check in include/hw/ppc/spapr_irq.h but since
>>>>> SPAPR_XIRQ_BASE is only used in hw/ppc/spapr_irq.c which is always
>>>>> compiled, this is fine. You might want to change that in case a
>>>>> respin is asked for.
>>>>>
>>>>
>>>> I had initially tried keeping it in spapr_irq.h , but that would 
>>>> give a build break for XICS_IRQ_BASE not defined since that gets 
>>>> defined in spapr_xics.h and is included later in some files, 
>>>> however, the QEMU_BUILD_BUG_ON expects it to be defined before it 
>>>> reaches here.
>>>
>>> ah. good catch. this went unnoticed and is a bit ugly. We should fix
>>> in some ways. May with a define SPAPR_XIRQ_BASE to 0x1000 simply ?
>>>
>>
>> Hmm, I can do that if a re-spin is reqd, or can be done as a separate
>> patch later also along with other improvements.
>
> yes. This is food for thoughts for further improvements.
>
> Thanks,
>
> C.
>
>
>
>>
>>> Also, we could probably define the ICS offset to SPAPR_XIRQ_BASE
>>> directly under spapr_irq_init() and get rid of ics_instance_init().
>>> The HW IRQ Number offset in the PNV ICS instances is assigned
>>> dynamically by the OS (see pnv_phb3). So it should befine to do
>>> the same for spapr. In which case we can get rid of XICS_IRQ_BASE.
>>>
>>
>> Hmm, I am not so familiar with XICS yet, so not sure if we really need
>> to do that, but it can be done along with other improvements if needed.
>>
>> regards,
>> Harsh
>>
>>> Thanks,
>>>
>>> C.
>>>
>>>
>>>
>>>>
>>>> regards,
>>>> Harsh
>>>>
>>>>> Thanks,
>>>>>
>>>>> C.
>>>>>
>>>>>
>>>>>>   static const TypeInfo spapr_intc_info = {
>>>>>>       .name = TYPE_SPAPR_INTC,
>>>>>>       .parent = TYPE_INTERFACE,
>>>>>> @@ -329,7 +331,7 @@ void spapr_irq_init(SpaprMachineState *spapr, 
>>>>>> Error **errp)
>>>>>>           int i;
>>>>>>           dev = qdev_new(TYPE_SPAPR_XIVE);
>>>>>> -        qdev_prop_set_uint32(dev, "nr-irqs", smc->nr_xirqs + 
>>>>>> SPAPR_XIRQ_BASE);
>>>>>> +        qdev_prop_set_uint32(dev, "nr-irqs", smc->nr_xirqs + 
>>>>>> SPAPR_IRQ_NR_IPIS);
>>>>>>           /*
>>>>>>            * 8 XIVE END structures per CPU. One for each available
>>>>>>            * priority
>>>>>> @@ -356,7 +358,7 @@ void spapr_irq_init(SpaprMachineState *spapr, 
>>>>>> Error **errp)
>>>>>>       }
>>>>>>       spapr->qirqs = qemu_allocate_irqs(spapr_set_irq, spapr,
>>>>>> -                                      smc->nr_xirqs + 
>>>>>> SPAPR_XIRQ_BASE);
>>>>>> +                                      smc->nr_xirqs + 
>>>>>> SPAPR_IRQ_NR_IPIS);
>>>>>>       /*
>>>>>>        * Mostly we don't actually need this until reset, except 
>>>>>> that not
>>>>>
>>>
>
> I've applied these patches and verified on the latest upstream qemu. 
> The code is working as expected.
>
> Tested-by: Kowshik Jois<kowsjois@linux.ibm.com>