Skip to content

insert NOP after STI#588

Merged
phil-opp merged 2 commits intomasterfrom
fix/nop-after-sti
Apr 28, 2026
Merged

insert NOP after STI#588
phil-opp merged 2 commits intomasterfrom
fix/nop-after-sti

Conversation

@Freax13
Copy link
Copy Markdown
Member

@Freax13 Freax13 commented Apr 27, 2026

Insert a NOP instruction after STI to make sure the interrupt shadow doesn't extend past the enable function.

From the AMD64 APM, Volume 3:

If STI sets the IF flag and IF was initially clear, then interrupts
are not enabled until after the instruction following STI. Thus, if
IF is 0, this code will not allow an INTR to happen:

STI
CLI

In the following sequence, INTR will be allowed to happen only after the NOP.

STI
NOP
CLI

Insert a NOP instruction after STI to make sure the interrupt shadow
doesn't extend past the enable function.

From the AMD64 APM, Volume 3:
> If STI sets the IF flag and IF was initially clear, then interrupts
> are not enabled until after the instruction following STI. Thus, if
> IF is 0, this code will not allow an INTR to happen:
>
> STI
> CLI
>
> In the following sequence, INTR will be allowed to happen only after the NOP.
>
> STI
> NOP
> CLI
@Freax13 Freax13 requested review from josephlr and phil-opp April 27, 2026 12:49
@phil-opp
Copy link
Copy Markdown
Member

The change itself looks good to me, but I'm not sure if this is a breaking change. There might be some use cases that rely on the interrupt shadow, which this change might break in subtle ways.

@Freax13
Copy link
Copy Markdown
Member Author

Freax13 commented Apr 27, 2026

I don't think one could have reliably relied on the interrupt shadow after the enable function. Rust doesn't guarantee inlining, so one would always run the risk of the next instruction after STI being ret.
Note that we do have enable_and_hlt for STI+HLT with the guarantee that HLT is in the interrupt shadow.

@phil-opp
Copy link
Copy Markdown
Member

Good point! We should update the method documentation though to document that the method does a sti+nop and explain why (interrupt shadow). Maybe also link enable_and_hlt

Copy link
Copy Markdown
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@phil-opp phil-opp merged commit 4fbf718 into master Apr 28, 2026
13 checks passed
@phil-opp phil-opp deleted the fix/nop-after-sti branch April 28, 2026 08:23
@Freax13
Copy link
Copy Markdown
Member Author

Freax13 commented Apr 28, 2026

Thanks for the quick review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants