|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH v1 06/21] ARM: NUMA: Parse CPU NUMA information
On Mon, Feb 20, 2017 at 11:02 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 CPU node and fetch numa-node-id information.
>> For each node-id found, update nodemask_t mask.
>
>
> A link to the bindings would have been useful...
>
>> Call numa_init() from setup_mm() with start and end
>> pfn of the complete ram..
>
>
> s/.././
>
>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
>> ---
>> xen/arch/arm/Makefile | 1 +
>> xen/arch/arm/bootfdt.c | 8 ++---
>> xen/arch/arm/dt_numa.c | 72
>> +++++++++++++++++++++++++++++++++++++++++++
>> xen/arch/arm/numa.c | 14 +++++++++
>> xen/arch/arm/setup.c | 3 ++
>> xen/include/asm-arm/numa.h | 14 +++++++++
>> xen/include/xen/device_tree.h | 4 +++
>> 7 files changed, 112 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index b5d7a19..7694485 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -50,6 +50,7 @@ obj-y += vtimer.o
>> obj-y += vpsci.o
>> obj-y += vuart.o
>> obj-$(CONFIG_NUMA) += numa.o
>> +obj-$(CONFIG_NUMA) += dt_numa.o
>
>
> I would prefer if we introduce a directly numa within arm. This would make
> the root cleaner.
As it is very specific to DT, I have introduced in this file. ACPI is
implemented
in separate file. Common arm specific numa code is in numa.c file.
>
>
>>
>> #obj-bin-y += ....o
>>
>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
>> index 979f675..d1122d8 100644
>> --- a/xen/arch/arm/bootfdt.c
>> +++ b/xen/arch/arm/bootfdt.c
>> @@ -17,8 +17,8 @@
>> #include <xsm/xsm.h>
>> #include <asm/setup.h>
>>
>> -static bool_t __init device_tree_node_matches(const void *fdt, int node,
>> - const char *match)
>> +bool_t __init device_tree_node_matches(const void *fdt, int node,
>> + const char *match)
>> {
>> const char *name;
>> size_t match_len;
>> @@ -63,8 +63,8 @@ static void __init device_tree_get_reg(const __be32
>> **cell, u32 address_cells,
>> *size = dt_next_cell(size_cells, cell);
>> }
>>
>> -static u32 __init device_tree_get_u32(const void *fdt, int node,
>> - const char *prop_name, u32 dflt)
>> +u32 __init device_tree_get_u32(const void *fdt, int node,
>> + const char *prop_name, u32 dflt)
>> {
>> const struct fdt_property *prop;
>>
>> diff --git a/xen/arch/arm/dt_numa.c b/xen/arch/arm/dt_numa.c
>> new file mode 100644
>> index 0000000..4b94c36
>> --- /dev/null
>> +++ b/xen/arch/arm/dt_numa.c
>> @@ -0,0 +1,72 @@
>> +/*
>> + * OF NUMA Parsing support.
>> + *
>> + * Copyright (C) 2015 - 2016 Cavium Inc.
>> + *
>> + * Some code extracts are taken from linux drivers/of/of_numa.c
>
>
> Please specify which code has been extract from Linux and the commit id.
>
> Looking at this patch only, none of this code is from Linux. Or it has been
> heavily modified.
It is heavily modified. I will drop this statement.
>
>> + *
>> + * 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/config.h>
>
>
> No need to include xen/config.h
>
>> +#include <xen/device_tree.h>
>
>
> This include should not be there as the device tree is not yet parsed.
>
>> +#include <xen/libfdt/libfdt.h>
>> +#include <xen/mm.h>
>> +#include <xen/nodemask.h>
>> +#include <asm/mm.h>
>> +#include <xen/numa.h>
>> +
>> +nodemask_t numa_nodes_parsed;
>
>
> Why does this variable live in dt_numa.c when this is used by common and
> acpi code?
>
> Also, any variable/function exported should have there prototype in the
> associated header.
ok. will check.
>
>> +
>> +/*
>> + * Even though we connect cpus to numa domains later in SMP
>> + * init, we need to know the node ids now for all cpus.
>> +*/
>
>
> Coding style:
>
> /*
> * ...
>
> */
>
>> +static int __init dt_numa_process_cpu_node(const void *fdt, int node,
>> + const char *name,
>> + u32 address_cells, u32
>> size_cells)
>> +{
>> + u32 nid;
>> +
>> + nid = device_tree_get_u32(fdt, node, "numa-node-id", MAX_NUMNODES);
>> +
>> + if ( nid >= MAX_NUMNODES )
>> + printk(XENLOG_WARNING "NUMA: Node id %u exceeds maximum value\n",
>> nid);
>> + else
>> + 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,
>> + size_cells);
>
>
> This code is wrong. CPUs nodes can only be in /cpus and you cannot rely on
> the name to be "cpu" (see binding in
> Documentation/devicetree/bindings/arm/cpu.txt). The only way to check if it
> is a CPU is to look for the property device_type.
OK
>
>> +
>> + return 0;
>> +}
>> +
>> +int __init dt_numa_init(void)
>> +{
>> + int ret;
>> +
>> + nodes_clear(numa_nodes_parsed);
>
>
> Why this is done in dt_numa_init and not numa_init?
ok.
>
>
>> + ret = device_tree_for_each_node((void *)device_tree_flattened,
>> + dt_numa_scan_cpu_node, NULL);
>> + return ret;
>> +}
>> diff --git a/xen/arch/arm/numa.c b/xen/arch/arm/numa.c
>> index 59d09c7..9a7f0bb 100644
>> --- a/xen/arch/arm/numa.c
>> +++ b/xen/arch/arm/numa.c
>> @@ -21,6 +21,20 @@
>> #include <xen/nodemask.h>
>> #include <asm/mm.h>
>> #include <xen/numa.h>
>> +#include <asm/acpi.h>
>> +
>> +int __init numa_init(void)
>> +{
>> + int ret = 0;
>> +
>> + if ( numa_off )
>> + goto no_numa;
>> +
>> + ret = dt_numa_init();
>> +
>> +no_numa:
>> + return ret;
>> +}
>>
>> int __init arch_numa_setup(char *opt)
>> {
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 049e449..b6618ca 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -39,6 +39,7 @@
>> #include <xen/libfdt/libfdt.h>
>> #include <xen/acpi.h>
>> #include <asm/alternative.h>
>> +#include <xen/numa.h>
>> #include <asm/page.h>
>> #include <asm/current.h>
>> #include <asm/setup.h>
>> @@ -753,6 +754,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>> /* Parse the ACPI tables for possible boot-time configuration */
>> acpi_boot_table_init();
>>
>> + numa_init();
>
>
> I see very little point to have a function return a value but never check
> it. If the return does not matter, then the function should be void.
ok
>
>> +
>> end_boot_allocator();
>>
>> vm_init();
>> diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h
>> index c1e8a7d..cdfeecd 100644
>> --- a/xen/include/asm-arm/numa.h
>> +++ b/xen/include/asm-arm/numa.h
>> @@ -1,18 +1,32 @@
>> #ifndef __ARCH_ARM_NUMA_H
>> #define __ARCH_ARM_NUMA_H
>>
>> +#include <xen/errno.h>
>> +
>> typedef u8 nodeid_t;
>>
>> #define NODES_SHIFT 2
>>
>> #ifdef CONFIG_NUMA
>> int arch_numa_setup(char *opt);
>> +int __init numa_init(void);
>
>
> Please drop __init, it should only be only on the declaration.
ok
>
>> +int __init dt_numa_init(void);
>
>
> Ditto.
>
>> #else
>> static inline int arch_numa_setup(char *opt)
>> {
>> return 1;
>> }
>>
>> +static inline int __init numa_init(void)
>> +{
>> + return 0;
>> +}
>> +
>> +static inline int __init dt_numa_init(void)
>> +{
>> + return -EINVAL;
>> +}
>
>
> This wrapper should never be called when NUMA is disabled. So please drop
> it.
ok
>
>> +
>> /* Fake one node for now. See also node_online_map. */
>> #define cpu_to_node(cpu) 0
>> #define node_to_cpumask(node) (cpu_online_map)
>> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
>> index 0aecbe0..de6b351 100644
>> --- a/xen/include/xen/device_tree.h
>> +++ b/xen/include/xen/device_tree.h
>> @@ -188,6 +188,10 @@ int device_tree_for_each_node(const void *fdt,
>> device_tree_node_func func,
>> void *data);
>>
>> +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);
>
>
> Please don't mix common and arch code. Those functions are arch specific.
> This should be defined in asm/setup.h.
ok
>
>> /**
>> * dt_unflatten_host_device_tree - Unflatten the host device tree
>> *
>>
>
> 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 |