RE: [XEN RFC PATCH 25/40] xen/arm: unified entry to parse all NUMA data from device tree

[Wei Chen <Wei.Chen@xxxxxxx>]

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Sent: 2021年9月2日 2:31
> To: Julien Grall <julien@xxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei Chen
> <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx; jbeulich@xxxxxxxx;
> Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
> Subject: Re: [XEN RFC PATCH 25/40] xen/arm: unified entry to parse all
> NUMA data from device tree
> 
> On Tue, 31 Aug 2021, Julien Grall wrote:
> > Hi Stefano,
> >
> > On 31/08/2021 01:54, Stefano Stabellini wrote:
> > > On Wed, 11 Aug 2021, Wei Chen wrote:
> > > > In this API, we scan whole device tree to parse CPU node id, memory
> > > > node id and distance-map. Though early_scan_node will invoke has a
> > > > handler to process memory nodes. If we want to parse memory node id
> > > > in this handler, we have to embeded NUMA parse code in this handler.
> > > > But we still need to scan whole device tree to find CPU NUMA id and
> > > > distance-map. In this case, we include memory NUMA id parse in this
> > > > API too. Another benefit is that we have a unique entry for device
> > > > tree NUMA data parse.
> > > >
> > > > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > > > ---
> > > >   xen/arch/arm/numa_device_tree.c | 31 ++++++++++++++++++++++++++++-
> --
> > > >   xen/include/asm-arm/numa.h      |  1 +
> > > >   2 files changed, 29 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/xen/arch/arm/numa_device_tree.c
> > > > b/xen/arch/arm/numa_device_tree.c
> > > > index 6e0d1d3d9f..27ffb72f7b 100644
> > > > --- a/xen/arch/arm/numa_device_tree.c
> > > > +++ b/xen/arch/arm/numa_device_tree.c
> > > > @@ -131,7 +131,8 @@ save_memblk:
> > > >   }
> > > >     /* Parse CPU NUMA node info */
> > > > -int __init device_tree_parse_numa_cpu_node(const void *fdt, int
> node)
> > > > +static int __init
> > > > +device_tree_parse_numa_cpu_node(const void *fdt, int node)
> > > >   {
> > > >       uint32_t nid;
> > > >   @@ -147,7 +148,7 @@ int __init
> device_tree_parse_numa_cpu_node(const
> > > > void *fdt, int node)
> > > >   }
> > > >     /* Parse memory node NUMA info */
> > > > -int __init
> > > > +static int __init
> > > >   device_tree_parse_numa_memory_node(const void *fdt, int node,
> > > >       const char *name, uint32_t addr_cells, uint32_t size_cells)
> > > >   {
> > > > @@ -202,7 +203,7 @@ device_tree_parse_numa_memory_node(const void
> *fdt,
> > > > int node,
> > > >   }
> > > >     /* Parse NUMA distance map v1 */
> > > > -int __init
> > > > +static int __init
> > > >   device_tree_parse_numa_distance_map_v1(const void *fdt, int node)
> > > >   {
> > > >       const struct fdt_property *prop;
> > > > @@ -267,3 +268,27 @@ device_tree_parse_numa_distance_map_v1(const
> void
> > > > *fdt, int node)
> > > >         return 0;
> > > >   }
> > > > +
> > > > +static int __init fdt_scan_numa_nodes(const void *fdt,
> > > > +                int node, const char *uname, int depth,
> > > > +                u32 address_cells, u32 size_cells, void *data)
> > > > +{
> > > > +    int ret = 0;
> > > > +
> > > > +    if ( fdt_node_check_type(fdt, node, "cpu") == 0 )
> > > > +        ret = device_tree_parse_numa_cpu_node(fdt, node);
> > > > +    else if ( fdt_node_check_type(fdt, node, "memory") == 0 )
> > > > +        ret = device_tree_parse_numa_memory_node(fdt, node, uname,
> > > > +                                address_cells, size_cells);
> > > > +    else if ( fdt_node_check_compatible(fdt, node,
> > > > +                                "numa-distance-map-v1") == 0 )
> > > > +        ret = device_tree_parse_numa_distance_map_v1(fdt, node);
> > > > +
> > > > +    return ret;
> > > > +}
> > >
> > > Julien, do you have an opinion on whether it might be worth reusing
> the
> > > existing early_scan_node function for this to avoiding another full
> FDT
> > > scan (to avoid another call to device_tree_for_each_node)?
> >
> > I don't like the full FDT scan and actually drafted an e-mail in reply-
> to [1]
> > to suggest parse all the NUMA information from early_scan_node().
> >
> > However, we don't know whether ACPI or DT will be used at the time
> > early_scan_node() is called. So we will need to revert any change which
> can
> > make the code a little more awkward.
> >
> > So I decided to drop my e-mail because I prefer the full DT scan for now.
> We
> > can look at optimizing later if this becomes a pain point.
> 
> Uhm, yes you are right.
> 
> We would have to move some of the logic to detect ACPI earlier to
> early_scan_node (e.g. xen/arch/arm/acpi/boot.c:dt_scan_depth1_nodes).
> That could actually be a good idea, but it is true that could be done
> with a separate patch later.

Sounds good to me.