seL4 / sel4test

Test suite for seL4.
http://sel4.systems
Other
24 stars 60 forks source link

Use the reliable illegal instruction encoding #77

Closed ahmedcharles closed 2 years ago

ahmedcharles commented 2 years ago

The unprivileged spec says the following:

We consider it a feature that any length of instruction containing all zero bits is not legal, as this quickly traps erroneous jumps into zeroed memory regions. Similarly, we also reserve the instruction encoding containing all ones to be an illegal instruction, to catch the other common pattern observed with unprogrammed non-volatile memory devices, disconnected memory buses, or broken memory devices. Software can rely on a naturally aligned 32-bit word containing zero to act as an illegal instruction on all RISC-V implementations, to be used by software where an illegal instruction is explicitly desired. Defining a corresponding known illegal value for all ones is more difficult due to the variable-length encoding. Software cannot generally use the illegal value of ILEN bits of all 1s, as software might not know ILEN for the eventual target machine (e.g., if software is compiled into a standard binary library used by many different machines). Defining a 32-bit word of all ones as illegal was also considered, as all machines must support a 32-bit instruction size, but this requires the instruction-fetch unit on machines with ILEN>32 report an illegal instruction exception rather than access fault when such an instruction borders a protection boundary, complicating variable-instruction-length fetch and decode.

Signed-off-by: Ahmed Charles acharles@outlook.com

I'm not sure why this test currently fails on recent versions of QEMU, but I thought that putting this change up for review might help with figuring that out. I do think that the zero bit pattern is 'better' because of the statement in the spec.

kent-mcleod commented 2 years ago

The test originally used 0x00000000, but I changed it to 0xffffffff because a simulator wasn't treating 0x00000000 as an illegal value... https://github.com/seL4/sel4test/commit/8260c13ae9761b28a9cd84aa509ce61e0a3e4cc0

kent-mcleod commented 2 years ago

As also mentioned in that commit, using 0xffffffff also tests that the kernel is sending the right value in the fault message to a user level fault handler.

kent-mcleod commented 2 years ago

(I'm not saying that this PR shouldn't be considered though).

Ivan-Velickovic commented 2 years ago

As of b97028b8c5a2865bec784bc9b8c4c31ad23a9351, QEMU no longer fails this test. The earliest where the test fails is 48eaeb56debf91817dea00a2cd9c1f6c986eb531. It seems to me this was a bug in QEMU (https://gitlab.com/qemu-project/qemu/-/issues/1060).

@kent-mcleod do you remember what simulator wasn't treating all zeroes as an illegal instruction?

kent-mcleod commented 2 years ago

@kent-mcleod do you remember what simulator wasn't treating all zeroes as an illegal instruction?

qemu-system-riscv64 -machine sifive_u I think, in a version from back in 2019

kent-mcleod commented 2 years ago

It seems to me this was a bug in QEMU (https://gitlab.com/qemu-project/qemu/-/issues/1060).

That looks like it could be a different issue? I think the issue I was originally having was that the instruction wouldn't get trapped at all.

Ivan-Velickovic commented 2 years ago

It seems to me this was a bug in QEMU (https://gitlab.com/qemu-project/qemu/-/issues/1060).

That looks like it could be a different issue? I think the issue I was originally having was that the instruction wouldn't get trapped at all.

Based on the contents of the commits that I listed above, it seems like what was causing PAGEFAULT0005 to fail with v7.0.0 (on the Spike). I haven't looked closely enough at it though, I could be wrong. (But I don't think it is related to the issue you were having)

ahmedcharles commented 2 years ago

I'm fine with waiting for the bug in QEMU to be fixed. I changed the PR to add a comment, I hope that's ok. Thanks.

ahmedcharles commented 2 years ago

@kent-mcleod Thoughts?

kent-mcleod commented 2 years ago

Adding a comment is fine. Thanks for making it less confusing for anyone in the future.

ahmedcharles commented 2 years ago

I updated the commit so it didn't run afoul of the line length checks. Thanks.