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

[Julien Grall <julien@xxxxxxx>]

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.
> 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".
Cheers,

--
Julien Grall