[v6] RISCV device tree mapping

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

Posted by Oleksii Kurochko 1 year, 5 months ago

The functions {read,write}{b,w,l,q}_cpu() do not need to be memory-ordered
atomic operations in Xen, based on their definitions 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().

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V6:
 - revert changes connected to _write_atomic() prototype and in definition of write_atomic().
 - update the commit message.
---
Changes in v5:
 - new patch.
---
 xen/arch/riscv/include/asm/atomic.h | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

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;
 #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)
+                                        unsigned long x,
+                                        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(x, p); break;
+    case 2: writew_cpu(x, p); break;
+    case 4: writel_cpu(x, p); break;
 #ifndef CONFIG_RISCV_32
-    case 8: writeq(x, p); break;
+    case 8: writeq_cpu(x, p); break;
 #endif
     default: __bad_atomic_size(); break;
     }
-- 
2.46.0

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

Posted by Andrew Cooper 1 year, 5 months ago

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...

~Andrew

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

Posted by oleksii.kurochko@gmail.com 1 year, 5 months ago

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.

~ Oleksii

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

Posted by Andrew Cooper 1 year, 5 months ago

On 04/09/2024 11:27 am, oleksii.kurochko@gmail.com 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".

~Andrew

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

Posted by oleksii.kurochko@gmail.com 1 year, 5 months ago

On Wed, 2024-09-04 at 11:31 +0100, Andrew Cooper wrote:
> On 04/09/2024 11:27 am, oleksii.kurochko@gmail.com 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

[PATCH v6 1/9] xen/riscv: prevent recursion when ASSERT(), BUG*(), or panic() are called
[PATCH v6 2/9] xen/riscv: use {read,write}{b,w,l,q}_cpu() to define {read,write}_atomic()
[PATCH v6 3/9] xen/riscv: allow write_atomic() to work with non-scalar types
[PATCH v6 4/9] xen/riscv: set up fixmap mappings
[PATCH v6 5/9] xen/riscv: introduce asm/pmap.h header
[PATCH v6 6/9] xen/riscv: introduce functionality to work with CPU info
[PATCH v6 7/9] xen/riscv: introduce and initialize SBI RFENCE extension
[PATCH v6 8/9] xen/riscv: page table handling
[PATCH v6 9/9] xen/riscv: introduce early_fdt_map()