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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.12.2023 16:12, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/cmpxchg.h
> @@ -0,0 +1,496 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright (C) 2014 Regents of the University of California */
> +
> +#ifndef _ASM_RISCV_CMPXCHG_H
> +#define _ASM_RISCV_CMPXCHG_H
> +
> +#include <xen/compiler.h>
> +#include <xen/lib.h>
> +
> +#include <asm/fence.h>
> +#include <asm/io.h>
> +#include <asm/system.h>
> +
> +#define __xchg_relaxed(ptr, new, size) \
> +({ \
> +    __typeof__(ptr) ptr__ = (ptr); \
> +    __typeof__(new) new__ = (new); \
> +    __typeof__(*(ptr)) ret__; \

I expect the types of new and *ptr want to actually match. Which
you then want to enforce, so that issues at use sites would either
be reported by the compiler, or be permitted by a type conversion
of new.

> +    switch (size) \
> +     { \

Nit: Hard tab left here. (Also again you want to either stick to
Linux style or fully switch to Xen style.)

> +    case 4: \
> +        asm volatile( \
> +            "        amoswap.w %0, %2, %1\n" \

I don't think a leading tab (or leading whitespace in general) is
needed in single-line-output asm()s. The trailing \n also isn't
needed if I'm not mistaken.

> +            : "=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.

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

> +})

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

> +#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?

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

> +    case 4:
> +        return __xchg_case_4(ptr, x);
> +    case 8:
> +        return __xchg_case_8(ptr, x);
> +    default:
> +        ASSERT_UNREACHABLE();
> +    }
> +
> +    return -1;
> +}
> +
> +#define xchg(ptr,x) \
> +({ \
> +    __typeof__(*(ptr)) ret__; \
> +    ret__ = (__typeof__(*(ptr))) \
> +            __xchg((ptr), (unsigned long)(x), sizeof(*(ptr))); \
> +    ret__; \
> +})
> +
> +#define xchg32(ptr, x) \
> +({ \
> +    BUILD_BUG_ON(sizeof(*(ptr)) != 4); \
> +    xchg((ptr), (x)); \
> +})
> +
> +#define xchg64(ptr, x) \
> +({ \
> +    BUILD_BUG_ON(sizeof(*(ptr)) != 8); \
> +    xchg((ptr), (x)); \
> +})

What are these two (and their cmpxchg counterparts) needed for?

> +/*
> + * Atomic compare and exchange.  Compare OLD with MEM, if identical,
> + * store NEW in MEM.  Return the initial value in MEM.  Success is
> + * indicated by comparing RETURN with OLD.
> + */
> +#define __cmpxchg_relaxed(ptr, old, new, size) \
> +({ \
> +    __typeof__(ptr) ptr__ = (ptr); \
> +    __typeof__(*(ptr)) __old = (old); \

Leftover leading underscores?

> +    __typeof__(*(ptr)) new__ = (new); \

Related to my earlier comment on types needing to be compatible - see
how here you're using "ptr" throughout.

> +    __typeof__(*(ptr)) ret__; \
> +    register unsigned int __rc; \

More leftover leading underscores?

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

Also you shouldn't be casting away volatile (here and below). Avoiding
the casts (by suitable using volatile void * parameter types) would
likely be best.

Jan