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@arm.com>
---
 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)
+{
+    if ( srat_disabled() )
+        return -EINVAL;
+    else if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) {
+		bad_srat();
+		return -EINVAL;
+	}
+
+    node_set(node, processor_nodes_parsed);
+
+    device_tree_numa = 1;
+    printk(KERN_INFO "DT: NUMA node %u processor parsed\n", node);
+
+    return 0;
+}
+
+/* Parse CPU NUMA node info */
+int __init device_tree_parse_numa_cpu_node(const void *fdt, int node)
+{
+    uint32_t nid;
+
+    nid = device_tree_get_u32(fdt, node, "numa-node-id", MAX_NUMNODES);
+    printk(XENLOG_WARNING "CPU on NUMA node:%u\n", nid);
+    if ( nid >= MAX_NUMNODES )
+    {
+        printk(XENLOG_WARNING "Node id %u exceeds maximum value\n", nid);
+        return -EINVAL;
+    }
+
+    return dtb_numa_processor_affinity_init(nid);
+}
-- 
2.25.1

Hi Wei,

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@arm.com>
> ---
>   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)
> +{
> +    if ( srat_disabled() )
> +        return -EINVAL;
> +    else if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) {
> +		bad_srat();
> +		return -EINVAL;

You seem to have a mix of soft and hard tab in this file. Is there a lot 
of the code that was directly copied from Linux? If not, then the file 
should be using Xen coding style.

> +	}
> +
> +    node_set(node, processor_nodes_parsed);
> +
> +    device_tree_numa = 1;
> +    printk(KERN_INFO "DT: NUMA node %u processor parsed\n", node);
> +
> +    return 0;
> +}
> +
> +/* Parse CPU NUMA node info */
> +int __init device_tree_parse_numa_cpu_node(const void *fdt, int node)

AFAICT, you are going to turn this helper static in a follow-up patch. 
This is a bad practice. Instead, the function should be static from the 
beginning. If it is not possible, then you should re-order the code.

In this case, I think you can add the boilerplate to parse the NUMA 
information (patch #25) here and then extend it in each patch.


> +{
> +    uint32_t nid;
> +
> +    nid = device_tree_get_u32(fdt, node, "numa-node-id", MAX_NUMNODES);
> +    printk(XENLOG_WARNING "CPU on NUMA node:%u\n", nid);
> +    if ( nid >= MAX_NUMNODES )
> +    {
> +        printk(XENLOG_WARNING "Node id %u exceeds maximum value\n", nid);
> +        return -EINVAL;
> +    }
> +
> +    return dtb_numa_processor_affinity_init(nid);
> +}
> 

Cheers,

-- 
Julien Grall

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 2021年8月20日 2:10
> To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org; jbeulich@suse.com
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>
> Subject: Re: [XEN RFC PATCH 22/40] xen/arm: introduce a helper to parse
> device tree processor node
> 
> Hi Wei,
> 
> 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@arm.com>
> > ---
> >   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)
> > +{
> > +    if ( srat_disabled() )
> > +        return -EINVAL;
> > +    else if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) {
> > +		bad_srat();
> > +		return -EINVAL;
> 
> You seem to have a mix of soft and hard tab in this file. Is there a lot
> of the code that was directly copied from Linux? If not, then the file
> should be using Xen coding style.
> 

I copied some code from x86, and x86 is Linux style.
So, yes, I should adjust them it Xen coding style.
I will do it in next version.

> > +	}
> > +
> > +    node_set(node, processor_nodes_parsed);
> > +
> > +    device_tree_numa = 1;
> > +    printk(KERN_INFO "DT: NUMA node %u processor parsed\n", node);
> > +
> > +    return 0;
> > +}
> > +
> > +/* Parse CPU NUMA node info */
> > +int __init device_tree_parse_numa_cpu_node(const void *fdt, int node)
> 
> AFAICT, you are going to turn this helper static in a follow-up patch.
> This is a bad practice. Instead, the function should be static from the
> beginning. If it is not possible, then you should re-order the code.
> 
> In this case, I think you can add the boilerplate to parse the NUMA
> information (patch #25) here and then extend it in each patch.
> 
> 

That's make sense, I will try to address it in next version.

> > +{
> > +    uint32_t nid;
> > +
> > +    nid = device_tree_get_u32(fdt, node, "numa-node-id", MAX_NUMNODES);
> > +    printk(XENLOG_WARNING "CPU on NUMA node:%u\n", nid);
> > +    if ( nid >= MAX_NUMNODES )
> > +    {
> > +        printk(XENLOG_WARNING "Node id %u exceeds maximum value\n",
> nid);
> > +        return -EINVAL;
> > +    }
> > +
> > +    return dtb_numa_processor_affinity_init(nid);
> > +}
> >
> 
> Cheers,
> 
> --
> Julien Grall

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@arm.com>
> ---
>   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?

> +{
> +    if ( srat_disabled() )
> +        return -EINVAL;
> +    else if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) {
> +		bad_srat();
> +		return -EINVAL;
> +	}
> +
> +    node_set(node, processor_nodes_parsed);
> +
> +    device_tree_numa = 1;
> +    printk(KERN_INFO "DT: NUMA node %u processor parsed\n", node);
> +
> +    return 0;
> +}
> +
> +/* Parse CPU NUMA node info */
> +int __init device_tree_parse_numa_cpu_node(const void *fdt, int node)
> +{
> +    uint32_t nid;
> +
> +    nid = device_tree_get_u32(fdt, node, "numa-node-id", MAX_NUMNODES);
> +    printk(XENLOG_WARNING "CPU on NUMA node:%u\n", nid);
> +    if ( nid >= MAX_NUMNODES )
> +    {
> +        printk(XENLOG_WARNING "Node id %u exceeds maximum value\n", nid);
> +        return -EINVAL;
> +    }
> +
> +    return dtb_numa_processor_affinity_init(nid);
> +}
> 

