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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: 2021年8月20日 1:35
> To: Wei Chen <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx; jbeulich@xxxxxxxx
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
> Subject: Re: [XEN RFC PATCH 18/40] xen/arm: Keep memory nodes in dtb for
> NUMA when boot from EFI
> 
> Hi Wei,
> 
> On 11/08/2021 11:24, 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
> 
> s/prvent/prevent/
> 

Ok

> > 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,
> 
> "If the system boot". Although, I would suggest to write:
> 
> "If Xen has been booted via UEFI, the memory banks will already be
> populated. So we should skip the parsing."
> 

Yes, that would be better. I will change it in next version.

> > +     * so we don't need to parse memory node from DTB.
> > +     */
> > +    if ( device_tree_node_matches(fdt, node, "memory") &&
> > +         !arch_efi_enabled(EFI_BOOT) )
> 
> arch_efi_enabled() is going to be less expensive than
> device_tree_node_matches(). So I would suggest to re-order the operands.
> 

yes.

> >           rc = process_memory_node(fdt, node, name, depth,
> >                                    address_cells, size_cells,
> &bootinfo.mem);
> >       else if ( depth == 1 && !dt_node_cmp(name, "reserved-memory") )
> > diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> > index cf9c37153f..d0a9987fa4 100644
> > --- a/xen/arch/arm/efi/efi-boot.h
> > +++ b/xen/arch/arm/efi/efi-boot.h
> > @@ -197,33 +197,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;
> > -    }
> > -
> >      /*
> >       * 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/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
> > index c4b6af6029..e4fb5f0d49 100644
> > --- a/xen/include/asm-arm/setup.h
> > +++ b/xen/include/asm-arm/setup.h
> > @@ -123,6 +123,12 @@ void device_tree_get_reg(const __be32 **cell, u32
> address_cells,
> >   u32 device_tree_get_u32(const void *fdt, int node,
> >                           const char *prop_name, u32 dflt);
> >
> > +#if defined(CONFIG_ARM_64)
> > +#define arch_efi_enabled(x) efi_enabled(x)
> > +#else
> > +#define arch_efi_enabled(x) (0)
> > +#endif
> 
> I would prefer if we introduce CONFIG_EFI that would stub efi_enabled
> for architecture not supporting EFI.
> 

Yes, that's a good idea.
I also feel a little awkward with the current arch helpers.

> Cheers,
> 
> --
> Julien Grall