Re: [PATCH] riscv: CONFIG_EFI should not depend on CONFIG_RISCV_ISA_C
From: Björn Töpel
Date: Wed Nov 08 2023 - 00:45:48 EST
Björn Töpel <bjorn@xxxxxxxxxx> writes:
> Palmer Dabbelt <palmer@xxxxxxxxxxx> writes:
>
>> On Tue, 24 Oct 2023 12:26:48 PDT (-0700), bjorn@xxxxxxxxxx wrote:
>>> From: Björn Töpel <bjorn@xxxxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxxxx>
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@xxxxxxxxxxxx>
---
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