|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH v2 12/25] ARM: NUMA: Parse CPU NUMA information
On Mon, May 8, 2017 at 11:01 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> 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.
OK.
>
>> +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.
The function device_tree_type_matches() isn't looking for device_type?.
Below is dt info on device_type. Anything missing?
cpu@10101 {
compatible = "cavium,thunder", "arm,armv8";
device_type = "cpu";
....
};
>
> 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |