Re: [Xen-devel] [PATCH] x86/shadow: sh_pagetable_dying() cleanup

[Tim Deegan <tim@xxxxxxx>]

At 07:17 -0600 on 08 Jun (1465370220), Jan Beulich wrote:
> Don't call shadow_hash_lookup() at all when get_gfn_query_unlocked()
> didn't return a valid MFN.
> 
> Also no need for local variables used only once, the more with scopes
> much wider than their actual use.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Question is whether we shouldn't also get rid of
> guest_l[234]e_get_paddr(), as they're now effectively unused.
> 
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -4501,7 +4501,6 @@ static void sh_pagetable_dying(struct vc
>      p2m_type_t p2mt;
>      char *gl3pa = NULL;
>      guest_l3e_t *gl3e = NULL;
> -    paddr_t gl2a = 0;
>      unsigned long l3gfn;
>      mfn_t l3mfn;
>  
> @@ -4528,7 +4527,6 @@ static void sh_pagetable_dying(struct vc
>      }
>      for ( i = 0; i < 4; i++ )
>      {
> -        unsigned long gfn;
>          mfn_t smfn, gmfn;
>  
>          if ( fast_path ) {
> @@ -4540,10 +4538,11 @@ static void sh_pagetable_dying(struct vc
>          else
>          {
>              /* retrieving the l2s */
> -            gl2a = guest_l3e_get_paddr(gl3e[i]);
> -            gfn = gl2a >> PAGE_SHIFT;
> -            gmfn = get_gfn_query_unlocked(d, gfn, &p2mt);
> -            smfn = shadow_hash_lookup(d, mfn_x(gmfn), SH_type_l2_pae_shadow);
> +            gmfn = get_gfn_query_unlocked(d, 
> gfn_x(guest_l3e_get_gfn(gl3e[i])),
> +                                          &p2mt);
> +            smfn = likely(mfn_x(gmfn) != INVALID_MFN)
> +                   ? shadow_hash_lookup(d, mfn_x(gmfn), 
> SH_type_l2_pae_shadow)
> +                   : gmfn;

I think this is clearer as:

            smfn = unlikely(mfn_x(gmfn) == INVALID_MFN)
                   : _mfn(INVALID_MFN)
                   ? shadow_hash_lookup(d, mfn_x(gmfn), SH_type_l2_pae_shadow);

With that change, Acked-by: Tim Deegan <tim@xxxxxxx>

And yes, we can drop the _get_paddr() functions, if you like.

Cheers,

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel