[PATCH RESEND v2 0/2] Introduce SPAPR_NR_IPIS and fix max-cpus

[PATCH RESEND v2 0/2] Introduce SPAPR_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_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().

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

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

-- 
2.39.3

[PATCH v2 1/2] ppc/spapr: Introduce SPAPR_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>
---
 hw/ppc/spapr_irq.c         | 4 ++--
 include/hw/ppc/spapr_irq.h | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index a0d1e1298e..0c5db6b161 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -329,7 +329,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_NR_IPIS);
         /*
          * 8 XIVE END structures per CPU. One for each available
          * priority
@@ -356,7 +356,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_NR_IPIS);
 
     /*
      * Mostly we don't actually need this until reset, except that not
diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
index c22a72c9e2..e7a80a8349 100644
--- a/include/hw/ppc/spapr_irq.h
+++ b/include/hw/ppc/spapr_irq.h
@@ -28,6 +28,7 @@
 #define SPAPR_IRQ_MSI        (SPAPR_XIRQ_BASE + 0x0300)
 
 #define SPAPR_NR_XIRQS       0x1000
+#define SPAPR_NR_IPIS        0x1000
 
 struct SpaprMachineState;
 
-- 
2.39.3

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

[Philippe Mathieu-Daudé]

On 22/11/23 10:28, 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>
> ---
>   hw/ppc/spapr_irq.c         | 4 ++--
>   include/hw/ppc/spapr_irq.h | 1 +
>   2 files changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

[Philippe Mathieu-Daudé]

Hi Harsh,

On 22/11/23 10:28, 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>
> ---
>   hw/ppc/spapr_irq.c         | 4 ++--
>   include/hw/ppc/spapr_irq.h | 1 +
>   2 files changed, 3 insertions(+), 2 deletions(-)


> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index c22a72c9e2..e7a80a8349 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -28,6 +28,7 @@
>   #define SPAPR_IRQ_MSI        (SPAPR_XIRQ_BASE + 0x0300)
>   
>   #define SPAPR_NR_XIRQS       0x1000
> +#define SPAPR_NR_IPIS        0x1000

BTW why hexadecimal and not decimal?

Regards,

Phil.

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

[Cédric Le Goater]

On 11/22/23 12:13, Philippe Mathieu-Daudé wrote:
> Hi Harsh,
> 
> On 22/11/23 10:28, 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>
>> ---
>>   hw/ppc/spapr_irq.c         | 4 ++--
>>   include/hw/ppc/spapr_irq.h | 1 +
>>   2 files changed, 3 insertions(+), 2 deletions(-)
> 
> 
>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
>> index c22a72c9e2..e7a80a8349 100644
>> --- a/include/hw/ppc/spapr_irq.h
>> +++ b/include/hw/ppc/spapr_irq.h
>> @@ -28,6 +28,7 @@
>>   #define SPAPR_IRQ_MSI        (SPAPR_XIRQ_BASE + 0x0300)
>>   #define SPAPR_NR_XIRQS       0x1000
>> +#define SPAPR_NR_IPIS        0x1000
> 
> BTW why hexadecimal and not decimal?

I think because the HW IRQs are displayed in hex under Linux. Probably.
It has been a while.

Thanks,

C.

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

[Cédric Le Goater]

Hello Harsh,

Please add to your .git/config file:

[diff]
	orderFile = /path/to/qemu/scripts/git.orderfile


On 11/22/23 10:28, 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>
> ---
>   hw/ppc/spapr_irq.c         | 4 ++--
>   include/hw/ppc/spapr_irq.h | 1 +
>   2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index a0d1e1298e..0c5db6b161 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -329,7 +329,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_NR_IPIS);

SPAPR_IRQ_NR_IPIS ?

>           /*
>            * 8 XIVE END structures per CPU. One for each available
>            * priority
> @@ -356,7 +356,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_NR_IPIS);
>   
>       /*
>        * Mostly we don't actually need this until reset, except that not
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index c22a72c9e2..e7a80a8349 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -28,6 +28,7 @@

In include/hw/ppc/spapr_irq.h, we should describe the ranges a bit more.
See commit dcc345b61ebe and ad8de98636e7 for more info. Something like :

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


And to make sure the ranges don't overlap, let's add :

   QEMU_BUILD_BUG_ON(SPAPR_IRQ_NR_IPIS > SPAPR_XIRQ_BASE)


Thanks,

C.

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

[Harsh Prateek Bora]

On 11/22/23 17:01, Cédric Le Goater wrote:
> Hello Harsh,
> 
> Please add to your .git/config file:
> 
> [diff]
>      orderFile = /path/to/qemu/scripts/git.orderfile
> 

Sure, thanks for the suggestion.

> 
> On 11/22/23 10:28, 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>
>> ---
>>   hw/ppc/spapr_irq.c         | 4 ++--
>>   include/hw/ppc/spapr_irq.h | 1 +
>>   2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>> index a0d1e1298e..0c5db6b161 100644
>> --- a/hw/ppc/spapr_irq.c
>> +++ b/hw/ppc/spapr_irq.c
>> @@ -329,7 +329,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_NR_IPIS);
> 
> SPAPR_IRQ_NR_IPIS ?
> 
>>           /*
>>            * 8 XIVE END structures per CPU. One for each available
>>            * priority
>> @@ -356,7 +356,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_NR_IPIS);
>>       /*
>>        * Mostly we don't actually need this until reset, except that not
>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
>> index c22a72c9e2..e7a80a8349 100644
>> --- a/include/hw/ppc/spapr_irq.h
>> +++ b/include/hw/ppc/spapr_irq.h
>> @@ -28,6 +28,7 @@
> 
> In include/hw/ppc/spapr_irq.h, we should describe the ranges a bit more.
> See commit dcc345b61ebe and ad8de98636e7 for more info. Something like :
> 
>    /*
>     * 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 */
> 
> 
> And to make sure the ranges don't overlap, let's add :
> 
>    QEMU_BUILD_BUG_ON(SPAPR_IRQ_NR_IPIS > SPAPR_XIRQ_BASE)
> 

