Re: [PATCH v3 2/6] x86: extend update_intpte() to support atomic get-and-update

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/10/2026 6:47 pm, Kevin Lampis wrote:
> diff --git a/xen/arch/x86/pv/mm.h b/xen/arch/x86/pv/mm.h
> index bfee0feb7b21..363db35b5fe3 100644
> --- a/xen/arch/x86/pv/mm.h
> +++ b/xen/arch/x86/pv/mm.h
> @@ -64,16 +64,23 @@ static inline intpte_t paging_cmpxchg_guest_entry(
>  
>  #define PTE_UPDATE_PRESERVE_AD  (1u << 0)
>  #define PTE_UPDATE_NO_TRANSLATE (1u << 1)
> +#define PTE_UPDATE_SWAP         (1u << 2)
>  
>  /*
>   * How to write an entry to the guest pagetables.
> + * If the SWAP flag is set then does an atomic swap and returns the old PTE
> + * value, otherwise returns 0.
>   */
> -static inline void update_intpte(intpte_t *p, intpte_t old, intpte_t new,
> -                                 mfn_t mfn, struct vcpu *v, unsigned int 
> flags)
> +static inline intpte_t update_intpte(intpte_t *p, intpte_t old, intpte_t new,
> +                                     mfn_t mfn, struct vcpu *v,
> +                                     unsigned int flags)
>  {
>  #ifndef PTE_UPDATE_WITH_CMPXCHG
> -    if ( !(flags & PTE_UPDATE_PRESERVE_AD) )
> +    if ( !(flags & (PTE_UPDATE_PRESERVE_AD | PTE_UPDATE_SWAP)) )
> +    {
>          paging_write_guest_entry(v, p, new, mfn);
> +        return 0;
> +    }
>      else

Not for this patch, but a little bit of follow-on work could make this a
lot nicer.

We really want to be using XCHG rather than CMPXCHG for SWAP, and indeed
doing actually allow the optimiser room to remove the loop.

However, this would require introducing paging_xchg_guest_entry().  In
the shadow code, falling back to CMPXCHG is probably fine, and at least
that allows the non-shadow case to avoid the loop.

>  #endif
>      {
> @@ -95,17 +102,26 @@ static inline void update_intpte(intpte_t *p, intpte_t 
> old, intpte_t new,
>              old = t;
>          }
>      }
> +    return old;
>  }
>  
>  /*
>   * 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 , fl)                   \
> -    update_intpte(&_t ## e_get_intpte(*(_p)),                       \
> -                  _t ## e_get_intpte(_o), _t ## e_get_intpte(_n),   \
> +#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, fl)
>  
> +/*
> + * Macro that wraps UPDATE_ENTRY.
> + * This version converts the return type to l{1,2,3,4}_pgentry_t
> + * The return value must be used because of compiler flags.
> + */
> +#define UPDATE_ENTRY_SWAP(_t ,_p ,_o ,_n ,_m ,_v , fl)            \
> +    _t ## e_from_intpte(UPDATE_ENTRY(_t, _p, _o, _n, _m, _v, fl))
> +

I'm not sure we want this wrapper.  But see the next patch for the
discussion in the context of the caller.

~Andrew