Re: [RFC PATCH 1/2] xen/arm: Add DT reserve map regions to bootinfo.reserved_mem
Hi Luca, On 15/05/2024 11:05, Luca Fancellu wrote: On 14 May 2024, at 22:06, Julien Grall wrote: Hi, On 14/05/2024 08:53, Luca Fancellu wrote: Hi Julien, Thanks for having a look on the patch, On 13 May 2024, at 22:54, Julien Grall wrote: Hi Luca, On 25/04/2024 14:11, Luca Fancellu wrote: Currently the code is listing device tree reserve map regions as reserved memory for Xen, but they are not added into bootinfo.reserved_mem and they are fetched in multiple places using the same code sequence, causing duplication. Fix this by adding them to the bootinfo.reserved_mem at early stage. Do we have enough space in bootinfo.reserved_mem for them? So we have 255 banks, in my experience I would say I’ve never saw too many reserved regions in the DT, maybe a couple, but I’ve always had to deal with embedded platforms. I’ve tested this one with ADLINK AVA board, n1sdp, Juno, raspberry pi, qemu, fvp. In your experience, have you seen any numbers that could be concerning? I know in the past we had to bump the memory banks a few times. But as you tested on a few platforms, I think we should be ok. It would be best if this patch goes sooner than later to allow wider testing before we release 4.19. Acked-by: Julien Grall Yes it would make sense, this patch makes sense on its own, would you/anyone commit it separately while I work on the second patch? Thank you for the confirmation. This is now committed. Cheers, -- Julien Grall
Re: [RFC PATCH 1/2] xen/arm: Add DT reserve map regions to bootinfo.reserved_mem
> On 14 May 2024, at 22:06, Julien Grall wrote: > > Hi, > > On 14/05/2024 08:53, Luca Fancellu wrote: >> Hi Julien, >> Thanks for having a look on the patch, >>> On 13 May 2024, at 22:54, Julien Grall wrote: >>> >>> Hi Luca, >>> >>> On 25/04/2024 14:11, Luca Fancellu wrote: Currently the code is listing device tree reserve map regions as reserved memory for Xen, but they are not added into bootinfo.reserved_mem and they are fetched in multiple places using the same code sequence, causing duplication. Fix this by adding them to the bootinfo.reserved_mem at early stage. >>> >>> Do we have enough space in bootinfo.reserved_mem for them? >> So we have 255 banks, in my experience I would say I’ve never saw too many >> reserved regions >> in the DT, maybe a couple, but I’ve always had to deal with embedded >> platforms. >> I’ve tested this one with ADLINK AVA board, n1sdp, Juno, raspberry pi, qemu, >> fvp. >> In your experience, have you seen any numbers that could be concerning? > I know in the past we had to bump the memory banks a few times. But as you > tested on a few platforms, I think we should be ok. > > It would be best if this patch goes sooner than later to allow wider testing > before we release 4.19. > > Acked-by: Julien Grall Yes it would make sense, this patch makes sense on its own, would you/anyone commit it separately while I work on the second patch?
Re: [RFC PATCH 1/2] xen/arm: Add DT reserve map regions to bootinfo.reserved_mem
Hi, On 14/05/2024 08:53, Luca Fancellu wrote: Hi Julien, Thanks for having a look on the patch, On 13 May 2024, at 22:54, Julien Grall wrote: Hi Luca, On 25/04/2024 14:11, Luca Fancellu wrote: Currently the code is listing device tree reserve map regions as reserved memory for Xen, but they are not added into bootinfo.reserved_mem and they are fetched in multiple places using the same code sequence, causing duplication. Fix this by adding them to the bootinfo.reserved_mem at early stage. Do we have enough space in bootinfo.reserved_mem for them? So we have 255 banks, in my experience I would say I’ve never saw too many reserved regions in the DT, maybe a couple, but I’ve always had to deal with embedded platforms. I’ve tested this one with ADLINK AVA board, n1sdp, Juno, raspberry pi, qemu, fvp. In your experience, have you seen any numbers that could be concerning? I know in the past we had to bump the memory banks a few times. But as you tested on a few platforms, I think we should be ok. It would be best if this patch goes sooner than later to allow wider testing before we release 4.19. Acked-by: Julien Grall Cheers, -- Julien Grall
Re: [RFC PATCH 1/2] xen/arm: Add DT reserve map regions to bootinfo.reserved_mem
Hi Julien, Thanks for having a look on the patch, > On 13 May 2024, at 22:54, Julien Grall wrote: > > Hi Luca, > > On 25/04/2024 14:11, Luca Fancellu wrote: >> Currently the code is listing device tree reserve map regions >> as reserved memory for Xen, but they are not added into >> bootinfo.reserved_mem and they are fetched in multiple places >> using the same code sequence, causing duplication. Fix this >> by adding them to the bootinfo.reserved_mem at early stage. > > Do we have enough space in bootinfo.reserved_mem for them? So we have 255 banks, in my experience I would say I’ve never saw too many reserved regions in the DT, maybe a couple, but I’ve always had to deal with embedded platforms. I’ve tested this one with ADLINK AVA board, n1sdp, Juno, raspberry pi, qemu, fvp. In your experience, have you seen any numbers that could be concerning? Cheers, Luca
Re: [RFC PATCH 1/2] xen/arm: Add DT reserve map regions to bootinfo.reserved_mem
Hi Luca, On 25/04/2024 14:11, Luca Fancellu wrote: Currently the code is listing device tree reserve map regions as reserved memory for Xen, but they are not added into bootinfo.reserved_mem and they are fetched in multiple places using the same code sequence, causing duplication. Fix this by adding them to the bootinfo.reserved_mem at early stage. Do we have enough space in bootinfo.reserved_mem for them? The rest of the changes LGTM. Cheers, -- Julien Grall
[RFC PATCH 1/2] xen/arm: Add DT reserve map regions to bootinfo.reserved_mem
Currently the code is listing device tree reserve map regions
as reserved memory for Xen, but they are not added into
bootinfo.reserved_mem and they are fetched in multiple places
using the same code sequence, causing duplication. Fix this
by adding them to the bootinfo.reserved_mem at early stage.
Signed-off-by: Luca Fancellu
---
xen/arch/arm/arm32/mmu/mm.c | 29 +
xen/arch/arm/bootfdt.c | 51 ++
xen/arch/arm/domain_build.c | 3 +-
xen/arch/arm/include/asm/setup.h | 5 +++
xen/arch/arm/setup.c | 53 +---
5 files changed, 53 insertions(+), 88 deletions(-)
diff --git a/xen/arch/arm/arm32/mmu/mm.c b/xen/arch/arm/arm32/mmu/mm.c
index 23150122f7c4..be480c31ea05 100644
--- a/xen/arch/arm/arm32/mmu/mm.c
+++ b/xen/arch/arm/arm32/mmu/mm.c
@@ -73,33 +73,6 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
}
}
-/* Now check any fdt reserved areas. */
-
-nr = fdt_num_mem_rsv(device_tree_flattened);
-
-for ( ; i < mi->nr_mods + nr; i++ )
-{
-paddr_t mod_s, mod_e;
-
-if ( fdt_get_mem_rsv_paddr(device_tree_flattened,
- i - mi->nr_mods,
- _s, _e ) < 0 )
-/* If we can't read it, pretend it doesn't exist... */
-continue;
-
-/* fdt_get_mem_rsv_paddr returns length */
-mod_e += mod_s;
-
-if ( s < mod_e && mod_s < e )
-{
-mod_e = consider_modules(mod_e, e, size, align, i+1);
-if ( mod_e )
-return mod_e;
-
-return consider_modules(s, mod_s, size, align, i+1);
-}
-}
-
/*
* i is the current bootmodule we are evaluating, across all
* possible kinds of bootmodules.
@@ -108,7 +81,7 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
* need to index the reserved_mem bank starting from 0, and only counting
* the reserved-memory modules. Hence, we need to use i - nr.
*/
-nr += mi->nr_mods;
+nr = mi->nr_mods;
for ( ; i - nr < reserved_mem->nr_banks; i++ )
{
paddr_t r_s = reserved_mem->bank[i - nr].start;
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 4d708442a19e..6e060111d95b 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -475,8 +475,7 @@ static void __init early_print_info(void)
const struct membanks *mem_resv = bootinfo_get_reserved_mem();
struct bootmodules *mods =
struct bootcmdlines *cmds =
-unsigned int i, j;
-int nr_rsvd;
+unsigned int i;
for ( i = 0; i < mi->nr_banks; i++ )
printk("RAM: %"PRIpaddr" - %"PRIpaddr"\n",
@@ -490,26 +489,11 @@ static void __init early_print_info(void)
mods->module[i].start + mods->module[i].size,
boot_module_kind_as_string(mods->module[i].kind));
-nr_rsvd = fdt_num_mem_rsv(device_tree_flattened);
-if ( nr_rsvd < 0 )
-panic("Parsing FDT memory reserve map failed (%d)\n", nr_rsvd);
-
-for ( i = 0; i < nr_rsvd; i++ )
-{
-paddr_t s, e;
-
-if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, , ) < 0 )
-continue;
-
-/* fdt_get_mem_rsv_paddr returns length */
-e += s;
-printk(" RESVD[%u]: %"PRIpaddr" - %"PRIpaddr"\n", i, s, e);
-}
-for ( j = 0; j < mem_resv->nr_banks; j++, i++ )
+for ( i = 0; i < mem_resv->nr_banks; i++ )
{
printk(" RESVD[%u]: %"PRIpaddr" - %"PRIpaddr"\n", i,
- mem_resv->bank[j].start,
- mem_resv->bank[j].start + mem_resv->bank[j].size - 1);
+ mem_resv->bank[i].start,
+ mem_resv->bank[i].start + mem_resv->bank[i].size - 1);
}
early_print_info_shmem();
printk("\n");
@@ -550,7 +534,10 @@ static void __init swap_memory_node(void *_a, void *_b,
size_t size)
*/
size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
{
+struct membanks *reserved_mem = bootinfo_get_reserved_mem();
struct membanks *mem = bootinfo_get_mem();
+unsigned int i;
+int nr_rsvd;
int ret;
ret = fdt_check_header(fdt);
@@ -559,6 +546,30 @@ 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);
+
+for ( i = 0; i < nr_rsvd; i++ )
+{
+struct membank *bank;
+paddr_t s, sz;
+
+if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, , ) < 0 )
+continue;
+
+if ( reserved_mem->nr_banks < reserved_mem->max_banks )
+{
+bank = _mem->bank[reserved_mem->nr_banks];
+bank->start = s;
+bank->size = sz;
+bank->type = MEMBANK_FDT_RESVMEM;
+reserved_mem->nr_banks++;
+
