|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] 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 <julien.grall@xxxxxxx> wrote:
> Hello Vijay,
>
>
> On 09/02/17 15:56, vijay.kilari@xxxxxxxxx wrote:
>>
>> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
>>
>> 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.Kumar@xxxxxxxxxx>
>> ---
>> 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.
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 overlap range really matters here.
OK
>
>> + 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".
if (!dt_node_type(node, "memory") ) should be fine?
>
>
>> +
>> + 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.
I think x86 can use this. I will make it part of initial code clean up.
>
>> +
>> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |