Re: [PATCH v3 17/19] xen/arm64: mm: Add memory to the boot allocator first
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 =
> > > +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
Re: [PATCH v3 17/19] xen/arm64: mm: Add memory to the boot allocator first
Hi Stefano,
On 05/04/2022 22:50, Stefano Stabellini wrote:
+static void __init setup_mm(void)
+{
+const struct meminfo *banks =
+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,
--
Julien Grall
Re: [PATCH v3 17/19] xen/arm64: mm: Add memory to the boot allocator first
On Mon, 21 Feb 2022, Julien Grall wrote:
> From: Julien Grall
>
> 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
>
> ---
> 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 =
>
> -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 = >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, );
>
> 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 =
> +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 = >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
>
[PATCH v3 17/19] xen/arm64: mm: Add memory to the boot allocator first
From: Julien Grall
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
---
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 =
-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 = >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, );
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 =
+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 = >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
