Re: [PATCH v3 15/34] xen/riscv: introduce atomic.h

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2024-01-22 at 17:56 +0100, Jan Beulich wrote:
> On 22.12.2023 16:12, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/atomic.h
> > @@ -0,0 +1,384 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Taken and modified from Linux.
> > + * 
> > + * 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((const uint8_t *)p); break;
> > +    case 2: *(uint16_t *)res = readw((const uint16_t *)p); break;
> > +    case 4: *(uint32_t *)res = readl((const uint32_t *)p); break;
> > +    case 8: *(uint32_t *)res  = readq((const uint64_t *)p); break;
> 
> Just like const, you should also avoid casting away volatile.
Thanks. I will drop casting.

> 
> > +    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;                                                            
> > \
> > +})
> > +
> > +
> 
> Nit: No double blank lines please.
Sure. I'll drtop one blank line.
> 
> > +#define write_atomic(p, x)
> > ({                                           \
> > +    typeof(*p) x__ =
> > (x);                                               \
> > +    switch ( sizeof(*p)
> > )                                                                           
> >                 \
> > +   
> > {                                                                           
> >                 \
> 
> These lines look excessively long, possibly as a result of leaving
> hard tabs
> in place.
> 
> Overall some of the style comments on the earlier patch seem to apply
> here
> as well.
> 
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/fence.h
> > @@ -0,0 +1,13 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +#ifndef _ASM_RISCV_FENCE_H
> > +#define _ASM_RISCV_FENCE_H
> > +
> > +#ifdef CONFIG_SMP
> > +#define RISCV_ACQUIRE_BARRIER              "\tfence r , rw\n"
> > +#define RISCV_RELEASE_BARRIER              "\tfence rw,  w\n"
> > +#else
> > +#define RISCV_ACQUIRE_BARRIER
> > +#define RISCV_RELEASE_BARRIER
> > +#endif
> 
> Do you really care about the !SMP case? On x86 at least we stopped
> special-
> casing that configuration many years ago (the few cases where for
> typically
> build reasons it matters, using CONFIG_NR_CPUS is sufficient). If you
> care
> about it, there needs to be somewhere you actually #define
> CONFIG_SMP.
Can't we use instead of CONFIG_SMP - CONFIG_NR_CPUS? I missed that
CONFIG_SMP is present in Linux, but not in Xen.

~ Oleksii