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
