Re: [PATCH v2 17/39] xen/riscv: introduce asm/atomic.h

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2023-12-07 at 16:57 +0100, Jan Beulich wrote:
> On 24.11.2023 11:30, Oleksii Kurochko wrote:
> > From: Bobby Eshleman <bobbyeshleman@xxxxxxxxx>
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> > Changes in V2:
> >  - Change an author of commit. I got this header from Bobby's old
> > repo.
> 
> Not sure how to deal with that when there's not also an S-o-b.
> 
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/atomic.h
> > @@ -0,0 +1,375 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * 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>
> 
> This and ...
> 
> > +#include <asm/fence.h>
> > +#include <asm/io.h>
> 
> .. this header are only introduced later. Bad ordering of the series?
Yes, bad ordering. I'll re-order this patch
Thanks.
> 
> > +#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 ) {
> 
> Nit (style): Brace on its own line (again further down).
> 
> > +    case 1: *(uint8_t *)res = readb((uint8_t *)p); break;
> > +    case 2: *(uint16_t *)res = readw((uint16_t *)p); break;
> > +    case 4: *(uint32_t *)res = readl((uint32_t *)p); break;
> > +    case 8: *(uint32_t *)res  = readq((uint64_t *)p); break;
> 
> Please don't cast away const-ness.
> 
> > +    default: __bad_atomic_size(); break;
> > +    }
> > +}
> > +
> > +#define read_atomic(p)
> > ({                                               \
> > +    union { typeof(*p) val; char c[0]; }
> > x_;                            \
> 
> Hmm, you avoid leading underscores here, but then ...
> 
> > +    read_atomic_size(p, x_.c,
> > sizeof(*p));                              \
> > +   
> > x_.val;                                                            
> > \
> > +})
> > +
> > +
> > +#define write_atomic(p, x)
> > ({                                           \
> > +    typeof(*p) __x =
> > (x);                                               \
> 
> ... they're still there here.
I'll rename __x to x__. Thanks.
> 
> > +    switch ( sizeof(*p) )
> > {                                             \
> > +    case 1: writeb((uint8_t)__x,  (uint8_t *)  p);
> > break;              \
> > +    case 2: writew((uint16_t)__x, (uint16_t *) p);
> > break;              \
> > +    case 4: writel((uint32_t)__x, (uint32_t *) p);
> > break;              \
> > +    case 8: writeq((uint64_t)__x, (uint64_t *) p);
> > break;              \
> > +    default: __bad_atomic_size();
> > break;                                \
> > +   
> > }                                                                  
> > \
> > +   
> > __x;                                                               
> > \
> > +})
> > +
> > +/* TODO: Fix this */
> > +#define add_sized(p, x)
> > ({                                              \
> > +    typeof(*(p)) __x =
> > (x);                                             \
> > +    switch ( sizeof(*(p))
> > )                                             \
> > +   
> > {                                                                  
> > \
> > +    case 1: writeb(read_atomic(p) + __x, (uint8_t *)(p));
> > break;        \
> > +    case 2: writew(read_atomic(p) + __x, (uint16_t *)(p));
> > break;       \
> > +    case 4: writel(read_atomic(p) + __x, (uint32_t *)(p));
> > break;       \
> 
> Instead of this, considering the comment perhaps better just BUG()?
I think this TODO can be removed. Macros looks fine to me.

> 
> > +    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),                                       \
> 
> I think you still owe us an update to ./README, clarifying what
> compiler versions
> may be used for building RISC-V. Unless of course all that exist
> support _Generic
> (which then would be nice to say in the description).
I'll provide a separate patch.

> 
> > +        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)))
> > +
> > +#define READ_ONCE(x)  (*(const volatile __unqual_scalar_typeof(x)
> > *)&(x))
> > +#define WRITE_ONCE(x, val)                                      \
> > +    do {                                                        \
> > +            *(volatile typeof(x) *)&(x) = (val);                \
> 
> Nit (style): Too deep indentation.
> 
> > +    } while (0)
> > +
> > +#define
> > __atomic_acquire_fence()                                    \
> > +   __asm__ __volatile__(RISCV_ACQUIRE_BARRIER "" :::
> > "memory")
> 
> Suddenly using tab indentation here and below? And missing blanks
> again.
Thanks. I'll fix  that.
> 
> Jan