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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan