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

[oleksii.kurochko@xxxxxxxxx]

On Thu, 2024-09-12 at 13:15 +0200, oleksii.kurochko@xxxxxxxxx wrote:
> 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)));                         
>    \
>    })
Or maybe we can use 'unsigned long' instead of char c[] and then the
casts inside _write_atomic() could be dropped as we can start to use
_write_atomic(..., const unsigned long x, ...).

But then probably it will be good to init: x_.c = 0UL to be sure that
when type of val is uint8_t for example then the significant bytes of
'union {...; unsigned long c}' are 0.

~ Oleksii