Re: [Xen-devel] [PATCH v3 3/9] x86/mm: introduce l{1, 2}t local variables to map_pages_to_xen

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.10.2019 19:16, Hongyan Xia wrote:
> From: Wei Liu <wei.liu2@xxxxxxxxxx>
> 
> The pl2e and pl1e variables are heavily (ab)used in that function. It
> is fine at the moment because all page tables are always mapped so
> there is no need to track the life time of each variable.
> 
> We will soon have the requirement to map and unmap page tables. We
> need to track the life time of each variable to avoid leakage.
> 
> Introduce some l{1,2}t variables with limited scope so that we can
> track life time of pointers to xen page tables more easily.
> 
> No functional change.
> 
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with a couple of remarks:

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5061,10 +5061,12 @@ int map_pages_to_xen(
>                  }
>                  else
>                  {
> -                    pl2e = l3e_to_l2e(ol3e);
> +                    l2_pgentry_t *l2t;
> +
> +                    l2t = l3e_to_l2e(ol3e);

Here and elsewhere assignments could have become variable
initializers.

> @@ -5123,12 +5127,12 @@ int map_pages_to_xen(
>                  continue;
>              }
>  
> -            pl2e = alloc_xen_pagetable();
> -            if ( pl2e == NULL )
> +            l2t = alloc_xen_pagetable();
> +            if ( l2t == NULL )
>                  return -ENOMEM;
>  
>              for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
> -                l2e_write(pl2e + i,
> +                l2e_write(l2t + i,

The style here and ...

> @@ -5222,12 +5229,12 @@ int map_pages_to_xen(
>                      goto check_l3;
>                  }
>  
> -                pl1e = alloc_xen_pagetable();
> -                if ( pl1e == NULL )
> +                l1t = alloc_xen_pagetable();
> +                if ( l1t == NULL )
>                      return -ENOMEM;
>  
>                  for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
> -                    l1e_write(&pl1e[i],
> +                    l1e_write(&l1t[i],

... here (and perhaps elsewhere when touched anyway) would have
been nice if it was brought in sync (I guess I'm guilty of the
difference).

> @@ -5272,6 +5279,7 @@ int map_pages_to_xen(
>                      ((1u << PAGETABLE_ORDER) - 1)) == 0)) )
>              {
>                  unsigned long base_mfn;
> +                l1_pgentry_t *l1t;

const, as this looks to be used for lookup only?

> @@ -5325,6 +5333,7 @@ int map_pages_to_xen(
>                  ((1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT)) - 1))) )
>          {
>              unsigned long base_mfn;
> +            l2_pgentry_t *l2t;

Same here then. There also look to be more cases further up that
I did miss initially.

Whether you address the style aspects further up I'll leave to you,
but I'd really like to see the const-ness added wherever possible.

Jan

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