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 <[email protected]>
> ---
> 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);
