uefi/perf: Capture FPDT data from a QEMU boot - #212
Open
kat-perez wants to merge 10 commits into
Open
Conversation
Boots a Patina Q35 firmware pair under QEMU and reports whether it reaches BDS, so boot behaviour can be exercised somewhere reproducible instead of only on hardware. Distinguishes a firmware that failed to reach BDS from a setup problem via separate exit codes, and keeps the debug console log as the artifact to inspect on failure. The README records how to obtain firmware, including why performance tracing has to be selected at build time: the platform always publishes the performance configuration HOB, and a disabled HOB outranks the DXE Core default, so swapping only the DXE Core binary cannot enable it. Shell scripts are pinned to LF, which autocrlf would otherwise break for Linux CI and WSL. Assisted-by: GitHub Copilot:claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kat-perez
requested review from
kurtjd,
maxperga-msft,
philgweber and
rogurr
September 4, 2026 16:02
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new shell scripts have confirmed robustness issues around option parsing and dependency preflight that can produce incorrect exit codes and unhelpful failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds the “capture” half of the UEFI/QEMU performance harness: building a dump disk that runs FbptDump.efi, booting Patina Q35 firmware under QEMU until the guest powers off, extracting the captured FBPT binary, and parsing it on non-Windows CI hosts via a small fpdt_parser wrapper.
Changes:
- Add QEMU boot and capture scripts for producing
FBPT.binplus parsedfbpt.xml/fbpt.txt. - Add a cross-platform Python wrapper to run
edk2toolext.perf.fpdt_parseron Linux. - Add docs and repo settings tweaks for shell-script EOLs and Python cache ignores.
File summaries
| File | Description |
|---|---|
| uefi/perf/qemu/run-q35-boot.sh | Boots Patina Q35 firmware under QEMU and watches the debug console for the BDS marker. |
| uefi/perf/qemu/capture-fbpt.sh | Boots with a dump disk, waits for guest poweroff, extracts FBPT, and runs the parser. |
| uefi/perf/qemu/make-fbpt-disk.sh | Builds a FAT image that boots to UEFI Shell and runs FbptDump.efi, redirecting output to disk. |
| uefi/perf/qemu/fpdt_parser_any_platform.py | Monkeypatch wrapper to make fpdt_parser import/run on non-Windows when parsing a captured binary. |
| uefi/perf/qemu/README.md | Usage documentation for bringup + capture flow and parser/report tooling. |
| .gitattributes | Forces LF for *.sh to avoid CRLF shebang issues under CI/WSL. |
| .gitignore | Ignores Python __pycache__/ artifacts. |
Review details
Suppressed comments (2)
uefi/perf/qemu/run-q35-boot.sh:102
- If
qemu-system-x86_64is not installed/on PATH, this will currently fail withcommand not found(exit 127), even though the script’s contract says setup problems should beEXIT_USAGE(2). It’s better to preflight the dependency and fail with a clear message and consistent exit code.
: > "$boot_log"
qemu-system-x86_64 \
uefi/perf/qemu/capture-fbpt.sh:109
- This script relies on external tools (
qemu-system-x86_64,mtype/mdir/mcopy, and the selected--pythoninterpreter). If any are missing, the script will currently fail withcommand not found(exit 127), even though setup problems are meant to useEXIT_USAGE(2). Preflighting dependencies keeps failures clear and exit codes consistent.
: > "$boot_log"
qemu-system-x86_64 \
-debugcon "file:${boot_log}" \
- Files reviewed: 6/7 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A flag given without a value read an unset positional, so 'set -u' aborted with a bash error and exit 1 -- the code reserved for firmware that failed to reach BDS. The timeout was worse: it is only used in arithmetic after QEMU has started, so a non-numeric value killed the run mid-boot, and an empty one reported reaching no BDS "within s" when nothing had actually been measured. Require a value for every flag that takes one, and reject a timeout that is not a positive whole number, both before any firmware runs. Assisted-by: GitHub Copilot:claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kat-perez
force-pushed
the
kat-perez/issue-119-fpdt-capture
branch
from
September 4, 2026 18:34
dbc2b09 to
0ed1c69
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new scripts’ documented exit-code/behavior contract depends on external tools and Python deps, but missing prerequisites currently surface as generic shell failures instead of consistent setup/usage errors.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/7 changed files
- Comments generated: 3
- Review effort level: Lite
A missing qemu-system-x86_64 exited 127, which callers cannot tell apart from firmware that failed to reach BDS. Check the command up front and report it as a setup problem instead. Assisted-by: GitHub Copilot:claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kat-perez
force-pushed
the
kat-perez/issue-119-fpdt-capture
branch
from
September 4, 2026 19:46
0ed1c69 to
d2deca2
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The capture script’s current behavior has a misleading failure mode when extracting the dump log and it doesn’t yet directly emit ms-resolution timing output per the linked acceptance criteria.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/7 changed files
- Comments generated: 2
- Review effort level: Lite
Assisted-by: GitHub Copilot:claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds the capture half of the harness: a disk image that boots to the UEFI Shell and dumps the firmware basic boot performance table, and a script that boots the firmware with it, recovers the table and parses it. The guest powers itself off when the dump finishes, so a capture ends on its own rather than on a timeout. Firmware built without performance tracing still boots and silently produces nothing, so the capture checks the configuration the firmware reports and fails with the flag to rebuild with. The packaged fpdt_parser reads the live table from a running Windows system and so cannot even be imported elsewhere, though none of that is used when parsing a captured binary. A wrapper supplies the missing pieces so the same parser runs where CI does, and raises if a Windows-only path is genuinely reached. Assisted-by: GitHub Copilot:claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Running the harness parser wrapper leaves a __pycache__ directory beside it, which is the repository's first Python source. Assisted-by: GitHub Copilot:claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Same argument handling as the boot harness: a flag given without a value read an unset positional and aborted under 'set -u' with exit 1, the code that means the capture itself failed, and a non-numeric timeout was only caught once QEMU was already running. Assisted-by: GitHub Copilot:claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A missing qemu, mtools or parser dependency exited 127 or failed partway through a capture, which callers cannot tell apart from firmware that produced no data. Check them up front and report a setup problem. Locate the parser module rather than importing it. On Linux the import itself raises on Windows-only ctypes names, which is exactly what the wrapper works around, so importing would reject a working host. Assisted-by: GitHub Copilot:claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
WinError returns an exception rather than raising one, so binding it to OSError handed back an instance the caller could ignore and carry on with nonsense values. Raise instead, matching the windll stand-in. Assisted-by: GitHub Copilot:claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Print ms-resolution boot milestones from the ACPI basic boot record after each capture, and install the dump log only once mtype succeeds so a failed extraction is no longer reported as a missing table. Assisted-by: GitHub Copilot:claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kat-perez
force-pushed
the
kat-perez/issue-119-fpdt-capture
branch
from
September 4, 2026 20:16
d2deca2 to
170fe52
Compare
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The capture flow, parser wrapper, and documentation are consistent with the stated behavior and add the required per-invocation ms boot-time output without introducing verified defects in the changed code.
Review details
- Files reviewed: 7/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
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.
Adds the capture half of the QEMU performance harness: boot the firmware, recover the firmware basic boot performance table (FBPT) it produced, and parse it.
Closes #119.
Stacked on #211 — base that first, or review only the second commit here.
What it does
make-fbpt-disk.shbuilds a FAT image that boots to the UEFI Shell and runsFbptDump.efi, which writes the table back to the same image. The shell's output is redirected onto the disk, since the dump application reports through standard output rather than the debug console and the platform has no serial console inConOut.capture-fbpt.shboots the firmware with that disk attached, waits for the guest to power itself off, copies the table out and parses it to XML and text.Firmware built without
PERF_TRACE_ENABLEboots normally and silently produces nothing, so the capture asserts on the configuration the firmware reports and fails with the flag to rebuild with.The parser wrapper
fpdt_parserfromedk2-pytool-extensionscan read the live FPDT from a running Windows system. It therefore importswindll/WinErrorat module scope and constructs its Windows firmware-table accessor unconditionally inmain, so it cannot even be imported on Linux — which is where CI runs.None of that is used when parsing a captured binary: every call site is guarded on
input_fbpt_bin is None.fpdt_parser_any_platform.pysupplies the missing names and replaces the accessor, and raises if a Windows-only path is genuinely reached rather than silently misbehaving. Verified to produce identical output on Windows and Linux from the same binary (564 records, same XML length).Verification
End-to-end from a clean state, against firmware built from
patina-qemuv4.0.5 withBLD_*_PERF_TRACE_ENABLE=TRUE:Failure paths checked individually:
Sample phase timings from a captured run, via
perf_report_generator:Notes
FbptDump.efiis not in a default build; the README records thatUefiTestingPkg/PerfTests/FbptDump/FbptDump.infhas to be added to the platform description first.