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.
>
> When booting Fedora 30, I can see a handful of these exits and it seems
> to work reliable. (it never get's triggered via EXECUTE, though)
>
> Suggested-by: Richard Henderson <richard.hender...@linaro.org>
> Signed-off-by: David Hildenbrand <da...@redhat.com>
> ---
>
> v1 -> v2:
> - Check only if icount_decr.u32 < 0
> - Drop should_interrupt_instruction() and perform the check inline
> - Rephrase comment, subject, and description
>
> ---
> target/s390x/mem_helper.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> index 4254548935..87e4ebd169 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,14 @@ 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. Check if somebody (e.g., cpu_interrupt() or
> + * cpu_exit()) asked us to return to the main loop. In case there is
> + * no deliverable interrupt, we'll end up back in this handler.
> + */
> + if
> (unlikely((int32_t)atomic_read(&cpu_neg(cs)->icount_decr.u32) < 0)) {
I'm not sure about directly checking the icount_decr here. It really is
an internal implementation detail for the generated code. Having said
that is seems cpu_interrupt() is messing with this directly rather than
calling cpu_exit() which sets the more easily checked &cpu->exit_request.
This is potentially problematic as in other points in the cpu loop code
you see checks like this:
/* Finally, check if we need to exit to the main loop. */
if (unlikely(atomic_read(&cpu->exit_request))
|| (use_icount
&& cpu_neg(cpu)->icount_decr.u16.low + cpu->icount_extra == 0)) {
atomic_set(&cpu->exit_request, 0);
if (cpu->exception_index == -1) {
cpu->exception_index = EXCP_INTERRUPT;
}
return true;
}
although I guess this is because interrupts and "exits" take subtly
different paths through the outer loop. Given that exits and interrupts
are slightly different is what you want to check
atomic_read(&cpu->interrupt_request))?
> + cpu_loop_exit_restore(cs, ra);
> + }
> }
> return cc;
> }
--
Alex Bennée
