[5/8] arm64: memory: Simplify _VA_START and _PAGE_OFFSET definitions
diff mbox series

Message ID 20190813170149.26037-6-will@kernel.org
State New
Headers show
Series
  • Fix issues with 52-bit kernel virtual addressing
Related show

Commit Message

Will Deacon Aug. 13, 2019, 5:01 p.m. UTC
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(-)

Comments

Steve Capper Aug. 13, 2019, 6:55 p.m. UTC | #1
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>

Reviewed-by: Steve Capper <steve.capper@arm.com>

> ---
>  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)))
>  
>  #define KERNEL_START      _text
>  #define KERNEL_END        _end
> -- 
> 2.11.0
>
Catalin Marinas Aug. 14, 2019, 9:33 a.m. UTC | #2
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>

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Mark Rutland Aug. 14, 2019, 11:23 a.m. UTC | #3
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?

Thanks,
Mark.
Will Deacon Aug. 14, 2019, noon UTC | #4
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
Mark Rutland Aug. 14, 2019, 1:18 p.m. UTC | #5
On Wed, Aug 14, 2019 at 01:00:00PM +0100, Will Deacon wrote:
> 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.

Yes; you're right about ptdump_init().

> 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...

Sure, I'll post a couple of patches momentarily...

Mark.

Patch
diff mbox series

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)))
 
 #define KERNEL_START      _text
 #define KERNEL_END        _end