snp: Add legacy attestation workflow for snpguest #4428
Conversation
WalkthroughA new Bash script for SNP attestation workflow was added at qemu/deps/sev-snp/regular_attestation_workflow_legacy.sh. The script requires a cpu_model parameter, provides a check_status helper for error handling, and a fetch_retry helper that retries up to three times with 20s backoff. It runs a sequence of snpguest commands to generate and display an attestation report, fetches CA and VCEK certificates, and verifies certificates and attestation. Four configuration files in qemu/tests/cfg/ now reference this script by changing attestation_script from regular_attestation_workflow.sh to regular_attestation_workflow_legacy.sh. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
The updated regular_attestation_workflow.sh uses upstream snpguest command syntax which is incompatible with RHEL packaged snpguest. Add regular_attestation_workflow_legacy.sh to support RHEL snpguest package as a workaround. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
qemu/deps/sev-snp/regular_attestation_workflow_legacy.sh (2)
3-8: Consider accepting the exit code as a parameter for robustness.The
check_statusfunction relies on$?from the immediately preceding command. While this works when called correctly, it's fragile if any command inadvertently runs between the target command and this function call.🔎 Suggested refactor to accept exit code as parameter
check_status() { - if [ $? -ne 0 ]; then - echo "Error: $1" + local exit_code=$1 + local error_msg=$2 + if [ $exit_code -ne 0 ]; then + echo "Error: $error_msg" exit 1 fi }Then call it as:
check_status $? "Failed display attestation-report."
38-44: Consider checking the exit status of the report generation command.The script checks file existence (lines 39-42) but not the exit status of the
snpguest reportcommand (line 38). While this works, it's inconsistent with the error-checking pattern used for other commands.🔎 Suggested enhancement for consistency
snpguest report attestation-report.bin request-data.txt --random +check_status "Failed to generate attestation report." if [[ ! -f attestation-report.bin ]]; then echo "attestation-report.bin not created." exit 1 fi
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
qemu/deps/sev-snp/regular_attestation_workflow_legacy.shqemu/tests/cfg/snp_basic_config.cfgqemu/tests/cfg/snp_cpu_model.cfgqemu/tests/cfg/snp_multi_vm.cfg
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: JinLiul
Repo: autotest/tp-qemu PR: 4362
File: qemu/tests/cfg/sev_direct_kernel_boot.cfg:18-18
Timestamp: 2025-09-30T07:12:07.731Z
Learning: In QEMU SEV testing configurations, the vm_sev_kernel_hashes property works with both SEV and SNP variants as it was moved to common SevCommonProperties in recent QEMU patches, so it can be declared at the top level to apply to both variants.
📚 Learning: 2025-09-30T07:12:07.731Z
Learnt from: JinLiul
Repo: autotest/tp-qemu PR: 4362
File: qemu/tests/cfg/sev_direct_kernel_boot.cfg:18-18
Timestamp: 2025-09-30T07:12:07.731Z
Learning: In QEMU SEV testing configurations, the vm_sev_kernel_hashes property works with both SEV and SNP variants as it was moved to common SevCommonProperties in recent QEMU patches, so it can be declared at the top level to apply to both variants.
Applied to files:
qemu/tests/cfg/snp_multi_vm.cfgqemu/tests/cfg/snp_cpu_model.cfgqemu/deps/sev-snp/regular_attestation_workflow_legacy.shqemu/tests/cfg/snp_basic_config.cfg
🔇 Additional comments (6)
qemu/tests/cfg/snp_cpu_model.cfg (1)
18-18: LGTM!Consistent with the attestation script update in
snp_basic_config.cfg. The change correctly references the new legacy workflow script.qemu/tests/cfg/snp_multi_vm.cfg (1)
17-17: LGTM!Consistent with the attestation script updates in the other configuration files.
qemu/deps/sev-snp/regular_attestation_workflow_legacy.sh (4)
10-17: LGTM!Good defensive programming with clear parameter validation and usage message. The
cpu_modelvalue validation is delegated tosnpguest, which is appropriate.
19-35: LGTM!The retry logic is well-implemented with appropriate backoff. The use of
evalis acceptable here since the command strings are controlled literals rather than user input.
46-51: LGTM!Certificate fetching with retry logic is correctly implemented. The
cpu_modelparameter is properly passed tosnpguestcommands, and error handling is consistent.
53-57: LGTM!Verification steps are correctly implemented with appropriate error handling. Note that temporary files (attestation report, certificates) are not cleaned up, which may be intentional for test debugging purposes.
| snp_guest_check = "journalctl|grep -i -w snp" | ||
| guest_tool_install = "dnf install -y snpguest" | ||
| attestation_script = regular_attestation_workflow.sh | ||
| attestation_script = regular_attestation_workflow_legacy.sh |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all attestation workflow files
fd --type f "regular_attestation_workflow" qemu/Repository: autotest/tp-qemu
Length of output: 166
🏁 Script executed:
#!/bin/bash
# Check file sizes first
wc -l qemu/deps/sev-snp/regular_attestation_workflow.sh qemu/deps/sev-snp/regular_attestation_workflow_legacy.shRepository: autotest/tp-qemu
Length of output: 187
🏁 Script executed:
#!/bin/bash
# Read both files to compare
echo "=== regular_attestation_workflow.sh ==="
cat -n qemu/deps/sev-snp/regular_attestation_workflow.sh
echo ""
echo "=== regular_attestation_workflow_legacy.sh ==="
cat -n qemu/deps/sev-snp/regular_attestation_workflow_legacy.shRepository: autotest/tp-qemu
Length of output: 3950
🏁 Script executed:
#!/bin/bash
# Check the snp_basic_config.cfg file for context around the attestation script
cat -n qemu/tests/cfg/snp_basic_config.cfg | head -30Repository: autotest/tp-qemu
Length of output: 1288
🏁 Script executed:
#!/bin/bash
# Search for any comments or documentation about the legacy attestation workflow
rg -i "legacy.*attestation|attestation.*legacy" qemu/tests/cfg/ -B 2 -A 2Repository: autotest/tp-qemu
Length of output: 1297
🏁 Script executed:
#!/bin/bash
# Search for any commit messages or documentation mentioning the legacy workflow
rg "regular_attestation_workflow" qemu/deps/sev-snp/ -B 3 -A 3Repository: autotest/tp-qemu
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check for any README or documentation files in the sev-snp directory
ls -la qemu/deps/sev-snp/ | head -20Repository: autotest/tp-qemu
Length of output: 414
Document the reason for using the legacy attestation workflow.
Both regular_attestation_workflow.sh and regular_attestation_workflow_legacy.sh exist in the codebase. The legacy version uses an older snpguest command syntax (different argument ordering and missing the -p flag in certain commands), indicating compatibility with an older tool version. While the use of the legacy script is consistent across all SNP test configurations (snp_basic_config.cfg, snp_multi_vm.cfg, snp_cpu_model.cfg), the codebase contains no documentation explaining why this older version is required. Add a comment in the configuration file documenting the compatibility requirement or tool version constraint that necessitates the legacy workflow.
🤖 Prompt for AI Agents
In qemu/tests/cfg/snp_basic_config.cfg around line 19, the attestation_script is
set to regular_attestation_workflow_legacy.sh but there is no documentation
explaining why the legacy workflow is required; add a concise comment above this
line stating the compatibility constraint (e.g., which snpguest/tool version or
argument-ordering requirement necessitates using the legacy script) so readers
know why the older syntax is used and do the same explanatory comment in other
SNP config files for consistency.
|
(7/9) Host_RHEL.m10.u2.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.2.x86_64.io-github-autotest-qemu.snp_basic_config.policy_default.q35: STARTED |
|
@PaulYuuu could you please help to review? Thanks. |
|
@PaulYuuu could you please help to review this patch and any feedback would be appreciated. Thanks. |
Signed-off-by: Jin Liu <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @qemu/tests/cfg/sev_direct_kernel_boot.cfg:
- Line 14: The top-level settings attestation_script =
regular_attestation_workflow_legacy.sh and guest_tool_install (which installs
snpguest) are SNP-specific and must be moved into the SNP variant block to avoid
running snpguest commands in the SEV variant; remove these two entries from the
global scope and add them inside the SNP variant section so only the SNP
configuration uses regular_attestation_workflow_legacy.sh and installs snpguest
(the script uses snpguest report/display/fetch/verify).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
qemu/deps/sev-snp/regular_attestation_workflow_legacy.shqemu/tests/cfg/sev_direct_kernel_boot.cfgqemu/tests/cfg/snp_basic_config.cfgqemu/tests/cfg/snp_cpu_model.cfgqemu/tests/cfg/snp_multi_vm.cfg
🚧 Files skipped from review as they are similar to previous changes (3)
- qemu/deps/sev-snp/regular_attestation_workflow_legacy.sh
- qemu/tests/cfg/snp_multi_vm.cfg
- qemu/tests/cfg/snp_cpu_model.cfg
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: JinLiul
Repo: autotest/tp-qemu PR: 4362
File: qemu/tests/cfg/sev_direct_kernel_boot.cfg:18-18
Timestamp: 2025-09-30T07:12:07.731Z
Learning: In QEMU SEV testing configurations, the vm_sev_kernel_hashes property works with both SEV and SNP variants as it was moved to common SevCommonProperties in recent QEMU patches, so it can be declared at the top level to apply to both variants.
📚 Learning: 2025-09-30T07:12:07.731Z
Learnt from: JinLiul
Repo: autotest/tp-qemu PR: 4362
File: qemu/tests/cfg/sev_direct_kernel_boot.cfg:18-18
Timestamp: 2025-09-30T07:12:07.731Z
Learning: In QEMU SEV testing configurations, the vm_sev_kernel_hashes property works with both SEV and SNP variants as it was moved to common SevCommonProperties in recent QEMU patches, so it can be declared at the top level to apply to both variants.
Applied to files:
qemu/tests/cfg/sev_direct_kernel_boot.cfgqemu/tests/cfg/snp_basic_config.cfg
🔇 Additional comments (1)
qemu/tests/cfg/snp_basic_config.cfg (1)
19-19: LGTM! Attestation script update is appropriate for SNP configuration.The change to use the legacy attestation workflow is appropriate for this SNP-specific configuration and aligns with the PR objective of supporting RHEL-packaged snpguest.
| module_status = Y y 1 | ||
| guest_tool_install = "dnf install -y snpguest" | ||
| attestation_script = regular_attestation_workflow.sh | ||
| attestation_script = regular_attestation_workflow_legacy.sh |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the legacy script exists and check if it's SNP-specific
# First, let's find the script
echo "=== Searching for regular_attestation_workflow_legacy.sh ==="
fd -t f "regular_attestation_workflow_legacy.sh" 2>/dev/null || echo "Script not found via fd"
echo ""
echo "=== Checking qemu/deps/sev-snp/ directory structure ==="
ls -la qemu/deps/sev-snp/ 2>/dev/null | head -20 || echo "Directory not found"
echo ""
echo "=== Searching for any attestation script files ==="
fd -t f ".*attestation.*" qemu/ 2>/dev/null | head -20
echo ""
echo "=== Checking config file structure ==="
cat -n qemu/tests/cfg/sev_direct_kernel_boot.cfg | head -35Repository: autotest/tp-qemu
Length of output: 2181
🏁 Script executed:
#!/bin/bash
echo "=== Content of regular_attestation_workflow_legacy.sh ==="
cat -n qemu/deps/sev-snp/regular_attestation_workflow_legacy.sh
echo ""
echo "=== Comparing with the newer attestation workflow ==="
diff -u qemu/deps/sev-snp/regular_attestation_workflow.sh qemu/deps/sev-snp/regular_attestation_workflow_legacy.sh || true
echo ""
echo "=== Checking for snpguest references in scripts ==="
rg "snpguest" qemu/deps/sev-snp/Repository: autotest/tp-qemu
Length of output: 5894
Scope the legacy attestation script and snpguest installation to the SNP variant only.
The regular_attestation_workflow_legacy.sh script is entirely SNP-specific—it uses only snpguest commands (e.g., snpguest report, snpguest display, snpguest fetch, snpguest verify). However, both attestation_script (line 14) and guest_tool_install (line 13) are set at the top level, applying to both the SNP variant (lines 21–25) and SEV variant (lines 26–29).
The SEV variant would attempt to execute SNP-specific commands that don't exist in its environment, causing failures. Move these settings into the SNP variant block to prevent incompatibility.
🤖 Prompt for AI Agents
In @qemu/tests/cfg/sev_direct_kernel_boot.cfg at line 14, The top-level settings
attestation_script = regular_attestation_workflow_legacy.sh and
guest_tool_install (which installs snpguest) are SNP-specific and must be moved
into the SNP variant block to avoid running snpguest commands in the SEV
variant; remove these two entries from the global scope and add them inside the
SNP variant section so only the SNP configuration uses
regular_attestation_workflow_legacy.sh and installs snpguest (the script uses
snpguest report/display/fetch/verify).
| module_status = Y y 1 | ||
| guest_tool_install = "dnf install -y snpguest" | ||
| attestation_script = regular_attestation_workflow.sh | ||
| attestation_script = regular_attestation_workflow_legacy.sh |
There was a problem hiding this comment.
Can this combine with the original script regular_attestation_workflow.sh, they are similar but used for different areas, if we can use an unified script, that should better for maintainence.
There was a problem hiding this comment.
This is because the current RHEL snpguest version is relatively old. The regular_attestation_workflow.sh script is designed for the upstream snpguest version, and their usage is different, so they can’t be directly combined.
There was a problem hiding this comment.
While this change addresses the current RHEL snpguest version compatibility issue, I have a concern about future maintenance:
The RHEL snpguest version won't stay old forever. When RHEL eventually upgrades snpguest to a newer version with -p flag support, this legacy script will become incompatible and may block tests from running.
We could consider adding version detection logic to auto-switch between usage in one script.
There was a problem hiding this comment.
My original thought was to only target the RHEL provided snpguest, rather than trying to support both upstream and RHEL. At the moment, the version gap between upstream snpguest and the RHEL version is quite large (several upstream releases), and each version may have a different behavior.So implementing version detection might introduce some hard coded logic.
I’d prefer to keep the script aligned with the currently RHEL version and update it once the RHEL snpguest is upgraded.
There was a problem hiding this comment.
RHEL may also have different snpguest versions in different releases in the future, at that time, what's the solution of? 1 script per 1 version?
The current 2 scripts has common utility functions and they are dup. Actually I don't know why use shell script in the test, all can be converted to session.cmd. Right now the test need to copy the script to the guest, but if using session, 1 func can call another one, so similar functions can share same logic.
I would also cc @bssrikanth here, to discuss more.
There was a problem hiding this comment.
That’s a fair question.I don’t think maintaining one script per snpguest version would be a good solution.
If snpguest is updated in RHEL in the future, I’m glad to add version-detection logic at that point and adjust the behavior. I’d prefer to do this when there is a concrete RHEL version to support, rather than handling multiple middle upstream versions now.
I also agree that using session.cmd would make more sense. I’d like to refactor this logic with tp-libvirt, since we are in the process of migrating to it now.
There was a problem hiding this comment.
My thoughts:
I was just thinking if we can merge into a single Script with version detection? something like:
if [[ snpguest_version > X.Y.Z ]]; then
use -p, or the newer workflow
else
Use legacy mode
fi
But like @JinLiul mentioned if different snpguest versions have different flags/flow then its too much hard coded logic.
I would think for now we can have 2 attestation scripts as @JinLiul has posted here. One for legacy and one for upstream. New flags (like the -p mentioned) or behavioral changes will force updates anyway, and backward compatibility for older RHEL versions adds maintenance overhead regardless of the approach (shell scripts vs. session.cmd). That said upstream flow maintenance would be bit easier compared to RHEL packages versioning.
3 participants
ID: 4892
Summary by CodeRabbit
New Features
Configuration
✏️ Tip: You can customize this high-level summary in your review settings.