On 27.06.2024 14:01, oleksii.kuroc...@gmail.com wrote:
> On Thu, 2024-06-27 at 12:10 +0200, Jan Beulich wrote:
>> On 27.06.2024 11:58, oleksii.kuroc...@gmail.com wrote:
>>> On Thu, 2024-06-27 at 09:59 +0200, Jan Beulich wrote:
>>>> On 26.06.2024 19:27, oleksii.kuroc...@gmail.com 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
This would be my preference, but please also consult with Andrew.
> 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.
This can be looked at different ways. The splitting into NOT+AND was a
rather late change.
Jan
