Message-ID: <fqwkkyt253uvdaj6qlsu67b25qj35ongh4rbxzgzuwnykl36hi@xinsnpcltpgx>
Date:   Fri, 1 Dec 2023 17:54:10 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     Jiaxun Yang <jiaxun.yang@...goat.com>
Cc:     Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Arnd Bergmann <arnd@...db.de>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Mike Rapoport <rppt@...nel.org>,
        Matthew Wilcox <willy@...radead.org>,
        Tiezhu Yang <yangtiezhu@...ngson.cn>,
        Huacai Chen <chenhuacai@...nel.org>,
        Yinglu Yang <yangyinglu@...ngson.cn>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Aleksandar Rikalo <aleksandar.rikalo@...mia.com>,
        Aleksandar Rikalo <arikalo@...il.com>,
        Dragan Mladjenovic <dragan.mladjenovic@...mia.com>,
        Chao-ying Fu <cfu@...ecomp.com>, Marc Zyngier <maz@...nel.org>,
        "linux-mips@...r.kernel.org" <linux-mips@...r.kernel.org>,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/7] mips: dmi: Fix early remap on MIPS32

On Fri, Dec 01, 2023 at 12:13:22AM +0000, Jiaxun Yang wrote:
> 
> 
> 在2023年11月30日十一月 下午7:16，Serge Semin写道：
> > On Tue, Nov 28, 2023 at 03:46:37PM +0000, Jiaxun Yang wrote:
> [...]
> >
> >> I'd say the safest option is to use CKSEG0 or TO_CAC here, 
> >
> > I would have agreed with you if MIPS didn't have that special
> > _page_cachable_default variable which is undefined for some platforms
> > and which might be re-defined during the boot-up process, and if
> > MIPS64 didn't have ioremap_prot() always mapping to the uncached
> > region.  But IMO updating ioremap_prot() currently seems more risky
> > than just converting dmi_early_remap() to the uncached version
> > especially seeing it won't change anything. MIPS64 always have IO
> > remapped to the uncached region. MIPS32 won't be able to have cached
> > mapping until VM is available, and paging and slabs are initialized.
> > So on the early MIPS32 bootup stages ioremap_cache() wouldn't have
> > worked anyway.
> 

> I really didn't get that, using CKSEG0 on 32bit system and TO_CAC
> on 64bit system won't hurt.
> 
> Something like:
> #ifdef CONFIG_64BIT
> #define dmi_remap(x, l)		(void *)TO_CAC(x)
> #else
> #define dmi_remap(x, l)		(void *)CKSEG0(x)
> #endif
> 
> Can help us avoid all the hassle. Since it always ensures we are
> using same CCA to access DMI tables. We can always trust Config.K0
> left by firmware in this case.

Please note my only concern is about dmi_early_remap(), not
dmi_remap(). The later one can be safely left backended by the
ioremap_cache() method because at the stage it's utilized MIPS32
version of ioremap_prot() will be able to create any mapping it's
requested to. The dmi_early_remap() function is called very early with
no paging or VM or even cache stuff initialized. So currently AFAICS
it just doesn't work on _all_ _MIPS32_ platform, because
ioremap_prot() relies on VM and slab being available to have any
cacheable mapping, which aren't at the moment of the dmi_setup()
function invocation. Seeing the ioremap_cache() is just a stub on
MIPS64 which always performs the uncached mapping, it will be
completely safe to just convert dmi_early_remap() to ioremap() with
no risk to beak anything. dmi_early_remap() semantics won't be
actually changed, it will work as before on MIPS64 and will be fixed
on MIPS32. This (AFAICS) is a completely safe fix of the problem with
just a few affected platforms around.

Getting back to what you suggest. You want to change the
ioremap_prot() semantics so one would return a pointer to the cached
unmapped region for the ioremap_cache() method. First of all
ioremap_cache() doesn't define what type of the cached mapping it
needs but merely relies on the _page_cachable_default variable value.
That variable is uninitialized on the early stages and then only
initialized for the r4k platforms (this makes me also thinking that
ioremap_cache() doesn't properly work for r3k and Octeon platforms),
thus we would need to have it initialized with some value until the
cpu_cache_init() is called and have the r3k and Octen cache init
functions fixed to get it back to the uninitialized zero value .
Moreover all the _CACHE_* field values are already occupied. What
default value should be use then for _page_cachable_default? You say
to read Config.K0 earlier, but Config.K0 may be changed later in the
framework of the cps_smp_setup() method and actually in
cpu_cache_init() for r4k if 'cca' kernel parameter is specified. So do
we need _page_cachable_default being re-initialized then?.. There
might be some other underwater rocks in the fix you suggest. But all
of that already makes your solution much more risky than the one
described before.

Howbeit if you still think that none of the concerns listed above is
worth being that much worried about, then please note your solution is
mainly targeted to fix ioremap_cache(). Meanwhile this patch is about
the DMI region mapping. So if ioremap_cache() needs to be fixed in a
way you suggest it's better to be done in a framework of another
patch. But considering the possible problems it may cause I wouldn't
risk to have it backported to the stable kernels.

-Serge(y)

> 
> You may add some sanity check on 32 bit to avoid generating invalid
> pointer. (And perhaps implement it as ioremap_early.....) 
> 
> Thanks
> -- 
> - Jiaxun

Powered by blists - more mailing lists