Hi Julien,
Thanks for your review.
> -----Original Message-----
> From: Julien Grall <jul...@xen.org>
> Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and
> heap allocator
>
> Hi Henry,
>
> On 24/08/2022 08:31, Henry Wang wrote:
> > This commit firstly adds a global variable `reserved_heap`.
> > This newly introduced global variable is set at the device tree
> > parsing time if the reserved heap ranges are defined in the device
> > tree chosen node.
> >
> > For Arm32, In `setup_mm`, if the reserved heap is enabled, we use
> > the reserved heap region for both domheap and xenheap allocation.
> >
> > For Arm64, In `setup_mm`, if the reserved heap is enabled and used,
> > we make sure that only these reserved heap pages are added to the
> > boot allocator. These reserved heap pages in the boot allocator are
> > added to the heap allocator at `end_boot_allocator()`.
> >
> > If the reserved heap is disabled, we stick to current page allocation
> > strategy at boot time.
> >
> > Also, take the chance to correct a "double not" print in Arm32
> > `setup_mm()`.
> >
> > Signed-off-by: Henry Wang <henry.w...@arm.com>
> > ---
> > With reserved heap enabled, for Arm64, naming of global variables such
> > as `xenheap_mfn_start` and `xenheap_mfn_end` seems to be ambiguous,
> > wondering if we should rename these variables.
> > ---
> > Changes from RFC to v1:
> > - Rebase on top of latest `setup_mm()` changes.
> > - Added Arm32 logic in `setup_mm()`.
> > ---
> > xen/arch/arm/bootfdt.c | 2 +
> > xen/arch/arm/include/asm/setup.h | 2 +
> > xen/arch/arm/setup.c | 79 +++++++++++++++++++++++++-------
> > 3 files changed, 67 insertions(+), 16 deletions(-)
> >
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index 33704ca487..ab73b6e212 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -325,6 +325,8 @@ static int __init process_chosen_node(const void
> *fdt, int node,
> > true);
> > if ( rc )
> > return rc;
> > +
> > + reserved_heap = true;
> > }
> >
> > printk("Checking for initrd in /chosen\n");
> > diff --git a/xen/arch/arm/include/asm/setup.h
> b/xen/arch/arm/include/asm/setup.h
> > index e80f3d6201..00536a6d55 100644
> > --- a/xen/arch/arm/include/asm/setup.h
> > +++ b/xen/arch/arm/include/asm/setup.h
> > @@ -92,6 +92,8 @@ extern struct bootinfo bootinfo;
> >
> > extern domid_t max_init_domid;
> >
> > +extern bool reserved_heap;
> > +
> > void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
> >
> > size_t estimate_efi_size(unsigned int mem_nr_banks);
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index 500307edc0..fe76cf6325 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -73,6 +73,8 @@ integer_param("xenheap_megabytes",
> opt_xenheap_megabytes);
> >
> > domid_t __read_mostly max_init_domid;
> >
> > +bool __read_mostly reserved_heap;
> > +
> > static __used void init_done(void)
> > {
> > /* Must be done past setting system_state. */
> > @@ -699,8 +701,10 @@ static void __init populate_boot_allocator(void)
> > #ifdef CONFIG_ARM_32
> > static void __init setup_mm(void)
> > {
> > - paddr_t ram_start, ram_end, ram_size, e;
> > - unsigned long ram_pages;
> > + paddr_t ram_start, ram_end, ram_size, e, bank_start, bank_end,
> bank_size;
> > + paddr_t reserved_heap_start = ~0, reserved_heap_end = 0,
> > + reserved_heap_size = 0;
> > + unsigned long ram_pages, reserved_heap_pages = 0;
> > unsigned long heap_pages, xenheap_pages, domheap_pages;
> > unsigned int i;
> > const uint32_t ctr = READ_CP32(CTR);
> > @@ -720,9 +724,9 @@ static void __init setup_mm(void)
> >
> > for ( i = 1; i < bootinfo.mem.nr_banks; i++ )
> > {
> > - paddr_t bank_start = bootinfo.mem.bank[i].start;
> > - paddr_t bank_size = bootinfo.mem.bank[i].size;
> > - paddr_t bank_end = bank_start + bank_size;
> > + bank_start = bootinfo.mem.bank[i].start;
> > + bank_size = bootinfo.mem.bank[i].size;
> > + bank_end = bank_start + bank_size;
> >
> > ram_size = ram_size + bank_size;
> > ram_start = min(ram_start,bank_start);
> > @@ -731,6 +735,25 @@ static void __init setup_mm(void)
> >
> > total_pages = ram_pages = ram_size >> PAGE_SHIFT;
> >
> > + if ( reserved_heap )
> > + {
> > + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
> > + {
> > + if ( bootinfo.reserved_mem.bank[i].xen_heap )
> > + {
> > + bank_start = bootinfo.reserved_mem.bank[i].start;
> > + bank_size = bootinfo.reserved_mem.bank[i].size;
> > + bank_end = bank_start + bank_size;
> > +
> > + reserved_heap_size += bank_size;
> > + reserved_heap_start = min(reserved_heap_start, bank_start);
> > + reserved_heap_end = max(reserved_heap_end, bank_end);
> > + }
> > + }
> > +
> > + reserved_heap_pages = reserved_heap_size >> PAGE_SHIFT;
> > + }
> > +
> > /*
> > * If the user has not requested otherwise via the command line
> > * then locate the xenheap using these constraints:
> > @@ -743,7 +766,8 @@ static void __init setup_mm(void)
> > * We try to allocate the largest xenheap possible within these
> > * constraints.
> > */
> > - heap_pages = ram_pages;
> > + heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages;
> > +
> > if ( opt_xenheap_megabytes )
> > xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
> > else
> > @@ -755,17 +779,21 @@ static void __init setup_mm(void)
> >
> > do
> > {
> > - e = consider_modules(ram_start, ram_end,
> > + e = !reserved_heap ?
> > + consider_modules(ram_start, ram_end,
> > pfn_to_paddr(xenheap_pages),
> > - 32<<20, 0);
> > + 32<<20, 0) :
> > + reserved_heap_end;
>
> Not entirely related to this series. Now the assumption is the admin
> will make sure that none of the reserved regions will overlap.
>
> Do we have any tool to help the admin to verify it? If yes, can we have
> a pointer in the documentation? If not, should this be done in Xen?
In the RFC we had the same discussion of this issue [1] and I think a
follow-up series might needed to do the overlap check if we want to
do that in Xen. For the existing tool, I am thinking of ImageBuilder, but
I am curious about Stefano's opinion.
>
> Also, what happen with UEFI? Is it easy to guarantee the region will not
> be used?
For now I think it is not easy to guarantee that, do you have some ideas
in mind? I think I can follow this in above follow-up series to improve things.
>
> > +
> > if ( e )
> > break;
> >
> > xenheap_pages >>= 1;
> > } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-
> PAGE_SHIFT) );
> >
> > - if ( ! e )
> > - panic("Not not enough space for xenheap\n");
> > + if ( ! e ||
> > + ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )
> > + panic("Not enough space for xenheap\n");
>
> So on arm32, the xenheap *must* be contiguous. AFAICT,
> reserved_heap_pages is the total number of pages in the heap. They may
> not be contiguous. So I think this wants to be reworked so we look for
> one of the region that match the definition written above the loop.
Thanks for raising this concern, I will do this in V2.
>
> >
> > domheap_pages = heap_pages - xenheap_pages;
> >
> > @@ -810,9 +838,9 @@ static void __init setup_mm(void)
> > 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;
> > + paddr_t ram_start = ~0, bank_start = ~0;
> > + paddr_t ram_end = 0, bank_end = 0;
> > + paddr_t ram_size = 0, bank_size = 0;
> > unsigned int i;
> >
> > init_pdx();
> > @@ -821,17 +849,36 @@ static void __init setup_mm(void)
> > * 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...).
> > - *
> > + * If reserved heap regions are properly defined, (only) add these
> regions
> > + * in the boot allocator. > + */
> > + if ( reserved_heap )
> > + {
> > + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
> > + {
> > + if ( bootinfo.reserved_mem.bank[i].xen_heap )
> > + {
> > + bank_start = bootinfo.reserved_mem.bank[i].start;
> > + bank_size = bootinfo.reserved_mem.bank[i].size;
> > + bank_end = bank_start + bank_size;
> > +
> > + init_boot_pages(bank_start, bank_end);
> > + }
> > + }
> > + }
> > + /*
> > + * No reserved heap regions:
> > * For simplicity, add all the free regions in the boot allocator.
> > */
> > - populate_boot_allocator();
> > + else
> > + populate_boot_allocator();
> >
> > total_pages = 0;
> >
> > for ( i = 0; i < banks->nr_banks; i++ )
> > {
>
> This code is now becoming quite confusing to understanding. This loop is
> meant to map the xenheap. If I follow your documentation, it would mean
> that only the reserved region should be mapped.
Yes I think this is the same question that I raised in the scissors line of the
commit message of this patch. What I intend to do is still mapping the whole
RAM because of the xenheap_* variables that you mentioned in...
>
> More confusingly, xenheap_* variables will cover the full RAM.
...here. But only adding the reserved region to the boot allocator so the
reserved region can become the heap later on. I am wondering if we
have a more clear way to do that, any suggestions?
> Effectively, this is now more obvious that this is use for
> direct-mapping. So I think it would be better to rename the variable to
> directmap_* or similar.
>
> Ideally this should be in a separate patch.
[1]
https://lore.kernel.org/xen-devel/48a0712c-eff8-dfc1-2136-59317f223...@xen.org/
Kind regards,
Henry
>
> Cheers,
>
> --
> Julien Grall
