Re: [PATCH v9 06/15] xen/riscv: introduce cmpxchg.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.05.2024 12:15, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/cmpxchg.h
> @@ -0,0 +1,239 @@
> +/* 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 _amoswap_generic(ptr, new, ret, sfx) \
> +    asm volatile ( \
> +        " amoswap" sfx " %0, %2, %1" \
> +        : "=r" (ret), "+A" (*(ptr)) \
> +        : "r" (new) \
> +        : "memory" );
> +
> +/*
> + * For LR and SC, the A extension requires that the address held in rs1 be
> + * naturally aligned to the size of the operand (i.e., eight-byte aligned
> + * for 64-bit words and four-byte aligned for 32-bit words).
> + * If the address is not naturally aligned, an address-misaligned exception
> + * or an access-fault exception will be generated.
> + *
> + * Thereby:
> + * - for 1-byte xchg access the containing word by clearing low two bits.
> + * - for 2-byte xchg access the containing word by clearing bit 1.
> + *
> + * If resulting 4-byte access is still misalgined, it will fault just as
> + * non-emulated 4-byte access would.
> + */
> +#define emulate_xchg_1_2(ptr, new, lr_sfx, sc_sfx) \
> +({ \
> +    uint32_t *aligned_ptr; \
> +    unsigned long alignment_mask = sizeof(*aligned_ptr) - sizeof(*(ptr)); \
> +    unsigned int new_val_bit = \
> +        ((unsigned long)(ptr) & alignment_mask) * BITS_PER_BYTE; \
> +    unsigned long mask = \
> +        GENMASK(((sizeof(*(ptr))) * BITS_PER_BYTE) - 1, 0) << new_val_bit; \
> +    unsigned int new_ = (new) << new_val_bit; \
> +    unsigned int old; \
> +    unsigned int scratch; \
> +    \
> +    aligned_ptr = (uint32_t *)((unsigned long)(ptr) & ~alignment_mask); \
> +    \
> +    asm volatile ( \
> +        "0: lr.w" lr_sfx " %[old], %[ptr_]\n" \
> +        "   andn  %[scratch], %[old], %[mask]\n" \
> +        "   or   %[scratch], %[scratch], %z[new_]\n" \
> +        "   sc.w" sc_sfx " %[scratch], %[scratch], %[ptr_]\n" \
> +        "   bnez %[scratch], 0b\n" \
> +        : [old] "=&r" (old), [scratch] "=&r" (scratch), \
> +          [ptr_] "+A" (*aligned_ptr) \
> +        : [new_] "rJ" (new_), [mask] "r" (mask) \
> +        : "memory" ); \
> +    \
> +    (__typeof__(*(ptr)))((old & mask) >> new_val_bit); \
> +})
> +
> +/*
> + * This function doesn't exist, so you'll get a linker error
> + * if something tries to do an invalid xchg().
> + */
> +extern unsigned long __bad_xchg(volatile void *ptr, int size);
> +
> +static always_inline unsigned long __xchg(volatile void *ptr, unsigned long 
> new, int size)

Nit: Line too long.

I also find "int size" odd: This can't be negative, can it? Ought to
be "unsigned int" then. Same for __cmpxchg() then.

> +#define xchg(ptr, x) \
> +({ \
> +    __typeof__(*(ptr)) n_ = (x); \
> +    (__typeof__(*(ptr))) \
> +        __xchg((ptr), (unsigned long)n_, sizeof(*(ptr))); \

Nit: I'm pretty sure I mentioned before that the parentheses around
the first argument are unnecessary. Same for cmpxchg() then.

With these taken care of
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

> --- a/xen/arch/riscv/include/asm/config.h
> +++ b/xen/arch/riscv/include/asm/config.h
> @@ -119,6 +119,8 @@
>  
>  #define BITS_PER_LLONG 64
>  
> +#define BITS_PER_BYTE 8
> +
>  /* xen_ulong_t is always 64 bits */
>  #define BITS_PER_XEN_ULONG 64

I'm in no way going to insist, but imo this would better be inserted
ahead of BYTES_PER_LONG.

Jan