diff mbox series

[v4,2/4] RISC-V: Fix ioremap_cache() and ioremap_wc() for systems with Svpbmt

Message ID 20221019131128.237026-3-apatel@ventanamicro.com
State New
Headers
Series Add PMEM support for RISC-V |

Commit Message

Anup Patel Oct. 19, 2022, 1:11 p.m. UTC 
  Currently, all flavors of ioremap_xyz() function maps to the generic
ioremap() which means any ioremap_xyz() call will always map the
target memory as IO using _PAGE_IOREMAP page attributes. This breaks
ioremap_cache() and ioremap_wc() on systems with Svpbmt because memory
remapped using ioremap_cache() and ioremap_wc() will use _PAGE_IOREMAP
page attributes.

To address above (just like other architectures), we implement RISC-V
specific ioremap_cache() and ioremap_wc() which maps memory using page
attributes as defined by the Svpbmt specification.

Fixes: ff689fd21cb1 ("riscv: add RISC-V Svpbmt extension support")
Co-developed-by: Mayuresh Chitale <mchitale@ventanamicro.com>
Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 arch/riscv/include/asm/io.h      | 10 ++++++++++
 arch/riscv/include/asm/pgtable.h |  2 ++
 2 files changed, 12 insertions(+)

Comments

Heiko Stübner Oct. 19, 2022, 2:19 p.m. UTC | #1 
Am Mittwoch, 19. Oktober 2022, 15:11:26 CEST schrieb Anup Patel:
> Currently, all flavors of ioremap_xyz() function maps to the generic
> ioremap() which means any ioremap_xyz() call will always map the
> target memory as IO using _PAGE_IOREMAP page attributes. This breaks
> ioremap_cache() and ioremap_wc() on systems with Svpbmt because memory
> remapped using ioremap_cache() and ioremap_wc() will use _PAGE_IOREMAP
> page attributes.
> 
> To address above (just like other architectures), we implement RISC-V
> specific ioremap_cache() and ioremap_wc() which maps memory using page
> attributes as defined by the Svpbmt specification.
> 
> Fixes: ff689fd21cb1 ("riscv: add RISC-V Svpbmt extension support")
> Co-developed-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>

Wasn't there discussion around those functions in general in v2?

In any case, the patch doesn't break anything on qemu and d1, so

Tested-by: Heiko Stuebner <heiko@sntech.de>


> ---
>  arch/riscv/include/asm/io.h      | 10 ++++++++++
>  arch/riscv/include/asm/pgtable.h |  2 ++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/io.h b/arch/riscv/include/asm/io.h
> index 92080a227937..92a31e543388 100644
> --- a/arch/riscv/include/asm/io.h
> +++ b/arch/riscv/include/asm/io.h
> @@ -133,6 +133,16 @@ __io_writes_outs(outs, u64, q, __io_pbr(), __io_paw())
>  #define outsq(addr, buffer, count) __outsq(PCI_IOBASE + (addr), buffer, count)
>  #endif
>  
> +#ifdef CONFIG_MMU
> +#define ioremap_wc(addr, size)		\
> +	ioremap_prot((addr), (size), _PAGE_IOREMAP_WC)
> +#endif
> +
>  #include <asm-generic/io.h>
>  
> +#ifdef CONFIG_MMU
> +#define ioremap_cache(addr, size)	\
> +	ioremap_prot((addr), (size), _PAGE_KERNEL)
> +#endif
> +
>  #endif /* _ASM_RISCV_IO_H */
> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> index 7ec936910a96..346b7c1a3eeb 100644
> --- a/arch/riscv/include/asm/pgtable.h
> +++ b/arch/riscv/include/asm/pgtable.h
> @@ -182,6 +182,8 @@ extern struct pt_alloc_ops pt_ops __initdata;
>  #define PAGE_TABLE		__pgprot(_PAGE_TABLE)
>  
>  #define _PAGE_IOREMAP	((_PAGE_KERNEL & ~_PAGE_MTMASK) | _PAGE_IO)
> +#define _PAGE_IOREMAP_WC	((_PAGE_KERNEL & ~_PAGE_MTMASK) | \
> +				 _PAGE_NOCACHE)
>  #define PAGE_KERNEL_IO		__pgprot(_PAGE_IOREMAP)
>  
>  extern pgd_t swapper_pg_dir[];
>
Anup Patel Oct. 19, 2022, 4:10 p.m. UTC | #2 
On Wed, Oct 19, 2022 at 7:49 PM Heiko Stuebner <heiko@sntech.de> wrote:
>
> Am Mittwoch, 19. Oktober 2022, 15:11:26 CEST schrieb Anup Patel:
> > Currently, all flavors of ioremap_xyz() function maps to the generic
> > ioremap() which means any ioremap_xyz() call will always map the
> > target memory as IO using _PAGE_IOREMAP page attributes. This breaks
> > ioremap_cache() and ioremap_wc() on systems with Svpbmt because memory
> > remapped using ioremap_cache() and ioremap_wc() will use _PAGE_IOREMAP
> > page attributes.
> >
> > To address above (just like other architectures), we implement RISC-V
> > specific ioremap_cache() and ioremap_wc() which maps memory using page
> > attributes as defined by the Svpbmt specification.
> >
> > Fixes: ff689fd21cb1 ("riscv: add RISC-V Svpbmt extension support")
> > Co-developed-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
>
> Wasn't there discussion around those functions in general in v2?

Yes, there was discussion about a few drivers using ioremap_xyz()
which is discouraged and drivers should use memremap().

We still need the arch specific ioremap_xyz() functions/macros
added by this patch because these are required by the generic
kernel memremap() implementation (refer, kernel/iomem.c).

>
> In any case, the patch doesn't break anything on qemu and d1, so
>
> Tested-by: Heiko Stuebner <heiko@sntech.de>

Regards,
Anup

>
>
> > ---
> >  arch/riscv/include/asm/io.h      | 10 ++++++++++
> >  arch/riscv/include/asm/pgtable.h |  2 ++
> >  2 files changed, 12 insertions(+)
> >
> > diff --git a/arch/riscv/include/asm/io.h b/arch/riscv/include/asm/io.h
> > index 92080a227937..92a31e543388 100644
> > --- a/arch/riscv/include/asm/io.h
> > +++ b/arch/riscv/include/asm/io.h
> > @@ -133,6 +133,16 @@ __io_writes_outs(outs, u64, q, __io_pbr(), __io_paw())
> >  #define outsq(addr, buffer, count) __outsq(PCI_IOBASE + (addr), buffer, count)
> >  #endif
> >
> > +#ifdef CONFIG_MMU
> > +#define ioremap_wc(addr, size)               \
> > +     ioremap_prot((addr), (size), _PAGE_IOREMAP_WC)
> > +#endif
> > +
> >  #include <asm-generic/io.h>
> >
> > +#ifdef CONFIG_MMU
> > +#define ioremap_cache(addr, size)    \
> > +     ioremap_prot((addr), (size), _PAGE_KERNEL)
> > +#endif
> > +
> >  #endif /* _ASM_RISCV_IO_H */
> > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > index 7ec936910a96..346b7c1a3eeb 100644
> > --- a/arch/riscv/include/asm/pgtable.h
> > +++ b/arch/riscv/include/asm/pgtable.h
> > @@ -182,6 +182,8 @@ extern struct pt_alloc_ops pt_ops __initdata;
> >  #define PAGE_TABLE           __pgprot(_PAGE_TABLE)
> >
> >  #define _PAGE_IOREMAP        ((_PAGE_KERNEL & ~_PAGE_MTMASK) | _PAGE_IO)
> > +#define _PAGE_IOREMAP_WC     ((_PAGE_KERNEL & ~_PAGE_MTMASK) | \
> > +                              _PAGE_NOCACHE)
> >  #define PAGE_KERNEL_IO               __pgprot(_PAGE_IOREMAP)
> >
> >  extern pgd_t swapper_pg_dir[];
> >
>
>
>
>
Arnd Bergmann Oct. 19, 2022, 8:50 p.m. UTC | #3 
On Wed, Oct 19, 2022, at 18:10, Anup Patel wrote:
> On Wed, Oct 19, 2022 at 7:49 PM Heiko Stuebner <heiko@sntech.de> wrote:
>>
>> Am Mittwoch, 19. Oktober 2022, 15:11:26 CEST schrieb Anup Patel:
>> > Currently, all flavors of ioremap_xyz() function maps to the generic
>> > ioremap() which means any ioremap_xyz() call will always map the
>> > target memory as IO using _PAGE_IOREMAP page attributes. This breaks
>> > ioremap_cache() and ioremap_wc() on systems with Svpbmt because memory
>> > remapped using ioremap_cache() and ioremap_wc() will use _PAGE_IOREMAP
>> > page attributes.
>> >
>> > To address above (just like other architectures), we implement RISC-V
>> > specific ioremap_cache() and ioremap_wc() which maps memory using page
>> > attributes as defined by the Svpbmt specification.
>> >
>> > Fixes: ff689fd21cb1 ("riscv: add RISC-V Svpbmt extension support")
>> > Co-developed-by: Mayuresh Chitale <mchitale@ventanamicro.com>
>> > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
>> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
>>
>> Wasn't there discussion around those functions in general in v2?
>
> Yes, there was discussion about a few drivers using ioremap_xyz()
> which is discouraged and drivers should use memremap().
>
> We still need the arch specific ioremap_xyz() functions/macros
> added by this patch because these are required by the generic
> kernel memremap() implementation (refer, kernel/iomem.c).

There is a difference between the strongly discouraged
ioremap_cache() that pretty much has no valid users, and
the ioremap_wt/ioremap_wc functions that are sometimes used
for mapping video framebuffer or similar.

It should be sufficient to provide a arch_memremap_wb()
and no ioremap_cache() to make memremap() work correctly.

      Arnd
Anup Patel Oct. 20, 2022, 7:38 a.m. UTC | #4 
On Thu, Oct 20, 2022 at 2:20 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Oct 19, 2022, at 18:10, Anup Patel wrote:
> > On Wed, Oct 19, 2022 at 7:49 PM Heiko Stuebner <heiko@sntech.de> wrote:
> >>
> >> Am Mittwoch, 19. Oktober 2022, 15:11:26 CEST schrieb Anup Patel:
> >> > Currently, all flavors of ioremap_xyz() function maps to the generic
> >> > ioremap() which means any ioremap_xyz() call will always map the
> >> > target memory as IO using _PAGE_IOREMAP page attributes. This breaks
> >> > ioremap_cache() and ioremap_wc() on systems with Svpbmt because memory
> >> > remapped using ioremap_cache() and ioremap_wc() will use _PAGE_IOREMAP
> >> > page attributes.
> >> >
> >> > To address above (just like other architectures), we implement RISC-V
> >> > specific ioremap_cache() and ioremap_wc() which maps memory using page
> >> > attributes as defined by the Svpbmt specification.
> >> >
> >> > Fixes: ff689fd21cb1 ("riscv: add RISC-V Svpbmt extension support")
> >> > Co-developed-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> >> > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> >> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> >>
> >> Wasn't there discussion around those functions in general in v2?
> >
> > Yes, there was discussion about a few drivers using ioremap_xyz()
> > which is discouraged and drivers should use memremap().
> >
> > We still need the arch specific ioremap_xyz() functions/macros
> > added by this patch because these are required by the generic
> > kernel memremap() implementation (refer, kernel/iomem.c).
>
> There is a difference between the strongly discouraged
> ioremap_cache() that pretty much has no valid users, and
> the ioremap_wt/ioremap_wc functions that are sometimes used
> for mapping video framebuffer or similar.
>
> It should be sufficient to provide a arch_memremap_wb()
> and no ioremap_cache() to make memremap() work correctly.
>

Okay, I will simplify this patch to only implement arch_memremap_wb().

This will make the MEMREMAP_WB flag to work correctly but other
MEMREMAP_xyz flags will always use non-cacheable IO mappings.

Regards,
Anup
diff mbox series

Patch

diff --git a/arch/riscv/include/asm/io.h b/arch/riscv/include/asm/io.h
index 92080a227937..92a31e543388 100644
--- a/arch/riscv/include/asm/io.h
+++ b/arch/riscv/include/asm/io.h
@@ -133,6 +133,16 @@  __io_writes_outs(outs, u64, q, __io_pbr(), __io_paw())
 #define outsq(addr, buffer, count) __outsq(PCI_IOBASE + (addr), buffer, count)
 #endif
 
+#ifdef CONFIG_MMU
+#define ioremap_wc(addr, size)		\
+	ioremap_prot((addr), (size), _PAGE_IOREMAP_WC)
+#endif
+
 #include <asm-generic/io.h>
 
+#ifdef CONFIG_MMU
+#define ioremap_cache(addr, size)	\
+	ioremap_prot((addr), (size), _PAGE_KERNEL)
+#endif
+
 #endif /* _ASM_RISCV_IO_H */
diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
index 7ec936910a96..346b7c1a3eeb 100644
--- a/arch/riscv/include/asm/pgtable.h
+++ b/arch/riscv/include/asm/pgtable.h
@@ -182,6 +182,8 @@  extern struct pt_alloc_ops pt_ops __initdata;
 #define PAGE_TABLE		__pgprot(_PAGE_TABLE)
 
 #define _PAGE_IOREMAP	((_PAGE_KERNEL & ~_PAGE_MTMASK) | _PAGE_IO)
+#define _PAGE_IOREMAP_WC	((_PAGE_KERNEL & ~_PAGE_MTMASK) | \
+				 _PAGE_NOCACHE)
 #define PAGE_KERNEL_IO		__pgprot(_PAGE_IOREMAP)
 
 extern pgd_t swapper_pg_dir[];