Re: [PATCH v1] s390x/tcg: MVCL: Exit to main loop if there are pending interrupts

[David Hildenbrand]

On 01.10.19 21:17, Richard Henderson wrote:
> On 10/1/19 11:16 AM, David Hildenbrand wrote:
>> +static inline bool should_interrupt_instruction(CPUState *cs)
>> +{
>> +    /*
>> +     * Something asked us to stop executing chained TBs, e.g.,
>> +     * cpu_interrupt() or cpu_exit().
>> +     */
>> +    if ((int32_t)atomic_read(&cpu_neg(cs)->icount_decr.u32) < 0) {
>> +        return true;
>> +    }
>> +
>> +    /* We have a deliverable interrupt pending. */
>> +    if ((atomic_read(&cs->interrupt_request) & CPU_INTERRUPT_HARD) &&
>> +        s390_cpu_has_int(S390_CPU(cs))) {
>> +        return true;
>> +    }
>> +    return false;
>> +}
> 
> The first condition should be true whenever the second condition is true.

@@ -1018,6 +1018,7 @@ static inline bool should_interrupt_instruction(CPUState 
*cs)
     /* We have a deliverable interrupt pending. */
     if ((atomic_read(&cs->interrupt_request) & CPU_INTERRUPT_HARD) &&
         s390_cpu_has_int(S390_CPU(cs))) {
+        g_assert((int32_t)atomic_read(&cpu_neg(cs)->icount_decr.u32) < 0);
         return true;
     }
     return false;


...


[   60.109761] systemd[1]: Set hostname to <rhel8>.
**
ERROR:/home/dhildenb/git/qemu/target/s390x/mem_helper.c:1021:should_interrupt_instruction:
 assertion failed: ((int32_t)atomic_read(&cpu_neg(cs)->icount_decr.u32) < 0)


A race? Roughly 20-30% pass the first but not the second check. And
in total, on a Fedora 30 boot, I can maybe see 30 calls of
should_interrupt_instruction() succeeding.

I thought these could be pending interrupts that were not deliverable
when injected but are now deliverable. For these,
icount_decr.u32.high would already have been set to 0.

OTOH, I guess we always exit the TB in case we change the "deliverable" state
of an IRQ, e.g., after LPSW or LCTL. E.g.,

static DisasJumpType op_lctlg(DisasContext *s, DisasOps *o)
{
...
    /* Exit to main loop to reevaluate s390_cpu_exec_interrupt.  */
    return DISAS_PC_STALE_NOCHAIN;
}

Maybe really a race then - or we are not properly exiting back to the
main loop in all scenarios.

> 
> In particular, tcg_handle_interrupt sets icount_decr.u16.high = -1 for
> qemu_cpu_is_self; otherwise, qemu_cpu_kick calls cpu_exit which does the same
> thing.
> 
> Think of it this way: we only test icount_decr.u32 at the start of each TB, 
> and
> that's the only thing we have that brings us back to the main loop for any
> other kind of interrupt.
> 

-- 

Thanks,

David / dhildenb