Yeh, this looks much better. Will update and post.

regards,
Harsh
> 
> Thanks,
> 
> C.
> 
> 

[PATCH v2 2/2] ppc/spapr: Initialize max_cpus limit to SPAPR_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..0de11a4458 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_NR_IPIS;
 
     mc->no_parallel = 1;
     mc->default_boot_order = "";
-- 
2.39.3

Re: [PATCH v2 2/2] ppc/spapr: Initialize max_cpus limit to SPAPR_NR_IPIS.

[Philippe Mathieu-Daudé]

Hi Harsh,

On 22/11/23 10:28, 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>
> ---
>   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..0de11a4458 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_NR_IPIS;

Is SPAPR_NR_IPIS also the upper limit for KVM?

>       mc->no_parallel = 1;
>       mc->default_boot_order = "";

Re: [PATCH v2 2/2] ppc/spapr: Initialize max_cpus limit to SPAPR_NR_IPIS.

[Harsh Prateek Bora]

Hi Philippe,

On 11/22/23 16:46, Philippe Mathieu-Daudé wrote:
> Hi Harsh,
> 
> On 22/11/23 10:28, 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>
>> ---
>>   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..0de11a4458 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_NR_IPIS;
> 
> Is SPAPR_NR_IPIS also the upper limit for KVM?

In KVM mode, the limit is restricted to what is supported by KVM which 
is checked using kvm_ioctl via wrappers in kvm_init and appears to be 
evaluating to 2048. So, having a larger default works for both case.

regards,
Harsh

> 
>>       mc->no_parallel = 1;
>>       mc->default_boot_order = "";
> 

Re: [PATCH v2 2/2] ppc/spapr: Initialize max_cpus limit to SPAPR_NR_IPIS.

[Cédric Le Goater]

On 11/23/23 06:03, Harsh Prateek Bora wrote:
> Hi Philippe,
> 
> On 11/22/23 16:46, Philippe Mathieu-Daudé wrote:
>> Hi Harsh,
>>
>> On 22/11/23 10:28, 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>
>>> ---
>>>   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..0de11a4458 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_NR_IPIS;
>>
>> Is SPAPR_NR_IPIS also the upper limit for KVM?
> 
> In KVM mode, the limit is restricted to what is supported by KVM which is checked using kvm_ioctl via wrappers in kvm_init and appears to be evaluating to 2048. So, having a larger default works for both case.

QEMU sets the number of cpus with KVM ioctls :

	KVM_DEV_XICS_NR_SERVERS
	KVM_DEV_XIVE_NR_SERVERS

This is important for the host since the interrupt controller is then
configured with these values through FW.

The default value is indeed 2K but this is large and wastes a lot of
HW resources, page mappings, etc.

