MVCL is interruptible and we should check for interrupts and process
them after writing back the variables to the registers. Let's check
for any exit requests and exit to the main loop. Introduce a new helper
function for that: cpu_loop_exit_requested().

When booting Fedora 30, I can see a handful of these exits and it seems
to work reliable. Also, Richard explained why this works correctly even
when MVCL is called via EXECUTE:

    (1) TB with EXECUTE runs, at address Ae
        - env->psw_addr stored with Ae.
        - helper_ex() runs, memory address Am computed
          from D2a(X2a,B2a) or from psw.addr+RI2.
        - env->ex_value stored with memory value modified by R1a

    (2) TB of executee runs,
        - env->ex_value stored with 0.
        - helper_mvcl() runs, using and updating R1b, R1b+1, R2b, R2b+1.

    (3a) helper_mvcl() completes,
         - TB of executee continues, psw.addr += ilen.
         - Next instruction is the one following EXECUTE.

    (3b) helper_mvcl() exits to main loop,
         - cpu_loop_exit_restore() unwinds psw.addr = Ae.
         - Next instruction is the EXECUTE itself...
         - goto 1.

As the PoP mentiones that an interruptible instruction called via EXECUTE
should avoid modifying storage/registers that are used by EXECUTE itself,
it is fine to retrigger EXECUTE.

Cc: Alex Bennée <alex.bennee@linaro.org>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---

v3 -> v4:
- Switch to cpu_loop_exit_requested() and perform the actual exit in the
  caller

v2 -> v3:
- Add TCG helper function
- Add details about EXECUTE to description
- Return to main loop only if there is work left to do

v1 -> v2:
- Check only if icount_decr.u32 < 0
- Drop should_interrupt_instruction() and perform the check inline
- Rephrase comment, subject, and description

---
 include/exec/exec-all.h   | 17 +++++++++++++++++
 target/s390x/mem_helper.c | 11 ++++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 49db07ba0b..04795c49bf 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -72,6 +72,23 @@ void QEMU_NORETURN cpu_loop_exit(CPUState *cpu);
 void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc);
 void QEMU_NORETURN cpu_loop_exit_atomic(CPUState *cpu, uintptr_t pc);
 
+/**
+ * cpu_loop_exit_requested:
+ * @cpu: The CPU state to be tested
+ *
+ * Indicate if somebody asked for a return of the CPU to the main loop
+ * (e.g., via cpu_exit() or cpu_interrupt()).
+ *
+ * This is helpful for architectures that support interruptible
+ * instructions. After writing back all state to registers/memory, this
+ * call can be used to check if it makes sense to return to the main loop
+ * or to continue executing the interruptible instruction.
+ */
+static inline bool cpu_loop_exit_requested(CPUState *cpu)
+{
+    return (int32_t)atomic_read(&cpu_neg(cpu)->icount_decr.u32) < 0;
+}
+
 #if !defined(CONFIG_USER_ONLY)
 void cpu_reloading_memory_map(void);
 /**
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 44e535856d..740728368c 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -1015,6 +1015,7 @@ uint32_t HELPER(mvcl)(CPUS390XState *env, uint32_t r1, uint32_t r2)
     uint64_t srclen = env->regs[r2 + 1] & 0xffffff;
     uint64_t src = get_address(env, r2);
     uint8_t pad = env->regs[r2 + 1] >> 24;
+    CPUState *cs = env_cpu(env);
     S390Access srca, desta;
     uint32_t cc, cur_len;
 
@@ -1065,7 +1066,15 @@ uint32_t HELPER(mvcl)(CPUS390XState *env, uint32_t r1, uint32_t r2)
         env->regs[r1 + 1] = deposit64(env->regs[r1 + 1], 0, 24, destlen);
         set_address_zero(env, r1, dest);
 
-        /* TODO: Deliver interrupts. */
+        /*
+         * MVCL is interruptible. Return to the main loop if requested after
+         * writing back all state to registers. If no interrupt will get
+         * injected, we'll end up back in this handler and continue processing
+         * the remaining parts.
+         */
+        if (destlen && unlikely(cpu_loop_exit_requested(cs))) {
+            cpu_loop_exit_restore(cs, ra);
+        }
     }
     return cc;
 }
-- 
2.21.0

David Hildenbrand <david@redhat.com> writes:

> MVCL is interruptible and we should check for interrupts and process
> them after writing back the variables to the registers. Let's check
> for any exit requests and exit to the main loop. Introduce a new helper
> function for that: cpu_loop_exit_requested().
>
> When booting Fedora 30, I can see a handful of these exits and it seems
> to work reliable. Also, Richard explained why this works correctly even
> when MVCL is called via EXECUTE:
>
>     (1) TB with EXECUTE runs, at address Ae
>         - env->psw_addr stored with Ae.
>         - helper_ex() runs, memory address Am computed
>           from D2a(X2a,B2a) or from psw.addr+RI2.
>         - env->ex_value stored with memory value modified by R1a
>
>     (2) TB of executee runs,
>         - env->ex_value stored with 0.
>         - helper_mvcl() runs, using and updating R1b, R1b+1, R2b, R2b+1.
>
>     (3a) helper_mvcl() completes,
>          - TB of executee continues, psw.addr += ilen.
>          - Next instruction is the one following EXECUTE.
>
>     (3b) helper_mvcl() exits to main loop,
>          - cpu_loop_exit_restore() unwinds psw.addr = Ae.
>          - Next instruction is the EXECUTE itself...
>          - goto 1.
>
> As the PoP mentiones that an interruptible instruction called via EXECUTE
> should avoid modifying storage/registers that are used by EXECUTE itself,
> it is fine to retrigger EXECUTE.
>
> Cc: Alex Bennée <alex.bennee@linaro.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: David Hildenbrand <david@redhat.com>

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

> ---
>
> v3 -> v4:
> - Switch to cpu_loop_exit_requested() and perform the actual exit in the
>   caller
>
> v2 -> v3:
> - Add TCG helper function
> - Add details about EXECUTE to description
> - Return to main loop only if there is work left to do
>
> v1 -> v2:
> - Check only if icount_decr.u32 < 0
> - Drop should_interrupt_instruction() and perform the check inline
> - Rephrase comment, subject, and description
>
> ---
>  include/exec/exec-all.h   | 17 +++++++++++++++++
>  target/s390x/mem_helper.c | 11 ++++++++++-
>  2 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 49db07ba0b..04795c49bf 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -72,6 +72,23 @@ void QEMU_NORETURN cpu_loop_exit(CPUState *cpu);
>  void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc);
>  void QEMU_NORETURN cpu_loop_exit_atomic(CPUState *cpu, uintptr_t pc);
>
> +/**
> + * cpu_loop_exit_requested:
> + * @cpu: The CPU state to be tested
> + *
> + * Indicate if somebody asked for a return of the CPU to the main loop
> + * (e.g., via cpu_exit() or cpu_interrupt()).
> + *
> + * This is helpful for architectures that support interruptible
> + * instructions. After writing back all state to registers/memory, this
> + * call can be used to check if it makes sense to return to the main loop
> + * or to continue executing the interruptible instruction.
> + */
> +static inline bool cpu_loop_exit_requested(CPUState *cpu)
> +{
> +    return (int32_t)atomic_read(&cpu_neg(cpu)->icount_decr.u32) < 0;
> +}
> +
>  #if !defined(CONFIG_USER_ONLY)
>  void cpu_reloading_memory_map(void);
>  /**
> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> index 44e535856d..740728368c 100644
> --- a/target/s390x/mem_helper.c
> +++ b/target/s390x/mem_helper.c
> @@ -1015,6 +1015,7 @@ uint32_t HELPER(mvcl)(CPUS390XState *env, uint32_t r1, uint32_t r2)
>      uint64_t srclen = env->regs[r2 + 1] & 0xffffff;
>      uint64_t src = get_address(env, r2);
>      uint8_t pad = env->regs[r2 + 1] >> 24;
> +    CPUState *cs = env_cpu(env);
>      S390Access srca, desta;
>      uint32_t cc, cur_len;
>
> @@ -1065,7 +1066,15 @@ uint32_t HELPER(mvcl)(CPUS390XState *env, uint32_t r1, uint32_t r2)
>          env->regs[r1 + 1] = deposit64(env->regs[r1 + 1], 0, 24, destlen);
>          set_address_zero(env, r1, dest);
>
> -        /* TODO: Deliver interrupts. */
> +        /*
> +         * MVCL is interruptible. Return to the main loop if requested after
> +         * writing back all state to registers. If no interrupt will get
> +         * injected, we'll end up back in this handler and continue processing
> +         * the remaining parts.
> +         */
> +        if (destlen && unlikely(cpu_loop_exit_requested(cs))) {
> +            cpu_loop_exit_restore(cs, ra);
> +        }
>      }
>      return cc;
>  }


