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

[Harsh Prateek Bora <harshpb@linux.ibm.com>]

On 11/23/23 14:20, Cé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.

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
>