[PATCH] xen/arm: Allow setting the number of CPUs to activate at runtime

[PATCH] xen/arm: Allow setting the number of CPUs to activate at runtime

[Michal Orzel]

Introduce a command line parameter "maxcpus" on Arm to allow adjusting
the number of CPUs to activate. Currently the limit is defined by the
config option CONFIG_NR_CPUS. Such parameter already exists on x86.

Define a parameter "maxcpus" and a corresponding static variable
max_cpus in Arm smpboot.c. Modify function smp_get_max_cpus to take
max_cpus as a limit and to return proper unsigned int instead of int.

Take the opportunity to remove redundant variable cpus from start_xen
function and to directly assign the return value from smp_get_max_cpus
to nr_cpu_ids (global variable in Xen used to store the number of CPUs
actually activated).

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
max_cpus is also defined in x86 setup.c. It would be possible to join
these definitions in xen/common/cpu.c. However in that case, max_cpus
would become global which is not really what we want. There is already
global nr_cpu_ids used everywhere and max_cpus being used only in x86
start_xen and Arm smp_get_max_cpus should be kept local. Also there are
already lots of places in Xen using max_cpus (local versions) and that
would start to be hard to read (variable shadowing).
---
 docs/misc/xen-command-line.pandoc |  2 +-
 xen/arch/arm/include/asm/smp.h    |  2 +-
 xen/arch/arm/setup.c              | 10 ++++------
 xen/arch/arm/smpboot.c            | 18 ++++++++++++------
 4 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 1dc7e1ca07..a40d0ae2e8 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1651,7 +1651,7 @@ with **crashinfo_maxaddr**.
 Specify the threshold below which Xen will inform dom0 that the quantity of
 free memory is getting low.  Specifying `0` will disable this notification.
 
-### maxcpus (x86)
+### maxcpus
 > `= <integer>`
 
 Specify the maximum number of CPUs that should be brought up.
diff --git a/xen/arch/arm/include/asm/smp.h b/xen/arch/arm/include/asm/smp.h
index 83c0cd6976..8133d5c295 100644
--- a/xen/arch/arm/include/asm/smp.h
+++ b/xen/arch/arm/include/asm/smp.h
@@ -33,7 +33,7 @@ extern void init_secondary(void);
 
 extern void smp_init_cpus(void);
 extern void smp_clear_cpu_maps (void);
-extern int smp_get_max_cpus (void);
+extern unsigned int smp_get_max_cpus(void);
 
 #define cpu_physical_id(cpu) cpu_logical_map(cpu)
 
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index d5d0792ed4..b8d97950b7 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -862,11 +862,10 @@ void __init start_xen(unsigned long boot_phys_offset,
                       unsigned long fdt_paddr)
 {
     size_t fdt_size;
-    int cpus, i;
     const char *cmdline;
     struct bootmodule *xen_bootmodule;
     struct domain *d;
-    int rc;
+    int rc, i;
 
     dcache_line_bytes = read_dcache_line_bytes();
 
@@ -942,9 +941,8 @@ void __init start_xen(unsigned long boot_phys_offset,
     processor_id();
 
     smp_init_cpus();
-    cpus = smp_get_max_cpus();
-    printk(XENLOG_INFO "SMP: Allowing %u CPUs\n", cpus);
-    nr_cpu_ids = cpus;
+    nr_cpu_ids = smp_get_max_cpus();
+    printk(XENLOG_INFO "SMP: Allowing %u CPUs\n", nr_cpu_ids);
 
     /*
      * Some errata relies on SMCCC version which is detected by psci_init()
@@ -988,7 +986,7 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     for_each_present_cpu ( i )
     {
-        if ( (num_online_cpus() < cpus) && !cpu_online(i) )
+        if ( (num_online_cpus() < nr_cpu_ids) && !cpu_online(i) )
         {
             int ret = cpu_up(i);
             if ( ret != 0 )
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 9bb32a301a..22fede6600 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -43,6 +43,10 @@ cpumask_t cpu_possible_map;
 
 struct cpuinfo_arm cpu_data[NR_CPUS];
 
+/* maxcpus: maximum number of CPUs to activate. */
+static unsigned int __initdata max_cpus;
+integer_param("maxcpus", max_cpus);
+
 /* CPU logical map: map xen cpuid to an MPIDR */
 register_t __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = MPIDR_INVALID };
 
@@ -277,16 +281,18 @@ void __init smp_init_cpus(void)
                     "unless the cpu affinity of all domains is specified.\n");
 }
 
-int __init
-smp_get_max_cpus (void)
+unsigned int __init smp_get_max_cpus(void)
 {
-    int i, max_cpus = 0;
+    unsigned int i, cpus = 0;
+
+    if ( ( !max_cpus ) || ( max_cpus > nr_cpu_ids ) )
+        max_cpus = nr_cpu_ids;
 
-    for ( i = 0; i < nr_cpu_ids; i++ )
+    for ( i = 0; i < max_cpus; i++ )
         if ( cpu_possible(i) )
-            max_cpus++;
+            cpus++;
 
-    return max_cpus;
+    return cpus;
 }
 
 void __init
-- 
2.25.1

Re: [PATCH] xen/arm: Allow setting the number of CPUs to activate at runtime

[Julien Grall]

Hi,

On 23/05/2022 10:13, Michal Orzel wrote:
> Introduce a command line parameter "maxcpus" on Arm to allow adjusting
> the number of CPUs to activate.

The current definition "maxcpus" is not really suitable for big.LITTLE 
systems as you have no flexibility to say how many types of each cores 
you want to boot.

Instead, Xen will pick-up the first CPUs it parsed from the firmware tables.


So what's your use-case/target?

> Currently the limit is defined by the
> config option CONFIG_NR_CPUS. Such parameter already exists on x86.
> 
> Define a parameter "maxcpus" and a corresponding static variable
> max_cpus in Arm smpboot.c. Modify function smp_get_max_cpus to take
> max_cpus as a limit and to return proper unsigned int instead of int.
> 
> Take the opportunity to remove redundant variable cpus from start_xen
> function and to directly assign the return value from smp_get_max_cpus
> to nr_cpu_ids (global variable in Xen used to store the number of CPUs
> actually activated).
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> ---
> max_cpus is also defined in x86 setup.c. It would be possible to join
> these definitions in xen/common/cpu.c. However in that case, max_cpus
> would become global which is not really what we want.

If we move the global variable, then I would also expect to move the 
parsing parsing (i.e. smp_get_max_cpus()). So why would max_cpus end up 
to be global? Is it because the x86 would continue to use it?

> There is already
> global nr_cpu_ids used everywhere and max_cpus being used only in x86
> start_xen and Arm smp_get_max_cpus should be kept local. Also there are
> already lots of places in Xen using max_cpus (local versions) and that
> would start to be hard to read (variable shadowing).

We should avoid variable shadowing.

> ---
>   docs/misc/xen-command-line.pandoc |  2 +-
>   xen/arch/arm/include/asm/smp.h    |  2 +-
>   xen/arch/arm/setup.c              | 10 ++++------
>   xen/arch/arm/smpboot.c            | 18 ++++++++++++------
>   4 files changed, 18 insertions(+), 14 deletions(-)

The patch looks ok to me (see one coding style comment below). I haven't 
acked it because I am waiting to get more input on your use-cases.


[...]

> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 9bb32a301a..22fede6600 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -43,6 +43,10 @@ cpumask_t cpu_possible_map;
>   
>   struct cpuinfo_arm cpu_data[NR_CPUS];
>   
> +/* maxcpus: maximum number of CPUs to activate. */
> +static unsigned int __initdata max_cpus;
> +integer_param("maxcpus", max_cpus);
> +
>   /* CPU logical map: map xen cpuid to an MPIDR */
>   register_t __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = MPIDR_INVALID };
>   
> @@ -277,16 +281,18 @@ void __init smp_init_cpus(void)
>                       "unless the cpu affinity of all domains is specified.\n");
>   }
>   
> -int __init
> -smp_get_max_cpus (void)
> +unsigned int __init smp_get_max_cpus(void)
>   {
> -    int i, max_cpus = 0;
> +    unsigned int i, cpus = 0;
> +
> +    if ( ( !max_cpus ) || ( max_cpus > nr_cpu_ids ) )

Coding style: We don't add space in the inner parentheses. I.e:

if ( (!max_cpus) || (max_cpus > nr_cpu_ids) )

> +        max_cpus = nr_cpu_ids;
>   
> -    for ( i = 0; i < nr_cpu_ids; i++ )
> +    for ( i = 0; i < max_cpus; i++ )
>           if ( cpu_possible(i) )
> -            max_cpus++;
> +            cpus++;
>   
> -    return max_cpus;
> +    return cpus;
>   }
>   
>   void __init

-- 
Julien Grall

Re: [PATCH] xen/arm: Allow setting the number of CPUs to activate at runtime

[Michal Orzel]

Hi Julien,

On 23.05.2022 12:05, Julien Grall wrote:
> Hi,
> 
> On 23/05/2022 10:13, Michal Orzel wrote:
>> Introduce a command line parameter "maxcpus" on Arm to allow adjusting
>> the number of CPUs to activate.
> 
> The current definition "maxcpus" is not really suitable for big.LITTLE systems as you have no flexibility to say how many types of each cores you want to boot.
> 
> Instead, Xen will pick-up the first CPUs it parsed from the firmware tables.
> 
> 
> So what's your use-case/target?
> 
- use cases where we have no big little (although even on big.LITTLE limiting this number makes sense if we do not care about the types)
- debug cases where we want to set maxcpus=1

>> Currently the limit is defined by the
>> config option CONFIG_NR_CPUS. Such parameter already exists on x86.
>>
>> Define a parameter "maxcpus" and a corresponding static variable
>> max_cpus in Arm smpboot.c. Modify function smp_get_max_cpus to take
>> max_cpus as a limit and to return proper unsigned int instead of int.
>>
>> Take the opportunity to remove redundant variable cpus from start_xen
>> function and to directly assign the return value from smp_get_max_cpus
>> to nr_cpu_ids (global variable in Xen used to store the number of CPUs
>> actually activated).
>>
>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>> ---
>> max_cpus is also defined in x86 setup.c. It would be possible to join
>> these definitions in xen/common/cpu.c. However in that case, max_cpus
>> would become global which is not really what we want.
> 
> If we move the global variable, then I would also expect to move the parsing parsing (i.e. smp_get_max_cpus()). So why would max_cpus end up to be global? Is it because the x86 would continue to use it?
> 
Yes, that would involve more x86 modifications that actual Arm coding. That is why I wanted to avoid it.

>> There is already
>> global nr_cpu_ids used everywhere and max_cpus being used only in x86
>> start_xen and Arm smp_get_max_cpus should be kept local. Also there are
>> already lots of places in Xen using max_cpus (local versions) and that
>> would start to be hard to read (variable shadowing).
> 
> We should avoid variable shadowing.
> 
>> ---
>>   docs/misc/xen-command-line.pandoc |  2 +-
>>   xen/arch/arm/include/asm/smp.h    |  2 +-
>>   xen/arch/arm/setup.c              | 10 ++++------
>>   xen/arch/arm/smpboot.c            | 18 ++++++++++++------
>>   4 files changed, 18 insertions(+), 14 deletions(-)
> 
> The patch looks ok to me (see one coding style comment below). I haven't acked it because I am waiting to get more input on your use-cases.
> 
> 
> [...]
> 
>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>> index 9bb32a301a..22fede6600 100644
>> --- a/xen/arch/arm/smpboot.c
>> +++ b/xen/arch/arm/smpboot.c
>> @@ -43,6 +43,10 @@ cpumask_t cpu_possible_map;
>>     struct cpuinfo_arm cpu_data[NR_CPUS];
>>   +/* maxcpus: maximum number of CPUs to activate. */
>> +static unsigned int __initdata max_cpus;
>> +integer_param("maxcpus", max_cpus);
>> +
>>   /* CPU logical map: map xen cpuid to an MPIDR */
>>   register_t __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = MPIDR_INVALID };
>>   @@ -277,16 +281,18 @@ void __init smp_init_cpus(void)
>>                       "unless the cpu affinity of all domains is specified.\n");
>>   }
>>   -int __init
>> -smp_get_max_cpus (void)
>> +unsigned int __init smp_get_max_cpus(void)
>>   {
>> -    int i, max_cpus = 0;
>> +    unsigned int i, cpus = 0;
>> +
>> +    if ( ( !max_cpus ) || ( max_cpus > nr_cpu_ids ) )
> 
> Coding style: We don't add space in the inner parentheses. I.e:
Noted, thanks.

> 
> if ( (!max_cpus) || (max_cpus > nr_cpu_ids) )
> 
>> +        max_cpus = nr_cpu_ids;
>>   -    for ( i = 0; i < nr_cpu_ids; i++ )
>> +    for ( i = 0; i < max_cpus; i++ )
>>           if ( cpu_possible(i) )
>> -            max_cpus++;
>> +            cpus++;
>>   -    return max_cpus;
>> +    return cpus;
>>   }
>>     void __init
> 

Cheers,
Michal

Re: [PATCH] xen/arm: Allow setting the number of CPUs to activate at runtime

[Julien Grall]

On 23/05/2022 11:21, Michal Orzel wrote:
> Hi Julien,

Hi Michal,

> 
> On 23.05.2022 12:05, Julien Grall wrote:
>> Hi,
>>
>> On 23/05/2022 10:13, Michal Orzel wrote:
>>> Introduce a command line parameter "maxcpus" on Arm to allow adjusting
>>> the number of CPUs to activate.
>>
>> The current definition "maxcpus" is not really suitable for big.LITTLE systems as you have no flexibility to say how many types of each cores you want to boot.
>>
>> Instead, Xen will pick-up the first CPUs it parsed from the firmware tables.
>>
>>
>> So what's your use-case/target?
>>
> - use cases where we have no big little (although even on big.LITTLE limiting this number makes sense if we do not care about the types)

This may make sense in debug build, but for prod I think you need some 
certainty how which CPUs you are going to use.

So I would like a warning in the documentation "maxcpus" that in 
big.LITTLE system, there are no guarantee on which types will be used.

This is technically a lie, but I don't want a user to start relying on 
how Xen will parse the DT.

> - debug cases where we want to set maxcpus=1

Thanks for the clarification. I would be happy to add my tag with a 
warning in the documentation.

Cheers,

-- 
Julien Grall

Re: [PATCH] xen/arm: Allow setting the number of CPUs to activate at runtime

[Michal Orzel]

Hi Julien,

On 23.05.2022 22:00, Julien Grall wrote:
> 
> 
> On 23/05/2022 11:21, Michal Orzel wrote:
>> Hi Julien,
> 
> Hi Michal,
> 
>>
>> On 23.05.2022 12:05, Julien Grall wrote:
>>> Hi,
>>>
>>> On 23/05/2022 10:13, Michal Orzel wrote:
>>>> Introduce a command line parameter "maxcpus" on Arm to allow adjusting
>>>> the number of CPUs to activate.
>>>
>>> The current definition "maxcpus" is not really suitable for big.LITTLE systems as you have no flexibility to say how many types of each cores you want to boot.
>>>
>>> Instead, Xen will pick-up the first CPUs it parsed from the firmware tables.
>>>
>>>
>>> So what's your use-case/target?
>>>
>> - use cases where we have no big little (although even on big.LITTLE limiting this number makes sense if we do not care about the types)
> 
> This may make sense in debug build, but for prod I think you need some certainty how which CPUs you are going to use.
My conviction was that using big.LITTLE by enabling hmp-unsafe is not really used in the production systems (after all it's called *unsafe*)
as it may easily end up in an insecure/unstable platform without specifying the cpu affinity (which must be done carefully).

> 
> So I would like a warning in the documentation "maxcpus" that in big.LITTLE system, there are no guarantee on which types will be used.
I'm fully ok with adding this warning.

**WARNING: On Arm big.LITTLE systems, when `hmp-unsafe` option is enabled, this command line
option does not guarantee on which CPU types will be used.**

> 
> This is technically a lie, but I don't want a user to start relying on how Xen will parse the DT.
> 
>> - debug cases where we want to set maxcpus=1
> 
> Thanks for the clarification. I would be happy to add my tag with a warning in the documentation.
> 
Does it mean you want to do this on commit or should I handle it in v2?

> Cheers,
> 

Cheers,
Michal

Re: [PATCH] xen/arm: Allow setting the number of CPUs to activate at runtime

[Michal Orzel]

Hi Julien,

Gentle ping asking to reply to my previous mail.

On 24.05.2022 08:34, Michal Orzel wrote:
> Hi Julien,
> 
> On 23.05.2022 22:00, Julien Grall wrote:
>>
>>
>> On 23/05/2022 11:21, Michal Orzel wrote:
>>> Hi Julien,
>>
>> Hi Michal,
>>
>>>
>>> On 23.05.2022 12:05, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 23/05/2022 10:13, Michal Orzel wrote:
>>>>> Introduce a command line parameter "maxcpus" on Arm to allow adjusting
>>>>> the number of CPUs to activate.
>>>>
>>>> The current definition "maxcpus" is not really suitable for big.LITTLE systems as you have no flexibility to say how many types of each cores you want to boot.
>>>>
>>>> Instead, Xen will pick-up the first CPUs it parsed from the firmware tables.
>>>>
>>>>
>>>> So what's your use-case/target?
>>>>
>>> - use cases where we have no big little (although even on big.LITTLE limiting this number makes sense if we do not care about the types)
>>
>> This may make sense in debug build, but for prod I think you need some certainty how which CPUs you are going to use.
> My conviction was that using big.LITTLE by enabling hmp-unsafe is not really used in the production systems (after all it's called *unsafe*)
> as it may easily end up in an insecure/unstable platform without specifying the cpu affinity (which must be done carefully).
> 
>>
>> So I would like a warning in the documentation "maxcpus" that in big.LITTLE system, there are no guarantee on which types will be used.
> I'm fully ok with adding this warning.
> 
> **WARNING: On Arm big.LITTLE systems, when `hmp-unsafe` option is enabled, this command line
> option does not guarantee on which CPU types will be used.**
> 
>>
>> This is technically a lie, but I don't want a user to start relying on how Xen will parse the DT.
>>
>>> - debug cases where we want to set maxcpus=1
>>
>> Thanks for the clarification. I would be happy to add my tag with a warning in the documentation.
>>
> Does it mean you want to do this on commit or should I handle it in v2?
> 
>> Cheers,
>>
> 
> Cheers,
> Michal
> 

Re: [PATCH] xen/arm: Allow setting the number of CPUs to activate at runtime

[Julien Grall]

On 24/05/2022 07:34, Michal Orzel wrote:
> Hi Julien,

Hi Michal,

> 
> On 23.05.2022 22:00, Julien Grall wrote:
>>
>>
>> On 23/05/2022 11:21, Michal Orzel wrote:
>>> Hi Julien,
>>
>> Hi Michal,
>>
>>>
>>> On 23.05.2022 12:05, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 23/05/2022 10:13, Michal Orzel wrote:
>>>>> Introduce a command line parameter "maxcpus" on Arm to allow adjusting
>>>>> the number of CPUs to activate.
>>>>
>>>> The current definition "maxcpus" is not really suitable for big.LITTLE systems as you have no flexibility to say how many types of each cores you want to boot.
>>>>
>>>> Instead, Xen will pick-up the first CPUs it parsed from the firmware tables.
>>>>
>>>>
>>>> So what's your use-case/target?
>>>>
>>> - use cases where we have no big little (although even on big.LITTLE limiting this number makes sense if we do not care about the types)
>>
>> This may make sense in debug build, but for prod I think you need some certainty how which CPUs you are going to use.
> My conviction was that using big.LITTLE by enabling hmp-unsafe is not really used in the production systems (after all it's called *unsafe*)
> as it may easily end up in an insecure/unstable platform without specifying the cpu affinity (which must be done carefully).
> 
>>
>> So I would like a warning in the documentation "maxcpus" that in big.LITTLE system, there are no guarantee on which types will be used.
> I'm fully ok with adding this warning.
> 
> **WARNING: On Arm big.LITTLE systems, when `hmp-unsafe` option is enabled, this command line
> option does not guarantee on which CPU types will be used.**

NIT: s/on//

> 
>>
>> This is technically a lie, but I don't want a user to start relying on how Xen will parse the DT.
>>
>>> - debug cases where we want to set maxcpus=1
>>
>> Thanks for the clarification. I would be happy to add my tag with a warning in the documentation.
>>
> Does it mean you want to do this on commit or should I handle it in v2?

It depends whether there are other comments on the series. If there are 
none, then I will do it on commit.

I will wait until next week to give a chance to Bertrand/Stefano to comment.

Cheers,

-- 
Julien Grall

Re: [PATCH] xen/arm: Allow setting the number of CPUs to activate at runtime

[Bertrand Marquis]

Hi Michal,

> On 23 May 2022, at 10:13, Michal Orzel <Michal.Orzel@arm.com> wrote:
> 
> Introduce a command line parameter "maxcpus" on Arm to allow adjusting
> the number of CPUs to activate. Currently the limit is defined by the
> config option CONFIG_NR_CPUS. Such parameter already exists on x86.
> 
> Define a parameter "maxcpus" and a corresponding static variable
> max_cpus in Arm smpboot.c. Modify function smp_get_max_cpus to take
> max_cpus as a limit and to return proper unsigned int instead of int.
> 
> Take the opportunity to remove redundant variable cpus from start_xen
> function and to directly assign the return value from smp_get_max_cpus
> to nr_cpu_ids (global variable in Xen used to store the number of CPUs
> actually activated).
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>

With the warning added in the documentation (which is ok to do on commit):

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

Re: [PATCH] xen/arm: Allow setting the number of CPUs to activate at runtime

[Julien Grall]

Hi,

On 30/05/2022 10:09, Bertrand Marquis wrote:
>> On 23 May 2022, at 10:13, Michal Orzel <Michal.Orzel@arm.com> wrote:
>>
>> Introduce a command line parameter "maxcpus" on Arm to allow adjusting
>> the number of CPUs to activate. Currently the limit is defined by the
>> config option CONFIG_NR_CPUS. Such parameter already exists on x86.
>>
>> Define a parameter "maxcpus" and a corresponding static variable
>> max_cpus in Arm smpboot.c. Modify function smp_get_max_cpus to take
>> max_cpus as a limit and to return proper unsigned int instead of int.
>>
>> Take the opportunity to remove redundant variable cpus from start_xen
>> function and to directly assign the return value from smp_get_max_cpus
>> to nr_cpu_ids (global variable in Xen used to store the number of CPUs
>> actually activated).
>>
>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> 
> With the warning added in the documentation (which is ok to do on commit):
> 
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

I have committed it with the update in the documentation.

Cheers,

-- 
Julien Grall