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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: 2021年8月23日 18:59
> To: Wei Chen <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
> Subject: Re: [XEN RFC PATCH 22/40] xen/arm: introduce a helper to parse
> device tree processor node
> 
> 
> 
> On 23/08/2021 09:47, Wei Chen wrote:
> > Hi Julien,
> 
> Hi Wei,
> 
> >> -----Original Message-----
> >> From: Julien Grall <julien@xxxxxxx>
> >> Sent: 2021年8月20日 2:11
> >> To: Wei Chen <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> >> sstabellini@xxxxxxxxxx; jbeulich@xxxxxxxx
> >> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
> >> Subject: Re: [XEN RFC PATCH 22/40] xen/arm: introduce a helper to parse
> >> device tree processor node
> >>
> >> On 11/08/2021 11:24, Wei Chen wrote:
> >>> Processor NUMA ID information is stored in device tree's processor
> >>> node as "numa-node-id". We need a new helper to parse this ID from
> >>> processor node. If we get this ID from processor node, this ID's
> >>> validity still need to be checked. Once we got a invalid NUMA ID
> >>> from any processor node, the device tree will be marked as NUMA
> >>> information invalid.
> >>>
> >>> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> >>> ---
> >>>    xen/arch/arm/numa_device_tree.c | 41
> +++++++++++++++++++++++++++++++--
> >>>    1 file changed, 39 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/numa_device_tree.c
> >> b/xen/arch/arm/numa_device_tree.c
> >>> index 1c74ad135d..37cc56acf3 100644
> >>> --- a/xen/arch/arm/numa_device_tree.c
> >>> +++ b/xen/arch/arm/numa_device_tree.c
> >>> @@ -20,16 +20,53 @@
> >>>    #include <xen/init.h>
> >>>    #include <xen/nodemask.h>
> >>>    #include <xen/numa.h>
> >>> +#include <xen/device_tree.h>
> >>> +#include <asm/setup.h>
> >>>
> >>>    s8 device_tree_numa = 0;
> >>> +static nodemask_t processor_nodes_parsed __initdata;
> >>>
> >>> -int srat_disabled(void)
> >>> +static int srat_disabled(void)
> >>>    {
> >>>        return numa_off || device_tree_numa < 0;
> >>>    }
> >>>
> >>> -void __init bad_srat(void)
> >>> +static __init void bad_srat(void)
> >>>    {
> >>>        printk(KERN_ERR "DT: NUMA information is not used.\n");
> >>>        device_tree_numa = -1;
> >>>    }
> >>> +
> >>> +/* Callback for device tree processor affinity */
> >>> +static int __init dtb_numa_processor_affinity_init(nodeid_t node)
> >>
> >> I forgot to answer. It seems odd that some of the function names start
> >> with dtb_* while other starts device_tree_*. Any particular reason for
> >> that difference of naming?
> >>
> >
> > yes, in the very beginning, I want to keep device_tree_ prefix for
> > functions that will handle dtb file. And use dtb_ prefix to replace
> > acpi, to indicate, this function is device tree version numa
> implementation.
> 
> Thanks for the clarification. The difference between "dtb" and
> "device_tree" is quite subttle: the former refers to the binary while
> the latter refers to the format. Most of the readers are likely to infer
> they mean the same. So I think this will bring more confusion.
> 

Thanks for the clarification.

> >
> > If that's not the right reason, I will unify all prefix to device_tree_
> > in next version. How do you think about it?
> 
> AFAICT, your parsing functions will always start with
> "device_tree_parse_". I would prefer if the set replacing the ACPI
> helpers start with "device_tree_".
> 
> If you are concern with the length of the function name, then I would
> suggest to prefix all the functions with "fdt" (We are dealing with the
> flattened DT after all) or "dt".

That makes sense, I will do it.

> 
> Cheers,
> 
> --
> Julien Grall