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

[Julien Grall <julien@xxxxxxx>]

Hi Henry

On 06/09/2022 02:53, Henry Wang wrote:
> 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;
> ...
> ```
>
> ?
Yes.

> 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.
I don't think I agree on the less likely here. The regions are provided 
by in the Device-Tree. And there are more chance they are incorrect 
because the value will be specific to a software/device stack.
Related to this discussion, I can't find any alignment requirement in 
the device-tree binding. I think we at least want to require 64KB 
aligned (so the same Device-Tree works if we were going to support 64KB 
page granularity).
> > > +
> > > +            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.
Xenheap is currently allocated the highest possible so there is enough 
low memory available for domain memory. This is in order to allow 32-bit
DMA device to function.

I am less certain this makes sense when the heap is reserved. Because an
admin could decide to define the heap solely above/below 4GB.

That said, nothing in the document suggests that domain memory would not 
be allocated from the reserved heap. So I would suggest to write the 
following comment:
"Allocate the xenheap as high as possible to keep low-memory available 
(assuming the admin supplied region below 4GB) for other use (e.g. 
domain memory allocation)."
Also, I think the documentation wants to be updated to clarify whether 
the reserved heap could be used to allocate domain. If it could, then I 
think we need to explain that the region should contain enough memory 
below some 4GB to cater 32-bit DMA.
> > 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;
> ```
> ?
Yes.

> If so I will do that in v3.
Thanks.

> > > +     * 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.
Right, I wasn't sure where to comment because you don't touch the call 
to populate_boot_allocator().
> 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.
Yes I am requesting some change because I think the code on arm32 is 
incorrect (the boot allocator will not be populated with the reserved heap).
I think the code should be moved in populate_boot_allocator():

if ( bootinfo.reserved_heap )
{
    for ( ...; i < bootinfo.reserved_mem.nr_banks; i++ )
       [....]
       init_boot_pages_pages()
}

Note that to handle arm32, you will also need to exclude the xenheap area.

Cheers,

--
Julien Grall