Re: [XEN RFC PATCH 23/40] xen/arm: introduce a helper to parse device tree memory node

[Julien Grall <julien@xxxxxxx>]

On 26/08/2021 07:35, Wei Chen wrote:
> Hi Julien,
Hi Wei,

> > -----Original Message-----
> > From: Julien Grall <julien@xxxxxxx>
> > Sent: 2021年8月25日 21:49
> > To: Wei Chen <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> > sstabellini@xxxxxxxxxx; jbeulich@xxxxxxxx
> > Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
> > Subject: Re: [XEN RFC PATCH 23/40] xen/arm: introduce a helper to parse
> > device tree memory node
> >
> > Hi Wei,
> >
> > On 11/08/2021 11:24, Wei Chen wrote:
> > > Memory blocks' NUMA ID information is stored in device tree's
> > > memory nodes as "numa-node-id". We need a new helper to parse
> > > and verify this ID from memory nodes.
> > >
> > > In order to support memory affinity in later use, the valid
> > > memory ranges and NUMA ID will be saved to tables.
> > >
> > > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > > ---
> > >    xen/arch/arm/numa_device_tree.c | 130 ++++++++++++++++++++++++++++++++
> > >    1 file changed, 130 insertions(+)
> > >
> > > diff --git a/xen/arch/arm/numa_device_tree.c
> > b/xen/arch/arm/numa_device_tree.c
> > > index 37cc56acf3..bbe081dcd1 100644
> > > --- a/xen/arch/arm/numa_device_tree.c
> > > +++ b/xen/arch/arm/numa_device_tree.c
> > > @@ -20,11 +20,13 @@
> > >    #include <xen/init.h>
> > >    #include <xen/nodemask.h>
> > >    #include <xen/numa.h>
> > > +#include <xen/libfdt/libfdt.h>
> > >    #include <xen/device_tree.h>
> > >    #include <asm/setup.h>
> > >
> > >    s8 device_tree_numa = 0;
> > >    static nodemask_t processor_nodes_parsed __initdata;
> > > +static nodemask_t memory_nodes_parsed __initdata;
> > >
> > >    static int srat_disabled(void)
> > >    {
> > > @@ -55,6 +57,79 @@ static int __init
> > dtb_numa_processor_affinity_init(nodeid_t node)
> > >        return 0;
> > >    }
> > >
> > > +/* Callback for parsing of the memory regions affinity */
> > > +static int __init dtb_numa_memory_affinity_init(nodeid_t node,
> > > +                                paddr_t start, paddr_t size)
> > > +{
> > The implementation of this function is quite similar ot the ACPI
> > version. Can this be abstracted?
> In my draft, I had tried to merge ACPI and DTB versions in one
> function. I introduced a number of "if else" to distinguish ACPI
> from DTB, especially ACPI hotplug. The function seems very messy.
> Not enough benefits to make up for the mess, so I gave up.
It think you can get away from distinguishing between ACPI and DT in 
that helper:
  * ma->flags & ACPI_SRAT_MEM_HOTPLUGGABLE could be replace by an 
argument indicating whether the region is hotpluggable (this would 
always be false for DT)
  * Access to memblk_hotplug can be stubbed (in the future we may want 
to consider memory hotplug even on Arm).
Do you still have the "if else" version? If so can you post it?

Cheers,

--
Julien Grall