RE: [PATCH v3 1/6] xen/x86: Provide helpers for common code to access acpi_numa

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 2022年8月25日 18:18
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: nd <nd@xxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Roger Pau
> Monné <roger.pau@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3 1/6] xen/x86: Provide helpers for common code to
> access acpi_numa
> 
> On 22.08.2022 04:58, Wei Chen wrote:
> > --- a/xen/arch/x86/include/asm/numa.h
> > +++ b/xen/arch/x86/include/asm/numa.h
> > @@ -32,8 +32,9 @@ extern void numa_add_cpu(int cpu);
> >  extern void numa_init_array(void);
> >  extern bool numa_off;
> >
> > -
> > -extern int srat_disabled(void);
> > +extern int arch_numa_setup(const char *opt);
> > +extern bool arch_numa_disabled(bool init_as_disable);
> 
> What is the parameter name intended to mean? Since the only caller
> passes "false", this also isn't really possible to guess from the
> use(s) in this patch. In any event perhaps best for the parameter
> to be introduced only once it's actually needed.
> 

This parameter will be used in patch#5 and set to true, I will introduce
this parameter in that patch.

> > --- a/xen/arch/x86/numa.c
> > +++ b/xen/arch/x86/numa.c
> > @@ -50,9 +50,31 @@ nodemask_t __read_mostly node_online_map = { { [0] =
> 1UL } };
> >  bool numa_off;
> >  s8 acpi_numa = 0;
> >
> > -int srat_disabled(void)
> > +int __init arch_numa_setup(const char *opt)
> >  {
> > -    return numa_off || acpi_numa < 0;
> > +#ifdef CONFIG_ACPI_NUMA
> > +    if ( !strncmp(opt, "noacpi", 6) )
> > +    {
> > +        numa_off = false;
> > +        acpi_numa = -1;
> > +        return 0;
> 
> With this "return" ...
> 
> > +    }
> > +    else
> 
> ... this "else" is unnecessary and hence would better be dropped,
> not the least to ...
> 
> > +#endif
> > +    return -EINVAL;
> 
> ... avoid the otherwise ambiguous indentation of this line.
> 

This is a good suggestion, current indentation looks weird, I will fix above 3
in next version.

Thanks,
Wei Che

> Jan