| Message ID | 20200815170055.286732-2-anup.patel@wdc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] lib: sbi_init: Avoid thundering hurd problem with coldbook_lock | expand |
On 8/15/20 7:00 PM, Anup Patel wrote: > The Kendryte K210 follows RISC-V v1.9 spec so MTVAL has instruction > address (instead of instruction encoding) on illegal instruction trap. > > To handle above case, we fix sbi_illegal_insn_handler() without any > impact on RISC-V v1.10 (or higher) systems. This achieved by exploiting > the fact that program counter (and instruction address) is always 2-byte > aligned in RISC-V world. > > Signed-off-by: Anup Patel <anup.patel@wdc.com> On the Sipeed Maixduino (Kendryte K210) this resolves the problem of handling rdtime in S-mode U-Boot. Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > lib/sbi/sbi_illegal_insn.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/lib/sbi/sbi_illegal_insn.c b/lib/sbi/sbi_illegal_insn.c > index 0e5523f..779a727 100644 > --- a/lib/sbi/sbi_illegal_insn.c > +++ b/lib/sbi/sbi_illegal_insn.c > @@ -118,13 +118,22 @@ int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs) > { > struct sbi_trap_info uptrap; > > + /* > + * We only deal with 32bit (or longer) illegal instructions. If we > + * see instruction is zero OR instruction is 16bit then we fetch and > + * check the instruction encoding using unprivilege access. > + * > + * The program counter (PC) in RISC-V world is always 2-byte aligned > + * so handling only 32bit (or longer) illegal instructions also help > + * the case where MTVAL CSR contains instruction address for illegal > + * instruction trap. > + */ > + > if (unlikely((insn & 3) != 3)) { > - if (insn == 0) { > - insn = sbi_get_insn(regs->mepc, &uptrap); > - if (uptrap.cause) { > - uptrap.epc = regs->mepc; > - return sbi_trap_redirect(regs, &uptrap); > - } > + insn = sbi_get_insn(regs->mepc, &uptrap); > + if (uptrap.cause) { > + uptrap.epc = regs->mepc; > + return sbi_trap_redirect(regs, &uptrap); > } > if ((insn & 3) != 3) > return truly_illegal_insn(insn, regs); >
On Sat, Aug 15, 2020 at 10:01 AM Anup Patel <anup.patel@wdc.com> wrote: > > The Kendryte K210 follows RISC-V v1.9 spec so MTVAL has instruction > address (instead of instruction encoding) on illegal instruction trap. > > To handle above case, we fix sbi_illegal_insn_handler() without any > impact on RISC-V v1.10 (or higher) systems. This achieved by exploiting > the fact that program counter (and instruction address) is always 2-byte > aligned in RISC-V world. > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > --- > lib/sbi/sbi_illegal_insn.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/lib/sbi/sbi_illegal_insn.c b/lib/sbi/sbi_illegal_insn.c > index 0e5523f..779a727 100644 > --- a/lib/sbi/sbi_illegal_insn.c > +++ b/lib/sbi/sbi_illegal_insn.c > @@ -118,13 +118,22 @@ int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs) > { > struct sbi_trap_info uptrap; > > + /* > + * We only deal with 32bit (or longer) illegal instructions. If we > + * see instruction is zero OR instruction is 16bit then we fetch and > + * check the instruction encoding using unprivilege access. > + * > + * The program counter (PC) in RISC-V world is always 2-byte aligned > + * so handling only 32bit (or longer) illegal instructions also help > + * the case where MTVAL CSR contains instruction address for illegal > + * instruction trap. > + */ > + > if (unlikely((insn & 3) != 3)) { > - if (insn == 0) { > - insn = sbi_get_insn(regs->mepc, &uptrap); > - if (uptrap.cause) { > - uptrap.epc = regs->mepc; > - return sbi_trap_redirect(regs, &uptrap); > - } > + insn = sbi_get_insn(regs->mepc, &uptrap); > + if (uptrap.cause) { > + uptrap.epc = regs->mepc; > + return sbi_trap_redirect(regs, &uptrap); > } > if ((insn & 3) != 3) > return truly_illegal_insn(insn, regs); > -- > 2.25.1 > > > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi Reviewed-by: Atish Patra <atish.patra@wdc.com>
On 15 Aug 2020, at 18:00, Anup Patel <Anup.Patel@wdc.com> wrote: > > The Kendryte K210 follows RISC-V v1.9 spec so MTVAL has instruction > address (instead of instruction encoding) on illegal instruction trap. > > To handle above case, we fix sbi_illegal_insn_handler() without any > impact on RISC-V v1.10 (or higher) systems. This achieved by exploiting > the fact that program counter (and instruction address) is always 2-byte > aligned in RISC-V world. > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > --- > lib/sbi/sbi_illegal_insn.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/lib/sbi/sbi_illegal_insn.c b/lib/sbi/sbi_illegal_insn.c > index 0e5523f..779a727 100644 > --- a/lib/sbi/sbi_illegal_insn.c > +++ b/lib/sbi/sbi_illegal_insn.c > @@ -118,13 +118,22 @@ int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs) > { > struct sbi_trap_info uptrap; > > + /* > + * We only deal with 32bit (or longer) illegal instructions. If we > + * see instruction is zero OR instruction is 16bit then we fetch and > + * check the instruction encoding using unprivilege access. > + * > + * The program counter (PC) in RISC-V world is always 2-byte aligned > + * so handling only 32bit (or longer) illegal instructions also help > + * the case where MTVAL CSR contains instruction address for illegal > + * instruction trap. > + */ > + > if (unlikely((insn & 3) != 3)) { Isn't `(insn & 3) == 1` also unambiguous? It's only ambiguous when the LSB is 0. I do also wonder if this would be better as a single boot-time quirk test (ie deliberately fault and see what the hart reports), so as to not overly penalise 1.10-implementing harts. Jess > - if (insn == 0) { > - insn = sbi_get_insn(regs->mepc, &uptrap); > - if (uptrap.cause) { > - uptrap.epc = regs->mepc; > - return sbi_trap_redirect(regs, &uptrap); > - } > + insn = sbi_get_insn(regs->mepc, &uptrap); > + if (uptrap.cause) { > + uptrap.epc = regs->mepc; > + return sbi_trap_redirect(regs, &uptrap); > } > if ((insn & 3) != 3) > return truly_illegal_insn(insn, regs); > -- > 2.25.1 > > > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi
On Sun, Aug 16, 2020 at 1:02 AM Anup Patel <anup.patel@wdc.com> wrote: > were => where in the commit title? > The Kendryte K210 follows RISC-V v1.9 spec so MTVAL has instruction > address (instead of instruction encoding) on illegal instruction trap. > > To handle above case, we fix sbi_illegal_insn_handler() without any > impact on RISC-V v1.10 (or higher) systems. This achieved by exploiting > the fact that program counter (and instruction address) is always 2-byte > aligned in RISC-V world. > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > --- > lib/sbi/sbi_illegal_insn.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/lib/sbi/sbi_illegal_insn.c b/lib/sbi/sbi_illegal_insn.c > index 0e5523f..779a727 100644 > --- a/lib/sbi/sbi_illegal_insn.c > +++ b/lib/sbi/sbi_illegal_insn.c > @@ -118,13 +118,22 @@ int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs) > { > struct sbi_trap_info uptrap; > > + /* > + * We only deal with 32bit (or longer) illegal instructions. If we > + * see instruction is zero OR instruction is 16bit then we fetch and > + * check the instruction encoding using unprivilege access. nits: please use 32-bit, 16-bit, unprivileged access > + * > + * The program counter (PC) in RISC-V world is always 2-byte aligned > + * so handling only 32bit (or longer) illegal instructions also help > + * the case where MTVAL CSR contains instruction address for illegal > + * instruction trap. > + */ > + > if (unlikely((insn & 3) != 3)) { > - if (insn == 0) { > - insn = sbi_get_insn(regs->mepc, &uptrap); > - if (uptrap.cause) { > - uptrap.epc = regs->mepc; > - return sbi_trap_redirect(regs, &uptrap); > - } > + insn = sbi_get_insn(regs->mepc, &uptrap); > + if (uptrap.cause) { > + uptrap.epc = regs->mepc; > + return sbi_trap_redirect(regs, &uptrap); > } > if ((insn & 3) != 3) > return truly_illegal_insn(insn, regs); > -- Otherwise Reviewed-by: Bin Meng <bin.meng@windriver.com>
On Sun, Aug 16, 2020 at 1:21 AM Jessica Clarke <jrtc27@jrtc27.com> wrote: > > On 15 Aug 2020, at 18:00, Anup Patel <Anup.Patel@wdc.com> wrote: > > > > The Kendryte K210 follows RISC-V v1.9 spec so MTVAL has instruction > > address (instead of instruction encoding) on illegal instruction trap. > > > > To handle above case, we fix sbi_illegal_insn_handler() without any > > impact on RISC-V v1.10 (or higher) systems. This achieved by exploiting > > the fact that program counter (and instruction address) is always 2-byte > > aligned in RISC-V world. > > > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > > --- > > lib/sbi/sbi_illegal_insn.c | 21 +++++++++++++++------ > > 1 file changed, 15 insertions(+), 6 deletions(-) > > > > diff --git a/lib/sbi/sbi_illegal_insn.c b/lib/sbi/sbi_illegal_insn.c > > index 0e5523f..779a727 100644 > > --- a/lib/sbi/sbi_illegal_insn.c > > +++ b/lib/sbi/sbi_illegal_insn.c > > @@ -118,13 +118,22 @@ int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs) > > { > > struct sbi_trap_info uptrap; > > > > + /* > > + * We only deal with 32bit (or longer) illegal instructions. If we > > + * see instruction is zero OR instruction is 16bit then we fetch and > > + * check the instruction encoding using unprivilege access. > > + * > > + * The program counter (PC) in RISC-V world is always 2-byte aligned > > + * so handling only 32bit (or longer) illegal instructions also help > > + * the case where MTVAL CSR contains instruction address for illegal > > + * instruction trap. > > + */ > > + > > if (unlikely((insn & 3) != 3)) { > > Isn't `(insn & 3) == 1` also unambiguous? It's only ambiguous when the > LSB is 0. I did not understand your question. The "(insn & 3) != 3" means either: 1. insn[1:0] == 0 hence it is Quadrant0 16bit instruction OR entire insn == 0 2. insn[1:0] == 1 hence it is Quadrant1 16bit instruction 3. insn[1:0] == 2 hence it is Quadrant2 16bit instruction > > I do also wonder if this would be better as a single boot-time quirk > test (ie deliberately fault and see what the hart reports), so as to > not overly penalise 1.10-implementing harts. We are not penalising 1.10-implemetation because we are not emulating any 16bit illegal instructions. In fact, we are removing in "insn == 0" check for 1.10-implementation so we have removed couple of instructions for 1.10-implementation. > > Jess > > > - if (insn == 0) { > > - insn = sbi_get_insn(regs->mepc, &uptrap); > > - if (uptrap.cause) { > > - uptrap.epc = regs->mepc; > > - return sbi_trap_redirect(regs, &uptrap); > > - } > > + insn = sbi_get_insn(regs->mepc, &uptrap); > > + if (uptrap.cause) { > > + uptrap.epc = regs->mepc; > > + return sbi_trap_redirect(regs, &uptrap); > > } > > if ((insn & 3) != 3) > > return truly_illegal_insn(insn, regs); > > -- > > 2.25.1 > > > > > > -- > > opensbi mailing list > > opensbi@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/opensbi Regards, Anup
On 16 Aug 2020, at 15:15, Anup Patel <anup@brainfault.org> wrote: > > On Sun, Aug 16, 2020 at 1:21 AM Jessica Clarke <jrtc27@jrtc27.com> wrote: >> >> On 15 Aug 2020, at 18:00, Anup Patel <Anup.Patel@wdc.com> wrote: >>> >>> The Kendryte K210 follows RISC-V v1.9 spec so MTVAL has instruction >>> address (instead of instruction encoding) on illegal instruction trap. >>> >>> To handle above case, we fix sbi_illegal_insn_handler() without any >>> impact on RISC-V v1.10 (or higher) systems. This achieved by exploiting >>> the fact that program counter (and instruction address) is always 2-byte >>> aligned in RISC-V world. >>> >>> Signed-off-by: Anup Patel <anup.patel@wdc.com> >>> --- >>> lib/sbi/sbi_illegal_insn.c | 21 +++++++++++++++------ >>> 1 file changed, 15 insertions(+), 6 deletions(-) >>> >>> diff --git a/lib/sbi/sbi_illegal_insn.c b/lib/sbi/sbi_illegal_insn.c >>> index 0e5523f..779a727 100644 >>> --- a/lib/sbi/sbi_illegal_insn.c >>> +++ b/lib/sbi/sbi_illegal_insn.c >>> @@ -118,13 +118,22 @@ int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs) >>> { >>> struct sbi_trap_info uptrap; >>> >>> + /* >>> + * We only deal with 32bit (or longer) illegal instructions. If we >>> + * see instruction is zero OR instruction is 16bit then we fetch and >>> + * check the instruction encoding using unprivilege access. >>> + * >>> + * The program counter (PC) in RISC-V world is always 2-byte aligned >>> + * so handling only 32bit (or longer) illegal instructions also help >>> + * the case where MTVAL CSR contains instruction address for illegal >>> + * instruction trap. >>> + */ >>> + >>> if (unlikely((insn & 3) != 3)) { >> >> Isn't `(insn & 3) == 1` also unambiguous? It's only ambiguous when the >> LSB is 0. > > I did not understand your question. > > The "(insn & 3) != 3" means either: > 1. insn[1:0] == 0 hence it is Quadrant0 16bit instruction OR entire insn == 0 > 2. insn[1:0] == 1 hence it is Quadrant1 16bit instruction > 3. insn[1:0] == 2 hence it is Quadrant2 16bit instruction My point was, for case 2, the LSB is 1, therefore MTVAL must be an instruction and not a PC, so there is no need to fetch the instruction. >> I do also wonder if this would be better as a single boot-time quirk >> test (ie deliberately fault and see what the hart reports), so as to >> not overly penalise 1.10-implementing harts. > > We are not penalising 1.10-implemetation because we are not > emulating any 16bit illegal instructions. In fact, we are removing > in "insn == 0" check for 1.10-implementation so we have removed > couple of instructions for 1.10-implementation. Right, I had forgotten that the only compressed instruction emulation is for unaligned accesses, at least for the current C specification. Though this does still add overhead on 1.10 implementations for "truly illegal" (I don't know if OpenSBI reuses that term from BBL) instructions. For ones that are just going to cause the program in question to be killed with SIGILL that's probably not so important (and hopefully rare), but floating point instructions (like C.FLW) are important to think about here. OSes like to lazily context-switch floating-point state, and do so on RISC-V by setting MSTATUS.FS back to 0 so that the first time the switched-to process tries to perform a floating-point operation it will trap, the OS can restore the right floating-point state and then retry the instruction. Maybe the overhead of doing all that already means that the increased inefficiency in OpenSBI forwarding it to S-mode isn't significant, but it is a little sad to be doing this unnecessary work. Jess >>> - if (insn == 0) { >>> - insn = sbi_get_insn(regs->mepc, &uptrap); >>> - if (uptrap.cause) { >>> - uptrap.epc = regs->mepc; >>> - return sbi_trap_redirect(regs, &uptrap); >>> - } >>> + insn = sbi_get_insn(regs->mepc, &uptrap); >>> + if (uptrap.cause) { >>> + uptrap.epc = regs->mepc; >>> + return sbi_trap_redirect(regs, &uptrap); >>> } >>> if ((insn & 3) != 3) >>> return truly_illegal_insn(insn, regs); >>> -- >>> 2.25.1 >>> >>> >>> -- >>> opensbi mailing list >>> opensbi@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/opensbi > > Regards, > Anup
diff --git a/lib/sbi/sbi_illegal_insn.c b/lib/sbi/sbi_illegal_insn.c index 0e5523f..779a727 100644 --- a/lib/sbi/sbi_illegal_insn.c +++ b/lib/sbi/sbi_illegal_insn.c @@ -118,13 +118,22 @@ int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs) { struct sbi_trap_info uptrap; + /* + * We only deal with 32bit (or longer) illegal instructions. If we + * see instruction is zero OR instruction is 16bit then we fetch and + * check the instruction encoding using unprivilege access. + * + * The program counter (PC) in RISC-V world is always 2-byte aligned + * so handling only 32bit (or longer) illegal instructions also help + * the case where MTVAL CSR contains instruction address for illegal + * instruction trap. + */ + if (unlikely((insn & 3) != 3)) { - if (insn == 0) { - insn = sbi_get_insn(regs->mepc, &uptrap); - if (uptrap.cause) { - uptrap.epc = regs->mepc; - return sbi_trap_redirect(regs, &uptrap); - } + insn = sbi_get_insn(regs->mepc, &uptrap); + if (uptrap.cause) { + uptrap.epc = regs->mepc; + return sbi_trap_redirect(regs, &uptrap); } if ((insn & 3) != 3) return truly_illegal_insn(insn, regs);
The Kendryte K210 follows RISC-V v1.9 spec so MTVAL has instruction address (instead of instruction encoding) on illegal instruction trap. To handle above case, we fix sbi_illegal_insn_handler() without any impact on RISC-V v1.10 (or higher) systems. This achieved by exploiting the fact that program counter (and instruction address) is always 2-byte aligned in RISC-V world. Signed-off-by: Anup Patel <anup.patel@wdc.com> --- lib/sbi/sbi_illegal_insn.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)