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 24/11/2020 00:25, Stefano Stabellini wrote:
> On Mon, 23 Nov 2020, Julien Grall wrote:
>> 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?
> 
> In pseudo-code:
> 
>    mask = mfn | vfn | nr_mfns;
>    if (mask & ((1<<FIRST_ORDER) - 1))
>    if (mask & ((1<<SECOND_ORDER) - 1))
>    if (mask & ((1<<THIRD_ORDER) - 1))
>    ...
> 
> As you wrote the mask is used to find the max size that can be used for
> the mapping.
> 
> But let's take nr_mfns out of the equation for a moment for clarity:
> 
>    mask = mfn | vfn;
>    if (mask & ((1<<FIRST_ORDER) - 1))
>    if (mask & ((1<<SECOND_ORDER) - 1))
>    if (mask & ((1<<THIRD_ORDER) - 1))
>    ...
> 
> How would you describe this check? I'd call this an alignment check,
> is it not?
If you take the ``if`` alone, yes they are alignment check. But if you 
take the overall code, then it will just compute which mapping size can 
be used.

However, what I am disputing here is "rely" because there are no 
assumption made on the alignment in the loop (we are able to cater any 
size). In fact, the fact mfn and vfn should be aligned to the mapping 
size is a requirement from the hardware and not the implementation.

Cheers,

-- 
Julien Grall