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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

Thanks for your comments, I added my reply and some of the questions
that I am not 100% sure inline below.

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Hi Henry,
> > +
> > +/*
> > + * Find the contiguous xenheap region that fits in the reserved heap region
> with
> 
> There might be multiple. So "Find a contiguous...". I would also drop
> "xenheap".

I will follow the wording that you suggested here and ...

> 
> > + * required size and alignment, and return the end address of xenheap.
> 
> I would write "and return the end address of the region if found
> otherwise 0".

...here.

> 
> > + */
> > +static paddr_t __init fit_xenheap_in_reserved_heap(uint32_t size, paddr_t
> align)
> > +{
> > +    int i;
> 
> Please use unsigned int.

Ah sure.

> 
> > +    paddr_t end = 0, aligned_start, aligned_end;
> > +    paddr_t bank_start, bank_size, bank_end;
> > +
> > +    for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
> > +    {
> > +        if ( bootinfo.reserved_mem.bank[i].type == MEMBANK_RSVD_HEAP )
> NIT: You could avoid the extra indentation by reverting the condition.

Sorry I am not 100% sure the extra indentation you were referring to.
Are you suggesting that we need to do a 
```
if ( bootinfo.reserved_mem.bank[i].type != MEMBANK_RSVD_HEAP )
    continue;

bank_start = bootinfo.reserved_mem.bank[i].start;
...
```

?

If so I will change in v3.

> 
> > +        {
> > +            bank_start = bootinfo.reserved_mem.bank[i].start;
> > +            bank_size = bootinfo.reserved_mem.bank[i].size;
> > +            bank_end = bank_start + bank_size;
> > +
> > +            if ( bank_size < size )
> > +                continue;
> > +
> > +            aligned_end = bank_end & ~(align - 1);
> > +            aligned_start = (aligned_end - size) & ~(align - 1);
> 
> I find the logic a bit confusing. AFAIU, aligned_start could be below
> the start of the RAM which is not what I would usually expect.

Yeah I understand your concern. Here I want to make sure even if
the given size is not aligned (although less likely happen in real life
given the size calculation logic in setup_mm) the code still work. So
the aligned_start = (aligned_end - size) & ~(align - 1) and below
if (aligned_start > bank_start) check.

> 
> The code works. So no change requested.

Thanks!

> 
> 
> > +
> > +            if ( aligned_start > bank_start )
> > +                /*
> > +                 * Arm32 allocates xenheap from higher address to lower, 
> > so if
> 
> This code is also called on arm32. So what are you referring to? Is it
> consider_modules()?

Yes, I think the current arm32 behavior in consider_modules() is what
I am referring to. In fact, I just want to add some comments that explain why
we need the end = max(end, aligned_end) since technically if there are
multiple reserved heap banks and all of them can fit the xenheap region,
we can use either of them. But following the current behavior we can only use
the highest bank to keep the consistency.

> 
> > +                 * there are multiple memory banks that satisfy the 
> > requirement,
> > +                 * use the highest bank.
> > +                 */
> > +                end = max(end, aligned_end);
> > +        }
> > +    }
> > +
> > +    return end;
> > +}
> >   #endif
> >
> >   /*
> > @@ -713,8 +750,9 @@ 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_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);
> > @@ -734,9 +772,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);
> > @@ -745,19 +783,42 @@ static void __init setup_mm(void)
> >
> >       total_pages = ram_pages = ram_size >> PAGE_SHIFT;
> >
> > +    if ( bootinfo.reserved_heap )
> > +    {
> > +        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
> > +        {
> > +            if ( bootinfo.reserved_mem.bank[i].type ==
> MEMBANK_RSVD_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_end = max(reserved_heap_end, bank_end);
> > +            }
> > +        }
> > +
> > +        reserved_heap_pages = reserved_heap_size >> PAGE_SHIFT;
> > +        if ( reserved_heap_pages < 32<<(20-PAGE_SHIFT) )
> > +            panic("Too small reserved heap region, should be at least 
> > 32M\n");
> 
> This is a bit misleading. 32MB is not sufficient, it also has to be
> contiguous. So I would drop this panic() completely.

Sure.

> 
> > +    }
> > +
> >       /*
> >        * If the user has not requested otherwise via the command line
> >        * then locate the xenheap using these constraints:
> >        *
> >        *  - must be 32 MiB aligned
> >        *  - must not include Xen itself or the boot modules
> > -     *  - must be at most 1GB or 1/32 the total RAM in the system if less
> > +     *  - must be at most 1GB or 1/32 the total RAM in the system
> > +     *    (there is no reserved heap) or 1/32 the total reserved
> 
> Did you forgot to add "if" before "there"?

I will use the wording that you suggested in ...

> 
> > +     *    heap region (there is reserved heap) if less
> 
> The new wording suggests that the 1GB limit only applies when the admin
> doesn't specify the reserved heap. However, we don't support larger heap
> than 1GB. So the limit should also apply for the reserved heap. So how
> about:
> 
> - must be at most 1GB or 1/32 the total RAM in the system (or reserved
> heap if enabled)

...here.

> 
> >        *  - must be at least 32M
> >        *
> >        * We try to allocate the largest xenheap possible within these
> >        * constraints.
> >        */
> > -    heap_pages = ram_pages;
> > +    heap_pages = bootinfo.reserved_heap ? reserved_heap_pages :
> ram_pages;
> 
> You can avoid the ternary operation here by setting heap_pages in the
> 'if' above and add a else for the 'ram_pages' part.

