[PATCH RFC v4] x86/mm: Clean up p2m_finish_type_change return value

* [PATCH RFC v4] x86/mm: Clean up p2m_finish_type_change return value
@ 2019-03-27 15:21 Alexandru Stefan ISAILA
  2019-03-27 16:07 ` Jan Beulich
  2019-03-27 16:44 ` George Dunlap
  0 siblings, 2 replies; 5+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-03-27 15:21 UTC (permalink / raw)
  To: xen-devel
  Cc: kevin.tian, wei.liu2, jun.nakajima, george.dunlap,
	andrew.cooper3, Paul.Durrant, jbeulich, Alexandru Stefan ISAILA,
	roger.pau

In the case of any errors, finish_type_change() passes values returned
from p2m->recalc() up the stack (with some exceptions in the case where
an error is expected); this eventually ends up being returned to the
XEN_DOMOP_map_mem_type_to_ioreq_server hypercall.

However, on Intel processors (but not on AMD processor), p2m->recalc()
can also return '1' as well as '0'.  This case is handled very
inconsistently: finish_type_change() will return the value of the final
entry it attempts, discarding results for other entries;
p2m_finish_type_change() will attempt to accumulate '1's, so that it
returns '1' if any of the calls to finish_type_change() returns '1'; and
dm_op() will again return '1' only if the very last call to
p2m_finish_type_change() returns '1'.  The result is that the
XEN_DMOP_map_mem_type_to_ioreq_server() hypercall will sometimes return
0 and sometimes return 1 on success, in an unpredictable manner.

The hypercall documentation doesn't mention return values; but it's not
clear what the caller could do with the information about whether
entries had been changed or not.  At the moment it's always 0 on AMD
boxes, and *usually* 1 on Intel boxes; so nothing can be relying on a
'1' return value for correctness (or if it is, it's broken).

Make the return value on success consistently '0' by only returning
0/-ERROR from finish_type_change().  Also remove the accumulation code
from p2m_finish_type_change().

Suggested-by: George Dunlap <george.dunlap@eu.citrix.com>
Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>

---
Changes since V3:
	- Remove positive returned values from p2m->recalc.
---
 xen/arch/x86/mm/p2m-ept.c | 10 ++++++----
 xen/arch/x86/mm/p2m.c     | 15 +++++----------
 2 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index e3044bee2e..d336c138b0 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -459,8 +459,7 @@ static int ept_invalidate_emt_range(struct p2m_domain *p2m,
  *   for entries not involved in the translation of the given GFN.
  * Returns:
  * - negative errno values in error,
- * - zero if no adjustment was done,
- * - a positive value if at least one adjustment was done.
+ * - zero for ok
  */
 static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
 {
@@ -600,6 +599,9 @@ static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
             v->arch.hvm.vmx.ept_spurious_misconfig = 1;
     }
 
+    if ( rc > 0 )
+        rc = 0;
+
     return rc;
 }
 
@@ -621,7 +623,7 @@ bool_t ept_handle_misconfig(uint64_t gpa)
 
     p2m_unlock(p2m);
 
-    return spurious ? (rc >= 0) : (rc > 0);
+    return spurious && !rc;
 }
 
 /*
@@ -668,7 +670,7 @@ ept_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
 
     /* Carry out any eventually pending earlier changes first. */
     ret = resolve_misconfig(p2m, gfn);
-    if ( ret < 0 )
+    if ( ret )
         return ret;
 
     ASSERT((target == 2 && hap_has_1gb) ||
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index b9bbb8f485..d5690b96bf 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1172,7 +1172,7 @@ static int finish_type_change(struct p2m_domain *p2m,
     {
         rc = p2m->recalc(p2m, gfn);
         /*
-         * ept->recalc could return 0/1/-ENOMEM. pt->recalc could return
+         * ept->recalc could return 0/-ENOMEM. pt->recalc could return
          * 0/-ENOMEM/-ENOENT, -ENOENT isn't an error as we are looping
          * gfn here.
          */
@@ -1201,7 +1201,7 @@ int p2m_finish_type_change(struct domain *d,
 
     rc = finish_type_change(hostp2m, first_gfn, max_nr);
 
-    if ( rc < 0 )
+    if ( rc )
         goto out;
 
 #ifdef CONFIG_HVM
@@ -1213,22 +1213,17 @@ int p2m_finish_type_change(struct domain *d,
             if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
             {
                 struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
-                int rc1;
 
                 p2m_lock(altp2m);
-                rc1 = finish_type_change(altp2m, first_gfn, max_nr);
+                rc = finish_type_change(altp2m, first_gfn, max_nr);
                 p2m_unlock(altp2m);
 
-                if ( rc1 < 0 )
-                {
-                    rc = rc1;
+                if ( rc < 0 )
                     goto out;
-                }
-
-                rc |= rc1;
             }
     }
 #endif
+    rc = 0;
 
  out:
     p2m_unlock(hostp2m);
-- 
2.17.1


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

^ permalink raw reply related	[flat|nested] 5+ messages in thread