| Message ID | 20231210190147.129734-2-lrh2000@pku.edu.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/i386: Give IRQs a chance when resetting HF_INHIBIT_IRQ_MASK | expand |
On Mon, Dec 11, 2023 at 03:01:48AM +0800, Ruihan Li wrote: > [ .. ] > > This problem is caused because the previous code may choose not to end > the TB even if the HF_INHIBIT_IRQ_MASK has just been reset (e.g., in the > case where the RET instruction is immediately followed by the STI > instruction), so that IRQs may not have a change to trigger. There is a typo. I mean "in the case where the STI instruction is immediately followed by the RET instruction", not the other way around. I'll send a V2 patch if necessary, but let's hear from others first. Thanks, Ruihan Li
Hi all, On Mon, Dec 11, 2023 at 03:01:48AM +0800, Ruihan Li 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. > > This problem is caused because the previous code may choose not to end > the TB even if the HF_INHIBIT_IRQ_MASK has just been reset (e.g., in the > case where the RET instruction is immediately followed by the STI > instruction), so that IRQs may not have a change to trigger. This commit > fixes the problem by always terminating the current TB to give IRQs a > chance to trigger when HF_INHIBIT_IRQ_MASK is reset. > > Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn> > --- > The same problem was discovered two years ago, see [StackOverflow][so]. > > [so]: https://stackoverflow.com/questions/68135305/executing-ret-after-sti-doesnt-start-interrupts > > target/i386/tcg/translate.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c > index 587d886..6b7deb5 100644 > --- a/target/i386/tcg/translate.c > +++ b/target/i386/tcg/translate.c > @@ -2767,13 +2767,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) { > @@ -2784,7 +2790,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); > -- > 2.43.0 A friendly ping. Anyone here to confirm this BUG and/or comment on the patch? Thanks, Ruihan Li
On 12/20/23 02:04, Ruihan Li wrote: > Hi all, > > On Mon, Dec 11, 2023 at 03:01:48AM +0800, Ruihan Li 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. >> >> This problem is caused because the previous code may choose not to end >> the TB even if the HF_INHIBIT_IRQ_MASK has just been reset (e.g., in the >> case where the RET instruction is immediately followed by the STI >> instruction), so that IRQs may not have a change to trigger. This commit >> fixes the problem by always terminating the current TB to give IRQs a >> chance to trigger when HF_INHIBIT_IRQ_MASK is reset. >> >> Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn> >> --- >> The same problem was discovered two years ago, see [StackOverflow][so]. >> >> [so]: https://stackoverflow.com/questions/68135305/executing-ret-after-sti-doesnt-start-interrupts >> >> target/i386/tcg/translate.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c >> index 587d886..6b7deb5 100644 >> --- a/target/i386/tcg/translate.c >> +++ b/target/i386/tcg/translate.c >> @@ -2767,13 +2767,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) { >> @@ -2784,7 +2790,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); >> -- >> 2.43.0 > > A friendly ping. > > Anyone here to confirm this BUG and/or comment on the patch? Looks correct. Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c index 587d886..6b7deb5 100644 --- a/target/i386/tcg/translate.c +++ b/target/i386/tcg/translate.c @@ -2767,13 +2767,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) { @@ -2784,7 +2790,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);
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. This problem is caused because the previous code may choose not to end the TB even if the HF_INHIBIT_IRQ_MASK has just been reset (e.g., in the case where the RET instruction is immediately followed by the STI instruction), so that IRQs may not have a change to trigger. This commit fixes the problem by always terminating the current TB to give IRQs a chance to trigger when HF_INHIBIT_IRQ_MASK is reset. Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn> --- The same problem was discovered two years ago, see [StackOverflow][so]. [so]: https://stackoverflow.com/questions/68135305/executing-ret-after-sti-doesnt-start-interrupts target/i386/tcg/translate.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)