Re: [PATCH v4 09/30] xen/riscv: introduce bitops.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.02.2024 16:32, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/bitops.h
> @@ -0,0 +1,164 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2012 Regents of the University of California */
> +
> +#ifndef _ASM_RISCV_BITOPS_H
> +#define _ASM_RISCV_BITOPS_H
> +
> +#include <asm/system.h>
> +
> +#include <asm-generic/bitops/bitops-bits.h>

Especially with ...

> +/* Based on linux/arch/include/linux/bits.h */
> +
> +#define BIT_MASK(nr)        (1UL << ((nr) % BITS_PER_LONG))
> +#define BIT_WORD(nr)        ((nr) / BITS_PER_LONG)

... these it's not entirely obvious why bitops-bits.h would be needed
here.

> +#define __set_bit(n,p)      set_bit(n,p)
> +#define __clear_bit(n,p)    clear_bit(n,p)

Nit (as before?): Missing blanks after commas.

> +/* Based on linux/arch/include/asm/bitops.h */
> +
> +#if ( BITS_PER_LONG == 64 )

Imo the parentheses here make things only harder to read.

> +#define __AMO(op)   "amo" #op ".d"
> +#elif ( BITS_PER_LONG == 32 )
> +#define __AMO(op)   "amo" #op ".w"
> +#else
> +#error "Unexpected BITS_PER_LONG"
> +#endif
> +
> +#define __test_and_op_bit_ord(op, mod, nr, addr, ord)   \

The revision log says __test_and_* were renamed. Same anomaly for
__test_and_op_bit() then.

> +({                                                      \
> +    unsigned long __res, __mask;                        \

Leftover leading underscores?

> +    __mask = BIT_MASK(nr);                              \
> +    __asm__ __volatile__ (                              \
> +        __AMO(op) #ord " %0, %2, %1"                    \
> +        : "=r" (__res), "+A" (addr[BIT_WORD(nr)])       \
> +        : "r" (mod(__mask))                             \
> +        : "memory");                                    \
> +    ((__res & __mask) != 0);                            \
> +})
> +
> +#define __op_bit_ord(op, mod, nr, addr, ord)    \
> +    __asm__ __volatile__ (                      \
> +        __AMO(op) #ord " zero, %1, %0"          \
> +        : "+A" (addr[BIT_WORD(nr)])             \
> +        : "r" (mod(BIT_MASK(nr)))               \
> +        : "memory");
> +
> +#define __test_and_op_bit(op, mod, nr, addr)    \
> +    __test_and_op_bit_ord(op, mod, nr, addr, .aqrl)
> +#define __op_bit(op, mod, nr, addr) \
> +    __op_bit_ord(op, mod, nr, addr, )
> +
> +/* Bitmask modifiers */
> +#define __NOP(x)    (x)
> +#define __NOT(x)    (~(x))

Here the (double) leading underscores are truly worrying: Simple
names like this aren't impossible to be assigned meaninb by a compiler.

> +/**
> + * __test_and_set_bit - Set a bit and return its old value
> + * @nr: Bit to set
> + * @addr: Address to count from
> + *
> + * This operation may be reordered on other architectures than x86.
> + */
> +static inline int test_and_set_bit(int nr, volatile void *p)
> +{
> +    volatile uint32_t *addr = p;

With BIT_WORD() / BIT_MASK() being long-based, is the use of uint32_t
here actually correct?

> +    return __test_and_op_bit(or, __NOP, nr, addr);
> +}
> +
> +/**
> + * __test_and_clear_bit - Clear a bit and return its old value
> + * @nr: Bit to clear
> + * @addr: Address to count from
> + *
> + * This operation can be reordered on other architectures other than x86.

Nit: double "other" (and I think it's the 1st one that wants dropping,
not - as the earlier comment suggests - the 2nd one). Question is: Are
the comments correct? Both resolve to something which is (also) at
least a compiler barrier. Same concern also applies further down, to
at least set_bit() and clear_bit().

> + */
> +static inline int test_and_clear_bit(int nr, volatile void *p)
> +{
> +    volatile uint32_t *addr = p;
> +
> +    return __test_and_op_bit(and, __NOT, nr, addr);
> +}
> +
> +/**
> + * set_bit - Atomically set a bit in memory
> + * @nr: the bit to set
> + * @addr: the address to start counting from
> + *
> + * Note: there are no guarantees that this function will not be reordered
> + * on non x86 architectures, so if you are writing portable code,
> + * make sure not to rely on its reordering guarantees.
> + *
> + * Note that @nr may be almost arbitrarily large; this function is not
> + * restricted to acting on a single-word quantity.
> + */
> +static inline void set_bit(int nr, volatile void *p)
> +{
> +    volatile uint32_t *addr = p;
> +
> +    __op_bit(or, __NOP, nr, addr);
> +}
> +
> +/**
> + * clear_bit - Clears a bit in memory
> + * @nr: Bit to clear
> + * @addr: Address to start counting from
> + *
> + * Note: there are no guarantees that this function will not be reordered
> + * on non x86 architectures, so if you are writing portable code,
> + * make sure not to rely on its reordering guarantees.
> + */
> +static inline void clear_bit(int nr, volatile void *p)
> +{
> +    volatile uint32_t *addr = p;
> +
> +    __op_bit(and, __NOT, nr, addr);
> +}
> +
> +/**
> + * test_and_change_bit - Change a bit and return its old value

How come this one's different? I notice the comments are the same (and
hence as confusing) in Linux; are you sure they're applicable there?

> + * @nr: Bit to change
> + * @addr: Address to count from
> + *
> + * This operation is atomic and cannot be reordered.
> + * It also implies a memory barrier.
> + */
> +static inline int test_and_change_bit(int nr, volatile unsigned long *addr)
> +{
> +     return __test_and_op_bit(xor, __NOP, nr, addr);
> +}
> +
> +#undef __test_and_op_bit
> +#undef __op_bit
> +#undef __NOP
> +#undef __NOT
> +#undef __AMO
> +
> +#include <asm-generic/bitops/generic-non-atomic.h>
> +
> +#define __test_and_set_bit generic___test_and_set_bit
> +#define __test_and_clear_bit generic___test_and_clear_bit
> +#define __test_and_change_bit generic___test_and_change_bit
> +
> +#include <asm-generic/bitops/fls.h>
> +#include <asm-generic/bitops/flsl.h>
> +#include <asm-generic/bitops/__ffs.h>
> +#include <asm-generic/bitops/ffs.h>
> +#include <asm-generic/bitops/ffsl.h>
> +#include <asm-generic/bitops/ffz.h>
> +#include <asm-generic/bitops/find-first-set-bit.h>
> +#include <asm-generic/bitops/hweight.h>
> +#include <asm-generic/bitops/test-bit.h>

To be honest there's too much stuff being added here to asm-generic/,
all in one go. I'll see about commenting on the remaining parts here,
but I'd like to ask that you seriously consider splitting.

Jan