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

[Julien Grall <julien@xxxxxxx>]

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