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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> +    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.

> +#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.

Jan