Re: [PATCH 1/2] ARM: mmu64: allow to disable null pointer trap on zero page

[Michael Tretter <m.tretter@pengutronix.de>]

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