[PATCH] lib: sbi_illegal_insn: Emulate 'MZ'/c.li s4,-13 instruction

[Jessica Clarke]

On 9 Nov 2023, at 04:04, Xiang W <wxjstz at 126.com> wrote:
> 
> 在 2023-11-09星期四的 01:22 +0100，Björn Töpel写道：
>> On Thu, 9 Nov 2023 at 01:11, Jessica Clarke <jrtc27 at jrtc27.com> wrote:
>>> 
>>> On 9 Nov 2023, at 00:02, Björn Töpel <bjorn at kernel.org> wrote:
>>>> 
>>>> Hi Andreas!
>>>> 
>>>> On Wed, 8 Nov 2023 at 22:55, Andreas Schwab <schwab at linux-m68k.org> wrote:
>>>>> 
>>>>> On Nov 08 2023, Björn Töpel wrote:
>>>>> 
>>>>>> +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;
>>>>> 
>>>>> By skipping 4 bytes execution will resume in the middle of the next insn
>>>>> (the jump around the header).
>>>> 
>>>> This is in a non-C environment -- "!misa_extension('C')", so we're not
>>>> jumping into the middle of the next insn.
>>> 
>>> But that’s not c.li, is it. That’s bjorn.c.li, a 32-bit instruction
>>                                                  ^^^^ My very own! :-D
>> 
>>> whose first 16-bit parcel is the same as c.li and whose second 16-bit
>>> parcel is ignored. I really do not think this is a good idea.
>>> 
>>>> Note that the Linux kernel needs the change pointed out in this patch,
>>>> to build w/o C.
>>> 
>>> That sounds like a Linux problem. Other OSes cope just fine. Fixing in
>>> firmware is not the right approach; fix your software instead. Which
>>> may mean abandoning an image format that isn’t fit for purpose. But the
>>> solution isn’t to hack in whatever random crud Linux wants in firmware,
>>> and other non-firmware SBI implementations like hypervisors.
>> 
>> There are other kernels that people care about?
>> 
>>> So this is a resounding NAK from me.
>> 
>> Jokes aside; Fair enough! I'll go back to the drawing board.
> 
> The kernel cannot handle this problem because the kernel has not initialized
> stvec at this time. And the MZ cannot be changed because it is part of the PE
> header. The best way is to skip the MZ of the PE header through the upper
> bootloader. Therefore, when the kernel is used as the payload of opensbi,
> skipped operations also need to be added. sbi_hart_switch_mode needs to add
> some code to detect whether the current core supports C extension, whether
> there is 0x5a4d5a4d at next_addr, and modify next_addr.

That’s also extremely ugly; now you’re just skipping instructions in
the payload rather than trapping like the current de-facto spec would
mandate. And what would you skip? Again, you can’t just skip 2 bytes,
you would have to skip a whole 4 bytes, which is more than just the
c.li instruction. And what happens if a vendor, not implementing C,
wants to use that encoding space? For example, Qualcomm, who are
proposing to ditch C entirely. It may not conform to a standard
profile, but it is a thing that SBI must support.

Linux is abusing file formats and trying to create a polyglot where
it’s not possible. That’s Linux’s problem, and it needs to stop
assuming things that aren’t true. This grotesque approach is not
present in other OSes.

Jess