Re: [PATCH v4] s390x/tcg: MVCL: Exit to main loop if requested

[Alex Bennée]

David Hildenbrand <address@hidden> 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 <address@hidden>
> Cc: Peter Maydell <address@hidden>
> Cc: Paolo Bonzini <address@hidden>
> Suggested-by: Richard Henderson <address@hidden>
> Signed-off-by: David Hildenbrand <address@hidden>

Reviewed-by: Alex Bennée <address@hidden>

> ---
>
> 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