Re: [PATCH v3 10/34] xen/riscv: introduce bitops.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.01.2024 14:06, Oleksii wrote:
> On Mon, 2024-01-15 at 17:44 +0100, Jan Beulich wrote:
>> On 22.12.2023 16:12, Oleksii Kurochko wrote:
>>> +#define test_and_set_bit   __test_and_set_bit
>>> +#define test_and_clear_bit __test_and_clear_bit
>>
>> I realize test-and-change have no present users, despite being made
>> available by Arm and x86, but I think they would better be provided
>> right away, rather than someone introducing a use then needing to
>> fiddle with RISC-V (and apparently also PPC) code.
> Sure, it makes sense. I'll add test-and-change too.
> 
>> I'm also puzzled by this aliasing: Aren't there cheaper non-atomic
>> insn forms that could be used for the double-underscore-prefixed
>> variants?
> It will be cheaper, but I assume that this API should be safe in the
> case of SMP where different CPUs can access the same variable or
> similar cases with simultaneous access to the variable.

Of course, that's what test_and_...() are for. __test_and_...() are
for cases where there's no concurrency, when hence the cheaper forms
can be used. Thus my asking about the aliasing done above.

>>> +#if BITS_PER_LONG == 64
>>> +    if ((word & 0xffffffff) == 0) {
>>> +        num += 32;
>>> +        word >>= 32;
>>> +    }
>>
>> You're ending up with neither Xen nor Linux style this way. May I
>> suggest to settle on either?
> 
> Will it fine to rework header from Linux to Xen style? Does it make
> sense?
> I think this file can be reworked to Xen style as I don't expect that
> it will be changed since it will be merged.

You may keep Linux style or fully switch to Xen style - which one is
largely up to you. All I'm asking is to avoid introducing further
mixed-style source files.

>>> --- /dev/null
>>> +++ b/xen/include/asm-generic/bitops/bitops-bits.h
>>> @@ -0,0 +1,10 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +#ifndef _ASM_GENERIC_BITOPS_BITS_H_
>>> +#define _ASM_GENERIC_BITOPS_BITS_H_
>>> +
>>> +#define BITOP_BITS_PER_WORD     32
>>> +#define BITOP_MASK(nr)          (1UL << ((nr) %
>>> BITOP_BITS_PER_WORD))
>>
>> Why 1UL and not just 1U, when bits per word is 32?
> There is no specific reason, should 1U. ( I originally used
> BITOPS_BITS_PER_LONG ) and with introduction of asm-generic bitops
> decided to follow what other archs provide.
> 
> Regarding to the second part of the question, I don't understand it
> fully. Considering BITOP_BIT_PER_WORD definition for other archs ( ARM
> and PPC ) it is expected that word is 32 bits.

The 2nd part was explaining why I'm asking. It wasn't another question.

>>> --- /dev/null
>>> +++ b/xen/include/asm-generic/bitops/test-bit.h
>>> @@ -0,0 +1,16 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +#ifndef _ASM_GENERIC_BITOPS_TESTBIT_H_
>>> +#define _ASM_GENERIC_BITOPS_TESTBIT_H_
>>> +
>>> +/**
>>> + * test_bit - Determine whether a bit is set
>>> + * @nr: bit number to test
>>> + * @addr: Address to start counting from
>>> + */
>>> +static inline int test_bit(int nr, const volatile void *addr)
>>> +{
>>> +    const volatile unsigned int *p = addr;
>>
>> With BITOP_BITS_PER_WORD I think you really mean uint32_t here.
> Isn't it the same: 'unsigned int' and 'uint32_t'?

No, or else there wouldn't have been a need to introduce uint<N>_t (and
others) in C99. It just so happens that right now all architectures Xen
can be built for have sizeof(int) == 4 and CHAR_BITS == 8. In an arch-
specific header I would see this as less of an issue, but in a generic
header we'd better avoid encoding wrong assumptions. The one assumption
we generally make is that sizeof(int) >= 4 and CHAR_BITS >= 8 (albeit I
bet really in various places we assume CHAR_BITS == 8).

>> Also you want to make sure asm-generic/bitops/bitops-bits.h is
>> really in use here, or else an arch overriding / not using that
>> header may end up screwed.
> I am not really understand what do you mean. Could you please explain a
> little bit more.

Whichever type you use here, it needs to be in sync with
BITOP_BITS_PER_WORD. Hence you want to include the _local_ bitops-bits.h
here, such that in case of an inconsistent override by an arch the
compiler would complain about the two differring #define-s. (IOW an
arch overriding BITOP_BITS_PER_WORD cannot re-use this header as-is.)

The same may, btw, be true for others of the new headers you add - the
same #include would therefore be needed there as well.

Jan