| Message ID | 20230127191758.755844-1-debug@rivosinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCH:,fix,for,virt,instr,exception] target/riscv: fix for virtual instr exception | expand |
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 > > >
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 > > > > >
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 > >
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 > >
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 > > > >
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)
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(+)