RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

Thanks for your review.

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> 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.Wang@xxxxxxx>
> > ---
> > 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-59317f22321f@xxxxxxx/

Kind regards,
Henry


> 
> Cheers,
> 
> --
> Julien Grall