Re: [XEN RFC PATCH 18/40] xen/arm: Keep memory nodes in dtb for NUMA when boot from EFI

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 27/08/2021 00:24, Stefano Stabellini wrote:
> On Wed, 11 Aug 2021, Wei Chen wrote:
> > EFI can get memory map from EFI system table. But EFI system
> > table doesn't contain memory NUMA information, EFI depends on
> > ACPI SRAT or device tree memory node to parse memory blocks'
> > NUMA mapping.
> >
> > But in current code, when Xen is booting from EFI, it will
> > delete all memory nodes in device tree. So in UEFI + DTB
> > boot, we don't have numa-node-id for memory blocks any more.
> >
> > So in this patch, we will keep memory nodes in device tree for
> > NUMA code to parse memory numa-node-id later.
> >
> > As a side effect, if we still parse boot memory information in
> > early_scan_node, bootmem.info will calculate memory ranges in
> > memory nodes twice. So we have to prvent early_scan_node to
> > parse memory nodes in EFI boot.
> >
> > As EFI APIs only can be used in Arm64, so we introduced a wrapper
> > in header file to prevent #ifdef CONFIG_ARM_64/32 in code block.
> >
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > ---
> >   xen/arch/arm/bootfdt.c      |  8 +++++++-
> >   xen/arch/arm/efi/efi-boot.h | 25 -------------------------
> >   xen/include/asm-arm/setup.h |  6 ++++++
> >   3 files changed, 13 insertions(+), 26 deletions(-)
> >
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index 476e32e0f5..7df149dbca 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -11,6 +11,7 @@
> >   #include <xen/lib.h>
> >   #include <xen/kernel.h>
> >   #include <xen/init.h>
> > +#include <xen/efi.h>
> >   #include <xen/device_tree.h>
> >   #include <xen/libfdt/libfdt.h>
> >   #include <xen/sort.h>
> > @@ -335,7 +336,12 @@ static int __init early_scan_node(const void *fdt,
> >   {
> >       int rc = 0;
> >   
> > -    if ( device_tree_node_matches(fdt, node, "memory") )
> > +    /*
> > +     * If system boot from EFI, bootinfo.mem has been set by EFI,
> > +     * so we don't need to parse memory node from DTB.
> > +     */
> > +    if ( device_tree_node_matches(fdt, node, "memory") &&
> > +         !arch_efi_enabled(EFI_BOOT) )
> >           rc = process_memory_node(fdt, node, name, depth,
> >                                    address_cells, size_cells, &bootinfo.mem);
> >       else if ( depth == 1 && !dt_node_cmp(name, "reserved-memory") )
>
> If we are going to use the device tree info for the numa nodes (and
> related memory) does it make sense to still rely on the EFI tables for
> the memory map?
Yes. AFAIK, when booting using EFI, the Device-Tree may not contain all 
the reserved regions. Furthermore, we are still too early to know 
whether we boot using ACPI and DT.
> I wonder if we should just use device tree for memory and ignore EFI
> instead. Do you know what Linux does in this regard?
I looked at Linux when I first reviewed this patch because I was 
wondering what happens if the DT and UEFI map disagrees.
Linux and Xen are the same after this patch:
  1) The memory map is coming from UEFI map
  2) NUMA ID is coming from the DT

The commit that introduced the change in Linux is:

commit 500899c2cc3e3f06140373b587a69d30650f2d9d
Author: Ard Biesheuvel <ardb@xxxxxxxxxx>
Date:   Fri Apr 8 15:50:23 2016 -0700

    efi: ARM/arm64: ignore DT memory nodes instead of removing them

    There are two problems with the UEFI stub DT memory node removal
    routine:
    - it deletes nodes as it traverses the tree, which happens to work
      but is not supported, as deletion invalidates the node iterator;
    - deleting memory nodes entirely may discard annotations in the form
      of additional properties on the nodes.

    Since the discovery of DT memory nodes occurs strictly before the
    UEFI init sequence, we can simply clear the memblock memory table
    before parsing the UEFI memory map. This way, it is no longer
    necessary to remove the nodes, so we can remove that logic from the
    stub as well.

    Reviewed-by: Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx>
    Acked-by: Steve Capper <steve.capper@xxxxxxx>
    Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
    Signed-off-by: David Daney <david.daney@xxxxxxxxxx>
    Signed-off-by: Will Deacon <will.deacon@xxxxxxx>

Cheers,

--
Julien Grall