Cheers,

-- 
Julien Grall

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 2021年8月20日 2:11
> To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org; jbeulich@suse.com
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>
> 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@arm.com>
> > ---
> >   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.

If that's not the right reason, I will unify all prefix to device_tree_
in next version. How do you think about it?

> > +{
> > +    if ( srat_disabled() )
> > +        return -EINVAL;
> > +    else if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) {
> > +		bad_srat();
> > +		return -EINVAL;
> > +	}
> > +
> > +    node_set(node, processor_nodes_parsed);
> > +
> > +    device_tree_numa = 1;
> > +    printk(KERN_INFO "DT: NUMA node %u processor parsed\n", node);
> > +
> > +    return 0;
> > +}
> > +
> > +/* Parse CPU NUMA node info */
> > +int __init device_tree_parse_numa_cpu_node(const void *fdt, int node)
> > +{
> > +    uint32_t nid;
> > +
> > +    nid = device_tree_get_u32(fdt, node, "numa-node-id", MAX_NUMNODES);
> > +    printk(XENLOG_WARNING "CPU on NUMA node:%u\n", nid);
> > +    if ( nid >= MAX_NUMNODES )
> > +    {
> > +        printk(XENLOG_WARNING "Node id %u exceeds maximum value\n",
> nid);
> > +        return -EINVAL;
> > +    }
> > +
> > +    return dtb_numa_processor_affinity_init(nid);
> > +}
> >
> 
> Cheers,
> 
> --
> Julien Grall

On 23/08/2021 09:47, Wei Chen wrote:
> Hi Julien,

Hi Wei,

>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: 2021年8月20日 2:11
>> To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org;
>> sstabellini@kernel.org; jbeulich@suse.com
>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>
>> 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@arm.com>
>>> ---
>>>    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

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 2021年8月23日 18:59
> To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>
> 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@xen.org>
> >> Sent: 2021年8月20日 2:11
> >> To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org;
> >> sstabellini@kernel.org; jbeulich@suse.com
> >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>
> >> 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@arm.com>
> >>> ---
> >>>    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

Hi Wei,

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@arm.com>
> ---
>   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>

Nothing in this file seems to depend on xen/device_tree.h. So why do you 
need to include it?

> +#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)
> +{
> +    if ( srat_disabled() )
> +        return -EINVAL;
> +    else if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) {
> +		bad_srat();
> +		return -EINVAL;
> +	}
> +
> +    node_set(node, processor_nodes_parsed);
> +
> +    device_tree_numa = 1;
> +    printk(KERN_INFO "DT: NUMA node %u processor parsed\n", node);
> +
> +    return 0;
> +}
> +
> +/* Parse CPU NUMA node info */
> +int __init device_tree_parse_numa_cpu_node(const void *fdt, int node)
> +{
> +    uint32_t nid;
> +
> +    nid = device_tree_get_u32(fdt, node, "numa-node-id", MAX_NUMNODES);
> +    printk(XENLOG_WARNING "CPU on NUMA node:%u\n", nid);
> +    if ( nid >= MAX_NUMNODES )
> +    {
> +        printk(XENLOG_WARNING "Node id %u exceeds maximum value\n", nid);
> +        return -EINVAL;
> +    }
> +
> +    return dtb_numa_processor_affinity_init(nid);
> +}
> 

-- 
Julien Grall

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 2021年8月20日 2:13
> To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org; jbeulich@suse.com
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>
> Subject: Re: [XEN RFC PATCH 22/40] xen/arm: introduce a helper to parse
> device tree processor node
> 
> Hi Wei,
> 
> 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@arm.com>
> > ---
> >   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>
> 
> Nothing in this file seems to depend on xen/device_tree.h. So why do you
> need to include it?
> 

I remember that without this header file, device_tree_get_u32 in this patch
will cause compiling failed.

> > +#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)
> > +{
> > +    if ( srat_disabled() )
> > +        return -EINVAL;
> > +    else if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) {
> > +		bad_srat();
> > +		return -EINVAL;
> > +	}
> > +
> > +    node_set(node, processor_nodes_parsed);
> > +
> > +    device_tree_numa = 1;
> > +    printk(KERN_INFO "DT: NUMA node %u processor parsed\n", node);
> > +
> > +    return 0;
> > +}
> > +
> > +/* Parse CPU NUMA node info */
> > +int __init device_tree_parse_numa_cpu_node(const void *fdt, int node)
> > +{
> > +    uint32_t nid;
> > +
> > +    nid = device_tree_get_u32(fdt, node, "numa-node-id", MAX_NUMNODES);
> > +    printk(XENLOG_WARNING "CPU on NUMA node:%u\n", nid);
> > +    if ( nid >= MAX_NUMNODES )
> > +    {
> > +        printk(XENLOG_WARNING "Node id %u exceeds maximum value\n",
> nid);
> > +        return -EINVAL;
> > +    }
> > +
> > +    return dtb_numa_processor_affinity_init(nid);
> > +}
> >
> 
> --
> Julien Grall

On 20/08/2021 03:23, Wei Chen wrote:
> Hi Julien,

Hi Wei,

>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: 2021年8月20日 2:13
>> To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org;
>> sstabellini@kernel.org; jbeulich@suse.com
>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>
>> Subject: Re: [XEN RFC PATCH 22/40] xen/arm: introduce a helper to parse
>> device tree processor node
>>
>> Hi Wei,
>>
>> 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@arm.com>
>>> ---
>>>    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>
>>
>> Nothing in this file seems to depend on xen/device_tree.h. So why do you
>> need to include it?
>>
> 
> I remember that without this header file, device_tree_get_u32 in this patch
> will cause compiling failed.

I looked at the prototype of device_tree_get_u32() and I can't find how 
it depends on bits from device_tree.h. Can you paste the compilation error?

