[PATCH 0/2] x86/shadow: shadow_get_page_from_l1e() adjustments

[PATCH 0/2] x86/shadow: shadow_get_page_from_l1e() adjustments

[Jan Beulich]

The aspect of the function the second patch here changes has been
puzzling me for many years. I finally thought I ought to make an
attempt at reducing this to just a single get_page_from_l1e()
invocation. The first patch mainly helps readability (of the code
in general as well as the second patch).

Note that the first patch here takes "VMX: use a single, global APIC
access page" as a prereq; it could be re-based ahead of that one.

1: re-use variables in shadow_get_page_from_l1e()
2: streamline shadow_get_page_from_l1e()

Jan

[PATCH 1/2] x86/shadow: re-use variables in shadow_get_page_from_l1e()

[Jan Beulich]

There's little point in doing multiple mfn_to_page() or page_get_owner()
on all the same MFN. Calculate them once at the start of the function.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/shadow/set.c
+++ b/xen/arch/x86/mm/shadow/set.c
@@ -89,25 +89,27 @@ shadow_get_page_from_l1e(shadow_l1e_t sl
 {
     int res;
     mfn_t mfn;
-    struct domain *owner;
+    const struct page_info *pg = NULL;
+    struct domain *owner = NULL;
 
     ASSERT(!sh_l1e_is_magic(sl1e));
     ASSERT(shadow_mode_refcounts(d));
 
+    if ( mfn_valid(mfn = shadow_l1e_get_mfn(sl1e)) )
+    {
+        pg = mfn_to_page(mfn);
+        owner = page_get_owner(pg);
+    }
+
     /*
      * VMX'es APIC access MFN is just a surrogate page.  It doesn't actually
      * get accessed, and hence there's no need to refcount it (and refcounting
      * would fail, due to the page having no owner).
      */
-    if ( mfn_valid(mfn = shadow_l1e_get_mfn(sl1e)) )
+    if ( pg && !owner && (pg->count_info & PGC_extra) )
     {
-        const struct page_info *pg = mfn_to_page(mfn);
-
-        if ( !page_get_owner(pg) && (pg->count_info & PGC_extra) )
-        {
-            ASSERT(type == p2m_mmio_direct);
-            return 0;
-        }
+        ASSERT(type == p2m_mmio_direct);
+        return 0;
     }
 
     res = get_page_from_l1e(sl1e, d, d);
@@ -118,9 +120,7 @@ shadow_get_page_from_l1e(shadow_l1e_t sl
      */
     if ( unlikely(res < 0) &&
          !shadow_mode_translate(d) &&
-         mfn_valid(mfn = shadow_l1e_get_mfn(sl1e)) &&
-         (owner = page_get_owner(mfn_to_page(mfn))) &&
-         (d != owner) )
+         owner && (d != owner) )
     {
         res = xsm_priv_mapping(XSM_TARGET, d, owner);
         if ( !res )
@@ -143,9 +143,8 @@ shadow_get_page_from_l1e(shadow_l1e_t sl
          * already have checked that we're supposed to have access, so
          * we can just grab a reference directly.
          */
-        mfn = shadow_l1e_get_mfn(sl1e);
-        if ( mfn_valid(mfn) )
-            res = get_page_from_l1e(sl1e, d, page_get_owner(mfn_to_page(mfn)));
+        if ( owner )
+            res = get_page_from_l1e(sl1e, d, owner);
     }
 
     if ( unlikely(res < 0) )

[PATCH 2/2] x86/shadow: streamline shadow_get_page_from_l1e()

[Jan Beulich]

Trying get_page_from_l1e() up to three times isn't helpful; in debug
builds it may lead to log messages mking things look as if there was a
problem somewhere. And there's no need to have more than one try: The
function can only possibly succeed for one domain passed as 3rd
argument (unless the page is an MMIO one to which both have access,
but MMIO pages should be "got" by specifying the requesting domain
anyway). Re-arrange things so just the one call gets made which has a
chance of succeeding.

The code could in principle be arranged such that there's only a single
call to get_page_from_l1e(), but the conditional would become pretty
complex then and hence hard to follow / understand / adjust.

The redundant (with shadow_mode_refcounts()) shadow_mode_translate()
gets dropped.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/shadow/set.c
+++ b/xen/arch/x86/mm/shadow/set.c
@@ -112,40 +112,36 @@ shadow_get_page_from_l1e(shadow_l1e_t sl
         return 0;
     }
 
-    res = get_page_from_l1e(sl1e, d, d);
+    if ( owner == dom_io )
+        owner = NULL;
 
     /*
      * If a privileged domain is attempting to install a map of a page it does
      * not own, we let it succeed anyway.
      */
-    if ( unlikely(res < 0) &&
-         !shadow_mode_translate(d) &&
-         owner && (d != owner) )
+    if ( owner && (d != owner) &&
+         !(res = xsm_priv_mapping(XSM_TARGET, d, owner)) )
     {
-        res = xsm_priv_mapping(XSM_TARGET, d, owner);
-        if ( !res )
-        {
-            res = get_page_from_l1e(sl1e, d, owner);
-            SHADOW_PRINTK("privileged %pd installs map of mfn %"PRI_mfn" owned by %pd: %s\n",
-                           d, mfn_x(mfn), owner,
-                           res >= 0 ? "success" : "failed");
-        }
+        res = get_page_from_l1e(sl1e, d, owner);
+        SHADOW_PRINTK("privileged %pd installs map of %pd's mfn %"PRI_mfn": %s\n",
+                      d, owner, mfn_x(mfn),
+                      res >= 0 ? "success" : "failed");
     }
-
     /* Okay, it might still be a grant mapping PTE.  Try it. */
-    if ( unlikely(res < 0) &&
-         (type == p2m_grant_map_rw ||
-          (type == p2m_grant_map_ro &&
-           !(shadow_l1e_get_flags(sl1e) & _PAGE_RW))) )
+    else if ( owner &&
+              (type == p2m_grant_map_rw ||
+               (type == p2m_grant_map_ro &&
+                !(shadow_l1e_get_flags(sl1e) & _PAGE_RW))) )
     {
         /*
          * It's a grant mapping.  The grant table implementation will
          * already have checked that we're supposed to have access, so
          * we can just grab a reference directly.
          */
-        if ( owner )
-            res = get_page_from_l1e(sl1e, d, owner);
+        res = get_page_from_l1e(sl1e, d, owner);
     }
+    else
+        res = get_page_from_l1e(sl1e, d, d);
 
     if ( unlikely(res < 0) )
     {

Re: [PATCH 0/2] x86/shadow: shadow_get_page_from_l1e() adjustments

[Tim Deegan]

At 13:46 +0200 on 12 Apr (1618235218), Jan Beulich wrote:
> The aspect of the function the second patch here changes has been
> puzzling me for many years. I finally thought I ought to make an
> attempt at reducing this to just a single get_page_from_l1e()
> invocation. The first patch mainly helps readability (of the code
> in general as well as the second patch).
> 
> Note that the first patch here takes "VMX: use a single, global APIC
> access page" as a prereq; it could be re-based ahead of that one.
> 
> 1: re-use variables in shadow_get_page_from_l1e()
> 2: streamline shadow_get_page_from_l1e()

Acked-by: Tim Deegan <tim@xen.org>

Thanks,

Tim.

Re: [PATCH 0/2] x86/shadow: shadow_get_page_from_l1e() adjustments

[Jan Beulich]

On 15.04.2021 18:10, Tim Deegan wrote:
> At 13:46 +0200 on 12 Apr (1618235218), Jan Beulich wrote:
>> The aspect of the function the second patch here changes has been
>> puzzling me for many years. I finally thought I ought to make an
>> attempt at reducing this to just a single get_page_from_l1e()
>> invocation. The first patch mainly helps readability (of the code
>> in general as well as the second patch).
>>
>> Note that the first patch here takes "VMX: use a single, global APIC
>> access page" as a prereq; it could be re-based ahead of that one.
>>
>> 1: re-use variables in shadow_get_page_from_l1e()
>> 2: streamline shadow_get_page_from_l1e()
> 
> Acked-by: Tim Deegan <tim@xen.org>

Thanks. Any thoughts on the shadow part of the prereq patch mentioned
above?

Jan