Disable interrupts around multiple MmioRead32 calls and similar. - #107
Disable interrupts around multiple MmioRead32 calls and similar.#107Jay Krell (jaykrell) wants to merge 1 commit into
Conversation
|
traditionally we have handled this by forcing the higher priority process (isr) to restore the port. This was a very common problem with SMI using CMOS/RTC. I don't think you will see guarded io like this in most of the UEFI code. Code that runs at higher TPL should be aware of this type of issue. |
|
https://microsoft.visualstudio.com/OS/_workitems/edit/63044137/ I thought I did see some code like this already, or around these functions. |
There was a problem hiding this comment.
🟡 Changes recommended
It introduces new BaseLib dependencies that are not declared in the corresponding INF files (and PlatformPei also lacks the required include), and it leaves an unprotected multi-write BIOS device sequence in PlatformPei.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR aims to prevent interrupt-driven races by disabling interrupts around multi-step device register access sequences (paired address/data writes and address-then-read patterns) in the Hyper-V guest firmware device access helpers.
Changes:
- Wrapped BIOS device read access in Platform PEI with SaveAndDisableInterrupts()/SetInterruptState().
- Wrapped TPM port read/write helpers in Tpm2DeviceLib with SaveAndDisableInterrupts()/SetInterruptState().
- Wrapped BIOS device read/write helpers in BiosDeviceLibCore with SaveAndDisableInterrupts()/SetInterruptState().
File summaries
| File | Description |
|---|---|
| MsvmPkg/PlatformPei/Platform.c | Adds interrupt masking around BIOS device read sequence in PEI. |
| MsvmPkg/Library/Tpm2DeviceLib/Tpm2DeviceLib.c | Adds interrupt masking around TPM port read/write sequences. |
| MsvmPkg/Library/BiosDeviceLib/BiosDeviceLibCore.c | Adds interrupt masking around BIOS device read/write sequences in the shared library. |
Review details
Suppressed comments (1)
MsvmPkg/Library/Tpm2DeviceLib/Tpm2DeviceLib.c:134
- The newly added interrupt guard in ReadTpmPort has the same spacing inconsistencies (missing spaces in helper calls and around parentheses). Keeping formatting consistent reduces diff noise in future changes.
BOOLEAN interruptState = SaveAndDisableInterrupts();
#if defined(MDE_CPU_AARCH64)
UINT64 Port = FixedPcdGet64(PcdTpmBaseAddress) + 0x80;
MmioWrite32(Port, AddressRegisterValue);
UINT32 result = MmioRead32(Port + 4);
#elif defined(MDE_CPU_X64)
- Files reviewed: 3/3 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| BOOLEAN interruptState = SaveAndDisableInterrupts(); | ||
|
|
| UINTN biosBaseAddress = PcdGet32(PcdBiosBaseAddress); | ||
|
|
||
| #if defined (MDE_CPU_AARCH64) | ||
| MmioWrite32 (biosBaseAddress, AddressRegisterValue); | ||
| return MmioRead32 (biosBaseAddress + 4); | ||
| #elif defined (MDE_CPU_X64) | ||
| IoWrite32 (biosBaseAddress, AddressRegisterValue); | ||
| return IoRead32 (biosBaseAddress + 4); | ||
| #endif | ||
| BOOLEAN interruptState = SaveAndDisableInterrupts(); | ||
|
|
||
| #if defined(MDE_CPU_AARCH64) |
| UINTN biosBaseAddress = PcdGet32(PcdBiosBaseAddress); | ||
|
|
||
| #if defined (MDE_CPU_AARCH64) | ||
| MmioWrite32 (biosBaseAddress, AddressRegisterValue); | ||
| return MmioRead32 (biosBaseAddress + 4); | ||
| #elif defined (MDE_CPU_X64) | ||
| IoWrite32 (biosBaseAddress, AddressRegisterValue); | ||
| return IoRead32 (biosBaseAddress + 4); | ||
| #endif | ||
| BOOLEAN interruptState = SaveAndDisableInterrupts(); | ||
|
|
||
| #if defined(MDE_CPU_AARCH64) |
| BOOLEAN interruptState = SaveAndDisableInterrupts(); | ||
|
|
||
| #if _USING_BIOS_MMIO_ | ||
| MmioWrite32(mBiosBaseAddress, AddressRegisterValue); | ||
| UINT32 result = MmioRead32(mBiosBaseAddress + 4); | ||
| #else | ||
| IoWrite32(mBiosBaseAddress, AddressRegisterValue); | ||
| UINT32 result = IoRead32(mBiosBaseAddress + 4); | ||
| #endif | ||
|
|
||
| SetInterruptState(interruptState); | ||
| return result; |
| BOOLEAN interruptState = SaveAndDisableInterrupts(); | ||
|
|
||
| #if defined(MDE_CPU_AARCH64) | ||
| UINT64 Port = FixedPcdGet64(PcdTpmBaseAddress) + 0x80; | ||
| MmioWrite32(Port, AddressRegisterValue); | ||
| MmioWrite32(Port + 4, DataRegisterValue); | ||
| #elif defined(MDE_CPU_X64) | ||
| IoWrite32(TpmControlPort, AddressRegisterValue); | ||
| IoWrite32(TpmDataPort, DataRegisterValue); | ||
| #endif | ||
|
|
||
| SetInterruptState(interruptState); |
| UINTN biosBaseAddress = PcdGet32 (PcdBiosBaseAddress); | ||
|
|
||
| #if defined (MDE_CPU_AARCH64) | ||
| BOOLEAN interruptState = SaveAndDisableInterrupts (); |
There was a problem hiding this comment.
are interrupts ever enabled in PEI? I think this change is unnecessary.
Maybe BiosDeviceLib wants a PEI flavor
There was a problem hiding this comment.
Is it worth "just" being consistent?
There was a problem hiding this comment.
What about e.g.
LayerGraphicsSetFrame
in upstream package, it sets an address in two halves (copy/paste not working..I'll get it)
There was a problem hiding this comment.
And others nearby.
Maybe no graphics display will occur this early, so the torn address never used?
3fc6003 to
a644123
Compare
|
Sean Brogan (@spbrogan) do you have a pointer or example to share? In reply to: 5518166017 |
I believe we agreed it does, right? Interrupt disabling is a single processor at a time thing, right? Another CPU could still interrupt and cause the same problem. |
There are race conditions of the form:
MmioWrite(port, x)
MmioWrite(port2, y)
These need to occur one after the other without interrupt.
In case an interrupt writes to one of the ports.
Disable interrupts around such sequences.