Re: [PATCH v5 1/3] xen/device-tree: Let DT reserve map entries overlap reserved-memory

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.09.2024 00:24, Shawn Anastasio wrote:
> Commit 53dc37829c31 ("xen/arm: Add DT reserve map regions to
> bootinfo.reserved_mem") changes the way reserve map regions are tracked,
> and as a result broke bootfdt's ability to handle device trees in which
> the reserve map and the `reserved-memory` node contain the same entries
> as each other, as is the case on PPC when booted by skiboot.
> 
> Fix this behavior by moving the reserve map check to after the DT has
> been parsed and by explicitly allowing overlap with entries created by
> `reserved-memory` nodes.
> 
> Fixes: 53dc37829c31 ("xen/arm: Add DT reserve map regions to 
> bootinfo.reserved_mem")
> Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> ---
>  xen/common/device-tree/bootfdt.c  | 28 +++++++++++++++++++++++-----
>  xen/common/device-tree/bootinfo.c | 11 +++++++++--
>  xen/include/xen/bootfdt.h         |  3 ++-
>  3 files changed, 34 insertions(+), 8 deletions(-)

DT maintainers?

Jan

> diff --git a/xen/common/device-tree/bootfdt.c 
> b/xen/common/device-tree/bootfdt.c
> index 911a630e7d..2a51ee44a3 100644
> --- a/xen/common/device-tree/bootfdt.c
> +++ b/xen/common/device-tree/bootfdt.c
> @@ -177,7 +177,7 @@ static int __init device_tree_get_meminfo(const void 
> *fdt, int node,
>      {
>          device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
>          if ( mem == bootinfo_get_reserved_mem() &&
> -             check_reserved_regions_overlap(start, size) )
> +             check_reserved_regions_overlap(start, size, NULL) )
>              return -EINVAL;
>          /* Some DT may describe empty bank, ignore them */
>          if ( !size )
> @@ -590,14 +590,36 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t 
> paddr)
>      if ( nr_rsvd < 0 )
>          panic("Parsing FDT memory reserve map failed (%d)\n", nr_rsvd);
>  
> +    ret = device_tree_for_each_node(fdt, 0, early_scan_node, NULL);
> +    if ( ret )
> +        panic("Early FDT parsing failed (%d)\n", ret);
> +
>      for ( i = 0; i < nr_rsvd; i++ )
>      {
> +        const struct membanks *overlap = NULL;
>          struct membank *bank;
>          paddr_t s, sz;
>  
>          if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, &s, &sz) < 0 )
>              continue;
>  
> +        if ( check_reserved_regions_overlap(s, sz, &overlap) )
> +        {
> +            if ( overlap == bootinfo_get_reserved_mem() )
> +            {
> +                /*
> +                 * Some valid device trees, such as those generated by 
> OpenPOWER
> +                 * skiboot firmware, expose all reserved memory regions in 
> the
> +                 * FDT memory reservation block (here) AND in the
> +                 * reserved-memory node which has already been parsed. Thus, 
> any
> +                 * overlaps in the mem_reserved banks should be ignored.
> +                 */
> +                 continue;
> +            }
> +            else
> +                panic("FDT reserve map overlapped with membanks/modules\n");
> +        }
> +
>          if ( reserved_mem->nr_banks < reserved_mem->max_banks )
>          {
>              bank = &reserved_mem->bank[reserved_mem->nr_banks];
> @@ -610,10 +632,6 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t 
> paddr)
>              panic("Cannot allocate reserved memory bank\n");
>      }
>  
> -    ret = device_tree_for_each_node(fdt, 0, early_scan_node, NULL);
> -    if ( ret )
> -        panic("Early FDT parsing failed (%d)\n", ret);
> -
>      /*
>       * On Arm64 setup_directmap_mappings() expects to be called with the 
> lowest
>       * bank in memory first. There is no requirement that the DT will provide
> diff --git a/xen/common/device-tree/bootinfo.c 
> b/xen/common/device-tree/bootinfo.c
> index f2e6a1145b..c1752bfdc8 100644
> --- a/xen/common/device-tree/bootinfo.c
> +++ b/xen/common/device-tree/bootinfo.c
> @@ -171,7 +171,8 @@ void __init fw_unreserved_regions(paddr_t s, paddr_t e,
>   * existing reserved memory regions, otherwise false.
>   */
>  bool __init check_reserved_regions_overlap(paddr_t region_start,
> -                                           paddr_t region_size)
> +                                           paddr_t region_size,
> +                                           const struct membanks 
> **out_overlapping_membanks)
>  {
>      const struct membanks *mem_banks[] = {
>          bootinfo_get_reserved_mem(),
> @@ -190,8 +191,14 @@ bool __init check_reserved_regions_overlap(paddr_t 
> region_start,
>       * shared memory banks (when static shared memory feature is enabled)
>       */
>      for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ )
> +    {
>          if ( meminfo_overlap_check(mem_banks[i], region_start, region_size) )
> +        {
> +            if ( out_overlapping_membanks )
> +                *out_overlapping_membanks = mem_banks[i];
>              return true;
> +        }
> +    }
>  
>      /* Check if input region is overlapping with bootmodules */
>      if ( bootmodules_overlap_check(&bootinfo.modules,
> @@ -216,7 +223,7 @@ struct bootmodule __init *add_boot_module(bootmodule_kind 
> kind,
>          return NULL;
>      }
>  
> -    if ( check_reserved_regions_overlap(start, size) )
> +    if ( check_reserved_regions_overlap(start, size, NULL) )
>          return NULL;
>  
>      for ( i = 0 ; i < mods->nr_mods ; i++ )
> diff --git a/xen/include/xen/bootfdt.h b/xen/include/xen/bootfdt.h
> index 16fa05f38f..03e1d5fde8 100644
> --- a/xen/include/xen/bootfdt.h
> +++ b/xen/include/xen/bootfdt.h
> @@ -158,7 +158,8 @@ struct bootinfo {
>  extern struct bootinfo bootinfo;
>  
>  bool check_reserved_regions_overlap(paddr_t region_start,
> -                                    paddr_t region_size);
> +                                    paddr_t region_size,
> +                                    const struct membanks 
> **out_overlapping_membanks);
>  
>  struct bootmodule *add_boot_module(bootmodule_kind kind,
>                                     paddr_t start, paddr_t size, bool domU);