Re: [PATCH v8 08/17] xen/riscv: introduce atomic.h

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2024-04-29 at 15:45 +0200, Jan Beulich wrote:
> On 17.04.2024 12:04, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/atomic.h
> > @@ -0,0 +1,281 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Taken and modified from Linux.
> > + *
> > + * The following changes were done:
> > + * - * atomic##prefix##_*xchg_*(atomic##prefix##_t *v, c_t n) were
> > updated
> > + *     to use__*xchg_generic()
> > + * - drop casts in write_atomic() as they are unnecessary
> > + * - drop introduction of WRITE_ONCE() and READ_ONCE().
> > + *   Xen provides ACCESS_ONCE()
> > + * - remove zero-length array access in read_atomic()
> > + * - drop defines similar to pattern
> > + *   #define atomic_add_return_relaxed   atomic_add_return_relaxed
> > + * - move not RISC-V specific functions to asm-generic/atomics-
> > ops.h
> > + * 
> > + * Copyright (C) 2007 Red Hat, Inc. All Rights Reserved.
> > + * Copyright (C) 2012 Regents of the University of California
> > + * Copyright (C) 2017 SiFive
> > + * Copyright (C) 2024 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);
> > +
> > +/*
> > + * Legacy from Linux kernel. For some reason they wanted to have
> > ordered
> > + * read/write access. Thereby read* is used instead of read*_cpu()
> > + */
> > +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;
> > +#ifndef CONFIG_RISCV_32
> > +    case 8: *(uint32_t *)res = readq(p); break;
> > +#endif
> > +    default: __bad_atomic_size(); break;
> > +    }
> > +}
> > +
> > +#define read_atomic(p) ({                                   \
> > +    union { typeof(*(p)) val; char c[sizeof(*(p))]; } x_;   \
> > +    read_atomic_size(p, x_.c, sizeof(*(p)));                \
> > +    x_.val;                                                 \
> > +})
> > +
> > +static always_inline void _write_atomic(volatile void *p,
> > +                                       unsigned long x, unsigned
> > int size)
> > +{
> > +    switch ( size )
> > +    {
> > +    case 1: writeb(x, p); break;
> > +    case 2: writew(x, p); break;
> > +    case 4: writel(x, p); break;
> > +#ifndef CONFIG_RISCV_32
> > +    case 8: writeq(x, p); break;
> > +#endif
> > +    default: __bad_atomic_size(); break;
> > +    }
> > +}
> > +
> > +#define write_atomic(p, x)                              \
> > +({                                                      \
> > +    typeof(*(p)) x_ = (x);                              \
> > +    _write_atomic((p), x_, sizeof(*(p)));               \
> 
> Nit: There are still excess parentheses here.
> 
> > +    x_;                                                 \
> 
> Why is this? The macro isn't supposed to "return" a value, is it?
You are right. write_atomic() isn't supposed to return a value, so
"x_;" can be dropped.

> 
> > +})
> > +
> > +static always_inline void _add_sized(volatile void *p,
> > +                                     unsigned long x, unsigned int
> > size)
> > +{
> > +    switch ( size )
> > +    {
> > +    case 1:
> > +    {
> > +        volatile uint8_t *ptr = (volatile uint8_t *)p;
> 
> Here and below: Why the casts?
Yes, they are not needed. Initially I decided that it could be a
compilation issue.

~ Oleksii