riscv: CONFIG_EFI should not depend on CONFIG_RISCV_ISA_C

  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.

[1] Documentation/riscv/boot-image-header.rst

Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
---
 arch/riscv/Kconfig       | 1 -
 arch/riscv/kernel/head.S | 8 ++++++--
 2 files changed, 6 insertions(+), 3 deletions(-)


base-commit: d88520ad73b79e71e3ddf08de335b8520ae41c5c
  

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.

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).

> [1] Documentation/riscv/boot-image-header.rst
>
> Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
> ---
>  arch/riscv/Kconfig       | 1 -
>  arch/riscv/kernel/head.S | 8 ++++++--
>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index d607ab0f7c6d..9c5bbbc93951 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -799,7 +799,6 @@ config EFI
>  	select EFI_RUNTIME_WRAPPERS
>  	select EFI_STUB
>  	select LIBFDT
> -	select RISCV_ISA_C
>  	select UCS2_STRING
>  	help
>  	  This option provides support for runtime services provided
> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
> index 3710ea5d160f..33d69b569843 100644
> --- a/arch/riscv/kernel/head.S
> +++ b/arch/riscv/kernel/head.S
> @@ -27,9 +27,13 @@ ENTRY(_start)
>  	 */
>  #ifdef CONFIG_EFI
>  	/*
> -	 * This instruction decodes to "MZ" ASCII required by UEFI.
> +	 * The compressed (C extension) "c.li s4,-13" instruction
> +	 * decodes to 0x5a4d/"MZ" (ASCII), which is required by UEFI.
> +	 *
> +	 * In order to support non-compressed EFI kernels, the
> +	 * instruction is written in hex.
>  	 */
> -	c.li s4,-13
> +	.word 0x5a4d5a4d
>  	j _start_kernel
>  #else
>  	/* jump to start kernel */
>
> base-commit: d88520ad73b79e71e3ddf08de335b8520ae41c5c
  

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?)


Björn
  

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