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

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

On Mon, Aug 17, 2020 at 12:05:54PM +0100, Julien Grall wrote:
> Hi,
> 
> On 17/08/2020 11:33, Roger Pau Monné wrote:
> > 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?
> 
> We have only 4 registers (r0 - r4) available for the arguments. With 64-bit
> value, we will be using 2 registers, some will end up to be pushed on the
> stack.
> 
> This is assuming the compiler is not clever enough to see we are only using
> the bottom 32-bit with some cmpxchg.
> 
> > 
> > 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;                                                            \
> > })
> 
> 
> Unfortunately this can't compile if o and n are pointers because the
> compiler will complain about the cast to uint64_t.

Right, we would have to cast to unsigned long first and then to
uint64_t, which is not very nice.

> 
> We would also need a cast when assigning to tmp because tmp may not be a
> scalar type. This would lead to the same compiler issue.

Yes, we would have to do a bunch of casts.

> The only way I could see to make it work would be to use the same trick as
> we do for {read, write}_atomic() (see asm-arm/atomic.h). We are using union
> and void pointer to prevent explicit cast.

I'm mostly worried about common code having assumed that cmpxchg
does also handle 64bit sized parameters, and thus failing to use
cmpxchg64 when required. I assume this is not much of a deal as then
the Arm 32 build would fail, so it should be fairly easy to catch
those.

I don't think the union is so bad, but let's wait to see what others
think.

FWIW x86 already has a specific handler for 128bit values: cmpxchg16b.
Maybe it would be better to name this cmpxchg8b? Or rename the
existing one to cmpxchg128 for coherence.

Thanks, Roger.