[PATCH 0/2] x86/mm: {paging,sh}_{cmpxchg,write}_guest_entry() adjustments

Jan Beulich posted 2 patches 3 years, 6 months ago
Only 0 patches received!
[PATCH 0/2] x86/mm: {paging,sh}_{cmpxchg,write}_guest_entry() adjustments
Posted by Jan Beulich 3 years, 6 months ago
1: {paging,sh}_{cmpxchg,write}_guest_entry() cannot fault
2: remove some indirection from {paging,sh}_cmpxchg_guest_entry()

Jan

Re: [PATCH 0/2] x86/mm: {paging,sh}_{cmpxchg,write}_guest_entry() adjustments
Posted by Tim Deegan 3 years, 5 months ago
At 13:56 +0200 on 28 Sep (1601301371), Jan Beulich wrote:
> 1: {paging,sh}_{cmpxchg,write}_guest_entry() cannot fault
> 2: remove some indirection from {paging,sh}_cmpxchg_guest_entry()

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

[PATCH 1/2] x86/mm: {paging, sh}_{cmpxchg, write}_guest_entry() cannot fault
Posted by Jan Beulich 3 years, 6 months ago
As of 2d0557c5cbeb ("x86: Fold page_info lock into type_info") we
haven't been updating guest page table entries through linear page
tables anymore. All updates have been using domain mappings since then.
Drop the use of guest/user access helpers there, and hence also the
boolean return values of the involved functions.

update_intpte(), otoh, gets its boolean return type retained for now,
as we may want to bound the CMPXCHG retry loop, indicating failure to
the caller instead when the retry threshold got exceeded.

With this {,__}cmpxchg_user() become unused, so they too get dropped.
(In fact, dropping them was the motivation of making the change.)

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

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4033,8 +4033,8 @@ long do_mmu_update(
 
                 case PGT_writable_page:
                     perfc_incr(writable_mmu_updates);
-                    if ( paging_write_guest_entry(v, va, req.val, mfn) )
-                        rc = 0;
+                    paging_write_guest_entry(v, va, req.val, mfn);
+                    rc = 0;
                     break;
                 }
                 page_unlock(page);
@@ -4044,9 +4044,9 @@ long do_mmu_update(
             else if ( get_page_type(page, PGT_writable_page) )
             {
                 perfc_incr(writable_mmu_updates);
-                if ( paging_write_guest_entry(v, va, req.val, mfn) )
-                    rc = 0;
+                paging_write_guest_entry(v, va, req.val, mfn);
                 put_page_type(page);
+                rc = 0;
             }
 
             put_page(page);
--- a/xen/arch/x86/mm/shadow/private.h
+++ b/xen/arch/x86/mm/shadow/private.h
@@ -396,9 +396,9 @@ int shadow_write_p2m_entry(struct p2m_do
                            unsigned int level);
 
 /* Functions that atomically write PV guest PT entries */
-bool sh_write_guest_entry(struct vcpu *v, intpte_t *p, intpte_t new,
+void sh_write_guest_entry(struct vcpu *v, intpte_t *p, intpte_t new,
                           mfn_t gmfn);
-bool sh_cmpxchg_guest_entry(struct vcpu *v, intpte_t *p, intpte_t *old,
+void sh_cmpxchg_guest_entry(struct vcpu *v, intpte_t *p, intpte_t *old,
                             intpte_t new, mfn_t gmfn);
 
 /* Update all the things that are derived from the guest's CR0/CR3/CR4.
--- a/xen/arch/x86/mm/shadow/pv.c
+++ b/xen/arch/x86/mm/shadow/pv.c
@@ -26,43 +26,35 @@
 
 /*
  * Write a new value into the guest pagetable, and update the shadows
- * appropriately.  Returns false if we page-faulted, true for success.
+ * appropriately.
  */
-bool
+void
 sh_write_guest_entry(struct vcpu *v, intpte_t *p, intpte_t new, mfn_t gmfn)
 {
-    unsigned int failed;
-
     paging_lock(v->domain);
-    failed = __copy_to_user(p, &new, sizeof(new));
-    if ( failed != sizeof(new) )
-        sh_validate_guest_entry(v, gmfn, p, sizeof(new));
+    write_atomic(p, new);
+    sh_validate_guest_entry(v, gmfn, p, sizeof(new));
     paging_unlock(v->domain);
-
-    return !failed;
 }
 
 /*
  * Cmpxchg a new value into the guest pagetable, and update the shadows
- * appropriately. Returns false if we page-faulted, true if not.
+ * appropriately.
  * N.B. caller should check the value of "old" to see if the cmpxchg itself
  * was successful.
  */
-bool
+void
 sh_cmpxchg_guest_entry(struct vcpu *v, intpte_t *p, intpte_t *old,
                        intpte_t new, mfn_t gmfn)
 {
-    bool failed;
-    intpte_t t = *old;
+    intpte_t t;
 
     paging_lock(v->domain);
-    failed = cmpxchg_user(p, t, new);
+    t = cmpxchg(p, *old, new);
     if ( t == *old )
         sh_validate_guest_entry(v, gmfn, p, sizeof(new));
     *old = t;
     paging_unlock(v->domain);
-
-    return !failed;
 }
 
 /*
--- a/xen/arch/x86/pv/mm.h
+++ b/xen/arch/x86/pv/mm.h
@@ -43,9 +43,7 @@ static inline bool update_intpte(intpte_
 
 #ifndef PTE_UPDATE_WITH_CMPXCHG
     if ( !preserve_ad )
-    {
-        rv = paging_write_guest_entry(v, p, new, mfn);
-    }
+        paging_write_guest_entry(v, p, new, mfn);
     else
 #endif
     {
@@ -58,14 +56,7 @@ static inline bool update_intpte(intpte_
             if ( preserve_ad )
                 _new |= old & (_PAGE_ACCESSED | _PAGE_DIRTY);
 
-            rv = paging_cmpxchg_guest_entry(v, p, &t, _new, mfn);
-            if ( unlikely(rv == 0) )
-            {
-                gdprintk(XENLOG_WARNING,
-                         "Failed to update %" PRIpte " -> %" PRIpte
-                         ": saw %" PRIpte "\n", old, _new, t);
-                break;
-            }
+            paging_cmpxchg_guest_entry(v, p, &t, _new, mfn);
 
             if ( t == old )
                 break;
--- a/xen/arch/x86/pv/ro-page-fault.c
+++ b/xen/arch/x86/pv/ro-page-fault.c
@@ -168,10 +168,9 @@ static int ptwr_emulated_update(unsigned
     if ( p_old )
     {
         ol1e = l1e_from_intpte(old);
-        if ( !paging_cmpxchg_guest_entry(v, &l1e_get_intpte(*pl1e),
-                                         &old, l1e_get_intpte(nl1e), mfn) )
-            ret = X86EMUL_UNHANDLEABLE;
-        else if ( l1e_get_intpte(ol1e) == old )
+        paging_cmpxchg_guest_entry(v, &l1e_get_intpte(*pl1e), &old,
+                                   l1e_get_intpte(nl1e), mfn);
+        if ( l1e_get_intpte(ol1e) == old )
             ret = X86EMUL_OKAY;
         else
         {
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -96,9 +96,9 @@ struct shadow_paging_mode {
 #ifdef CONFIG_SHADOW_PAGING
     void          (*detach_old_tables     )(struct vcpu *v);
 #ifdef CONFIG_PV
-    bool          (*write_guest_entry     )(struct vcpu *v, intpte_t *p,
+    void          (*write_guest_entry     )(struct vcpu *v, intpte_t *p,
                                             intpte_t new, mfn_t gmfn);
-    bool          (*cmpxchg_guest_entry   )(struct vcpu *v, intpte_t *p,
+    void          (*cmpxchg_guest_entry   )(struct vcpu *v, intpte_t *p,
                                             intpte_t *old, intpte_t new,
                                             mfn_t gmfn);
 #endif
@@ -324,15 +324,15 @@ static inline void paging_update_paging_
  * paging-assistance state appropriately.  Returns false if we page-faulted,
  * true for success.
  */
-static inline bool paging_write_guest_entry(
+static inline void paging_write_guest_entry(
     struct vcpu *v, intpte_t *p, intpte_t new, mfn_t gmfn)
 {
 #ifdef CONFIG_SHADOW_PAGING
     if ( unlikely(paging_mode_shadow(v->domain)) && paging_get_hostmode(v) )
-        return paging_get_hostmode(v)->shadow.write_guest_entry(v, p, new,
-                                                                gmfn);
+        paging_get_hostmode(v)->shadow.write_guest_entry(v, p, new, gmfn);
+    else
 #endif
-    return !__copy_to_user(p, &new, sizeof(new));
+        write_atomic(p, new);
 }
 
 
@@ -342,15 +342,16 @@ static inline bool paging_write_guest_en
  * true if not.  N.B. caller should check the value of "old" to see if the
  * cmpxchg itself was successful.
  */
-static inline bool paging_cmpxchg_guest_entry(
+static inline void paging_cmpxchg_guest_entry(
     struct vcpu *v, intpte_t *p, intpte_t *old, intpte_t new, mfn_t gmfn)
 {
 #ifdef CONFIG_SHADOW_PAGING
     if ( unlikely(paging_mode_shadow(v->domain)) && paging_get_hostmode(v) )
-        return paging_get_hostmode(v)->shadow.cmpxchg_guest_entry(v, p, old,
-                                                                  new, gmfn);
+        paging_get_hostmode(v)->shadow.cmpxchg_guest_entry(v, p, old,
+                                                           new, gmfn);
+    else
 #endif
-    return !cmpxchg_user(p, *old, new);
+        *old = cmpxchg(p, *old, new);
 }
 
 #endif /* CONFIG_PV */
--- a/xen/include/asm-x86/x86_64/system.h
+++ b/xen/include/asm-x86/x86_64/system.h
@@ -59,47 +59,4 @@ static always_inline __uint128_t cmpxchg
     __cmpxchg16b(_p, (void *)(o), (void *)(n));            \
 })
 
-/*
- * This function causes value _o to be changed to _n at location _p.
- * If this access causes a fault then we return 1, otherwise we return 0.
- * If no fault occurs then _o is updated to the value we saw at _p. If this
- * is the same as the initial value of _o then _n is written to location _p.
- */
-#define __cmpxchg_user(_p, _o, _n, _oppre, _regtype)                    \
-    stac();                                                             \
-    asm volatile (                                                      \
-        "1: lock cmpxchg %"_oppre"[new], %[ptr]\n"                      \
-        "2:\n"                                                          \
-        ".section .fixup,\"ax\"\n"                                      \
-        "3:     movl $1, %[rc]\n"                                       \
-        "       jmp 2b\n"                                               \
-        ".previous\n"                                                   \
-        _ASM_EXTABLE(1b, 3b)                                            \
-        : "+a" (_o), [rc] "=r" (_rc),                                   \
-          [ptr] "+m" (*(volatile typeof(*(_p)) *)(_p))                  \
-        : [new] _regtype (_n), "[rc]" (0)                               \
-        : "memory");                                                    \
-    clac()
-
-#define cmpxchg_user(_p, _o, _n)                                        \
-({                                                                      \
-    int _rc;                                                            \
-    switch ( sizeof(*(_p)) )                                            \
-    {                                                                   \
-    case 1:                                                             \
-        __cmpxchg_user(_p, _o, _n, "b", "q");                           \
-        break;                                                          \
-    case 2:                                                             \
-        __cmpxchg_user(_p, _o, _n, "w", "r");                           \
-        break;                                                          \
-    case 4:                                                             \
-        __cmpxchg_user(_p, _o, _n, "k", "r");                           \
-        break;                                                          \
-    case 8:                                                             \
-        __cmpxchg_user(_p, _o, _n, "q", "r");                           \
-        break;                                                          \
-    }                                                                   \
-    _rc;                                                                \
-})
-
 #endif /* __X86_64_SYSTEM_H__ */


[PATCH 2/2] x86/mm: remove some indirection from {paging,sh}_cmpxchg_guest_entry()
Posted by Jan Beulich 3 years, 6 months ago
Make the functions more similar to cmpxchg() in that they now take an
integral "old" input and return the value read.

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

--- a/xen/arch/x86/mm/shadow/private.h
+++ b/xen/arch/x86/mm/shadow/private.h
@@ -398,8 +398,8 @@ int shadow_write_p2m_entry(struct p2m_do
 /* Functions that atomically write PV guest PT entries */
 void sh_write_guest_entry(struct vcpu *v, intpte_t *p, intpte_t new,
                           mfn_t gmfn);
-void sh_cmpxchg_guest_entry(struct vcpu *v, intpte_t *p, intpte_t *old,
-                            intpte_t new, mfn_t gmfn);
+intpte_t sh_cmpxchg_guest_entry(struct vcpu *v, intpte_t *p, intpte_t old,
+                                intpte_t new, mfn_t gmfn);
 
 /* Update all the things that are derived from the guest's CR0/CR3/CR4.
  * Called to initialize paging structures if the paging mode
--- a/xen/arch/x86/mm/shadow/pv.c
+++ b/xen/arch/x86/mm/shadow/pv.c
@@ -39,22 +39,22 @@ sh_write_guest_entry(struct vcpu *v, int
 
 /*
  * Cmpxchg a new value into the guest pagetable, and update the shadows
- * appropriately.
- * N.B. caller should check the value of "old" to see if the cmpxchg itself
- * was successful.
+ * appropriately.  Returns the previous entry found, which the caller is
+ * expected to check to see if the cmpxchg was successful.
  */
-void
-sh_cmpxchg_guest_entry(struct vcpu *v, intpte_t *p, intpte_t *old,
+intpte_t
+sh_cmpxchg_guest_entry(struct vcpu *v, intpte_t *p, intpte_t old,
                        intpte_t new, mfn_t gmfn)
 {
     intpte_t t;
 
     paging_lock(v->domain);
-    t = cmpxchg(p, *old, new);
-    if ( t == *old )
+    t = cmpxchg(p, old, new);
+    if ( t == old )
         sh_validate_guest_entry(v, gmfn, p, sizeof(new));
-    *old = t;
     paging_unlock(v->domain);
+
+    return t;
 }
 
 /*
--- a/xen/arch/x86/pv/mm.h
+++ b/xen/arch/x86/pv/mm.h
@@ -47,16 +47,14 @@ static inline bool update_intpte(intpte_
     else
 #endif
     {
-        intpte_t t = old;
-
         for ( ; ; )
         {
-            intpte_t _new = new;
+            intpte_t _new = new, t;
 
             if ( preserve_ad )
                 _new |= old & (_PAGE_ACCESSED | _PAGE_DIRTY);
 
-            paging_cmpxchg_guest_entry(v, p, &t, _new, mfn);
+            t = paging_cmpxchg_guest_entry(v, p, old, _new, mfn);
 
             if ( t == old )
                 break;
--- a/xen/arch/x86/pv/ro-page-fault.c
+++ b/xen/arch/x86/pv/ro-page-fault.c
@@ -168,8 +168,8 @@ static int ptwr_emulated_update(unsigned
     if ( p_old )
     {
         ol1e = l1e_from_intpte(old);
-        paging_cmpxchg_guest_entry(v, &l1e_get_intpte(*pl1e), &old,
-                                   l1e_get_intpte(nl1e), mfn);
+        old = paging_cmpxchg_guest_entry(v, &l1e_get_intpte(*pl1e), old,
+                                         l1e_get_intpte(nl1e), mfn);
         if ( l1e_get_intpte(ol1e) == old )
             ret = X86EMUL_OKAY;
         else
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -98,8 +98,8 @@ struct shadow_paging_mode {
 #ifdef CONFIG_PV
     void          (*write_guest_entry     )(struct vcpu *v, intpte_t *p,
                                             intpte_t new, mfn_t gmfn);
-    void          (*cmpxchg_guest_entry   )(struct vcpu *v, intpte_t *p,
-                                            intpte_t *old, intpte_t new,
+    intpte_t      (*cmpxchg_guest_entry   )(struct vcpu *v, intpte_t *p,
+                                            intpte_t old, intpte_t new,
                                             mfn_t gmfn);
 #endif
 #ifdef CONFIG_HVM
@@ -342,16 +342,15 @@ static inline void paging_write_guest_en
  * true if not.  N.B. caller should check the value of "old" to see if the
  * cmpxchg itself was successful.
  */
-static inline void paging_cmpxchg_guest_entry(
-    struct vcpu *v, intpte_t *p, intpte_t *old, intpte_t new, mfn_t gmfn)
+static inline intpte_t paging_cmpxchg_guest_entry(
+    struct vcpu *v, intpte_t *p, intpte_t old, intpte_t new, mfn_t gmfn)
 {
 #ifdef CONFIG_SHADOW_PAGING
     if ( unlikely(paging_mode_shadow(v->domain)) && paging_get_hostmode(v) )
-        paging_get_hostmode(v)->shadow.cmpxchg_guest_entry(v, p, old,
-                                                           new, gmfn);
-    else
+        return paging_get_hostmode(v)->shadow.cmpxchg_guest_entry(v, p, old,
+                                                                  new, gmfn);
 #endif
-        *old = cmpxchg(p, *old, new);
+    return cmpxchg(p, old, new);
 }
 
 #endif /* CONFIG_PV */