diff mbox series

[v4,07/17] xen/arm: introduce a helper to parse device tree processor node

Message ID 20230425075655.4037980-8-Henry.Wang@arm.com (mailing list archive)
State Superseded
Headers show
Series Device tree based NUMA support for Arm - Part#3 | expand

Commit Message

Henry Wang April 25, 2023, 7:56 a.m. UTC 
From: Wei Chen <wei.chen@arm.com>

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.

Since new helpers need to know the NUMA status, move the
enum dt_numa_status to the Arm NUMA header.

Signed-off-by: Wei Chen <wei.chen@arm.com>
Signed-off-by: Henry Wang <Henry.Wang@arm.com>
---
v3 -> v4:
1. No change.
v2 -> v3:
1. Move the enum dt_numa_status to the Arm NUMA header.
2. Update the year in copyright to 2023.
v1 -> v2:
1. Move numa_disabled from fdt_numa_processor_affinity_init
   to fdt_parse_numa_cpu_node.
2. Move invalid NUMA id check to fdt_parse_numa_cpu_node.
3. Return ENODATA for normal dtb without NUMA info.
4. Use NUMA status helpers instead of SRAT functions.
---
 xen/arch/arm/Makefile           |  1 +
 xen/arch/arm/include/asm/numa.h |  8 +++++
 xen/arch/arm/numa.c             |  8 +----
 xen/arch/arm/numa_device_tree.c | 64 +++++++++++++++++++++++++++++++++
 4 files changed, 74 insertions(+), 7 deletions(-)
 create mode 100644 xen/arch/arm/numa_device_tree.c

Comments

Jan Beulich April 25, 2023, 8:20 a.m. UTC | #1 
On 25.04.2023 09:56, Henry Wang wrote:
> From: Wei Chen <wei.chen@arm.com>
> 
> 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.
> 
> Since new helpers need to know the NUMA status, move the
> enum dt_numa_status to the Arm NUMA header.
> 
> Signed-off-by: Wei Chen <wei.chen@arm.com>
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
> ---
> v3 -> v4:
> 1. No change.
> v2 -> v3:
> 1. Move the enum dt_numa_status to the Arm NUMA header.
> 2. Update the year in copyright to 2023.
> v1 -> v2:
> 1. Move numa_disabled from fdt_numa_processor_affinity_init
>    to fdt_parse_numa_cpu_node.
> 2. Move invalid NUMA id check to fdt_parse_numa_cpu_node.
> 3. Return ENODATA for normal dtb without NUMA info.
> 4. Use NUMA status helpers instead of SRAT functions.
> ---
>  xen/arch/arm/Makefile           |  1 +
>  xen/arch/arm/include/asm/numa.h |  8 +++++
>  xen/arch/arm/numa.c             |  8 +----
>  xen/arch/arm/numa_device_tree.c | 64 +++++++++++++++++++++++++++++++++
>  4 files changed, 74 insertions(+), 7 deletions(-)
>  create mode 100644 xen/arch/arm/numa_device_tree.c

As asked for in various other contexts, may I please ask that new files
prefer dashes over underscores in their names? Additionally short but
still descriptive names are imo to be generally preferred; in the case
here how about numa-dt.c?

Jan
Henry Wang April 25, 2023, 8:26 a.m. UTC | #2 
Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Subject: Re: [PATCH v4 07/17] xen/arm: introduce a helper to parse device
> tree processor node
> 
> >  xen/arch/arm/numa_device_tree.c | 64
> +++++++++++++++++++++++++++++++++
> >  4 files changed, 74 insertions(+), 7 deletions(-)
> >  create mode 100644 xen/arch/arm/numa_device_tree.c
> 
> As asked for in various other contexts, may I please ask that new files
> prefer dashes over underscores in their names? Additionally short but
> still descriptive names are imo to be generally preferred; in the case
> here how about numa-dt.c?

Sounds good to me. I will follow your suggestion if there will be no
explicit objection from other maintainers. Thanks for the suggestion
as always :)

Kind regards,
Henry

> 
> Jan
Michal Orzel April 25, 2023, 8:39 a.m. UTC | #3 
Hi Jan,

On 25/04/2023 10:20, Jan Beulich wrote:
> 
> 
> On 25.04.2023 09:56, Henry Wang wrote:
>> From: Wei Chen <wei.chen@arm.com>
>>
>> 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.
>>
>> Since new helpers need to know the NUMA status, move the
>> enum dt_numa_status to the Arm NUMA header.
>>
>> Signed-off-by: Wei Chen <wei.chen@arm.com>
>> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
>> ---
>> v3 -> v4:
>> 1. No change.
>> v2 -> v3:
>> 1. Move the enum dt_numa_status to the Arm NUMA header.
>> 2. Update the year in copyright to 2023.
>> v1 -> v2:
>> 1. Move numa_disabled from fdt_numa_processor_affinity_init
>>    to fdt_parse_numa_cpu_node.
>> 2. Move invalid NUMA id check to fdt_parse_numa_cpu_node.
>> 3. Return ENODATA for normal dtb without NUMA info.
>> 4. Use NUMA status helpers instead of SRAT functions.
>> ---
>>  xen/arch/arm/Makefile           |  1 +
>>  xen/arch/arm/include/asm/numa.h |  8 +++++
>>  xen/arch/arm/numa.c             |  8 +----
>>  xen/arch/arm/numa_device_tree.c | 64 +++++++++++++++++++++++++++++++++
>>  4 files changed, 74 insertions(+), 7 deletions(-)
>>  create mode 100644 xen/arch/arm/numa_device_tree.c
> 
> As asked for in various other contexts, may I please ask that new files
> prefer dashes over underscores in their names? Additionally short but
> still descriptive names are imo to be generally preferred; in the case
> here how about numa-dt.c?

