* Re: [PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr
2023-04-05 18:59 [PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr Richard Henderson
@ 2023-04-05 21:09 ` Mark Cave-Ayland
2023-04-05 23:02 ` Richard Henderson
2023-05-10 10:23 ` Richard Henderson
2023-06-20 11:21 ` Philippe Mathieu-Daudé
2 siblings, 1 reply; 11+ messages in thread
From: Mark Cave-Ayland @ 2023-04-05 21:09 UTC (permalink / raw)
To: Richard Henderson, qemu-devel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr
2023-04-05 21:09 ` Mark Cave-Ayland
@ 2023-04-05 23:02 ` Richard Henderson
0 siblings, 0 replies; 11+ messages in thread
From: Richard Henderson @ 2023-04-05 23:02 UTC (permalink / raw)
To: Mark Cave-Ayland, qemu-devel
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~
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr
2023-04-05 18:59 [PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr Richard Henderson
2023-04-05 21:09 ` Mark Cave-Ayland
@ 2023-05-10 10:23 ` Richard Henderson
2023-05-11 8:40 ` Mark Cave-Ayland
2023-06-20 11:21 ` Philippe Mathieu-Daudé
2 siblings, 1 reply; 11+ messages in thread
From: Richard Henderson @ 2023-05-10 10:23 UTC (permalink / raw)
To: qemu-devel; +Cc: mark.cave-ayland
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;
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr
2023-05-10 10:23 ` Richard Henderson
@ 2023-05-11 8:40 ` Mark Cave-Ayland
2023-05-11 11:02 ` Richard Henderson
0 siblings, 1 reply; 11+ messages in thread
From: Mark Cave-Ayland @ 2023-05-11 8:40 UTC (permalink / raw)
To: Richard Henderson, qemu-devel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr
2023-05-11 8:40 ` Mark Cave-Ayland
@ 2023-05-11 11:02 ` Richard Henderson
2023-06-19 15:41 ` Richard Henderson
0 siblings, 1 reply; 11+ messages in thread
From: Richard Henderson @ 2023-05-11 11:02 UTC (permalink / raw)
To: Mark Cave-Ayland, qemu-devel
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~
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr
2023-05-11 11:02 ` Richard Henderson
@ 2023-06-19 15:41 ` Richard Henderson
2023-06-21 9:14 ` Mark Cave-Ayland
0 siblings, 1 reply; 11+ messages in thread
From: Richard Henderson @ 2023-06-19 15:41 UTC (permalink / raw)
To: Mark Cave-Ayland, qemu-devel
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~
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr
2023-06-19 15:41 ` Richard Henderson
@ 2023-06-21 9:14 ` Mark Cave-Ayland
2023-06-21 13:47 ` Mark Cave-Ayland
0 siblings, 1 reply; 11+ messages in thread
From: Mark Cave-Ayland @ 2023-06-21 9:14 UTC (permalink / raw)
To: Richard Henderson, qemu-devel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr
2023-06-21 9:14 ` Mark Cave-Ayland
@ 2023-06-21 13:47 ` Mark Cave-Ayland
2023-06-21 14:17 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 11+ messages in thread
From: Mark Cave-Ayland @ 2023-06-21 13:47 UTC (permalink / raw)
To: Richard Henderson, qemu-devel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr
2023-06-21 13:47 ` Mark Cave-Ayland
@ 2023-06-21 14:17 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-06-21 14:17 UTC (permalink / raw)
To: Mark Cave-Ayland, Richard Henderson, qemu-devel
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/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr
2023-04-05 18:59 [PATCH for-8.1] target/sparc: Use tcg_gen_lookup_and_goto_ptr Richard Henderson
2023-04-05 21:09 ` Mark Cave-Ayland
2023-05-10 10:23 ` Richard Henderson
@ 2023-06-20 11:21 ` Philippe Mathieu-Daudé
2 siblings, 0 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-06-20 11:21 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: mark.cave-ayland, Artyom Tarasenko
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;
>
^ permalink raw reply [flat|nested] 11+ messages in thread