Re: [PATCH RFC 4/6] xen/arm: mm: Allow other mapping size in xen_pt_update_entry()

[Julien Grall <julien@xen.org>]

Hi Stefano,

On 23/11/2020 22:27, Stefano Stabellini wrote:
> On Fri, 20 Nov 2020, Julien Grall wrote:
>>>>        /*
>>>>         * For arm32, page-tables are different on each CPUs. Yet, they
>>>> share
>>>> @@ -1265,14 +1287,43 @@ static int xen_pt_update(unsigned long virt,
>>>>          spin_lock(&xen_pt_lock);
>>>>    -    for ( ; addr < addr_end; addr += PAGE_SIZE )
>>>> +    while ( left )
>>>>        {
>>>> -        rc = xen_pt_update_entry(root, addr, mfn, flags);
>>>> +        unsigned int order;
>>>> +        unsigned long mask;
>>>> +
>>>> +        /*
>>>> +         * Don't take into account the MFN when removing mapping (i.e
>>>> +         * MFN_INVALID) to calculate the correct target order.
>>>> +         *
>>>> +         * XXX: Support superpage mappings if nr is not aligned to a
>>>> +         * superpage size.
>>>
>>> It would be good to add another sentence to explain that the checks
>>> below are simply based on masks and rely on the mfn, vfn, and also
>>> nr_mfn to be superpage aligned. (It took me some time to figure it out.)
>>
>> I am not sure to understand what you wrote here. Could you suggest a sentence?
> 
> Something like the following:
> 
> /*
>   * Don't take into account the MFN when removing mapping (i.e
>   * MFN_INVALID) to calculate the correct target order.
>   *
>   * This loop relies on mfn, vfn, and nr_mfn, to be all superpage
>   * aligned, and it uses `mask' to check for that.

Unfortunately, I am still not sure to understand this comment.
The loop can deal with any (super)page size (4KB, 2MB, 1GB). There are 
no assumption on any alignment for mfn, vfn and nr_mfn.

By OR-ing the 3 components together, we can use it to find out the 
maximum size that can be used for the mapping.

So can you clarify what you mean?

>   *
>   * XXX: Support superpage mappings if nr_mfn is not aligned to a
>   * superpage size.
>   */
> 
> 
>> Regarding the TODO itself, we have the exact same one in the P2M code. I
>> couldn't find a clever way to deal with it yet. Any idea how this could be
>> solved?
>   
> I was thinking of a loop that start with the highest possible superpage
> size that virt and mfn are aligned to, and also smaller or equal to
> nr_mfn. So rather than using the mask to also make sure nr_mfns is
> aligned, I would only use the mask to check that mfn and virt are
> aligned. Then, we only need to check that superpage_size <= left.
> 
> Concrete example: virt and mfn are 2MB aligned, nr_mfn is 5MB / 1280 4K
> pages. We allocate 2MB superpages until onlt 1MB is left. At that point
> superpage_size <= left fails and we go down to 4K allocations.
> 
> Would that work?

Unfortunately no, AFAICT, your assumption is that vfn/mfn are originally 
aligned to higest possible superpage size. There are situation where 
this is not the case.

To give a concrete example, at the moment the RAM is mapped using 1GB 
superpage in Xen. But in the future, we will only want to map RAM 
regions in the directmap that haven't been marked as reserved [1].

Those reserved regions don't have architectural alignment or placement.

I will use an over-exegerated example (or maybe not :)).

Imagine you have 4GB of RAM starting at 0. The HW/Software engineer 
decided to place a 2MB reserved region start at 512MB.

As a result we would want to map two RAM regions:
    1) 0 to 512MB
    2) 514MB to 4GB

I will only focus on 2). In the ideal situation, we would want to map
    a) 514MB to 1GB using 2MB superpage
    b) 1GB to 4GB using 1GB superpage

We don't want be to use 2MB superpage because this will increase TLB 
pressure (we want to avoid Xen using too much TLB entries) and also 
increase the size of the page-tables.

Therefore, we want to select the best size for each iteration. For now, 
the only solution I can come up with is to OR vfn/mfn and then use a 
series of check to compare the mask and nr_mfn.

In addition to the "classic" mappings (i.e. 4KB, 2MB, 1GB). I would like 
to explore contiguous mapping (e.g. 64KB, 32MB) to further reduce the 
TLBs pressure. Note that a processor may or may not take advantage of 
contiguous mapping to reduce the number of TLBs used.

This will unfortunately increase the numbers of check. I will try to 
come up with a patch and we can discuss from there.

Cheers,

[1] Reserved region may be marked as uncacheable and therefore we 
shouldn't map them in Xen address space to avoid break cache coherency.

-- 
Julien Grall