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 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:
> > > > --- 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.
> 
> No, that's definitely unsafe.

Then, at the moment, I don't see a better option than having const void
*x as an argument for the _write_atomic() function and then performing
casts when writeX_cpu(*(const uintX *)x, p) is called.

> 
> > > > @@ -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
(?).

__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)));                 \
   })

And another option could be just drop volatile by casting:
   #define write_atomic(p, x)                              \
   ...
       _write_atomic(p, (const void *)&x_, sizeof(*(p)));                 
   
~ Oleksii