Hi Ilias, On Thu, 30 Jan 2025 at 00:21, Ilias Apalodimas <ilias.apalodi...@linaro.org> wrote: > > U-Boot maps all pages as RWX. Sadly it's not not 1990 anymore and we are > better off mapping binaries with proper permissions. > > This is an attempt to map the U-Boot binary properly, but leave the > area we load binaries unaffected and RWX. > > patch #1 adds printing capabilities for page permissions in meminfo > patch #2 prepares our linker scripts, aligns sections in page boundaries etc > patch #3 is adding a function to change page tables > patch #4 sets the permissions for .rodata, .text and .data > > There's a few problems problem in this RFC ... > - U-Boot has no MMU APIs in place. As a result I just wired up the function > changing the page permissions in board_r.c but only for arm64. In the long > term we should add an API that other archs can use if they wish/can
But this creates more work for others. It would be better to do the job properly at the start. You don't need to support other archs, but you should create a plausible API and some structs which others can use. See cpu.h for some ideas. Of course you can't predict the future, but others will build on it if you make an effort. Otherwise, I doubt anyone else will (later) clean it up. > - meminfo doesn't align all results properly (Caleb?) > - I am not setting the permissions of EFI runtime services section on purpose. > Since the OS is allowed to call SetVirtualAddressMap, we need to reset the > pages to RWX just before ExitBootServices, so the OS can relocate that code. > In arm64 this isn't a huge problem, because we explicitly disable SVAM if > if VA_BITS > 39, but other OSes/archs will have a problem. > > # TODO: > - Long term. Add an MMU API and use that instead of exporting arch specific > functions > - Add an enum in mmu_set_attrs() instead of 1,2,3 arguments > - Set the permissions of EFI runtime services anbd reset them before EBS() > - Fix alignment when printing pages & information > - Add a Kconfig option so the feature can be turned off. > The reason is that QEMU CI and some boards, fail because they are trying to > write RO pages. This usually happens due to variables being wrongly defined > as > const, but with the updated page permissions this leads to a crash [0] [1]. > In both cases the reported ESR is 0x9600004f which translates to > "Abort caused by writing to memory" "Permission fault, level 3.". On top of > that not setting pages as RO (and only setting .data and .text sections) > works > fine. So until we find and fix the bugs above we can't turn this on > unconditionally. > > # Open questions > - Is initr_reloc_global_data() the right place to change permissions? Or is > there > a better/safer place to do that? > > # How to test patches > - Aplly them an enable > CONFIG_CMD_MEMINFO=y > CONFIG_CMD_MEMINFO_MAP=y > 'meminfo' should print something along the lines of > => meminfo > DRAM: 8 GiB > Walking pagetable at 000000023ffe0000, va_bits: 40. Using 4 levels > [0x23ffe1000] | Table | | > [0x23ffe2000] | Table | | > [0x000000000 - 0x008000000] | Block | RWX | Normal | > Inner-shareable > [0x008000000 - 0x040000000] | Block | RW | Device-nGnRnE | > Non-shareable > [0x040000000 - 0x200000000] | Block | RWX | Normal | > Inner-shareable > [0x23ffea000] | Table | | > [0x200000000 - 0x23f600000] | Block | RWX | Normal | > Inner-shareable > [0x23ffeb000] | Table | | > [0x23f600000 - 0x23f68b000] | Pages | RWX | Normal | > Inner-shareable > [0x23f68b000 - 0x23f74e000] | Pages | RX | Normal | > Inner-shareable > [0x23f74e000 - 0x23f793000] | Pages | RO | Normal | > Inner-shareable > [0x23f793000 - 0x23f794000] | Pages | RWX | Normal | > Inner-shareable > [0x23f794000 - 0x23f79d000] | Pages | RW | Normal | > Inner-shareable > [0x23f79d000 - 0x23f800000] | Pages | RWX | Normal | > Inner-shareable > [0x23f800000 - 0x240000000] | Block | RWX | Normal | > Inner-shareable > [0x240000000 - 0x4000000000] | Block | RWX | Normal | > Inner-shareable > [0x23ffe3000] | Table | | > [0x4010000000 - 0x4020000000] | Block | RW | Device-nGnRnE | > Non-shareable > [0x23ffe4000] | Table | | > [0x8000000000 - 0x10000000000] | Block | RW | Device-nGnRnE | > Non-shareable No, this is not a memory map. Please make all that stuff an option. Try to use generic structs to print it, not ARM-specific code. > > [0] > https://lore.kernel.org/u-boot/20250129-rockchip-pinctrl-const-v1-0-450ccdadf...@cherry.de/ > [1] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/jobs/1008714 > > Ilias Apalodimas (4): > meminfo: add memory details for armv8 > arm: Prepare linker scripts for memory permissions > arm64: mmu_change_region_attr() add an option not to break PTEs > arm64: Change mapping for data/rodata/text > > Makefile | 15 +++++---- > arch/arm/cpu/armv8/cache_v8.c | 45 +++++++++++++++++++++++-- > arch/arm/cpu/armv8/fsl-layerscape/cpu.c | 10 +++--- > arch/arm/cpu/armv8/u-boot.lds | 32 +++++++++++------- > arch/arm/include/asm/armv8/mmu.h | 2 ++ > arch/arm/include/asm/system.h | 3 +- > arch/arm/mach-snapdragon/board.c | 2 +- > cmd/meminfo.c | 5 +++ > common/board_r.c | 7 ++++ > include/asm-generic/sections.h | 2 ++ > lib/efi_loader/efi_runtime.c | 2 ++ > 11 files changed, 97 insertions(+), 28 deletions(-) > > -- > 2.43.0 > Regards, Simon
