Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 16 Aug 2022 14:26:44 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Peter Gonda <pgonda@google.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        marcorr@google.com, michael.roth@amd.com, thomas.lendacky@amd.com,
        joro@8bytes.org, mizhang@google.com, pbonzini@redhat.com,
        andrew.jones@linux.dev, vannapurve@google.com
Subject: Re: [V3 09/11] tools: Add atomic_test_and_set_bit()
Message-ID: <YvupJOJ38CWUDhze@google.com>
References: <20220810152033.946942-1-pgonda@google.com>
 <20220810152033.946942-10-pgonda@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220810152033.946942-10-pgonda@google.com>
Precedence: bulk

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@google.com>
> Suggested-by: Sean Christopherson <seanjc@google.com>
> ---
>  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@mail.gmail.com

> +
> +	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
>