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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: 2021年8月26日 16:22
> To: Wei Chen <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
> Subject: Re: [XEN RFC PATCH 23/40] xen/arm: introduce a helper to parse
> device tree memory node
> 
> 
> 
> 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?
> 

I just tried to do that in draft process, because I was not satisfied
with the changes, I haven't saved them as a patch.

I think your suggestions are worth to try again, I will do it
in next version.


> Cheers,
> 
> --
> Julien Grall