Re: [PATCH v4 14/30] xen/riscv: introduce atomic.h

[Julien Grall <julien@xxxxxxx>]

Hi,

On 05/02/2024 15:32, Oleksii Kurochko wrote:
> From: Bobby Eshleman <bobbyeshleman@xxxxxxxxx>
>
> Additionally, this patch introduces macros in fence.h,
> which are utilized in atomic.h.
>
> atomic##prefix##_*xchg_*(atomic##prefix##_t *v, c_t n)
> were updated to use __*xchg_generic().
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
The author is Bobby, but I don't see a Signed-off-by. Did you forgot it?

> ---
> Changes in V4:
>   - do changes related to the updates of [PATCH v3 13/34] xen/riscv: introduce 
> cmpxchg.h
>   - drop casts in read_atomic_size(), write_atomic(), add_sized()
>   - tabs -> spaces
>   - drop #ifdef CONFIG_SMP ... #endif in fence.ha as it is simpler to handle 
> NR_CPUS=1
>     the same as NR_CPUS>1 with accepting less than ideal performance.
> ---
> Changes in V3:
>    - update the commit message
>    - add SPDX for fence.h
>    - code style fixes
>    - Remove /* TODO: ... */ for add_sized macros. It looks correct to me.
>    - re-order the patch
>    - merge to this patch fence.h
> ---
> Changes in V2:
>   - Change an author of commit. I got this header from Bobby's old repo.
> ---
>   xen/arch/riscv/include/asm/atomic.h | 395 ++++++++++++++++++++++++++++
>   xen/arch/riscv/include/asm/fence.h  |   8 +
>   2 files changed, 403 insertions(+)
>   create mode 100644 xen/arch/riscv/include/asm/atomic.h
>   create mode 100644 xen/arch/riscv/include/asm/fence.h
>
> diff --git a/xen/arch/riscv/include/asm/atomic.h 
> b/xen/arch/riscv/include/asm/atomic.h
> new file mode 100644
> index 0000000000..267d3c0803
> --- /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.
Which version of Linux? Can you also spell out what are the big changes? 
This would be helpful if we need to re-sync.
> + *
> + * 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;
> +    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));              \
> +    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;            \
> +    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'
Xen is technically built using c99/gnu99. So it is feels a bit odd to 
introduce a C11 feature. I see that _Generic is already used in PPC... 
However, if we decide to add more use of it, then I think this should at 
minimum be documented in docs/misra/C-language-toolchain.rst (the more 
if we plan the macro is moved to common as Jan suggested).
Cheers,

--
Julien Grall