[PATCH v2 0/4] target/s390x: Exit tb after executing ex_value

[PATCH v2 0/4] target/s390x: Exit tb after executing ex_value

[Richard Henderson]

Ok, version 1 didn't work, so once more with feeling.
Can you give it a try, Sven?


r~


Richard Henderson (4):
  target/s390x: Remove DISAS_GOTO_TB
  target/s390x: Remove DISAS_PC_STALE
  target/s390x: Remove DISAS_PC_STALE_NOCHAIN
  target/s390x: Exit tb after executing ex_value

 target/s390x/tcg/translate.c | 42 +++++++++++++++---------------------
 1 file changed, 17 insertions(+), 25 deletions(-)

-- 
2.34.1

[PATCH v2 1/4] target/s390x: Remove DISAS_GOTO_TB

[Richard Henderson]

There is nothing to distinguish this from DISAS_NORETURN.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/tcg/translate.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index fd2433d625..e38ae9ce09 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -1123,9 +1123,6 @@ typedef struct {
    exiting the TB.  */
 #define DISAS_PC_UPDATED        DISAS_TARGET_0
 
-/* We have emitted one or more goto_tb.  No fixup required.  */
-#define DISAS_GOTO_TB           DISAS_TARGET_1
-
 /* We have updated the PC and CC values.  */
 #define DISAS_PC_CC_UPDATED     DISAS_TARGET_2
 
@@ -1189,7 +1186,7 @@ static DisasJumpType help_goto_direct(DisasContext *s, uint64_t dest)
         tcg_gen_goto_tb(0);
         tcg_gen_movi_i64(psw_addr, dest);
         tcg_gen_exit_tb(s->base.tb, 0);
-        return DISAS_GOTO_TB;
+        return DISAS_NORETURN;
     } else {
         tcg_gen_movi_i64(psw_addr, dest);
         per_branch(s, false);
@@ -1258,7 +1255,7 @@ static DisasJumpType help_branch(DisasContext *s, DisasCompare *c,
             tcg_gen_movi_i64(psw_addr, dest);
             tcg_gen_exit_tb(s->base.tb, 1);
 
-            ret = DISAS_GOTO_TB;
+            ret = DISAS_NORETURN;
         } else {
             /* Fallthru can use goto_tb, but taken branch cannot.  */
             /* Store taken branch destination before the brcond.  This
@@ -6634,7 +6631,6 @@ static void s390x_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
     DisasContext *dc = container_of(dcbase, DisasContext, base);
 
     switch (dc->base.is_jmp) {
-    case DISAS_GOTO_TB:
     case DISAS_NORETURN:
         break;
     case DISAS_TOO_MANY:
-- 
2.34.1

[PATCH v2 2/4] target/s390x: Remove DISAS_PC_STALE

[Richard Henderson]

There is nothing to distinguish this from DISAS_TOO_MANY.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/tcg/translate.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index e38ae9ce09..a3422c0eb0 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -1126,10 +1126,6 @@ typedef struct {
 /* We have updated the PC and CC values.  */
 #define DISAS_PC_CC_UPDATED     DISAS_TARGET_2
 
-/* We are exiting the TB, but have neither emitted a goto_tb, nor
-   updated the PC for the next instruction to be executed.  */
-#define DISAS_PC_STALE          DISAS_TARGET_3
-
 /* We are exiting the TB to the main loop.  */
 #define DISAS_PC_STALE_NOCHAIN  DISAS_TARGET_4
 
@@ -3993,7 +3989,7 @@ static DisasJumpType op_sacf(DisasContext *s, DisasOps *o)
 {
     gen_helper_sacf(cpu_env, o->in2);
     /* Addressing mode has changed, so end the block.  */
-    return DISAS_PC_STALE;
+    return DISAS_TOO_MANY;
 }
 #endif
 
@@ -4029,7 +4025,7 @@ static DisasJumpType op_sam(DisasContext *s, DisasOps *o)
     tcg_temp_free_i64(tsam);
 
     /* Always exit the TB, since we (may have) changed execution mode.  */
-    return DISAS_PC_STALE;
+    return DISAS_TOO_MANY;
 }
 
 static DisasJumpType op_sar(DisasContext *s, DisasOps *o)
@@ -6562,13 +6558,13 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
 
     /* io should be the last instruction in tb when icount is enabled */
     if (unlikely(icount && ret == DISAS_NEXT)) {
-        ret = DISAS_PC_STALE;
+        ret = DISAS_TOO_MANY;
     }
 
 #ifndef CONFIG_USER_ONLY
     if (s->base.tb->flags & FLAG_MASK_PER) {
         /* An exception might be triggered, save PSW if not already done.  */
-        if (ret == DISAS_NEXT || ret == DISAS_PC_STALE) {
+        if (ret == DISAS_NEXT || ret == DISAS_TOO_MANY) {
             tcg_gen_movi_i64(psw_addr, s->pc_tmp);
         }
 
@@ -6634,7 +6630,6 @@ static void s390x_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
     case DISAS_NORETURN:
         break;
     case DISAS_TOO_MANY:
-    case DISAS_PC_STALE:
     case DISAS_PC_STALE_NOCHAIN:
         update_psw_addr(dc);
         /* FALLTHRU */
-- 
2.34.1

[PATCH v2 3/4] target/s390x: Remove DISAS_PC_STALE_NOCHAIN

[Richard Henderson]

Replace this with a flag: exit_to_mainloop.
We can now control the exit for each of DISAS_TOO_MANY,
DISAS_PC_UPDATED, and DISAS_PC_CC_UPDATED, and fold in
the check for PER.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/tcg/translate.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index a3422c0eb0..eac59c3dd1 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -149,6 +149,7 @@ struct DisasContext {
     uint64_t pc_tmp;
     uint32_t ilen;
     enum cc_op cc_op;
+    bool exit_to_mainloop;
 };
 
 /* Information carried about a condition to be evaluated.  */
@@ -1126,9 +1127,6 @@ typedef struct {
 /* We have updated the PC and CC values.  */
 #define DISAS_PC_CC_UPDATED     DISAS_TARGET_2
 
-/* We are exiting the TB to the main loop.  */
-#define DISAS_PC_STALE_NOCHAIN  DISAS_TARGET_4
-
 
 /* Instruction flags */
 #define IF_AFP1     0x0001      /* r1 is a fp reg for HFP/FPS instructions */
@@ -3022,7 +3020,8 @@ static DisasJumpType op_lctl(DisasContext *s, DisasOps *o)
     tcg_temp_free_i32(r1);
     tcg_temp_free_i32(r3);
     /* Exit to main loop to reevaluate s390_cpu_exec_interrupt.  */
-    return DISAS_PC_STALE_NOCHAIN;
+    s->exit_to_mainloop = true;
+    return DISAS_TOO_MANY;
 }
 
 static DisasJumpType op_lctlg(DisasContext *s, DisasOps *o)
@@ -3033,7 +3032,8 @@ static DisasJumpType op_lctlg(DisasContext *s, DisasOps *o)
     tcg_temp_free_i32(r1);
     tcg_temp_free_i32(r3);
     /* Exit to main loop to reevaluate s390_cpu_exec_interrupt.  */
-    return DISAS_PC_STALE_NOCHAIN;
+    s->exit_to_mainloop = true;
+    return DISAS_TOO_MANY;
 }
 
 static DisasJumpType op_lra(DisasContext *s, DisasOps *o)
@@ -4283,7 +4283,8 @@ static DisasJumpType op_ssm(DisasContext *s, DisasOps *o)
 {
     tcg_gen_deposit_i64(psw_mask, psw_mask, o->in2, 56, 8);
     /* Exit to main loop to reevaluate s390_cpu_exec_interrupt.  */
-    return DISAS_PC_STALE_NOCHAIN;
+    s->exit_to_mainloop = true;
+    return DISAS_TOO_MANY;
 }
 
 static DisasJumpType op_stap(DisasContext *s, DisasOps *o)
@@ -4548,7 +4549,8 @@ static DisasJumpType op_stnosm(DisasContext *s, DisasOps *o)
     }
 
     /* Exit to main loop to reevaluate s390_cpu_exec_interrupt.  */
-    return DISAS_PC_STALE_NOCHAIN;
+    s->exit_to_mainloop = true;
+    return DISAS_TOO_MANY;
 }
 
 static DisasJumpType op_stura(DisasContext *s, DisasOps *o)
@@ -6591,6 +6593,7 @@ static void s390x_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
 
     dc->cc_op = CC_OP_DYNAMIC;
     dc->ex_value = dc->base.tb->cs_base;
+    dc->exit_to_mainloop = (dc->base.tb->flags & FLAG_MASK_PER);
 }
 
 static void s390x_tr_tb_start(DisasContextBase *db, CPUState *cs)
@@ -6630,7 +6633,6 @@ static void s390x_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
     case DISAS_NORETURN:
         break;
     case DISAS_TOO_MANY:
-    case DISAS_PC_STALE_NOCHAIN:
         update_psw_addr(dc);
         /* FALLTHRU */
     case DISAS_PC_UPDATED:
@@ -6640,8 +6642,7 @@ static void s390x_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
         /* FALLTHRU */
     case DISAS_PC_CC_UPDATED:
         /* Exit the TB, either by raising a debug exception or by return.  */
-        if ((dc->base.tb->flags & FLAG_MASK_PER) ||
-             dc->base.is_jmp == DISAS_PC_STALE_NOCHAIN) {
+        if (dc->exit_to_mainloop) {
             tcg_gen_exit_tb(NULL, 0);
         } else {
             tcg_gen_lookup_and_goto_ptr();
-- 
2.34.1

[PATCH v2 4/4] target/s390x: Exit tb after executing ex_value

[Richard Henderson]

When EXECUTE sets ex_value to interrupt the constructed instruction,
we implicitly disable interrupts so that the value is not corrupted.
Exit to the main loop after execution, so that we re-evaluate any
pending interrupts.

Reported-by: Sven Schnelle <svens@linux.ibm.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/tcg/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index eac59c3dd1..e2ee005671 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -6593,7 +6593,7 @@ static void s390x_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
 
     dc->cc_op = CC_OP_DYNAMIC;
     dc->ex_value = dc->base.tb->cs_base;
-    dc->exit_to_mainloop = (dc->base.tb->flags & FLAG_MASK_PER);
+    dc->exit_to_mainloop = (dc->base.tb->flags & FLAG_MASK_PER) || dc->ex_value;
 }
 
 static void s390x_tr_tb_start(DisasContextBase *db, CPUState *cs)
-- 
2.34.1

Re: [PATCH v2 0/4] target/s390x: Exit tb after executing ex_value

[Sven Schnelle]

Hi Richard,

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

> Ok, version 1 didn't work, so once more with feeling.
> Can you give it a try, Sven?

This seems to work. Thanks a lot!

Regards
Sven

Re: [PATCH v2 0/4] target/s390x: Exit tb after executing ex_value

[Thomas Huth]

On 02/07/2022 08.02, Richard Henderson wrote:
> Ok, version 1 didn't work, so once more with feeling.
> Can you give it a try, Sven?
> 
> 
> r~
> 
> 
> Richard Henderson (4):
>    target/s390x: Remove DISAS_GOTO_TB
>    target/s390x: Remove DISAS_PC_STALE
>    target/s390x: Remove DISAS_PC_STALE_NOCHAIN
>    target/s390x: Exit tb after executing ex_value
> 
>   target/s390x/tcg/translate.c | 42 +++++++++++++++---------------------
>   1 file changed, 17 insertions(+), 25 deletions(-)
> 

Thanks, queued to my s390x-next branch now:

  https://gitlab.com/thuth/qemu/-/commits/s390x-next/

  Thomas

Re: [PATCH v2 1/4] target/s390x: Remove DISAS_GOTO_TB

[David Hildenbrand]

On 02.07.22 08:02, Richard Henderson wrote:
> There is nothing to distinguish this from DISAS_NORETURN.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/s390x/tcg/translate.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
> index fd2433d625..e38ae9ce09 100644
> --- a/target/s390x/tcg/translate.c
> +++ b/target/s390x/tcg/translate.c
> @@ -1123,9 +1123,6 @@ typedef struct {
>     exiting the TB.  */
>  #define DISAS_PC_UPDATED        DISAS_TARGET_0
>  
> -/* We have emitted one or more goto_tb.  No fixup required.  */
> -#define DISAS_GOTO_TB           DISAS_TARGET_1
> -
>  /* We have updated the PC and CC values.  */
>  #define DISAS_PC_CC_UPDATED     DISAS_TARGET_2
>  
> @@ -1189,7 +1186,7 @@ static DisasJumpType help_goto_direct(DisasContext *s, uint64_t dest)
>          tcg_gen_goto_tb(0);
>          tcg_gen_movi_i64(psw_addr, dest);
>          tcg_gen_exit_tb(s->base.tb, 0);
> -        return DISAS_GOTO_TB;
> +        return DISAS_NORETURN;
>      } else {
>          tcg_gen_movi_i64(psw_addr, dest);
>          per_branch(s, false);
> @@ -1258,7 +1255,7 @@ static DisasJumpType help_branch(DisasContext *s, DisasCompare *c,
>              tcg_gen_movi_i64(psw_addr, dest);
>              tcg_gen_exit_tb(s->base.tb, 1);
>  
> -            ret = DISAS_GOTO_TB;
> +            ret = DISAS_NORETURN;
>          } else {
>              /* Fallthru can use goto_tb, but taken branch cannot.  */
>              /* Store taken branch destination before the brcond.  This
> @@ -6634,7 +6631,6 @@ static void s390x_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
>      DisasContext *dc = container_of(dcbase, DisasContext, base);
>  
>      switch (dc->base.is_jmp) {
> -    case DISAS_GOTO_TB:
>      case DISAS_NORETURN:
>          break;
>      case DISAS_TOO_MANY:

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 2/4] target/s390x: Remove DISAS_PC_STALE

[David Hildenbrand]

On 02.07.22 08:02, Richard Henderson wrote:
> There is nothing to distinguish this from DISAS_TOO_MANY.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/s390x/tcg/translate.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
> index e38ae9ce09..a3422c0eb0 100644
> --- a/target/s390x/tcg/translate.c
> +++ b/target/s390x/tcg/translate.c
> @@ -1126,10 +1126,6 @@ typedef struct {
>  /* We have updated the PC and CC values.  */
>  #define DISAS_PC_CC_UPDATED     DISAS_TARGET_2
>  
> -/* We are exiting the TB, but have neither emitted a goto_tb, nor
> -   updated the PC for the next instruction to be executed.  */
> -#define DISAS_PC_STALE          DISAS_TARGET_3
> -
>  /* We are exiting the TB to the main loop.  */
>  #define DISAS_PC_STALE_NOCHAIN  DISAS_TARGET_4
>  
> @@ -3993,7 +3989,7 @@ static DisasJumpType op_sacf(DisasContext *s, DisasOps *o)
>  {
>      gen_helper_sacf(cpu_env, o->in2);
>      /* Addressing mode has changed, so end the block.  */
> -    return DISAS_PC_STALE;
> +    return DISAS_TOO_MANY;
>  }
>  #endif
>  
> @@ -4029,7 +4025,7 @@ static DisasJumpType op_sam(DisasContext *s, DisasOps *o)
>      tcg_temp_free_i64(tsam);
>  
>      /* Always exit the TB, since we (may have) changed execution mode.  */
> -    return DISAS_PC_STALE;
> +    return DISAS_TOO_MANY;
>  }
>  
>  static DisasJumpType op_sar(DisasContext *s, DisasOps *o)
> @@ -6562,13 +6558,13 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
>  
>      /* io should be the last instruction in tb when icount is enabled */
>      if (unlikely(icount && ret == DISAS_NEXT)) {
> -        ret = DISAS_PC_STALE;
> +        ret = DISAS_TOO_MANY;
>      }
>  
>  #ifndef CONFIG_USER_ONLY
>      if (s->base.tb->flags & FLAG_MASK_PER) {
>          /* An exception might be triggered, save PSW if not already done.  */
> -        if (ret == DISAS_NEXT || ret == DISAS_PC_STALE) {
> +        if (ret == DISAS_NEXT || ret == DISAS_TOO_MANY) {
>              tcg_gen_movi_i64(psw_addr, s->pc_tmp);
>          }
>  
> @@ -6634,7 +6630,6 @@ static void s390x_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
>      case DISAS_NORETURN:
>          break;
>      case DISAS_TOO_MANY:
> -    case DISAS_PC_STALE:
>      case DISAS_PC_STALE_NOCHAIN:
>          update_psw_addr(dc);
>          /* FALLTHRU */

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 3/4] target/s390x: Remove DISAS_PC_STALE_NOCHAIN

[David Hildenbrand]

On 02.07.22 08:02, Richard Henderson wrote:
> Replace this with a flag: exit_to_mainloop.
> We can now control the exit for each of DISAS_TOO_MANY,
> DISAS_PC_UPDATED, and DISAS_PC_CC_UPDATED, and fold in
> the check for PER.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 4/4] target/s390x: Exit tb after executing ex_value

[David Hildenbrand]

On 02.07.22 08:02, Richard Henderson wrote:
> When EXECUTE sets ex_value to interrupt the constructed instruction,
> we implicitly disable interrupts so that the value is not corrupted.
> Exit to the main loop after execution, so that we re-evaluate any
> pending interrupts.
> 
> Reported-by: Sven Schnelle <svens@linux.ibm.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/s390x/tcg/translate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
> index eac59c3dd1..e2ee005671 100644
> --- a/target/s390x/tcg/translate.c
> +++ b/target/s390x/tcg/translate.c
> @@ -6593,7 +6593,7 @@ static void s390x_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>  
>      dc->cc_op = CC_OP_DYNAMIC;
>      dc->ex_value = dc->base.tb->cs_base;
> -    dc->exit_to_mainloop = (dc->base.tb->flags & FLAG_MASK_PER);
> +    dc->exit_to_mainloop = (dc->base.tb->flags & FLAG_MASK_PER) || dc->ex_value;
>  }
>  
>  static void s390x_tr_tb_start(DisasContextBase *db, CPUState *cs)

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb