RE: [PATCH 34/37] xen/arm: enable device tree based NUMA in system init

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Stefano,

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Sent: 2021年9月24日 11:28
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; sstabellini@xxxxxxxxxx; julien@xxxxxxx;
> Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
> Subject: Re: [PATCH 34/37] xen/arm: enable device tree based NUMA in
> system init
> 
> On Thu, 23 Sep 2021, Wei Chen wrote:
> > In this patch, we can start to create NUMA system that is
> > based on device tree.
> >
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > ---
> >  xen/arch/arm/numa.c        | 55 ++++++++++++++++++++++++++++++++++++++
> >  xen/arch/arm/setup.c       |  7 +++++
> >  xen/include/asm-arm/numa.h |  6 +++++
> >  3 files changed, 68 insertions(+)
> >
> > diff --git a/xen/arch/arm/numa.c b/xen/arch/arm/numa.c
> > index 7f05299b76..d7a3d32d4b 100644
> > --- a/xen/arch/arm/numa.c
> > +++ b/xen/arch/arm/numa.c
> > @@ -18,8 +18,10 @@
> >   *
> >   */
> >  #include <xen/init.h>
> > +#include <xen/device_tree.h>
> >  #include <xen/nodemask.h>
> >  #include <xen/numa.h>
> > +#include <xen/pfn.h>
> >
> >  static uint8_t __read_mostly
> >  node_distance_map[MAX_NUMNODES][MAX_NUMNODES] = {
> > @@ -85,6 +87,59 @@ uint8_t __node_distance(nodeid_t from, nodeid_t to)
> >  }
> >  EXPORT_SYMBOL(__node_distance);
> >
> > +void __init numa_init(bool acpi_off)
> > +{
> > +    uint32_t idx;
> > +    paddr_t ram_start = ~0;
> 
> INVALID_PADDR
> 

Oh, yes

> 
> > +    paddr_t ram_size = 0;
> > +    paddr_t ram_end = 0;
> > +
> > +    /* NUMA has been turned off through Xen parameters */
> > +    if ( numa_off )
> > +        goto mem_init;
> > +
> > +    /* Initialize NUMA from device tree when system is not ACPI booted
> */
> > +    if ( acpi_off )
> > +    {
> > +        int ret = numa_device_tree_init(device_tree_flattened);
> > +        if ( ret )
> > +        {
> > +            printk(XENLOG_WARNING
> > +                   "Init NUMA from device tree failed, ret=%d\n", ret);
> 
> As I mentioned in other patches we need to distinguish between two
> cases:
> 
> 1) NUMA initialization failed because no NUMA information has been found
> 2) NUMA initialization failed because wrong/inconsistent NUMA info has
>    been found
> 
> In case of 1), we print nothing. Maybe a single XENLOG_DEBUG message.
> In case of 2), all the warnings are good to print.
> 
> 
> In this case, if ret != 0 because of 2), then it is fine to print this
> warning. But it looks like could be that ret is -EINVAL simply because a
> CPU node doesn't have numa-node-id, which is a normal condition for
> non-NUMA machines.
> 

Yes, we should have to distinguish these two cases. I will try to address
it in next version.

> 
> > +            numa_off = true;
> > +        }
> > +    }
> > +    else
> > +    {
> > +        /* We don't support NUMA for ACPI boot currently */
> > +        printk(XENLOG_WARNING
> > +               "ACPI NUMA has not been supported yet, NUMA off!\n");
> > +        numa_off = true;
> > +    }
> > +
> > +mem_init:
> > +    /*
> > +     * Find the minimal and maximum address of RAM, NUMA will
> > +     * build a memory to node mapping table for the whole range.
> > +     */
> > +    ram_start = bootinfo.mem.bank[0].start;
> > +    ram_size  = bootinfo.mem.bank[0].size;
> > +    ram_end   = ram_start + ram_size;
> > +    for ( idx = 1 ; idx < bootinfo.mem.nr_banks; idx++ )
> > +    {
> > +        paddr_t bank_start = bootinfo.mem.bank[idx].start;
> > +        paddr_t bank_size = bootinfo.mem.bank[idx].size;
> > +        paddr_t bank_end = bank_start + bank_size;
> > +
> > +        ram_size  = ram_size + bank_size;
> > +        ram_start = min(ram_start, bank_start);
> > +        ram_end   = max(ram_end, bank_end);
> > +    }
> > +
> > +    numa_initmem_init(PFN_UP(ram_start), PFN_DOWN(ram_end));
> > +    return;
> 
> No need for return
> 

Ok, I will remove it.

> 
> > +}
> > +
> >  uint32_t __init arch_meminfo_get_nr_bank(void)
> >  {
> >     return bootinfo.mem.nr_banks;
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index 1f0fbc95b5..6097850682 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -905,6 +905,13 @@ void __init start_xen(unsigned long
> boot_phys_offset,
> >      /* Parse the ACPI tables for possible boot-time configuration */
> >      acpi_boot_table_init();
> >
> > +    /*
> > +     * Try to initialize NUMA system, if failed, the system will
> > +     * fallback to uniform system which means system has only 1
> > +     * NUMA node.
> > +     */
> > +    numa_init(acpi_disabled);
> > +
> >      end_boot_allocator();
> >
> >      /*
> > diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h
> > index f46e8e2935..5b03dde87f 100644
> > --- a/xen/include/asm-arm/numa.h
> > +++ b/xen/include/asm-arm/numa.h
> > @@ -24,6 +24,7 @@ typedef u8 nodeid_t;
> >
> >  extern void numa_set_distance(nodeid_t from, nodeid_t to, uint32_t
> distance);
> >  extern int numa_device_tree_init(const void *fdt);
> > +extern void numa_init(bool acpi_off);
> >
> >  #else
> >
> > @@ -47,6 +48,11 @@ extern mfn_t first_valid_mfn;
> >  #define node_start_pfn(nid) (mfn_x(first_valid_mfn))
> >  #define __node_distance(a, b) (20)
> >
> > +static inline void numa_init(bool acpi_off)
> > +{
> > +
> > +}
> > +
> >  static inline void numa_add_cpu(int cpu)
> >  {
> >
> > --
> > 2.25.1
> >