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)

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@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.
> 
> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel