Repository navigation
MsGraphicsPkg: Prevent Recursive Reconnect Crash [Rebase & FF] - #948
Open
Michael Kubacki (makubacki) wants to merge 2 commits into
Open
Michael Kubacki (makubacki) wants to merge 2 commits into
Michael Kubacki (makubacki) wants to merge 2 commits into
Conversation
Michael Kubacki (makubacki)
changed the title
Contributor
✅ QEMU Validation PassedSource Dependencies
Results
Workflow run: https://github.com/microsoft/mu_plus/actions/runs/40357843605 This comment was automatically generated by the Mu QEMU PR Validation workflow. |
Michael Kubacki (makubacki)
force-pushed
the
prevent_swm_reconnect_r_fault_2608
branch
from
October 23, 2026 22:49
7230550 to
8254fcc
Compare
…during reconnect SimpleWindowManager aggregates pointer input and provides window-rendering services through the Simple Rendering Engine (SRE). Previously, its driver binding only managed pointer devices. Graphics interfaces were discovered separately and cached without a corresponding disconnect relationship (did not participate in the driver model). During a recursive controller reconnect, graphics teardown could invalidate the cached GOP mode information while SimpleWindowManager's periodic input callback continued reading it. This caused a page fault when accessing the screen resolution. Example page fault on QEMU Q35 (after running `reconnect -r`): ``` [OSK]: DriverBindingStop. INFO - UninstallProtocolInterface: 387477C1-69C7-11D2-8E39-00A0C969723B @ 0x0000000079ba1a28 <-- gEfiSimpleTextInProtocolGuid INFO - UninstallProtocolInterface: DD9E7534-7762-4698-8C14-F58517A625AA @ 0x0000000079ba1a40 <-- gEfiSimpleTextInputExProtocolGuid ERROR - EXCEPTION: PAGE FAULT ERROR - Dumping Exception Stack Trace: WARN - Dumping stack trace with PC: 0000000079B7CD54, SP: 0000000079105CF0, FP: 0000000079105D30 WARN - # Child-SP Return Address Call Site WARN - 0 0000000079105CF0 000000007B0F4B68 SimpleWindowManagerDxe+2D54 WARN - 1 0000000079105D80 000000007A64D6FE qemu_q35_dxe_core+88B68 WARN - 2 0000000079105E60 000000007B124432 Timer+16FE WARN - 3 0000000079105EA0 000000007B06E780 qemu_q35_dxe_core+B8432 ``` Where `SimpleWindowManagerDxe+2D54` maps to `ScreenMaxX = mGop->Mode->Info->HorizontalResolution;` in `CheckWatchListCallback()` which is a a periodic timer event for checking the Absolute Pointer protocol watchlist. This change extends the existing driver binding to manage the private SRE protocol using `BY_DRIVER`. On graphics disconnect, the teardown happens: cancel polling, release rendering surfaces, and clear cached graphics pointers before releasing the SRE. On reattachment, graphics interfaces are reacquired, dimensions are refreshed, client surfaces are recreated, and polling is resumed without repeating one-time driver initialization. Client registrations and client-visible paint-event handles are preserved across graphics restarts. When graphics are unavailable, rendering operations return EFI_NOT_READY, and failure and unload cleanup release the associated resources. The GOP is retrieved from the SRE controller without opening it `BY_DRIVER`. This ensures that the graphics console retains ownership to provide the text console, while existing pointer-device ownership remains unchanged. Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
With the Simple Rendering Engine (SRE) disconnect flow in a
functional state, OSK has its own issues that need to be addressed
to get to a base level of functionality on restart.
1. `Stop()` does not close the Device Path protocol that `Start()`
opened with `BY_DRIVER`. Therefore, future calls to `Supported()`
will fail.
2. `Stop()` and error paths close `WaitForKey()` and `WaitForKeyEx()`.
Those are only created in driver entry, so a restarted on-screen
keyboard will end up publishing closed events to ConSplitter.
3. `Stop()` does not call `UnregisterClient()`, so registering again
gets back `EFI_ALREADY_STARTED` (and hits an assert).
4. Technically, each restart leaks `pKeyTextBltBuffer`, but that's
relatively minor and does not directly impact functionality.
These are immediate issues impacting restarting the on-screen keyboard.
This change resolves them by:
- Recording the GOP handle (that `mGop` is on).
`OSKCheckDisplayModeTimerCallback()` returns early if that handle
no longer has the `mGop` protocol instance installed. This is to
prevent accessing a GOP protocol instance that is no longer valid.
- Closing the Device Path protocol in `Stop()`.
- No longer closing `WaitForKey()` and `WaitForKeyEx()`.
- Setting timer handles to `NULL` after closing them to keep state
clean.
- `Start()` now closes the display-mode timer even if the key-repeat
timer was not created.
- For the SWM registration (not calling `UnregisterClient()` in
`Stop()`), the keyboard code just keeps its registration. Then,
on a restart, `EFI_ALREADY_STARTED` is considered success as long
as `mOSKPaintEvent` is set. This was a compromise to make a lower
risk approach to handling SWM registration on restart, it allows:
- Other drivers (FrontPage, dialogs, etc.) to continue being able
to call the OSK protocol during the restart and the SWM keeps
clients registered across restarts.
This change is meant to be a relatively minimal (reduce risk) approach
to address the most impactful functional issues found in the on-screen
keyboard restart flow. It is not a complete or exhaustive solution to
all potential restart issues. Ideally, the entire stack would need to
be rewritten to more cleanly participate in the UEFI driver model.
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
Michael Kubacki (makubacki)
force-pushed
the
prevent_swm_reconnect_r_fault_2608
branch
from
October 26, 2026 16:09
8254fcc to
246830a
Compare
Member
Author
|
This has been a longstanding issue that this PR attempts to resolve. If anyone would like to go about this a different way or contribute their own changes to this PR, please feel free to do so. However, this currently failing with a page fault impacts testing disconnect/reconnect flows on platforms and having it resolved would benefit overall platform testing. |
Aaron (apop5)
approved these changes
Oct 26, 2026
John Schock (joschock)
approved these changes
Oct 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
reconnect -rcurrently results in a page fault:The immediate issue is that
SimpleWindowManagerDxe(SWM) caches the GOP and does not establish a driver-binding relationship with it for disconnect. It does for Absolute Pointer providers, but the timer and cached graphics pointers stay active.Simple Rendering Engine (SRE) uninstalls the GOP and releases the parent and the SWM's timer event stays active and dereferences stale/invalid pointers.
This PR has two fixes in SWM and the On-Screen Keyboard (OSK) driver that are meant to be minimal fixes intended to reduce risk to the overall driver stack. They primarily target the disconnect/reconnect flow which is currently very broken. In order to address this in SWM, a change had to be made to have it bind to the SRE protocol to get notified during disconnect. While this looks like a non-trivial amount of code change, I believe it is still the minimum/targeted change to address this without continuing to expand scope.
SRE and its relationship to the GOP protocol are not changed and SWM's relationship with Absolute Pointer is not changed. The main relationship established is the binding between SRE and SWM.
With the SWM change made, QEMU and physical platforms can successfully go through a
reconnect -rcall without crashing. However, the OSK does not restart. The second commit makes minimal changes to address the most impactful problem there.MsGraphicsPkg/SimpleWindowManagerDxe: Fix graphics lifetime handling during reconnect
SimpleWindowManager aggregates pointer input and provides window-rendering
services through the Simple Rendering Engine (SRE). Previously, its
driver binding only managed pointer devices. Graphics interfaces were
discovered separately and cached without a corresponding disconnect
relationship (did not participate in the driver model).
During a recursive controller reconnect, graphics teardown could
invalidate the cached GOP mode information while SimpleWindowManager's
periodic input callback continued reading it. This caused a page fault
when accessing the screen resolution.
Example page fault on QEMU Q35 (after running
reconnect -r):Where
SimpleWindowManagerDxe+2D54maps toScreenMaxX = mGop->Mode->Info->HorizontalResolution;inCheckWatchListCallback()which is a a periodic timer event forchecking the Absolute Pointer protocol watchlist.
This change extends the existing driver binding to manage the private
SRE protocol using
BY_DRIVER. On graphics disconnect, the teardownhappens: cancel polling, release rendering surfaces, and clear cached
graphics pointers before releasing the SRE.
On reattachment, graphics interfaces are reacquired, dimensions are
refreshed, client surfaces are recreated, and polling is resumed
without repeating one-time driver initialization.
Client registrations and client-visible paint-event handles are
preserved across graphics restarts. When graphics are unavailable,
rendering operations return EFI_NOT_READY, and failure and unload
cleanup release the associated resources.
The GOP is retrieved from the SRE controller without opening it
BY_DRIVER. This ensures that the graphics console retains ownershipto provide the text console, while existing pointer-device ownership
remains unchanged.
MsGraphicsPkg/OnScreenKeyboardDxe: Fix some restart issues
With the Simple Rendering Engine (SRE) disconnect flow in a
functional state, OSK has its own issues that need to be addressed
to get to a base level of functionality on restart.
Stop()does not close the Device Path protocol thatStart()opened with
BY_DRIVER. Therefore, future calls toSupported()will fail.
Stop()and error paths closeWaitForKey()andWaitForKeyEx().Those are only created in driver entry, so a restarted on-screen
keyboard will end up publishing closed events to ConSplitter.
Stop()does not callUnregisterClient(), so registering againgets back
EFI_ALREADY_STARTED(and hits an assert).pKeyTextBltBuffer, but that'srelatively minor and does not directly impact functionality.
These are immediate issues impacting restarting the on-screen keyboard.
This change resolves them by:
mGopis on).OSKCheckDisplayModeTimerCallback()returns early if that handleno longer has the
mGopprotocol instance installed. This is toprevent accessing a GOP protocol instance that is no longer valid.
Stop().WaitForKey()andWaitForKeyEx().NULLafter closing them to keep stateclean.
Start()now closes the display-mode timer even if the key-repeattimer was not created.
UnregisterClient()inStop()), the keyboard code just keeps its registration. Then,on a restart,
EFI_ALREADY_STARTEDis considered success as longas
mOSKPaintEventis set. This was a compromise to make a lowerrisk approach to handling SWM registration on restart, it allows:
to call the OSK protocol during the restart and the SWM keeps
clients registered across restarts.
This change is meant to be a relatively minimal (reduce risk) approach
to address the most impactful functional issues found in the on-screen
keyboard restart flow. It is not a complete or exhaustive solution to
all potential restart issues. Ideally, the entire stack would need to
be rewritten to more cleanly participate in the UEFI driver model.
Also, Uncrustify changed some code not directly modified.
How This Was Tested
reconnect -rreconnect -rreconnect -rreconnect -rwith the second commitreconnect -rbefore the changes and is nowIntegration Instructions