[PATCH: fix for virt instr exception] target/riscv: fix for virtual instr exception

[PATCH: fix for virt instr exception] target/riscv: fix for virtual instr exception

[Deepak Gupta]

commit fb3f3730e4 added mechanism to generate virtual instruction
exception during instruction decode when virt is enabled.

However in some situations, illegal instruction exception can be raised
due to state of CPU. One such situation is implementing branch tracking.
[1] An indirect branch if doesn't land on a landing pad instruction, then
cpu must raise an illegal instruction exception.
Implementation would raise such expcetion due to missing landing pad inst
and not due to decode. Thus DisasContext must have `virt_inst_excp`
initialized to false during DisasContxt initialization for TB.

[1] - https://github.com/riscv/riscv-cfi

Signed-off-by: Deepak Gupta <debug@rivosinc.com>
---
 target/riscv/translate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index df38db7553..76f61a39d3 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -1167,6 +1167,7 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
     ctx->pm_base_enabled = FIELD_EX32(tb_flags, TB_FLAGS, PM_BASE_ENABLED);
     ctx->itrigger = FIELD_EX32(tb_flags, TB_FLAGS, ITRIGGER);
     ctx->zero = tcg_constant_tl(0);
+    ctx->virt_inst_excp = false;
 }
 
 static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
-- 
2.25.1

Re: [PATCH: fix for virt instr exception] target/riscv: fix for virtual instr exception

[Deepak Gupta]

Please dis-regard this.
I've sent the patch to qemu-riscv@nongnu.org

On Sat, Jan 28, 2023 at 12:48 AM Deepak Gupta <debug@rivosinc.com> wrote:
>
> commit fb3f3730e4 added mechanism to generate virtual instruction
> exception during instruction decode when virt is enabled.
>
> However in some situations, illegal instruction exception can be raised
> due to state of CPU. One such situation is implementing branch tracking.
> [1] An indirect branch if doesn't land on a landing pad instruction, then
> cpu must raise an illegal instruction exception.
> Implementation would raise such expcetion due to missing landing pad inst
> and not due to decode. Thus DisasContext must have `virt_inst_excp`
> initialized to false during DisasContxt initialization for TB.
>
> [1] - https://github.com/riscv/riscv-cfi
>
> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> ---
>  target/riscv/translate.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index df38db7553..76f61a39d3 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -1167,6 +1167,7 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>      ctx->pm_base_enabled = FIELD_EX32(tb_flags, TB_FLAGS, PM_BASE_ENABLED);
>      ctx->itrigger = FIELD_EX32(tb_flags, TB_FLAGS, ITRIGGER);
>      ctx->zero = tcg_constant_tl(0);
> +    ctx->virt_inst_excp = false;
>  }
>
>  static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
> --
> 2.25.1
>

Re: [PATCH: fix for virt instr exception] target/riscv: fix for virtual instr exception

[Alistair Francis]

On Sat, Jan 28, 2023 at 6:37 AM Deepak Gupta <debug@rivosinc.com> wrote:
>
> Please dis-regard this.
> I've sent the patch to qemu-riscv@nongnu.org

That's not entirely correct either.

You can run the `./scripts/get_maintainer.pl` script on your patch to
get the email addresses to send this patch to. qemu-devel is the main
one, but it's also a good idea to CC the qemu-riscv list as well as
maintainers.

>
> On Sat, Jan 28, 2023 at 12:48 AM Deepak Gupta <debug@rivosinc.com> wrote:
> >
> > commit fb3f3730e4 added mechanism to generate virtual instruction
> > exception during instruction decode when virt is enabled.
> >
> > However in some situations, illegal instruction exception can be raised
> > due to state of CPU. One such situation is implementing branch tracking.
> > [1] An indirect branch if doesn't land on a landing pad instruction, then
> > cpu must raise an illegal instruction exception.
> > Implementation would raise such expcetion due to missing landing pad inst
> > and not due to decode. Thus DisasContext must have `virt_inst_excp`

Isn't a landing pad instruction just an instruction that needs to be
decoded? Or are you doing the check as part of the jump instruction?

Overall the change looks ok, but I'm not sure if it's required. Do you
have an implementation that needs this?

Alistair

