Re: [External] Re: [PATCH] riscv: introduce the ioremap_prot() function

[yunhui cui]

Hi Arnd,


On Thu, Mar 20, 2025 at 5:22 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> On Thu, Mar 20, 2025, at 09:44, Yunhui Cui wrote:
> > It's advisable to avoid mapping memory with the non-cache attribute.
> > This is because issues may arise when the same physical address is
> > mapped as both cacheable and non-cacheable simultaneously, such as
> > in the case of hardware prefetching.
> >
> > Signed-off-by: Yunhui Cui <cuiyunhui@xxxxxxxxxxxxx>
>
> Makes sense to me. Ideally we'd have the check in generic_ioremap_prot(),
> but I believe this would break on (at least) x86 because of
> legacy callers of ioremap() on memory.
>
> > diff --git a/arch/riscv/include/asm/io.h b/arch/riscv/include/asm/io.h
> > index a0e51840b9db..736c5557bd06 100644
> > --- a/arch/riscv/include/asm/io.h
> > +++ b/arch/riscv/include/asm/io.h
> > @@ -133,6 +133,8 @@ __io_writes_outs(outs, u64, q, __io_pbr(), __io_paw())
> >  #define outsq(addr, buffer, count) __outsq(PCI_IOBASE + (addr), buffer, count)
> >  #endif
> >
> > +#define ioremap_prot ioremap_prot
> > +
> >  #include <asm-generic/io.h>
> >
> >  #ifdef CONFIG_MMU
>
> This feels slightly wrong to me, the "#define foo foo" method
> is normally used to override a declaration or inline function with
> another one, but this one only overrides the implementation, not
> the declaration.
>
> I see the same is done on arc, arm64, parisc, powerpc, s390,
> sh and xtensa, so we can keep this one as well, but it would be
> nice to change all of these to a less surprising approach.
>
> Maybe we should just remove these macros from asm/io.h and
> the trivial wrapper from mm/ioremap.c, and instead change the
> other architectures that have GENERIC_IOREMAP to use
>
> #define ioremap_prot generic_ioremap_prot
>
> It seems this would be only csky, hexagon, (some) loongarch
> and openrisc.
>
>      Arnd

It seems that we can't simply use #define ioremap_prot
generic_ioremap_prot because some architectures have certain special
behaviors before calling generic_ioremap_prot(). For example, there's
the ioremap_prot_hook() logic on ARM64 and the CONFIG_EISA logic on
PA-RISC, among others.

Regarding the check of whether the address is a memory address, I
think we can directly incorporate pfn_valid() into
generic_ioremap_prot. This probably won't affect architectures that
directly use generic_ioremap_prot(), such as C-SKY, Hexagon, and
LoongArch.

So, my next plan is to add pfn_valid() to generic_ioremap_prot().

What's your opinion?

Thanks，
Yunhui