Re: [PATCH 1/4] x86: extend update_intpte() to support atomic get-and-update

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.07.2026 17:06, Kevin Lampis wrote:
> --- a/xen/arch/x86/pv/mm.h
> +++ b/xen/arch/x86/pv/mm.h
> @@ -66,13 +66,14 @@ static inline intpte_t paging_cmpxchg_guest_entry(
>   * How to write an entry to the guest pagetables.
>   * Returns false for failure (pointer not valid), true for success.
>   */
> -static inline bool update_intpte(intpte_t *p, intpte_t old, intpte_t new,
> -                                 mfn_t mfn, struct vcpu *v, bool preserve_ad)
> +static inline bool update_intpte(intpte_t *p, intpte_t *old, intpte_t new,
> +                                 mfn_t mfn, struct vcpu *v, bool preserve_ad,
> +                                 bool use_cmpxchg)

No 2nd boolean parameter, please. (use_cmpxchg also doesn't look to be an
overly good name; "swap" maybe?) As you switch old to being a pointer, and
as ...

>  {
>      bool rv = true;
>  
>  #ifndef PTE_UPDATE_WITH_CMPXCHG
> -    if ( !preserve_ad )
> +    if ( !preserve_ad && !use_cmpxchg )
>          paging_write_guest_entry(v, p, new, mfn);

... *old isn't used here, having callers pass in NULL in that case may be
one option.

(In any event, old becoming a pointer imo needs commenting upon, as otherwise
one might expect this to be only an output.)

Alternatively I have an old patch lying around which looks to apply cleanly,
and which may be useful here; see at the bottom.

> @@ -82,30 +83,36 @@ static inline bool update_intpte(intpte_t *p, intpte_t 
> old, intpte_t new,
>              intpte_t _new = new, t;
>  
>              if ( preserve_ad )
> -                _new |= old & (_PAGE_ACCESSED | _PAGE_DIRTY);
> +                _new |= *old & (_PAGE_ACCESSED | _PAGE_DIRTY);
>  
> -            t = paging_cmpxchg_guest_entry(v, p, old, _new, mfn);
> +            t = paging_cmpxchg_guest_entry(v, p, *old, _new, mfn);
>  
> -            if ( t == old )
> +            if ( t == *old )
>                  break;
>  
>              /* Allowed to change in Accessed/Dirty flags only. */
> -            BUG_ON((t ^ old) & ~(intpte_t)(_PAGE_ACCESSED|_PAGE_DIRTY));
> +            BUG_ON((t ^ *old) & ~(intpte_t)(_PAGE_ACCESSED|_PAGE_DIRTY));
>  
> -            old = t;
> +            *old = t;
>          }
>      }
>      return rv;
>  }
>  
> +static inline bool _update_intpte(intpte_t *p, intpte_t old, intpte_t new,
> +                                  mfn_t mfn, struct vcpu *v, bool 
> preserve_ad)
> +{
> +    return update_intpte(p, &old, new, mfn, v, preserve_ad, false);
> +}
> +
>  /*
>   * Macro that wraps the appropriate type-changes around update_intpte().
>   * Arguments are: type, ptr, old, new, mfn, vcpu
>   */
>  #define UPDATE_ENTRY(_t,_p,_o,_n,_m,_v,_ad)                         \
> -    update_intpte(&_t ## e_get_intpte(*(_p)),                       \
> -                  _t ## e_get_intpte(_o), _t ## e_get_intpte(_n),   \
> -                  (_m), (_v), (_ad))
> +    _update_intpte(&_t ## e_get_intpte(*(_p)),                      \
> +                   _t ## e_get_intpte(_o), _t ## e_get_intpte(_n),  \
> +                   (_m), (_v), (_ad))

Like the patch below does - if already this last line needs touching, I
think the excess parentheses then also want dropping.

Tangentially: We will want to consider dropping the function's return
value, since as of 1bc30c076a7f ("x86/mm:
{paging, sh}_{cmpxchg, write}_guest_entry() cannot fault") it only ever
returns true.

Jan

x86: make UPDATE_ENTRY() allow for multiple operation flags

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
Reviewed-by: George Dunlap <george.dunlap@xxxxxxxxxx>

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2149,10 +2149,9 @@ static void l3t_unlock(struct page_info
 
 /* Update the L1 entry at pl1e to new value nl1e. */
 static int mod_l1_entry(l1_pgentry_t *pl1e, l1_pgentry_t nl1e,
-                        mfn_t gl1mfn, unsigned int cmd,
+                        mfn_t gl1mfn, unsigned int update_flags,
                         struct vcpu *pt_vcpu, struct domain *pg_dom)
 {
-    bool preserve_ad = (cmd == MMU_PT_UPDATE_PRESERVE_AD);
     l1_pgentry_t ol1e = l1e_read(pl1e);
     struct domain *pt_dom = pt_vcpu->domain;
     int rc = 0;
@@ -2172,7 +2171,7 @@ static int mod_l1_entry(l1_pgentry_t *pl
         }
 
         /* Translate foreign guest address. */
-        if ( cmd != MMU_PT_UPDATE_NO_TRANSLATE &&
+        if ( !(update_flags & PTE_UPDATE_NO_TRANSLATE) &&
              paging_mode_translate(pg_dom) )
         {
             p2m_type_t p2mt;
@@ -2213,7 +2212,7 @@ static int mod_l1_entry(l1_pgentry_t *pl
         if ( !l1e_has_changed(ol1e, nl1e, ~FASTPATH_FLAG_WHITELIST) )
         {
             rc = UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu,
-                              preserve_ad);
+                              update_flags);
             if ( page )
                 put_page(page);
             return rc ? 0 : -EBUSY;
@@ -2237,7 +2236,7 @@ static int mod_l1_entry(l1_pgentry_t *pl
             put_page(page);
 
         if ( unlikely(!UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu,
-                                    preserve_ad)) )
+                                    update_flags)) )
         {
             ol1e = nl1e;
             rc = -EBUSY;
@@ -2246,7 +2245,7 @@ static int mod_l1_entry(l1_pgentry_t *pl
     else if ( pv_l1tf_check_l1e(pt_dom, nl1e) )
         return -ERESTART;
     else if ( unlikely(!UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, pt_vcpu,
-                                     preserve_ad)) )
+                                     update_flags)) )
     {
         return -EBUSY;
     }
@@ -2260,7 +2259,7 @@ static int mod_l1_entry(l1_pgentry_t *pl
 static int mod_l2_entry(l2_pgentry_t *pl2e,
                         l2_pgentry_t nl2e,
                         mfn_t mfn,
-                        int preserve_ad,
+                        unsigned int update_flags,
                         struct vcpu *vcpu)
 {
     l2_pgentry_t ol2e;
@@ -2292,7 +2291,7 @@ static int mod_l2_entry(l2_pgentry_t *pl
         /* Fast path for sufficiently-similar mappings. */
         if ( !l2e_has_changed(ol2e, nl2e, ~FASTPATH_PDE_FLAG_WHITELIST) )
         {
-            if ( UPDATE_ENTRY(l2, pl2e, ol2e, nl2e, mfn, vcpu, preserve_ad) )
+            if ( UPDATE_ENTRY(l2, pl2e, ol2e, nl2e, mfn, vcpu, update_flags) )
                 return 0;
             return -EBUSY;
         }
@@ -2301,7 +2300,7 @@ static int mod_l2_entry(l2_pgentry_t *pl
             return rc;
 
         if ( unlikely(!UPDATE_ENTRY(l2, pl2e, ol2e, nl2e, mfn, vcpu,
-                                    preserve_ad)) )
+                                    update_flags)) )
         {
             ol2e = nl2e;
             rc = -EBUSY;
@@ -2310,7 +2309,7 @@ static int mod_l2_entry(l2_pgentry_t *pl
     else if ( pv_l1tf_check_l2e(d, nl2e) )
         return -ERESTART;
     else if ( unlikely(!UPDATE_ENTRY(l2, pl2e, ol2e, nl2e, mfn, vcpu,
-                                     preserve_ad)) )
+                                     update_flags)) )
     {
         return -EBUSY;
     }
@@ -2324,7 +2323,7 @@ static int mod_l2_entry(l2_pgentry_t *pl
 static int mod_l3_entry(l3_pgentry_t *pl3e,
                         l3_pgentry_t nl3e,
                         mfn_t mfn,
-                        int preserve_ad,
+                        unsigned int update_flags,
                         struct vcpu *vcpu)
 {
     l3_pgentry_t ol3e;
@@ -2354,7 +2353,7 @@ static int mod_l3_entry(l3_pgentry_t *pl
         /* Fast path for sufficiently-similar mappings. */
         if ( !l3e_has_changed(ol3e, nl3e, ~FASTPATH_PDE_FLAG_WHITELIST) )
         {
-            rc = UPDATE_ENTRY(l3, pl3e, ol3e, nl3e, mfn, vcpu, preserve_ad);
+            rc = UPDATE_ENTRY(l3, pl3e, ol3e, nl3e, mfn, vcpu, update_flags);
             return rc ? 0 : -EFAULT;
         }
 
@@ -2364,7 +2363,7 @@ static int mod_l3_entry(l3_pgentry_t *pl
         rc = 0;
 
         if ( unlikely(!UPDATE_ENTRY(l3, pl3e, ol3e, nl3e, mfn, vcpu,
-                                    preserve_ad)) )
+                                    update_flags)) )
         {
             ol3e = nl3e;
             rc = -EFAULT;
@@ -2373,7 +2372,7 @@ static int mod_l3_entry(l3_pgentry_t *pl
     else if ( pv_l1tf_check_l3e(d, nl3e) )
         return -ERESTART;
     else if ( unlikely(!UPDATE_ENTRY(l3, pl3e, ol3e, nl3e, mfn, vcpu,
-                                     preserve_ad)) )
+                                     update_flags)) )
     {
         return -EFAULT;
     }
@@ -2386,7 +2385,7 @@ static int mod_l3_entry(l3_pgentry_t *pl
 static int mod_l4_entry(l4_pgentry_t *pl4e,
                         l4_pgentry_t nl4e,
                         mfn_t mfn,
-                        int preserve_ad,
+                        unsigned int update_flags,
                         struct vcpu *vcpu)
 {
     struct domain *d = vcpu->domain;
@@ -2416,7 +2415,7 @@ static int mod_l4_entry(l4_pgentry_t *pl
         /* Fast path for sufficiently-similar mappings. */
         if ( !l4e_has_changed(ol4e, nl4e, ~FASTPATH_PDE_FLAG_WHITELIST) )
         {
-            rc = UPDATE_ENTRY(l4, pl4e, ol4e, nl4e, mfn, vcpu, preserve_ad);
+            rc = UPDATE_ENTRY(l4, pl4e, ol4e, nl4e, mfn, vcpu, update_flags);
             return rc ? 0 : -EFAULT;
         }
 
@@ -2426,7 +2425,7 @@ static int mod_l4_entry(l4_pgentry_t *pl
         rc = 0;
 
         if ( unlikely(!UPDATE_ENTRY(l4, pl4e, ol4e, nl4e, mfn, vcpu,
-                                    preserve_ad)) )
+                                    update_flags)) )
         {
             ol4e = nl4e;
             rc = -EFAULT;
@@ -2435,7 +2434,7 @@ static int mod_l4_entry(l4_pgentry_t *pl
     else if ( pv_l1tf_check_l4e(d, nl4e) )
         return -ERESTART;
     else if ( unlikely(!UPDATE_ENTRY(l4, pl4e, ol4e, nl4e, mfn, vcpu,
-                                     preserve_ad)) )
+                                     update_flags)) )
     {
         return -EFAULT;
     }
@@ -4135,18 +4134,23 @@ long do_mmu_update(
 
             if ( page_lock(page) )
             {
+                unsigned int update_flags = (cmd == MMU_PT_UPDATE_PRESERVE_AD)
+                                            ? PTE_UPDATE_PRESERVE_AD
+                                            : (cmd == 
MMU_PT_UPDATE_NO_TRANSLATE)
+                                              ? PTE_UPDATE_NO_TRANSLATE : 0;
+
                 switch ( page->u.inuse.type_info & PGT_type_mask )
                 {
                 case PGT_l1_page_table:
                     rc = mod_l1_entry(va, l1e_from_intpte(req.val), mfn,
-                                      cmd, v, pg_owner);
+                                      update_flags, v, pg_owner);
                     break;
 
                 case PGT_l2_page_table:
                     if ( unlikely(pg_owner != pt_owner) )
                         break;
                     rc = mod_l2_entry(va, l2e_from_intpte(req.val), mfn,
-                                      cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
+                                      update_flags, v);
                     if ( !rc )
                         flush_linear_pt = true;
                     break;
@@ -4155,7 +4159,7 @@ long do_mmu_update(
                     if ( unlikely(pg_owner != pt_owner) )
                         break;
                     rc = mod_l3_entry(va, l3e_from_intpte(req.val), mfn,
-                                      cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
+                                      update_flags, v);
                     if ( !rc )
                         flush_linear_pt = true;
                     break;
@@ -4164,7 +4168,7 @@ long do_mmu_update(
                     if ( unlikely(pg_owner != pt_owner) )
                         break;
                     rc = mod_l4_entry(va, l4e_from_intpte(req.val), mfn,
-                                      cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
+                                      update_flags, v);
                     if ( !rc )
                         flush_linear_pt = true;
                     if ( !rc && pt_owner->arch.pv.xpti )
--- a/xen/arch/x86/pv/mm.h
+++ b/xen/arch/x86/pv/mm.h
@@ -62,17 +62,20 @@ static inline intpte_t paging_cmpxchg_gu
 #undef PTE_UPDATE_WITH_CMPXCHG
 #endif
 
+#define PTE_UPDATE_PRESERVE_AD  (1u << 0)
+#define PTE_UPDATE_NO_TRANSLATE (1u << 1)
+
 /*
  * How to write an entry to the guest pagetables.
  * Returns false for failure (pointer not valid), true for success.
  */
 static inline bool update_intpte(intpte_t *p, intpte_t old, intpte_t new,
-                                 mfn_t mfn, struct vcpu *v, bool preserve_ad)
+                                 mfn_t mfn, struct vcpu *v, unsigned int flags)
 {
     bool rv = true;
 
 #ifndef PTE_UPDATE_WITH_CMPXCHG
-    if ( !preserve_ad )
+    if ( !(flags & PTE_UPDATE_PRESERVE_AD) )
         paging_write_guest_entry(v, p, new, mfn);
     else
 #endif
@@ -81,7 +84,7 @@ static inline bool update_intpte(intpte_
         {
             intpte_t _new = new, t;
 
-            if ( preserve_ad )
+            if ( flags & PTE_UPDATE_PRESERVE_AD )
                 _new |= old & (_PAGE_ACCESSED | _PAGE_DIRTY);
 
             t = paging_cmpxchg_guest_entry(v, p, old, _new, mfn);
@@ -102,10 +105,10 @@ static inline bool update_intpte(intpte_
  * Macro that wraps the appropriate type-changes around update_intpte().
  * Arguments are: type, ptr, old, new, mfn, vcpu
  */
-#define UPDATE_ENTRY(_t,_p,_o,_n,_m,_v,_ad)                         \
+#define UPDATE_ENTRY(_t ,_p ,_o ,_n ,_m ,_v , fl)                   \
     update_intpte(&_t ## e_get_intpte(*(_p)),                       \
                   _t ## e_get_intpte(_o), _t ## e_get_intpte(_n),   \
-                  (_m), (_v), (_ad))
+                  _m, _v, fl)
 
 static always_inline l1_pgentry_t adjust_guest_l1e(l1_pgentry_t l1e,
                                                    const struct domain *d)