> > initialized to false during DisasContxt initialization for TB.
> >
> > [1] - https://github.com/riscv/riscv-cfi
> >
> > Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> > ---
> >  target/riscv/translate.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> > index df38db7553..76f61a39d3 100644
> > --- a/target/riscv/translate.c
> > +++ b/target/riscv/translate.c
> > @@ -1167,6 +1167,7 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
> >      ctx->pm_base_enabled = FIELD_EX32(tb_flags, TB_FLAGS, PM_BASE_ENABLED);
> >      ctx->itrigger = FIELD_EX32(tb_flags, TB_FLAGS, ITRIGGER);
> >      ctx->zero = tcg_constant_tl(0);
> > +    ctx->virt_inst_excp = false;
> >  }
> >
> >  static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
> > --
> > 2.25.1
> >
>

Re: [PATCH: fix for virt instr exception] target/riscv: fix for virtual instr exception

[Deepak Gupta]

I was traveling. Sorry for the late reply.

On Sun, Jan 29, 2023 at 2:49 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Sat, Jan 28, 2023 at 6:37 AM Deepak Gupta <debug@rivosinc.com> wrote:
> >
> > Please dis-regard this.
> > I've sent the patch to qemu-riscv@nongnu.org
>
> That's not entirely correct either.
>
> You can run the `./scripts/get_maintainer.pl` script on your patch to
> get the email addresses to send this patch to. qemu-devel is the main
> one, but it's also a good idea to CC the qemu-riscv list as well as
> maintainers.

Noted.

>
> >
> > On Sat, Jan 28, 2023 at 12:48 AM Deepak Gupta <debug@rivosinc.com> wrote:
> > >
> > > commit fb3f3730e4 added mechanism to generate virtual instruction
> > > exception during instruction decode when virt is enabled.
> > >
> > > However in some situations, illegal instruction exception can be raised
> > > due to state of CPU. One such situation is implementing branch tracking.
> > > [1] An indirect branch if doesn't land on a landing pad instruction, then
> > > cpu must raise an illegal instruction exception.
> > > Implementation would raise such expcetion due to missing landing pad inst
> > > and not due to decode. Thus DisasContext must have `virt_inst_excp`
>
> Isn't a landing pad instruction just an instruction that needs to be
> decoded? Or are you doing the check as part of the jump instruction?

Presence of landing pad instruction will not raise any illegal
instruction exception.
It's the absence of landing pad instruction at the target on an
indirect jump that must raise
illegal instruction exception.
The way I am implementing it is by leveraging the TB generation
mechanism and a TB flag.
An indirect jmp/call will be last instruction in a translation block
and thus can set a flag in TB flags.
Subsequent translation block must have landing pad instruction at the
start and it's translation will clear the TB flag.
If there is no landing pad instruction at start of translation block,
TB flag will not be cleared allowing to raise illegal instr exception.

In this case however it's raising an illegal instruction exception
because `virt_inst_excp` is initialized to probably `non-false` value.
More on the implementation below.

>
> Overall the change looks ok, but I'm not sure if it's required. Do you
> have an implementation that needs this?

RISCV Zisslpcfi extension: https://github.com/riscv/riscv-cfi
Qemu implementation for Zisslpcfi :
https://github.com/deepak0414/qemu/tree/gh_Zisslpcfi-0.2
Tests using which I am testing:
https://github.com/deepak0414/riscv-tests/blob/cfi_tests/isa/rv64zcfi/cfi.S

>
> Alistair
>
> > > initialized to false during DisasContxt initialization for TB.
> > >
> > > [1] - https://github.com/riscv/riscv-cfi
> > >
> > > Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> > > ---
> > >  target/riscv/translate.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> > > index df38db7553..76f61a39d3 100644
> > > --- a/target/riscv/translate.c
> > > +++ b/target/riscv/translate.c
> > > @@ -1167,6 +1167,7 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
> > >      ctx->pm_base_enabled = FIELD_EX32(tb_flags, TB_FLAGS, PM_BASE_ENABLED);
> > >      ctx->itrigger = FIELD_EX32(tb_flags, TB_FLAGS, ITRIGGER);
> > >      ctx->zero = tcg_constant_tl(0);
> > > +    ctx->virt_inst_excp = false;
> > >  }
> > >
> > >  static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
> > > --
> > > 2.25.1
> > >
> >

Re: [PATCH: fix for virt instr exception] target/riscv: fix for virtual instr exception

[Alistair Francis]

