memory protection for x86 dependent on XIP

[ghost]

If you disable CONFIG_XIP for x86, userspace/memory protection tests begin to fail.

Out of the box:
charles@cyouse-desk1:~/zephyr/zephyr$ sanitycheck -p qemu_x86 -T samples/userspace/shared_mem
JOBS: 6
Cleaning output directory /home/charles/zephyr/zephyr/sanity-out
Building testcase defconfigs...
Filtering test cases...
1 tests selected, 196 tests discarded due to filters
total complete: 1/ 1 100% failed: 0
1 of 1 tests passed with 0 warnings in 9 seconds

Modify the boards/x86/qemu_x86/Kconfig.board to comment out select XIP and then re-run:

$ vi boards/x86/qemu_x86/Kconfig.board
$ sanitycheck -p qemu_x86 -T samples/userspace/shared_mem
JOBS: 6
Cleaning output directory /home/charles/zephyr/zephyr/sanity-out
Building testcase defconfigs...
Filtering test cases...
1 tests selected, 196 tests discarded due to filters
total complete: 0/ 1 0% failed: 0

qemu_x86 samples/userspace/shared_mem/sample.kernel.memory_protection.shared_mem FAILED: handler_crash
see: sanity-out/qemu_x86/samples/userspace/shared_mem/sample.kernel.memory_protection.shared_mem/handler.log

total complete: 1/ 1 100% failed: 1
0 of 1 tests passed with 0 warnings in 5 seconds

[ghost]

@andrewboie There are 50-odd tests in sanitycheck that fail when XIP is deselected. I assume they're all related, and will share one (trivial) solution. As far as I can tell this problem existed before the recent implementation of per-thread page tables. I'll be looking into this - it's valuable to me as an exercise in getting to know the gritty details of the x86 userspace implementation - but I wanted to draw this to your attention in case you know of an obvious "whoopie" here.

[andrewboie]

I agree this probably has a single, fairly trivial fix. QEMU had XIP turned on by default for a long time (years) so it seems a matter of the non-XIP scenario never having been tested.

Can you post the error log with the console output?

[ghost]

Booting from ROM..Optimal CONFIG_X86_MMU_PAGE_POOL_PAGES 9
***** Booting Zephyr OS build v2.0.0-rc3-9-gd0ef464201b9 *****
FATAL: ***** CPU Page Fault (error code 0x00000000)
FATAL: Supervisor thread read address 0x00000000
FATAL: PDE: 0x0127 Present, Writable, User, Execute Enabled
FATAL: PTE: 0x00 Non-present, Read-only, Supervisor, Execute Enabled
FATAL: eax: 0x0012b000, ebx: 0x00001000, ecx: 0x0012c000, edx: 0x00001000
FATAL: esi: 0x00000000, edi: 0x0012b000, ebp: 0x0013b290, esp: 0x0013b284
FATAL: eflags: 0x00000212 cs: 0x0008 cr3: 0x0011d500
FATAL: call trace:
FATAL: eip: 0x00103189
FATAL:      0x001029ce (0x12b000)
FATAL:      0x0010234a (0x11d100)
FATAL:      0x00104e78 (0x11d100)
FATAL:      0x00104ee8 (0x11d100)
FATAL:      0x0010055d (0x11d100)
FATAL:      0x0010086b (0x143000)
FATAL:      0x00103775 (0x13b3e4)
FATAL:      0x001014ec (0x0)
FATAL: >>> ZEPHYR FATAL ERROR 0: CPU exception
FATAL: Current thread: 0x0011d5a0 (unknown)
FATAL: Halting system

[andrewboie]

The crash is in the code that constructs page tables for new user threads.

On further analysis, DT_PHYS_RAM_ADDR is being set wrong for qemu_x86 if CONFIG_XIP is turned off, it had a value of 0x500000. But with XIP turned off, RAM spans from 0x100000 - 0x4FFFFF.

The below code works for me, although tests/kernel/xip unsurprisingly crashes. The rest pass. Below code snippet is not a fix, it warns about overlapping the "flash" region.

diff --git a/boards/x86/qemu_x86/Kconfig.board b/boards/x86/qemu_x86/Kconfig.board
index 0ac4e6d036..3893e62575 100644
--- a/boards/x86/qemu_x86/Kconfig.board
+++ b/boards/x86/qemu_x86/Kconfig.board
@@ -1,9 +1,4 @@
 # SPDX-License-Identifier: Apache-2.0
-#
-# The QEMU targets themselves are not XIP, everything is actually RAM, but we
-# pretend the first 4 megabytes are a memory-mapped flash region. This is done
-# to ensure that the XIP data copying infrastructure doesn't bit-rot on
-# x86.
 
 config BOARD_QEMU_X86
        bool "QEMU x86"
@@ -12,4 +7,3 @@ config BOARD_QEMU_X86
        select HAS_DTS_ETHERNET
        select CPU_HAS_FPU
        select HAS_COVERAGE_SUPPORT
-       select XIP
diff --git a/dts/x86/ia32.dtsi b/dts/x86/ia32.dtsi
index 3b203fdc7b..3691fff61b 100644
--- a/dts/x86/ia32.dtsi
+++ b/dts/x86/ia32.dtsi
@@ -33,10 +33,10 @@
                reg = <0x00100000 DT_FLASH_SIZE>;
        };
 
-       sram0: memory@500000 {
+       sram0: memory@100000 {
                device_type = "memory";
                compatible = "mmio-sram";
-               reg = <0x00500000 DT_SRAM_SIZE>;
+               reg = <0x00100000 DT_SRAM_SIZE>;
        };
 
        soc {

I think we need to move some policy out of ia32.dtsi. There shouldn't be a flash0 region defined there at all, and sram0 should start at 1MB.

If qemu_x86 wants to test flash and xip this stuff needs to be moved qemu_x86's dts and not in the main ia32 one.

Still looking into this to find the best fix.

[andrewboie]

So this is definitely a DTS/board config problem and not a userspace one.

I'm working on a PR which does the following:

• IA32 SOC will no longer define a flash region
• IA32 SOC will start RAM at 1MB
• IA32 SOC will allow XIP to be enabled, but the board-level DTS will have to define a flash region
• qemu_x86 will have XIP off by default, but tests like tests/kernel/xip will be able to turn it on for testing purposes
• Anything related to apollo_lake SOC will have flash0 removed, no ability to enable XIP, SRAM always starting at 1MB, it doesn't make sense to support XIP for apollo_lake SOC

This will have the effect of testing both XIP and non-XIP on qemu_x86 to ensure it doesn't bit-rot, with qemu's default being non-XIP.

[ghost]

Just FYI, that page allocator I mentioned last week will be submitted this afternoon. The "SRAM" information from DTS is going to be a "default" memory map, which can be overridden by a memory map obtained at runtime (from EFI/ACPI/multiboot, or whatever).

There's going to be some overlap here, and to be complete, will probably require some minor changes to the early MMU setup (since the memory map isn't necessarily defined by constants like DT_PHYS_RAM_ADDR anymore) and newlib's heap stuff. All minor details though that I think won't collide with what you're doing here.