Re: [PATCH v8 02/17] xen: introduce generic non-atomic test_*bit()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.05.2024 10:16, Oleksii wrote:
> On Mon, 2024-05-06 at 08:33 +0200, Jan Beulich wrote:
>> On 03.05.2024 19:15, Oleksii wrote:
>>> On Thu, 2024-04-25 at 17:35 +0200, Jan Beulich wrote:
>>>>>   #include <asm/bitops.h>
>>>>>   
>>>>> +#ifndef arch_check_bitop_size
>>>>> +#define arch_check_bitop_size(addr)
>>>>
>>>> Can this really do nothing? Passing the address of an object
>>>> smaller
>>>> than
>>>> bitop_uint_t will read past the object in the generic__*_bit()
>>>> functions.
>>> It seems RISC-V isn' happy with the following generic definition:
>>>    extern void __bitop_bad_size(void);
>>>    
>>>    /* --------------------- Please tidy above here ----------------
>>> ----
>>>    - */
>>>    
>>>    #include <asm/bitops.h>
>>>    
>>>    #ifndef arch_check_bitop_size
>>>    
>>>    #define bitop_bad_size(addr) sizeof(*(addr)) <
>>> sizeof(bitop_uint_t)
>>>    
>>>    #define arch_check_bitop_size(addr) \
>>>        if ( bitop_bad_size(addr) ) __bitop_bad_size();
>>>    
>>>    #endif /* arch_check_bitop_size */
>>
>> I'm afraid you've re-discovered something that was also found during
>> the
>> original Arm porting effort. As nice and logical as it would (seem
>> to) be
>> to have bitop_uint_t match machine word size, there are places ...
>>
>>> The following errors occurs. bitop_uint_t for RISC-V is defined as
>>> unsigned long for now:
>>
>> ... where we assume such operations can be done on 32-bit quantities.
> Based on RISC-V spec machine word is 32-bit, so then I can just drop
> re-definition of bitop_uint_t in riscv/asm/types.h and use the
> definition of bitop_uint_t in xen/types.h.
> Also it will be needed to update __AMO() macros in <riscv>/asm/bitops.h
> in the following way:
>    #if BITOP_BITS_PER_WORD == 64
>    #define __AMO(op)   "amo" #op ".d"
>    #elif BITOP_BITS_PER_WORD == 32
>    #define __AMO(op)   "amo" #op ".w"
>    #else
>    #error "Unexpected BITS_PER_LONG"
>    #endif
> Note: BITS_PER_LONG was changed to BITOP_BITS_PER_WORD !
> 
> Only one question remains for me. Given that there are some operations 
> whichcan be performed on 32-bit quantities, it seems to me that bitop_uint_t
> can only be uint32_t.
> Am I correct? If yes, do we need to have ability to redefine
> bitop_uint_t and BITOP_BITS_PER_WORD in xen/types.h:
>    #ifndef BITOP_TYPE
>    #define BITOP_BITS_PER_WORD 32
>    
>    typedef uint32_t bitop_uint_t;
>    #endif

Probably not, right now. Hence me having said "As nice and logical as it
would (seem to) be" in the earlier reply.

Jan