* [PATCH 1/2] ARM: mmu64: allow to disable null pointer trap on zero page @ 2020-10-14 15:08 Michael Tretter 2020-10-14 15:08 ` [PATCH 2/2] uimage: disable zero page when loading to SDRAM at address 0x0 Michael Tretter 2020-10-14 16:29 ` [PATCH 1/2] ARM: mmu64: allow to disable null pointer trap on zero page Ahmad Fatoum 0 siblings, 2 replies; 10+ messages in thread From: Michael Tretter @ 2020-10-14 15:08 UTC (permalink / raw) To: barebox; +Cc: Michael Tretter Barebox uses the zero page to trap NULL pointer dereferences. However, if the SDRAM starts at address 0x0, this makes the first page of the SDRAM inaccessible and makes it impossible to load images to offset 0x0 in the SDRAM. Trapping NULL pointer dereferences on such systems is still desirable. Therefore, add a function to disable the traps if accessing the zero page is necessary and to re-enable the traps after the access is done. Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> --- arch/arm/cpu/Kconfig | 1 + arch/arm/cpu/mmu_64.c | 13 ++++++++++++- include/zero_page.h | 40 ++++++++++++++++++++++++++++++++++++++++ lib/Kconfig | 3 +++ 4 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 include/zero_page.h diff --git a/arch/arm/cpu/Kconfig b/arch/arm/cpu/Kconfig index f9f52a625260..ca3bd98962e2 100644 --- a/arch/arm/cpu/Kconfig +++ b/arch/arm/cpu/Kconfig @@ -89,6 +89,7 @@ config CPU_V8 select ARM_EXCEPTIONS select GENERIC_FIND_NEXT_BIT select ARCH_HAS_STACK_DUMP + select ARCH_HAS_ZERO_PAGE config CPU_XSC3 bool diff --git a/arch/arm/cpu/mmu_64.c b/arch/arm/cpu/mmu_64.c index 7e9ae84810f6..bd15807f9160 100644 --- a/arch/arm/cpu/mmu_64.c +++ b/arch/arm/cpu/mmu_64.c @@ -10,6 +10,7 @@ #include <init.h> #include <mmu.h> #include <errno.h> +#include <zero_page.h> #include <linux/sizes.h> #include <asm/memory.h> #include <asm/pgtable64.h> @@ -168,6 +169,16 @@ static void mmu_enable(void) set_cr(get_cr() | CR_M | CR_C | CR_I); } +void zero_page_disable(void) +{ + create_sections(0x0, 0x0, PAGE_SIZE, CACHED_MEM); +} + +void zero_page_enable(void) +{ + create_sections(0x0, 0x0, PAGE_SIZE, 0x0); +} + /* * Prepare MMU for usage enable it. */ @@ -194,7 +205,7 @@ void __mmu_init(bool mmu_on) create_sections(bank->start, bank->start, bank->size, CACHED_MEM); /* Make zero page faulting to catch NULL pointer derefs */ - create_sections(0x0, 0x0, 0x1000, 0x0); + zero_page_enable(); mmu_enable(); } diff --git a/include/zero_page.h b/include/zero_page.h new file mode 100644 index 000000000000..d8dd07cfe959 --- /dev/null +++ b/include/zero_page.h @@ -0,0 +1,40 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +#ifndef __ZERO_PAGE_H +#define __ZERO_PAGE_H + +#include <common.h> + +#if defined CONFIG_ARCH_HAS_ZERO_PAGE + +/* + * zero_page_enable - enable null pointer trap + */ +void zero_page_enable(void); + +/* + * zero_page_disable - disable null pointer trap + * + * Disable the null pointer trap on the zero page if access to the zero page + * is actually required. Disable the trap with care and re-enable it + * immediately after the access to properly trap null pointers. + */ +void zero_page_disable(void); + +#else + +static inline void zero_page_enable(void) +{ +} + +static inline void zero_page_disable(void) +{ +} + +#endif + +static inline bool zero_page_contains(unsigned long addr) +{ + return addr < PAGE_SIZE; +} + +#endif /* __ZERO_PAGE_H */ diff --git a/lib/Kconfig b/lib/Kconfig index 887f50ff003f..e5831ecdb9a7 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -182,6 +182,9 @@ config ARCH_HAS_STACK_DUMP config ARCH_HAS_DATA_ABORT_MASK bool +config ARCH_HAS_ZERO_PAGE + bool + config HAVE_EFFICIENT_UNALIGNED_ACCESS bool -- 2.20.1 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] uimage: disable zero page when loading to SDRAM at address 0x0 2020-10-14 15:08 [PATCH 1/2] ARM: mmu64: allow to disable null pointer trap on zero page Michael Tretter @ 2020-10-14 15:08 ` Michael Tretter 2020-10-14 16:33 ` Ahmad Fatoum 2020-10-14 16:29 ` [PATCH 1/2] ARM: mmu64: allow to disable null pointer trap on zero page Ahmad Fatoum 1 sibling, 1 reply; 10+ messages in thread From: Michael Tretter @ 2020-10-14 15:08 UTC (permalink / raw) To: barebox; +Cc: Michael Tretter If the SDRAM is mapped to address 0x0 and an image should be loaded to to the SDRAM without offset, Barebox would normally trap the access as a null pointer. However, since Linux kernel commit cfa7ede20f13 ("arm64: set TEXT_OFFSET to 0x0 in preparation for removing it entirely") no offset is the default for arm64. Therefore, copying the image to 0x0 of the SDRAM is necessary. Disable the zero page trap for copying an image to address 0x0. Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> --- common/uimage.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/common/uimage.c b/common/uimage.c index a84b8fddc4e7..b1e9b402e98a 100644 --- a/common/uimage.c +++ b/common/uimage.c @@ -27,6 +27,7 @@ #include <rtc.h> #include <filetype.h> #include <memory.h> +#include <zero_page.h> static inline int uimage_is_multi_image(struct uimage_handle *handle) { @@ -359,7 +360,13 @@ static int uimage_sdram_flush(void *buf, unsigned int len) } } - memcpy(uimage_buf + uimage_size, buf, len); + if (zero_page_contains((unsigned long)uimage_buf + uimage_size)) { + zero_page_disable(); + memcpy(uimage_buf + uimage_size, buf, len); + zero_page_enable(); + } else { + memcpy(uimage_buf + uimage_size, buf, len); + } uimage_size += len; @@ -388,7 +395,14 @@ struct resource *file_to_sdram(const char *filename, unsigned long adr) goto out; } - now = read_full(fd, (void *)(res->start + ofs), BUFSIZ); + if (zero_page_contains(res->start + ofs)) { + zero_page_disable(); + now = read_full(fd, (void *)(res->start + ofs), BUFSIZ); + zero_page_enable(); + } else { + now = read_full(fd, (void *)(res->start + ofs), BUFSIZ); + } + if (now < 0) { release_sdram_region(res); res = NULL; -- 2.20.1 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] uimage: disable zero page when loading to SDRAM at address 0x0 2020-10-14 15:08 ` [PATCH 2/2] uimage: disable zero page when loading to SDRAM at address 0x0 Michael Tretter @ 2020-10-14 16:33 ` Ahmad Fatoum 2020-10-15 7:40 ` Michael Tretter 0 siblings, 1 reply; 10+ messages in thread From: Ahmad Fatoum @ 2020-10-14 16:33 UTC (permalink / raw) To: Michael Tretter, barebox On 10/14/20 5:08 PM, Michael Tretter wrote: > If the SDRAM is mapped to address 0x0 and an image should be loaded to > to the SDRAM without offset, Barebox would normally trap the access as a > null pointer. > > However, since Linux kernel commit cfa7ede20f13 ("arm64: set TEXT_OFFSET > to 0x0 in preparation for removing it entirely") no offset is the > default for arm64. Therefore, copying the image to 0x0 of the SDRAM is > necessary. > > Disable the zero page trap for copying an image to address 0x0. > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > --- > common/uimage.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/common/uimage.c b/common/uimage.c > index a84b8fddc4e7..b1e9b402e98a 100644 > --- a/common/uimage.c > +++ b/common/uimage.c > @@ -27,6 +27,7 @@ > #include <rtc.h> > #include <filetype.h> > #include <memory.h> > +#include <zero_page.h> > > static inline int uimage_is_multi_image(struct uimage_handle *handle) > { > @@ -359,7 +360,13 @@ static int uimage_sdram_flush(void *buf, unsigned int len) > } > } > > - memcpy(uimage_buf + uimage_size, buf, len); > + if (zero_page_contains((unsigned long)uimage_buf + uimage_size)) { > + zero_page_disable(); > + memcpy(uimage_buf + uimage_size, buf, len); > + zero_page_enable(); If this remains, please add a memcpy_notrap or something. > + } else { > + memcpy(uimage_buf + uimage_size, buf, len); > + } > > uimage_size += len; > > @@ -388,7 +395,14 @@ struct resource *file_to_sdram(const char *filename, unsigned long adr) > goto out; > } > > - now = read_full(fd, (void *)(res->start + ofs), BUFSIZ); > + if (zero_page_contains(res->start + ofs)) { > + zero_page_disable(); > + now = read_full(fd, (void *)(res->start + ofs), BUFSIZ); > + zero_page_enable(); And use that new memcpy_notrap here to copy from an intermediate buffer. You open quite a can of worms when you treat NULL as a valid address. Better have this contained in a single file instead of hoping the compiler doesn't do a NULL-can't-happen-here optimization in all that block/cdev/fs code that read_full may call into. > + } else { > + now = read_full(fd, (void *)(res->start + ofs), BUFSIZ); > + } > + > if (now < 0) { > release_sdram_region(res); > res = NULL; > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] uimage: disable zero page when loading to SDRAM at address 0x0 2020-10-14 16:33 ` Ahmad Fatoum @ 2020-10-15 7:40 ` Michael Tretter 2020-10-15 8:35 ` Sascha Hauer 2020-10-15 9:12 ` Ahmad Fatoum 0 siblings, 2 replies; 10+ messages in thread From: Michael Tretter @ 2020-10-15 7:40 UTC (permalink / raw) To: Ahmad Fatoum; +Cc: barebox On Wed, 14 Oct 2020 18:33:25 +0200, Ahmad Fatoum wrote: > On 10/14/20 5:08 PM, Michael Tretter wrote: > > If the SDRAM is mapped to address 0x0 and an image should be loaded to > > to the SDRAM without offset, Barebox would normally trap the access as a > > null pointer. > > > > However, since Linux kernel commit cfa7ede20f13 ("arm64: set TEXT_OFFSET > > to 0x0 in preparation for removing it entirely") no offset is the > > default for arm64. Therefore, copying the image to 0x0 of the SDRAM is > > necessary. > > > > Disable the zero page trap for copying an image to address 0x0. > > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > > --- > > common/uimage.c | 18 ++++++++++++++++-- > > 1 file changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/common/uimage.c b/common/uimage.c > > index a84b8fddc4e7..b1e9b402e98a 100644 > > --- a/common/uimage.c > > +++ b/common/uimage.c > > @@ -27,6 +27,7 @@ > > #include <rtc.h> > > #include <filetype.h> > > #include <memory.h> > > +#include <zero_page.h> > > > > static inline int uimage_is_multi_image(struct uimage_handle *handle) > > { > > @@ -359,7 +360,13 @@ static int uimage_sdram_flush(void *buf, unsigned int len) > > } > > } > > > > - memcpy(uimage_buf + uimage_size, buf, len); > > + if (zero_page_contains((unsigned long)uimage_buf + uimage_size)) { > > + zero_page_disable(); > > + memcpy(uimage_buf + uimage_size, buf, len); > > + zero_page_enable(); > > If this remains, please add a memcpy_notrap or something. Should I check the destination before calling memcpy_notrap or should I always call the memcpy_notrap if there is a possibility to copy to 0x0 and check for the destination within the function? I fear that having such a "simple" function would encourage to use it more often. I would prefer to make the code to use it more clumsy and make it (similar to data_abort_mask()) the responsibility of the caller to be aware that bad things might happen when the zero_page is disabled. > > > + } else { > > + memcpy(uimage_buf + uimage_size, buf, len); > > + } > > > > uimage_size += len; > > > > @@ -388,7 +395,14 @@ struct resource *file_to_sdram(const char *filename, unsigned long adr) > > goto out; > > } > > > > - now = read_full(fd, (void *)(res->start + ofs), BUFSIZ); > > + if (zero_page_contains(res->start + ofs)) { > > + zero_page_disable(); > > + now = read_full(fd, (void *)(res->start + ofs), BUFSIZ); > > + zero_page_enable(); > > And use that new memcpy_notrap here to copy from an intermediate buffer. You open quite a can > of worms when you treat NULL as a valid address. Better have this contained in a single > file instead of hoping the compiler doesn't do a NULL-can't-happen-here optimization > in all that block/cdev/fs code that read_full may call into. Could you explain, what kind of optimization you would expect? Michael > > > + } else { > > + now = read_full(fd, (void *)(res->start + ofs), BUFSIZ); > > + } > > + > > if (now < 0) { > > release_sdram_region(res); > > res = NULL; > > _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] uimage: disable zero page when loading to SDRAM at address 0x0 2020-10-15 7:40 ` Michael Tretter @ 2020-10-15 8:35 ` Sascha Hauer 2020-10-15 9:12 ` Ahmad Fatoum 1 sibling, 0 replies; 10+ messages in thread From: Sascha Hauer @ 2020-10-15 8:35 UTC (permalink / raw) To: Michael Tretter; +Cc: barebox, Ahmad Fatoum On Thu, Oct 15, 2020 at 09:40:05AM +0200, Michael Tretter wrote: > On Wed, 14 Oct 2020 18:33:25 +0200, Ahmad Fatoum wrote: > > On 10/14/20 5:08 PM, Michael Tretter wrote: > > > If the SDRAM is mapped to address 0x0 and an image should be loaded to > > > to the SDRAM without offset, Barebox would normally trap the access as a > > > null pointer. > > > > > > However, since Linux kernel commit cfa7ede20f13 ("arm64: set TEXT_OFFSET > > > to 0x0 in preparation for removing it entirely") no offset is the > > > default for arm64. Therefore, copying the image to 0x0 of the SDRAM is > > > necessary. > > > > > > Disable the zero page trap for copying an image to address 0x0. > > > > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > > > --- > > > common/uimage.c | 18 ++++++++++++++++-- > > > 1 file changed, 16 insertions(+), 2 deletions(-) > > > > > > diff --git a/common/uimage.c b/common/uimage.c > > > index a84b8fddc4e7..b1e9b402e98a 100644 > > > --- a/common/uimage.c > > > +++ b/common/uimage.c > > > @@ -27,6 +27,7 @@ > > > #include <rtc.h> > > > #include <filetype.h> > > > #include <memory.h> > > > +#include <zero_page.h> > > > > > > static inline int uimage_is_multi_image(struct uimage_handle *handle) > > > { > > > @@ -359,7 +360,13 @@ static int uimage_sdram_flush(void *buf, unsigned int len) > > > } > > > } > > > > > > - memcpy(uimage_buf + uimage_size, buf, len); > > > + if (zero_page_contains((unsigned long)uimage_buf + uimage_size)) { > > > + zero_page_disable(); > > > + memcpy(uimage_buf + uimage_size, buf, len); > > > + zero_page_enable(); > > > > If this remains, please add a memcpy_notrap or something. > > Should I check the destination before calling memcpy_notrap or should I always > call the memcpy_notrap if there is a possibility to copy to 0x0 and check for > the destination within the function? > > I fear that having such a "simple" function would encourage to use it more > often. I would prefer to make the code to use it more clumsy and make it > (similar to data_abort_mask()) the responsibility of the caller to be aware > that bad things might happen when the zero_page is disabled. That's more a reason to hide that behind a function, because then you can easily catch and if necessary fix all users. Way easier than fixing all open coded places. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] uimage: disable zero page when loading to SDRAM at address 0x0 2020-10-15 7:40 ` Michael Tretter 2020-10-15 8:35 ` Sascha Hauer @ 2020-10-15 9:12 ` Ahmad Fatoum 1 sibling, 0 replies; 10+ messages in thread From: Ahmad Fatoum @ 2020-10-15 9:12 UTC (permalink / raw) To: Michael Tretter; +Cc: barebox Hello, On 10/15/20 9:40 AM, Michael Tretter wrote: > On Wed, 14 Oct 2020 18:33:25 +0200, Ahmad Fatoum wrote: >> On 10/14/20 5:08 PM, Michael Tretter wrote: >>> If the SDRAM is mapped to address 0x0 and an image should be loaded to >>> to the SDRAM without offset, Barebox would normally trap the access as a >>> null pointer. >>> >>> However, since Linux kernel commit cfa7ede20f13 ("arm64: set TEXT_OFFSET >>> to 0x0 in preparation for removing it entirely") no offset is the >>> default for arm64. Therefore, copying the image to 0x0 of the SDRAM is >>> necessary. >>> >>> Disable the zero page trap for copying an image to address 0x0. >>> >>> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> >>> --- >>> common/uimage.c | 18 ++++++++++++++++-- >>> 1 file changed, 16 insertions(+), 2 deletions(-) >>> >>> diff --git a/common/uimage.c b/common/uimage.c >>> index a84b8fddc4e7..b1e9b402e98a 100644 >>> --- a/common/uimage.c >>> +++ b/common/uimage.c >>> @@ -27,6 +27,7 @@ >>> #include <rtc.h> >>> #include <filetype.h> >>> #include <memory.h> >>> +#include <zero_page.h> >>> >>> static inline int uimage_is_multi_image(struct uimage_handle *handle) >>> { >>> @@ -359,7 +360,13 @@ static int uimage_sdram_flush(void *buf, unsigned int len) >>> } >>> } >>> >>> - memcpy(uimage_buf + uimage_size, buf, len); >>> + if (zero_page_contains((unsigned long)uimage_buf + uimage_size)) { >>> + zero_page_disable(); >>> + memcpy(uimage_buf + uimage_size, buf, len); >>> + zero_page_enable(); >> >> If this remains, please add a memcpy_notrap or something. > > Should I check the destination before calling memcpy_notrap or should I always > call the memcpy_notrap if there is a possibility to copy to 0x0 and check for > the destination within the function? > > I fear that having such a "simple" function would encourage to use it more > often. I would prefer to make the code to use it more clumsy and make it > (similar to data_abort_mask()) the responsibility of the caller to be aware > that bad things might happen when the zero_page is disabled. Give it a scary name then. > >> >>> + } else { >>> + memcpy(uimage_buf + uimage_size, buf, len); >>> + } >>> >>> uimage_size += len; >>> >>> @@ -388,7 +395,14 @@ struct resource *file_to_sdram(const char *filename, unsigned long adr) >>> goto out; >>> } >>> >>> - now = read_full(fd, (void *)(res->start + ofs), BUFSIZ); >>> + if (zero_page_contains(res->start + ofs)) { >>> + zero_page_disable(); >>> + now = read_full(fd, (void *)(res->start + ofs), BUFSIZ); >>> + zero_page_enable(); >> >> And use that new memcpy_notrap here to copy from an intermediate buffer. You open quite a can >> of worms when you treat NULL as a valid address. Better have this contained in a single >> file instead of hoping the compiler doesn't do a NULL-can't-happen-here optimization >> in all that block/cdev/fs code that read_full may call into. > > Could you explain, what kind of optimization you would expect? Real world example: https://lwn.net/Articles/342330/ Kernel and barebox both have -fno-delete-null-pointer-checks to avoid this, but you can't be sure that other null pointer optimizations are avoided. For example, the transformation of static void func(unsigned *ptr, bool x) { if (!x) return; *ptr = 0xDEADBEEF; } void f(void *ptr, bool x) { func(ptr, x); } to static void func(unsigned *ptr) { if (!ptr) return; *ptr = 0xDEADBEEF } void f(void *ptr, bool x) { if (!x) ptr = NULL; func(ptr); } is valid as far the standard is concerned. Better play it safe and reduce the surface where optimization can go awry. Cheers, Ahmad > > Michael > >> >>> + } else { >>> + now = read_full(fd, (void *)(res->start + ofs), BUFSIZ); >>> + } >>> + >>> if (now < 0) { >>> release_sdram_region(res); >>> res = NULL; >>> > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] ARM: mmu64: allow to disable null pointer trap on zero page 2020-10-14 15:08 [PATCH 1/2] ARM: mmu64: allow to disable null pointer trap on zero page Michael Tretter 2020-10-14 15:08 ` [PATCH 2/2] uimage: disable zero page when loading to SDRAM at address 0x0 Michael Tretter @ 2020-10-14 16:29 ` Ahmad Fatoum [not found] ` <20201015073331.GA29491@pengutronix.de> 2020-10-15 8:44 ` Sascha Hauer 1 sibling, 2 replies; 10+ messages in thread From: Ahmad Fatoum @ 2020-10-14 16:29 UTC (permalink / raw) To: Michael Tretter, barebox On 10/14/20 5:08 PM, Michael Tretter wrote: > Barebox uses the zero page to trap NULL pointer dereferences. However, > if the SDRAM starts at address 0x0, this makes the first page of the > SDRAM inaccessible and makes it impossible to load images to offset 0x0 > in the SDRAM. > > Trapping NULL pointer dereferences on such systems is still desirable. > Therefore, add a function to disable the traps if accessing the zero > page is necessary and to re-enable the traps after the access is done. Can't we map the (phy_addr_t)0 at some higher virtual address and change uimage to use phys_to_virt() ? Something like static inline void *phys_to_virt(unsigned long phys) { if (!IS_ENABLED(CONFIG_ARM_MACH_CUSTOM_MAPPING) || !arm_mach_phys_to_virt) return (void *)phys; return arm_mach_phys_to_virt(phys); } > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > --- > arch/arm/cpu/Kconfig | 1 + > arch/arm/cpu/mmu_64.c | 13 ++++++++++++- > include/zero_page.h | 40 ++++++++++++++++++++++++++++++++++++++++ > lib/Kconfig | 3 +++ > 4 files changed, 56 insertions(+), 1 deletion(-) > create mode 100644 include/zero_page.h > > diff --git a/arch/arm/cpu/Kconfig b/arch/arm/cpu/Kconfig > index f9f52a625260..ca3bd98962e2 100644 > --- a/arch/arm/cpu/Kconfig > +++ b/arch/arm/cpu/Kconfig > @@ -89,6 +89,7 @@ config CPU_V8 > select ARM_EXCEPTIONS > select GENERIC_FIND_NEXT_BIT > select ARCH_HAS_STACK_DUMP > + select ARCH_HAS_ZERO_PAGE > > config CPU_XSC3 > bool > diff --git a/arch/arm/cpu/mmu_64.c b/arch/arm/cpu/mmu_64.c > index 7e9ae84810f6..bd15807f9160 100644 > --- a/arch/arm/cpu/mmu_64.c > +++ b/arch/arm/cpu/mmu_64.c > @@ -10,6 +10,7 @@ > #include <init.h> > #include <mmu.h> > #include <errno.h> > +#include <zero_page.h> > #include <linux/sizes.h> > #include <asm/memory.h> > #include <asm/pgtable64.h> > @@ -168,6 +169,16 @@ static void mmu_enable(void) > set_cr(get_cr() | CR_M | CR_C | CR_I); > } > > +void zero_page_disable(void) > +{ > + create_sections(0x0, 0x0, PAGE_SIZE, CACHED_MEM); > +} > + > +void zero_page_enable(void) > +{ > + create_sections(0x0, 0x0, PAGE_SIZE, 0x0); > +} > + > /* > * Prepare MMU for usage enable it. > */ > @@ -194,7 +205,7 @@ void __mmu_init(bool mmu_on) > create_sections(bank->start, bank->start, bank->size, CACHED_MEM); > > /* Make zero page faulting to catch NULL pointer derefs */ > - create_sections(0x0, 0x0, 0x1000, 0x0); > + zero_page_enable(); > > mmu_enable(); > } > diff --git a/include/zero_page.h b/include/zero_page.h > new file mode 100644 > index 000000000000..d8dd07cfe959 > --- /dev/null > +++ b/include/zero_page.h > @@ -0,0 +1,40 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +#ifndef __ZERO_PAGE_H > +#define __ZERO_PAGE_H > + > +#include <common.h> > + > +#if defined CONFIG_ARCH_HAS_ZERO_PAGE > + > +/* > + * zero_page_enable - enable null pointer trap > + */ > +void zero_page_enable(void); > + > +/* > + * zero_page_disable - disable null pointer trap > + * > + * Disable the null pointer trap on the zero page if access to the zero page > + * is actually required. Disable the trap with care and re-enable it > + * immediately after the access to properly trap null pointers. > + */ > +void zero_page_disable(void); > + > +#else > + > +static inline void zero_page_enable(void) > +{ > +} > + > +static inline void zero_page_disable(void) > +{ > +} > + > +#endif > + > +static inline bool zero_page_contains(unsigned long addr) > +{ > + return addr < PAGE_SIZE; > +} > + > +#endif /* __ZERO_PAGE_H */ > diff --git a/lib/Kconfig b/lib/Kconfig > index 887f50ff003f..e5831ecdb9a7 100644 > --- a/lib/Kconfig > +++ b/lib/Kconfig > @@ -182,6 +182,9 @@ config ARCH_HAS_STACK_DUMP > config ARCH_HAS_DATA_ABORT_MASK > bool > > +config ARCH_HAS_ZERO_PAGE > + bool > + > config HAVE_EFFICIENT_UNALIGNED_ACCESS > bool > > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <20201015073331.GA29491@pengutronix.de>]
* Re: [PATCH 1/2] ARM: mmu64: allow to disable null pointer trap on zero page [not found] ` <20201015073331.GA29491@pengutronix.de> @ 2020-10-15 8:14 ` Ahmad Fatoum 2020-10-15 8:40 ` Michael Tretter 0 siblings, 1 reply; 10+ messages in thread From: Ahmad Fatoum @ 2020-10-15 8:14 UTC (permalink / raw) To: Michael Tretter; +Cc: barebox On 10/15/20 9:33 AM, Michael Tretter wrote: > On Wed, 14 Oct 2020 18:29:47 +0200, Ahmad Fatoum wrote: >> On 10/14/20 5:08 PM, Michael Tretter wrote: >>> Barebox uses the zero page to trap NULL pointer dereferences. However, >>> if the SDRAM starts at address 0x0, this makes the first page of the >>> SDRAM inaccessible and makes it impossible to load images to offset 0x0 >>> in the SDRAM. >>> >>> Trapping NULL pointer dereferences on such systems is still desirable. >>> Therefore, add a function to disable the traps if accessing the zero >>> page is necessary and to re-enable the traps after the access is done. >> >> Can't we map the (phy_addr_t)0 at some higher virtual address and >> change uimage to use phys_to_virt() ? > > To which virt would you map (phy_addr_t)0? You are on a 64-bit system. Just choose something > 4G that's suitable for your ZynqMP? > This would break the general virt = > phys assumption in Barebox. It might work for some cases, but might break in > unexpected ways in other. Therefore, I deem it saver to stay with virt = phys > and allow to disable the trap, if necessary. Can you be more specific what kind of problems do you foresee? You map the zero page unaccessible already, so any code that would object to having the page 0 and 1 not be contiguous in memory would already have problems to access page 0. > > Michael > >> >> Something like >> >> static inline void *phys_to_virt(unsigned long phys) >> { >> if (!IS_ENABLED(CONFIG_ARM_MACH_CUSTOM_MAPPING) || !arm_mach_phys_to_virt) >> return (void *)phys; >> return arm_mach_phys_to_virt(phys); >> } >> >> >>> >>> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> >>> --- >>> arch/arm/cpu/Kconfig | 1 + >>> arch/arm/cpu/mmu_64.c | 13 ++++++++++++- >>> include/zero_page.h | 40 ++++++++++++++++++++++++++++++++++++++++ >>> lib/Kconfig | 3 +++ >>> 4 files changed, 56 insertions(+), 1 deletion(-) >>> create mode 100644 include/zero_page.h >>> >>> diff --git a/arch/arm/cpu/Kconfig b/arch/arm/cpu/Kconfig >>> index f9f52a625260..ca3bd98962e2 100644 >>> --- a/arch/arm/cpu/Kconfig >>> +++ b/arch/arm/cpu/Kconfig >>> @@ -89,6 +89,7 @@ config CPU_V8 >>> select ARM_EXCEPTIONS >>> select GENERIC_FIND_NEXT_BIT >>> select ARCH_HAS_STACK_DUMP >>> + select ARCH_HAS_ZERO_PAGE >>> >>> config CPU_XSC3 >>> bool >>> diff --git a/arch/arm/cpu/mmu_64.c b/arch/arm/cpu/mmu_64.c >>> index 7e9ae84810f6..bd15807f9160 100644 >>> --- a/arch/arm/cpu/mmu_64.c >>> +++ b/arch/arm/cpu/mmu_64.c >>> @@ -10,6 +10,7 @@ >>> #include <init.h> >>> #include <mmu.h> >>> #include <errno.h> >>> +#include <zero_page.h> >>> #include <linux/sizes.h> >>> #include <asm/memory.h> >>> #include <asm/pgtable64.h> >>> @@ -168,6 +169,16 @@ static void mmu_enable(void) >>> set_cr(get_cr() | CR_M | CR_C | CR_I); >>> } >>> >>> +void zero_page_disable(void) >>> +{ >>> + create_sections(0x0, 0x0, PAGE_SIZE, CACHED_MEM); >>> +} >>> + >>> +void zero_page_enable(void) >>> +{ >>> + create_sections(0x0, 0x0, PAGE_SIZE, 0x0); >>> +} >>> + >>> /* >>> * Prepare MMU for usage enable it. >>> */ >>> @@ -194,7 +205,7 @@ void __mmu_init(bool mmu_on) >>> create_sections(bank->start, bank->start, bank->size, CACHED_MEM); >>> >>> /* Make zero page faulting to catch NULL pointer derefs */ >>> - create_sections(0x0, 0x0, 0x1000, 0x0); >>> + zero_page_enable(); >>> >>> mmu_enable(); >>> } >>> diff --git a/include/zero_page.h b/include/zero_page.h >>> new file mode 100644 >>> index 000000000000..d8dd07cfe959 >>> --- /dev/null >>> +++ b/include/zero_page.h >>> @@ -0,0 +1,40 @@ >>> +/* SPDX-License-Identifier: GPL-2.0-only */ >>> +#ifndef __ZERO_PAGE_H >>> +#define __ZERO_PAGE_H >>> + >>> +#include <common.h> >>> + >>> +#if defined CONFIG_ARCH_HAS_ZERO_PAGE >>> + >>> +/* >>> + * zero_page_enable - enable null pointer trap >>> + */ >>> +void zero_page_enable(void); >>> + >>> +/* >>> + * zero_page_disable - disable null pointer trap >>> + * >>> + * Disable the null pointer trap on the zero page if access to the zero page >>> + * is actually required. Disable the trap with care and re-enable it >>> + * immediately after the access to properly trap null pointers. >>> + */ >>> +void zero_page_disable(void); >>> + >>> +#else >>> + >>> +static inline void zero_page_enable(void) >>> +{ >>> +} >>> + >>> +static inline void zero_page_disable(void) >>> +{ >>> +} >>> + >>> +#endif >>> + >>> +static inline bool zero_page_contains(unsigned long addr) >>> +{ >>> + return addr < PAGE_SIZE; >>> +} >>> + >>> +#endif /* __ZERO_PAGE_H */ >>> diff --git a/lib/Kconfig b/lib/Kconfig >>> index 887f50ff003f..e5831ecdb9a7 100644 >>> --- a/lib/Kconfig >>> +++ b/lib/Kconfig >>> @@ -182,6 +182,9 @@ config ARCH_HAS_STACK_DUMP >>> config ARCH_HAS_DATA_ABORT_MASK >>> bool >>> >>> +config ARCH_HAS_ZERO_PAGE >>> + bool >>> + >>> config HAVE_EFFICIENT_UNALIGNED_ACCESS >>> bool >>> >>> > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] ARM: mmu64: allow to disable null pointer trap on zero page 2020-10-15 8:14 ` Ahmad Fatoum @ 2020-10-15 8:40 ` Michael Tretter 0 siblings, 0 replies; 10+ messages in thread From: Michael Tretter @ 2020-10-15 8:40 UTC (permalink / raw) To: Ahmad Fatoum; +Cc: barebox On Thu, 15 Oct 2020 10:14:41 +0200, Ahmad Fatoum wrote: > On 10/15/20 9:33 AM, Michael Tretter wrote: > > On Wed, 14 Oct 2020 18:29:47 +0200, Ahmad Fatoum wrote: > >> On 10/14/20 5:08 PM, Michael Tretter wrote: > >>> Barebox uses the zero page to trap NULL pointer dereferences. However, > >>> if the SDRAM starts at address 0x0, this makes the first page of the > >>> SDRAM inaccessible and makes it impossible to load images to offset 0x0 > >>> in the SDRAM. > >>> > >>> Trapping NULL pointer dereferences on such systems is still desirable. > >>> Therefore, add a function to disable the traps if accessing the zero > >>> page is necessary and to re-enable the traps after the access is done. > >> > >> Can't we map the (phy_addr_t)0 at some higher virtual address and > >> change uimage to use phys_to_virt() ? > > > > To which virt would you map (phy_addr_t)0? > > You are on a 64-bit system. Just choose something > 4G that's suitable for > your ZynqMP? This would introduce very platform specific handling and diverge from the shared mmu_64 code. I would rather go for a more general solution. What about 32 bit ARM systems? Currently, the zero page is simply mapped on such systems and NULL pointers will never trap. > > > This would break the general virt = > > phys assumption in Barebox. It might work for some cases, but might break in > > unexpected ways in other. Therefore, I deem it saver to stay with virt = phys > > and allow to disable the trap, if necessary. > > Can you be more specific what kind of problems do you foresee? You map the zero > page unaccessible already, so any code that would object to having the page 0 > and 1 not be contiguous in memory would already have problems to access page 0. For example, memtest accesses page 0 and just remaps it. Michael > > > > > Michael > > > >> > >> Something like > >> > >> static inline void *phys_to_virt(unsigned long phys) > >> { > >> if (!IS_ENABLED(CONFIG_ARM_MACH_CUSTOM_MAPPING) || !arm_mach_phys_to_virt) > >> return (void *)phys; > >> return arm_mach_phys_to_virt(phys); > >> } > >> > >> > >>> > >>> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > >>> --- > >>> arch/arm/cpu/Kconfig | 1 + > >>> arch/arm/cpu/mmu_64.c | 13 ++++++++++++- > >>> include/zero_page.h | 40 ++++++++++++++++++++++++++++++++++++++++ > >>> lib/Kconfig | 3 +++ > >>> 4 files changed, 56 insertions(+), 1 deletion(-) > >>> create mode 100644 include/zero_page.h > >>> > >>> diff --git a/arch/arm/cpu/Kconfig b/arch/arm/cpu/Kconfig > >>> index f9f52a625260..ca3bd98962e2 100644 > >>> --- a/arch/arm/cpu/Kconfig > >>> +++ b/arch/arm/cpu/Kconfig > >>> @@ -89,6 +89,7 @@ config CPU_V8 > >>> select ARM_EXCEPTIONS > >>> select GENERIC_FIND_NEXT_BIT > >>> select ARCH_HAS_STACK_DUMP > >>> + select ARCH_HAS_ZERO_PAGE > >>> > >>> config CPU_XSC3 > >>> bool > >>> diff --git a/arch/arm/cpu/mmu_64.c b/arch/arm/cpu/mmu_64.c > >>> index 7e9ae84810f6..bd15807f9160 100644 > >>> --- a/arch/arm/cpu/mmu_64.c > >>> +++ b/arch/arm/cpu/mmu_64.c > >>> @@ -10,6 +10,7 @@ > >>> #include <init.h> > >>> #include <mmu.h> > >>> #include <errno.h> > >>> +#include <zero_page.h> > >>> #include <linux/sizes.h> > >>> #include <asm/memory.h> > >>> #include <asm/pgtable64.h> > >>> @@ -168,6 +169,16 @@ static void mmu_enable(void) > >>> set_cr(get_cr() | CR_M | CR_C | CR_I); > >>> } > >>> > >>> +void zero_page_disable(void) > >>> +{ > >>> + create_sections(0x0, 0x0, PAGE_SIZE, CACHED_MEM); > >>> +} > >>> + > >>> +void zero_page_enable(void) > >>> +{ > >>> + create_sections(0x0, 0x0, PAGE_SIZE, 0x0); > >>> +} > >>> + > >>> /* > >>> * Prepare MMU for usage enable it. > >>> */ > >>> @@ -194,7 +205,7 @@ void __mmu_init(bool mmu_on) > >>> create_sections(bank->start, bank->start, bank->size, CACHED_MEM); > >>> > >>> /* Make zero page faulting to catch NULL pointer derefs */ > >>> - create_sections(0x0, 0x0, 0x1000, 0x0); > >>> + zero_page_enable(); > >>> > >>> mmu_enable(); > >>> } > >>> diff --git a/include/zero_page.h b/include/zero_page.h > >>> new file mode 100644 > >>> index 000000000000..d8dd07cfe959 > >>> --- /dev/null > >>> +++ b/include/zero_page.h > >>> @@ -0,0 +1,40 @@ > >>> +/* SPDX-License-Identifier: GPL-2.0-only */ > >>> +#ifndef __ZERO_PAGE_H > >>> +#define __ZERO_PAGE_H > >>> + > >>> +#include <common.h> > >>> + > >>> +#if defined CONFIG_ARCH_HAS_ZERO_PAGE > >>> + > >>> +/* > >>> + * zero_page_enable - enable null pointer trap > >>> + */ > >>> +void zero_page_enable(void); > >>> + > >>> +/* > >>> + * zero_page_disable - disable null pointer trap > >>> + * > >>> + * Disable the null pointer trap on the zero page if access to the zero page > >>> + * is actually required. Disable the trap with care and re-enable it > >>> + * immediately after the access to properly trap null pointers. > >>> + */ > >>> +void zero_page_disable(void); > >>> + > >>> +#else > >>> + > >>> +static inline void zero_page_enable(void) > >>> +{ > >>> +} > >>> + > >>> +static inline void zero_page_disable(void) > >>> +{ > >>> +} > >>> + > >>> +#endif > >>> + > >>> +static inline bool zero_page_contains(unsigned long addr) > >>> +{ > >>> + return addr < PAGE_SIZE; > >>> +} > >>> + > >>> +#endif /* __ZERO_PAGE_H */ > >>> diff --git a/lib/Kconfig b/lib/Kconfig > >>> index 887f50ff003f..e5831ecdb9a7 100644 > >>> --- a/lib/Kconfig > >>> +++ b/lib/Kconfig > >>> @@ -182,6 +182,9 @@ config ARCH_HAS_STACK_DUMP > >>> config ARCH_HAS_DATA_ABORT_MASK > >>> bool > >>> > >>> +config ARCH_HAS_ZERO_PAGE > >>> + bool > >>> + > >>> config HAVE_EFFICIENT_UNALIGNED_ACCESS > >>> bool > >>> > >>> > > > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > -- Pengutronix e.K. | Michael Tretter | Steuerwalder Str. 21 | https://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] ARM: mmu64: allow to disable null pointer trap on zero page 2020-10-14 16:29 ` [PATCH 1/2] ARM: mmu64: allow to disable null pointer trap on zero page Ahmad Fatoum [not found] ` <20201015073331.GA29491@pengutronix.de> @ 2020-10-15 8:44 ` Sascha Hauer 1 sibling, 0 replies; 10+ messages in thread From: Sascha Hauer @ 2020-10-15 8:44 UTC (permalink / raw) To: Ahmad Fatoum; +Cc: barebox, Michael Tretter On Wed, Oct 14, 2020 at 06:29:47PM +0200, Ahmad Fatoum wrote: > > > On 10/14/20 5:08 PM, Michael Tretter wrote: > > Barebox uses the zero page to trap NULL pointer dereferences. However, > > if the SDRAM starts at address 0x0, this makes the first page of the > > SDRAM inaccessible and makes it impossible to load images to offset 0x0 > > in the SDRAM. > > > > Trapping NULL pointer dereferences on such systems is still desirable. > > Therefore, add a function to disable the traps if accessing the zero > > page is necessary and to re-enable the traps after the access is done. > > Can't we map the (phy_addr_t)0 at some higher virtual address and > change uimage to use phys_to_virt() ? > > Something like > > static inline void *phys_to_virt(unsigned long phys) > { > if (!IS_ENABLED(CONFIG_ARM_MACH_CUSTOM_MAPPING) || !arm_mach_phys_to_virt) > return (void *)phys; > return arm_mach_phys_to_virt(phys); > } Please don't. A user of that function would have to implicitly know that phys_to_virt() would have to be called exactly for the zero page. Also how big is the area returned from phys_to_virt()? If it's up to the next page boundary a user would have to align all of it's memcpys to page boundaries for no gain. I rather suggest to introduce an explicit void *memcpy_to_zero(void *dst, void *src, size_t len) The implementation could then be architecture specific and could choose if it likes to just map the zero page to NULL or somewhere else. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-10-15 9:12 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-14 15:08 [PATCH 1/2] ARM: mmu64: allow to disable null pointer trap on zero page Michael Tretter
2020-10-14 15:08 ` [PATCH 2/2] uimage: disable zero page when loading to SDRAM at address 0x0 Michael Tretter
2020-10-14 16:33 ` Ahmad Fatoum
2020-10-15 7:40 ` Michael Tretter
2020-10-15 8:35 ` Sascha Hauer
2020-10-15 9:12 ` Ahmad Fatoum
2020-10-14 16:29 ` [PATCH 1/2] ARM: mmu64: allow to disable null pointer trap on zero page Ahmad Fatoum
[not found] ` <20201015073331.GA29491@pengutronix.de>
2020-10-15 8:14 ` Ahmad Fatoum
2020-10-15 8:40 ` Michael Tretter
2020-10-15 8:44 ` Sascha Hauer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox