Re: [Xen-devel] [RFC PATCH v2 12/25] ARM: NUMA: Parse CPU NUMA information

[Julien Grall <julien.grall@xxxxxxx>]

Hi Vijay,

The title likely needs to have the work device-tree/DT in it.

On 28/03/17 16:53, 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.
> Refer to /Documentation/devicetree/bindings/numa.txt.
In which repository?

> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
> ---
>  xen/arch/arm/Makefile       |  1 +
>  xen/arch/arm/bootfdt.c      | 16 ++++++++--
>  xen/arch/arm/numa/Makefile  |  2 ++
>  xen/arch/arm/numa/dt_numa.c | 78 +++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/numa/numa.c    | 50 +++++++++++++++++++++++++++++
>  xen/arch/arm/setup.c        |  4 +++
>  xen/include/asm-arm/numa.h  | 10 +++++-
>  xen/include/asm-arm/setup.h |  4 ++-
>  8 files changed, 161 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 0ce94a8..d13b79f 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -3,6 +3,7 @@ subdir-$(CONFIG_ARM_64) += arm64
>  subdir-y += platforms
>  subdir-$(CONFIG_ARM_64) += efi
>  subdir-$(CONFIG_ACPI) += acpi
> +subdir-$(CONFIG_NUMA) += numa
>
>  obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o
>  obj-y += bootfdt.init.o
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index ea188a0..1f876f0 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -62,8 +62,20 @@ 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)
> +bool_t __init device_tree_type_matches(const void *fdt, int node,
> +                                       const char *match)
> +{
> +    const void *prop;
> +
> +    prop = fdt_getprop(fdt, node, "device_type", NULL);
> +    if ( prop == NULL )
> +        return 0;
> +
> +    return strcmp(prop, match) == 0 ? 1 : 0;
> +}
> +
This change is not explained in the patch and does not belong to it anyway.

> +u32 __init device_tree_get_u32(const void *fdt, int node,
> +                               const char *prop_name, u32 dflt)
Ditto. I would recommend to read [1] for tips to break down a patch.

>  {
>      const struct fdt_property *prop;
>
> diff --git a/xen/arch/arm/numa/Makefile b/xen/arch/arm/numa/Makefile
> new file mode 100644
> index 0000000..3af3aff
> --- /dev/null
> +++ b/xen/arch/arm/numa/Makefile
> @@ -0,0 +1,2 @@
> +obj-y += dt_numa.o
> +obj-y += numa.o
> diff --git a/xen/arch/arm/numa/dt_numa.c b/xen/arch/arm/numa/dt_numa.c
> new file mode 100644
> index 0000000..66c6efb
> --- /dev/null
> +++ b/xen/arch/arm/numa/dt_numa.c
> @@ -0,0 +1,78 @@
> +/*
> + * OF NUMA Parsing support.
> + *
> + * Copyright (C) 2015 - 2016 Cavium Inc.
> + *
> + * 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/libfdt/libfdt.h>
> +#include <xen/mm.h>
> +#include <xen/nodemask.h>
> +#include <asm/mm.h>
This is already included by xen/mm.h

> +#include <xen/numa.h>
> +#include <xen/device_tree.h>
> +#include <asm/setup.h>
Please order the include.

> +
> +extern nodemask_t processor_nodes_parsed;
See my comment on patch #11. I may miss of them and hoping you will fix 
all the occurrence in the next version.
> +
> +/*
> + * Even though we connect cpus to numa domains later in SMP
> + * init, we need to know the node ids now for all cpus.
> + */
> +static int __init dt_numa_process_cpu_node(const void *fdt, int node,
> +                                           const char *name,
> +                                           uint32_t address_cells,
> +                                           uint32_t size_cells)
> +{
> +    uint32_t 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, processor_nodes_parsed);
> +
> +    return 0;
> +}
> +
> +static int __init dt_numa_scan_cpu_node(const void *fdt, int node,
> +                                        const char *name, int depth,
> +                                        uint32_t address_cells,
> +                                        uint32_t size_cells, void *data)
> +{
> +    if ( device_tree_type_matches(fdt, node, "cpu") )
> +        return dt_numa_process_cpu_node(fdt, node, name, address_cells,
> +                                        size_cells);
As said on v1, 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.
Cheers,

[1] https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel