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

[Jan Beulich <jbeulich@xxxxxxxx>]

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:
>>> +            : "=r" (ret__), "+A" (*ptr__) \
>>> +            : "r" (new__) \
>>> +            : "memory" ); \
>>> +        break; \
>>> +    case 8: \
>>> +        asm volatile( \
>>> +            "      amoswap.d %0, %2, %1\n" \
>>> +            : "=r" (ret__), "+A" (*ptr__) \
>>> +            : "r" (new__) \
>>> +            : "memory" ); \
>>> +        break; \
>>> +    default: \
>>> +        ASSERT_UNREACHABLE(); \
>>
>> If at all possible this wants to trigger a build failure, not a
>> runtime
>> one.
> I'll update that with BUILD_BUG_ON(size > 8);

What about size accidentally being e.g. 5? I'm also not sure you'll be
able to use BUILD_BUG_ON() here: That'll depend on what use sites there
are. And if all present ones are okay in this regard, you'd still set
out a trap for someone else to fall into later. We have a common
approach for this, which currently is under re-work. See
https://lists.xen.org/archives/html/xen-devel/2024-01/msg01115.html.

>>> +    } \
>>> +    ret__; \
>>> +})
>>> +
>>> +#define xchg_relaxed(ptr, x) \
>>> +({ \
>>> +    __typeof__(*(ptr)) x_ = (x); \
>>> +    (__typeof__(*(ptr))) __xchg_relaxed((ptr), x_,
>>> sizeof(*(ptr))); \
>>
>> Nit: Stray blank after cast. For readability I'd also suggest to
>> drop parentheses in cases like the first argument passed to
>> __xchg_relaxed() here.
> Thanks. I'll take that into account.
> 
>>
>>> +})
>>
>> For both: What does "relaxed" describe? I'm asking because it's not
>> really clear whether the memory clobbers are actually needed.
>>
>>> +#define __xchg_acquire(ptr, new, size) \
>>> +({ \
>>> +    __typeof__(ptr) ptr__ = (ptr); \
>>> +    __typeof__(new) new__ = (new); \
>>> +    __typeof__(*(ptr)) ret__; \
>>> +    switch (size) \
>>> +   { \
>>> +    case 4: \
>>> +        asm volatile( \
>>> +            "      amoswap.w %0, %2, %1\n" \
>>> +            RISCV_ACQUIRE_BARRIER \
>>> +            : "=r" (ret__), "+A" (*ptr__) \
>>> +            : "r" (new__) \
>>> +            : "memory" ); \
>>> +        break; \
>>> +    case 8: \
>>> +        asm volatile( \
>>> +            "      amoswap.d %0, %2, %1\n" \
>>> +            RISCV_ACQUIRE_BARRIER \
>>> +            : "=r" (ret__), "+A" (*ptr__) \
>>> +            : "r" (new__) \
>>> +            : "memory" ); \
>>> +        break; \
>>> +    default: \
>>> +        ASSERT_UNREACHABLE(); \
>>> +    } \
>>> +    ret__; \
>>> +})
>>
>> If I'm not mistaken this differs from __xchg_relaxed() only in the
>> use
>> of RISCV_ACQUIRE_BARRIER, and ...
>>
>>> +#define xchg_acquire(ptr, x) \
>>> +({ \
>>> +    __typeof__(*(ptr)) x_ = (x); \
>>> +    (__typeof__(*(ptr))) __xchg_acquire((ptr), x_,
>>> sizeof(*(ptr))); \
>>> +})
>>> +
>>> +#define __xchg_release(ptr, new, size) \
>>> +({ \
>>> +    __typeof__(ptr) ptr__ = (ptr); \
>>> +    __typeof__(new) new__ = (new); \
>>> +    __typeof__(*(ptr)) ret__; \
>>> +    switch (size) \
>>> +   { \
>>> +    case 4: \
>>> +        asm volatile ( \
>>> +            RISCV_RELEASE_BARRIER \
>>> +            "      amoswap.w %0, %2, %1\n" \
>>> +            : "=r" (ret__), "+A" (*ptr__) \
>>> +            : "r" (new__) \
>>> +            : "memory"); \
>>> +        break; \
>>> +    case 8: \
>>> +        asm volatile ( \
>>> +            RISCV_RELEASE_BARRIER \
>>> +            "      amoswap.d %0, %2, %1\n" \
>>> +            : "=r" (ret__), "+A" (*ptr__) \
>>> +            : "r" (new__) \
>>> +            : "memory"); \
>>> +        break; \
>>> +    default: \
>>> +        ASSERT_UNREACHABLE(); \
>>> +    } \
>>> +    ret__; \
>>> +})
>>
>> this only in the use of RISCV_RELEASE_BARRIER. If so they likely want
>> folding, to limit redundancy and make eventual updating easier. (Same
>> for the cmpxchg helper further down, as it seems.)
> Yes, you are right. The difference is only in RISCV_RELEASE_BARRIER and
> it is an absence of RISCV_RELEASE_BARRIER is a reason why we have
> "relaxed" versions.
> 
> I am not sure that I understand what you mean by folding here. Do you
> mean that there is no any sense to have to separate macros and it is
> needed only one with RISCV_RELEASE_BARRIER?

No. You should parameterize the folded common macro for the derived
macros to simply pass in the barriers needed, with empty macro
arguments indicating "this barrier not needed".

>>> +#define xchg_release(ptr, x) \
>>> +({ \
>>> +    __typeof__(*(ptr)) x_ = (x); \
>>> +    (__typeof__(*(ptr))) __xchg_release((ptr), x_,
>>> sizeof(*(ptr))); \
>>> +})
>>> +
>>> +static always_inline uint32_t __xchg_case_4(volatile uint32_t
>>> *ptr,
>>> +                                            uint32_t new)
>>> +{
>>> +    __typeof__(*(ptr)) ret;
>>> +
>>> +    asm volatile (
>>> +        "   amoswap.w.aqrl %0, %2, %1\n"
>>> +        : "=r" (ret), "+A" (*ptr)
>>> +        : "r" (new)
>>> +        : "memory" );
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static always_inline uint64_t __xchg_case_8(volatile uint64_t
>>> *ptr,
>>> +                                            uint64_t new)
>>> +{
>>> +    __typeof__(*(ptr)) ret;
>>> +
>>> +    asm volatile( \
>>> +        "   amoswap.d.aqrl %0, %2, %1\n" \
>>> +        : "=r" (ret), "+A" (*ptr) \
>>> +        : "r" (new) \
>>> +        : "memory" ); \
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static always_inline unsigned short __cmpxchg_case_2(volatile
>>> uint32_t *ptr,
>>> +                                                     uint32_t old,
>>> +                                                     uint32_t
>>> new);
>>
>> Don't you consistently mean uint16_t here (incl the return type) and
>> ...
>>
>>> +static always_inline unsigned short __cmpxchg_case_1(volatile
>>> uint32_t *ptr,
>>> +                                                     uint32_t old,
>>> +                                                     uint32_t
>>> new);
>>
>> ... uint8_t here?
> The idea was that we emulate __cmpxchg_case_1 and __cmpxchg_case_2
> using 4 bytes cmpxchg and __cmpxchg_case_4 expects uint32_t.

I consider this wrong. The functions would better be type-correct.

>>> +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).

>>> +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.

Jan