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

[oleksii.kurochko@xxxxxxxxx]

On Wed, 2024-09-11 at 13:49 +0200, Jan Beulich wrote:
> On 11.09.2024 13:34, oleksii.kurochko@xxxxxxxxx wrote:
> > On Tue, 2024-09-10 at 18:05 +0200, Jan Beulich wrote:
> > > On 10.09.2024 17:28, oleksii.kurochko@xxxxxxxxx wrote:
> > > > On Tue, 2024-09-10 at 11:53 +0200, Jan Beulich wrote:
> > > > > On 02.09.2024 19:01, Oleksii Kurochko wrote:
> > > > > > @@ -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 *'}
> > > 
> > > Oh, I think I see now. What we'd like write_atomic() to derive is
> > > the
> > > bare
> > > (unqualified) type of *ptr, yet iirc only recent compilers have a
> > > way
> > > to
> > > obtain that.
> > I assume that you are speaking about typeof_unqual which requires
> > C23
> > (?).
> 
> What C version it requires doesn't matter much for our purposes. The
> question is as of which gcc / clang version (if any) this is
> supported
> as an extension.
> 
> > __auto_type seems to me can also drop volatile quilifier but in the
> > docs I don't see that it should (or not) discard qualifier. Could
> > it be
> > an option:
> >    #define write_atomic(p, x)                              \
> >    ({                                                      \
> >        __auto_type x_ = (x);                              \
> >        _write_atomic(p, &x_, sizeof(*(p)));                 \
> >    })
> 
> For our purposes __auto_type doesn't provide advantages over
> typeof().
> Plus, more importantly, the use above is wrong, just like typeof(x)
> would also be wrong. It needs to be p that the type is derived from.
> Otherwise consider what happens when ptr is unsigned long * or
> unsigned short * and you write
> 
>     write_atomic(ptr, 0);
> 
> > And another option could be just drop volatile by casting:
> >    #define write_atomic(p, x)                              \
> >    ...
> >        _write_atomic(p, (const void *)&x_,
> > sizeof(*(p)));                 
> 
> See what I said earlier about casts: You shall not cast away
> qualifiers. Besides doing so being bad practice, you'll notice the
> latest when RISC-V code also becomes subject to Misra compliance.

Then probably the best one option will be to use union:
   #define write_atomic(p, x)                                         
   \
   ({                                                                 
   \
       union { typeof(*(p)) val; char c[sizeof(*(p))]; } x_ = { .val =
   (x) };  \
       _write_atomic(p, x_.c, sizeof(*(p)));                          
   \
   })
   
~ Oleksii