Re: [PATCH v2 3/7] x86: extend update_intpte() to support atomic get-and-update

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 10/09/2026 9:31 pm, Kevin Lampis wrote:
> diff --git a/xen/arch/x86/pv/mm.h b/xen/arch/x86/pv/mm.h
> index bfee0feb7b21..f2bb26aef5b3 100644
> --- a/xen/arch/x86/pv/mm.h
> +++ b/xen/arch/x86/pv/mm.h
> @@ -64,15 +64,18 @@ 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.
> + * Returns the old PTE value.
>   */
> -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);
>      else
>  #endif
> @@ -95,15 +98,16 @@ static inline void update_intpte(intpte_t *p, intpte_t 
> old, intpte_t new,
>              old = t;
>          }
>      }
> +    return old;
>  }

This seems unwise.  I think you want a return 0 in the
paging_write_guest_entry() case because returning the input cannot
possibly lead to anything good.

In turn, the comment needs adjusting.

>  
>  /*
>   * 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)

This delta is only moving the \'s I think.

However, you want to wrap the expression in _t ## e_from_intpte(...) to
hand back the type safe version.  In turn, this lets you remove the
'.l1' part of the assignments in the following patch.

~Andrew