Thanks,

C.

Re: [PATCH v2 2/2] ppc/spapr: Initialize max_cpus limit to SPAPR_NR_IPIS.

[Philippe Mathieu-Daudé]

On 23/11/23 09:47, Cédric Le Goater wrote:
> On 11/23/23 06:03, Harsh Prateek Bora wrote:
>> Hi Philippe,
>>
>> On 11/22/23 16:46, Philippe Mathieu-Daudé wrote:
>>> Hi Harsh,
>>>
>>> On 22/11/23 10:28, 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>
>>>> ---
>>>>   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..0de11a4458 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_NR_IPIS;
>>>
>>> Is SPAPR_NR_IPIS also the upper limit for KVM?
>>
>> In KVM mode, the limit is restricted to what is supported by KVM which 
>> is checked using kvm_ioctl via wrappers in kvm_init and appears to be 
>> evaluating to 2048. So, having a larger default works for both case.
> 
> QEMU sets the number of cpus with KVM ioctls :
> 
>      KVM_DEV_XICS_NR_SERVERS
>      KVM_DEV_XIVE_NR_SERVERS
> 
> This is important for the host since the interrupt controller is then
> configured with these values through FW.
> 
> The default value is indeed 2K but this is large and wastes a lot of
> HW resources, page mappings, etc.

I was wondering if one day KVM raise its limit to 5k, then the
machine will clamp to 4k, and someone will have to debug that.
Not a big deal ;)

Re: [PATCH v2 2/2] ppc/spapr: Initialize max_cpus limit to SPAPR_NR_IPIS.

[Cédric Le Goater]

On 11/23/23 11:26, Philippe Mathieu-Daudé wrote:
> On 23/11/23 09:47, Cédric Le Goater wrote:
>> On 11/23/23 06:03, Harsh Prateek Bora wrote:
>>> Hi Philippe,
>>>
>>> On 11/22/23 16:46, Philippe Mathieu-Daudé wrote:
>>>> Hi Harsh,
>>>>
>>>> On 22/11/23 10:28, 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>
>>>>> ---
>>>>>   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..0de11a4458 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_NR_IPIS;
>>>>
>>>> Is SPAPR_NR_IPIS also the upper limit for KVM?
>>>
>>> In KVM mode, the limit is restricted to what is supported by KVM which is checked using kvm_ioctl via wrappers in kvm_init and appears to be evaluating to 2048. So, having a larger default works for both case.
>>
>> QEMU sets the number of cpus with KVM ioctls :
>>
>>      KVM_DEV_XICS_NR_SERVERS
>>      KVM_DEV_XIVE_NR_SERVERS
>>
>> This is important for the host since the interrupt controller is then
>> configured with these values through FW.
>>
>> The default value is indeed 2K but this is large and wastes a lot of
>> HW resources, page mappings, etc.
> 
> I was wondering if one day KVM raise its limit to 5k, then the
> machine will clamp to 4k, and someone will have to debug that.
> Not a big deal ;)

Changing the number of CPUs will require some work in Linux first.
I think we (as ppc) tried to push it to 8K but there was some push
back upstream. Anyhow, there are DT issues also, memory layout, etc.
It won't happen without being noticed I am sure :)

Anyhow, If we need more IPIs to support more CPUs, the IRQ number
space will need an extra range after the device range to preserve
compatibility.

Thanks,

C.

[PATCH v2 1/2] ppc/spapr: Introduce SPAPR_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>
---
 hw/ppc/spapr_irq.c         | 4 ++--
 include/hw/ppc/spapr_irq.h | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index a0d1e1298e..0c5db6b161 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -329,7 +329,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_NR_IPIS);
         /*
          * 8 XIVE END structures per CPU. One for each available
          * priority
@@ -356,7 +356,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_NR_IPIS);
 
     /*
      * Mostly we don't actually need this until reset, except that not
diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
index c22a72c9e2..e7a80a8349 100644
--- a/include/hw/ppc/spapr_irq.h
+++ b/include/hw/ppc/spapr_irq.h
@@ -28,6 +28,7 @@
 #define SPAPR_IRQ_MSI        (SPAPR_XIRQ_BASE + 0x0300)
 
 #define SPAPR_NR_XIRQS       0x1000
+#define SPAPR_NR_IPIS        0x1000
 
 struct SpaprMachineState;
 
-- 
2.39.3