diff mbox series

[3/3] lib: sbi: Fix tval and tinst for sbi_get_insn()

Message ID TYYP286MB14393C382F94A378E9F529CDC6A79@TYYP286MB1439.JPNP286.PROD.OUTLOOK.COM
State Accepted
Headers show
Series Fixes for tval/tinst when using sbi_trap_redirect() | expand

Commit Message

dramforever June 9, 2022, 7:07 a.m. UTC 
We should not change trap->tval to mepc because mtval already points to
the faulting portion of the emulated instruction fetch, which is also
what stval is expected to be.

In addition, htinst is only allowed to be zero for instruction access
faults or page faults, and is only allowed to be zero or a
psuedoinstruction for instruction guest-page faults. Fix trap->tinst for
these cases.

Signed-off-by: dramforever <dramforever@live.com>
---
 lib/sbi/sbi_unpriv.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Anup Patel June 18, 2022, 5:58 a.m. UTC | #1 
On Thu, Jun 9, 2022 at 12:38 PM dramforever <dramforever@live.com> wrote:
>
> We should not change trap->tval to mepc because mtval already points to
> the faulting portion of the emulated instruction fetch, which is also
> what stval is expected to be.
>
> In addition, htinst is only allowed to be zero for instruction access
> faults or page faults, and is only allowed to be zero or a
> psuedoinstruction for instruction guest-page faults. Fix trap->tinst for
> these cases.
>
> Signed-off-by: dramforever <dramforever@live.com>

Looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

> ---
>  lib/sbi/sbi_unpriv.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/lib/sbi/sbi_unpriv.c b/lib/sbi/sbi_unpriv.c
> index 73b530c..9a34a71 100644
> --- a/lib/sbi/sbi_unpriv.c
> +++ b/lib/sbi/sbi_unpriv.c
> @@ -149,15 +149,17 @@ ulong sbi_get_insn(ulong mepc, struct sbi_trap_info *trap)
>         switch (trap->cause) {
>         case CAUSE_LOAD_ACCESS:
>                 trap->cause = CAUSE_FETCH_ACCESS;
> -               trap->tval = mepc;
> +               trap->tinst = 0UL;
>                 break;
>         case CAUSE_LOAD_PAGE_FAULT:
>                 trap->cause = CAUSE_FETCH_PAGE_FAULT;
> -               trap->tval = mepc;
> +               trap->tinst = 0UL;
>                 break;
>         case CAUSE_LOAD_GUEST_PAGE_FAULT:
>                 trap->cause = CAUSE_FETCH_GUEST_PAGE_FAULT;
> -               trap->tval = mepc;
> +               if (trap->tinst != INSN_PSEUDO_VS_LOAD &&
> +                   trap->tinst != INSN_PSEUDO_VS_STORE)
> +                       trap->tinst = 0UL;
>                 break;
>         default:
>                 break;
> --
> 2.36.0
>
>
> --
> opensbi mailing list
> opensbi@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
Anup Patel June 21, 2022, 4:22 a.m. UTC | #2 
On Sat, Jun 18, 2022 at 11:28 AM Anup Patel <anup@brainfault.org> wrote:
>
> On Thu, Jun 9, 2022 at 12:38 PM dramforever <dramforever@live.com> wrote:
> >
> > We should not change trap->tval to mepc because mtval already points to
> > the faulting portion of the emulated instruction fetch, which is also
> > what stval is expected to be.
> >
> > In addition, htinst is only allowed to be zero for instruction access
> > faults or page faults, and is only allowed to be zero or a
> > psuedoinstruction for instruction guest-page faults. Fix trap->tinst for
> > these cases.
> >
> > Signed-off-by: dramforever <dramforever@live.com>
>
> Looks good to me.
>
> Reviewed-by: Anup Patel <anup@brainfault.org>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

>
> Regards,
> Anup
>
> > ---
> >  lib/sbi/sbi_unpriv.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/sbi/sbi_unpriv.c b/lib/sbi/sbi_unpriv.c
> > index 73b530c..9a34a71 100644
> > --- a/lib/sbi/sbi_unpriv.c
> > +++ b/lib/sbi/sbi_unpriv.c
> > @@ -149,15 +149,17 @@ ulong sbi_get_insn(ulong mepc, struct sbi_trap_info *trap)
> >         switch (trap->cause) {
> >         case CAUSE_LOAD_ACCESS:
> >                 trap->cause = CAUSE_FETCH_ACCESS;
> > -               trap->tval = mepc;
> > +               trap->tinst = 0UL;
> >                 break;
> >         case CAUSE_LOAD_PAGE_FAULT:
> >                 trap->cause = CAUSE_FETCH_PAGE_FAULT;
> > -               trap->tval = mepc;
> > +               trap->tinst = 0UL;
> >                 break;
> >         case CAUSE_LOAD_GUEST_PAGE_FAULT:
> >                 trap->cause = CAUSE_FETCH_GUEST_PAGE_FAULT;
> > -               trap->tval = mepc;
> > +               if (trap->tinst != INSN_PSEUDO_VS_LOAD &&
> > +                   trap->tinst != INSN_PSEUDO_VS_STORE)
> > +                       trap->tinst = 0UL;
> >                 break;
> >         default:
> >                 break;
> > --
> > 2.36.0
> >
> >
> > --
> > opensbi mailing list
> > opensbi@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi
diff mbox series

Patch

diff --git a/lib/sbi/sbi_unpriv.c b/lib/sbi/sbi_unpriv.c
index 73b530c..9a34a71 100644
--- a/lib/sbi/sbi_unpriv.c
+++ b/lib/sbi/sbi_unpriv.c
@@ -149,15 +149,17 @@  ulong sbi_get_insn(ulong mepc, struct sbi_trap_info *trap)
 	switch (trap->cause) {
 	case CAUSE_LOAD_ACCESS:
 		trap->cause = CAUSE_FETCH_ACCESS;
-		trap->tval = mepc;
+		trap->tinst = 0UL;
 		break;
 	case CAUSE_LOAD_PAGE_FAULT:
 		trap->cause = CAUSE_FETCH_PAGE_FAULT;
-		trap->tval = mepc;
+		trap->tinst = 0UL;
 		break;
 	case CAUSE_LOAD_GUEST_PAGE_FAULT:
 		trap->cause = CAUSE_FETCH_GUEST_PAGE_FAULT;
-		trap->tval = mepc;
+		if (trap->tinst != INSN_PSEUDO_VS_LOAD &&
+		    trap->tinst != INSN_PSEUDO_VS_STORE)
+			trap->tinst = 0UL;
 		break;
 	default:
 		break;