Re: [PATCH v13 02/10] xen/riscv: introduce bitops.h

[oleksii.kurochko@xxxxxxxxx]

On Thu, 2024-06-27 at 09:59 +0200, Jan Beulich wrote:
> On 26.06.2024 19:27, oleksii.kurochko@xxxxxxxxx wrote:
> > On Wed, 2024-06-26 at 10:31 +0200, Jan Beulich wrote:
> > > On 25.06.2024 15:51, Oleksii Kurochko wrote:
> > > > --- /dev/null
> > > > +++ b/xen/arch/riscv/include/asm/bitops.h
> > > > @@ -0,0 +1,137 @@
> > > > +/* 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>
> > > > +
> > > > +#if BITOP_BITS_PER_WORD == 64
> > > > +#define __AMO(op)   "amo" #op ".d"
> > > > +#elif BITOP_BITS_PER_WORD == 32
> > > > +#define __AMO(op)   "amo" #op ".w"
> > > > +#else
> > > > +#error "Unexpected BITOP_BITS_PER_WORD"
> > > > +#endif
> > > > +
> > > > +/* 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)
> > > > +
> > > > +#define test_and_op_bit_ord(op, mod, nr, addr, ord)     \
> > > > +({                                                      \
> > > > +    bitop_uint_t res, mask;                             \
> > > > +    mask = BITOP_MASK(nr);                              \
> > > > +    asm volatile (                                      \
> > > > +        __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))
> > > 
> > > Since elsewhere you said we would use Zbb in bitops, I wanted to
> > > come
> > > back
> > > on that: Up to here all we use is AMO.
> > > 
> > > And further down there's no asm() anymore. What were you
> > > referring
> > > to?
> > RISC-V doesn't have a CLZ instruction in the base
> > ISA.  As a consequence, __builtin_ffs() emits a library call to
> > ffs()
> > on GCC,
> 
> Oh, so we'd need to implement that libgcc function, along the lines
> of
> Arm32 implementing quite a few of them to support shifts on 64-bit
> quantities as well as division and modulo.
Why we can't just live with Zbb extension? Zbb extension is presented
on every platform I have in access with hypervisor extension support.

~ Oleksii

> 
> Jan
> 
> > or a de Bruijn sequence on Clang.
> > 
> > The optional Zbb extension adds a CLZ instruction, after which
> > __builtin_ffs() emits a very simple sequence.
> > 
> > ~ Oleksii
>