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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.01.2024 10:43, Oleksii wrote:
> On Wed, 2024-01-17 at 14:42 +0100, Jan Beulich wrote:
>> On 17.01.2024 12:37, Oleksii wrote:
>>>>>>
>>>>>>>> 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.
>>>>> Now it clear to me.
>>>>>
>>>>>
>>>>> It seems like BITOP_BITS_PER_WORD, BITOP_MASK, BITOP_WORD, and
>>>>> BITS_PER_BYTE are defined in {arm, ppc,
>>>>> riscv}/include/asm/bitops.h.
>>>>> I expected that any architecture planning to use asm-
>>>>> generic/bitops/bitops-bits.h would include it at the beginning
>>>>> of
>>>>> <arch>/include/asm/bitops.h, similar to what is done for RISC-
>>>>> V:
>>>>>    #ifndef _ASM_RISCV_BITOPS_H
>>>>>    #define _ASM_RISCV_BITOPS_H
>>>>>    
>>>>>    #include <asm/system.h>
>>>>>    
>>>>>    #include <asm-generic/bitops/bitops-bits.h>
>>>>>    ...
>>>>>
>>>>> But in this case, to allow architecture overrides macros, it is
>>>>> necessary to update asm-generic/bitops/bitops-bits.h:
>>>>>     #ifndef BITOP_BITS_PER_WORD
>>>>>     #define BITOP_BITS_PER_WORD     32
>>>>>     #endif
>>>>>    ...
>>>>> Therefore,  if an architecture needs to override something, it
>>>>> will
>>>>> add
>>>>> #define ... before #include <asm-generic/bitops/bitops-bits.h>.
>>>>>
>>>>> Does it make sense?
>>>>
>>>> Sure. But then the arch also needs to provide a corresponding
>>>> typedef
>>>> (and bitops-bits.h the fallback one), for use wherever you use
>>>> any of
>>>> those #define-s.
>>> Which one typedef is needed to provide?
>>> <asm-generic/bitops/bitops-bits.h> contains only macros.
>>
>> A new one, to replace where right now you use "unsigned int" and I
>> initially said you need to use "uint32_t" instead. With what you said
>> earlier, uint32_t won't work there (anymore).
> Wouldn't it be enough just to "#include <xen/types.h>" in headers where
> "uint32_t" is used?

No, my point wasn't to make uint32_t available. We need a _separate_
typedef which matches the #define-s. Otherwise, if an arch defines
BITOP_BITS_PER_WORD to, say, 64, this generic code would do the wrong
thing.

Jan