Re: [Xen-devel] [RFC PATCH v1 07/21] ARM: NUMA: Parse memory NUMA information
On 02/03/17 15:08, Vijay Kilari wrote: On Thu, Mar 2, 2017 at 8:18 PM, Julien Grall wrote: Hello Vijay, On 02/03/17 12:25, Vijay Kilari wrote: On Mon, Feb 20, 2017 at 11:35 PM, Julien Grall wrote: On 09/02/17 15:56, vijay.kil...@gmail.com wrote: [...] 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. The process_memory_node() is called only if EFI_BOOT is not enabled. So hooking might not work. This series adds this change (see patch #5) and the code is not set in stone. I have not added process_memory_node() in the patch #5, I have only introduced the check. That's exactly what I said... We should rework the code when it makes sense rather than trying to find a more convolute way. I see two restrictions. 1) If we make a hook from process_memory_node(). This is called much earlier than numa_init(). 2) We cannot move numa_init() before setup_mm(). I didn't ask to move numa_init earlier but moving the parsing earlier. AFAICT, it does not require to allocate memory nor rely on anything initialized big in numa_init but nodes_clear(...). Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH v1 07/21] ARM: NUMA: Parse memory NUMA information
On Thu, Mar 2, 2017 at 8:18 PM, Julien Grall wrote: > Hello Vijay, > > On 02/03/17 12:25, Vijay Kilari wrote: >> >> On Mon, Feb 20, 2017 at 11:35 PM, Julien Grall >> wrote: >>> >>> On 09/02/17 15:56, vijay.kil...@gmail.com wrote: > > > [...] > >>> 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. >> >> >> The process_memory_node() is called only if EFI_BOOT is not enabled. So >> hooking might not work. > > > This series adds this change (see patch #5) and the code is not set in > stone. I have not added process_memory_node() in the patch #5, I have only introduced the check. > > We should rework the code when it makes sense rather than trying to find a > more convolute way. > I see two restrictions. 1) If we make a hook from process_memory_node(). This is called much earlier than numa_init(). 2) We cannot move numa_init() before setup_mm(). >>> >>> Similarly to the CPUs, this code is wrong. You should check the type = >>> "memory". >> >> >> if (!dt_node_type(node, "memory") ) should be fine? > > > You would have to create an helper for that. But the idea is there. > > Cheers, > > -- > Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH v1 07/21] ARM: NUMA: Parse memory NUMA information
Hello Vijay, On 02/03/17 12:25, Vijay Kilari wrote: On Mon, Feb 20, 2017 at 11:35 PM, Julien Grall wrote: On 09/02/17 15:56, vijay.kil...@gmail.com wrote: [...] 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. The process_memory_node() is called only if EFI_BOOT is not enabled. So hooking might not work. This series adds this change (see patch #5) and the code is not set in stone. We should rework the code when it makes sense rather than trying to find a more convolute way. Similarly to the CPUs, this code is wrong. You should check the type = "memory". if (!dt_node_type(node, "memory") ) should be fine? You would have to create an helper for that. But the idea is there. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH v1 07/21] ARM: NUMA: Parse memory NUMA information
On Mon, Feb 20, 2017 at 11:35 PM, Julien Grall wrote:
> Hello Vijay,
>
>
> On 09/02/17 15:56, vijay.kil...@gmail.com wrote:
>>
>> From: Vijaya Kumar K
>>
>> 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
>> ---
>> 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
>>
>> 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.
The process_memory_node() is called only if EFI_BOOT is not enabled. So
hooking might not work.
>
> 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.
OK.
>
>> +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.
OK
>
>> +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 o
Re: [Xen-devel] [RFC PATCH v1 07/21] ARM: NUMA: Parse memory NUMA information
Hello Vijay,
On 09/02/17 15:56, vijay.kil...@gmail.com wrote:
From: Vijaya Kumar K
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
---
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
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_ma
[Xen-devel] [RFC PATCH v1 07/21] ARM: NUMA: Parse memory NUMA information
From: Vijaya Kumar K
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
---
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
nodemask_t numa_nodes_parsed;
+extern struct node node_memblk_range[NR_NODE_MEMBLKS];
/*
* 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)
+{
+const struct fdt_property *prop;
+int i, ret, banks;
+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) {
+/*
+ * No node id found. Skip this memory node.
+ */
+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);
+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);
+ 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)
-
{
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);
+
+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, NUL
