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 <sanasta...@raptorengineering.com>
> ---
> 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
