Re: [kvm-unit-tests PATCH 1/8] s390x: lib: Extend bitops

[Janosch Frank <frankja@linux.ibm.com>]

On 8/13/21 10:32 AM, Claudio Imbrenda wrote:
> On Fri, 13 Aug 2021 07:36:08 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> Bit setting and clearing is never bad to have.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  lib/s390x/asm/bitops.h | 102
>> +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 102
>> insertions(+)
>>
>> diff --git a/lib/s390x/asm/bitops.h b/lib/s390x/asm/bitops.h
>> index 792881ec..f5612855 100644
>> --- a/lib/s390x/asm/bitops.h
>> +++ b/lib/s390x/asm/bitops.h
>> @@ -17,6 +17,78 @@
>>  
>>  #define BITS_PER_LONG	64
>>  
>> +static inline unsigned long *bitops_word(unsigned long nr,
>> +					 const volatile unsigned
>> long *ptr) +{
>> +	unsigned long addr;
>> +
>> +	addr = (unsigned long)ptr + ((nr ^ (nr & (BITS_PER_LONG -
>> 1))) >> 3);
>> +	return (unsigned long *)addr;
> 
> why not just 
> 
> return ptr + (nr / BITS_PER_LONG);
> 
>> +}
>> +
>> +static inline unsigned long bitops_mask(unsigned long nr)
>> +{
>> +	return 1UL << (nr & (BITS_PER_LONG - 1));
>> +}
>> +
>> +static inline uint64_t laog(volatile unsigned long *ptr, uint64_t
>> mask) +{
>> +	uint64_t old;
>> +
>> +	/* load and or 64bit concurrent and interlocked */
>> +	asm volatile(
>> +		"	laog	%[old],%[mask],%[ptr]\n"
>> +		: [old] "=d" (old), [ptr] "+Q" (*ptr)
>> +		: [mask] "d" (mask)
>> +		: "memory", "cc" );
>> +	return old;
>> +}
> 
> do we really need the artillery (asm) here?
> is there a reason why we can't do this in C?

Those are the interlocked/atomic instructions and even though we don't
exactly need them right now I wanted to add them for completeness.
We might be able to achieve the same via compiler functionality but this
is not my expertise. Maybe Thomas or David have a few pointers for me?

> 
>> +static inline uint64_t lang(volatile unsigned long *ptr, uint64_t
>> mask) +{
>> +	uint64_t old;
>> +
>> +	/* load and and 64bit concurrent and interlocked */
>> +	asm volatile(
>> +		"	lang	%[old],%[mask],%[ptr]\n"
>> +		: [old] "=d" (old), [ptr] "+Q" (*ptr)
>> +		: [mask] "d" (mask)
>> +		: "memory", "cc" );
>> +	return old;
>> +}
> 
> (same here as above)
> 
>> +
>> +static inline void set_bit(unsigned long nr,
>> +			   const volatile unsigned long *ptr)
>> +{
>> +	uint64_t mask = bitops_mask(nr);
>> +	uint64_t *addr = bitops_word(nr, ptr);
>> +
>> +	laog(addr, mask);
>> +}
>> +
>> +static inline void set_bit_inv(unsigned long nr,
>> +			       const volatile unsigned long *ptr)
>> +{
>> +	return set_bit(nr ^ (BITS_PER_LONG - 1), ptr);
>> +}
>> +
>> +static inline void clear_bit(unsigned long nr,
>> +			     const volatile unsigned long *ptr)
>> +{
>> +	uint64_t mask = bitops_mask(nr);
>> +	uint64_t *addr = bitops_word(nr, ptr);
>> +
>> +	lang(addr, ~mask);
>> +}
>> +
>> +static inline void clear_bit_inv(unsigned long nr,
>> +				 const volatile unsigned long *ptr)
>> +{
>> +	return clear_bit(nr ^ (BITS_PER_LONG - 1), ptr);
>> +}
>> +
>> +/* non-atomic bit manipulation functions */
>> +
>>  static inline bool test_bit(unsigned long nr,
>>  			    const volatile unsigned long *ptr)
>>  {
>> @@ -33,4 +105,34 @@ static inline bool test_bit_inv(unsigned long nr,
>>  	return test_bit(nr ^ (BITS_PER_LONG - 1), ptr);
>>  }
>>  
>> +static inline void __set_bit(unsigned long nr,
>> +			     const volatile unsigned long *ptr)
>> +{
>> +	uint64_t mask = bitops_mask(nr);
>> +	uint64_t *addr = bitops_word(nr, ptr);
>> +
>> +	*addr |= mask;
>> +}
>> +
>> +static inline void __set_bit_inv(unsigned long nr,
>> +				 const volatile unsigned long *ptr)
>> +{
>> +	return __set_bit(nr ^ (BITS_PER_LONG - 1), ptr);
>> +}
>> +
>> +static inline void __clear_bit(unsigned long nr,
>> +			       const volatile unsigned long *ptr)
>> +{
>> +	uint64_t mask = bitops_mask(nr);
>> +	uint64_t *addr = bitops_word(nr, ptr);
>> +
>> +	*addr &= ~mask;
>> +}
>> +
>> +static inline void __clear_bit_inv(unsigned long nr,
>> +				   const volatile unsigned long *ptr)
>> +{
>> +	return __clear_bit(nr ^ (BITS_PER_LONG - 1), ptr);
>> +}
>> +
>>  #endif
>