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

[Michal Orzel <michal.orzel@xxxxxxx>]

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(-)
> 
> 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);
This should be moved before fdt_num_mem_rsv so that the program flow makes 
sense. In your case nr_rsvd is
not used immediately after.

> +    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;
I think this is incorrect. Imagine this scenario:
/memreserve/ 0x40000000 0x40000000;
and /reserved-memory/foo with:
reg = <0x0 0x7FFFF000 0x0 0x1000>;

You would ignore the entire region described with /memreserve/ even though it 
overlaps just the last page.

The problem you're describing is about regions that match 1:1 in /memreserve/ 
and /reserved-memory/.
Therefore I think you should check that the overlapped regions match exactly.

~Michal