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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Vijay,

On 18/07/17 12:41, vijay.kilari@xxxxxxxxx wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
>
> Parse memory node and fetch numa-node-id information.
> For each memory range, store in node_memblk_range[]
> along with node id.
>
> When booting in UEFI mode, UEFI passes memory information
> to Dom0 using EFI memory descriptor table and deletes the
> memory nodes from the host DT. However to fetch the memory
> numa node id, memory DT node should not be deleted by EFI stub.
> With this patch, do not delete memory node from FDT.
>
> NUMA info of memory is extracted from process_memory_node()
> instead of parsing the DT again during numa_init().
This patch does too much and needs to be split. The splitting would be 
at least:
- EFI mode change
- Numa change

> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
> ---
> v3: - Set numa_off in numa_failed() and drop dt_numa variable
> ---
>  xen/arch/arm/bootfdt.c      | 25 +++++++++++++++++++++----
>  xen/arch/arm/efi/efi-boot.h | 25 -------------------------
>  xen/arch/arm/numa/dt_numa.c | 32 ++++++++++++++++++++++++++++++++
>  xen/arch/arm/numa/numa.c    |  5 +++++
>  xen/include/asm-arm/numa.h  |  2 ++
>  5 files changed, 60 insertions(+), 29 deletions(-)
>
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 6e8251b..b3a132c 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -13,6 +13,8 @@
>  #include <xen/init.h>
>  #include <xen/device_tree.h>
>  #include <xen/libfdt/libfdt.h>
> +#include <xen/numa.h>
> +#include <xen/efi.h>
Please add the headers in alphabetical order.

>  #include <xsm/xsm.h>
>  #include <asm/setup.h>
>
> @@ -146,6 +148,9 @@ static void __init process_memory_node(const void *fdt, int 
> node,
>      const __be32 *cell;
>      paddr_t start, size;
>      u32 reg_cells = address_cells + size_cells;
> +#ifdef CONFIG_NUMA
> +    uint32_t nid;
> +#endif
>
>      if ( address_cells < 1 || size_cells < 1 )
>      {
> @@ -154,24 +159,36 @@ static void __init process_memory_node(const void *fdt, 
> int node,
>          return;
>      }
>
> +#ifdef CONFIG_NUMA
> +    nid = device_tree_get_u32(fdt, node, "numa-node-id", NR_NODE_MEMBLKS);
Should not you use MAX_NUM_NODES rather than NR_NODE_MEMBLKS?

Also, where is the sanity check?

> +#endif
>      prop = fdt_get_property(fdt, node, "reg", NULL);
>      if ( !prop )
>      {
>          printk("fdt: node `%s': missing `reg' property\n", name);
> +#ifdef CONFIG_NUMA
> +       numa_failed();
This file is using soft-tab not hard one.

> +#endif
>          return;
>      }
>
>      cell = (const __be32 *)prop->data;
>      banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
>
> -    for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ )
> +    for ( i = 0; i < banks; i++ )
>      {
>          device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
>          if ( !size )
>              continue;
> -        bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
> -        bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
> -        bootinfo.mem.nr_banks++;
> +        if ( !efi_enabled(EFI_BOOT) && bootinfo.mem.nr_banks < NR_MEM_BANKS )
> +        {
> +            bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
> +            bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
> +            bootinfo.mem.nr_banks++;
> +        }
This change should be split.

> +#ifdef CONFIG_NUMA
> +        dt_numa_process_memory_node(nid, start, size);
> +#endif
>      }
>  }
>
> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index 56de26e..a8bde68 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -194,33 +194,8 @@ EFI_STATUS __init fdt_add_uefi_nodes(EFI_SYSTEM_TABLE 
> *sys_table,
>      int status;
>      u32 fdt_val32;
>      u64 fdt_val64;
> -    int prev;
>      int num_rsv;
>
> -    /*
> -     * Delete any memory nodes present.  The EFI memory map is the only
> -     * memory description provided to Xen.
> -     */
> -    prev = 0;
> -    for (;;)
> -    {
> -        const char *type;
> -        int len;
> -
> -        node = fdt_next_node(fdt, prev, NULL);
> -        if ( node < 0 )
> -            break;
> -
> -        type = fdt_getprop(fdt, node, "device_type", &len);
> -        if ( type && strncmp(type, "memory", len) == 0 )
> -        {
> -            fdt_del_node(fdt, node);
> -            continue;
> -        }
> -
> -        prev = node;
> -    }
> -
That chunk should move to the same patch as the EFI check.

>     /*
>      * Delete all memory reserve map entries. When booting via UEFI,
>      * kernel will use the UEFI memory map to find reserved regions.
> diff --git a/xen/arch/arm/numa/dt_numa.c b/xen/arch/arm/numa/dt_numa.c
> index 963bb40..84030e7 100644
> --- a/xen/arch/arm/numa/dt_numa.c
> +++ b/xen/arch/arm/numa/dt_numa.c
> @@ -58,6 +58,38 @@ static int __init dt_numa_process_cpu_node(const void *fdt)
>      return 0;
>  }
>
> +void __init dt_numa_process_memory_node(uint32_t nid, paddr_t start,
> +                                       paddr_t size)
> +{
> +    struct node *nd;
> +    int i;
> +
> +    i = conflicting_memblks(start, start + size);
> +    if ( i < 0 )
> +    {
> +         if ( numa_add_memblk(nid, start, size) )
> +         {
> +             printk(XENLOG_WARNING "DT: NUMA: node-id %u overflow \n", nid);
> +             numa_failed();
> +             return;
> +         }
> +    }
> +    else
> +    {
> +         nd = get_node_memblk_range(i);
> +         printk(XENLOG_ERR
> +                "NUMA DT: node %u (%"PRIx64"-%"PRIx64") overlaps with %d 
> (%"PRIx64"-%"PRIx64")\n",
s/PRIx64/PRI_paddr/

> +                nid, start, start + size, i, nd->start, nd->end);
> +
> +         numa_failed();
> +         return;
> +    }
> +
> +    node_set(nid, memory_nodes_parsed);
This code looks fairly similar to some bits of 
acpi_numa_memory_affinity_init. Is there any way we could introduce a 
common helper?
> +
> +    return;
> +}
> +
>  int __init dt_numa_init(void)
>  {
>      int ret;
> diff --git a/xen/arch/arm/numa/numa.c b/xen/arch/arm/numa/numa.c
> index 45cc418..8227361 100644
> --- a/xen/arch/arm/numa/numa.c
> +++ b/xen/arch/arm/numa/numa.c
> @@ -19,6 +19,11 @@
>  #include <xen/nodemask.h>
>  #include <xen/numa.h>
>
> +void numa_failed(void)
> +{
> +    numa_off = true;
> +}
> +
>  void __init numa_init(void)
>  {
>      int ret = 0;
> diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h
> index 8f517a2..36cd782 100644
> --- a/xen/include/asm-arm/numa.h
> +++ b/xen/include/asm-arm/numa.h
> @@ -3,6 +3,8 @@
>
>  typedef uint8_t nodeid_t;
>
> +void dt_numa_process_memory_node(uint32_t nid, paddr_t start, paddr_t size);
Likely, this should go under CONFIG_NUMA.

> +
>  #ifdef CONFIG_NUMA
>  void numa_init(void);
>  int dt_numa_init(void);
Cheers,

--
Julien Grall

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