[PATCH] x86/shadow: sh_pagetable_dying() cleanup

[PATCH] x86/shadow: sh_pagetable_dying() cleanup

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1588 bytes --]

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) )




[-- Attachment #2: x86-sh-get-gfn.patch --]
[-- Type: text/plain, Size: 1626 bytes --]

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) )

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

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

[Tim Deegan]

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.

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