On 02.10.19 18:47, Richard Henderson wrote:
> On 10/2/19 2:58 AM, Alex Bennée wrote:
>>
>> 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.
>
> But it's also the exact right thing to test.
>
>
>> 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))?
>
> No, this is not about interrupts per se.
>
> The thing we're trying to solve here is MVCL running for a long time. The
> length operand is 24 bits, so max 16MB can be copied with one instruction. We
> want to exit back to the main loop early when told to do so, as the insn is
> officially restartable.
>
> Ordinarily, I would say move the loop out to the tcg level, but that creates
> further complications and I'd rather not open up that can of worms.
While that is feasible, I agree that it's not the simplest approach.
>
> There is still the special case of EXECUTE of MVCL, which I suspect must have
> some failure mode that we're not considering -- the setting and clearing of
> ex_value can't help. I have a suspicion that we need to special case that
> within helper_ex, just so that ex_value doesn't enter into it.
We could rap that in something like
cpu_cond_loop_exit_restore()
inspired by cond_resched() in the kernel. Then, at least the
implementation specifics are kept where they actually belong.
--
Thanks,
David / dhildenb
