[PATCH v3 0/2] Introduce SPAPR_IRQ_NR_IPIS and fix max-cpus

[PATCH v3 0/2] Introduce SPAPR_IRQ_NR_IPIS and fix max-cpus

[Harsh Prateek Bora]

On spapr, the max number of CPU IPIs are 4096 which is accounted during
spapr_irq_init but currently existing macro SPAPR_XIRQ_BASE is being
used to refer to that. Introducing SPAPR_IRQ_NR_IPIS to refer to the range
of CPU IPIS which is being further used to initialize mc->max_cpus
during spapr_machine_class_init().

v3:
 - addressed further review comments from Cedric
v2:
 - addressed review comments from Cedric

Harsh Prateek Bora (2):
  ppc/spapr: Introduce SPAPR_IRQ_NR_IPIS to refer IRQ range for CPU
    IPIs.
  ppc/spapr: Initialize max_cpus limit to SPAPR_IRQ_NR_IPIS.

 include/hw/ppc/spapr_irq.h | 14 +++++++++++++-
 hw/ppc/spapr.c             |  9 +++------
 hw/ppc/spapr_irq.c         |  6 ++++--
 3 files changed, 20 insertions(+), 9 deletions(-)

-- 
2.39.3

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

[Harsh Prateek Bora]

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>
---
 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);
+
 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
-- 
2.39.3

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

[Cédric Le Goater]

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>

> ---
>   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.

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

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

[Harsh Prateek Bora]

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
> 

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

[Cédric Le Goater]

On 11/23/23 10:31, Harsh Prateek Bora wrote:
> 
> 
> 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.

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 ?

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.

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
>>

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

[Harsh Prateek Bora]

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, 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.
> 
> 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.

> 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
>>>
> 

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

[Cédric Le Goater]

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, 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.
>>
>> 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
>>>>
>>

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

[Kowshik Jois B S]

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>

[PATCH v3 2/2] ppc/spapr: Initialize max_cpus limit to SPAPR_IRQ_NR_IPIS.

[Harsh Prateek Bora]

Initialize the machine specific max_cpus limit as per the maximum range
of CPU IPIs available. Keeping between 4096 to 8192 will throw IRQ not
free error due to XIVE/XICS limitation and keeping beyond 8192 will hit
assert in tcg_region_init or spapr_xive_claim_irq.

Logs:

Without patch fix:

[root@host build]# qemu-system-ppc64 -accel tcg -smp 10,maxcpus=4097
qemu-system-ppc64: IRQ 4096 is not free
[root@host build]#

On LPAR:
[root@host build]# qemu-system-ppc64 -accel tcg -smp 10,maxcpus=8193
**
ERROR:../tcg/region.c:774:tcg_region_init: assertion failed:
(region_size >= 2 * page_size)
Bail out! ERROR:../tcg/region.c:774:tcg_region_init: assertion failed:
(region_size >= 2 * page_size)
Aborted (core dumped)
[root@host build]#

On x86:
[root@host build]# qemu-system-ppc64 -accel tcg -smp 10,maxcpus=8193
qemu-system-ppc64: ../hw/intc/spapr_xive.c:596: spapr_xive_claim_irq:
Assertion `lisn < xive->nr_irqs' failed.
Aborted (core dumped)
[root@host build]#

With patch fix:
[root@host build]# qemu-system-ppc64 -accel tcg -smp 10,maxcpus=4097
qemu-system-ppc64: Invalid SMP CPUs 4097. The max CPUs supported by
machine 'pseries-8.2' is 4096
[root@host build]#

Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
---
 hw/ppc/spapr.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index df09aa9d6a..222d926f46 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4647,13 +4647,10 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->block_default_type = IF_SCSI;
 
     /*
-     * Setting max_cpus to INT32_MAX. Both KVM and TCG max_cpus values
-     * should be limited by the host capability instead of hardcoded.
-     * max_cpus for KVM guests will be checked in kvm_init(), and TCG
-     * guests are welcome to have as many CPUs as the host are capable
-     * of emulate.
+     * While KVM determines max cpus in kvm_init() using kvm_max_vcpus(),
+     * In TCG the limit is restricted by the range of CPU IPIs available.
      */
-    mc->max_cpus = INT32_MAX;
+    mc->max_cpus = SPAPR_IRQ_NR_IPIS;
 
     mc->no_parallel = 1;
     mc->default_boot_order = "";
-- 
2.39.3

Re: [PATCH v3 2/2] ppc/spapr: Initialize max_cpus limit to SPAPR_IRQ_NR_IPIS.

[Cédric Le Goater]

On 11/23/23 06:57, Harsh Prateek Bora wrote:
> Initialize the machine specific max_cpus limit as per the maximum range
> of CPU IPIs available. Keeping between 4096 to 8192 will throw IRQ not
> free error due to XIVE/XICS limitation and keeping beyond 8192 will hit
> assert in tcg_region_init or spapr_xive_claim_irq.
> 
> Logs:
> 
> Without patch fix:
> 
> [root@host build]# qemu-system-ppc64 -accel tcg -smp 10,maxcpus=4097
> qemu-system-ppc64: IRQ 4096 is not free
> [root@host build]#
> 
> On LPAR:
> [root@host build]# qemu-system-ppc64 -accel tcg -smp 10,maxcpus=8193
> **
> ERROR:../tcg/region.c:774:tcg_region_init: assertion failed:
> (region_size >= 2 * page_size)
> Bail out! ERROR:../tcg/region.c:774:tcg_region_init: assertion failed:
> (region_size >= 2 * page_size)
> Aborted (core dumped)
> [root@host build]#
> 
> On x86:
> [root@host build]# qemu-system-ppc64 -accel tcg -smp 10,maxcpus=8193
> qemu-system-ppc64: ../hw/intc/spapr_xive.c:596: spapr_xive_claim_irq:
> Assertion `lisn < xive->nr_irqs' failed.
> Aborted (core dumped)
> [root@host build]#
> 
> With patch fix:
> [root@host build]# qemu-system-ppc64 -accel tcg -smp 10,maxcpus=4097
> qemu-system-ppc64: Invalid SMP CPUs 4097. The max CPUs supported by
> machine 'pseries-8.2' is 4096
> [root@host build]#
> 
> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>


Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.



> ---
>   hw/ppc/spapr.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index df09aa9d6a..222d926f46 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4647,13 +4647,10 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>       mc->block_default_type = IF_SCSI;
>   
>       /*
> -     * Setting max_cpus to INT32_MAX. Both KVM and TCG max_cpus values
> -     * should be limited by the host capability instead of hardcoded.
> -     * max_cpus for KVM guests will be checked in kvm_init(), and TCG
> -     * guests are welcome to have as many CPUs as the host are capable
> -     * of emulate.
> +     * While KVM determines max cpus in kvm_init() using kvm_max_vcpus(),
> +     * In TCG the limit is restricted by the range of CPU IPIs available.
>        */
> -    mc->max_cpus = INT32_MAX;
> +    mc->max_cpus = SPAPR_IRQ_NR_IPIS;
>   
>       mc->no_parallel = 1;
>       mc->default_boot_order = "";

Re: [PATCH v3 2/2] ppc/spapr: Initialize max_cpus limit to SPAPR_IRQ_NR_IPIS.

[Kowshik Jois B S]

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


On 23/11/23 14:21, Cédric Le Goater wrote:
> On 11/23/23 06:57, Harsh Prateek Bora wrote:
>> Initialize the machine specific max_cpus limit as per the maximum range
>> of CPU IPIs available. Keeping between 4096 to 8192 will throw IRQ not
>> free error due to XIVE/XICS limitation and keeping beyond 8192 will hit
>> assert in tcg_region_init or spapr_xive_claim_irq.
>>
>> Logs:
>>
>> Without patch fix:
>>
>> [root@host build]# qemu-system-ppc64 -accel tcg -smp 10,maxcpus=4097
>> qemu-system-ppc64: IRQ 4096 is not free
>> [root@host build]#
>>
>> On LPAR:
>> [root@host build]# qemu-system-ppc64 -accel tcg -smp 10,maxcpus=8193
>> **
>> ERROR:../tcg/region.c:774:tcg_region_init: assertion failed:
>> (region_size >= 2 * page_size)
>> Bail out! ERROR:../tcg/region.c:774:tcg_region_init: assertion failed:
>> (region_size >= 2 * page_size)
>> Aborted (core dumped)
>> [root@host build]#
>>
>> On x86:
>> [root@host build]# qemu-system-ppc64 -accel tcg -smp 10,maxcpus=8193
>> qemu-system-ppc64: ../hw/intc/spapr_xive.c:596: spapr_xive_claim_irq:
>> Assertion `lisn < xive->nr_irqs' failed.
>> Aborted (core dumped)
>> [root@host build]#
>>
>> With patch fix:
>> [root@host build]# qemu-system-ppc64 -accel tcg -smp 10,maxcpus=4097
>> qemu-system-ppc64: Invalid SMP CPUs 4097. The max CPUs supported by
>> machine 'pseries-8.2' is 4096
>> [root@host build]#
>>
>> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>
>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>
> Thanks,
>
> C.
>
>
>
>> ---
>>   hw/ppc/spapr.c | 9 +++------
>>   1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index df09aa9d6a..222d926f46 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -4647,13 +4647,10 @@ static void 
>> spapr_machine_class_init(ObjectClass *oc, void *data)
>>       mc->block_default_type = IF_SCSI;
>>         /*
>> -     * Setting max_cpus to INT32_MAX. Both KVM and TCG max_cpus values
>> -     * should be limited by the host capability instead of hardcoded.
>> -     * max_cpus for KVM guests will be checked in kvm_init(), and TCG
>> -     * guests are welcome to have as many CPUs as the host are capable
>> -     * of emulate.
>> +     * While KVM determines max cpus in kvm_init() using 
>> kvm_max_vcpus(),
>> +     * In TCG the limit is restricted by the range of CPU IPIs 
>> available.
>>        */
>> -    mc->max_cpus = INT32_MAX;
>> +    mc->max_cpus = SPAPR_IRQ_NR_IPIS;
>>         mc->no_parallel = 1;
>>       mc->default_boot_order = "";
>
> 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>




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