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