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