Re: [PATCH 5/7] ARM: MMU: map text segment ro and data segments execute never

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hi,

On 6/13/25 09:58, Sascha Hauer wrote:
> +	pr_debug("%s: 0x%08x 0x%08x type %d\n", __func__, virt_addr, size, map_type);

I can add a follow up patch turning type into a string.

> +	unsigned long text_start = (unsigned long)&_stext;
> +	unsigned long text_size = (unsigned long)&__start_rodata - (unsigned long)&_stext;

text_size is an unfortunate name as text_start + text_size != _etext -
1, which is surprising.

I would prefer:

text_start = code_start = &_stext;
text_size = &_etext - text_start;
code_size = &__start_ro_data - code_start;

> +	unsigned long rodata_start = (unsigned long)&__start_rodata;
> +	unsigned long rodata_size = (unsigned long)&__end_rodata - rodata_start;
>  
>  	if (!request_barebox_region("ttb", (unsigned long)ttb,
>  				    ARM_EARLY_PAGETABLE_SIZE))
> @@ -550,6 +574,8 @@ void __mmu_init(bool mmu_on)
>  
>  	pr_debug("ttb: 0x%p\n", ttb);
>  
> +	vectors_init();

Any particular reason to move this around?

> +
>  	/*
>  	 * Early mmu init will have mapped everything but the initial memory area
>  	 * (excluding final OPTEE_SIZE bytes) uncached. We have now discovered
> @@ -568,10 +594,22 @@ void __mmu_init(bool mmu_on)
>  			pos = rsv->end + 1;
>  		}
>  
> -		remap_range((void *)pos, bank->start + bank->size - pos, MAP_CACHED);
> +		if (IS_ENABLED(CONFIG_ARM_MMU_PERMISSIONS)) {
> +			if (region_overlap_size(pos, bank->start + bank->size - pos,
> +			    text_start, text_size)) {

Wouldn't matter, but we should check overlap against the full range we
are going to remap specially. With my suggested text_size/code_size
changes above that would be correct(er).

> +				remap_range((void *)pos, text_start - pos, MAP_CACHED);

This is ok.

> +				remap_range((void *)text_start, text_size, MAP_CODE);
> +				remap_range((void *)rodata_start, rodata_size, ARCH_MAP_CACHED_RO);

These I would move out of the loop after the iteration.

> +				remap_range((void *)(rodata_start + rodata_size),
> +					    bank->start + bank->size - (rodata_start + rodata_size),
> +					    MAP_CACHED);
> +			} else {
> +				remap_range((void *)pos, bank->start + bank->size - pos, MAP_CACHED);
> +			}
> +		} else {
> +			remap_range((void *)pos, bank->start + bank->size - pos, MAP_CACHED);
> +		}

If we combine the two if conditional into a single &&ed one, we can
replace the three remap_range calls by a single one:

		pos = text_start + text_end;
		if (pos >= bank->start + bank->size)
			continue;
		/* We carved out a gap for the barebox parts, so fall
                 * through to remapping the rest
                 */
	}

	remap_range((void *)pos, bank->start + bank->size - pos,
		    MAP_CACHED);
}

Then we can do the remapping of the sections here. I think that would
aid readability.

Thanks,
Ahmad


>  	}
> -
> -	vectors_init();
>  }
>  
>  /*
> @@ -624,7 +662,7 @@ void mmu_early_enable(unsigned long membase, unsigned long memsize, unsigned lon
>  	 * map the bulk of the memory as sections to avoid allocating too many page tables
>  	 * at this early stage
>  	 */
> -	early_remap_range(membase, barebox_start - membase, MAP_CACHED, false);
> +	early_remap_range(membase, barebox_start - membase, ARCH_MAP_CACHED_RWX, false);
>  	/*
>  	 * Map the remainder of the memory explicitly with two level page tables. This is
>  	 * the place where barebox proper ends at. In barebox proper we'll remap the code
> @@ -634,10 +672,10 @@ void mmu_early_enable(unsigned long membase, unsigned long memsize, unsigned lon
>  	 * a break-before-make sequence which we can't do when barebox proper is running
>  	 * at the location being remapped.
>  	 */
> -	early_remap_range(barebox_start, barebox_size, MAP_CACHED, true);
> +	early_remap_range(barebox_start, barebox_size, ARCH_MAP_CACHED_RWX, true);
>  	early_remap_range(optee_start, OPTEE_SIZE, MAP_UNCACHED, false);
>  	early_remap_range(PAGE_ALIGN_DOWN((uintptr_t)_stext), PAGE_ALIGN(_etext - _stext),
> -			  MAP_CACHED, false);
> +			  ARCH_MAP_CACHED_RWX, false);
>  
>  	__mmu_cache_on();
>  }
> diff --git a/arch/arm/lib32/barebox.lds.S b/arch/arm/lib32/barebox.lds.S
> index a52556a35696aea6f15cad5fd3f0275e8e6349b1..dbfdd2e9c110133f7fb45e06911bfc9ea9e8299c 100644
> --- a/arch/arm/lib32/barebox.lds.S
> +++ b/arch/arm/lib32/barebox.lds.S
> @@ -30,7 +30,7 @@ SECTIONS
>  	}
>  	BAREBOX_BARE_INIT_SIZE
>  
> -	. = ALIGN(4);
> +	. = ALIGN(4096);
>  	__start_rodata = .;
>  	.rodata : {
>  		*(.rodata*)
> @@ -53,6 +53,7 @@ SECTIONS
>  		__stop_unwind_tab = .;
>  	}
>  #endif
> +	. = ALIGN(4096);
>  	__end_rodata = .;
>  	_etext = .;
>  	_sdata = .;
> diff --git a/include/mmu.h b/include/mmu.h
> index 84ec6c5efb3eb8020fdc98e76a3614c137a0f8e9..20855e89eda301527b8cd69d868d58fc79637f5e 100644
> --- a/include/mmu.h
> +++ b/include/mmu.h
> @@ -8,6 +8,7 @@
>  #define MAP_UNCACHED	0
>  #define MAP_CACHED	1
>  #define MAP_FAULT	2
> +#define MAP_CODE	3
>  
>  /*
>   * Depending on the architecture the default mapping can be
> 

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