On 04/11/2024 13:39, Grygorii Strashko wrote:
>
>
> Hi All,
>
> On 04.11.24 12:49, Michal Orzel wrote:
>>
>>
>> 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.
>>
>
> I've also discovered an issue with Commit 53dc37829c31 ("xen/arm: Add DT
> reserve map
> regions to bootinfo.reserved_mem") - the bootloader adds Initrd in
> FDT reserved map which then conflicts with Initrd module (ARM64).
>
> This patch, as is, doesn't fix an issue for me:
>
> (XEN) Checking for initrd in /chosen
> (XEN) Initrd 0000000084000040-0000000086152ac6
> (XEN) Region: [0x00000084000040, 0x00000086152ac6) overlapping with mod[2]:
> [0x00000084000040, 0x00000086152ac6)
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) FDT reserve map overlapped with membanks/modules
> (XEN) ****************************************
>
> So I did fast try of Michal Orzel suggestion and it seems working for me.
> And if it's working for PPC - may be that's it (feel free to incorporate).
> Diff below.
>
> (XEN) Checking for initrd in /chosen
> (XEN) Initrd 0000000084000040-0000000086152ac6
> (XEN) RAM: 0000000048000000 - 00000000bfffffff
> (XEN) RAM: 0000000480000000 - 00000004ffffffff
> (XEN) RAM: 0000000600000000 - 00000006ffffffff
> (XEN)
> (XEN) MODULE[0]: 0000000048080000 - 00000000481ec000 Xen
> (XEN) MODULE[1]: 0000000048000000 - 000000004801e080 Device Tree
> (XEN) MODULE[2]: 0000000084000040 - 0000000086152ac6 Ramdisk
> (XEN) MODULE[3]: 0000000048300000 - 000000004a300000 Kernel
> (XEN) MODULE[4]: 0000000048070000 - 0000000048080000 XSM
> (XEN) RESVD[0]: 0000000060000000 - 000000007fffffff
> (XEN) RESVD[1]: 00000000b0000000 - 00000000bfffffff
> (XEN) RESVD[2]: 00000000a0000000 - 00000000afffffff
> ...
> (XEN) *** LOADING DOMAIN 0 ***
> (XEN) Loading d0 kernel from boot module @ 0000000048300000
> (XEN) Loading ramdisk from boot module @ 0000000084000040
> (XEN) Allocating 1:1 mappings totalling 256MB for dom0:
> (XEN) BANK[0] 0x00000050000000-0x00000060000000 (256MB)
> ...
>
>
> ---
> diff --git a/xen/common/device-tree/bootinfo.c
> b/xen/common/device-tree/bootinfo.c
> index f2e6a1145b7c..10e997eeca8d 100644
> --- a/xen/common/device-tree/bootinfo.c
> +++ b/xen/common/device-tree/bootinfo.c
> @@ -124,6 +124,30 @@ static bool __init meminfo_overlap_check(const struct
> membanks *mem,
> return false;
> }
>
> +static bool __init meminfo_is_exist(const struct membanks *mem,
> + paddr_t region_start,
> + paddr_t region_size)
> +{
> + paddr_t bank_start = INVALID_PADDR, bank_end = 0;
> + paddr_t region_end = region_start + region_size;
> + unsigned int i, bank_num = mem->nr_banks;
> +
> + for ( i = 0; i < bank_num; i++ )
> + {
> + bank_start = mem->bank[i].start;
> + bank_end = bank_start + mem->bank[i].size;
> +
> + if ( INVALID_PADDR == bank_start || region_end <= bank_start ||
> + region_start >= bank_end )
> + continue;
> +
> + if ( region_start == bank_start && region_end == bank_end)
> + return true;
> + }
> +
> + return false;
> +}
> +
> /*
> * TODO: '*_end' could be 0 if the module/region is at the end of the
> physical
> * address space. This is for now not handled as it requires more rework.
> @@ -154,6 +178,29 @@ static bool __init bootmodules_overlap_check(struct
> bootmodules *bootmodules,
> return false;
> }
>
> +static bool __init bootmodules_is_exist(struct bootmodules *bootmodules,
> + paddr_t region_start,
> + paddr_t region_size)
> +{
> + paddr_t mod_start = INVALID_PADDR, mod_end = 0;
> + paddr_t region_end = region_start + region_size;
> + unsigned int i, mod_num = bootmodules->nr_mods;
> +
> + for ( i = 0; i < mod_num; i++ )
> + {
> + mod_start = bootmodules->module[i].start;
> + mod_end = mod_start + bootmodules->module[i].size;
> +
> + if ( region_end <= mod_start || region_start >= mod_end )
> + continue;
> +
> + if (region_start == mod_start && region_end == mod_end)
> + return true;
> + }
> +
> + return false;
> +}
> +
> void __init fw_unreserved_regions(paddr_t s, paddr_t e,
> void (*cb)(paddr_t ps, paddr_t pe),
> unsigned int first)
> @@ -201,6 +248,37 @@ bool __init check_reserved_regions_overlap(paddr_t
> region_start,
> return false;
> }
>
> +bool __init check_reserved_regions_is_exist(paddr_t region_start,
> + paddr_t region_size)
> +{
> + const struct membanks *mem_banks[] = {
> + bootinfo_get_reserved_mem(),
> +#ifdef CONFIG_ACPI
> + bootinfo_get_acpi(),
> +#endif
> +#ifdef CONFIG_STATIC_SHM
> + bootinfo_get_shmem(),
> +#endif
> + };
> + unsigned int i;
> +
> + /*
> + * Check if input region is overlapping with reserved memory banks or
> + * ACPI EfiACPIReclaimMemory (when ACPI feature is enabled) or static
> + * shared memory banks (when static shared memory feature is enabled)
> + */
> + for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ )
> + if ( meminfo_is_exist(mem_banks[i], region_start, region_size) )
> + return true;
> +
> + /* Check if input region is overlapping with bootmodules */
> + if ( bootmodules_is_exist(&bootinfo.modules,
> + region_start, region_size) )
> + return true;
> +
> + return false;
> +}
> +
> struct bootmodule __init *add_boot_module(bootmodule_kind kind,
> paddr_t start, paddr_t size,
> bool domU)
> diff --git a/xen/include/xen/bootfdt.h b/xen/include/xen/bootfdt.h
> index 16fa05f38f38..b8db1335be6c 100644
> --- a/xen/include/xen/bootfdt.h
> +++ b/xen/include/xen/bootfdt.h
> @@ -159,6 +159,8 @@ extern struct bootinfo bootinfo;
>
> bool check_reserved_regions_overlap(paddr_t region_start,
> paddr_t region_size);
> +bool check_reserved_regions_is_exist(paddr_t region_start,
> + paddr_t region_size);
>
> struct bootmodule *add_boot_module(bootmodule_kind kind,
> paddr_t start, paddr_t size, bool domU);
>
>
>
>
I don't think there is a need for introduction of that many functions. For a
simple exact matching case
we can opencode the logic a bit. On top of Shawn patch, the minimal version
would look as follows:
diff --git a/xen/common/device-tree/bootfdt.c b/xen/common/device-tree/bootfdt.c
index d35b2629e5a1..759c790888f9 100644
--- a/xen/common/device-tree/bootfdt.c
+++ b/xen/common/device-tree/bootfdt.c
@@ -586,14 +586,14 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t
paddr)
add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false);
- nr_rsvd = fdt_num_mem_rsv(fdt);
- 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);
+ nr_rsvd = fdt_num_mem_rsv(fdt);
+ if ( nr_rsvd < 0 )
+ panic("Parsing FDT memory reserve map failed (%d)\n", nr_rsvd);
+
for ( i = 0; i < nr_rsvd; i++ )
{
const struct membanks *overlap = NULL;
@@ -605,19 +605,33 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t
paddr)
if ( check_reserved_regions_overlap(s, sz, &overlap) )
{
- if ( overlap == bootinfo_get_reserved_mem() )
+ unsigned int j;
+ bool match = false;
+
+ if ( overlap != reserved_mem )
+ panic("FDT reserve map overlapped with membanks/modules\n");
+
+ /*
+ * 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
+ * matching overlaps in the mem_reserved banks should be ignored.
+ */
+ for ( j = 0; j < overlap->nr_banks; j++ )
{
- /*
- * 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;
+ if ( (overlap->bank[j].start == s) &&
+ (overlap->bank[j].size == sz) )
+ {
+ match = true;
+ break;
+ }
}
- else
- panic("FDT reserve map overlapped with membanks/modules\n");
+
+ if ( match )
+ continue;
+
+ panic("FDT reserve map partially overlaps with
/reserved-memory\n");
}
if ( reserved_mem->nr_banks < reserved_mem->max_banks )
Let's wait for Shawn test and other DT maintainers opinion.
~Michal
