Re: [Xen-devel] Checking INVALID_MFN in mfn_add (WAS: Re: [PATCH MM-PART3 v2 04/12] xen/arm: mm: Only increment mfn when valid in xen_pt_update)

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 11/06/2019 20:56, Julien Grall wrote:
> Hi,
>
> On 11/06/2019 19:37, Stefano Stabellini wrote:
>> On Tue, 14 May 2019, Julien Grall wrote:
>>> Currently, the MFN will be incremented even if it is invalid. This will
>>> result to have a valid MFN after the first iteration.
>>>
>>> While this is not a major problem today, this will be in the future if
>>> the code expect an invalid MFN at every iteration.
>>>
>>> Such behavior is prevented by avoiding to increment an invalid MFN.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
>>> Reviewed-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
>>>
>>> ---
>>>      Changes in v2:
>>>          - Move the patch earlier on in the series
>>>          - Add Andrii's reviewed-by
>>> ---
>>>   xen/arch/arm/mm.c | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>>> index f956aa6399..9de2a1150f 100644
>>> --- a/xen/arch/arm/mm.c
>>> +++ b/xen/arch/arm/mm.c
>>> @@ -1051,11 +1051,14 @@ static int xen_pt_update(enum xenmap_operation op,
>>>   
>>>       spin_lock(&xen_pt_lock);
>>>   
>>> -    for(; addr < addr_end; addr += PAGE_SIZE, mfn = mfn_add(mfn, 1))
>>> +    for( ; addr < addr_end; addr += PAGE_SIZE )
>>>       {
>>>           rc = xen_pt_update_entry(op, addr, mfn, flags);
>>>           if ( rc )
>>>               break;
>>> +
>>> +        if ( !mfn_eq(mfn, INVALID_MFN) )
>>> +            mfn = mfn_add(mfn, 1);
>>>       }
>> This is OK but got me thinking: should we be updating the mfn in mfn_add
>> if the mfn is INVALID? The mfn_eq(mfn, INVALID_MFN) could live inside
>> the static inline mfn_t mfn_add function. What do you think? I don't
>> think there are any valid scenarios where we want to increment
>> INVALID_MFN...
> My first thought is mfn_add(...) may be used in place where we know the 
> mfn is not INVALID_MFN. So we would add extra check when it may not be 
> necessary. Although, I am not sure if it is important.
>
> I have added Andrew & Jan to get any opinions.

mfn_add(foo, bar) is shorthand for foo += bar, and should remain as such.

It exists only because we can't overload operators in C, and want
something slightly more readable than _mfn(mfn_x(foo) + bar)

Behind the scenes, the compiler will turn it back into a single add
instruction.

The saturating behaviour here is specific to the pagetable opereations
where passing INVALID_MFN is an alias for unmap, and is therefore not
useful in the majority of the users of mfn_add(), and certainly not
something we should have a hidden branch for in the middle of many tight
loops.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel