Hi Simon, On Thu, 6 Feb 2025 at 14:33, Simon Glass <s...@chromium.org> wrote: > > 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.
I can move that function to cpu_ops.h and have if NULL for other archs > > > - 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. This is *literally* the memory map. > Please make all that stuff an option. > Try to use generic structs to print it, not ARM-specific code. How? The information in the page tables is arch specific. Thanks /Ilias > > > > > [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
