Messages in this thread Patch in this message | | | From | Björn Töpel <> | Subject | Re: [PATCH] riscv: CONFIG_EFI should not depend on CONFIG_RISCV_ISA_C | Date | Wed, 08 Nov 2023 06:45:43 +0100 |
| |
Björn Töpel <bjorn@kernel.org> writes:
> Palmer Dabbelt <palmer@dabbelt.com> writes: > >> On Tue, 24 Oct 2023 12:26:48 PDT (-0700), bjorn@kernel.org wrote: >>> From: Björn Töpel <bjorn@rivosinc.com> >>> >>> UEFI/PE mandates that the kernel Image starts with "MZ" ASCII >>> (0x5A4D). Convenient enough, "MZ" is a valid compressed RISC-V >>> instruction. This means that a non-UEFI loader can simply jump to >>> "code0" in the Image header [1] and start executing. >>> >>> The Image specification [1] says the following about "code0": >>> | This header is also reused to support EFI stub for RISC-V. EFI >>> | specification needs PE/COFF image header in the beginning of the >>> | kernel image in order to load it as an EFI application. In order >>> | to support EFI stub, code0 is replaced with "MZ" magic string >>> | and res3(at offset 0x3c) points to the rest of the PE/COFF >>> | header. >>> >>> "MZ" is not a valid instruction for implementations without the C >>> extension. >>> >>> A non-UEFI loader, loading a non-C UEFI image have the following >>> options: >>> 1. Trap and emulate "code0" >>> 2. Avoid "code0" if it is "MZ", and have the kernel entry at >>> "code1". >>> >>> Replace the compressed instruction with a hex code variant, that works >>> for CONFIG_RISCV_ISA_C=n builds. Further, this change also make sure >>> that the "code0" instruction is 32b aligned. >> >> IMO this breaks the Image format on non-C systems: they'll jump straight >> to the start (as there's no symbols left or anything) and then just fail >> to execute the C instructions. > > That's right! My idea, which was probably really poor articulated, was > to add trap/emulate in OpenSBI/non-C for the MZ instructions. > >> We could amend the Image format to require bootloaders to handle this >> (ie, look at the first instructions and emulate them if there's no C), >> that would require some bootloader updates but given this doesn't work >> maybe that's fine. >> >> We could also just stop producing Image+PE binaries on non-C systems >> (ie, just produce PE). > > Yes, or make the Image loader in Qemu et al more robust, by e.g. > checking code0/code1. The Image spec does say that code0 is 'MZ' for > PE+Image builds, and for non-C capable systems one could argue that it > should be checked/skipped by the loader. > > Does the arguments above make sense for you? I.e.: > 1. Merge this patch > 2a. Add the trap/emu to OpenSBI > (2b. Improve the image loader in Qemu?)
FWIW, the OpenSBI patch would be something like:
From: =?UTF-8?q?Bj=C3=B6rn=20T=C3=B6pel?= <bjorn@rivosinc.com> Date: Wed, 8 Nov 2023 06:29:06 +0100 Subject: [PATCH] lib: sbi_illegal_insn: Emulate 'MZ'/c.li s4,-13 instruction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
The Linux kernel RISC-V image format allows that UEFI Images, can also be booted by non-UEFI firmware. However for that to work, the PE/Image combo requires that 'MZ' is a valid instruction. On RISC-V, 'MZ' is only a valid instruction if the hardware is C capable.
Emulate "c.li s4,-13" for non-C capable hardware.
Signed-off-by: Björn Töpel <bjorn@rivosinc.com> --- lib/sbi/sbi_illegal_insn.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/lib/sbi/sbi_illegal_insn.c b/lib/sbi/sbi_illegal_insn.c index 2be47575a365..c9588e364264 100644 --- a/lib/sbi/sbi_illegal_insn.c +++ b/lib/sbi/sbi_illegal_insn.c @@ -102,6 +102,19 @@ static int system_opcode_insn(ulong insn, struct sbi_trap_regs *regs) return 0; } +static int compressed_insn(ulong insn, struct sbi_trap_regs *regs) +{ + /* Only handle 'MZ'/c.li s4,-13/0x5a3d */ + if (!misa_extension('C') && (insn & 0xffff) == 0x5a4d) { + regs->s4 = -13; + regs->mepc += 4; + + return 0; + } + + return truly_illegal_insn(insn, regs); +} + static const illegal_insn_func illegal_insn_table[32] = { truly_illegal_insn, /* 0 */ truly_illegal_insn, /* 1 */ @@ -140,6 +153,7 @@ static const illegal_insn_func illegal_insn_table[32] = { int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs) { struct sbi_trap_info uptrap; + ulong tmp; /* * We only deal with 32-bit (or longer) illegal instructions. If we @@ -159,7 +173,10 @@ int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs) uptrap.epc = regs->mepc; return sbi_trap_redirect(regs, &uptrap); } - if ((insn & 3) != 3) + tmp = insn & 3; + if (tmp == 1) + return compressed_insn(insn, regs); + else if (tmp != 3) return truly_illegal_insn(insn, regs); } base-commit: cbdd86973901b6be2a1a2d3d6b54f3184fdf9a44 -- 2.40.1
| |