Skip to content

firmware_uefi: add diagnostics service for uefi logs #1209

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 35 commits into
base: main
Choose a base branch
from

Conversation

maheeraeron
Copy link

@maheeraeron maheeraeron commented Apr 19, 2025

This PR focuses on a new diagnostics service for firmware_uefi.

UEFI will write the GPA of the advanced logger buffer (which we formally call EFI Diagnostics here) to an Io port intercept called SET_EFI_DIAGNOSTICS_GPA.

UEFI will also write to a different Io port intercept called PROCESS_EFI_DIAGNOSTICS, which signals us to process the EFI Diagnostics buffer.

The diagnostics subsystem's role is to read into guest memory from the specified GPA, collect advanced logger entries and log them to our tracing facilities. This will get triggered by the following conditions:

  • UEFI encounters a failure (guest driven via PROCESS_EFI_DIAGNOSTICS)
  • UEFI fails to boot any device (guest driven via PROCESS_EFI_DIAGNOSTICS)
  • UEFI reaches exit boot services

@maheeraeron maheeraeron marked this pull request as ready for review April 20, 2025 03:02
@maheeraeron maheeraeron requested review from a team as code owners April 20, 2025 03:02
@maheeraeron maheeraeron changed the title [WIP] Parse and Log the EFI Diagnostics Buffer Parse and Log the EFI Diagnostics Buffer Apr 20, 2025
@maheeraeron maheeraeron changed the title Parse and Log the EFI Diagnostics Buffer [WIP] Parse and Log the EFI Diagnostics Buffer Apr 20, 2025
}

/// Process the diagnostics buffer into friendly logs
pub fn process_diagnostics(
Copy link
Collaborator

Choose a reason for hiding this comment

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

process_diagnostics

let's fuzz this

@chris-oo
Copy link
Member

Is it possible to add a vmm_tests test for this, where we just see that uefi logged something?

@chris-oo chris-oo changed the title Parse and Log the EFI Diagnostics Buffer firmware_uefi: add diagnostics service for uefi logs Apr 23, 2025
@jstarks
Copy link
Member

jstarks commented Apr 23, 2025

(GitHub is misbehaving, so I can't attach this to the .validate call in your code):

I am violently against "validate" methods that check things and don't return a value.

  1. It's extremely unclear what invariants you're validating. What are the post conditions on this method? The reviewer can't tell from the call site--they have to go read the code to see what you're actually validating. That's a recipe for failing to validate some important thing.

  2. After you validate some invariant on untrusted data, you typically have everything you need to produce a parsed representation. So you end up duplicating work in the "parsing" step. Better to just have a parse/process function that both validates and extracts the (now validated) data.

In this case, I would suggest just getting rid of these validation functions and just putting the checks inline, next to the code that relies on the validation having been performed.

Utf8Error(#[from] std::str::Utf8Error),

#[error("Encountered arithmetic overflow: {0}")]
Overflow(String),
Copy link
Member

Choose a reason for hiding this comment

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

No arbitrary strings in error types. Again, either use anyhow or enumerate your error conditions.

@maheeraeron
Copy link
Author

maheeraeron commented Apr 24, 2025

Is it possible to add a vmm_tests test for this, where we just see that uefi logged something?

That's a good idea. I think these would be part of openhcl_uefi tests? A couple questions:

  • Does this fit the scope of this PR? --> yes, this should be pretty small

  • Do we have a way to specify no_vhd in an openvmm_test? (useful to simulate a test for logs with no boot devices) --> yes, just specify "none"

  • Do we have example tests that look for specific tracing::info!() logs? --> yes, chris just added this 2 days ago to look through kmsg

use zerocopy::Immutable;
use zerocopy::KnownLayout;

// Advanced Logger Info signature
Copy link
Member

Choose a reason for hiding this comment

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

use /// doc comments here, please. Also, can we require doc comments?

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.

5 participants