Re: [PATCH v5 1/7] xen/riscv: use {read,write}{b,w,l,q}_cpu() to define {read,write}_atomic()

[oleksii.kurochko@xxxxxxxxx]

On Tue, 2024-08-27 at 12:06 +0200, Jan Beulich wrote:
> On 21.08.2024 18:06, Oleksii Kurochko wrote:
> > In Xen, memory-ordered atomic operations are not necessary,
> 
> This is an interesting statement.
I looked at the definition of build_atomic_{write,read}() for other
architectures and didn't find any additional memory-ordered primitives
such as fences.

> I'd like to suggest that you at least
> limit it to the two constructs in question, rather than stating this
> globally for everything.
I am not sure that I understand what is "the two constructs". Could you
please clarify?

> 
> > based on {read,write}_atomic() implementations for other
> > architectures.
> > Therefore, {read,write}{b,w,l,q}_cpu() can be used instead of
> > {read,write}{b,w,l,q}(), allowing the caller to decide if
> > additional
> > fences should be applied before or after {read,write}_atomic().
> > 
> > Change the declaration of _write_atomic() to accept a 'volatile
> > void *'
> > type for the 'x' argument instead of 'unsigned long'.
> > This prevents compilation errors such as:
> > 1."discards 'volatile' qualifier from pointer target type," which
> > occurs
> >   due to the initialization of a volatile pointer,
> >   e.g., `volatile uint8_t *ptr = p;` in _add_sized().
> 
> I don't follow you here.
This issue started occurring after the change mentioned in point 2
below.

I initially provided an incorrect explanation for the compilation error
mentioned above. Let me correct that now and update the commit message
in the next patch version. The reason for this error is that after the
_write_atomic() prototype was updated from _write_atomic(..., unsigned
long, ...) to _write_atomic(..., void *x, ...), the write_atomic()
macro contains x_, which is of type 'volatile uintX_t' because ptr has
the type 'volatile uintX_t *'.

Therefore, _write_atomic() should have its second argument declared as
volatile const void *. Alternatively, we can consider updating
write_atomic() to:
   #define write_atomic(p, x)                              \
   ({                                                      \
       typeof(*(p)) x_ = (x);                              \
       _write_atomic(p, (const void *)&x_, sizeof(*(p)));  \
   })
Would this be a better approach?Would it be better?

> It's the other argument of write_atomic() that
> has ptr passed there.
Probably the thing mentioned above it is what you mean here. I wasn't
sure that I understand this sentence correctly.

> 
> > 2."incompatible type for argument 2 of '_write_atomic'," which can
> > occur
> >   when calling write_pte(), where 'x' is of type pte_t rather than
> >   unsigned long.
> 
> How's this related to the change at hand? That isn't different ahead
> of
> this change, is it?
This is not directly related to the current change, which is why I
decided to add a sentence about write_pte().

Since write_pte(pte_t *p, pte_t pte) uses write_atomic(), and the
argument types are pte_t * and pte respectively, we encounter a
compilation issue in write_atomic() because:

_write_atomic() expects the second argument to be of type unsigned
long, leading to a compilation error like "incompatible type for
argument 2 of '_write_atomic'."
I considered defining write_pte() as write_atomic(p, pte.pte), but this
would fail at 'typeof(*(p)) x_ = (x);' and result in a compilation
error 'invalid initializer' or something like that.

It might be better to update write_pte() to:
   /* Write a pagetable entry. */
   static inline void write_pte(pte_t *p, pte_t pte)
   {
       write_atomic((unsigned long *)p, pte.pte);
   }
Then, we wouldn't need to modify the definition of write_atomic() or
change the type of the second argument of _write_atomic().
Would it be better?

> 
> > --- a/xen/arch/riscv/include/asm/atomic.h
> > +++ b/xen/arch/riscv/include/asm/atomic.h
> > @@ -31,21 +31,17 @@
> >  
> >  void __bad_atomic_size(void);
> >  
> > -/*
> > - * Legacy from Linux kernel. For some reason they wanted to have
> > ordered
> > - * read/write access. Thereby read* is used instead of read*_cpu()
> > - */
> >  static always_inline void read_atomic_size(const volatile void *p,
> >                                             void *res,
> >                                             unsigned int size)
> >  {
> >      switch ( size )
> >      {
> > -    case 1: *(uint8_t *)res = readb(p); break;
> > -    case 2: *(uint16_t *)res = readw(p); break;
> > -    case 4: *(uint32_t *)res = readl(p); break;
> > +    case 1: *(uint8_t *)res = readb_cpu(p); break;
> > +    case 2: *(uint16_t *)res = readw_cpu(p); break;
> > +    case 4: *(uint32_t *)res = readl_cpu(p); break;
> >  #ifndef CONFIG_RISCV_32
> > -    case 8: *(uint32_t *)res = readq(p); break;
> > +    case 8: *(uint32_t *)res = readq_cpu(p); break;
> >  #endif
> >      default: __bad_atomic_size(); break;
> >      }
> > @@ -58,15 +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, unsigned
> > int size)
> > +                                        volatile void *x,
> 
> If this really needs to become a pointer, it ought to also be
> pointer-
> to-const. Otherwise it is yet more confusing which operand is which.
Sure. I will add 'const' if the prototype of _write_atomic() won't use
'unsigned long' for x argument.

> 
> > +                                        unsigned int size)
> >  {
> >      switch ( size )
> >      {
> > -    case 1: writeb(x, p); break;
> > -    case 2: writew(x, p); break;
> > -    case 4: writel(x, p); break;
> > +    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(x, p); break;
> > +    case 8: writeq_cpu(*(uint64_t *)x, p); break;
> 
> Of course you may not cast away const-ness then. You also be casting
> away volatile-ness, but (as per above) I question the need for
> volatile
> on x.
I added an explanation about this earlier in the message. Let's discuss
whether volatile is needed there or not.

If I should not cast away the const and volatile qualifiers, then I
need to update the prototypes of writeX_cpu()?

~ Oleksii