Sorry I might understand your comment in the wrong way, but do you
suggest we need to:
```
if ( bootinfo.reserved_heap )
{
...
    heap_pages = reserved_heap_pages;
}
else
    heap_pages = total_pages;
```
?

If so I will do that in v3.

> 
> In fact, 'ram_pages' could be completely dropped in favor of 'total_pages'.

Good point, I will do that in v3.

> 
> > +
> >       if ( opt_xenheap_megabytes )
> >           xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
> >       else
> > @@ -767,9 +828,15 @@ static void __init setup_mm(void)
> >           xenheap_pages = min(xenheap_pages, 1UL<<(30-PAGE_SHIFT));
> >       }
> >
> > +    /*
> > +     * On Arm32, xenheap must be contiguous, look for one of the region
> > +     * that matches the above-mentioned xenheap constraints.
> > +     */
> 
> IMHO this is already implied by the large comment above. But if you want
> to be more obvious, then I think this should belong to the comment above.

I think I will add "the xenheap should be contiguous" as I missed this one
Before you mentioned this in the code review. I think adding this will avoid
people who change this part in the future making mistakes like I did :))

> 
> >       do
> >       {
> > -        e = consider_modules(ram_start, ram_end,
> > +        e = bootinfo.reserved_heap ?
> > +            fit_xenheap_in_reserved_heap(pfn_to_paddr(xenheap_pages),
> 32<<20) :
> 
> Please use MB(32) in new code.

Oh sure. Thanks for pointing this out.

> 
> > +            consider_modules(ram_start, ram_end,
> >                                pfn_to_paddr(xenheap_pages),
> >                                32<<20, 0);
> >           if ( e )
> > @@ -779,7 +846,7 @@ static void __init setup_mm(void)
> >       } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-
> PAGE_SHIFT) );
> >
> >       if ( ! e )
> > -        panic("Not not enough space for xenheap\n");
> > +        panic("Not enough space for xenheap\n");
> >
> >       domheap_pages = heap_pages - xenheap_pages;
> >
> > @@ -824,9 +891,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 = INVALID_PADDR, bank_start = INVALID_PADDR;
> > +    paddr_t ram_end = 0, bank_end = 0;
> > +    paddr_t ram_size = 0, bank_size = 0;
> >       unsigned int i;
> >
> >       init_pdx();
> > @@ -835,17 +902,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 there are non-empty reserved heap regions, (only) add these
> regions
> 
> I am not sure what you mean by "non-empty" here. How about something
> like:
> 
> "If a reserved heap was provided by the admin, populate the boot
> allocator with the corresponding regions only".

Sure.

> 
> > +     * in the boot allocator.
> > +     */
> > +    if ( bootinfo.reserved_heap )
> > +    {
> > +        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
> > +        {
> > +            if ( bootinfo.reserved_mem.bank[i].type ==
> MEMBANK_RSVD_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();
> 
> For arm32, shouldn't we also only add the reserved heap (minus the
> xenheap) to the boot allocator? At which point, I would move the change
> in populate_boot_allocator().

Sorry I am not sure what this comment about...as here the code is for arm64.
For the question, yes.
For the latter one, do you request some changes? If so, could you please kindly
elaborate a little bit more? Thanks.

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall