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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 30 Aug 2022, Henry Wang wrote:
> > >      /*
> > >       * 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;
> > > +
> > >          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");
> > 
> > 
> > I would skip the do/while loop completely if reserved_heap. We don't
> > need it anyway
> 
> I agree with this.
> 
> > and we can automatically calculate xenheap_pages in a single line.
> 
> Here I am a little bit confused. Sorry to ask but could you please explain
> a little bit more about why we can calculate the xenheap_pages in a single
> line? Below is the code snippet in my mind, is this correct?
> 
> if (reserved_heap)

coding style

>     e = reserved_heap_end;
> else
> {
>     do
>     {
>         e = consider_modules(ram_start, ram_end,
>                              pfn_to_paddr(xenheap_pages),
>                              32<<20, 0);
>         if ( e )
>             break;
> 
>         xenheap_pages >>= 1;
>     } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-PAGE_SHIFT) );
> }

Yes, this is what I meant.

But also, here the loop is also for adjusting xenheap_pages, and
xenheap_pages is initialized as follows:


    if ( opt_xenheap_megabytes )
        xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
    else
    {
        xenheap_pages = (heap_pages/32 + 0x1fffUL) & ~0x1fffUL;
        xenheap_pages = max(xenheap_pages, 32UL<<(20-PAGE_SHIFT));
        xenheap_pages = min(xenheap_pages, 1UL<<(30-PAGE_SHIFT));
    }


In the reserved_heap case, it doesn't make sense to initialize
xenheap_pages like that, right? It should be something like:

    if ( opt_xenheap_megabytes )
        xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
    else if ( reserved_heap )
        xenheap_pages = heap_pages;
    else
    {
        xenheap_pages = (heap_pages/32 + 0x1fffUL) & ~0x1fffUL;
        xenheap_pages = max(xenheap_pages, 32UL<<(20-PAGE_SHIFT));
        xenheap_pages = min(xenheap_pages, 1UL<<(30-PAGE_SHIFT));
    }

But also it looks like that on arm32 we have specific requirements for
Xen heap:

     *  - 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 least 32M

I think we should check at least the 32MB alignment and 32MB minimum
size before using the xen_heap bank.


In short I think this patch should:

- add a check for 32MB alignment and size of the xen_heap memory bank
- if reserved_heap, set xenheap_pages = heap_pages
- if reserved_heap, skip the consider_modules do/while

Does it make sense?