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

[Julien Grall <julien@xen.org>]

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@amazon.com>
>>>>
>>>> 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@amazon.com>
>>>> Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>> ---
>>>> 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.

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.

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.

But I am not sure whether the effort is really worth it.

Cheers,

-- 
Julien Grall