Hi Stefano,
> -----Original Message-----
> From: Stefano Stabellini <sstabell...@kernel.org>
> > > 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
