| Message ID | 20190813170149.26037-6-will@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix issues with 52-bit kernel virtual addressing | expand |
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 >
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>
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.
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
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.
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
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(-)