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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.01.2024 10:16, Oleksii wrote:
> On Thu, 2024-01-18 at 12:01 +0100, Jan Beulich wrote:
>> 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.
> Oh, yeah this is true.
> 
> We have to introduce in bitops-bits.h:
>    typedef uint_32t bitops_type; 

Perhaps e.g.

typedef uint32_t bitops_uint_t;

though.

Jan

> And then use it in function such as test_bit:
>    static inline int test_bit(int nr, const volatile void *addr)
>    {
>        const volatile bitops_type *p = addr;
>        return 1 & (p[BITOP_WORD(nr)] >> (nr & (BITOP_BITS_PER_WORD -
>    1)));
>    }
> 
> Thanks for clarification.
> 
> ~ Oleksii