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@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);
+ 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);
--
2.30.2
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@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
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 <sanastasio@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);
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 <sanastasio@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
On 05.11.24 12:42, Michal Orzel wrote:
>
>
> 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 <sanastasio@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");
No it will not work this way - as overlap I observe is with Initrd which is *Module* (Not reserved memory).
That's why i've had to add new functions.
> +
> + /*
> + * 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
>
On 04.11.24 14: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 <sanastasio@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);
>
>
Sorry missed part of the diff:
diff --git a/xen/common/device-tree/bootfdt.c b/xen/common/device-tree/bootfdt.c
index 927f59c64b0d..18816b6f08f1 100644
--- a/xen/common/device-tree/bootfdt.c
+++ b/xen/common/device-tree/bootfdt.c
@@ -586,6 +586,10 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false);
+ 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);
@@ -598,6 +602,9 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, &s, &sz) < 0 )
continue;
+ if (check_reserved_regions_is_exist(s, sz))
+ continue;
+
if ( reserved_mem->nr_banks < reserved_mem->max_banks )
{
bank = &reserved_mem->bank[reserved_mem->nr_banks];
@@ -610,10 +617,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
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@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(-)
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);
© 2016 - 2024 Red Hat, Inc.