Re: [PATCH for-4.14 2/3] xen/arm: atomic: Rewrite write_atomic()

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Sat, 2 May 2020, Julien Grall wrote:
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> The current implementation of write_atomic has two issues:
>     1) It cannot be used to write pointer value because the switch
>     contains cast to other size than the size of the pointer.
>     2) It will happily allow to write to a pointer to const.
> 
> Additionally, the Arm implementation is returning a value when the x86
> implementation does not anymore. This was introduced in commit
> 2934148a0773 "x86: simplify a few macros / inline functions". There are
> no users of the return value, so it is fine to drop it.
> 
> The switch is now moved in a static inline helper allowing the compiler
> to prevent use of const pointer and also allow to write pointer value.
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> ---
>  xen/include/asm-arm/atomic.h | 40 ++++++++++++++++++++++++++----------
>  1 file changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h
> index 3c3d6bb04ee8..ac2798d095eb 100644
> --- a/xen/include/asm-arm/atomic.h
> +++ b/xen/include/asm-arm/atomic.h
> @@ -98,23 +98,41 @@ static always_inline void read_atomic_size(const volatile 
> void *p,
>      }
>  }
>  
> +static always_inline void write_atomic_size(volatile void *p,
> +                                            void *val,
> +                                            unsigned int 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]; } x_;                            \
>      read_atomic_size(p, x_.c, sizeof(*p));                              \
>      x_.val;                                                             \
>  })
>  
> -#define write_atomic(p, x) ({                                           \
> -    typeof(*p) __x = (x);                                               \
> -    switch ( sizeof(*p) ) {                                             \
> -    case 1: write_u8_atomic((uint8_t *)p, (uint8_t)__x); break;         \
> -    case 2: write_u16_atomic((uint16_t *)p, (uint16_t)__x); break;      \
> -    case 4: write_u32_atomic((uint32_t *)p, (uint32_t)__x); break;      \
> -    case 8: write_u64_atomic((uint64_t *)p, (uint64_t)__x); break;      \
> -    default: __bad_atomic_size(); break;                                \
> -    }                                                                   \
> -    __x;                                                                \
> -})
> +#define write_atomic(p, x)                                              \
> +    do {                                                                \
> +        typeof(*p) x_ = (x);                                            \
> +        write_atomic_size(p, &x_, sizeof(*p));                          \
> +    } while ( false )
>  
>  #define add_sized(p, x) ({                                              \
>      typeof(*(p)) __x = (x);                                             \
> -- 
> 2.17.1
>