[Xen-devel] [PATCH MM-PART3 v2 04/12] xen/arm: mm: Only increment mfn when valid in xen_pt_update

Julien Grall posted 12 patches 1 year, 11 months ago

[Xen-devel] [PATCH MM-PART3 v2 04/12] xen/arm: mm: Only increment mfn when valid in xen_pt_update

Posted by Julien Grall 1 year, 11 months ago
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@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

---
    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);
     }
 
     /*
-- 
2.11.0


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

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

Posted by Stefano Stabellini 1 year, 10 months ago
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@arm.com>
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
> 
> ---
>     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...

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

[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)

Posted by Julien Grall 1 year, 10 months ago
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@arm.com>
>> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>>
>> ---
>>      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.

Cheers,

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

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)

Posted by Jan Beulich 1 year, 10 months ago
>>> On 11.06.19 at 21:56, <Julien.Grall@arm.com> 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@arm.com>
>>> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>>>
>>> ---
>>>      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.

FWIW - I agree with Andrew.

Jan



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

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)

Posted by Andrew Cooper 1 year, 10 months ago
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@arm.com>
>>> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>>>
>>> ---
>>>      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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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)

Posted by Julien Grall 1 year, 10 months ago
Hi,

On 11/06/2019 21:24, Andrew Cooper wrote:
> On 11/06/2019 20:56, Julien Grall wrote:
>> 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@arm.com>
>>>> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>>>>
>>>> ---
>>>>       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.

Thank you for the input! I will keep mfn_add() as it is.

Cheers,

-- 
Julien Grall

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

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)

Posted by Stefano Stabellini 1 year, 10 months ago
On Wed, 12 Jun 2019, Julien Grall wrote:
> Hi,
> 
> On 11/06/2019 21:24, Andrew Cooper wrote:
> > On 11/06/2019 20:56, Julien Grall wrote:
> > > 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@arm.com>
> > > > > Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
> > > > > 
> > > > > ---
> > > > >       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.
> 
> Thank you for the input! I will keep mfn_add() as it is.

Add my acked-by to the original patch.

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