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

[oleksii.kurochko@xxxxxxxxx]

On Tue, 2024-09-10 at 11:53 +0200, Jan Beulich wrote:
> 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.
If it would be better to keep 'unsigned long' as the type of x, then,
if I am not mistaken, write_atomic() should be updated in the following
way:
   #define write_atomic(p, x)                              \
   ({                                                      \
       typeof(*(p)) x_ = (x);                              \
       _write_atomic(p, *(unsigned long *)&x_, sizeof(*(p)));            
   \
   })
However, I am not sure if it is safe when x is a 2-byte value (for
example) that it will read more than 2 bytes before passing the value
to the _write_atomic() function.

> 
> > +    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?
if _add_sized() is defined as it was before:
   static always_inline void _add_sized(volatile void *p,
                                        unsigned long x, unsigned int
   size)
   {
       switch ( size )
       {
       case 1:
       {
           volatile uint8_t *ptr = p;
           write_atomic(ptr, read_atomic(ptr) + x);
           break;
       }
   ...
Then write_atomic(ptr, read_atomic(ptr) + x) will be be changed to:
   volatile uint8_t x_ = (x);
   
And that will cause a compiler error:
   ./arch/riscv/include/asm/atomic.h:75:22: error: passing argument 2
   of '_write_atomic' discards 'volatile' qualifier from pointer target
   type [-Werror=discarded-qualifiers]
      75 |     _write_atomic(p, &x_, sizeof(*(p)));
Because it can't cast 'volatile uint8_t *' to 'void *':
   expected 'void *' but argument is of type 'volatile uint8_t *' {aka
   'volatile unsigned char *'}

> 
> Also you surely mean readq_cpu() in the 8-byte case.
Yes, thanks for finding that.

~ Oleksii