Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 13 Jul 2022 10:01:28 +0300
From:   Serge Semin <fancer.lancer@gmail.com>
To:     Florian Fainelli <f.fainelli@gmail.com>
Cc:     linux-mips@vger.kernel.org, gerg@kernel.org,
        Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] MIPS: Fixed __debug_virt_addr_valid()
Message-ID: <20220713070128.lzcwgimruq7xjadi@mobilestation>
References: <20220707215237.1730283-1-f.fainelli@gmail.com>
 <20220708115851.ejsooiilxcopkoei@mobilestation>
 <a6d8fc0f-38bd-6afe-ef45-42ce3ee6c136@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a6d8fc0f-38bd-6afe-ef45-42ce3ee6c136@gmail.com>
Precedence: bulk

On Tue, Jul 12, 2022 at 09:28:02AM -0700, Florian Fainelli wrote:
> On 7/8/22 04:58, Serge Semin wrote:
> > On Thu, Jul 07, 2022 at 02:52:36PM -0700, Florian Fainelli wrote:
> > > It is permissible for kernel code to call virt_to_phys() against virtual
> > > addresses that are in KSEG0 or KSEG1 and we need to be dealing with both
> > > types. Add a final condition that ensures that the virtual address is
> > > below KSEG2.
> > > 
> > > Fixes: dfad83cb7193 ("MIPS: Add support for CONFIG_DEBUG_VIRTUAL")
> > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > > ---
> > >   arch/mips/mm/physaddr.c | 3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/mips/mm/physaddr.c b/arch/mips/mm/physaddr.c
> > > index a1ced5e44951..a82f8f57a652 100644
> > > --- a/arch/mips/mm/physaddr.c
> > > +++ b/arch/mips/mm/physaddr.c
> > > @@ -5,6 +5,7 @@
> > >   #include <linux/mmdebug.h>
> > >   #include <linux/mm.h>
> > > +#include <asm/addrspace.h>
> > >   #include <asm/sections.h>
> > >   #include <asm/io.h>
> > >   #include <asm/page.h>
> > > @@ -30,7 +31,7 @@ static inline bool __debug_virt_addr_valid(unsigned long x)
> > >   	if (x == MAX_DMA_ADDRESS)
> > >   		return true;
> > 
> > > -	return false;
> > > +	return KSEGX(x) < KSEG2;
> > 
> > With this do we really need the high_memory-based conditionals in this
> > method?
> > 
> > If the line above is the only way to take the uncached segment into
> > account then can we reduce the whole method to:
> > static inline bool __debug_virt_addr_valid {
> > 	return x >= PAGE_OFFSET && KSEGX(x) < KSEG2;
> > }
> > ?
> > 
> > Though this still may be invalid for EVA systems, like malta (see
> > arch/mips/include/asm/mach-malta/spaces.h).
> > 
> > Note AFAICS if EVA is enabled, highmem is implied to be disabled (see
> > the CPU_MIPS32_3_5_EVA config utilization and HIGHMEM config
> > dependencies). Thus all the memory is supposed to be linearly mapped
> > in that case.
> 

> OK, so if all of the memory is linearly mapped, then I am not too sure what
> we will be able to check, which is in essence pretty similar to what happens
> on MIPS64, right?

Essence is right, but in general situation is more complicated.
Basically EVA (seems like a Bible reference...) provides a way to
change the traditional MIPS memory address space to pretty much any
within 4GB virtual-to-physical address mapping. Most importantly it
can be used to eliminate the limitation of having just 512MB of the
directly mapped memory in kernel.

Anyway neither MIPS HIGHMEM kernel config nor the Malta's EVA mode
don't imply having high-memory enabled in case of EVAs. Most likely
the constraint has been set due to the MIPS arch code too much relying
on the traditional address space mapping. So the high-memory part just
wasn't fixed to be properly working in that case, while the CPU
MMU-type segments can still be defined for EVA. As the comment in the
Malta spaces.h header file says HIGHMEM_START is preserved just for
the correct high-mem macros arithmetics.

Just to note. Lack of high-memory in case of EVA is a big drawback
because some physical memory gets to be unavailable. At the very least
512MB segment must be preserved for the uncached kernel region for
MMIOs. Not to say that XPA won't work without high-memory. So it's
either XPA (greater than 4GB physical memory) or EVA (up to 3.5GB
directly mapped kernel memory). So annoying.

> 
> Maybe DEBUG_VIRTUAL should depend on !EVA as well?

In that case the debug-version of __phys_addr_symbol() will be
unavailable too. I would rather suggest to fix the
__debug_virt_addr_valid() method implementation to at least returning
always true in case of EVA or !HIGHMEM.

-Sergey

> -- 
> Florian