On Wed, Aug 10, 2022, Peter Gonda wrote:
> atomic_test_and_set_bit() allows for atomic bitmap usage from KVM
> selftests.
>
> Signed-off-by: Peter Gonda <pgonda@xxxxxxxxxx>
> Suggested-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
> tools/arch/x86/include/asm/atomic.h | 7 +++++++
> tools/include/asm-generic/atomic-gcc.h | 15 +++++++++++++++
> 2 files changed, 22 insertions(+)
>
> diff --git a/tools/arch/x86/include/asm/atomic.h b/tools/arch/x86/include/asm/atomic.h
> index 1f5e26aae9fc..01cc27ec4520 100644
> --- a/tools/arch/x86/include/asm/atomic.h
> +++ b/tools/arch/x86/include/asm/atomic.h
> @@ -8,6 +8,7 @@
>
> #define LOCK_PREFIX "\n\tlock; "
>
> +#include <asm/asm.h>
> #include <asm/cmpxchg.h>
>
> /*
> @@ -70,4 +71,10 @@ static __always_inline int atomic_cmpxchg(atomic_t *v, int old, int new)
> return cmpxchg(&v->counter, old, new);
> }
>
> +static inline int atomic_test_and_set_bit(long nr, unsigned long *addr)
> +{
> + GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(bts), *addr, "Ir", nr, "%0", "c");
> +
Unnecessary newline.
> +}
> +
> #endif /* _TOOLS_LINUX_ASM_X86_ATOMIC_H */
> diff --git a/tools/include/asm-generic/atomic-gcc.h b/tools/include/asm-generic/atomic-gcc.h
> index 4c1966f7c77a..8d9b2d1768bf 100644
> --- a/tools/include/asm-generic/atomic-gcc.h
> +++ b/tools/include/asm-generic/atomic-gcc.h
> @@ -4,6 +4,7 @@
>
> #include <linux/compiler.h>
> #include <linux/types.h>
> +#include <linux/bitops.h>
>
> /*
> * Atomic operations that C can't guarantee us. Useful for
> @@ -69,4 +70,18 @@ static inline int atomic_cmpxchg(atomic_t *v, int oldval, int newval)
> return cmpxchg(&(v)->counter, oldval, newval);
> }
> +static inline int atomic_test_and_set_bit(long nr, unsigned long *addr)
> +{
> + long old, val;
> + unsigned long mask = BIT_MASK(nr);
> +
> + addr += BIT_WORD(nr);
> + val = READ_ONCE(*addr);
> + if (val & mask)
> + return 1;
Probably should drop the READ_ONCE() shortcut to stay consistent with the kernel
proper.
https://lore.kernel.org/all/CAHk-=wgSNiT5qJX53RHtWECsUiFq6d6VWYNAvu71ViOEan07yw@xxxxxxxxxxxxxx
> +
> + old = cmpxchg(addr, val, val & mask);
This is wrong on two fronts: 1) cmpxchg() writes the entire new value, and 2) it
fails if the old value is not an exact match with what's in memory. Bug #1 means
that setting a bit will clear all existing bits, and bug #2 means that this will
fail to set the bit if another atomic_test_and_set_bit() sneaks in between reading
into "val" and doing the cmpxchg.
And obviously dropping the READ_ONCE() above makes cmpxchg impossible (not a
coincidence, it's simply the wrong operation to use).
I believe what we want is:
unsigned long mask = BIT_MASK(nr);
long old;
old = __sync_fetch_and_or(addr, mask);
return !!(old & mask);
> + return !!(old & mask);
> +}
> +
> #endif /* __TOOLS_ASM_GENERIC_ATOMIC_H */
> --
> 2.37.1.559.g78731f0fdb-goog
>
