Re: [PATCH v7 04/19] xen: introduce generic non-atomic test_*bit()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.04.2024 09:56, Oleksii wrote:
> On Fri, 2024-04-05 at 08:11 +0200, Jan Beulich wrote:
>> On 04.04.2024 18:24, Oleksii wrote:
>>> On Thu, 2024-04-04 at 18:12 +0200, Jan Beulich wrote:
>>>> On 04.04.2024 17:45, Oleksii wrote:
>>>>> On Thu, 2024-04-04 at 15:22 +0200, Jan Beulich wrote:
>>>>>> On 03.04.2024 12:19, Oleksii Kurochko wrote:
>>>>>>> --- a/xen/include/xen/bitops.h
>>>>>>> +++ b/xen/include/xen/bitops.h
>>>>>>> @@ -65,10 +65,164 @@ static inline int
>>>>>>> generic_flsl(unsigned
>>>>>>> long
>>>>>>> x)
>>>>>>>   * scope
>>>>>>>   */
>>>>>>>  
>>>>>>> +#define BITOP_BITS_PER_WORD 32
>>>>>>> +/* typedef uint32_t bitop_uint_t; */
>>>>>>> +#define bitop_uint_t uint32_t
>>>>>>
>>>>>> So no arch overrides permitted anymore at all?
>>>>> Not really, I agree that it is ugly, but I expected that arch
>>>>> will
>>>>> use
>>>>> undef to override.
>>>>
>>>> Which would be fine in principle, just that Misra wants us to
>>>> avoid
>>>> #undef-s
>>>> (iirc).
>>> Could you please give me a recommendation how to do that better?
>>>
>>> The reason why I put this defintions before inclusion of
>>> asm/bitops.h
>>> as RISC-V specific code uses these definitions inside it, so they
>>> should be defined before asm/bitops.h; other option is to define
>>> these
>>> definitions inside asm/bitops.h for each architecture.
>>
>> Earlier on you had it that other way already (in a different header,
>> but the principle is the same): Move the generic definitions
>> immediately
>> past inclusion of asm/bitops.h and frame them with #ifndef.
> It can be done in this way:
> xen/bitops.h:
>    ...
>    #include <asm/bitops.h>
>    
>    #ifndef BITOP_TYPE
>    #define BITOP_BITS_PER_WORD 32
>    /* typedef uint32_t bitop_uint_t; */
>    #define bitop_uint_t uint32_t
>    #endif
>    ...
>    
> But then RISC-V will fail as it is using bitop_uint_t inside
> asm/bitops.h.
> So, at least, for RISC-V it will be needed to add asm/bitops.h:
>    #define BITOP_BITS_PER_WORD 32
>    /* typedef uint32_t bitop_uint_t; */
>    #define bitop_uint_t uint32_t
>    
> 
> It seems to me that this breaks the idea of having these macro
> definitions generic, as RISC-V will redefine BITOP_BITS_PER_WORD and
> bitop_uint_t with the same values as the generic ones.

I don't follow. Right now patch 7 has

#undef BITOP_BITS_PER_WORD
#undef bitop_uint_t

#define BITOP_BITS_PER_WORD BITS_PER_LONG
#define bitop_uint_t unsigned long

You'd drop the #undef-s and keep the #define-s. You want to override them
both, after all.

A problem would arise for _another_ arch wanting to use these (default)
types in its asm/bitops.h. Which then could still be solved by having a
types-only header. Recall the discussion on the last summit of us meaning
to switch to such a model anyway (perhaps it being xen/types/bitops.h and
asm/types/bitops.h then), in a broader fashion? IOW for now you could use
the simple approach as long as no other arch needs the types in its
asm/bitops.h. Later we would introduce the types-only headers, thus
catering for possible future uses.

Jan