Hi Julien,
> -----Original Message-----
> From: Julien Grall <jul...@xen.org>
> Sent: 2021年8月27日 22:38
> To: Wei Chen <wei.c...@arm.com>; xen-devel@lists.xenproject.org;
> sstabell...@kernel.org; jbeul...@suse.com
> Cc: Bertrand Marquis <bertrand.marq...@arm.com>
> Subject: Re: [XEN RFC PATCH 39/40] xen/x86: move numa_setup to common to
> support NUMA switch in command line
>
> 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.c...@arm.com>
> > ---
> > 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.
>
Ok, I will update the relate document in next version.
> > +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.
>
Yes, I agree. I would consider a proper way to address it in next version.
If x86 maintainers can give some background of these two options would be
better.
> > + 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
