Message ID | 20240415064518.4951-4-lrh2000@pku.edu.cn |
---|---|
State | New |
Headers | show |
Series | [v2] target/i386: Give IRQs a chance when resetting HF_INHIBIT_IRQ_MASK | expand |
On Mon, Apr 15, 2024 at 8:50 AM Ruihan Li <lrh2000@pku.edu.cn> wrote: > > When emulated with QEMU, interrupts will never come in the following > loop. However, if the NOP instruction is uncommented, interrupts will > fire as normal. > > loop: > cli > call do_sti > jmp loop > > do_sti: > sti > # nop > ret > > This behavior is different from that of a real processor. For example, > if KVM is enabled, interrupts will always fire regardless of whether the > NOP instruction is commented or not. Also, the Intel Software Developer > Manual states that after the STI instruction is executed, the interrupt > inhibit should end as soon as the next instruction (e.g., the RET > instruction if the NOP instruction is commented) is executed. Thanks, interesting bug! What do you think about writing this: > /* If several instructions disable interrupts, only the first does it. */ > if (inhibit && !(s->flags & HF_INHIBIT_IRQ_MASK)) { > gen_set_hflag(s, HF_INHIBIT_IRQ_MASK); > - } else { > + inhibit_reset = false; > + } else if (!inhibit && (s->flags & HF_INHIBIT_IRQ_MASK)) { > gen_reset_hflag(s, HF_INHIBIT_IRQ_MASK); > + inhibit_reset = true; > + } else { > + inhibit_reset = false; > } in a slightly simpler manner: inhibit_reset = false; if (s->flags & HF_INHIBIT_IRQ_MASK) { gen_reset_hflag(s, HF_INHIBIT_IRQ_MASK); inhibit_reset = true; } else if (inhibit) { gen_set_hflag(s, HF_INHIBIT_IRQ_MASK); } No need to submit v3, I can do the change myself when applying. Paolo
Hi Paolo, On Mon, Apr 15, 2024 at 11:32:51AM +0200, Paolo Bonzini wrote: > What do you think about writing this: > > > /* If several instructions disable interrupts, only the first does it. */ > > if (inhibit && !(s->flags & HF_INHIBIT_IRQ_MASK)) { > > gen_set_hflag(s, HF_INHIBIT_IRQ_MASK); > > - } else { > > + inhibit_reset = false; > > + } else if (!inhibit && (s->flags & HF_INHIBIT_IRQ_MASK)) { > > gen_reset_hflag(s, HF_INHIBIT_IRQ_MASK); > > + inhibit_reset = true; > > + } else { > > + inhibit_reset = false; > > } > > in a slightly simpler manner: > > inhibit_reset = false; > if (s->flags & HF_INHIBIT_IRQ_MASK) { > gen_reset_hflag(s, HF_INHIBIT_IRQ_MASK); > inhibit_reset = true; > } else if (inhibit) { > gen_set_hflag(s, HF_INHIBIT_IRQ_MASK); > } Yes, I agree with you that your changes look a bit clearer. I have tested your changes and verified that they fix the reported bug. > No need to submit v3, I can do the change myself when applying. Thank you for your review. Feel free to do that. Thanks, Ruihan Li
On 15/4/24 11:32, Paolo Bonzini wrote: > On Mon, Apr 15, 2024 at 8:50 AM Ruihan Li <lrh2000@pku.edu.cn> wrote: >> >> When emulated with QEMU, interrupts will never come in the following >> loop. However, if the NOP instruction is uncommented, interrupts will >> fire as normal. >> >> loop: >> cli >> call do_sti >> jmp loop >> >> do_sti: >> sti >> # nop >> ret >> >> This behavior is different from that of a real processor. For example, >> if KVM is enabled, interrupts will always fire regardless of whether the >> NOP instruction is commented or not. Also, the Intel Software Developer >> Manual states that after the STI instruction is executed, the interrupt >> inhibit should end as soon as the next instruction (e.g., the RET >> instruction if the NOP instruction is commented) is executed. > > Thanks, interesting bug! > > What do you think about writing this: > >> /* If several instructions disable interrupts, only the first does it. */ >> if (inhibit && !(s->flags & HF_INHIBIT_IRQ_MASK)) { >> gen_set_hflag(s, HF_INHIBIT_IRQ_MASK); >> - } else { >> + inhibit_reset = false; >> + } else if (!inhibit && (s->flags & HF_INHIBIT_IRQ_MASK)) { >> gen_reset_hflag(s, HF_INHIBIT_IRQ_MASK); >> + inhibit_reset = true; >> + } else { >> + inhibit_reset = false; >> } > > in a slightly simpler manner: > > inhibit_reset = false; > if (s->flags & HF_INHIBIT_IRQ_MASK) { > gen_reset_hflag(s, HF_INHIBIT_IRQ_MASK); > inhibit_reset = true; > } else if (inhibit) { > gen_set_hflag(s, HF_INHIBIT_IRQ_MASK); > } > > No need to submit v3, I can do the change myself when applying. Cc: qemu-stable@nongnu.org
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c index 76a42c6..3f0fbdf 100644 --- a/target/i386/tcg/translate.c +++ b/target/i386/tcg/translate.c @@ -2798,13 +2798,19 @@ static void gen_bnd_jmp(DisasContext *s) static void do_gen_eob_worker(DisasContext *s, bool inhibit, bool recheck_tf, bool jr) { + bool inhibit_reset; + gen_update_cc_op(s); /* If several instructions disable interrupts, only the first does it. */ if (inhibit && !(s->flags & HF_INHIBIT_IRQ_MASK)) { gen_set_hflag(s, HF_INHIBIT_IRQ_MASK); - } else { + inhibit_reset = false; + } else if (!inhibit && (s->flags & HF_INHIBIT_IRQ_MASK)) { gen_reset_hflag(s, HF_INHIBIT_IRQ_MASK); + inhibit_reset = true; + } else { + inhibit_reset = false; } if (s->base.tb->flags & HF_RF_MASK) { @@ -2815,7 +2821,9 @@ do_gen_eob_worker(DisasContext *s, bool inhibit, bool recheck_tf, bool jr) tcg_gen_exit_tb(NULL, 0); } else if (s->flags & HF_TF_MASK) { gen_helper_single_step(tcg_env); - } else if (jr) { + } else if (jr && + /* give irqs a chance to happen */ + !inhibit_reset) { tcg_gen_lookup_and_goto_ptr(); } else { tcg_gen_exit_tb(NULL, 0);