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

[oleksii.kurochko@xxxxxxxxx]

On Wed, 2024-09-04 at 11:31 +0100, Andrew Cooper wrote:
> On 04/09/2024 11:27 am, oleksii.kurochko@xxxxxxxxx wrote:
> > On Tue, 2024-09-03 at 15:21 +0100, Andrew Cooper wrote:
> > > On 02/09/2024 6:01 pm, Oleksii Kurochko wrote:
> > > > diff --git a/xen/arch/riscv/include/asm/atomic.h
> > > > b/xen/arch/riscv/include/asm/atomic.h
> > > > index 31b91a79c8..3c6bd86406 100644
> > > > --- 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;
> > > This cast looks suspiciously like it's wrong already in
> > > staging...
> > Thanks for noticing that, it should be really uint64_t. I'll update
> > that in the next patch version.
> 
> This bug is in 4.19.
> 
> I know RISC-V is experimental, but this is the kind of thing that Jan
> might consider for backporting.
> 
> Whether it gets backported or not, it wants to be in a standalone
> bugfix, not as a part of "rewrite the accessors used".
It makes sense. I will send a separate patch tomorrow.

~ Oleksii