Re: [PATCH v2 3/8] xen/ppc: Implement atomic.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.08.2023 22:07, Shawn Anastasio wrote:
> --- /dev/null
> +++ b/xen/arch/ppc/include/asm/atomic.h
> @@ -0,0 +1,390 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * PowerPC64 atomic operations
> + *
> + * Copyright (C) 2001 Paul Mackerras <paulus@xxxxxxxxxx>, IBM
> + * Copyright (C) 2001 Anton Blanchard <anton@xxxxxxxxxx>, IBM
> + * Copyright Raptor Engineering LLC
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */

License text again despite the SPDX header?

> +#ifndef _ASM_PPC64_ATOMIC_H_
> +#define _ASM_PPC64_ATOMIC_H_
> +
> +#include <xen/atomic.h>
> +
> +#include <asm/memory.h>
> +#include <asm/system.h>

I can see that you need memory.h, but I can't spot a need for system.h.

> +static inline int atomic_read(const atomic_t *v)
> +{
> +    return *(volatile int *)&v->counter;
> +}
> +
> +static inline int _atomic_read(atomic_t v)
> +{
> +    return v.counter;
> +}
> +
> +static inline void atomic_set(atomic_t *v, int i)
> +{
> +    v->counter = i;
> +}
> +
> +static inline void _atomic_set(atomic_t *v, int i)
> +{
> +    v->counter = i;
> +}
> +
> +void __bad_atomic_read(const volatile void *p, void *res);
> +void __bad_atomic_size(void);
> +
> +#define build_atomic_read(name, insn, type)                                  
>   \
> +    static inline type name(const volatile type *addr)                       
>   \
> +    {                                                                        
>   \
> +        type ret;                                                            
>   \
> +        asm volatile ( insn "%U1%X1 %0,%1" : "=r" (ret) : "m<>" (*addr) );   
>   \
> +        return ret;                                                          
>   \
> +    }
> +
> +#define build_atomic_write(name, insn, type)                                 
>   \
> +    static inline void name(volatile type *addr, type val)                   
>   \
> +    {                                                                        
>   \
> +        asm volatile ( insn "%U0%X0 %1,%0" : "=m<>" (*addr) : "r" (val) );   
>   \
> +    }
> +
> +#define build_add_sized(name, ldinsn, stinsn, type)                          
>   \
> +    static inline void name(volatile type *addr, type val)                   
>   \
> +    {                                                                        
>   \
> +        type t;                                                              
>   \
> +        asm volatile ( "1: " ldinsn " %0,0,%3\n"                             
>   \
> +                       "add%I2 %0,%0,%2\n"                                   
>   \
> +                       stinsn " %0,0,%3 \n"                                  
>   \
> +                       "bne- 1b\n"                                           
>   \
> +                       : "=&r" (t), "+m" (*addr)                             
>   \
> +                       : "r" (val), "r" (addr)                               
>   \
> +                       : "cc" );                                             
>   \
> +    }
> +
> +build_atomic_read(read_u8_atomic, "lbz", uint8_t)
> +build_atomic_read(read_u16_atomic, "lhz", uint16_t)
> +build_atomic_read(read_u32_atomic, "lwz", uint32_t)
> +build_atomic_read(read_u64_atomic, "ldz", uint64_t)
> +
> +build_atomic_write(write_u8_atomic, "stb", uint8_t)
> +build_atomic_write(write_u16_atomic, "sth", uint16_t)
> +build_atomic_write(write_u32_atomic, "stw", uint32_t)
> +build_atomic_write(write_u64_atomic, "std", uint64_t)
> +
> +build_add_sized(add_u8_sized, "lbarx", "stbcx.",uint8_t)
> +build_add_sized(add_u16_sized, "lharx", "sthcx.", uint16_t)
> +build_add_sized(add_u32_sized, "lwarx", "stwcx.", uint32_t)
> +
> +#undef build_atomic_read
> +#undef build_atomic_write
> +#undef build_add_sized
> +
> +static always_inline void read_atomic_size(const volatile void *p, void *res,
> +                                           unsigned int size)
> +{
> +    ASSERT(IS_ALIGNED((vaddr_t) p, size));

Nit: Stray blank before p (several more below).

> +    switch ( size )
> +    {
> +    case 1:
> +        *(uint8_t *)res = read_u8_atomic(p);
> +        break;
> +    case 2:
> +        *(uint16_t *)res = read_u16_atomic(p);
> +        break;
> +    case 4:
> +        *(uint32_t *)res = read_u32_atomic(p);
> +        break;
> +    case 8:
> +        *(uint64_t *)res = read_u64_atomic(p);
> +        break;
> +    default:
> +        __bad_atomic_read(p, res);
> +        break;
> +    }
> +}
> +
> +static always_inline void write_atomic_size(volatile void *p, void *val,

const void *val? (But then below also don't cast away constness.)

> +                                            unsigned int size)
> +{
> +    ASSERT(IS_ALIGNED((vaddr_t) p, size));
> +    switch ( size )
> +    {
> +    case 1:
> +        write_u8_atomic(p, *(uint8_t *)val);
> +        break;
> +    case 2:
> +        write_u16_atomic(p, *(uint16_t *)val);
> +        break;
> +    case 4:
> +        write_u32_atomic(p, *(uint32_t *)val);
> +        break;
> +    case 8:
> +        write_u64_atomic(p, *(uint64_t *)val);
> +        break;
> +    default:
> +        __bad_atomic_size();
> +        break;
> +    }
> +}
> +
> +#define read_atomic(p)                                                       
>   \
> +    ({                                                                       
>   \
> +        union {                                                              
>   \
> +            typeof(*(p)) val;                                                
>   \
> +            char c[0];                                                       
>   \

Using [0] here is likely to set us up for compiler complaints ...

> +        } x_;                                                                
>   \
> +        read_atomic_size(p, x_.c, sizeof(*(p)));                             
>   \

... here. Can't this simply be c[sizeof(*(val))]? And do you need
a union here in the first place, when read_atomic() takes void* as
its 2nd parameter?

> +        x_.val;                                                              
>   \
> +    })
> +
> +#define write_atomic(p, x)                                                   
>   \
> +    do                                                                       
>   \
> +    {                                                                        
>   \
> +        typeof(*(p)) x_ = (x);                                               
>   \
> +        write_atomic_size(p, &x_, sizeof(*(p)));                             
>   \
> +    } while ( 0 )
> +
> +#define add_sized(p, x)                                                      
>   \
> +    ({                                                                       
>   \
> +        typeof(*(p)) x_ = (x);                                               
>  \
> +        switch ( sizeof(*(p)) )                                              
>   \
> +        {                                                                    
>   \
> +        case 1:                                                              
>   \
> +            add_u8_sized((uint8_t *) (p), x_);                               
>  \
> +            break;                                                           
>   \
> +        case 2:                                                              
>   \
> +            add_u16_sized((uint16_t *) (p), x_);                             
>  \
> +            break;                                                           
>   \
> +        case 4:                                                              
>   \
> +            add_u32_sized((uint32_t *) (p), x_);                             
>  \
> +            break;                                                           
>   \
> +        default:                                                             
>   \
> +            __bad_atomic_size();                                             
>   \
> +            break;                                                           
>   \
> +        }                                                                    
>   \
> +    })

Nit: Padding wants to align the backslashes.

> +static inline void atomic_add(int a, atomic_t *v)
> +{
> +    int t;
> +
> +    asm volatile ( "1: lwarx %0,0,%3\n"
> +                   "add %0,%2,%0\n"
> +                   "stwcx. %0,0,%3\n"
> +                   "bne- 1b"
> +                   : "=&r" (t), "+m" (v->counter)

I notice you use "+m" here, but ...

> +                   : "r" (a), "r" (&v->counter)
> +                   : "cc" );
> +}
> +
> +static inline int atomic_add_return(int a, atomic_t *v)
> +{
> +    int t;
> +
> +    asm volatile ( PPC_ATOMIC_ENTRY_BARRIER
> +                   "1: lwarx %0,0,%2\n"
> +                   "add %0,%1,%0\n"
> +                   "stwcx. %0,0,%2\n"
> +                   "bne- 1b"
> +                   PPC_ATOMIC_EXIT_BARRIER
> +                   : "=&r" (t)
> +                   : "r" (a), "r" (&v->counter)
> +                   : "cc", "memory" );
> +
> +    return t;
> +}
> +
> +static inline void atomic_sub(int a, atomic_t *v)
> +{
> +    int t;
> +
> +    asm volatile ( "1: lwarx %0,0,%3\n"
> +                   "subf %0,%2,%0\n"
> +                   "stwcx. %0,0,%3\n"
> +                   "bne- 1b"
> +                   : "=&r" (t), "=m" (v->counter)
> +                   : "r" (a), "r" (&v->counter), "m" (v->counter)

... why not here (and again below)?

> +                   : "cc" );
> +}
> +
> +static inline int atomic_sub_return(int a, atomic_t *v)
> +{
> +    int t;
> +
> +    asm volatile ( PPC_ATOMIC_ENTRY_BARRIER
> +                   "1: lwarx %0,0,%2\n"
> +                   "subf %0,%1,%0\n"
> +                   "stwcx. %0,0,%2\n"
> +                   "bne- 1b"
> +                   PPC_ATOMIC_EXIT_BARRIER
> +                   : "=&r" (t)
> +                   : "r" (a), "r" (&v->counter)
> +                   : "cc", "memory" );
> +
> +    return t;
> +}
> +
> +static inline void atomic_inc(atomic_t *v)
> +{
> +    int t;
> +
> +    asm volatile ( "1: lwarx %0,0,%2\n"
> +                   "addic %0,%0,1\n"
> +                   "stwcx. %0,0,%2\n"
> +                   "bne- 1b"
> +                   : "=&r" (t), "=m" (v->counter)
> +                   : "r" (&v->counter), "m" (v->counter)
> +                   : "cc" );
> +}
> +
> +static inline int atomic_inc_return(atomic_t *v)
> +{
> +    int t;
> +
> +    asm volatile ( PPC_ATOMIC_ENTRY_BARRIER
> +                   "1: lwarx %0,0,%1\n"
> +                   "addic %0,%0,1\n"
> +                   "stwcx. %0,0,%1\n"
> +                   "bne- 1b"
> +                   PPC_ATOMIC_EXIT_BARRIER
> +                   : "=&r" (t)
> +                   : "r" (&v->counter)
> +                   : "cc", "memory" );
> +
> +    return t;
> +}
> +
> +static inline void atomic_dec(atomic_t *v)
> +{
> +    int t;
> +
> +    asm volatile ( "1: lwarx %0,0,%2\n"
> +                   "addic %0,%0,-1\n"
> +                   "stwcx. %0,0,%2\n"
> +                   "bne- 1b"
> +                   : "=&r" (t), "=m" (v->counter)
> +                   : "r" (&v->counter), "m" (v->counter)
> +                   : "cc" );
> +}
> +
> +static inline int atomic_dec_return(atomic_t *v)
> +{
> +    int t;
> +
> +    asm volatile ( PPC_ATOMIC_ENTRY_BARRIER
> +                   "1: lwarx %0,0,%1\n"
> +                   "addic %0,%0,-1\n"
> +                   "stwcx. %0,0,%1\n"
> +                   "bne- 1b"
> +                   PPC_ATOMIC_EXIT_BARRIER
> +                   : "=&r" (t)
> +                   : "r" (&v->counter)
> +                   : "cc", "memory" );
> +
> +    return t;
> +}
> +
> +/*
> + * Atomically test *v and decrement if it is greater than 0.
> + * The function returns the old value of *v minus 1.
> + */
> +static inline int atomic_dec_if_positive(atomic_t *v)
> +{
> +    int t;
> +
> +    asm volatile( PPC_ATOMIC_ENTRY_BARRIER
> +                  "1: lwarx %0,0,%1 # atomic_dec_if_positive\n"
> +                  "addic. %0,%0,-1\n"
> +                  "blt- 2f\n"
> +                  "stwcx. %0,0,%1\n"
> +                  "bne- 1b\n"
> +                  PPC_ATOMIC_EXIT_BARRIER
> +                  "2:"
> +                  : "=&r" (t)
> +                  : "r" (&v->counter)
> +                  : "cc", "memory" );
> +
> +    return t;
> +}
> +
> +static inline atomic_t atomic_compareandswap(atomic_t old, atomic_t new,
> +                                             atomic_t *v)
> +{
> +    atomic_t rc;
> +    rc.counter = __cmpxchg(&v->counter, old.counter, new.counter, 
> sizeof(int));
> +    return rc;
> +}
> +
> +#define arch_cmpxchg(ptr, o, n)                                              
>   \
> +    ({                                                                       
>   \
> +        __typeof__(*(ptr)) o_ = (o);                                         
>  \
> +        __typeof__(*(ptr)) n_ = (n);                                         
>  \
> +        (__typeof__(*(ptr))) __cmpxchg((ptr), (unsigned long) o_,            
>  \
> +                                       (unsigned long) n_, sizeof(*(ptr)));  
>  \
> +    })

Nit: Padding again.

Jan