uefi/perf: Add QEMU Q35 boot harness by kat-perez · Pull Request #211 · OpenDevicePartnership/odp-platform-common

kat-perez · 2026-09-04T15:55:53Z

Adds uefi/perf/qemu/run-q35-boot.sh, which boots a Patina Q35 firmware pair under QEMU and reports whether it reaches BDS. This is the bringup step for measuring Patina boot behaviour on a reproducible machine rather than on hardware.

Closes #118.

What it does

Boots QEMUQ35_CODE.fd / QEMUQ35_VARS.fd with the machine wiring the Patina Q35 platform expects, copying the variable store so the firmware directory stays reusable.
Watches the debug console for the BDS entry marker and stops QEMU as soon as it appears, since the firmware keeps running when no boot device is attached.
Exits 0 on reaching BDS, 1 on timeout, and 2 for a setup problem, so a firmware regression is distinguishable from a broken invocation.
Keeps boot-debugcon.log as the artifact to inspect on failure.

Verification

Run against a locally built patina-qemu v4.0.5 firmware:

PASS: reached BDS
939:INFO - DXE Core Platform Binary v3.1.2
2769:[Bds] Entry...

Exit codes checked individually: unknown argument 2, missing --firmware-dir 2, firmware not found 2, --help 0.

Notes

The README documents why PERF_TRACE_ENABLE must be set at build time. The platform PEI always publishes the Patina performance configuration HOB, and read_performance_config returns it whenever the HOB exists, so a present-but-disabled HOB outranks the DXE Core's default. Replacing only the DXE Core binary cannot turn measurement on.
*.sh text eol=lf is added because this repository is cloned on Windows with core.autocrlf=true, which yields a CRLF shebang that neither Linux CI nor WSL will run.
No workflow calls this yet; CI integration is tracked separately.

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>

Copilot

🟡 Changes recommended

The new harness has confirmed argument/timeout handling bugs that can cause unintended exit codes and confusing failures under common CLI misuse (missing or non-numeric flag values).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds a reproducible QEMU Q35 boot bringup harness for Patina firmware, intended as the first step toward consistent performance/boot-behavior measurement in CI by detecting when the firmware reaches BDS via debug console output.

Changes:

Introduces run-q35-boot.sh to boot the Patina Q35 firmware pair under QEMU, monitor the debug console for the BDS entry marker, and stop QEMU once reached.
Adds a README documenting prerequisites, how to obtain/build the firmware, how to run the harness, and how to confirm performance tracing is enabled.
Pins *.sh files to LF line endings via .gitattributes to avoid CRLF-shebang execution failures on Linux/WSL.

File summaries

File	Description
uefi/perf/qemu/run-q35-boot.sh	New QEMU Q35 boot harness that detects BDS via debugcon log and returns structured exit codes.
uefi/perf/qemu/README.md	Usage and firmware acquisition/build documentation for the new harness.
.gitattributes	Enforces LF EOL for shell scripts to keep shebangs runnable in Linux CI/WSL.

Review details

Suppressed comments (1)

uefi/perf/qemu/run-q35-boot.sh:126

timeout_seconds is used in arithmetic expansion ($((SECONDS + timeout_seconds))). If --timeout is non-numeric (or empty due to a missing value), the script will exit due to set -e with a confusing error and exit code 1 instead of EXIT_USAGE (2). Consider validating --timeout before computing the deadline.

deadline=$((SECONDS + timeout_seconds))

Files reviewed: 3/3 changed files
Comments generated: 1
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>

Copilot

🔵 Needs a closer look

The script’s documented exit-code contract isn’t met for common setup/invocation failures (e.g., missing QEMU or early QEMU exit), which will misclassify errors and hinder regression triage.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

uefi/perf/qemu/run-q35-boot.sh:123

The PR description/README state that setup problems should exit with code 2, but if qemu-system-x86_64 is missing this script will currently exit with bash's command not found status (127) due to set -e. Add an explicit PATH check and use EXIT_USAGE so callers can reliably distinguish setup failures from firmware timeouts.

This issue also appears on line 155 of the same file.

uefi/perf/qemu/run-q35-boot.sh:158

When QEMU exits early (e.g., unsupported flags/options, missing KVM/TCG features, etc.), the loop breaks but the script still reports a timeout-style failure and exits 1 later. That contradicts the documented exit code contract (2 for setup/invocation problems) and makes regressions harder to triage. Exit EXIT_USAGE immediately when QEMU is gone, while still pointing to the boot log artifact.

  if ! kill -0 "$qemu_pid" 2>/dev/null; then
    echo "QEMU exited before reaching BDS" >&2
    break
  fi

Files reviewed: 3/3 changed files
Comments generated: 0 new
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>

Copilot

🟡 Changes recommended

The harness’ documented exit-code contract isn’t consistently enforced for setup/environment failures, which can misclassify harness problems as firmware timeouts/regressions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

uefi/perf/qemu/run-q35-boot.sh:121

Several setup steps rely on set -e and will exit with the underlying command status (often 1) instead of the documented EXIT_USAGE=2. For example, a non-writable --out-dir, a failed mktemp, or a failed cp of the VARS image will currently be indistinguishable from a firmware timeout (exit 1). Consider explicitly mapping these setup failures to exit 2 with a clear error message.

This issue also appears in the following locations of the same file:

line 127
line 146

uefi/perf/qemu/run-q35-boot.sh:147

If QEMU fails immediately due to an invocation/setup issue (unsupported flags, bad -global, etc.), the loop treats it as a firmware failure (exit 1). Since the PR description calls out exit 2 for setup problems, consider detecting an immediate QEMU exit with no debugcon output and returning EXIT_USAGE=2 in that case.

  -no-reboot &
qemu_pid=$!

uefi/perf/qemu/run-q35-boot.sh:131

cp/chmod of the writable VARS image and creation of boot-debugcon.log can still fail with exit code 1 due to set -e, even though these are setup/environment problems (and the PR description says setup problems should be exit 2). Consider explicitly mapping these failures to EXIT_USAGE=2 so callers can reliably distinguish harness failures from firmware timeouts.

vars_fd="${out_dir}/QEMUQ35_VARS.writable.fd"
cp "$vars_fd_source" "$vars_fd"
chmod u+w "$vars_fd"

: > "$boot_log"

Files reviewed: 3/3 changed files
Comments generated: 1
Review effort level: Lite

Assisted-by: GitHub Copilot:claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

🟡 Changes recommended

The harness can misclassify an early QEMU exit as a firmware timeout/failure (exit 1) rather than a setup/invocation problem (exit 2), which undermines the stated exit-code contract for regression detection.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 3/3 changed files
Comments generated: 1
Review effort level: Lite

+deadline=$((SECONDS + timeout_seconds))
+reached_bds=false
+
+while [ "$SECONDS" -lt "$deadline" ]; do
+  if grep -qF "$BDS_READY_MARKER" "$boot_log" 2>/dev/null; then
+    reached_bds=true
+    break
+  fi
+  if ! kill -0 "$qemu_pid" 2>/dev/null; then
+    echo "QEMU exited before reaching BDS" >&2
+    break
+  fi
+  sleep "$POLL_INTERVAL_SECONDS"
+done
+
+stop_qemu
+trap - EXIT
+
+echo "boot log: ${boot_log}"
+
+if [ "$reached_bds" != true ]; then
+  echo "FAIL: did not reach BDS within ${timeout_seconds}s" >&2
+  exit 1
+fi


rogurr · 2026-09-08T17:12:48Z

+
+## Getting firmware
+
+The quickest path is a published build. Releases of


Do we need to state there are published builds?

It looks like to get the performance data, it is a compilation time change using the 'BLD_*_PERF_TRACE_ENABLE=TRUE' flag. So maybe just show them how to compile?

rogurr · 2026-09-08T17:14:09Z

@@ -0,0 +1,71 @@
+# QEMU bringup for Patina firmware


I'm a little confused by the title, is this for running a performance measurement?

rogurr · 2026-09-08T17:18:48Z

+stuart_build  -c Platforms/QemuQ35Pkg/PlatformBuild.py 'BLD_*_PERF_TRACE_ENABLE=TRUE'
+```
+
+The images land in `Build/QemuQ35Pkg/DEBUG_CLANGPDB/FV/`. The build uses the


This looks a lot like it was AI generated since it tends to be kind of wordy on minute details not really relavent. Is there already a build README.md in the QEMU repo? If this tool is dependent on that build, maybe just say build using the instructions in [README.md] (http:// ) with the BLD switch?

And the next paragraph I think is good, but it appears AI targeted why we need to use the switch, not really what the new HOB is providing during boot.

rogurr · 2026-09-08T17:22:04Z

+## Requirements
+
+- `qemu-system-x86_64` on `PATH`.
+- A Q35 firmware directory containing `QEMUQ35_CODE.fd` and `QEMUQ35_VARS.fd`.


Is this only for the QEMU Q35 build from the ODP repository? Or can it work on other QEMU builds? What about Radxa? Or the other internal platform builds?

Copilot AI lite review requested due to automatic review settings September 4, 2026 15:55

kat-perez requested a review from a team as a code owner September 4, 2026 15:55

kat-perez requested review from maxperga-msft, philgweber, rogurr and williampMSFT September 4, 2026 15:55

Copilot started reviewing on behalf of kat-perez September 4, 2026 15:56 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Comment thread uefi/perf/qemu/run-q35-boot.sh

kat-perez mentioned this pull request Sep 4, 2026

uefi/perf: Capture FPDT data from a QEMU boot #212

Open

Copilot AI review requested due to automatic review settings September 4, 2026 18:32

Copilot started reviewing on behalf of kat-perez September 4, 2026 18:33 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 4, 2026 19:33

Copilot started reviewing on behalf of kat-perez September 4, 2026 19:34 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Comment thread uefi/perf/qemu/README.md Outdated

uefi/perf: Document the missing-tool exit code

af70c58

Assisted-by: GitHub Copilot:claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings September 4, 2026 20:15

Copilot started reviewing on behalf of kat-perez September 4, 2026 20:16 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

rogurr reviewed Sep 8, 2026

View reviewed changes

felipebalbi approved these changes Sep 8, 2026

View reviewed changes

Navigation Menu

Uh oh!

uefi/perf: Add QEMU Q35 boot harness - #211

uefi/perf: Add QEMU Q35 boot harness#211
kat-perez wants to merge 4 commits into
OpenDevicePartnership:mainfrom
kat-perez:kat-perez/issue-118-qemu-bringup

kat-perez commented Sep 4, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

rogurr Sep 8, 2026

Uh oh!

rogurr Sep 8, 2026

Uh oh!

rogurr Sep 8, 2026

Uh oh!

rogurr Sep 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants


		## Getting firmware

		The quickest path is a published build. Releases of

Uh oh!

Conversation

kat-perez commented Sep 4, 2026

What it does

Verification

Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (2)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (3)

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

rogurr Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

rogurr Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

rogurr Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

rogurr Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants