Re: [PATCH v3 13/34] xen/riscv: introduce cmpxchg.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.01.2024 13:18, Oleksii wrote:
> On Tue, 2024-01-23 at 11:28 +0100, Jan Beulich wrote:
>> On 23.01.2024 11:15, Oleksii wrote:
>>> On Mon, 2024-01-22 at 17:27 +0100, Jan Beulich wrote:
>>>> On 22.12.2023 16:12, Oleksii Kurochko wrote:
>>>>> +static inline unsigned long __xchg(volatile void *ptr,
>>>>> unsigned
>>>>> long x, int size)
>>>>> +{
>>>>> +    switch (size) {
>>>>> +    case 1:
>>>>> +        return __cmpxchg_case_1(ptr, (uint32_t)-1, x);
>>>>> +    case 2:
>>>>> +        return __cmpxchg_case_2(ptr, (uint32_t)-1, x);
>>>>
>>>> How are these going to work? You'll compare against ~0, and if
>>>> the
>>>> value
>>>> in memory isn't ~0, memory won't be updated; you will only
>>>> (correctly)
>>>> return the value found in memory.
>>>>
>>>> Or wait - looking at __cmpxchg_case_{1,2}() far further down, you
>>>> ignore
>>>> "old" there. Which apparently means they'll work for the use
>>>> here,
>>>> but
>>>> not for the use in __cmpxchg().
>>> Yes, the trick is that old is ignored and is read in
>>> __emulate_cmpxchg_case1_2() before __cmpxchg_case_4 is called:
>>>     do
>>> {                                                              
>>>         read_val =
>>> read_func(aligned_ptr);                            
>>>         swapped_new = read_val &
>>> ~mask;                               
>>>         swapped_new |=
>>> masked_new;                                    
>>>         ret = cmpxchg_func(aligned_ptr, read_val,
>>> swapped_new);       
>>>     } while ( ret != read_val
>>> );                                      
>>> read_val it is 'old'.
>>>
>>> But now I am not 100% sure that it is correct for __cmpxchg...
>>
>> It just can't be correct - you can't ignore "old" there. I think you
>> want simple cmpxchg primitives, which xchg then uses in a loop (while
>> cmpxchg uses them plainly).
> But xchg doesn't require 'old' value, so it should be ignored in some
> way by cmpxchg.

Well, no. If you have only cmpxchg, I think your only choice is - as
said - to read the old value and then loop over cmpxchg until that
succeeds. Not really different from other operations which need
emulating using cmpxchg.

>>>>> +static always_inline unsigned short __cmpxchg_case_2(volatile
>>>>> uint32_t *ptr,
>>>>> +                                                     uint32_t
>>>>> old,
>>>>> +                                                     uint32_t
>>>>> new)
>>>>> +{
>>>>> +    (void) old;
>>>>> +
>>>>> +    if (((unsigned long)ptr & 3) == 3)
>>>>> +    {
>>>>> +#ifdef CONFIG_64BIT
>>>>> +        return __emulate_cmpxchg_case1_2((uint64_t *)ptr, new,
>>>>> +                                         readq,
>>>>> __cmpxchg_case_8,
>>>>> 0xffffU);
>>>>
>>>> What if ((unsigned long)ptr & 7) == 7 (which is a sub-case of
>>>> what
>>>> the
>>>> if() above checks for? Isn't it more reasonable to require
>>>> aligned
>>>> 16-bit quantities here? Or if mis-aligned addresses are okay, you
>>>> could
>>>> as well emulate using __cmpxchg_case_4().
>>> Yes, it will be more reasonable. I'll use IS_ALIGNED instead.
>>
>> Not sure I get your use of "instead" here correctly. There's more
>> to change here than just the if() condition.
> I meant something like:
> 
> if ( IS_ALIGNED(ptr, 16) )
>     __emulate_cmpxchg_case1_2(...);
> else
>     assert_failed("ptr isn't aligned\n");

Except that you'd better not use assert_failed() directly anywhere,
and the above is easier as

    ASSERT(IS_ALIGNED(ptr, 16));
    __emulate_cmpxchg_case1_2(...);

anyway (leaving aside that I guess you mean 2, not 16).

Jan