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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Stefano,

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > 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.

Thank you very much for your detailed explanation below!
[...]

> 
> 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:

I am not sure about that, since we already have
heap_pages = reserved_heap ? reserved_heap_pages : ram_pages;

the heap_pages is supposed to contain domheap_pages + xenheap_pages
based on the reserved heap definition discussed in the RFC.

from the code in...

> 
>     if ( opt_xenheap_megabytes )
>         xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
>     else if ( reserved_heap )
>         xenheap_pages = heap_pages;

...here, setting xenheap_pages to heap_pages makes me a little bit
confused.

>     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));
>     }

If we keep this logic as this patch does, we can have the requirements...

> 
> 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

...here, with the "1/32 the total RAM" now being "1/32 of the total reserved
heap region", since heap_pages is now reserved_heap_pages.

> 
> 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?

I left some of my thoughts above to explain my understanding, but I might
be wrong, thank you for your patience!

Kind regards,
Henry