> 
>>> +#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)
>>> +{
>>> +    if ( srat_disabled() )
>>> +        return -EINVAL;
>>> +    else if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) {
>>> +		bad_srat();
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +    node_set(node, processor_nodes_parsed);
>>> +
>>> +    device_tree_numa = 1;
>>> +    printk(KERN_INFO "DT: NUMA node %u processor parsed\n", node);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +/* Parse CPU NUMA node info */
>>> +int __init device_tree_parse_numa_cpu_node(const void *fdt, int node)
>>> +{
>>> +    uint32_t nid;
>>> +
>>> +    nid = device_tree_get_u32(fdt, node, "numa-node-id", MAX_NUMNODES);
>>> +    printk(XENLOG_WARNING "CPU on NUMA node:%u\n", nid);
>>> +    if ( nid >= MAX_NUMNODES )
>>> +    {
>>> +        printk(XENLOG_WARNING "Node id %u exceeds maximum value\n",
>> nid);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    return dtb_numa_processor_affinity_init(nid);
>>> +}
>>>
>>
>> --
>> Julien Grall

Cheers,

-- 
Julien Grall

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 2021年8月20日 16:44
> To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org; jbeulich@suse.com
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>
> Subject: Re: [XEN RFC PATCH 22/40] xen/arm: introduce a helper to parse
> device tree processor node
> 
> 
> 
> On 20/08/2021 03:23, Wei Chen wrote:
> > Hi Julien,
> 
> Hi Wei,
> 
> >> -----Original Message-----
> >> From: Julien Grall <julien@xen.org>
> >> Sent: 2021年8月20日 2:13
> >> To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org;
> >> sstabellini@kernel.org; jbeulich@suse.com
> >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>
> >> Subject: Re: [XEN RFC PATCH 22/40] xen/arm: introduce a helper to parse
> >> device tree processor node
> >>
> >> Hi Wei,
> >>
> >> 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@arm.com>
> >>> ---
> >>>    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>
> >>
> >> Nothing in this file seems to depend on xen/device_tree.h. So why do
> you
> >> need to include it?
> >>
> >
> > I remember that without this header file, device_tree_get_u32 in this
> patch
> > will cause compiling failed.
> 
> I looked at the prototype of device_tree_get_u32() and I can't find how
> it depends on bits from device_tree.h. Can you paste the compilation error?
> 

I tested it again, this header file should be introduced in following patches:
numa_device_tree.c: In function ‘device_tree_parse_numa_distance_map_v1’:
numa_device_tree.c:243:16: error: implicit declaration of function ‘dt_read_number’ [-Werror=implicit-function-declaration]
  243 |         from = dt_read_number(matrix, 1);
      |                ^~~~~~~~~~~~~~
numa_device_tree.c:243:16: error: nested extern declaration of ‘dt_read_number’ [-Werror=nested-externs]
cc1: all warnings being treated as errors

I will move it to another patch.

> >
> >>> +#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)
> >>> +{
> >>> +    if ( srat_disabled() )
> >>> +        return -EINVAL;
> >>> +    else if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) {
> >>> +		bad_srat();
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>> +    node_set(node, processor_nodes_parsed);
> >>> +
> >>> +    device_tree_numa = 1;
> >>> +    printk(KERN_INFO "DT: NUMA node %u processor parsed\n", node);
> >>> +
> >>> +    return 0;
> >>> +}
> >>> +
> >>> +/* Parse CPU NUMA node info */
> >>> +int __init device_tree_parse_numa_cpu_node(const void *fdt, int node)
> >>> +{
> >>> +    uint32_t nid;
> >>> +
> >>> +    nid = device_tree_get_u32(fdt, node, "numa-node-id",
> MAX_NUMNODES);
> >>> +    printk(XENLOG_WARNING "CPU on NUMA node:%u\n", nid);
> >>> +    if ( nid >= MAX_NUMNODES )
> >>> +    {
> >>> +        printk(XENLOG_WARNING "Node id %u exceeds maximum value\n",
> >> nid);
> >>> +        return -EINVAL;
> >>> +    }
> >>> +
> >>> +    return dtb_numa_processor_affinity_init(nid);
> >>> +}
> >>>
> >>
> >> --
> >> Julien Grall
> 
> Cheers,
> 
> --
> Julien Grall

On Wed, 11 Aug 2021, 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@arm.com>
> ---
>  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)
> +{
> +    if ( srat_disabled() )
> +        return -EINVAL;
> +    else if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) {
> +		bad_srat();
> +		return -EINVAL;
> +	}
> +
> +    node_set(node, processor_nodes_parsed);
> +
> +    device_tree_numa = 1;
> +    printk(KERN_INFO "DT: NUMA node %u processor parsed\n", node);
> +
> +    return 0;
> +}
> +
> +/* Parse CPU NUMA node info */
> +int __init device_tree_parse_numa_cpu_node(const void *fdt, int node)
> +{
> +    uint32_t nid;
> +
> +    nid = device_tree_get_u32(fdt, node, "numa-node-id", MAX_NUMNODES);
> +    printk(XENLOG_WARNING "CPU on NUMA node:%u\n", nid);

Given that this is not actually a warning (is it?) then I would move it
to XENLOG_INFO


> +    if ( nid >= MAX_NUMNODES )
> +    {
> +        printk(XENLOG_WARNING "Node id %u exceeds maximum value\n", nid);

This could be XENLOG_ERR


> +        return -EINVAL;
> +    }
> +
> +    return dtb_numa_processor_affinity_init(nid);
> +}

Hi Stefano,

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@kernel.org>
> Sent: 2021年8月27日 8:06
> To: Wei Chen <Wei.Chen@arm.com>
> Cc: xen-devel@lists.xenproject.org; sstabellini@kernel.org; julien@xen.org;
> jbeulich@suse.com; Bertrand Marquis <Bertrand.Marquis@arm.com>
> Subject: Re: [XEN RFC PATCH 22/40] xen/arm: introduce a helper to parse
> device tree processor node
> 
> On Wed, 11 Aug 2021, 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@arm.com>
> > ---
> >  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)
> > +{
> > +    if ( srat_disabled() )
> > +        return -EINVAL;
> > +    else if ( node == NUMA_NO_NODE || node >= MAX_NUMNODES ) {
> > +		bad_srat();
> > +		return -EINVAL;
> > +	}
> > +
> > +    node_set(node, processor_nodes_parsed);
> > +
> > +    device_tree_numa = 1;
> > +    printk(KERN_INFO "DT: NUMA node %u processor parsed\n", node);
> > +
> > +    return 0;
> > +}
> > +
> > +/* Parse CPU NUMA node info */
> > +int __init device_tree_parse_numa_cpu_node(const void *fdt, int node)
> > +{
> > +    uint32_t nid;
> > +
> > +    nid = device_tree_get_u32(fdt, node, "numa-node-id", MAX_NUMNODES);
> > +    printk(XENLOG_WARNING "CPU on NUMA node:%u\n", nid);
> 
> Given that this is not actually a warning (is it?) then I would move it
> to XENLOG_INFO
> 
> 


OK

> > +    if ( nid >= MAX_NUMNODES )
> > +    {
> > +        printk(XENLOG_WARNING "Node id %u exceeds maximum value\n",
> nid);
> 
> This could be XENLOG_ERR
> 

OK

> 
> > +        return -EINVAL;
> > +    }
> > +
> > +    return dtb_numa_processor_affinity_init(nid);
> > +}