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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 31 Aug 2022, Henry Wang wrote:
> > -----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 see. I didn't realize the full implications of the memory being used
for both xenheap and domheap on arm32. In that case, I would simply do
the following:


    heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages;

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

    if ( reserved_heap )
        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) 
);
    }

    if ( ! e ||
         ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )
        panic("Not enough space for xenheap\n");

    domheap_pages = heap_pages - xenheap_pages;