| Message ID | 20220221102218.33785-18-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: mm: Remove open-coding mappings | expand |
On Mon, 21 Feb 2022, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > Currently, memory is added to the boot allocator after the xenheap > mappings are done. This will break if the first mapping is more than > 512GB of RAM. > > In addition to that, a follow-up patch will rework setup_xenheap_mappings() > to use smaller mappings (e.g. 2MB, 4KB). So it will be necessary to have > memory in the boot allocator earlier. > > Only free memory (e.g. not reserved or modules) can be added to the boot > allocator. It might be possible that some regions (including the first > one) will have no free memory. > > So we need to add all the free memory to the boot allocator first > and then add do the mappings. > > Signed-off-by: Julien Grall <jgrall@amazon.com> > > --- > Changes in v3: > - Patch added > --- > xen/arch/arm/setup.c | 63 +++++++++++++++++++++++++++++--------------- > 1 file changed, 42 insertions(+), 21 deletions(-) > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index d5d0792ed48a..777cf96639f5 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -767,30 +767,18 @@ static void __init setup_mm(void) > init_staticmem_pages(); > } > #else /* CONFIG_ARM_64 */ > -static void __init setup_mm(void) > +static void __init populate_boot_allocator(void) > { > - paddr_t ram_start = ~0; > - paddr_t ram_end = 0; > - paddr_t ram_size = 0; > - int bank; > - > - init_pdx(); > + unsigned int i; > + const struct meminfo *banks = &bootinfo.mem; > > - total_pages = 0; > - for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ ) > + for ( i = 0; i < banks->nr_banks; i++ ) > { > - paddr_t bank_start = bootinfo.mem.bank[bank].start; > - paddr_t bank_size = bootinfo.mem.bank[bank].size; > - paddr_t bank_end = bank_start + bank_size; > + const struct membank *bank = &banks->bank[i]; > + paddr_t bank_end = bank->start + bank->size; > paddr_t s, e; > > - ram_size = ram_size + bank_size; > - ram_start = min(ram_start,bank_start); > - ram_end = max(ram_end,bank_end); > - > - setup_xenheap_mappings(bank_start>>PAGE_SHIFT, bank_size>>PAGE_SHIFT); > - > - s = bank_start; > + s = bank->start; > while ( s < bank_end ) > { > paddr_t n = bank_end; > @@ -798,9 +786,7 @@ static void __init setup_mm(void) > e = next_module(s, &n); > > if ( e == ~(paddr_t)0 ) > - { > e = n = bank_end; > - } > > if ( e > bank_end ) > e = bank_end; > @@ -809,6 +795,41 @@ static void __init setup_mm(void) > s = n; > } > } > +} > + > +static void __init setup_mm(void) > +{ > + const struct meminfo *banks = &bootinfo.mem; > + paddr_t ram_start = ~0; > + paddr_t ram_end = 0; > + paddr_t ram_size = 0; > + unsigned int i; > + > + init_pdx(); > + > + /* > + * We need some memory to allocate the page-tables used for the xenheap > + * mappings. But some regions may contain memory already allocated > + * for other uses (e.g. modules, reserved-memory...). > + * > + * For simplify add all the free regions in the boot allocator. > + */ We currently have: BUG_ON(nr_bootmem_regions == (PAGE_SIZE / sizeof(struct bootmem_region))); Do you think we should check for the limit in populate_boot_allocator? Or there is no need because it is unrealistic to reach it? > + populate_boot_allocator(); > + > + total_pages = 0; > + > + for ( i = 0; i < banks->nr_banks; i++ ) > + { > + const struct membank *bank = &banks->bank[i]; > + paddr_t bank_end = bank->start + bank->size; > + > + ram_size = ram_size + bank->size; > + ram_start = min(ram_start, bank->start); > + ram_end = max(ram_end, bank_end); > + > + setup_xenheap_mappings(PFN_DOWN(bank->start), > + PFN_DOWN(bank->size)); > + } > > total_pages += ram_size >> PAGE_SHIFT; > > -- > 2.32.0 >
Hi Stefano, On 05/04/2022 22:50, Stefano Stabellini wrote: >> +static void __init setup_mm(void) >> +{ >> + const struct meminfo *banks = &bootinfo.mem; >> + paddr_t ram_start = ~0; >> + paddr_t ram_end = 0; >> + paddr_t ram_size = 0; >> + unsigned int i; >> + >> + init_pdx(); >> + >> + /* >> + * We need some memory to allocate the page-tables used for the xenheap >> + * mappings. But some regions may contain memory already allocated >> + * for other uses (e.g. modules, reserved-memory...). >> + * >> + * For simplify add all the free regions in the boot allocator. >> + */ > > We currently have: > > BUG_ON(nr_bootmem_regions == (PAGE_SIZE / sizeof(struct bootmem_region))); This has enough space for 256 distinct regions on arm64 (512 regions on arm32). > > Do you think we should check for the limit in populate_boot_allocator? This patch doesn't change the number of regions added to the boot allocator. So if we need to check the limit then I would rather deal separately (see more below). > Or there is no need because it is unrealistic to reach it? I can't say never because history told us on some UEFI systems, there will be a large number of regions exposed. I haven't heard anyone that would hit the BUG_ON(). The problem is what do we do if we hit the limit? We could ignore all the regions after. However, there are potentially a risk there would not be enough memory to cover the boot memory allocation (regions may be really small). So if we ever hit the limit, then I think we should update the boot allocator. Cheers,
On Tue, 5 Apr 2022, Julien Grall wrote: > On 05/04/2022 22:50, Stefano Stabellini wrote: > > > +static void __init setup_mm(void) > > > +{ > > > + const struct meminfo *banks = &bootinfo.mem; > > > + paddr_t ram_start = ~0; > > > + paddr_t ram_end = 0; > > > + paddr_t ram_size = 0; > > > + unsigned int i; > > > + > > > + init_pdx(); > > > + > > > + /* > > > + * We need some memory to allocate the page-tables used for the > > > xenheap > > > + * mappings. But some regions may contain memory already allocated > > > + * for other uses (e.g. modules, reserved-memory...). > > > + * > > > + * For simplify add all the free regions in the boot allocator. > > > + */ > > > > We currently have: > > > > BUG_ON(nr_bootmem_regions == (PAGE_SIZE / sizeof(struct bootmem_region))); > > This has enough space for 256 distinct regions on arm64 (512 regions on > arm32). > > > > > Do you think we should check for the limit in populate_boot_allocator? > > This patch doesn't change the number of regions added to the boot allocator. > So if we need to check the limit then I would rather deal separately (see more > below). > > > Or there is no need because it is unrealistic to reach it? > I can't say never because history told us on some UEFI systems, there will be > a large number of regions exposed. I haven't heard anyone that would hit the > BUG_ON(). > > The problem is what do we do if we hit the limit? We could ignore all the > regions after. However, there are potentially a risk there would not be enough > memory to cover the boot memory allocation (regions may be really small). > > So if we ever hit the limit, then I think we should update the boot allocator. OK, thanks for the explanation. Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index d5d0792ed48a..777cf96639f5 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -767,30 +767,18 @@ static void __init setup_mm(void) init_staticmem_pages(); } #else /* CONFIG_ARM_64 */ -static void __init setup_mm(void) +static void __init populate_boot_allocator(void) { - paddr_t ram_start = ~0; - paddr_t ram_end = 0; - paddr_t ram_size = 0; - int bank; - - init_pdx(); + unsigned int i; + const struct meminfo *banks = &bootinfo.mem; - total_pages = 0; - for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ ) + for ( i = 0; i < banks->nr_banks; i++ ) { - paddr_t bank_start = bootinfo.mem.bank[bank].start; - paddr_t bank_size = bootinfo.mem.bank[bank].size; - paddr_t bank_end = bank_start + bank_size; + const struct membank *bank = &banks->bank[i]; + paddr_t bank_end = bank->start + bank->size; paddr_t s, e; - ram_size = ram_size + bank_size; - ram_start = min(ram_start,bank_start); - ram_end = max(ram_end,bank_end); - - setup_xenheap_mappings(bank_start>>PAGE_SHIFT, bank_size>>PAGE_SHIFT); - - s = bank_start; + s = bank->start; while ( s < bank_end ) { paddr_t n = bank_end; @@ -798,9 +786,7 @@ static void __init setup_mm(void) e = next_module(s, &n); if ( e == ~(paddr_t)0 ) - { e = n = bank_end; - } if ( e > bank_end ) e = bank_end; @@ -809,6 +795,41 @@ static void __init setup_mm(void) s = n; } } +} + +static void __init setup_mm(void) +{ + const struct meminfo *banks = &bootinfo.mem; + paddr_t ram_start = ~0; + paddr_t ram_end = 0; + paddr_t ram_size = 0; + unsigned int i; + + init_pdx(); + + /* + * We need some memory to allocate the page-tables used for the xenheap + * mappings. But some regions may contain memory already allocated + * for other uses (e.g. modules, reserved-memory...). + * + * For simplify add all the free regions in the boot allocator. + */ + populate_boot_allocator(); + + total_pages = 0; + + for ( i = 0; i < banks->nr_banks; i++ ) + { + const struct membank *bank = &banks->bank[i]; + paddr_t bank_end = bank->start + bank->size; + + ram_size = ram_size + bank->size; + ram_start = min(ram_start, bank->start); + ram_end = max(ram_end, bank_end); + + setup_xenheap_mappings(PFN_DOWN(bank->start), + PFN_DOWN(bank->size)); + } total_pages += ram_size >> PAGE_SHIFT;