| Message ID | 20230405185922.2122668-1-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr | expand |
On 05/04/2023 19:59, Richard Henderson wrote: > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/sparc/translate.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/target/sparc/translate.c b/target/sparc/translate.c > index 137bdc5159..47940fd85e 100644 > --- a/target/sparc/translate.c > +++ b/target/sparc/translate.c > @@ -322,7 +322,7 @@ static void gen_goto_tb(DisasContext *s, int tb_num, > /* jump to another page: currently not optimized */ > tcg_gen_movi_tl(cpu_pc, pc); > tcg_gen_movi_tl(cpu_npc, npc); > - tcg_gen_exit_tb(NULL, 0); > + tcg_gen_lookup_and_goto_ptr(); > } > } > > @@ -4153,7 +4153,7 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn) > /* End TB to notice changed ASI. */ > save_state(dc); > gen_op_next_insn(); > - tcg_gen_exit_tb(NULL, 0); > + tcg_gen_lookup_and_goto_ptr(); > dc->base.is_jmp = DISAS_NORETURN; > break; > case 0x6: /* V9 wrfprs */ > @@ -4162,7 +4162,7 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn) > dc->fprs_dirty = 0; > save_state(dc); > gen_op_next_insn(); > - tcg_gen_exit_tb(NULL, 0); > + tcg_gen_lookup_and_goto_ptr(); > dc->base.is_jmp = DISAS_NORETURN; > break; > case 0xf: /* V9 sir, nop if user */ > @@ -5661,7 +5661,7 @@ static void sparc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs) > tcg_gen_movi_tl(cpu_pc, dc->pc); > } > save_npc(dc); > - tcg_gen_exit_tb(NULL, 0); > + tcg_gen_lookup_and_goto_ptr(); > } > break; I can certainly give this an R-B, however I'm fairly sure I tried this a couple of years back and found that it introduced random hangs on qemu-system-sparc64 :/. Have you seen any issues in the relevant avocado tests with this patch applied? ATB, Mark.
On 4/5/23 14:09, Mark Cave-Ayland wrote: > I can certainly give this an R-B, however I'm fairly sure I tried this a couple of years > back and found that it introduced random hangs on qemu-system-sparc64 :/. Have you seen > any issues in the relevant avocado tests with this patch applied? No issues. I suspect the last time this was tried all tcg_gen_exit_tb were replaced, including the one after changing psr. That would mean we wouldn't see exceptions after enabling. r~
Ping. r~ On 4/5/23 19:59, Richard Henderson wrote: > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/sparc/translate.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/target/sparc/translate.c b/target/sparc/translate.c > index 137bdc5159..47940fd85e 100644 > --- a/target/sparc/translate.c > +++ b/target/sparc/translate.c > @@ -322,7 +322,7 @@ static void gen_goto_tb(DisasContext *s, int tb_num, > /* jump to another page: currently not optimized */ > tcg_gen_movi_tl(cpu_pc, pc); > tcg_gen_movi_tl(cpu_npc, npc); > - tcg_gen_exit_tb(NULL, 0); > + tcg_gen_lookup_and_goto_ptr(); > } > } > > @@ -4153,7 +4153,7 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn) > /* End TB to notice changed ASI. */ > save_state(dc); > gen_op_next_insn(); > - tcg_gen_exit_tb(NULL, 0); > + tcg_gen_lookup_and_goto_ptr(); > dc->base.is_jmp = DISAS_NORETURN; > break; > case 0x6: /* V9 wrfprs */ > @@ -4162,7 +4162,7 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn) > dc->fprs_dirty = 0; > save_state(dc); > gen_op_next_insn(); > - tcg_gen_exit_tb(NULL, 0); > + tcg_gen_lookup_and_goto_ptr(); > dc->base.is_jmp = DISAS_NORETURN; > break; > case 0xf: /* V9 sir, nop if user */ > @@ -5661,7 +5661,7 @@ static void sparc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs) > tcg_gen_movi_tl(cpu_pc, dc->pc); > } > save_npc(dc); > - tcg_gen_exit_tb(NULL, 0); > + tcg_gen_lookup_and_goto_ptr(); > } > break; >
On 10/05/2023 11:23, Richard Henderson wrote: > Ping. > > r~ > > On 4/5/23 19:59, Richard Henderson wrote: >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> target/sparc/translate.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/target/sparc/translate.c b/target/sparc/translate.c >> index 137bdc5159..47940fd85e 100644 >> --- a/target/sparc/translate.c >> +++ b/target/sparc/translate.c >> @@ -322,7 +322,7 @@ static void gen_goto_tb(DisasContext *s, int tb_num, >> /* jump to another page: currently not optimized */ >> tcg_gen_movi_tl(cpu_pc, pc); >> tcg_gen_movi_tl(cpu_npc, npc); >> - tcg_gen_exit_tb(NULL, 0); >> + tcg_gen_lookup_and_goto_ptr(); >> } >> } >> @@ -4153,7 +4153,7 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int >> insn) >> /* End TB to notice changed ASI. */ >> save_state(dc); >> gen_op_next_insn(); >> - tcg_gen_exit_tb(NULL, 0); >> + tcg_gen_lookup_and_goto_ptr(); >> dc->base.is_jmp = DISAS_NORETURN; >> break; >> case 0x6: /* V9 wrfprs */ >> @@ -4162,7 +4162,7 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int >> insn) >> dc->fprs_dirty = 0; >> save_state(dc); >> gen_op_next_insn(); >> - tcg_gen_exit_tb(NULL, 0); >> + tcg_gen_lookup_and_goto_ptr(); >> dc->base.is_jmp = DISAS_NORETURN; >> break; >> case 0xf: /* V9 sir, nop if user */ >> @@ -5661,7 +5661,7 @@ static void sparc_tr_tb_stop(DisasContextBase *dcbase, >> CPUState *cs) >> tcg_gen_movi_tl(cpu_pc, dc->pc); >> } >> save_npc(dc); >> - tcg_gen_exit_tb(NULL, 0); >> + tcg_gen_lookup_and_goto_ptr(); >> } >> break; Obviously nothing notionally against this patch, however if you could give me a few days to run my OpenBIOS SPARC32/SPARC64 boot tests against git master with this patch applied to double-check there are no regressions, that would be great. ATB, Mark.
On 5/11/23 09:40, Mark Cave-Ayland wrote: > Obviously nothing notionally against this patch, however if you could give me a few days > to run my OpenBIOS SPARC32/SPARC64 boot tests against git master with this patch applied > to double-check there are no regressions, that would be great. No problem. I just didn't want it to get lost. r~
On 5/11/23 13:02, Richard Henderson wrote: > On 5/11/23 09:40, Mark Cave-Ayland wrote: >> Obviously nothing notionally against this patch, however if you could give me a few days >> to run my OpenBIOS SPARC32/SPARC64 boot tests against git master with this patch applied >> to double-check there are no regressions, that would be great. > > No problem. I just didn't want it to get lost. Ping for results? r~
On 5/4/23 20:59, Richard Henderson wrote: > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/sparc/translate.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/target/sparc/translate.c b/target/sparc/translate.c > index 137bdc5159..47940fd85e 100644 > --- a/target/sparc/translate.c > +++ b/target/sparc/translate.c > @@ -322,7 +322,7 @@ static void gen_goto_tb(DisasContext *s, int tb_num, [expanding diff context] if (use_goto_tb(s, pc, npc)) { /* jump to same page: we can use a direct jump */ tcg_gen_goto_tb(tb_num); tcg_gen_movi_tl(cpu_pc, pc); tcg_gen_movi_tl(cpu_npc, npc); tcg_gen_exit_tb(s->base.tb, tb_num); } else { > /* jump to another page: currently not optimized */ > tcg_gen_movi_tl(cpu_pc, pc); > tcg_gen_movi_tl(cpu_npc, npc); > - tcg_gen_exit_tb(NULL, 0); > + tcg_gen_lookup_and_goto_ptr(); Per https://qemu.readthedocs.io/en/latest/devel/tcg.html#lookup-and-goto-ptr [*]: This helper will look for an existing TB that matches the current CPU state. If the destination TB is available its code address is returned, otherwise the address of the JIT epilogue is returned. OK. IIUC this is the "optimized" form (trying to not exit the current TB). Should the comment be updated to "/* jump to another page */"? > } > } > > @@ -4153,7 +4153,7 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn) [expanding diff context] case 0x3: /* V9 wrasi */ wrasi = "Write ASI register" instruction. > /* End TB to notice changed ASI. */ > save_state(dc); > gen_op_next_insn(); > - tcg_gen_exit_tb(NULL, 0); > + tcg_gen_lookup_and_goto_ptr(); Memory mapping is not changed, CPU state change is constant, no interrupt updated, OK. > dc->base.is_jmp = DISAS_NORETURN; > break; > case 0x6: /* V9 wrfprs */ wrfprs = "Write floating-point registers state register". > dc->fprs_dirty = 0; > save_state(dc); > gen_op_next_insn(); > - tcg_gen_exit_tb(NULL, 0); > + tcg_gen_lookup_and_goto_ptr(); Similar analysis, OK. > dc->base.is_jmp = DISAS_NORETURN; > break; > case 0xf: /* V9 sir, nop if user */ > @@ -5661,7 +5661,7 @@ static void sparc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs) [expanding diff context] switch (dc->base.is_jmp) { case DISAS_NEXT: case DISAS_TOO_MANY: if (dc->pc != DYNAMIC_PC && (dc->npc != DYNAMIC_PC && dc->npc != JUMP_PC)) { /* static PC and NPC: we can use direct chaining */ gen_goto_tb(dc, 0, dc->pc, dc->npc); } else { if (dc->pc != DYNAMIC_PC) { > tcg_gen_movi_tl(cpu_pc, dc->pc); > } > save_npc(dc); > - tcg_gen_exit_tb(NULL, 0); > + tcg_gen_lookup_and_goto_ptr(); Per [*] "we either branch to the next TB or return to the main loop." So here we just perform an indirect branch, possibly using the JIT epilogue instead of directly returning to the main loop. OK. To the best of my knowledge: Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > } > break; >
On 19/06/2023 16:41, Richard Henderson wrote: > On 5/11/23 13:02, Richard Henderson wrote: >> On 5/11/23 09:40, Mark Cave-Ayland wrote: >>> Obviously nothing notionally against this patch, however if you could give me a >>> few days to run my OpenBIOS SPARC32/SPARC64 boot tests against git master with >>> this patch applied to double-check there are no regressions, that would be great. >> >> No problem. I just didn't want it to get lost. > > Ping for results? > > r~ Sorry I haven't had a chance to test this yet - I'll try and get to it later today. ATB, Mark.
On 21/06/2023 10:14, Mark Cave-Ayland wrote: > On 19/06/2023 16:41, Richard Henderson wrote: > >> On 5/11/23 13:02, Richard Henderson wrote: >>> On 5/11/23 09:40, Mark Cave-Ayland wrote: >>>> Obviously nothing notionally against this patch, however if you could give me a >>>> few days to run my OpenBIOS SPARC32/SPARC64 boot tests against git master with >>>> this patch applied to double-check there are no regressions, that would be great. >>> >>> No problem. I just didn't want it to get lost. >> >> Ping for results? >> >> r~ > > Sorry I haven't had a chance to test this yet - I'll try and get to it later today. I got as far as running my OpenBIOS sparc32 tests, and I'm seeing an issue with my Solaris 8 image in that the mouse is frozen when booting into the GUI with this patch applied so looks like something still isn't right here :( ATB, Mark.
On 21/6/23 15:47, Mark Cave-Ayland wrote: > On 21/06/2023 10:14, Mark Cave-Ayland wrote: > >> On 19/06/2023 16:41, Richard Henderson wrote: >> >>> On 5/11/23 13:02, Richard Henderson wrote: >>>> On 5/11/23 09:40, Mark Cave-Ayland wrote: >>>>> Obviously nothing notionally against this patch, however if you >>>>> could give me a few days to run my OpenBIOS SPARC32/SPARC64 boot >>>>> tests against git master with this patch applied to double-check >>>>> there are no regressions, that would be great. >>>> >>>> No problem. I just didn't want it to get lost. >>> >>> Ping for results? >>> >>> r~ >> >> Sorry I haven't had a chance to test this yet - I'll try and get to it >> later today. > > I got as far as running my OpenBIOS sparc32 tests, and I'm seeing an > issue with my Solaris 8 image in that the mouse is frozen when booting > into the GUI with this patch applied so looks like something still isn't > right here :( Could you isolate the 4 changes [*] to see which one breaks? [*] https://lore.kernel.org/qemu-devel/676ac594-77c6-3953-7355-1b96a09d93df@linaro.org/
diff --git a/target/sparc/translate.c b/target/sparc/translate.c index 137bdc5159..47940fd85e 100644 --- a/target/sparc/translate.c +++ b/target/sparc/translate.c @@ -322,7 +322,7 @@ static void gen_goto_tb(DisasContext *s, int tb_num, /* jump to another page: currently not optimized */ tcg_gen_movi_tl(cpu_pc, pc); tcg_gen_movi_tl(cpu_npc, npc); - tcg_gen_exit_tb(NULL, 0); + tcg_gen_lookup_and_goto_ptr(); } } @@ -4153,7 +4153,7 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn) /* End TB to notice changed ASI. */ save_state(dc); gen_op_next_insn(); - tcg_gen_exit_tb(NULL, 0); + tcg_gen_lookup_and_goto_ptr(); dc->base.is_jmp = DISAS_NORETURN; break; case 0x6: /* V9 wrfprs */ @@ -4162,7 +4162,7 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn) dc->fprs_dirty = 0; save_state(dc); gen_op_next_insn(); - tcg_gen_exit_tb(NULL, 0); + tcg_gen_lookup_and_goto_ptr(); dc->base.is_jmp = DISAS_NORETURN; break; case 0xf: /* V9 sir, nop if user */ @@ -5661,7 +5661,7 @@ static void sparc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs) tcg_gen_movi_tl(cpu_pc, dc->pc); } save_npc(dc); - tcg_gen_exit_tb(NULL, 0); + tcg_gen_lookup_and_goto_ptr(); } break;
Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/sparc/translate.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)