Seeing that you made this request multiple times within the last months, maybe it would
be better to write it down in CODING_STYLE or CONTRIBUTING? Otherwise, you will keep making
these requests indefinitely without people being able to know things like that before pushing
new files (+ this always requires respin).

~Michal
Jan Beulich April 25, 2023, 8:47 a.m. UTC | #4 
On 25.04.2023 10:39, Michal Orzel wrote:
> Hi Jan,
> 
> On 25/04/2023 10:20, Jan Beulich wrote:
>>
>>
>> On 25.04.2023 09:56, Henry Wang wrote:
>>> From: Wei Chen <wei.chen@arm.com>
>>>
>>> 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.
>>>
>>> Since new helpers need to know the NUMA status, move the
>>> enum dt_numa_status to the Arm NUMA header.
>>>
>>> Signed-off-by: Wei Chen <wei.chen@arm.com>
>>> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
>>> ---
>>> v3 -> v4:
>>> 1. No change.
>>> v2 -> v3:
>>> 1. Move the enum dt_numa_status to the Arm NUMA header.
>>> 2. Update the year in copyright to 2023.
>>> v1 -> v2:
>>> 1. Move numa_disabled from fdt_numa_processor_affinity_init
>>>    to fdt_parse_numa_cpu_node.
>>> 2. Move invalid NUMA id check to fdt_parse_numa_cpu_node.
>>> 3. Return ENODATA for normal dtb without NUMA info.
>>> 4. Use NUMA status helpers instead of SRAT functions.
>>> ---
>>>  xen/arch/arm/Makefile           |  1 +
>>>  xen/arch/arm/include/asm/numa.h |  8 +++++
>>>  xen/arch/arm/numa.c             |  8 +----
>>>  xen/arch/arm/numa_device_tree.c | 64 +++++++++++++++++++++++++++++++++
>>>  4 files changed, 74 insertions(+), 7 deletions(-)
>>>  create mode 100644 xen/arch/arm/numa_device_tree.c
>>
>> As asked for in various other contexts, may I please ask that new files
>> prefer dashes over underscores in their names? Additionally short but
>> still descriptive names are imo to be generally preferred; in the case
>> here how about numa-dt.c?
> 
> Seeing that you made this request multiple times within the last months, maybe it would
> be better to write it down in CODING_STYLE or CONTRIBUTING? Otherwise, you will keep making
> these requests indefinitely without people being able to know things like that before pushing
> new files (+ this always requires respin).

Well. I could make a try, but getting ./CODING_STYLE changed has proven
to be difficult in the past, and I would prefer to not again sit on such
a patch for months or years. IOW I've become quite hesitant to submit
such patches, even more so for things which - imo - should go without
saying.

Jan
Michal Orzel April 25, 2023, 9:16 a.m. UTC | #5 
On 25/04/2023 10:47, Jan Beulich wrote:
> 
> 
> On 25.04.2023 10:39, Michal Orzel wrote:
>> Hi Jan,
>>
>> On 25/04/2023 10:20, Jan Beulich wrote:
>>>
>>>
>>> On 25.04.2023 09:56, Henry Wang wrote:
>>>> From: Wei Chen <wei.chen@arm.com>
>>>>
>>>> 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.
>>>>
>>>> Since new helpers need to know the NUMA status, move the
>>>> enum dt_numa_status to the Arm NUMA header.
>>>>
>>>> Signed-off-by: Wei Chen <wei.chen@arm.com>
>>>> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
>>>> ---
>>>> v3 -> v4:
>>>> 1. No change.
>>>> v2 -> v3:
>>>> 1. Move the enum dt_numa_status to the Arm NUMA header.
>>>> 2. Update the year in copyright to 2023.
>>>> v1 -> v2:
>>>> 1. Move numa_disabled from fdt_numa_processor_affinity_init
>>>>    to fdt_parse_numa_cpu_node.
>>>> 2. Move invalid NUMA id check to fdt_parse_numa_cpu_node.
>>>> 3. Return ENODATA for normal dtb without NUMA info.
>>>> 4. Use NUMA status helpers instead of SRAT functions.
>>>> ---
>>>>  xen/arch/arm/Makefile           |  1 +
>>>>  xen/arch/arm/include/asm/numa.h |  8 +++++
>>>>  xen/arch/arm/numa.c             |  8 +----
>>>>  xen/arch/arm/numa_device_tree.c | 64 +++++++++++++++++++++++++++++++++
>>>>  4 files changed, 74 insertions(+), 7 deletions(-)
>>>>  create mode 100644 xen/arch/arm/numa_device_tree.c
>>>
>>> As asked for in various other contexts, may I please ask that new files
>>> prefer dashes over underscores in their names? Additionally short but
>>> still descriptive names are imo to be generally preferred; in the case
>>> here how about numa-dt.c?
>>
>> Seeing that you made this request multiple times within the last months, maybe it would
>> be better to write it down in CODING_STYLE or CONTRIBUTING? Otherwise, you will keep making
>> these requests indefinitely without people being able to know things like that before pushing
>> new files (+ this always requires respin).
> 
> Well. I could make a try, but getting ./CODING_STYLE changed has proven
> to be difficult in the past, and I would prefer to not again sit on such
> a patch for months or years. IOW I've become quite hesitant to submit
> such patches, even more so for things which - imo - should go without
> saying.

I understand your point. It might be worth asking e.g. on a community call
to see what others think (for now I did not see any objections). It might be
that everyone is ok with it under good justification.

~Michal
diff mbox series

Patch

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 9073398d6e..bbc68e3735 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -39,6 +39,7 @@  obj-y += mem_access.o
 obj-y += mm.o
 obj-y += monitor.o
 obj-$(CONFIG_NUMA) += numa.o
+obj-$(CONFIG_DEVICE_TREE_NUMA) += numa_device_tree.o
 obj-y += p2m.o
 obj-y += percpu.o
 obj-y += platform.o
diff --git a/xen/arch/arm/include/asm/numa.h b/xen/arch/arm/include/asm/numa.h
index b04ace26db..2987158d16 100644
--- a/xen/arch/arm/include/asm/numa.h
+++ b/xen/arch/arm/include/asm/numa.h
@@ -22,6 +22,14 @@  typedef u8 nodeid_t;
  */
 #define NR_NODE_MEMBLKS NR_MEM_BANKS
 
+enum dt_numa_status {
+    DT_NUMA_DEFAULT,
+    DT_NUMA_ON,
+    DT_NUMA_OFF,
+};
+
+extern enum dt_numa_status device_tree_numa;
+
 /*
  * In ACPI spec, 0-9 are the reserved values for node distance,
  * 10 indicates local node distance, 20 indicates remote node
diff --git a/xen/arch/arm/numa.c b/xen/arch/arm/numa.c
index 7ae9ace15d..b953c2574a 100644
--- a/xen/arch/arm/numa.c
+++ b/xen/arch/arm/numa.c
@@ -20,13 +20,7 @@ 
 #include <xen/init.h>
 #include <xen/numa.h>
 
-enum dt_numa_status {
-    DT_NUMA_DEFAULT,
-    DT_NUMA_ON,
-    DT_NUMA_OFF,
-};
-
-static enum dt_numa_status __ro_after_init device_tree_numa = DT_NUMA_DEFAULT;
+enum dt_numa_status __ro_after_init device_tree_numa = DT_NUMA_DEFAULT;
 
 static unsigned char __ro_after_init
 node_distance_map[MAX_NUMNODES][MAX_NUMNODES] = {
diff --git a/xen/arch/arm/numa_device_tree.c b/xen/arch/arm/numa_device_tree.c
new file mode 100644
index 0000000000..83601c83e7
--- /dev/null
+++ b/xen/arch/arm/numa_device_tree.c
@@ -0,0 +1,64 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Arm Architecture support layer for device tree NUMA.
+ *
+ * Copyright (C) 2023 Arm Ltd
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+#include <xen/init.h>
+#include <xen/nodemask.h>
+#include <xen/numa.h>
+#include <xen/libfdt/libfdt.h>
+#include <xen/device_tree.h>
+
+/* Callback for device tree processor affinity */
+static int __init fdt_numa_processor_affinity_init(nodeid_t node)
+{
+    numa_set_processor_nodes_parsed(node);
+    device_tree_numa = DT_NUMA_ON;
+
+    printk(KERN_INFO "DT: NUMA node %"PRIu8" processor parsed\n", node);
+
+    return 0;
+}
+
+/* Parse CPU NUMA node info */
+static int __init fdt_parse_numa_cpu_node(const void *fdt, int node)
+{
+    unsigned int nid;
+
+    if ( numa_disabled() )
+        return -EINVAL;
+
+    /*
+     * device_tree_get_u32 will return NUMA_NO_NODE when this CPU
+     * DT node doesn't have numa-node-id. This can help us to
+     * distinguish a bad DTB and a normal DTB without NUMA info.
+     */
+    nid = device_tree_get_u32(fdt, node, "numa-node-id", NUMA_NO_NODE);
+    if ( nid == NUMA_NO_NODE )
+    {
+        numa_fw_bad();
+        return -ENODATA;
+    }
+    else if ( nid >= MAX_NUMNODES )
+    {
+        printk(XENLOG_ERR "DT: CPU NUMA node id %u is invalid\n", nid);
+        numa_fw_bad();
+        return -EINVAL;
+    }
+
+    return fdt_numa_processor_affinity_init(nid);
+}