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

[Richard Henderson]

On 10/2/19 2:58 AM, Alex Bennée wrote:
> 
> 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.
>>
>> 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 <address@hidden>
>> Signed-off-by: David Hildenbrand <address@hidden>
>> ---
>>
>> 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.

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.


r~