Re: [PATCH v6 3/9] xen/riscv: allow write_atomic() to work with non-scalar types

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.09.2024 19:01, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/atomic.h
> +++ b/xen/arch/riscv/include/asm/atomic.h
> @@ -54,16 +54,16 @@ static always_inline void read_atomic_size(const volatile 
> void *p,
>  })
>  
>  static always_inline void _write_atomic(volatile void *p,
> -                                        unsigned long x,
> +                                        void *x,

Pointer-to-const please, to further aid in easily recognizing which
parameter is what. After all ...

>                                          unsigned int size)
>  {
>      switch ( size )
>      {
> -    case 1: writeb_cpu(x, p); break;
> -    case 2: writew_cpu(x, p); break;
> -    case 4: writel_cpu(x, p); break;

... unhelpfully enough parameters are then swapped, just to confuse
things.

> +    case 1: writeb_cpu(*(uint8_t *)x, p); break;
> +    case 2: writew_cpu(*(uint16_t *)x, p); break;
> +    case 4: writel_cpu(*(uint32_t *)x, p); break;
>  #ifndef CONFIG_RISCV_32
> -    case 8: writeq_cpu(x, p); break;
> +    case 8: writeq_cpu(*(uint64_t *)x, p); break;

With const added to the parameter, please further make sure to then not
cast that away again.

> @@ -72,7 +72,7 @@ static always_inline void _write_atomic(volatile void *p,
>  #define write_atomic(p, x)                              \
>  ({                                                      \
>      typeof(*(p)) x_ = (x);                              \
> -    _write_atomic(p, x_, sizeof(*(p)));                 \
> +    _write_atomic(p, &x_, sizeof(*(p)));                \
>  })
>  
>  static always_inline void _add_sized(volatile void *p,
> @@ -82,27 +82,23 @@ static always_inline void _add_sized(volatile void *p,
>      {
>      case 1:
>      {
> -        volatile uint8_t *ptr = p;
> -        write_atomic(ptr, read_atomic(ptr) + x);
> +        writeb_cpu(readb_cpu(p) + x, p);
>          break;
>      }
>      case 2:
>      {
> -        volatile uint16_t *ptr = p;
> -        write_atomic(ptr, read_atomic(ptr) + x);
> +        writew_cpu(readw_cpu(p) + x, p);
>          break;
>      }
>      case 4:
>      {
> -        volatile uint32_t *ptr = p;
> -        write_atomic(ptr, read_atomic(ptr) + x);
> +        writel_cpu(readl_cpu(p) + x, p);
>          break;
>      }
>  #ifndef CONFIG_RISCV_32
>      case 8:
>      {
> -        volatile uint64_t *ptr = p;
> -        write_atomic(ptr, read_atomic(ptr) + x);
> +        writeq_cpu(readw_cpu(p) + x, p);
>          break;
>      }
>  #endif

I'm afraid I don't understand this part, or more specifically the respective
part of the description. It is the first parameter of write_atomic() which is
volatile qualified. And it is the first argument that's volatile qualified
here. Isn't the problem entirely unrelated to volatile-ness, and instead a
result of the other parameter changing from scalar to pointer type, which
doesn't fit the addition expressions you pass in?

Also you surely mean readq_cpu() in the 8-byte case.

Jan