Re: [PATCH v2 0/8] target/sparc: Use tcg_gen_lookup_and_goto_ptr

[Philippe Mathieu-Daudé]

On 27/6/23 08:46, Mark Cave-Ayland wrote:
> On 22/06/2023 13:26, Mark Cave-Ayland wrote:
>
> > On 21/06/2023 19:05, Richard Henderson wrote:
> >
> > > Changes from v1:
> > >    * Split into teeny weeny pieces.
> > >
> > >    * It turns out the sparc_tr_tb_stop hunk of v1 was buggy,
> > >      in that things that are not simple branches use DYNAMIC_PC,
> > >      e.g. the RETT (return from trap) instruction.
> > >
> > >      Introduce DYNAMIC_PC_LOOKUP to distinguish the couple of
> > >      places where we have a dynamic pc, but no other change
> > >      of state (conditional branches, JMPL, RETURN).
> > >
> > >    * Drop the change for WRFPRS, because it's too infrequent.
> > >      The WRASI change affects memcpy/memset, so that's more important.
> > >
> > > Boots Mark's sol8 install cdrom.  :-)
> > >
> > > Top of the profile changes from
> > >
> > >      41.55%  qemu-system-sparc              [.] cpu_exec_loop
> > >      14.02%  qemu-system-sparc              [.] cpu_tb_exec
> > >       8.74%  qemu-system-sparc              [.] tb_lookup
> > >       2.11%  qemu-system-sparc              [.] tcg_splitwx_to_rw
> > >       1.63%  memfd:tcg-jit (deleted)        [.] 0x0000000000000004
> > >
> > > to
> > >
> > >      31.59%  qemu-system-sparc              [.] helper_lookup_tb_ptr
> > >      17.79%  qemu-system-sparc              [.] tb_lookup
> > >       5.38%  qemu-system-sparc              [.] compute_all_sub
> > >       2.38%  qemu-system-sparc              [.] helper_compute_psr
> > >       2.36%  qemu-system-sparc              [.] helper_check_align
> > >       1.79%  memfd:tcg-jit (deleted)        [.] 0x000000000063fc8e
> > >
> > > This probably indicates that cpu_get_tb_cpu_state could be
> > > improved to not consume so much overhead.
> >
> > Nice! I've just run this through all of my sun4m/sun4u/sun4v test 
> > images and I don't see any regressions with v2. The guests feel 
> > noticeably more responsive too :)
> >
> > Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >
> > I've skimmed the patches and without looking in too much detail they 
> > seem to be okay so I'm happy to give:
> >
> > Acked-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >
> > Side note: the niagara tests require the patch at 
> > https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg03537.html 
> > which still hasn't been merged yet.
> >
> > > Richard Henderson (8):
> > >    target/sparc: Use tcg_gen_lookup_and_goto_ptr in gen_goto_tb
> > >    target/sparc: Fix npc comparison in sparc_tr_insn_start
> > >    target/sparc: Drop inline markers from translate.c
> > >    target/sparc: Introduce DYNAMIC_PC_LOOKUP
> > >    target/sparc: Use DYNAMIC_PC_LOOKUP for conditional branches
> > >    target/sparc: Use DYNAMIC_PC_LOOKUP for JMPL
> > >    target/sparc: Use DYNAMIC_PC_LOOKUP for v9 RETURN
> > >    target/sparc: Use tcg_gen_lookup_and_goto_ptr for v9 WRASI
> > >
> > >   target/sparc/translate.c | 410 ++++++++++++++++++++++-----------------
> > >   1 file changed, 233 insertions(+), 177 deletions(-)
>
> I've just noticed during testing there is an issue with this series when 
> used with a real SS-5 PROM image (I was using OpenBIOS for my previous 
> tests) which causes it to assert() almost immediately on startup:
>
> $ ./qemu-system-sparc -bios ss5.bin
> ERROR:../target/sparc/translate.c:5695:sparc_tr_tb_stop: code should not 
> be reached
> Bail out! ERROR:../target/sparc/translate.c:5695:sparc_tr_tb_stop: code 
> should not be reached
> Aborted

Could you try this fix:

-- >8 --
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -5682,5 +5682,5 @@ static void sparc_tr_tb_stop(DisasContextBase 
*dcbase, CPUState *cs)

         save_npc(dc);
-        switch (dc->npc) {
+        switch (dc->npc & 3) {
         case DYNAMIC_PC_LOOKUP:
             if (may_lookup) {
---

?