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