Add snp direct kernel boot case #4362
Conversation
|
(1/1) Host_RHEL.m9.u7.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.7.0.x86_64.io-github-autotest-qemu.snp_direct_kernel_boot.q35: STARTED |
JinLiul
force-pushed
the
snp_kernel_hash
branch
from
a80d8a9 to
f3fc812
Compare
|
(1/2) Host_RHEL.m10.u1.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.1.x86_64.io-github-autotest-qemu.unattended_install.cdrom.extra_cdrom_ks.default_install.aio_threads.q35: STARTED |
JinLiul
force-pushed
the
snp_kernel_hash
branch
from
f3fc812 to
081472b
Compare
|
@zixi-chen @XueqiangWei @zhencliu Could you please help to review? Thanks. |
qemu/tests/snp_direct_kernel_boot.py
Outdated
| session.cmd_output("chmod 755 %s" % guest_cmd) | ||
| except Exception as e: | ||
| test.fail("Guest test preparation fail: %s" % str(e)) | ||
| guest_cmd = guest_cmd + " " + host_cpu_model |
There was a problem hiding this comment.
guest_cmd += f" {host_cpu_model}" should be simpler. Certainly, your code also works.
There was a problem hiding this comment.
Thanks for the suggestion. Currently, several cases are using this style. I plan to simplify it during a later optimization.
Signed-off-by: Jin Liu <[email protected]>
JinLiul
force-pushed
the
snp_kernel_hash
branch
from
081472b to
1fb653e
Compare
WalkthroughIntroduces a new QEMU test for SEV direct kernel boot with configuration and a Python test runner. Supports SEV and SNP variants, sets VM parameters, boots a kernel/initrd, verifies dmesg and guest checks, and performs SNP attestation using provided guest/host scripts. Includes CPU model detection and structured error handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester
participant Runner as Test Runner
participant QEMU as QEMU VM
participant Guest as Guest OS
participant Host as Host (attest svc)
Tester->>Runner: Start sev_direct_kernel_boot
Runner->>Runner: Load params (variant, kernel/initrd, scripts)
Runner->>QEMU: Launch VM (SEV/SNP, memory/CPU, kernel boot)
QEMU-->>Runner: Console/login ready
Runner->>Guest: Login session
Runner->>Guest: verify_dmesg + guest_check_cmd
alt secure_guest_type == SNP
Runner->>Guest: Push guest_script, set exec perms
Runner->>Guest: Install guest tools (if needed)
Runner->>Guest: Run attestation cmd (cpu model param)
Guest->>Host: Attestation request
Host-->>Guest: Attestation result
Guest-->>Runner: Exit status
else secure_guest_type == SEV
Note over Runner,Guest: Skip attestation
end
Runner->>QEMU: Shutdown/destroy VM
Runner-->>Tester: Report pass/fail
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
|
(1/1) Host_RHEL.m10.u1.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.1.x86_64.io-github-autotest-qemu.sev_direct_kernel_boot.sev.q35: STARTED (1/1) Host_RHEL.m10.u1.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.1.x86_64.io-github-autotest-qemu.sev_direct_kernel_boot.snp.q35: STARTED |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
qemu/tests/cfg/sev_direct_kernel_boot.cfg (3)
19-19: Kernel params tie to “rhel” LVM VG; consider parameterizingHard-coding rd.lvm.lv=rhel/root and rhel/swap may fail on images with different VG names (e.g., RHEL 10 derivatives). Consider parameterizing VG or deriving it from image metadata to avoid false negatives.
15-17: Use a safer staging dir in guestCopying scripts to /home can hit permission/SELinux issues on minimal images. Prefer /var/tmp (or a param) to reduce flakiness.
6-6: Avoid config/code tug-of-war on start_vmThe test flips start_vm to “yes” in code. To reduce confusion, either remove start_vm = no here or rely purely on config. Pick one source of truth.
qemu/tests/sev_direct_kernel_boot.py (1)
63-73: Harden attestation step: existence check and better diagnosticsFail fast if the host script is missing, and surface stdout on failures. Pass CPU arg only when known.
deps_dir = virttest_data_dir.get_deps_dir() host_file = os.path.join(deps_dir, host_script) try: + if not os.path.exists(host_file): + test.error(f"Host script not found: {host_file}") vm.copy_files_to(host_file, guest_dir) session.cmd_output(params["guest_tool_install"], timeout=240) session.cmd_output("chmod 755 %s" % guest_cmd) except Exception as e: test.fail("Guest test preparation fail: %s" % str(e)) - guest_cmd = guest_cmd + " " + host_cpu_model - s = session.cmd_status(guest_cmd, timeout=360) - if s: - test.fail("Guest script error") + attest_cmd = guest_cmd if host_cpu_model == "unknown" else f"{guest_cmd} {host_cpu_model}" + status, out = session.cmd_status_output(attest_cmd, timeout=360) + if status: + test.fail(f"Guest script error (exit={status}): {out}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/sev_direct_kernel_boot.cfg(1 hunks)qemu/tests/sev_direct_kernel_boot.py(1 hunks)
🔇 Additional comments (1)
qemu/tests/cfg/sev_direct_kernel_boot.cfg (1)
26-29: Confirm OVMF for SEV variant (and consider setting bios_path explicitly)Only the snp branch sets bios_path. If the framework doesn’t auto-pick an SEV-capable OVMF for the sev variant, it can silently boot without SEV. Please confirm on your target hosts and, if needed, add an explicit bios_path for sev as well.
| guest_dir = /home | ||
| guest_cmd = ${guest_dir}/${attestation_script} | ||
| host_script = sev-snp/${attestation_script} | ||
| vm_sev_kernel_hashes = on |
There was a problem hiding this comment.
Scope kernel-hashes to SNP only to avoid SEV variant breakage
The kernel-hashes property is SNP-specific. Keeping it at the top-level risks leaking into the SEV variant depending on how the QEMU object is built. Move it under the snp variant.
- vm_sev_kernel_hashes = on
@@
- snp:
bios_path = /usr/share/edk2/ovmf/OVMF.amdsev.fd
vm_secure_guest_type = snp
+ vm_sev_kernel_hashes = on
snp_module_path = "/sys/module/kvm_amd/parameters/sev_snp"
snp_guest_check = "journalctl|grep -i -w snp"Also applies to: 21-26
🤖 Prompt for AI Agents
In qemu/tests/cfg/sev_direct_kernel_boot.cfg around line 18 (and also apply same
change to lines 21-26), the vm_sev_kernel_hashes setting is declared at
top-level but is SNP-specific; move the vm_sev_kernel_hashes = on entry into the
snp variant block (indent appropriately to match surrounding variant keys) so it
is only set when snp is selected and not leaked into the SEV variant.
There was a problem hiding this comment.
kernel-hashes can work with both snp and sev.
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
QEMU vm_sev_kernel_hashes SEV SNP compatibility
💡 Result:
Short answer: Yes — recent QEMU patches add support for vm_sev_kernel_hashes for SEV‑SNP (measured direct kernel boot). QEMU now places the kernel-hashes area into the SNP-designated metadata page (a whole 4 KiB page is zeroed/filled and encrypted for SNP), and the kernel-hashes property was moved to common SEV/SNP properties so it works for SNP as well. Key details are in the QEMU patch series. [1][2][3]
References
- QEMU patch: "Allow measured direct kernel boot on SNP" (describes SNP kernel-hashes page handling, 4 KiB page measurement). [1]
- QEMU patch: "Move kernel-hashes to SevCommonProperties" (makes kernel-hashes available for both SEV and SNP). [2]
- QEMU patch: "Extract build_kernel_loader_hashes" (prepares hashes handling changes used for SNP). [3]
@JinLiul, thank you for the clarification! You're correct. Recent QEMU patches moved kernel-hashes to common SEV/SNP properties, making it available for both SEV and SNP variants. The current top-level placement is appropriate. I apologize for the confusion in my initial review.
✏️ Learnings added
Learnt from: JinLiul
PR: autotest/tp-qemu#4362
File: qemu/tests/cfg/sev_direct_kernel_boot.cfg:18-18
Timestamp: 2025-09-30T07:12:07.712Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| family_id = cpu.get_family() | ||
| model_id = cpu.get_model() | ||
| dict_cpu = {"251": "milan", "2517": "genoa", "2617": "turin"} | ||
| key = str(family_id) + str(model_id) | ||
| host_cpu_model = dict_cpu.get(key, "unknown") | ||
|
|
There was a problem hiding this comment.
Make CPU model detection robust; avoid string concatenation collisions
Concatenating family+model (e.g., "2517") is brittle. Use tuple keys and a fallback for family ranges.
- family_id = cpu.get_family()
- model_id = cpu.get_model()
- dict_cpu = {"251": "milan", "2517": "genoa", "2617": "turin"}
- key = str(family_id) + str(model_id)
- host_cpu_model = dict_cpu.get(key, "unknown")
+ family_id = int(cpu.get_family())
+ model_id = int(cpu.get_model())
+ cpu_map = {
+ (25, 1): "milan",
+ (25, 17): "genoa",
+ (26, 17): "turin",
+ }
+ host_cpu_model = cpu_map.get((family_id, model_id))
+ if not host_cpu_model:
+ # Fallback by family grouping
+ if family_id == 25 and model_id >= 16:
+ host_cpu_model = "genoa"
+ elif family_id == 26:
+ host_cpu_model = "turin"
+ else:
+ host_cpu_model = "unknown"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| family_id = cpu.get_family() | |
| model_id = cpu.get_model() | |
| dict_cpu = {"251": "milan", "2517": "genoa", "2617": "turin"} | |
| key = str(family_id) + str(model_id) | |
| host_cpu_model = dict_cpu.get(key, "unknown") | |
| family_id = int(cpu.get_family()) | |
| model_id = int(cpu.get_model()) | |
| cpu_map = { | |
| (25, 1): "milan", | |
| (25, 17): "genoa", | |
| (26, 17): "turin", | |
| } | |
| host_cpu_model = cpu_map.get((family_id, model_id)) | |
| if not host_cpu_model: | |
| # Fallback by family grouping | |
| if family_id == 25 and model_id >= 16: | |
| host_cpu_model = "genoa" | |
| elif family_id == 26: | |
| host_cpu_model = "turin" | |
| else: | |
| host_cpu_model = "unknown" |
🤖 Prompt for AI Agents
In qemu/tests/sev_direct_kernel_boot.py around lines 33 to 38, the CPU model
detection currently concatenates family_id and model_id into a string key which
can collide; change the lookup to use tuple keys of integers (key = (family_id,
model_id)) with a dict that maps explicit (family, model) pairs to names, and
add a fallback family-only mapping (or range checks) that maps family_id to a
default model name if the exact tuple is not found, finally defaulting to
"unknown".
| vm_name = params["main_vm"] | ||
| env_process.preprocess_vm(test, params, env, vm_name) | ||
| vm = env.get_vm(vm_name) | ||
| vm.verify_alive() | ||
| session = vm.wait_for_login(timeout=timeout) | ||
| verify_dmesg() |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Broaden try/finally for reliable cleanup; verify host dmesg at end
If preprocess/login fails before Line 51, VM/session aren’t cleaned up. Also, verify_dmesg is more useful after the test. Wrap from preprocess through the end in try/finally, check dmesg at the end, and guard session/vm.
- vm_name = params["main_vm"]
- env_process.preprocess_vm(test, params, env, vm_name)
- vm = env.get_vm(vm_name)
- vm.verify_alive()
- session = vm.wait_for_login(timeout=timeout)
- verify_dmesg()
-
- try:
+ vm_name = params["main_vm"]
+ vm = None
+ session = None
+ try:
+ env_process.preprocess_vm(test, params, env, vm_name)
+ vm = env.get_vm(vm_name)
+ vm.verify_alive()
+ session = vm.wait_for_login(timeout=timeout)
+ # Defer host dmesg verification to the end to catch issues during/after boot
session.cmd_output(guest_check_cmd, timeout=240)
@@
- finally:
- session.close()
- vm.destroy()
+ finally:
+ # Verify host dmesg at end (warn-only to not mask earlier failures)
+ try:
+ verify_dmesg()
+ except Exception as e:
+ test.log.warning(f"Host dmesg verification failed: {e}")
+ if session:
+ session.close()
+ if vm:
+ vm.destroy()Also applies to: 51-76
🤖 Prompt for AI Agents
In qemu/tests/sev_direct_kernel_boot.py around lines 44 to 76, broaden the
cleanup scope so that preprocess_vm, vm lookup, login and the rest of the test
are wrapped in a single try/finally; move verify_dmesg to run in the finally
block after all test actions so host dmesg is always checked at the end; in the
finally guard accesses by checking for session existence before closing it and
for vm existence before performing VM cleanup (stop/destroy) to avoid
AttributeError when preprocess/login fails; ensure any exceptions from cleanup
are caught/logged but do not prevent verify_dmesg from executing.
|
@zixi-chen I combined sev+snp direct kernel boot into one test case. Please help to review. Thanks. |
ID: 4069
Summary by CodeRabbit