Skip to content

firmware_uefi: EfiDiagnostics should flush when unprocessed after reset or shutdown#3542

Open
maheeraeron wants to merge 3 commits into
microsoft:mainfrom
maheeraeron:user/maheeraeron/efidiagnostics-force
Open

firmware_uefi: EfiDiagnostics should flush when unprocessed after reset or shutdown#3542
maheeraeron wants to merge 3 commits into
microsoft:mainfrom
maheeraeron:user/maheeraeron/efidiagnostics-force

Conversation

@maheeraeron
Copy link
Copy Markdown
Contributor

This PR addresses a gap where a user could face a VM shutdown or reset on failure that goes beyond the UEFI crash path before ExitBootServices. In this case, so long as we have a GPA, we will forcibly flush the UEFI logs on reset or shutdown if they haven't been processed before

@maheeraeron maheeraeron requested a review from a team as a code owner May 20, 2026 21:30
Copilot AI review requested due to automatic review settings May 20, 2026 21:30
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR ensures UEFI diagnostics are not silently lost when a guest resets or stops without going through the normal diagnostics/ExitBootServices path by proactively flushing pending diagnostics if a diagnostics buffer GPA is present and has not yet been processed.

Changes:

  • Added a helper (has_unprocessed_diagnostics) to detect when diagnostics are pending and a GPA is available.
  • Triggered diagnostics processing during UefiDevice::stop() and UefiDevice::reset() when pending diagnostics exist.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
vm/devices/firmware/firmware_uefi/src/service/diagnostics/mod.rs Adds a predicate to detect pending/unprocessed diagnostics with a valid GPA.
vm/devices/firmware/firmware_uefi/src/lib.rs Flushes diagnostics on device stop/reset when they haven’t been processed yet.
Comments suppressed due to low confidence (1)

vm/devices/firmware/firmware_uefi/src/lib.rs:360

  • This stop/reset flush path uses WATCHDOG_LOGS_PER_PERIOD, but the constant name/doc in service::diagnostics indicates it is specifically for watchdog timeouts. Using a watchdog-specific limit here is confusing and makes it easy to change the watchdog limit accidentally impacting stop/reset behavior; consider using DEFAULT_LOGS_PER_PERIOD or introducing a dedicated constant for the stop/reset flush case.
            let _ = self.process_diagnostics(
                false,
                service::diagnostics::DiagnosticsEmitter::Tracing {
                    limit: Some(WATCHDOG_LOGS_PER_PERIOD),
                },
                Some(LogLevel::make_info()),
            );

Comment thread vm/devices/firmware/firmware_uefi/src/lib.rs Outdated
Comment thread vm/devices/firmware/firmware_uefi/src/lib.rs
Comment thread vm/devices/firmware/firmware_uefi/src/lib.rs Outdated
Copilot AI review requested due to automatic review settings May 20, 2026 22:15
@maheeraeron
Copy link
Copy Markdown
Contributor Author

Now the real question is, how can I reliably test this? There was an incident with Databricks who had a custom bootloader that unknowingly fails and resets the guest, dodging the UEFI crash path and exit boot services.

Can we test a private winload that reproduces this? Simply turning off the VM in hyper-v won't work, that tears down everything so we never get to this point

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment thread vm/devices/firmware/firmware_uefi/src/lib.rs
@github-actions
Copy link
Copy Markdown

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.

3 participants