Re: [PATCH 5/8] arm64: memory: Simplify _VA_START and _PAGE_OFFSET definitions

From: Will Deacon <will@kernel.org>
To: Mark Rutland <mark.rutland@arm.com>
Cc: Steve Capper <steve.capper@arm.com>,
	Andrey Konovalov <andreyknvl@google.com>,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	Catalin Marinas <catalin.marinas@arm.com>, Qian Cai <cai@lca.pw>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 5/8] arm64: memory: Simplify _VA_START and _PAGE_OFFSET definitions
Date: Wed, 14 Aug 2019 13:00:00 +0100	[thread overview]
Message-ID: <20190814115959.7epzszx53bidti7m@willie-the-truck> (raw)
In-Reply-To: <20190814112337.GB17931@lakrids.cambridge.arm.com>

On Wed, Aug 14, 2019 at 12:23:39PM +0100, Mark Rutland wrote:
> On Tue, Aug 13, 2019 at 06:01:46PM +0100, Will Deacon wrote:
> > Rather than subtracting from -1 and then adding 1, we can simply
> > subtract from 0.
> > 
> > Cc: Steve Capper <steve.capper@arm.com>
> > Signed-off-by: Will Deacon <will@kernel.org>
> > ---
> >  arch/arm64/include/asm/memory.h | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> > index 56be462c69ce..5552c8cba1e2 100644
> > --- a/arch/arm64/include/asm/memory.h
> > +++ b/arch/arm64/include/asm/memory.h
> > @@ -44,8 +44,7 @@
> >   * VA_START - the first kernel virtual address.
> >   */
> >  #define VA_BITS			(CONFIG_ARM64_VA_BITS)
> > -#define _PAGE_OFFSET(va)	(UL(0xffffffffffffffff) - \
> > -					(UL(1) << (va)) + 1)
> > +#define _PAGE_OFFSET(va)	(-(UL(1) << (va)))
> >  #define PAGE_OFFSET		(_PAGE_OFFSET(VA_BITS))
> >  #define KIMAGE_VADDR		(MODULES_END)
> >  #define BPF_JIT_REGION_START	(KASAN_SHADOW_END)
> > @@ -63,8 +62,7 @@
> >  #else
> >  #define VA_BITS_MIN		(VA_BITS)
> >  #endif
> > -#define _VA_START(va)		(UL(0xffffffffffffffff) - \
> > -				(UL(1) << ((va) - 1)) + 1)
> > +#define _VA_START(va)		(-(UL(1) << ((va) - 1)))
> 
> This didn't make any sense to me until I realised that we changed the
> meaning of VA_START when flippnig the VA space. Given that, this cleanup
> looks sound to me.
> 
> However...
> 
> VA_START used to be the start of the TTBR1 address space, which was what
> the "first kernel virtual address" comment was trying to say. Now it's
> the first non-linear kernel virtual addres, which I think is very
> confusing.
> 
> AFAICT, that change breaks at least:
> 
> * is_ttbr1_addr() -- now returns false for linear map addresses
> * ptdump_check_wx() -- now skips the linear map
> * ptdump_init() -- initialises start_address inccorrectly.
> 
> ... so could we please find a new name for the first non-linear address,
> e.g. PAGE_END, and leave VA_START as the first TTBR1 address?

I think VA_START becomes PAGE_END and then things like is_ttbr1_addr()
just refer to PAGE_OFFSET instead. ptdump_init() looks ok to me, but I could
be missing something.

Anyway, these seem to be comments on the original patches from Steve rather
than my fixes, so please send additional fixes on top. I'll push out an
updated branch for you to work with...

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel