Re: [PATCH RFCv2 14/15] xen/arm: mm: Rework setup_xenheap_mappings()

[Julien Grall <julien@xxxxxxx>]

Hi,

On 15/05/2021 00:51, Stefano Stabellini wrote:
> On Sun, 25 Apr 2021, Julien Grall wrote:
> > From: Julien Grall <julien.grall@xxxxxxx>
> >
> > A few issues have been reported with setup_xenheap_mappings() over the
> > last couple of years. The main ones are:
> >      - It will break on platform supporting more than 512GB of RAM
> >        because the memory allocated by the boot allocator is not yet
> >        mapped.
> >      - Aligning all the regions to 1GB may lead to unexpected result
> >        because we may alias non-cacheable region (such as device or reserved
> >        regions).
> >
> > map_pages_to_xen() was recently reworked to allow superpage mappings and
> > deal with the use of page-tables before they are mapped.
> >
> > Most of the code in setup_xenheap_mappings() is now replaced with a
> > single call to map_pages_to_xen().
> >
> > This also require to re-order the steps setup_mm() so the regions are
> > given to the boot allocator first and then we setup the xenheap
> > mappings.
> I know this is paranoia but doesn't this introduce a requirement on the
> size of the first bank in bootinfo.mem.bank[] ?
>
> It should be at least 8KB?
AFAIK, the current requirement is 4KB because of the 1GB mapping. Long 
term, it would be 8KB.
> I know it is unlikely but it is theoretically possible to have a first
> bank of just 1KB.
All the page allocators are working at the page granularity level. I am 
not entirely sure whether the current Xen would ignore the region or break.
Note that setup_xenheap_mappings() is taking a base MFN and a number of 
pages to map. So this doesn't look to be a new problem here.
For the 8KB requirement, we can look at first all the pages to the boot 
allocator and then do the mapping.
> I think we should write the requirement down with an in-code comment?
> Or better maybe we should write a message about it in the panic below?
I can write an in-code comment but I think writing down in the panic() 
would be wrong because there is no promise this is going to be the only 
reason it fails (imagine there is a bug in the code...).
Cheers,

--
Julien Grall