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

[Julien Grall <julien@xen.org>]

Hi,

Sorry for the late answering. I finally picked up that series again and 
now preparing a new version.

On 18/05/2021 01:50, Stefano Stabellini wrote:
> On Sat, 15 May 2021, Julien Grall wrote:
>> Hi,
>>
>> On 15/05/2021 00:51, Stefano Stabellini wrote:
>>> On Sun, 25 Apr 2021, Julien Grall wrote:
>>>> From: Julien Grall <julien.grall@arm.com>
>>>>
>>>> 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.
> 
> Yeah... the example of the first bank being 1KB is wrong because, like
> you wrote, it wouldn't work before your patches either, and probably it
> will never work.
> 
> Maybe a better example is a first bank of 4KB exactly.

I have done more testing with the 1GB alignment dropped. The 
restrictions are a bit more complicated.

Not all the memory in a bank will go to the boot allocator. This can 
happen if the memory were have already been allocated for other purpose 
(e.g. modules, reserved area...).

So the region needs enough free memory to be able to map the entire 
region. The amount needed will depend on the size of the region.

So I will split the loop in two separate loops. The first loop will add 
all available pages to the boot allocator. The second loop will actually 
do the mapping.

Cheers,

-- 
Julien Grall