tcg/sparc64: Disable direct linking for goto_tb

[PATCH for-8.0] tcg/sparc64: Disable direct linking for goto_tb

Posted by Richard Henderson 1 year, 1 month ago

Something is wrong with this code, and also wrong with gdb on the
sparc systems to which I have access, so I cannot debug it either.
Disable for now, so the release is not broken.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/sparc64/tcg-target.c.inc | 30 ++++--------------------------
 1 file changed, 4 insertions(+), 26 deletions(-)

diff --git a/tcg/sparc64/tcg-target.c.inc b/tcg/sparc64/tcg-target.c.inc
index ccc4144f7c..694f2b9dd4 100644
--- a/tcg/sparc64/tcg-target.c.inc
+++ b/tcg/sparc64/tcg-target.c.inc
@@ -1445,12 +1445,12 @@ static void tcg_out_goto_tb(TCGContext *s, int which)
 {
     ptrdiff_t off = tcg_tbrel_diff(s, (void *)get_jmp_target_addr(s, which));
 
-    /* Direct branch will be patched by tb_target_set_jmp_target. */
+    /* Load link and indirect branch. */
     set_jmp_insn_offset(s, which);
-    tcg_out32(s, CALL);
-    /* delay slot */
-    tcg_debug_assert(check_fit_ptr(off, 13));
     tcg_out_ld(s, TCG_TYPE_PTR, TCG_REG_TB, TCG_REG_TB, off);
+    tcg_out_arithi(s, TCG_REG_G0, TCG_REG_TB, 0, JMPL);
+    /* delay slot */
+    tcg_out_nop(s);
     set_jmp_reset_offset(s, which);
 
     /*
@@ -1469,28 +1469,6 @@ static void tcg_out_goto_tb(TCGContext *s, int which)
 void tb_target_set_jmp_target(const TranslationBlock *tb, int n,
                               uintptr_t jmp_rx, uintptr_t jmp_rw)
 {
-    uintptr_t addr = tb->jmp_target_addr[n];
-    intptr_t br_disp = (intptr_t)(addr - jmp_rx) >> 2;
-    tcg_insn_unit insn;
-
-    br_disp >>= 2;
-    if (check_fit_ptr(br_disp, 19)) {
-        /* ba,pt %icc, addr */
-        insn = deposit32(INSN_OP(0) | INSN_OP2(1) | INSN_COND(COND_A)
-                         | BPCC_ICC | BPCC_PT, 0, 19, br_disp);
-    } else if (check_fit_ptr(br_disp, 22)) {
-        /* ba addr */
-        insn = deposit32(INSN_OP(0) | INSN_OP2(2) | INSN_COND(COND_A),
-                         0, 22, br_disp);
-    } else {
-        /* The code_gen_buffer can't be larger than 2GB.  */
-        tcg_debug_assert(check_fit_ptr(br_disp, 30));
-        /* call addr */
-        insn = deposit32(CALL, 0, 30, br_disp);
-    }
-
-    qatomic_set((uint32_t *)jmp_rw, insn);
-    flush_idcache_range(jmp_rx, jmp_rw, 4);
 }
 
 static void tcg_out_op(TCGContext *s, TCGOpcode opc,
-- 
2.34.1

Re: [PATCH for-8.0] tcg/sparc64: Disable direct linking for goto_tb

Posted by Jordan Niethe 10 months ago


On 5/4/23 1:04 am, Richard Henderson wrote:
> Something is wrong with this code, and also wrong with gdb on the
> sparc systems to which I have access, so I cannot debug it either.
> Disable for now, so the release is not broken.

I'm not sure if it is the entire problem but it looks like the broken 
code had the same race as on ppc [1] between loading TCG_REG_TB and 
patching and executing the direct branch.

[1] 
https://lore.kernel.org/qemu-devel/20230717093001.13167-1-jniethe5@gmail.com/#t

> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   tcg/sparc64/tcg-target.c.inc | 30 ++++--------------------------
>   1 file changed, 4 insertions(+), 26 deletions(-)
> 
> diff --git a/tcg/sparc64/tcg-target.c.inc b/tcg/sparc64/tcg-target.c.inc
> index ccc4144f7c..694f2b9dd4 100644
> --- a/tcg/sparc64/tcg-target.c.inc
> +++ b/tcg/sparc64/tcg-target.c.inc
> @@ -1445,12 +1445,12 @@ static void tcg_out_goto_tb(TCGContext *s, int which)
>   {
>       ptrdiff_t off = tcg_tbrel_diff(s, (void *)get_jmp_target_addr(s, which));
>   
> -    /* Direct branch will be patched by tb_target_set_jmp_target. */
> +    /* Load link and indirect branch. */
>       set_jmp_insn_offset(s, which);
> -    tcg_out32(s, CALL);
> -    /* delay slot */
> -    tcg_debug_assert(check_fit_ptr(off, 13));
>       tcg_out_ld(s, TCG_TYPE_PTR, TCG_REG_TB, TCG_REG_TB, off);
> +    tcg_out_arithi(s, TCG_REG_G0, TCG_REG_TB, 0, JMPL);
> +    /* delay slot */
> +    tcg_out_nop(s);
>       set_jmp_reset_offset(s, which);
>   
>       /*
> @@ -1469,28 +1469,6 @@ static void tcg_out_goto_tb(TCGContext *s, int which)
>   void tb_target_set_jmp_target(const TranslationBlock *tb, int n,
>                                 uintptr_t jmp_rx, uintptr_t jmp_rw)
>   {
> -    uintptr_t addr = tb->jmp_target_addr[n];
> -    intptr_t br_disp = (intptr_t)(addr - jmp_rx) >> 2;
> -    tcg_insn_unit insn;
> -
> -    br_disp >>= 2;
> -    if (check_fit_ptr(br_disp, 19)) {
> -        /* ba,pt %icc, addr */
> -        insn = deposit32(INSN_OP(0) | INSN_OP2(1) | INSN_COND(COND_A)
> -                         | BPCC_ICC | BPCC_PT, 0, 19, br_disp);
> -    } else if (check_fit_ptr(br_disp, 22)) {
> -        /* ba addr */
> -        insn = deposit32(INSN_OP(0) | INSN_OP2(2) | INSN_COND(COND_A),
> -                         0, 22, br_disp);
> -    } else {
> -        /* The code_gen_buffer can't be larger than 2GB.  */
> -        tcg_debug_assert(check_fit_ptr(br_disp, 30));
> -        /* call addr */
> -        insn = deposit32(CALL, 0, 30, br_disp);
> -    }
> -
> -    qatomic_set((uint32_t *)jmp_rw, insn);
> -    flush_idcache_range(jmp_rx, jmp_rw, 4);
>   }
>   
>   static void tcg_out_op(TCGContext *s, TCGOpcode opc,
>

Re: [PATCH for-8.0] tcg/sparc64: Disable direct linking for goto_tb

Posted by Richard Henderson 9 months, 4 weeks ago

On 7/19/23 02:03, Jordan Niethe wrote:
> 
> 
> On 5/4/23 1:04 am, Richard Henderson wrote:
>> Something is wrong with this code, and also wrong with gdb on the
>> sparc systems to which I have access, so I cannot debug it either.
>> Disable for now, so the release is not broken.
> 
> I'm not sure if it is the entire problem but it looks like the broken code had the same 
> race as on ppc [1] between loading TCG_REG_TB and patching and executing the direct branch.
> 
> [1] https://lore.kernel.org/qemu-devel/20230717093001.13167-1-jniethe5@gmail.com/#t

Probably, yes.  Thanks for the reminder.


r~

Re: [PATCH for-8.0] tcg/sparc64: Disable direct linking for goto_tb

Posted by Alex Bennée 1 year, 1 month ago

Richard Henderson <richard.henderson@linaro.org> writes:

> Something is wrong with this code, and also wrong with gdb on the
> sparc systems to which I have access, so I cannot debug it either.
> Disable for now, so the release is not broken.

Why isn't this a revert then?

>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  tcg/sparc64/tcg-target.c.inc | 30 ++++--------------------------
>  1 file changed, 4 insertions(+), 26 deletions(-)
>
> diff --git a/tcg/sparc64/tcg-target.c.inc b/tcg/sparc64/tcg-target.c.inc
> index ccc4144f7c..694f2b9dd4 100644
> --- a/tcg/sparc64/tcg-target.c.inc
> +++ b/tcg/sparc64/tcg-target.c.inc
> @@ -1445,12 +1445,12 @@ static void tcg_out_goto_tb(TCGContext *s, int which)
>  {
>      ptrdiff_t off = tcg_tbrel_diff(s, (void *)get_jmp_target_addr(s, which));
>  
> -    /* Direct branch will be patched by tb_target_set_jmp_target. */
> +    /* Load link and indirect branch. */
>      set_jmp_insn_offset(s, which);
> -    tcg_out32(s, CALL);
> -    /* delay slot */
> -    tcg_debug_assert(check_fit_ptr(off, 13));
>      tcg_out_ld(s, TCG_TYPE_PTR, TCG_REG_TB, TCG_REG_TB, off);
> +    tcg_out_arithi(s, TCG_REG_G0, TCG_REG_TB, 0, JMPL);
> +    /* delay slot */
> +    tcg_out_nop(s);
>      set_jmp_reset_offset(s, which);
>  
>      /*
> @@ -1469,28 +1469,6 @@ static void tcg_out_goto_tb(TCGContext *s, int which)
>  void tb_target_set_jmp_target(const TranslationBlock *tb, int n,
>                                uintptr_t jmp_rx, uintptr_t jmp_rw)
>  {
> -    uintptr_t addr = tb->jmp_target_addr[n];
> -    intptr_t br_disp = (intptr_t)(addr - jmp_rx) >> 2;
> -    tcg_insn_unit insn;
> -
> -    br_disp >>= 2;
> -    if (check_fit_ptr(br_disp, 19)) {
> -        /* ba,pt %icc, addr */
> -        insn = deposit32(INSN_OP(0) | INSN_OP2(1) | INSN_COND(COND_A)
> -                         | BPCC_ICC | BPCC_PT, 0, 19, br_disp);
> -    } else if (check_fit_ptr(br_disp, 22)) {
> -        /* ba addr */
> -        insn = deposit32(INSN_OP(0) | INSN_OP2(2) | INSN_COND(COND_A),
> -                         0, 22, br_disp);
> -    } else {
> -        /* The code_gen_buffer can't be larger than 2GB.  */
> -        tcg_debug_assert(check_fit_ptr(br_disp, 30));
> -        /* call addr */
> -        insn = deposit32(CALL, 0, 30, br_disp);
> -    }
> -
> -    qatomic_set((uint32_t *)jmp_rw, insn);
> -    flush_idcache_range(jmp_rx, jmp_rw, 4);

So the result it we never patch the jump so return to the main loop
after every block?

In so far this won't break anything else and I suspect you are one of
the last people who actually uses the backend:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH for-8.0] tcg/sparc64: Disable direct linking for goto_tb

Posted by Richard Henderson 1 year, 1 month ago

On 4/4/23 08:32, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> Something is wrong with this code, and also wrong with gdb on the
>> sparc systems to which I have access, so I cannot debug it either.
>> Disable for now, so the release is not broken.
> 
> Why isn't this a revert then?

A revert of a228ae3ea7f6 wouldn't build.
This was part of an infrastructure change.

> So the result it we never patch the jump so return to the main loop
> after every block?

No, we simply perform an indirect branch to the next block.
Slower than a direct branch, but way faster than returning
to the main loop.

> In so far this won't break anything else and I suspect you are one of
> the last people who actually uses the backend:

Probably.

> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Thanks,


r~