Re: [RFC PATCH v1 06/21] ARM: NUMA: Parse CPU NUMA information

[Julien Grall <julien.grall@arm.com>]

Hello Vijay,

On 09/02/17 15:56, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>
> 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@cavium.com>
> ---
>  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.

>
>  #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.

> + *
> + * 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.

> +
> +/*
> + * 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.

> +
> +    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?

> +    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.

> +
>      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.

> +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.

> +
>  /* 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.

>  /**
>   * dt_unflatten_host_device_tree - Unflatten the host device tree
>   *
>

Regards,

-- 
Julien Grall

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