Re: [PATCH 1/7] mips: dmi: Fix early remap on MIPS32

[Serge Semin]

On Tue, Nov 28, 2023 at 10:59:10PM +0100, Arnd Bergmann wrote:
> On Tue, Nov 28, 2023, at 14:52, Serge Semin wrote:
> > On Tue, Nov 28, 2023 at 01:41:51PM +0100, Arnd Bergmann wrote:
> >> On Mon, Nov 27, 2023, at 17:23, Serge Semin wrote:
> >> > On Fri, Nov 24, 2023 at 10:03:49PM +0000, Jiaxun Yang wrote:
> >> but ioremap_cache() is generally underspecified because the
> >> resulting pointer is neither safe to dereference nor to pass into
> >> readl()/writel()/memcpy_fromio() on all architectures.
> >
> > I don't know about ARM64 (which for instance has it utilized to access
> > the DMI region), but at least in case of MIPS32 (a fortiori MIPS64
> > seeing the ioremap_cache() method actually returns a pointer to the
> > uncached region) I don't see a reason why it wouldn't be safe in both
> > cases described by you. All IO and memory regions are accessed by the
> > generic load and store instructions. The only difference is that the
> > MMIO-space accessors normally implies additional barriers, which just
> > slow down the execution, but shouldn't cause any other problem. Could
> > you clarify why do you think otherwise?
> 
> On arch/powerpc, CONFIG_PPC_INDIRECT_MMIO makes all ioremap()
> type functions return a token that can be passed into the readl/writel
> family but that is not a pointer you can dereference.
> 
> On s390, the mechanism is different, but similarly __iomem
> tokens are not pointers at all.

Ah, you meant that it was not generically safe. Then your were correct
for sure. I was talking about the MIPS arch, which doesn't
differentiate normal and IO memory pointers: all of them are accessed
by the same instructions. So ioremap_prot() returns just a normal
pointer there, which can be safely de-referenced.

> 
> >> There was an effort to convert the remaining ioremap_cache() calls
> >> into memremap() a few years ago, not sure if that's still being worked
> >> on but it would be the right thing to do.
> >
> > I see. Thanks for the pointing out to that. I guess it could be done
> > for MIPS too (at least on our MIPS32 platform DMI is just a memory
> > region pre-initialized by the bootloader), but the conversion would
> > require much efforts. Alas currently I can't afford to get it
> > implemented in the framework of this patchset. (I saved your note in
> > my MIPS TODO list though. Let's hope eventually I'll be able to get
> > back to this topic.)
> 

> I just noticed that the only architectures that actually provide
> ioremap_cache() are x86, arm, arm64, mips, loongarch, powerpc, sh
> and xtensa. The ones that have ACPI support still definitely
> need it, most of the other ones can probably be fixed without
> too much trouble.

Ok. Thanks. I'll have a look at that on my free time.

-Serge(y)

> 
>      Arnd