diff mbox

x86/shadow: sh_pagetable_dying() cleanup

Message ID 575836EC02000078000F3131@prv-mh.provo.novell.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jan Beulich June 8, 2016, 1:17 p.m. UTC 
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@suse.com>
---
Question is whether we shouldn't also get rid of
guest_l[234]e_get_paddr(), as they're now effectively unused.
x86/shadow: sh_pagetable_dying() cleanup

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@suse.com>
---
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;
         }
 
         if ( mfn_valid(smfn) )

Comments

Tim Deegan June 9, 2016, 10:46 a.m. UTC | #1 
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@suse.com>
> ---
> 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@xen.org>

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

Cheers,

Tim.
diff mbox

Patch

--- 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;
         }
 
         if ( mfn_valid(smfn) )