Re: [PATCH] xen: Introduce cmpxchg64() and guest_cmpxchg64()

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Aug 17, 2020 at 10:42:54AM +0100, Julien Grall wrote:
> Hi,
> 
> On 17/08/2020 10:24, Roger Pau Monné wrote:
> > On Sat, Aug 15, 2020 at 06:21:43PM +0100, Julien Grall wrote:
> > > From: Julien Grall <jgrall@xxxxxxxxxx>
> > > 
> > > The IOREQ code is using cmpxchg() with 64-bit value. At the moment, this
> > > is x86 code, but there is plan to make it common.
> > > 
> > > To cater 32-bit arch, introduce two new helpers to deal with 64-bit
> > > cmpxchg.
> > > 
> > > The Arm 32-bit implementation of cmpxchg64() is based on the __cmpxchg64
> > > in Linux v5.8 (arch/arm/include/asm/cmpxchg.h).
> > > 
> > > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> > > Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > > ---
> > > diff --git a/xen/include/asm-x86/guest_atomics.h 
> > > b/xen/include/asm-x86/guest_atomics.h
> > > index 029417c8ffc1..f4de9d3631ff 100644
> > > --- a/xen/include/asm-x86/guest_atomics.h
> > > +++ b/xen/include/asm-x86/guest_atomics.h
> > > @@ -20,6 +20,8 @@
> > >       ((void)(d), test_and_change_bit(nr, p))
> > >   #define guest_cmpxchg(d, ptr, o, n) ((void)(d), cmpxchg(ptr, o, n))
> > > +#define guest_cmpxchg64(d, ptr, o, n) ((void)(d), cmpxchg64(ptr, o, n))
> > > +
> > >   #endif /* _X86_GUEST_ATOMICS_H */
> > >   /*
> > > diff --git a/xen/include/asm-x86/x86_64/system.h 
> > > b/xen/include/asm-x86/x86_64/system.h
> > > index f471859c19cc..c1b16105e9f2 100644
> > > --- a/xen/include/asm-x86/x86_64/system.h
> > > +++ b/xen/include/asm-x86/x86_64/system.h
> > > @@ -5,6 +5,8 @@
> > >       ((__typeof__(*(ptr)))__cmpxchg((ptr),(unsigned long)(o),            
> > > \
> > >                                      (unsigned long)(n),sizeof(*(ptr))))
> > > +#define cmpxchg64(ptr, o, n) cmpxchg(ptr, o, n)
> > 
> > Why do you need to introduce an explicitly sized version of cmpxchg
> > for 64bit values?
> > 
> > There's no cmpxchg{8,16,32}, so I would expect cmpxchg64 to just be
> > handled by cmpxchg detecting the size of the parameter passed to the
> > function.
> That works quite well for 64-bit arches. However, for 32-bit, you would need
> to take some detour so 32-bit and 64-bit can cohabit (you cannot simply
> replace unsigned long with uint64_t).

Oh, I see. Switching __cmpxchg on Arm 32 to use unsigned long long or
uint64_t would be bad, as you would then need two registers to pass
the value to the function, or push it on the stack?

Maybe do something like:

#define cmpxchg(ptr,o,n) ({                                             \
        typeof(*(ptr)) tmp;                                             \
                                                                        \
        switch ( sizeof(*(ptr)) )                                       \
        {                                                               \
        case 8:                                                         \
                tmp = __cmpxchg_mb64((ptr), (uint64_t)(o),              \
                                (uint64_t)(n), sizeof(*(ptr))))         \
                break;                                                  \
        default:                                                        \
                tmp = __cmpxchg_mb((ptr), (unsigned long)(o),           \
                                (unsigned long)(n), sizeof(*(ptr))))    \
                break;                                                  \
        }                                                               \
        tmp;                                                            \
})

Roger.