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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.04.2024 13:56, Oleksii wrote:
> On Fri, 2024-04-05 at 13:53 +0200, Oleksii wrote:
>> On Fri, 2024-04-05 at 10:05 +0200, Jan Beulich wrote:
>>> 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.
>> This problem arise now for Arm and PPC which use BITOP_BITS_PER_WORD
>> inside it. Then it is needed to define BITOP_BITS_PER_WORD=32 in
>> asm/bitops.h for Arm and PPC. If it is okay, then I will happy to
>> follow this approach.
>>
>>>  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.
>> Do we really need asm/types/bitops.h? Can't we just do the following
>> in
>> asm/bitops.h:
>>   #ifndef BITOP_TYPE
>>   #include <xen/types/bitops.h>
>>   #endif

This might do, yes.

> Or as an options just update <xen/types.h> with after inclusion of
> <asm/types.h>:
>    #ifndef BITOP_TYPE
>       #define BITOP_BITS_PER_WORD 32
>       /* typedef uint32_t bitop_uint_t; */
>       #define bitop_uint_t uint32_t
>    #endif
>    
> And then just include <xen/types.h> to <<xen/bitops.h>.

That's a (transient) option as well, I guess.

Jan