Re: [XEN RFC PATCH 39/40] xen/x86: move numa_setup to common to support NUMA switch in command line

[Julien Grall <julien@xxxxxxx>]

Hi Wei,

On 11/08/2021 11:24, Wei Chen wrote:
> Xen x86 has created a command line parameter "numa" as NUMA switch for
> user to turn on/off NUMA. As device tree based NUMA has been enabled
> for Arm, this parameter can be reused by Arm. So in this patch, we move
> this parameter to common.
>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> ---
>   xen/arch/x86/numa.c    | 34 ----------------------------------
>   xen/common/numa.c      | 35 ++++++++++++++++++++++++++++++++++-
>   xen/include/xen/numa.h |  1 -
>   3 files changed, 34 insertions(+), 36 deletions(-)
>
> diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
> index 8b43be4aa7..380d8ed6fd 100644
> --- a/xen/arch/x86/numa.c
> +++ b/xen/arch/x86/numa.c
> @@ -11,7 +11,6 @@
>   #include <xen/nodemask.h>
>   #include <xen/numa.h>
>   #include <xen/keyhandler.h>
> -#include <xen/param.h>
>   #include <xen/time.h>
>   #include <xen/smp.h>
>   #include <xen/pfn.h>
> @@ -19,9 +18,6 @@
>   #include <xen/sched.h>
>   #include <xen/softirq.h>
>   
> -static int numa_setup(const char *s);
> -custom_param("numa", numa_setup);
> -
>   #ifndef Dprintk
>   #define Dprintk(x...)
>   #endif
> @@ -50,35 +46,6 @@ void numa_set_node(int cpu, nodeid_t node)
>       cpu_to_node[cpu] = node;
>   }
>   
> -/* [numa=off] */
> -static __init int numa_setup(const char *opt)
> -{
> -    if ( !strncmp(opt,"off",3) )
> -        numa_off = true;
> -    else if ( !strncmp(opt,"on",2) )
> -        numa_off = false;
> -#ifdef CONFIG_NUMA_EMU
> -    else if ( !strncmp(opt, "fake=", 5) )
> -    {
> -        numa_off = false;
> -        numa_fake = simple_strtoul(opt+5,NULL,0);
> -        if ( numa_fake >= MAX_NUMNODES )
> -            numa_fake = MAX_NUMNODES;
> -    }
> -#endif
> -#ifdef CONFIG_ACPI_NUMA
> -    else if ( !strncmp(opt,"noacpi",6) )
> -    {
> -        numa_off = false;
> -        acpi_numa = -1;
> -    }
> -#endif
> -    else
> -        return -EINVAL;
> -
> -    return 0;
> -}
> -
>   /*
>    * Setup early cpu_to_node.
>    *
> @@ -287,4 +254,3 @@ static __init int register_numa_trigger(void)
>       return 0;
>   }
>   __initcall(register_numa_trigger);
> -
> diff --git a/xen/common/numa.c b/xen/common/numa.c
> index 88f1594127..c98eb8d571 100644
> --- a/xen/common/numa.c
> +++ b/xen/common/numa.c
> @@ -14,8 +14,12 @@
>   #include <xen/smp.h>
>   #include <xen/pfn.h>
>   #include <xen/sched.h>
> +#include <xen/param.h>
>   #include <asm/acpi.h>
>   
> +static int numa_setup(const char *s);
> +custom_param("numa", numa_setup);
> +
>   struct node_data node_data[MAX_NUMNODES];
>   
>   /* Mapping from pdx to node id */
> @@ -324,7 +328,7 @@ int __init numa_scan_nodes(u64 start, u64 end)
>   }
>   
>   #ifdef CONFIG_NUMA_EMU
> -int numa_fake __initdata = 0;
> +static int numa_fake __initdata = 0;
>   
>   /* Numa emulation */
>   static int __init numa_emulation(u64 start_pfn, u64 end_pfn)
> @@ -409,3 +413,32 @@ void __init numa_initmem_init(unsigned long start_pfn, 
> unsigned long end_pfn)
>       /* architecture specified fallback operations */
>       arch_numa_init_failed_fallback();
>   }
> +
> +/* [numa=off] */
The documentation also needs be be updated to reflect that facts this 
option is not architecture-agnostic.
> +static __init int numa_setup(const char *opt)
> +{
> +    if ( !strncmp(opt,"off",3) )
> +        numa_off = true;
> +    else if ( !strncmp(opt,"on",2) )
> +        numa_off = false;
> +#ifdef CONFIG_NUMA_EMU
> +    else if ( !strncmp(opt, "fake=", 5) )
> +    {
> +        numa_off = false;
> +        numa_fake = simple_strtoul(opt+5,NULL,0);
> +        if ( numa_fake >= MAX_NUMNODES )
> +            numa_fake = MAX_NUMNODES;
> +    }
> +#endif
> +#ifdef CONFIG_ACPI_NUMA
> +    else if ( !strncmp(opt,"noacpi",6) )
> +    {
> +        numa_off = false;
> +        acpi_numa = -1;
> +    }
> +#endif
Looking at this code, I am not quite too sure to understand the 
difference between between "numa=noacpi" and "numa=off".
In fact, I am tempted to say this option should disappear because this 
is odd to have a firmware specific option just for ACPI but not DT. Even 
if we have one for each, this makes things a bit more complicated for 
the admin.
> +    else
> +        return -EINVAL;
> +
> +    return 0;
> +}
> diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
> index b9b5d1ad88..c647fef736 100644
> --- a/xen/include/xen/numa.h
> +++ b/xen/include/xen/numa.h
> @@ -83,7 +83,6 @@ extern void numa_initmem_init(unsigned long start_pfn, 
> unsigned long end_pfn);
>   extern void numa_set_node(int cpu, nodeid_t node);
>   extern int numa_scan_nodes(u64 start, u64 end);
>   extern bool numa_off;
> -extern int numa_fake;
>   extern s8 acpi_numa;
>   
>   extern void setup_node_bootmem(nodeid_t nodeid, u64 start, u64 end);
Cheers,

--
Julien Grall