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

[oleksii.kurochko@xxxxxxxxx]

On Thu, 2024-06-27 at 12:10 +0200, Jan Beulich wrote:
> On 27.06.2024 11:58, oleksii.kurochko@xxxxxxxxx wrote:
> > 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.
> 
> I'd be fine that way, but then you don't need to break up ANDN into
> NOT
> and AND. It is my understanding that Andrew has concerns here, even
> if
> - iirc - it was him to originally suggest to build upon that
> extension
> being available. If these concerns are solely about being able to
> build
> with Zbb-unaware tool chains, then what to do about the build issues
> there has already been said.
Not much we can do except probably using .insn, as you suggested for
the "pause" instruction in cpu_relax(), for every instruction ( at the
moment it is only ANDB but who knows which instruction will be used in
the future ) from the Zbb extension.

But then we will need to do the same for each possible extension we are
going to use, as there is still a small chance that we might encounter
an extension-unaware toolchain.

I am a little bit confused about what we should do.

In my opinion, the best approach at the moment is to use .insn for the
ANDN and PAUSE instructions and add an explanation to
docs/misc/riscv/booting.txt or create a separate document where such
issues are documented (I am not sure that README is the correct place
for this).

I am also okay to go with ANDN break up int NOT and AND, but it is
needed to come up  with concept which instruction/extenstion should be
used and how consistently to deal with such situations.

Furthermore, I don't think these changes should block the merging (
doesn't matter in 4.19 or in 4.20 Xen release ) of PATCHes v13 01-07 of
this patch series.

~ Oleksii