Re: [PATCH] riscv: CONFIG_EFI should not depend on CONFIG_RISCV_ISA_C

[Björn Töpel]

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