| Message ID | 20231108191310.49950-1-bjorn@kernel.org |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | lib: sbi_illegal_insn: Emulate 'MZ'/c.li s4,-13 instruction | expand |
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).
Hi Andreas! On Wed, 8 Nov 2023 at 22:55, Andreas Schwab <schwab@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. Note that the Linux kernel needs the change pointed out in this patch, to build w/o C. Björn
On 9 Nov 2023, at 00:02, Björn Töpel <bjorn@kernel.org> wrote: > > Hi Andreas! > > On Wed, 8 Nov 2023 at 22:55, Andreas Schwab <schwab@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 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. So this is a resounding NAK from me. Jess > Björn > > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi
On Thu, 9 Nov 2023 at 01:11, Jessica Clarke <jrtc27@jrtc27.com> wrote: > > On 9 Nov 2023, at 00:02, Björn Töpel <bjorn@kernel.org> wrote: > > > > Hi Andreas! > > > > On Wed, 8 Nov 2023 at 22:55, Andreas Schwab <schwab@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. Thank you for the input! Björn
在 2023-11-09星期四的 01:22 +0100，Björn Töpel写道： > On Thu, 9 Nov 2023 at 01:11, Jessica Clarke <jrtc27@jrtc27.com> wrote: > > > > On 9 Nov 2023, at 00:02, Björn Töpel <bjorn@kernel.org> wrote: > > > > > > Hi Andreas! > > > > > > On Wed, 8 Nov 2023 at 22:55, Andreas Schwab <schwab@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. Regards, Xiang W > > Thank you for the input! > > > Björn >
On 9 Nov 2023, at 04:04, Xiang W <wxjstz@126.com> wrote: > > 在 2023-11-09星期四的 01:22 +0100，Björn Töpel写道： >> On Thu, 9 Nov 2023 at 01:11, Jessica Clarke <jrtc27@jrtc27.com> wrote: >>> >>> On 9 Nov 2023, at 00:02, Björn Töpel <bjorn@kernel.org> wrote: >>>> >>>> Hi Andreas! >>>> >>>> On Wed, 8 Nov 2023 at 22:55, Andreas Schwab <schwab@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
在 2023-11-09星期四的 06:24 +0000，Jessica Clarke写道： > On 9 Nov 2023, at 04:04, Xiang W <wxjstz@126.com> wrote: > > > > 在 2023-11-09星期四的 01:22 +0100，Björn Töpel写道： > > > On Thu, 9 Nov 2023 at 01:11, Jessica Clarke <jrtc27@jrtc27.com> wrote: > > > > > > > > On 9 Nov 2023, at 00:02, Björn Töpel <bjorn@kernel.org> wrote: > > > > > > > > > > Hi Andreas! > > > > > > > > > > On Wed, 8 Nov 2023 at 22:55, Andreas Schwab <schwab@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. How do other operating systems solve it? This is a problem with the uefi image file format. Can we remove dos header? Regards, Xiang W > > Jess
On 9 Nov 2023, at 06:55, Xiang W <wxjstz@126.com> wrote: > 在 2023-11-09星期四的 06:24 +0000，Jessica Clarke写道： >> On 9 Nov 2023, at 04:04, Xiang W <wxjstz@126.com> wrote: >>> >>> 在 2023-11-09星期四的 01:22 +0100，Björn Töpel写道： >>>> On Thu, 9 Nov 2023 at 01:11, Jessica Clarke <jrtc27@jrtc27.com> wrote: >>>>> >>>>> On 9 Nov 2023, at 00:02, Björn Töpel <bjorn@kernel.org> wrote: >>>>>> >>>>>> Hi Andreas! >>>>>> >>>>>> On Wed, 8 Nov 2023 at 22:55, Andreas Schwab <schwab@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. > How do other operating systems solve it? This is a problem with the uefi > image file format. Can we remove dos header? They don’t delude themselves that you can just jump to the start of a UEFI PE/COFF executable and interpret it as machine code. Jess
在 2023-11-09星期四的 07:00 +0000，Jessica Clarke写道： > On 9 Nov 2023, at 06:55, Xiang W <wxjstz@126.com> wrote: > > 在 2023-11-09星期四的 06:24 +0000，Jessica Clarke写道： > > > On 9 Nov 2023, at 04:04, Xiang W <wxjstz@126.com> wrote: > > > > > > > > 在 2023-11-09星期四的 01:22 +0100，Björn Töpel写道： > > > > > On Thu, 9 Nov 2023 at 01:11, Jessica Clarke <jrtc27@jrtc27.com> wrote: > > > > > > > > > > > > On 9 Nov 2023, at 00:02, Björn Töpel <bjorn@kernel.org> wrote: > > > > > > > > > > > > > > Hi Andreas! > > > > > > > > > > > > > > On Wed, 8 Nov 2023 at 22:55, Andreas Schwab <schwab@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. > > How do other operating systems solve it? This is a problem with the uefi > > image file format. Can we remove dos header? > > They don’t delude themselves that you can just jump to the start of a > UEFI PE/COFF executable and interpret it as machine code. Show me code. Thx, Xiang W > > Jess
On 9 Nov 2023, at 07:10, Xiang W <wxjstz@126.com> wrote: > 在 2023-11-09星期四的 07:00 +0000，Jessica Clarke写道： >> On 9 Nov 2023, at 06:55, Xiang W <wxjstz@126.com> wrote: >>> 在 2023-11-09星期四的 06:24 +0000，Jessica Clarke写道： >>>> On 9 Nov 2023, at 04:04, Xiang W <wxjstz@126.com> wrote: >>>>> >>>>> 在 2023-11-09星期四的 01:22 +0100，Björn Töpel写道： >>>>>> On Thu, 9 Nov 2023 at 01:11, Jessica Clarke <jrtc27@jrtc27.com> wrote: >>>>>>> >>>>>>> On 9 Nov 2023, at 00:02, Björn Töpel <bjorn@kernel.org> wrote: >>>>>>>> >>>>>>>> Hi Andreas! >>>>>>>> >>>>>>>> On Wed, 8 Nov 2023 at 22:55, Andreas Schwab <schwab@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. >>> How do other operating systems solve it? This is a problem with the uefi >>> image file format. Can we remove dos header? >> >> They don’t delude themselves that you can just jump to the start of a >> UEFI PE/COFF executable and interpret it as machine code. > Show me code. They are separate binaries, one for UEFI PE/COFF and one for direct SBI booting if you want it. Unless you introduce a new image format for SBI payloads there is no way around that without inappropriate hacks like these suggestions. So there is no magic code. The answer is, like I’ve been saying, to just not, because it’s not possible. Jess
>On 9 Nov 2023, at 07:10, Xiang W <wxjstz@126.com> wrote: >> 在 2023-11-09星期四的 07:00 +0000，Jessica Clarke写道： >>> On 9 Nov 2023, at 06:55, Xiang W <wxjstz@126.com> wrote: >>>> 在 2023-11-09星期四的 06:24 +0000，Jessica Clarke写道： >>>>> On 9 Nov 2023, at 04:04, Xiang W <wxjstz@126.com> wrote: >>>>>> >>>>>> 在 2023-11-09星期四的 01:22 +0100，Björn Töpel写道： >>>>>>> On Thu, 9 Nov 2023 at 01:11, Jessica Clarke <jrtc27@jrtc27.com> wrote: >>>>>>>> >>>>>>>> On 9 Nov 2023, at 00:02, Björn Töpel <bjorn@kernel.org> wrote: >>>>>>>>> >>>>>>>>> Hi Andreas! >>>>>>>>> >>>>>>>>> On Wed, 8 Nov 2023 at 22:55, Andreas Schwab <schwab@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! 😂 >>>>>>> >>>>>>>> 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. >>>> How do other operating systems solve it? This is a problem with the uefi >>>> image file format. Can we remove dos header? >>> >>> They don’t delude themselves that you can just jump to the start of a >>> UEFI PE/COFF executable and interpret it as machine code. >> Show me code. > >They are separate binaries, one for UEFI PE/COFF and one for direct SBI >booting if you want it. Unless you introduce a new image format for SBI >payloads there is no way around that without inappropriate hacks like >these suggestions. So there is no magic code. The answer is, like I’ve >been saying, to just not, because it’s not possible. > >Jess > As the UEFI header can be vaild code, it can be the first inst of bare program. Skipping it may cause some bare program not work. In fact, this is a problem releated to the UEFI, not SBI.
diff --git a/lib/sbi/sbi_illegal_insn.c b/lib/sbi/sbi_illegal_insn.c index 2be47575a365..a3ade71295ce 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,11 @@ 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); }