[v2] target/i386: Give IRQs a chance when resetting HF_INHIBIT_IRQ_MASK

[PATCH v2] target/i386: Give IRQs a chance when resetting HF_INHIBIT_IRQ_MASK

Posted by Ruihan Li 2 weeks ago

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 STI instruction is immediately followed by the RET
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.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
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

Changes since v1:
 - Fix a typo: "RET is followed by STI" -> "STI is followed by RET"
Link to v1:
 - https://lore.kernel.org/qemu-devel/20231210190147.129734-2-lrh2000@pku.edu.cn/

 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 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);
-- 
2.44.0

Re: [PATCH v2] target/i386: Give IRQs a chance when resetting HF_INHIBIT_IRQ_MASK

Posted by Paolo Bonzini 2 weeks ago

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

Re: [PATCH v2] target/i386: Give IRQs a chance when resetting HF_INHIBIT_IRQ_MASK

Posted by Philippe Mathieu-Daudé 1 week, 6 days ago

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

Re: [PATCH v2] target/i386: Give IRQs a chance when resetting HF_INHIBIT_IRQ_MASK

Posted by Ruihan Li 2 weeks ago

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