Re: [RFC PATCH v3 12/24] ARM: NUMA: DT: Parse CPU NUMA information

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

Hi,

On 18/07/17 12:41, 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.
> Refer to Documentation/devicetree/bindings/numa.txt
> in linux kernel.
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> ---
> v3: - Parse cpu nodes under path /cpus
>     - Move changes to bootfdt.c as separate patch
>     - Set numa_off on dt_numa_init() failure
> ---
>  xen/arch/arm/Makefile       |  1 +
>  xen/arch/arm/numa/Makefile  |  2 ++
>  xen/arch/arm/numa/dt_numa.c | 77 +++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/numa/numa.c    | 48 ++++++++++++++++++++++++++++
>  xen/arch/arm/setup.c        |  4 +++
>  xen/include/asm-arm/numa.h  | 10 +++++-
>  6 files changed, 141 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 49e1fb2..a89be66 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/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..963bb40
> --- /dev/null
> +++ b/xen/arch/arm/numa/dt_numa.c
> @@ -0,0 +1,77 @@
> +/*
> + * 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/mm.h>
> +#include <xen/nodemask.h>
> +#include <xen/libfdt/libfdt.h>
> +#include <xen/device_tree.h>

Again, this include should not be there as the device tree is not yet 
parsed.

> +#include <xen/numa.h>
> +#include <asm/setup.h>

Again, please order in alphabetically the includes...

> +
> +/*
> + * 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, offset;
> +    uint32_t nid;
> +
> +    offset = fdt_path_offset(fdt, "/cpus");
> +    if ( offset < 0 )
> +        return -EINVAL;
> +
> +    node = fdt_first_subnode(fdt, offset);
> +    if ( node == -FDT_ERR_NOTFOUND )
> +        return -EINVAL;
> +
> +    do {
> +        if ( device_tree_type_matches(fdt, node, "cpu") )
> +        {
> +            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);
> +        }
> +
> +        offset = node;
> +        node = fdt_next_subnode(fdt, offset);
> +    } while (node != -FDT_ERR_NOTFOUND);
> +
> +    return 0;
> +}
> +
> +int __init dt_numa_init(void)
> +{
> +    int ret;
> +
> +    ret = dt_numa_process_cpu_node((void *)device_tree_flattened);
> +
> +    return ret;

return dt_numa_process_cpu_node(....);

But I am still not sure to understand why you can't parse the numa node 
in directly in bootfdt.c as you do for the memory.

> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/numa/numa.c b/xen/arch/arm/numa/numa.c
> new file mode 100644
> index 0000000..45cc418
> --- /dev/null
> +++ b/xen/arch/arm/numa/numa.c
> @@ -0,0 +1,48 @@
> +/*
> + * ARM NUMA Implementation
> + *
> + * Copyright (C) 2016 - Cavium Inc.
> + * Vijaya Kumar K <vijaya.kumar@cavium.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions 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.
> + */
> +
> +#include <xen/init.h>
> +#include <xen/ctype.h>
> +#include <xen/nodemask.h>
> +#include <xen/numa.h>
> +
> +void __init numa_init(void)
> +{
> +    int ret = 0;
> +
> +    nodes_clear(processor_nodes_parsed);

Why do you need to clear processor_nodes_parsed? It should already be 
all zeroed.

> +    if ( numa_off )
> +        goto no_numa;
> +
> +    ret = dt_numa_init();
> +    if ( ret )
> +    {
> +        numa_off = true;
> +        printk(XENLOG_WARNING "DT NUMA init failed\n");
> +    }
> +
> +no_numa:

	printk("No NUMA support\n"); or something similar.

And to be honest, this label does not seem really useful...

> +    return;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 3b34855..a6d1499 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -38,6 +38,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>
> @@ -755,6 +756,9 @@ void __init start_xen(unsigned long boot_phys_offset,
>      /* Parse the ACPI tables for possible boot-time configuration */
>      acpi_boot_table_init();
>
> +    /* numa_init parses acpi tables. So call after acpi init */
> +    numa_init();
> +
>      end_boot_allocator();
>
>      vm_init();
> diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h
> index 7f00a36..8f517a2 100644
> --- a/xen/include/asm-arm/numa.h
> +++ b/xen/include/asm-arm/numa.h
> @@ -3,7 +3,15 @@
>
>  typedef uint8_t nodeid_t;
>
> -#ifndef CONFIG_NUMA
> +#ifdef CONFIG_NUMA
> +void numa_init(void);
> +int dt_numa_init(void);
> +#else
> +static inline void numa_init(void)
> +{
> +    return;
> +}
> +
>  /* Fake one node for now. See also node_online_map. */
>  #define cpu_to_node(cpu) 0
>  #define node_to_cpumask(node)   (cpu_online_map)
>

Cheers,

-- 
Julien Grall

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