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

[Richard Henderson <richard.henderson@linaro.org>]

On 10/1/19 12:47 PM, David Hildenbrand wrote:
> 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.

I think that it's a race right here in should_interrupt_instruction.

Notice, interrupt_request gets set before icount_decr.  Indeed, the barrier
happens immediately before the set of icount_decr in cpu_exit().

(It is briefly confusing that we have a barrier in cpu_exit and not in
tcg_handle_interrupt.  But that's explained by the cpu_is_self -- no need for a
barrier for the current cpu.  I also think we could usefully use
atomic_store_release instead of a separate smp_wmb.)

Therefore checking interrupt_request after checking icount_decr violates the
ordering rules.

This is confirmed, ish, by noticing putting a breakpoint at that second return
(or assert) and noticing that icount_decr.u16.hi == -1.  It did get set by one
of the other threads, and before gdb managed to stop the world.


r~