Re: [PATCH for-4.14 1/3] xen/arm: atomic: Allow read_atomic() to be used in more cases

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Sat, 2 May 2020, Julien Grall wrote:
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> The current implementation of read_atomic() on Arm will not allow to:
>     1) Read a value from a pointer to const because the temporary
>     variable will be const and therefore it is not possible to assign
>     any value. This can be solved by using a union between the type and
>     a char[0].
>     2) Read a pointer value (e.g void *) because the switch contains
>     cast from other type than the size of a pointer. This can be solved by
>     by introducing a static inline for the switch and use void * for the
>     pointer.
> 
> Reported-by: Juergen Gross <jgross@xxxxxxxx>
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> ---
>  xen/include/asm-arm/atomic.h | 37 +++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h
> index e81bf80e305c..3c3d6bb04ee8 100644
> --- a/xen/include/asm-arm/atomic.h
> +++ b/xen/include/asm-arm/atomic.h
> @@ -71,18 +71,37 @@ build_add_sized(add_u32_sized, "", WORD, uint32_t)
>  #undef build_atomic_write
>  #undef build_add_sized
>  
> +void __bad_atomic_read(const volatile void *p, void *res);
>  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 = 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;
> +    }
> +}
> +
>  #define read_atomic(p) ({                                               \
> -    typeof(*p) __x;                                                     \
> -    switch ( sizeof(*p) ) {                                             \
> -    case 1: __x = (typeof(*p))read_u8_atomic((uint8_t *)p); break;      \
> -    case 2: __x = (typeof(*p))read_u16_atomic((uint16_t *)p); break;    \
> -    case 4: __x = (typeof(*p))read_u32_atomic((uint32_t *)p); break;    \
> -    case 8: __x = (typeof(*p))read_u64_atomic((uint64_t *)p); break;    \
> -    default: __x = 0; __bad_atomic_size(); break;                       \
> -    }                                                                   \
> -    __x;                                                                \
> +    union { typeof(*p) val; char c[0]; } x_;                            \
> +    read_atomic_size(p, x_.c, sizeof(*p));                              \

Wouldn't it be better to pass x_ as follows:

    read_atomic_size(p, &x_, sizeof(*p));

?

In my tests both ways works. I prefer the version with &x_ but given
that it works either way:

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>



> +    x_.val;                                                             \
>  })
>  
>  #define write_atomic(p, x) ({                                           \
> -- 
> 2.17.1
>