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

[David Hildenbrand <david@redhat.com>]

On 02.10.19 18:47, Richard Henderson wrote:
> On 10/2/19 2:58 AM, Alex Bennée wrote:
>>
>> 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.
> 
> 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