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

["Alex Bennée" <alex.bennee@linaro.org>]

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.
>
> 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.henderson@linaro.org>
> Signed-off-by: David Hildenbrand <david@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