David Hildenbrand <da...@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.ben...@linaro.org>
> Cc: Peter Maydell <peter.mayd...@linaro.org>
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Suggested-by: Richard Henderson <richard.hender...@linaro.org>
> Signed-off-by: David Hildenbrand <da...@redhat.com>
Reviewed-by: Alex Bennée <alex.ben...@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
