Tdx guest: multi guests boot with attestation #4356
Conversation
zixi-chen
force-pushed
the
tdx_multi_vm
branch
from
966b2b5 to
b38d449
Compare
zixi-chen
force-pushed
the
tdx_multi_vm
branch
2 times, most recently
from
5d6d8a5 to
658863a
Compare
|
@XueqiangWei @zhencliu Could you please help to review this patch? |
|
@zixi-chen Can you help add the test results? Thanks. |
Test results are added to ticket 4345, thanks. |
zixi-chen
force-pushed
the
tdx_multi_vm
branch
from
658863a to
c256bcb
Compare
WalkthroughAdds TDX attestation components: a new shell attestation script, a QEMU test configuration for multi-VM TDX scenarios (single_vcpu, four_vms, max_vms), and a Python test that provisions multiple VMs, verifies guest TDX state, runs attestation, and ensures cleanup. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Runner as Test Runner
participant Host as Host
participant Controller as Test Module
participant VM as VM Instance
participant Guest as Guest OS
participant Script as attestation.sh
participant Py as tdx.py
Runner->>Host: check tdx_module_path (host capability)
Host-->>Runner: capability OK / cancel
Controller->>Host: run max_vms_cmd / max_vcpu_cmd
Host-->>Controller: num_vms & smp info
Controller->>VM: create & start VMs (distribute smp/mem)
VM-->>Controller: booted / alive
Controller->>Guest: open session & run tdx_guest_check
Guest-->>Controller: verification result
alt Verified
Controller->>Guest: copy `host_script` & `attestation.sh`
Controller->>Guest: set perms & run `guest_cmd`
Guest->>Script: invoke attestation.sh (generate report, write inblob)
Script->>Py: download & execute Python attestation helper with outblob
Py-->>Script: attestation result
Script-->>Guest: output result
else Failed
Controller-->>Runner: mark test failed
end
Controller->>VM: destroy VMs
Controller->>Guest: close sessions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
qemu/tests/cfg/tdx_multi_vms.cfg (2)
14-14: Make the guest dmesg check more robust and quiet.Prefer restricting to kernel logs and suppressing pager/noise.
Apply:
- tdx_guest_check = "journalctl|grep -i -w tdx" + tdx_guest_check = journalctl -k --no-pager | grep -iqw tdx
21-34: Align VM naming and sources of truth for multi‑VM counts.Variants define vms as vm2/vm3/vm4 while the Python test may append vm_1..vm_{N-1} when num_vms is set (32_vms). Mixing styles risks mismatches.
- Use a single strategy: either fully declare vms in cfg per variant, or let the test compute and set them consistently (vm1..vmN).
- If keeping dynamic creation for 32_vms, standardize on vm1..vm32 (no underscores) to match other variants.
qemu/tests/tdx_multi_vms.py (1)
28-28: Fix log banner typo.It should be TDX only.
Apply:
- error_context.context("Start sev-tdx test", test.log.info) + error_context.context("Start TDX test", test.log.info)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
qemu/deps/tdx/attestation.sh(1 hunks)qemu/tests/cfg/tdx_multi_vms.cfg(1 hunks)qemu/tests/tdx_multi_vms.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/tdx_multi_vms.py (1)
generic/tests/trans_hugepage_swapping.py (1)
get_mem_info(21-29)
🪛 Shellcheck (0.11.0)
qemu/deps/tdx/attestation.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🔇 Additional comments (1)
qemu/tests/cfg/tdx_multi_vms.cfg (1)
10-10: Remove concern; quoting is consistent with repository convention.Double-quoted values for
machine_type_extra_paramsalign with the established pattern across the codebase. A search of existing configs reveals ~42 instances use double quotes (including similar params like"kernel-irqchip=split"), with only 3 unquoted outliers. The current usage at line 10 is correct and follows the standard.Likely an incorrect or invalid review comment.
| @@ -0,0 +1,8 @@ | |||
| set -e | |||
There was a problem hiding this comment.
Add a shebang and stricter shell options.
Current script has no shebang and only set -e. Add bash shebang and safer defaults.
Apply:
+#!/usr/bin/env bash
-set -e
+set -Eeuo pipefail
+IFS=$'\n\t'Based on ShellCheck SC2148 hint.
📝 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.
| set -e | |
| #!/usr/bin/env bash | |
| set -Eeuo pipefail | |
| IFS=$'\n\t' |
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🤖 Prompt for AI Agents
In qemu/deps/tdx/attestation.sh around line 1, the script currently only has
"set -e" and lacks a shebang and stricter shell options; add a bash shebang
(e.g. #!/usr/bin/env bash) as the first line and replace or augment the options
with safer defaults such as set -euo pipefail (and optionally set IFS=$'\n\t' if
the script relies on word-splitting safety), ensuring the file remains
executable.
| report=/sys/kernel/config/tsm/report/report0 | ||
| mkdir $report | ||
| dd if=/dev/urandom bs=64 count=1 > $report/inblob | ||
| hexdump -C $report/outblob |
There was a problem hiding this comment.
Harden configfs use and avoid brittle mkdir/expansions.
- mkdir without -p fails if report exists (and exits due to set -e).
- Unquoted expansions.
- No check that configfs/tsm path exists/mounted.
Apply:
-report=/sys/kernel/config/tsm/report/report0
-mkdir $report
-dd if=/dev/urandom bs=64 count=1 > $report/inblob
-hexdump -C $report/outblob
+report=/sys/kernel/config/tsm/report/report0
+report_dir="$(dirname "$report")"
+mkdir -p "$report"
+# Generate exactly 64 bytes for inblob
+dd if=/dev/urandom bs=64 count=1 status=none > "$report/inblob"
+# Optional: avoid dumping the full outblob into logs; print size only
+# hexdump -C "$report/outblob"
+stat -c 'outblob size: %s bytes' "$report/outblob" || true(Optional) Add a preflight check for configfs/tsm and mount if needed.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In qemu/deps/tdx/attestation.sh around lines 2 to 5, the script uses brittle
unquoted expansions and a plain mkdir which will fail under set -e if the
directory already exists, and it doesn't verify that configfs/tsm is present or
mounted. Change mkdir to mkdir -p and quote all variable expansions (e.g.
"$report" and "$report/inblob") everywhere; add a preflight check that the
configfs base (e.g. /sys/kernel/config/tsm) exists and is a mountpoint (and
attempt to mount configfs if absent) before creating the report directory; and
check for successful creation/writability before writing with dd and reading
with hexdump, failing with a clear message if any step fails.
| wget https://file.rdu.redhat.com/~berrange/tdx.py | ||
| pip install ecdsa pyopenssl pyasn1_modules | ||
| python tdx.py $report/outblob |
There was a problem hiding this comment.
Blocker: remote code execution and global pip installs.
Downloading and executing tdx.py from a remote URL at runtime, plus unconstrained global pip installs, are high‑risk (supply chain, reproducibility, network fragility). Use a vendored script and an isolated venv pinned to versions/hashes.
Apply:
-wget https://file.rdu.redhat.com/~berrange/tdx.py
-pip install ecdsa pyopenssl pyasn1_modules
-python tdx.py $report/outblob
+# Use vendored script and pinned deps
+# (example paths assuming this lives in qemu/deps/tdx/)
+script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+tdx_py="$script_dir/tdx.py"
+reqs="$script_dir/requirements.txt"
+venv_dir="${TMPDIR:-/tmp}/tdx-attest-venv"
+python3 -m venv "$venv_dir"
+"$venv_dir/bin/python" -m pip install --no-input --upgrade pip
+"$venv_dir/bin/python" -m pip install --no-input -r "$reqs"
+"$venv_dir/bin/python" "$tdx_py" "$report/outblob"Also pin package versions in requirements.txt and consider integrity verification (hash‑pinned installs).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In qemu/deps/tdx/attestation.sh around lines 6–8, do not wget and exec a remote
tdx.py or run global pip installs; instead vendor the tdx.py into the repo
(e.g., qemu/deps/tdx/tdx.py) and update the script to create and activate a
python virtualenv before installing dependencies; move dependencies to a
requirements.txt with pinned versions (and ideally hashes) and install them into
the venv via pip install --require-hashes -r requirements.txt; ensure the script
invokes the vendored tdx.py from the venv Python interpreter and remove any
global pip usage or network-executed code.
| if params.get_numeric("smp") > 1: | ||
| MemFree = float( | ||
| normalize_data_size("%s KB" % get_mem_info(attr="MemFree"), "M") | ||
| ) | ||
| vm_num = len(params.get("vms").split()) | ||
| params["mem"] = MemFree // (2 * vm_num) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Memory calc: confirm API and ensure integer MiB.
- get_mem_info(attr="MemFree") signature may differ; confirm correct import.
- params["mem"] becomes a float; pass an int MiB.
Apply:
- MemFree = float(
- normalize_data_size("%s KB" % get_mem_info(attr="MemFree"), "M")
- )
+ MemFree = int(float(normalize_data_size(f"{get_mem_info(attr='MemFree')} KB", "M")))
vm_num = len(params.get("vms").split())
- params["mem"] = MemFree // (2 * vm_num)
+ params["mem"] = int(MemFree // (2 * vm_num))Verify the function signature in your tree:
🏁 Script executed:
#!/bin/bash
rg -nC2 -P 'def\s+get_mem_info\s*\('Length of output: 338
🏁 Script executed:
#!/bin/bash
# Find all get_mem_info definitions in the codebase
rg -n 'def\s+get_mem_info' --type=py
# Also check imports in the file under review
echo "--- Imports in tdx_multi_vms.py ---"
head -30 qemu/tests/tdx_multi_vms.py | grep -E '^(import|from)'Length of output: 294
🏁 Script executed:
#!/bin/bash
# Get full import block from tdx_multi_vms.py
head -40 qemu/tests/tdx_multi_vms.py
# Check if get_mem_info is defined or imported in virttest modules
echo "--- Searching for get_mem_info in virttest ---"
rg -n 'get_mem_info' --type=py | grep -E '(virttest|def|import)' | head -20Length of output: 1611
🏁 Script executed:
#!/bin/bash
# Search for get_mem_info in virttest directory specifically
rg -n 'def\s+get_mem_info' --type=py | grep virttest
# Also try to find the virttest module path and check utils_misc
fd -t f 'utils_misc' 2>/dev/null | head -10
# Search with more context around get_mem_info definitions
rg -nB2 -A5 'def\s+get_mem_info\s*\(' --type=pyLength of output: 539
🌐 Web query:
virttest.utils_misc get_mem_info function signature
💡 Result:
Which virttest version do you need the get_mem_info signature for? (e.g. the ReadTheDocs release, a GitHub commit/tag, or your local install)
🏁 Script executed:
#!/bin/bash
# Verify the float division behavior and check if params["mem"] is expected to be int
# Search for how params["mem"] is used elsewhere
rg -n 'params\["mem"\]' --type=py -C2 | head -40
# Also check if there are other assignments to params["mem"] in this file
grep -n 'params\[.*mem.*\]' qemu/tests/tdx_multi_vms.pyLength of output: 2607
🏁 Script executed:
#!/bin/bash
# Check the context around snp_multi_vm.py lines 40-50 to verify identical pattern
sed -n '40,50p' qemu/tests/snp_multi_vm.py
# Also check if snp_multi_vm.py has any fixes or different approach
rg -n 'MemFree' qemu/tests/snp_multi_vm.py -B3 -A3Length of output: 856
Float division produces float instead of int; fix confirmed necessary.
The review comment correctly identifies a real issue. In Python, float // int returns a float, not an int. Since MemFree is explicitly cast to float (lines 50-52), the expression MemFree // (2 * vm_num) on line 55 produces a float. This is inconsistent with expected usage elsewhere in the codebase where params["mem"] is routinely cast to int before use (e.g., hugepage_reset.py:87, mlock_basic.py:20). The identical code pattern exists in snp_multi_vm.py:47.
Regarding get_mem_info(attr="MemFree"): The import is confirmed as from virttest.utils_misc import get_mem_info, and the keyword argument usage is consistent across multiple test files. The function signature cannot be verified from the repository alone, but the pattern suggests it is supported.
The suggested fix is mostly correct; however, int(float(...)) can be simplified to int(...) without the intermediate float conversion.
🤖 Prompt for AI Agents
In qemu/tests/tdx_multi_vms.py around lines 50-56, MemFree is cast to float
which makes the floor-division produce a float; replace the float conversion
with an integer conversion so MemFree becomes an int (e.g., use
int(normalize_data_size(..., "M"))) and keep params["mem"] = MemFree // (2 *
vm_num) to yield an int; apply the same change to the identical pattern in
snp_multi_vm.py at the corresponding line (around line 47).
c256bcb to
1df9a23
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
qemu/deps/tdx/attestation.sh (1)
1-8: Past critical issues remain unaddressed.The following critical security and reliability issues previously flagged in past reviews remain unresolved:
- Line 1: Missing shebang and insufficient error handling (only
set -e).- Lines 2-5: Unquoted variable expansions and
mkdirwithout-pflag, causing brittleness.- Lines 6-8: Remote code execution via
wget, globalpipinstalls, and execution of unverified script—a supply chain security risk.Please address the previously documented concerns before proceeding.
qemu/tests/tdx_multi_vms.py (3)
40-47: Guard against insufficient CPUs and verify VM count calculation.The previously flagged issue persists: if
max_smp < max_vms, the division on line 47 producessmp=0, causing VM preparation to fail.Additionally, verify the VM count logic:
range(1, max_vms)createsmax_vms - 1VMs, which when combined with existing VMs may not match the divisormax_vmson line 47.Apply this diff to add the CPU guard:
if max_vms_cmd: max_vms = int(process.system_output(max_vms_cmd, shell=True)) for i in range(1, max_vms): params["vms"] += " vm_%s" % i max_smp = int(process.system_output(params["max_vcpu_cmd"], shell=True)) + if max_smp < max_vms: + test.cancel(f"Not enough CPUs ({max_smp}) to split across {max_vms} VMs") params["smp"] = max_smp // max_vms
50-55: Memory calculation produces float instead of int.The previously flagged issue persists: since
MemFreeis cast tofloat(line 51), the floor division on line 55 produces afloatrather than anint, which is inconsistent with howparams["mem"]is used elsewhere in the codebase.Apply this diff:
if params.get_numeric("smp") > 1: - MemFree = float( - normalize_data_size("%s KB" % get_mem_info(attr="MemFree"), "M") - ) + MemFree = int(normalize_data_size("%s KB" % get_mem_info(attr="MemFree"), "M")) vm_num = len(params.get("vms").split()) - params["mem"] = MemFree // (2 * vm_num) + params["mem"] = int(MemFree // (2 * vm_num))
65-92: Undefined session variable in finally block.The previously flagged issue persists: if
wait_for_login()(line 68) raises an exception,sessionremains undefined, causing thefinallyblock (line 91) to fail when attemptingsession.close(), which masks the original error.Apply this diff:
for vm in vms_queue: vm.verify_alive() verify_dmesg() + session = None - session = vm.wait_for_login(timeout=timeout) - guest_check_cmd = params["tdx_guest_check"] try: + session = vm.wait_for_login(timeout=timeout) + guest_check_cmd = params["tdx_guest_check"] session.cmd_output(guest_check_cmd, timeout=240) except Exception as e: test.fail("Guest tdx verify fail: %s" % str(e)) else: # Verify attestation ... finally: - session.close() + if session: + session.close() vm.destroy()
🧹 Nitpick comments (1)
qemu/tests/cfg/tdx_multi_vms.cfg (1)
11-11: Consider making the BIOS path configurable.The hardcoded path
/usr/share/edk2/ovmf/OVMF.inteltdx.fdmay not exist on all systems. If tests need to run across different environments, consider making this configurable via an environment variable or allowing override from a separate config.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
qemu/deps/tdx/attestation.sh(1 hunks)qemu/tests/cfg/tdx_multi_vms.cfg(1 hunks)qemu/tests/tdx_multi_vms.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/tdx_multi_vms.py (1)
generic/tests/trans_hugepage_swapping.py (1)
get_mem_info(21-29)
🪛 Shellcheck (0.11.0)
qemu/deps/tdx/attestation.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🔇 Additional comments (2)
qemu/tests/tdx_multi_vms.py (2)
57-64: VM creation flow is well-structured.The preprocessing, retrieval, queuing, and creation sequence is clean and follows expected patterns.
67-67: Clarify the intent of verify_dmesg() call.The
verify_dmesg()call appears to check the host's dmesg, not the guest's. If the intent is to verify TDX-related messages in the guest, this should be run via the guest session after login. If checking host dmesg for each VM is intentional, consider moving it outside the loop or adding a comment to clarify.
qemu/tests/cfg/tdx_multi_vms.cfg
Outdated
| tdx_module_path = "/sys/module/kvm_intel/parameters/tdx" | ||
| module_status = Y y 1 | ||
| tdx_guest_check = "journalctl|grep -i -w tdx" | ||
| # guest_tool_install = "dnf install -y tdxguesddt" |
There was a problem hiding this comment.
Fix the typo in the commented package name.
The package name appears to contain a typo: tdxguesddt should likely be tdxguestddt or similar. Even though this line is commented out, correcting it ensures accuracy when it's eventually enabled.
Apply this diff:
-# guest_tool_install = "dnf install -y tdxguesddt"
+# guest_tool_install = "dnf install -y tdxguestddt"📝 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.
| # guest_tool_install = "dnf install -y tdxguesddt" | |
| # guest_tool_install = "dnf install -y tdxguestddt" |
🤖 Prompt for AI Agents
In qemu/tests/cfg/tdx_multi_vms.cfg around line 15, the commented package name
contains a typo `tdxguesddt`; update the comment to the correct package name
(e.g., `tdxguestddt`) so the line reads `# guest_tool_install = "dnf install
-y tdxguestddt"` to ensure accuracy when uncommented.
Signed-off-by: Zixi Chen <[email protected]>
zixi-chen
force-pushed
the
tdx_multi_vm
branch
from
1df9a23 to
289a5eb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
qemu/deps/tdx/attestation.sh (3)
1-1: Add shebang and stricter shell options.The script lacks a shebang and uses only
set -e. Add a bash shebang as the first line and consider stricter options likeset -euo pipefail.As per static analysis hints.
2-5: Harden configfs usage and quote variable expansions.The
mkdircommand will fail if the directory exists (combined withset -e), variable expansions are unquoted, and there's no verification that configfs/tsm is mounted.
6-8: Eliminate remote code execution and global pip installs.Downloading and executing a script from a remote URL introduces supply-chain risks, and global pip installs lack version control and isolation. Vendor the script into the repository and use a virtual environment with pinned dependencies.
qemu/tests/tdx_multi_vms.py (1)
64-91: Critical: Finally block will raise AttributeError if session is None.While
session = Noneis correctly initialized at line 67, the finally block at line 90 callssession.close()unconditionally. Ifwait_for_login()raises an exception,sessionremainsNoneandsession.close()will raiseAttributeError, masking the original error.Apply this diff:
for vm in vms_queue: vm.verify_alive() guest_check_cmd = params["tdx_guest_check"] session = None try: session = vm.wait_for_login(timeout=timeout) session.cmd_output(guest_check_cmd, timeout=240) except Exception as e: test.fail("Guest tdx verify fail: %s" % str(e)) else: # Verify attestation error_context.context("Start to do attestation", test.log.info) guest_dir = params["guest_dir"] host_script = params["host_script"] guest_cmd = params["guest_cmd"] deps_dir = virttest_data_dir.get_deps_dir() host_file = os.path.join(deps_dir, host_script) try: vm.copy_files_to(host_file, guest_dir) session.cmd_output("chmod 755 %s" % guest_cmd) except Exception as e: test.fail("Guest test preparation fail: %s" % str(e)) s = session.cmd_status(guest_cmd, timeout=360) if s: test.fail("Guest script error") finally: - session.close() + if session: + session.close() vm.destroy()
🧹 Nitpick comments (1)
qemu/tests/tdx_multi_vms.py (1)
49-54: Consider converting MemFree to int directly.While the current code correctly produces an integer for
params["mem"], convertingMemFreeto int directly would be cleaner and avoid the intermediate float.Apply this diff:
if params.get_numeric("smp") > 1: - MemFree = float( + MemFree = int( normalize_data_size("%s KB" % get_mem_info(attr="MemFree"), "M") ) vm_num = len(params.get("vms").split()) - params["mem"] = int(MemFree // (2 * vm_num)) + params["mem"] = MemFree // (2 * vm_num)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
qemu/deps/tdx/attestation.sh(1 hunks)qemu/tests/cfg/tdx_multi_vms.cfg(1 hunks)qemu/tests/tdx_multi_vms.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/cfg/tdx_multi_vms.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/tdx_multi_vms.py (1)
generic/tests/trans_hugepage_swapping.py (1)
get_mem_info(21-29)
🪛 Shellcheck (0.11.0)
qemu/deps/tdx/attestation.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🔇 Additional comments (3)
qemu/tests/tdx_multi_vms.py (3)
12-28: LGTM: Function structure and initial setup.The test function is properly decorated with
@error_context.context_aware, includes clear documentation, and correctly identifies itself as a TDX test.
30-37: LGTM: Host capability check.The TDX module presence and status verification correctly uses
test.cancelwhen host support is missing.
56-62: LGTM: VM preparation and creation.The VM preprocessing and creation loop follows the standard pattern for multi-VM scenarios.
|
@XueqiangWei @PaulYuuu Could you please review the patch? Test result is updated in the ticket. |
2 participants
ID: 4345
Summary by CodeRabbit
New Features
Tests