Re: [PATCH v7 07/19] xen/riscv: introduce bitops.h

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2024-04-04 at 16:47 +0200, Jan Beulich wrote:
> On 03.04.2024 12:20, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/bitops.h
> > @@ -0,0 +1,146 @@
> > +/* 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>
> > +
> > +#undef BITOP_BITS_PER_WORD
> > +#undef bitop_uint_t
> > +
> > +#define BITOP_BITS_PER_WORD BITS_PER_LONG
> > +#define bitop_uint_t unsigned long
> > +
> > +#if BITS_PER_LONG == 64
> > +#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 __set_bit(n, p)      set_bit(n, p)
> > +#define __clear_bit(n, p)    clear_bit(n, p)
> 
> First without comment and then ...
> 
> > +/* Based on linux/arch/include/asm/bitops.h */
> > +
> > +/*
> > + * Non-atomic bit manipulation.
> > + *
> > + * Implemented using atomics to be interrupt safe. Could
> > alternatively
> > + * implement with local interrupt masking.
> > + */
> > +#define __set_bit(n, p)      set_bit(n, p)
> > +#define __clear_bit(n, p)    clear_bit(n, p)
> 
> ... with one?
Hmm, it seems like rebasing issue with autoconflict resolving. It
should be only definitions with the comment.

> 
> > +/* Based on linux/arch/include/asm/bitops.h */
> 
> Does this really need repeating?
> 
> > +#define test_and_op_bit_ord(op, mod, nr, addr, ord)     \
> > +({                                                      \
> > +    unsigned long res, mask;                            \
> 
> bitop_uint_t?
> 
> > +    mask = BITOP_MASK(nr);                              \
> > +    asm volatile (                                       \
> 
> Nit: One too many padding blanks.
> 
> > +        __AMO(op) #ord " %0, %2, %1"                    \
> > +        : "=r" (res), "+A" (addr[BITOP_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[BITOP_WORD(nr)])           \
> > +        : "r" (mod(BITOP_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))
> > +
> > +/**
> > + * test_and_set_bit - Set a bit and return its old value
> > + * @nr: Bit to set
> > + * @addr: Address to count from
> > + */
> > +static inline int test_and_set_bit(int nr, volatile void *p)
> 
> In patch 4 you switched to bool. Any reason you didn't here, too?
Sure, it should return bool here too and probably some other functions
below.

> 
> > +{
> > +    volatile bitop_uint_t *addr = p;
> > +
> > +    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
> > + */
> > +static inline int test_and_clear_bit(int nr, volatile void *p)
> > +{
> > +    volatile bitop_uint_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 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 bitop_uint_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
> > + */
> > +static inline void clear_bit(int nr, volatile void *p)
> > +{
> > +    volatile bitop_uint_t *addr = p;
> > +
> > +    op_bit(and, NOT, nr, addr);
> > +}
> > +
> > +/**
> > + * test_and_change_bit - Toggle (change) a bit and return its old
> > value
> > + * @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)
> 
> unsigned long?
It should be volatile void *. Something that was copied from Linux
kernel and I missed to change.

~ Oleksii

> 
> > +{
> > +   return test_and_op_bit(xor, NOP, nr, addr);
> > +}
> 
> Perhaps better to move up a little, next to the other test_and-s?
> 
> Also - nit: Hard tab used for indentation.
> 
> > +#undef test_and_op_bit
> > +#undef __op_bit
> 
> This no longer has any effect in this shape.
> 
> Jan