Hello Vijay,
On 09/02/17 15:56, vijay.kil...@gmail.com wrote:
From: Vijaya Kumar K <vijaya.ku...@cavium.com>
Parse memory node and fetch numa-node-id information.
For each memory range, store in node_memblk_range[]
along with node id.
Signed-off-by: Vijaya Kumar K <vijaya.ku...@cavium.com>
---
xen/arch/arm/bootfdt.c | 4 +--
xen/arch/arm/dt_numa.c | 84 ++++++++++++++++++++++++++++++++++++++++++-
xen/common/numa.c | 8 +++++
xen/include/xen/device_tree.h | 3 ++
xen/include/xen/numa.h | 1 +
5 files changed, 97 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index d1122d8..5e2df92 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -56,8 +56,8 @@ static bool_t __init device_tree_node_compatible(const void
*fdt, int node,
return 0;
}
-static void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
- u32 size_cells, u64 *start, u64 *size)
+void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
+ u32 size_cells, u64 *start, u64 *size)
{
*start = dt_next_cell(address_cells, cell);
*size = dt_next_cell(size_cells, cell);
diff --git a/xen/arch/arm/dt_numa.c b/xen/arch/arm/dt_numa.c
index 4b94c36..fce9e67 100644
--- a/xen/arch/arm/dt_numa.c
+++ b/xen/arch/arm/dt_numa.c
@@ -27,6 +27,7 @@
#include <xen/numa.h>
nodemask_t numa_nodes_parsed;
+extern struct node node_memblk_range[NR_NODE_MEMBLKS];
This should have been declared in an header (likely in patch #3).
/*
* Even though we connect cpus to numa domains later in SMP
@@ -48,11 +49,73 @@ static int __init dt_numa_process_cpu_node(const void *fdt,
int node,
return 0;
}
+static int __init dt_numa_process_memory_node(const void *fdt, int node,
+ const char *name,
+ u32 address_cells,
+ u32 size_cells)
Rather than reimplementing the wheel, it might be better to hook the
parsing in bootfdt.c. This would avoid an extra parsing of the
device-tree, duplicate the code and expose primitive for early DT parsing.
If parsing NUMA information can be done after the DT has been
unflattened, this will be even better.
+{
+ const struct fdt_property *prop;
+ int i, ret, banks;
Both banks and i should be unsigned.
+ const __be32 *cell;
+ paddr_t start, size;
+ u32 reg_cells = address_cells + size_cells;
+ u32 nid;
+
+ if ( address_cells < 1 || size_cells < 1 )
+ {
+ printk(XENLOG_WARNING
+ "fdt: node `%s': invalid #address-cells or #size-cells", name);
+ return -EINVAL;
+ }
+
+ nid = device_tree_get_u32(fdt, node, "numa-node-id", MAX_NUMNODES);
+ if ( nid >= MAX_NUMNODES) {
Coding style
if ( ... )
{
+ /*
+ * No node id found. Skip this memory node.
+ */
This could be a single line:
/* ..... */
So no warning if it is impossible to get the numa-node-id? Also, I don't
think this is right to boot using NUMA on platform having a buggy DT. So
we should probably return an error and disable NUMA.
+ return 0;
+ }
+
+ prop = fdt_get_property(fdt, node, "reg", NULL);
+ if ( !prop )
+ {
+ printk(XENLOG_WARNING "fdt: node `%s': missing `reg' property\n",
+ name);
+ return -EINVAL;
+ }
+
+ cell = (const __be32 *)prop->data;
+ banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
+
+ for ( i = 0; i < banks; i++ )
+ {
+ device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
+ if ( !size )
+ continue;
+
+ /* It is fine to add this area to the nodes data it will be used
later*/
+ ret = conflicting_memblks(start, start + size);
+ if (ret < 0)
+ numa_add_memblk(nid, start, size);
numa_add_memblk rely on the caller to check whether the array is not
full. I think we should move the check in numa_add_memblk and return an
error in this case.
+ else
+ {
+ printk(XENLOG_ERR
+ "NUMA DT: node %u (%"PRIx64"-%"PRIx64") overlaps with ret %d
(%"PRIx64"-%"PRIx64")\n",
+ nid, start, start + size, ret,
+ node_memblk_range[i].start, node_memblk_range[i].end);
i != ret. Please use the correct variable accordingly. However, I don't
think the overlap range really matters here.
+ return -EINVAL;
+ }
+ }
+
+ node_set(nid, numa_nodes_parsed);
+
+ return 0;
+}
+
static int __init dt_numa_scan_cpu_node(const void *fdt, int node,
const char *name, int depth,
u32 address_cells, u32 size_cells,
void *data)
-
Spurious change. Please don't add the blank line at the first place
(patch #6).
{
if ( device_tree_node_matches(fdt, node, "cpu") )
return dt_numa_process_cpu_node(fdt, node, name, address_cells,
@@ -61,6 +124,18 @@ static int __init dt_numa_scan_cpu_node(const void *fdt,
int node,
return 0;
}
+static int __init dt_numa_scan_memory_node(const void *fdt, int node,
+ const char *name, int depth,
+ u32 address_cells, u32 size_cells,
+ void *data)
+{
+ if ( device_tree_node_matches(fdt, node, "memory") )
+ return dt_numa_process_memory_node(fdt, node, name, address_cells,
+ size_cells);
Similarly to the CPUs, this code is wrong. You should check the type =
"memory".
+
+ return 0;
+}
+
int __init dt_numa_init(void)
{
int ret;
@@ -68,5 +143,12 @@ int __init dt_numa_init(void)
nodes_clear(numa_nodes_parsed);
ret = device_tree_for_each_node((void *)device_tree_flattened,
dt_numa_scan_cpu_node, NULL);
+
+ if ( ret )
+ return ret;
+
+ ret = device_tree_for_each_node((void *)device_tree_flattened,
+ dt_numa_scan_memory_node, NULL);
+
return ret;
}
diff --git a/xen/common/numa.c b/xen/common/numa.c
index 9b9cf9c..62c76af 100644
--- a/xen/common/numa.c
+++ b/xen/common/numa.c
@@ -55,6 +55,14 @@ struct node node_memblk_range[NR_NODE_MEMBLKS];
nodeid_t memblk_nodeid[NR_NODE_MEMBLKS];
struct node nodes[MAX_NUMNODES] __initdata;
+void __init numa_add_memblk(nodeid_t nodeid, u64 start, u64 size)
Please replace u64 by paddr_t.
+{
+ node_memblk_range[num_node_memblks].start = start;
+ node_memblk_range[num_node_memblks].end = start + size;
+ memblk_nodeid[num_node_memblks] = nodeid;
+ num_node_memblks++;
+}
You probably want to factor the code in acpi_numa_memory_affinity_init
to create this function.
Also, you don't check if the array is full.
+
int valid_numa_range(u64 start, u64 end, nodeid_t node)
{
#ifdef CONFIG_NUMA
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index de6b351..d92e47e 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -192,6 +192,9 @@ bool_t device_tree_node_matches(const void *fdt, int node,
const char *match);
u32 device_tree_get_u32(const void *fdt, int node,
const char *prop_name, u32 dflt);
+void device_tree_get_reg(const __be32 **cell, u32 address_cells,
+ u32 size_cells, u64 *start, u64 *size);
+
Same remark as on patch #6 for the position of the declaration.
/**
* dt_unflatten_host_device_tree - Unflatten the host device tree
*
diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
index 77c5cfd..9392a89 100644
--- a/xen/include/xen/numa.h
+++ b/xen/include/xen/numa.h
@@ -67,6 +67,7 @@ static inline __attribute__((pure)) nodeid_t
phys_to_nid(paddr_t addr)
#define clear_node_cpumask(cpu) do {} while (0)
#endif /* CONFIG_NUMA */
+extern void numa_add_memblk(nodeid_t nodeid, u64 start, u64 size);
extern int valid_numa_range(u64 start, u64 end, nodeid_t node);
extern int conflicting_memblks(u64 start, u64 end);
extern void cutoff_node(int i, u64 start, u64 end);
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
