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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Wed, Jul 19, 2017 at 11:56 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hi,
>
>
> On 18/07/17 12:41, 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 linux kernel.
>>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
>> ---
>> 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.

I believe that below code needs this header file.

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

IRC, Initially I was facing issue with this approach. I will re-look into it.

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

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

ok

>
>
>> +    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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel