[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] xen: Introduce cmpxchg64() and guest_cmpxchg64()
On 17/08/2020 12:50, Roger Pau Monné wrote:
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.
If you use (uint64_t)(unsigned long) in the 64-bit case, then you would
lose the top 32-bit. So cmpxchg() wouldn't work as expected.
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.
I don't think there is a way to solve this using just cast.
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.
FWIW, this is not very different to the existing approach. If one would
use cmpxchg() with 64-bit, then it would fail to compile.
Furthermore, there is no guarantee that a new 32-bit arch would have
64-bit atomic operations. For instance, not all 32-bit Arm processors
have 64-bit atomic operations. Although, the one supporting
virtualization will have them.
So I think we will always to rely on review and build testing to catch
error.
I don't think the union is so bad, but let's wait to see what others
think.
I am not concerned about the code itself but the assembly generated. I
don't want to increase the number memory access or instructions just for
the sake of trying to get cmpxchg() to work with 64-bit.
I will have to implement it and see the code generated.
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.
I dislike the name cmpxchg8b(). This is much easier to match the type
and the name with cmpxchg64().
I would be happy to rename cmpxchg16b() if the x86 folks would want it.
Cheers,
--
Julien Grall
|