On Sat, Jan 28, 2023 at 6:36 AM Deepak Gupta <debug@rivosinc.com> wrote:
>
> commit fb3f3730e4 added mechanism to generate virtual instruction
> exception during instruction decode when virt is enabled.
>
> However in some situations, illegal instruction exception can be raised
> due to state of CPU. One such situation is implementing branch tracking.
> [1] An indirect branch if doesn't land on a landing pad instruction, then
> cpu must raise an illegal instruction exception.
> Implementation would raise such expcetion due to missing landing pad inst
> and not due to decode. Thus DisasContext must have `virt_inst_excp`
> initialized to false during DisasContxt initialization for TB.
>
> [1] - https://github.com/riscv/riscv-cfi
>
> Signed-off-by: Deepak Gupta <debug@rivosinc.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/translate.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index df38db7553..76f61a39d3 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -1167,6 +1167,7 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>      ctx->pm_base_enabled = FIELD_EX32(tb_flags, TB_FLAGS, PM_BASE_ENABLED);
>      ctx->itrigger = FIELD_EX32(tb_flags, TB_FLAGS, ITRIGGER);
>      ctx->zero = tcg_constant_tl(0);
> +    ctx->virt_inst_excp = false;
>  }
>
>  static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
> --
> 2.25.1
>
>

Re: [PATCH: fix for virt instr exception] target/riscv: fix for virtual instr exception

[Alistair Francis]

On Sat, Jan 28, 2023 at 6:36 AM Deepak Gupta <debug@rivosinc.com> wrote:
>
> commit fb3f3730e4 added mechanism to generate virtual instruction
> exception during instruction decode when virt is enabled.
>
> However in some situations, illegal instruction exception can be raised
> due to state of CPU. One such situation is implementing branch tracking.
> [1] An indirect branch if doesn't land on a landing pad instruction, then
> cpu must raise an illegal instruction exception.
> Implementation would raise such expcetion due to missing landing pad inst
> and not due to decode. Thus DisasContext must have `virt_inst_excp`
> initialized to false during DisasContxt initialization for TB.
>
> [1] - https://github.com/riscv/riscv-cfi
>
> Signed-off-by: Deepak Gupta <debug@rivosinc.com>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  target/riscv/translate.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index df38db7553..76f61a39d3 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -1167,6 +1167,7 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>      ctx->pm_base_enabled = FIELD_EX32(tb_flags, TB_FLAGS, PM_BASE_ENABLED);
>      ctx->itrigger = FIELD_EX32(tb_flags, TB_FLAGS, ITRIGGER);
>      ctx->zero = tcg_constant_tl(0);
> +    ctx->virt_inst_excp = false;
>  }
>
>  static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
> --
> 2.25.1
>
>

Re: [PATCH: fix for virt instr exception] target/riscv: fix for virtual instr exception

[Deepak Gupta]

Thank you!

On Sun, Feb 5, 2023 at 4:02 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Sat, Jan 28, 2023 at 6:36 AM Deepak Gupta <debug@rivosinc.com> wrote:
> >
> > commit fb3f3730e4 added mechanism to generate virtual instruction
> > exception during instruction decode when virt is enabled.
> >
> > However in some situations, illegal instruction exception can be raised
> > due to state of CPU. One such situation is implementing branch tracking.
> > [1] An indirect branch if doesn't land on a landing pad instruction, then
> > cpu must raise an illegal instruction exception.
> > Implementation would raise such expcetion due to missing landing pad inst
> > and not due to decode. Thus DisasContext must have `virt_inst_excp`
> > initialized to false during DisasContxt initialization for TB.
> >
> > [1] - https://github.com/riscv/riscv-cfi
> >
> > Signed-off-by: Deepak Gupta <debug@rivosinc.com>
>
> Thanks!
>
> Applied to riscv-to-apply.next
>
> Alistair
>
> > ---
> >  target/riscv/translate.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> > index df38db7553..76f61a39d3 100644
> > --- a/target/riscv/translate.c
> > +++ b/target/riscv/translate.c
> > @@ -1167,6 +1167,7 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
> >      ctx->pm_base_enabled = FIELD_EX32(tb_flags, TB_FLAGS, PM_BASE_ENABLED);
> >      ctx->itrigger = FIELD_EX32(tb_flags, TB_FLAGS, ITRIGGER);
> >      ctx->zero = tcg_constant_tl(0);
> > +    ctx->virt_inst_excp = false;
> >  }
> >
> >  static void riscv_tr_tb_start(DisasContextBase *db, CPUState *cpu)
> > --
> > 2.25.1
> >
> >