--
Alex Bennée

On 10.10.19 10:26, Alex Bennée wrote:
> 
> David Hildenbrand <david@redhat.com> writes:
> 
>> MVCL is interruptible and we should check for interrupts and process
>> them after writing back the variables to the registers. Let's check
>> for any exit requests and exit to the main loop. Introduce a new helper
>> function for that: cpu_loop_exit_requested().
>>
>> When booting Fedora 30, I can see a handful of these exits and it seems
>> to work reliable. Also, Richard explained why this works correctly even
>> when MVCL is called via EXECUTE:
>>
>>     (1) TB with EXECUTE runs, at address Ae
>>         - env->psw_addr stored with Ae.
>>         - helper_ex() runs, memory address Am computed
>>           from D2a(X2a,B2a) or from psw.addr+RI2.
>>         - env->ex_value stored with memory value modified by R1a
>>
>>     (2) TB of executee runs,
>>         - env->ex_value stored with 0.
>>         - helper_mvcl() runs, using and updating R1b, R1b+1, R2b, R2b+1.
>>
>>     (3a) helper_mvcl() completes,
>>          - TB of executee continues, psw.addr += ilen.
>>          - Next instruction is the one following EXECUTE.
>>
>>     (3b) helper_mvcl() exits to main loop,
>>          - cpu_loop_exit_restore() unwinds psw.addr = Ae.
>>          - Next instruction is the EXECUTE itself...
>>          - goto 1.
>>
>> As the PoP mentiones that an interruptible instruction called via EXECUTE
>> should avoid modifying storage/registers that are used by EXECUTE itself,
>> it is fine to retrigger EXECUTE.
>>
>> Cc: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Thanks,

Queued to

https://github.com/davidhildenbrand/qemu.git s390-tcg-next


-- 

Thanks,

David / dhildenb

On 10/9/19 7:00 AM, David Hildenbrand wrote:
> MVCL is interruptible and we should check for interrupts and process
> them after writing back the variables to the registers. Let's check
> for any exit requests and exit to the main loop. Introduce a new helper
> function for that: cpu_loop_exit_requested().
> 
> When booting Fedora 30, I can see a handful of these exits and it seems
> to work reliable. Also, Richard explained why this works correctly even
> when MVCL is called via EXECUTE:
> 
>     (1) TB with EXECUTE runs, at address Ae
>         - env->psw_addr stored with Ae.
>         - helper_ex() runs, memory address Am computed
>           from D2a(X2a,B2a) or from psw.addr+RI2.
>         - env->ex_value stored with memory value modified by R1a
> 
>     (2) TB of executee runs,
>         - env->ex_value stored with 0.
>         - helper_mvcl() runs, using and updating R1b, R1b+1, R2b, R2b+1.
> 
>     (3a) helper_mvcl() completes,
>          - TB of executee continues, psw.addr += ilen.
>          - Next instruction is the one following EXECUTE.
> 
>     (3b) helper_mvcl() exits to main loop,
>          - cpu_loop_exit_restore() unwinds psw.addr = Ae.
>          - Next instruction is the EXECUTE itself...
>          - goto 1.
> 
> As the PoP mentiones that an interruptible instruction called via EXECUTE
> should avoid modifying storage/registers that are used by EXECUTE itself,
> it is fine to retrigger EXECUTE.
> 
> Cc: Alex Bennée <alex.bennee@linaro.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> 
> v3 -> v4:
> - Switch to cpu_loop_exit_requested() and perform the actual exit in the
>   caller

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~