[PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr

[PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr

[Richard Henderson]

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;
 
-- 
2.34.1

Re: [PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr

[Mark Cave-Ayland]

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.

Re: [PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr

[Richard Henderson]

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~

Re: [PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr

[Richard Henderson]

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;
>   

Re: [PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr

[Mark Cave-Ayland]

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.

Re: [PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr

[Richard Henderson]

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~

Re: [PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr

[Richard Henderson]

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~

Re: [PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr

[Philippe Mathieu-Daudé]

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;
>   

Re: [PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr

[Mark Cave-Ayland]

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.

Re: [PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr

[Mark Cave-Ayland]

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.

Re: [PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr

[Philippe Mathieu-Daudé]

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/