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

[Stefano Stabellini <sstabellini@kernel.org>]

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