|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 14/30] xen/riscv: introduce atomic.h
On 05.02.2024 16:32, Oleksii Kurochko wrote:
> From: Bobby Eshleman <bobbyeshleman@xxxxxxxxx>
>
> Additionally, this patch introduces macros in fence.h,
> which are utilized in atomic.h.
These are used in an earlier patch already, so either you want to
re-order the series, or you want to move that introduction ahead.
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/atomic.h
> @@ -0,0 +1,395 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Taken and modified from Linux.
> + *
> + * atomic##prefix##_*xchg_*(atomic##prefix##_t *v, c_t n) were updated to use
> + * __*xchg_generic()
> + *
> + * Copyright (C) 2007 Red Hat, Inc. All Rights Reserved.
> + * Copyright (C) 2012 Regents of the University of California
> + * Copyright (C) 2017 SiFive
> + * Copyright (C) 2021 Vates SAS
> + */
> +
> +#ifndef _ASM_RISCV_ATOMIC_H
> +#define _ASM_RISCV_ATOMIC_H
> +
> +#include <xen/atomic.h>
> +#include <asm/cmpxchg.h>
> +#include <asm/fence.h>
> +#include <asm/io.h>
> +#include <asm/system.h>
> +
> +void __bad_atomic_size(void);
> +
> +static always_inline void read_atomic_size(const volatile void *p,
> + void *res,
> + unsigned int size)
> +{
> + switch ( size )
> + {
> + case 1: *(uint8_t *)res = readb(p); break;
> + case 2: *(uint16_t *)res = readw(p); break;
> + case 4: *(uint32_t *)res = readl(p); break;
> + case 8: *(uint32_t *)res = readq(p); break;
Why is it the MMIO primitives you use here, i.e. not read<X>_cpu()?
It's RAM you're accessing after all.
Also - no CONFIG_64BIT conditional here (like you have in the other
patch)?
> + default: __bad_atomic_size(); break;
> + }
> +}
> +
> +#define read_atomic(p) ({ \
> + union { typeof(*p) val; char c[0]; } x_; \
> + read_atomic_size(p, x_.c, sizeof(*p)); \
I'll be curious for how much longer gcc will tolerate this accessing
of a zero-length array, without issuing at least a warning. I'd
recommend using sizeof(*(p)) as the array dimension right away. (From
this not also the missing parentheses in what you have.)
> + x_.val; \
> +})
> +
> +#define write_atomic(p, x) \
> +({ \
> + typeof(*p) x__ = (x); \
> + switch ( sizeof(*p) ) \
> + { \
> + case 1: writeb((uint8_t)x__, p); break; \
> + case 2: writew((uint16_t)x__, p); break; \
> + case 4: writel((uint32_t)x__, p); break; \
> + case 8: writeq((uint64_t)x__, p); break; \
Are the casts actually necessary here?
> + default: __bad_atomic_size(); break; \
> + } \
> + x__; \
> +})
> +
> +#define add_sized(p, x) \
> +({ \
> + typeof(*(p)) x__ = (x); \
> + switch ( sizeof(*(p)) ) \
> + { \
> + case 1: writeb(read_atomic(p) + x__, p); break; \
> + case 2: writew(read_atomic(p) + x__, p); break; \
> + case 4: writel(read_atomic(p) + x__, p); break; \
> + default: __bad_atomic_size(); break; \
> + } \
> +})
> +
> +/*
> + * __unqual_scalar_typeof(x) - Declare an unqualified scalar type, leaving
> + * non-scalar types unchanged.
> + *
> + * Prefer C11 _Generic for better compile-times and simpler code. Note:
> 'char'
> + * is not type-compatible with 'signed char', and we define a separate case.
> + */
> +#define __scalar_type_to_expr_cases(type) \
> + unsigned type: (unsigned type)0, \
> + signed type: (signed type)0
> +
> +#define __unqual_scalar_typeof(x) typeof( \
> + _Generic((x), \
> + char: (char)0, \
> + __scalar_type_to_expr_cases(char), \
> + __scalar_type_to_expr_cases(short), \
> + __scalar_type_to_expr_cases(int), \
> + __scalar_type_to_expr_cases(long), \
> + __scalar_type_to_expr_cases(long long), \
> + default: (x)))
This isn't RISC-V specific, is it? In which case it wants moving to,
perhaps, xen/macros.h (and then also have the leading underscores
dropped).
> +#define READ_ONCE(x) (*(const volatile __unqual_scalar_typeof(x) *)&(x))
> +#define WRITE_ONCE(x, val) \
> + do { \
> + *(volatile typeof(x) *)&(x) = (val); \
> + } while (0)
In Xen we use ACCESS_ONCE(); any reason you need to introduce
{READ,WRITE}_ONCE() in addition? Without them, __unqual_scalar_typeof()
may then also not be needed (or, if there's a need to enhance it, may
then be needed for ACCESS_ONCE()). Which in turn raises the question
why only READ_ONCE() uses it here.
> +#define __atomic_acquire_fence() \
> + __asm__ __volatile__( RISCV_ACQUIRE_BARRIER "" ::: "memory" )
Missing blank here and ...
> +#define __atomic_release_fence() \
> + __asm__ __volatile__( RISCV_RELEASE_BARRIER "" ::: "memory" );
... here, and stray semicolon additionally just here.
> +static inline int atomic_read(const atomic_t *v)
> +{
> + return READ_ONCE(v->counter);
> +}
> +
> +static inline int _atomic_read(atomic_t v)
> +{
> + return v.counter;
> +}
> +
> +static inline void atomic_set(atomic_t *v, int i)
> +{
> + WRITE_ONCE(v->counter, i);
> +}
> +
> +static inline void _atomic_set(atomic_t *v, int i)
> +{
> + v->counter = i;
> +}
> +
> +static inline int atomic_sub_and_test(int i, atomic_t *v)
> +{
> + return atomic_sub_return(i, v) == 0;
> +}
> +
> +static inline void atomic_inc(atomic_t *v)
> +{
> + atomic_add(1, v);
> +}
> +
> +static inline int atomic_inc_return(atomic_t *v)
> +{
> + return atomic_add_return(1, v);
> +}
> +
> +static inline void atomic_dec(atomic_t *v)
> +{
> + atomic_sub(1, v);
> +}
> +
> +static inline int atomic_dec_return(atomic_t *v)
> +{
> + return atomic_sub_return(1, v);
> +}
> +
> +static inline int atomic_dec_and_test(atomic_t *v)
> +{
> + return atomic_sub_return(1, v) == 0;
> +}
> +
> +static inline int atomic_add_negative(int i, atomic_t *v)
> +{
> + return atomic_add_return(i, v) < 0;
> +}
> +
> +static inline int atomic_inc_and_test(atomic_t *v)
> +{
> + return atomic_add_return(1, v) == 0;
> +}
None of these look RISC-V-specific. Perhaps worth having something in
asm-generic/ that can be utilized here?
> +/*
> + * First, the atomic ops that have no ordering constraints and therefor don't
> + * have the AQ or RL bits set. These don't return anything, so there's only
> + * one version to worry about.
> + */
> +#define ATOMIC_OP(op, asm_op, I, asm_type, c_type, prefix) \
> +static inline \
> +void atomic##prefix##_##op(c_type i, atomic##prefix##_t *v) \
> +{ \
> + __asm__ __volatile__ ( \
> + " amo" #asm_op "." #asm_type " zero, %1, %0" \
> + : "+A" (v->counter) \
> + : "r" (I) \
> + : "memory" ); \
> +} \
> +
> +#define ATOMIC_OPS(op, asm_op, I) \
> + ATOMIC_OP (op, asm_op, I, w, int, )
So the last three parameters are to be ready to also support
atomic64, without actually doing so right now?
> +ATOMIC_OPS(add, add, i)
> +ATOMIC_OPS(sub, add, -i)
> +ATOMIC_OPS(and, and, i)
> +ATOMIC_OPS( or, or, i)
> +ATOMIC_OPS(xor, xor, i)
> +
> +#undef ATOMIC_OP
> +#undef ATOMIC_OPS
> +
> +/*
> + * Atomic ops that have ordered, relaxed, acquire, and release variants.
> + * There's two flavors of these: the arithmatic ops have both fetch and
> return
> + * versions, while the logical ops only have fetch versions.
> + */
I'm somewhat confused by the comment: It first talks of 4 variants, but
then says there are only 2 (arithmetic) or 1 (logical) ones.
> +#define ATOMIC_FETCH_OP(op, asm_op, I, asm_type, c_type, prefix) \
> +static inline \
> +c_type atomic##prefix##_fetch_##op##_relaxed(c_type i, \
> + atomic##prefix##_t *v) \
> +{ \
> + register c_type ret; \
> + __asm__ __volatile__ ( \
> + " amo" #asm_op "." #asm_type " %1, %2, %0" \
> + : "+A" (v->counter), "=r" (ret) \
> + : "r" (I) \
> + : "memory" ); \
> + return ret; \
> +} \
> +static inline \
> +c_type atomic##prefix##_fetch_##op(c_type i, atomic##prefix##_t *v) \
> +{ \
> + register c_type ret; \
> + __asm__ __volatile__ ( \
> + " amo" #asm_op "." #asm_type ".aqrl %1, %2, %0" \
> + : "+A" (v->counter), "=r" (ret) \
> + : "r" (I) \
> + : "memory" ); \
> + return ret; \
> +}
> +
> +#define ATOMIC_OP_RETURN(op, asm_op, c_op, I, asm_type, c_type, prefix) \
> +static inline \
> +c_type atomic##prefix##_##op##_return_relaxed(c_type i, \
> + atomic##prefix##_t *v) \
> +{ \
> + return atomic##prefix##_fetch_##op##_relaxed(i, v) c_op I; \
> +} \
> +static inline \
> +c_type atomic##prefix##_##op##_return(c_type i, atomic##prefix##_t *v) \
> +{ \
> + return atomic##prefix##_fetch_##op(i, v) c_op I; \
> +}
> +
> +#define ATOMIC_OPS(op, asm_op, c_op, I) \
> + ATOMIC_FETCH_OP( op, asm_op, I, w, int, ) \
> + ATOMIC_OP_RETURN(op, asm_op, c_op, I, w, int, )
> +
> +ATOMIC_OPS(add, add, +, i)
> +ATOMIC_OPS(sub, add, +, -i)
> +
> +#define atomic_add_return_relaxed atomic_add_return_relaxed
> +#define atomic_sub_return_relaxed atomic_sub_return_relaxed
> +#define atomic_add_return atomic_add_return
> +#define atomic_sub_return atomic_sub_return
> +
> +#define atomic_fetch_add_relaxed atomic_fetch_add_relaxed
> +#define atomic_fetch_sub_relaxed atomic_fetch_sub_relaxed
> +#define atomic_fetch_add atomic_fetch_add
> +#define atomic_fetch_sub atomic_fetch_sub
What are all of these #define-s (any yet more further down) about?
> +static inline int atomic_sub_if_positive(atomic_t *v, int offset)
> +{
> + int prev, rc;
> +
> + __asm__ __volatile__ (
> + "0: lr.w %[p], %[c]\n"
> + " sub %[rc], %[p], %[o]\n"
> + " bltz %[rc], 1f\n"
> + " sc.w.rl %[rc], %[rc], %[c]\n"
> + " bnez %[rc], 0b\n"
> + " fence rw, rw\n"
> + "1:\n"
> + : [p]"=&r" (prev), [rc]"=&r" (rc), [c]"+A" (v->counter)
> + : [o]"r" (offset)
Nit: Blanks please between ] and ".
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/fence.h
> @@ -0,0 +1,8 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef _ASM_RISCV_FENCE_H
> +#define _ASM_RISCV_FENCE_H
> +
> +#define RISCV_ACQUIRE_BARRIER "\tfence r , rw\n"
> +#define RISCV_RELEASE_BARRIER "\tfence rw, w\n"
Seeing that another "fence rw, rw" appears in this patch, I'm now pretty
sure you want to add e.g. RISCV_FULL_BARRIER here as well.
Jan
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |