Conversation
|
this pr depends on avocado-framework/avocado-vt#4232 |
bssrikanth
force-pushed
the
idle_hlt
branch
from
88cd747 to
bda5022
Compare
|
Run results from AMD EPYC Turin machine: Run results on unsupported platform: |
bssrikanth
force-pushed
the
idle_hlt
branch
from
bda5022 to
f0211ba
Compare
|
Hello, if anyone has any review comments on this test please feel free to comment. Happy to address any review comments |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new Avocado QEMU test: a config entry at qemu/tests/cfg/idlehlt.cfg defining VM options, nested variants for baseline/cvm and SEV families (sev, seves, snp) with per-variant QEMU requirements and SEV parameters; and a new test implementation qemu/tests/idlehlt.py. The Python test ensures cpuid is available (building/installing if needed), verifies host/guest SEV settings, configures ftrace to capture kvm_exit events filtered by exit_reason, boots and logs into the VM, collects/parses trace output for idle-halt events, and performs guarded cleanup. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (2)
qemu/tests/idlehlt.py (2)
137-138: Exception handler forCmdErroris unreachable.Since
process.run()is called withignore_status=Trueon line 122, it will never raiseCmdError. The exception handler at lines 137-138 is dead code.- except process.CmdError as e: - test.cancel("Error executing cpuid: {}".format(e))
164-165: Broad exception handling obscures specific failures.Catching generic
Exceptionand converting totest.failloses the original stack trace and makes debugging harder.Consider letting specific exceptions propagate or logging the full traceback:
except Exception as e: - test.fail("Test failed: %s" % str(e)) + test.log.error("Test failed with exception", exc_info=True) + test.fail("Test failed: %s" % str(e))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/idlehlt.cfg(1 hunks)qemu/tests/idlehlt.py(1 hunks)
🧰 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/idlehlt.cfg
🔇 Additional comments (5)
qemu/tests/cfg/idlehlt.cfg (3)
1-17: Configuration looks reasonable for the test setup.The base configuration appropriately restricts to Linux on AMD hosts, sets reasonable VM resources (8GB RAM, 8 CPUs), and configures necessary virtio/IOMMU options for secure guest support.
35-41: SNP variant configuration is well-structured.The SNP variant correctly specifies newer QEMU requirements (9.1.0+), appropriate module path, and memory backend configuration for SNP support.
29-34: No changes needed. The configuration is correct. SEV-ES variants usevm_secure_guest_type = sev(distinguished viavm_sev_policy = 7), consistent with all SEV-ES configurations in the codebase. Thecvm_guest_checksearching for "sev-es" validates the OS-level guest state, not the QEMU type parameter.Likely an incorrect or invalid review comment.
qemu/tests/idlehlt.py (2)
23-44: Setup function is well-structured.The ftrace configuration logic properly checks for directory existence, uses context managers for file operations, and handles relevant exceptions.
72-73:make installmay require elevated privileges.The
build.make(sourcedir, extra_args="install", ...)call runsmake install, which typically requires root permissions to install to system directories. The comment mentions "use sudo for make install" but the code doesn't actually use sudo.Consider installing to a local directory or verifying the test runs with appropriate privileges:
- # Build and install (use sudo for make install) - build.make(sourcedir, extra_args="install", ignore_status=False) + # Build and install to local prefix + build.make(sourcedir, extra_args=f"PREFIX={test.teststmpdir} install", ignore_status=False)
qemu/tests/idlehlt.py
Outdated
| if params.get("vm_secure_guest_type"): | ||
| secure_guest_type = params.get("vm_secure_guest_type") | ||
| supported_secureguest = ["sev", "sev-es", "snp"] | ||
| if secure_guest_type not in supported_secureguest: | ||
| test.cancel( | ||
| "Testcase does not support vm_secure_guest_type %s" % secure_guest_type | ||
| ) | ||
| # Check host kernel sev support | ||
| verify_sev_host(params) |
There was a problem hiding this comment.
Variable secure_guest_type scope issue causes bug at line 151.
The variable secure_guest_type is defined conditionally inside the if block (line 97), but line 151 checks if "secure_guest_type" in locals(). If the if block on line 96 is not entered, secure_guest_type won't exist, but even if entered, the variable name in the check should match.
Initialize the variable before the conditional to ensure consistent scope:
+ secure_guest_type = params.get("vm_secure_guest_type")
- if params.get("vm_secure_guest_type"):
- secure_guest_type = params.get("vm_secure_guest_type")
+ if secure_guest_type:
supported_secureguest = ["sev", "sev-es", "snp"]📝 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.
| if params.get("vm_secure_guest_type"): | |
| secure_guest_type = params.get("vm_secure_guest_type") | |
| supported_secureguest = ["sev", "sev-es", "snp"] | |
| if secure_guest_type not in supported_secureguest: | |
| test.cancel( | |
| "Testcase does not support vm_secure_guest_type %s" % secure_guest_type | |
| ) | |
| # Check host kernel sev support | |
| verify_sev_host(params) | |
| secure_guest_type = params.get("vm_secure_guest_type") | |
| if secure_guest_type: | |
| supported_secureguest = ["sev", "sev-es", "snp"] | |
| if secure_guest_type not in supported_secureguest: | |
| test.cancel( | |
| "Testcase does not support vm_secure_guest_type %s" % secure_guest_type | |
| ) | |
| # Check host kernel sev support | |
| verify_sev_host(params) |
🤖 Prompt for AI Agents
In qemu/tests/idlehlt.py around lines 96 to 104 and the later check at line 151,
secure_guest_type is only set inside a conditional which causes scope/lookup
issues; declare secure_guest_type = None immediately before the if
params.get("vm_secure_guest_type") block so it always exists, and replace the
later locals() check with an explicit check like if secure_guest_type is not
None (or if secure_guest_type:) to reliably detect whether it was set.
| if "session" in locals() and session: | ||
| session.close() | ||
| vm.destroy() |
There was a problem hiding this comment.
VM may not be destroyed if login fails.
The cleanup only destroys the VM if session exists. If vm.create() succeeds but vm.wait_for_login() fails, the VM remains running.
if "session" in locals() and session:
session.close()
- vm.destroy()
+ if "vm" in locals() and vm:
+ vm.destroy()📝 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.
| if "session" in locals() and session: | |
| session.close() | |
| vm.destroy() | |
| if "session" in locals() and session: | |
| session.close() | |
| if "vm" in locals() and vm: | |
| vm.destroy() |
🤖 Prompt for AI Agents
In qemu/tests/idlehlt.py around lines 184 to 186, the cleanup only calls
vm.destroy() when a successful session exists, leaving the VM running if
vm.create() succeeded but vm.wait_for_login() failed; change the cleanup so
session.close() is called only if session exists and vm.destroy() is always
attempted if the VM object was created (e.g., check "if 'vm' in locals() and
vm:" or use a finally block), and wrap both close/destroy calls in try/except to
avoid masking errors during teardown.
c4228ce to
a69cf3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
qemu/tests/idlehlt.py (2)
96-104: Previous review comment not addressed: Variable scope issue forsecure_guest_type.The variable
secure_guest_typeis defined conditionally inside theifblock (line 97), creating a scope dependency that requires checking"secure_guest_type" in locals()at line 151. This pattern is error-prone and makes the code harder to reason about.Initialize
secure_guest_typebefore the conditional block:+ secure_guest_type = params.get("vm_secure_guest_type") - if params.get("vm_secure_guest_type"): - secure_guest_type = params.get("vm_secure_guest_type") + if secure_guest_type: supported_secureguest = ["sev", "sev-es", "snp"] if secure_guest_type not in supported_secureguest: test.cancel( "Testcase does not support vm_secure_guest_type %s" % secure_guest_type ) # Check host kernel sev support verify_sev_host(params)Then at line 151, simplify the check to:
- if "secure_guest_type" in locals() and secure_guest_type: + if secure_guest_type: verify_sev_guest(session, params, vm)
184-186: Previous review comment not addressed: VM cleanup is incomplete.The VM is only destroyed if
sessionexists (line 184). Ifvm.create()succeeds butvm.wait_for_login()fails, the VM process will be left running, causing a resource leak.Separate the session and VM cleanup logic:
if "session" in locals() and session: session.close() - vm.destroy() + if "vm" in locals() and vm: + vm.destroy()Better yet, wrap each cleanup operation in its own try-except to ensure one failure doesn't prevent the other:
if "session" in locals() and session: - session.close() - vm.destroy() + try: + session.close() + except Exception as e: + test.log.warning("Failed to close session: %s", e) + if "vm" in locals() and vm: + try: + vm.destroy() + except Exception as e: + test.log.warning("Failed to destroy VM: %s", e)
🧹 Nitpick comments (2)
qemu/tests/idlehlt.py (2)
153-153: Consider making the sleep duration configurable.The 5-second sleep is hard-coded. Consider making it a test parameter to allow tuning based on system performance or documenting why 5 seconds is the appropriate wait time for idle-halt events to occur.
+ idle_wait_time = params.get_numeric("idle_wait_time", 5) verify_dmesg() if "secure_guest_type" in locals() and secure_guest_type: verify_sev_guest(session, params, vm) - time.sleep(5) + time.sleep(idle_wait_time)
164-165: Broad exception handler masks specific errors.Catching
Exceptionis overly broad and can mask specific errors (like AttributeError, TypeError, etc.) that might indicate bugs. Consider catching only expected exceptions or letting unexpected errors propagate for better debugging.- except Exception as e: - test.fail("Test failed: %s" % str(e)) + except (process.CmdError, IOError, OSError) as e: + test.fail("Test failed: %s" % str(e))Alternatively, if you need to catch all exceptions for test reporting, at least log the full traceback:
except Exception as e: + test.log.error("Test failed with exception: %s", e, exc_info=True) test.fail("Test failed: %s" % str(e))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/idlehlt.cfg(1 hunks)qemu/tests/idlehlt.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/cfg/idlehlt.cfg
🧰 Additional context used
🧠 Learnings (1)
📓 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.
| test.log.info("Extracted cpuid source to %s", sourcedir) | ||
|
|
||
| # Build and install (use sudo for make install) | ||
| build.make(sourcedir, extra_args="install", ignore_status=False) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine qemu/tests/idlehlt.py, focusing on the cpuid_tool_build function
fd -n idlehlt.py | head -5Repository: autotest/tp-qemu
Length of output: 290
🏁 Script executed:
# Find idlehlt.py file
fd idlehlt.pyRepository: autotest/tp-qemu
Length of output: 81
🏁 Script executed:
# Examine the cpuid_tool_build function around line 73
cat -n qemu/tests/idlehlt.py | sed -n '46,94p'Repository: autotest/tp-qemu
Length of output: 2443
🏁 Script executed:
# Search for build.make implementation and how it handles make commands
rg -A 10 "def make" --type py | head -40Repository: autotest/tp-qemu
Length of output: 2532
🏁 Script executed:
# Check imports in idlehlt.py to understand where build module comes from
head -20 qemu/tests/idlehlt.pyRepository: autotest/tp-qemu
Length of output: 703
🏁 Script executed:
# Search for other uses of build.make in the codebase to see if sudo is used elsewhere
rg "build\.make" --type py -B 2 -A 2 | head -60Repository: autotest/tp-qemu
Length of output: 701
🏁 Script executed:
# Search for any documentation or comments about build.make and sudo usage
rg -B 5 -A 5 "sudo.*make|make.*sudo" qemu/tests/ --type pyRepository: autotest/tp-qemu
Length of output: 863
🏁 Script executed:
# Look for any PREFIX or DESTDIR related patterns in build or make usage
rg "PREFIX|DESTDIR|--prefix" qemu/tests/ --type pyRepository: autotest/tp-qemu
Length of output: 287
🏁 Script executed:
# Check if there's documentation about build.make with sudo or privilege handling
rg -B 3 -A 3 "make install" qemu/tests/ --type py -C 5Repository: autotest/tp-qemu
Length of output: 2537
🏁 Script executed:
# Check the cpuid tool's Makefile to see if it supports PREFIX or --prefix
# Download and extract the cpuid source to examine its Makefile
python3 << 'EOF'
import urllib.request
import tarfile
import os
import tempfile
# Create temp directory
tmpdir = tempfile.mkdtemp()
# Download cpuid source (using URL from test)
url = "http://www.etallen.com/cpuid/cpuid-20250513.src.tar.gz"
tarball = os.path.join(tmpdir, "cpuid.tar.gz")
try:
print(f"Downloading cpuid source from {url}...")
urllib.request.urlretrieve(url, tarball)
# Extract
print(f"Extracting to {tmpdir}...")
with tarfile.open(tarball, 'r:gz') as tar:
tar.extractall(tmpdir)
# Find and read Makefile
for root, dirs, files in os.walk(tmpdir):
if 'Makefile' in files:
makefile_path = os.path.join(root, 'Makefile')
print(f"\n=== Found Makefile at {makefile_path} ===\n")
with open(makefile_path, 'r') as f:
content = f.read()
# Look for install target and PREFIX/DESTDIR
lines = content.split('\n')
for i, line in enumerate(lines):
if 'install:' in line or 'PREFIX' in line or 'DESTDIR' in line or 'bindir' in line:
start = max(0, i-2)
end = min(len(lines), i+10)
print('\n'.join(lines[start:end]))
print("---")
break
except Exception as e:
print(f"Error: {e}")
finally:
# Cleanup
import shutil
shutil.rmtree(tmpdir, ignore_errors=True)
EOFRepository: autotest/tp-qemu
Length of output: 146
🏁 Script executed:
# Check if there are any recent changes or TODOs related to cpuid installation
rg -i "TODO|FIXME|XXX" qemu/tests/idlehlt.pyRepository: autotest/tp-qemu
Length of output: 42
🏁 Script executed:
# Verify the exact behavior by checking build.make documentation
rg -B 10 "def make" /dev/null 2>/dev/null || find . -name "*.py" -exec grep -l "from avocado.utils import.*build" {} \; | head -3Repository: autotest/tp-qemu
Length of output: 105
🏁 Script executed:
# Double-check: look at the full cpuid_tool_build function and surrounding context
cat -n qemu/tests/idlehlt.py | sed -n '40,100p'Repository: autotest/tp-qemu
Length of output: 3017
Remove comment mismatch or implement privilege elevation for make install.
Line 72 contains a comment stating "use sudo for make install", but line 73 calls build.make(...) without any privilege elevation. This will fail with permission denied errors when the test runs without root privileges. Either remove the comment or implement the suggested approach:
- Use
--prefix=$HOME/.localto install to user directory (preferred, consistent with other tests in the suite) - Add sudo/privilege elevation with documentation of root requirement
- Use
build.make(sourcedir, extra_args="install --prefix=$HOME/.local")or similar
🤖 Prompt for AI Agents
In qemu/tests/idlehlt.py around line 73, the code calls build.make(sourcedir,
extra_args="install") while the comment on the previous line says "use sudo for
make install"; update the implementation to match the comment policy by
installing into the user directory: change the install invocation to include a
user prefix (e.g. use extra_args="install --prefix=$HOME/.local") so no root is
required, or alternatively remove the misleading comment and keep current
behavior if root is guaranteed — prefer the --prefix approach to remain
consistent with other tests.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
qemu/tests/idlehlt.py (3)
72-73: Installation will fail without root privileges.The comment mentions using sudo for
make install, but the implementation callsbuild.make()without privilege elevation. This will fail when installing to system directories.As suggested in previous reviews, install to a user directory instead:
- # Build and install (use sudo for make install) - build.make(sourcedir, extra_args="install", ignore_status=False) + # Build and install to user directory + build.make(sourcedir, extra_args="install --prefix=$HOME/.local", ignore_status=False)
96-104: Variable scope issue causes unreliable check at line 151.The variable
secure_guest_typeis only defined inside this conditional block, but line 151 checks for its existence usinglocals(). Initialize it before the conditional to ensure it always exists.Apply this fix:
+ secure_guest_type = params.get("vm_secure_guest_type") - if params.get("vm_secure_guest_type"): - secure_guest_type = params.get("vm_secure_guest_type") + if secure_guest_type: supported_secureguest = ["sev", "sev-es", "snp"]
184-186: VM may leak if login fails.The cleanup only calls
vm.destroy()when a session exists. Ifvm.create()succeeds butvm.wait_for_login()fails, the VM remains running.Fix the cleanup logic:
if "session" in locals() and session: session.close() - vm.destroy() + if "vm" in locals() and vm: + try: + vm.destroy() + except Exception as e: + test.log.warning("Failed to destroy VM: %s", e)
🧹 Nitpick comments (1)
qemu/tests/idlehlt.py (1)
153-153: Consider making the idle wait time configurable.The hard-coded 5-second sleep allows the system to idle before capturing trace data. Consider making this configurable via test parameters for flexibility in different environments.
+ idle_wait_time = params.get_numeric("idle_wait_time", 5) - time.sleep(5) + time.sleep(idle_wait_time)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/idlehlt.cfg(1 hunks)qemu/tests/idlehlt.py(1 hunks)
🧰 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/idlehlt.cfg
🔇 Additional comments (9)
qemu/tests/idlehlt.py (5)
1-21: LGTM!The imports are appropriate for this test's requirements, and the function signature follows the standard Avocado QEMU test pattern.
23-44: LGTM!The ftrace setup is well-structured with proper error handling and appropriate use of
test.cancel()for infrastructure issues.
105-116: LGTM!Parameter handling is correct with sensible defaults, and the conditional build of the cpuid tool is well-implemented.
118-138: LGTM!The CPUID feature detection correctly checks bit 30 of EDX from leaf 0x8000000A with proper error handling and clear cancellation messages.
150-150: The usage ofverify_dmesg()is correct.The function is imported from
virttest.utils_miscand is consistently called without parameters throughout the codebase in 20+ test files (migration_with_sgx.py, sgx_cpu.py, snp_basic_config.py, sev_basic_config.py, and many others). This uniform pattern across the entire test suite confirms thatverify_dmesg()is designed to be invoked without arguments. The original review comment questioning this usage is unfounded.qemu/tests/cfg/idlehlt.cfg (4)
18-21: LGTM!The variant structure clearly separates non-secure (svm) from confidential VM (cvm) configurations, with appropriate SEV physical bits reduction for CVM variants.
23-28: LGTM!The SEV variant configuration is correct with appropriate QEMU version requirement (2.12+), module path, guest check, and policy value.
35-41: LGTM!The SNP variant configuration is correct with appropriate QEMU version requirement (9.1.0+), module path, guest check, policy value, and memory backend.
29-34: The configuration at lines 29-34 is actually correct, not inconsistent. SEV-ES is enabled if SEV is enabled and the SEV-ES policy bit is set in the policy object. Settingvm_secure_guest_type = sevwithvm_sev_policy = 7properly enables SEV-ES because SEV-ES is enabled when policy & 0x4 != 0, and 7 (binary 0111) has bit 2 set. The module path and guest check references to "sev_es" and "sev-es" are string literals for checking host support—they do not indicate the config value should be "sev-es".However, the actual issue is in idlehlt.py line 98: the
supported_secureguestlist includes "sev-es", but tp-qemu's configuration framework never uses "sev-es" as avm_secure_guest_typevalue. Per standard QEMU behavior, only "sev" and "snp" are valid guest types; SEV-ES is a variant controlled via policy bits. Line 98 should be corrected to:supported_secureguest = ["sev", "snp"](remove "sev-es").Likely an incorrect or invalid review comment.
| trace_dir = "/sys/kernel/tracing" | ||
| hlt_exit_reason = "0x0a6" | ||
| url_cpuid_tool = "http://www.etallen.com/cpuid/cpuid-20250513.src.tar.gz" | ||
| bios_path = /usr/share/ovmf/OVMF.fd |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for bios_path usage across cfg files
echo "=== Searching for bios_path usage in test configs ==="
find qemu/tests/cfg -name "*.cfg" -type f | head -20
echo -e "\n=== Checking how bios_path is used in idlehlt.cfg and similar configs ==="
rg -n 'bios_path' qemu/tests/cfg/ --type cfg -B 2 -A 2
echo -e "\n=== Checking base.cfg for bios_path default or fallback ==="
if [ -f qemu/cfg/base.cfg ]; then
rg -n 'bios' qemu/cfg/base.cfg | head -20
else
echo "qemu/cfg/base.cfg not found"
fi
echo -e "\n=== Looking for OVMF path patterns in all configs ==="
rg -n 'ovmf|OVMF' qemu/cfg/ --type cfg | head -30Repository: autotest/tp-qemu
Length of output: 1077
🏁 Script executed:
#!/bin/bash
# Search for bios_path in all config files without using --type flag
echo "=== Searching for bios_path across entire repository ==="
grep -r 'bios_path' qemu/ 2>/dev/null | head -40
echo -e "\n=== Checking idlehlt.cfg content ==="
cat qemu/tests/cfg/idlehlt.cfg 2>/dev/null
echo -e "\n=== Checking for OVMF paths in configs ==="
grep -r 'ovmf\|OVMF\|\.fd' qemu/cfg/ qemu/tests/cfg/ 2>/dev/null | head -30
echo -e "\n=== Check if there's a base configuration ==="
ls -la qemu/cfg/ 2>/dev/null | head -20Repository: autotest/tp-qemu
Length of output: 4301
🏁 Script executed:
#!/bin/bash
# Check if there's any path resolution or fallback in the framework code
echo "=== Checking for path resolution in framework code ==="
find qemu -type f -name "*.py" | xargs grep -l 'bios_path' 2>/dev/null | head -10
echo -e "\n=== Look for any path fallback or existence checking ==="
grep -r 'bios_path\|OVMF' qemu --include="*.py" 2>/dev/null | grep -i 'fallback\|exist\|check\|path' | head -20
echo -e "\n=== Check if idlehlt test is special or part of security test suite ==="
head -30 qemu/tests/cfg/idlehlt.cfgRepository: autotest/tp-qemu
Length of output: 1754
Update hard-coded BIOS path to match modern distribution layouts.
The path /usr/share/ovmf/OVMF.fd is outdated and distribution-specific. Modern systems use different paths: Ubuntu 24.04 has /usr/share/OVMF/ and other distributions use /usr/share/edk2/ovmf/. Additionally, other tests in this same repository use /usr/share/edk2/ovmf/OVMF.amdsev.fd, indicating this config is inconsistent. Update to a more portable path or add fallback logic to support multiple common locations.
🤖 Prompt for AI Agents
In qemu/tests/cfg/idlehlt.cfg around line 16, the BIOS path is hard-coded to
/usr/share/ovmf/OVMF.fd which is distribution-specific; change the config to use
a portable location or implement fallback resolution: update the setting to
reference the more common OVMF.amdsev.fd (e.g.
/usr/share/edk2/ovmf/OVMF.amdsev.fd) or replace the single path with logic (or a
variable) that checks multiple common locations in order
(/usr/share/edk2/ovmf/OVMF.amdsev.fd, /usr/share/OVMF/OVMF.fd,
/usr/share/ovmf/OVMF.fd) and selects the first existing file, falling back to a
clear error if none found.
|
Results: |
|
@YongxueHong , @zhencliu , @nickzhq request your review comments. Thank you for your time. |
|
Made changes to this testcase aligning to the review comments received on dependent avocado-vt PR avocado-framework/avocado-vt#4232. Run results: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
qemu/tests/idlehlt.py (4)
67-67: Make tarball name parsing more robust.The filename parsing with
.split(".src.tar.")assumes a specific naming pattern. If the tarball name doesn't contain ".src.tar.", this will fail silently or produce unexpected results.🔎 More robust parsing approach
- source_dir_name = os.path.basename(tarball).split(".src.tar.")[0] + # Extract using tar's logic, then find the actual directory created + archive.extract(tarball, test.teststmpdir) + # The extracted directory is typically the tarball name without extensions + import tarfile + with tarfile.open(tarball, 'r:gz') as tar: + # Get the top-level directory from the first member + first_member = tar.getmembers()[0] + source_dir_name = first_member.name.split('/')[0] sourcedir = os.path.join(test.teststmpdir, source_dir_name) - archive.extract(tarball, test.teststmpdir)Alternatively, use a more flexible pattern:
- source_dir_name = os.path.basename(tarball).split(".src.tar.")[0] + # Remove common archive extensions + source_dir_name = os.path.basename(tarball) + for ext in ['.tar.gz', '.src.tar.gz', '.tgz']: + if source_dir_name.endswith(ext): + source_dir_name = source_dir_name[:-len(ext)] + break sourcedir = os.path.join(test.teststmpdir, source_dir_name)
82-82: Consider removingshell=Truefor security.Lines 82 and 122 use
shell=Truewithprocess.run(). While the commands are currently controlled strings, usingshell=False(the default) with a list of arguments is more secure and avoids potential shell injection if these commands are ever modified to include user input.🔎 Safer alternatives
- result = process.run("cpuid --version", shell=True, ignore_status=True) + result = process.run(["cpuid", "--version"], ignore_status=True)- result = process.run( - "cpuid -1 -r -l 0x8000000A", shell=True, ignore_status=True - ) + result = process.run( + ["cpuid", "-1", "-r", "-l", "0x8000000A"], ignore_status=True + )Also applies to: 122-122
153-153: Consider making the sleep duration configurable.The hard-coded 5-second sleep might not be optimal for all systems. Faster systems might need less time, while slower or heavily loaded systems might need more time to generate detectable idle-halt events.
🔎 Suggested enhancement
Add a parameter to the config file:
# In qemu/tests/cfg/idlehlt.cfg + idle_wait_time = 5Then use it in the test:
- time.sleep(5) + idle_wait_time = params.get_numeric("idle_wait_time", 5) + test.log.info("Waiting %s seconds for idle-halt events...", idle_wait_time) + time.sleep(idle_wait_time)
158-159: Enhance trace output validation.The simple substring check
"idle-halt" not in trace_outputmay produce false negatives if the trace format varies or uses different capitalization. Consider making the check more robust.🔎 More robust validation
- if "idle-halt" not in trace_output: + # Check for idle-halt events more robustly + idle_halt_pattern = re.compile(r'idle[_-]halt', re.IGNORECASE) + if not idle_halt_pattern.search(trace_output): test.fail("No idle-halt exits detected in ftrace output.") else: + # Count the number of idle-halt events + event_count = len(idle_halt_pattern.findall(trace_output)) test.log.info( - "Idle-halt exits detected in ftrace output:\n%s", trace_output + "Detected %d idle-halt exit(s) in ftrace output.", event_count ) + test.log.debug("Full trace output:\n%s", trace_output)qemu/tests/cfg/idlehlt.cfg (1)
15-15: Change the cpuid tool URL to HTTPS.The resource is available over HTTPS at
https://www.etallen.com/cpuid/cpuid-20250513.src.tar.gz, which prevents potential man-in-the-middle attacks during download.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/idlehlt.cfgqemu/tests/idlehlt.py
🧰 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/idlehlt.pyqemu/tests/cfg/idlehlt.cfg
🔇 Additional comments (12)
qemu/tests/cfg/idlehlt.cfg (5)
1-12: LGTM!The basic test configuration is well-structured with appropriate platform restrictions (Linux on AMD hosts), resource allocation (8GB RAM, 8 vCPUs), and Virtio IOMMU settings suitable for secure guest testing.
18-22: LGTM!The variant structure cleanly separates baseline SVM testing from confidential VM (cvm) variants. The
vm_sev_reduced_phys_bits = 1setting is appropriate for SEV-based secure guests.
23-28: LGTM!The SEV variant configuration is correct with appropriate QEMU version requirement (2.12+), kernel module path, guest verification command, and policy value (3 = NoDebug + NoKeySharing).
29-34: LGTM!The SEV-ES variant correctly uses
vm_secure_guest_type = sevwith policy 7 (adds bit 2 for ES: NoDebug + NoKeySharing + ES). This properly differentiates SEV-ES from basic SEV via the policy setting rather than a separate guest type.
35-41: LGTM!The SNP variant configuration is correct with QEMU 9.1.0+ requirement, separate SNP module path, appropriate policy value (196608 = 0x30000 for SNP-specific bits), and the required
memory-backend-memfdbackend.qemu/tests/idlehlt.py (7)
1-10: LGTM!The imports are well-organized and all necessary dependencies are included for ftrace management, cpuid tool building, VM control, and secure guest verification.
11-22: LGTM!The function signature follows the standard Avocado test pattern, and the docstring clearly documents the test's purpose and parameters.
23-44: LGTM!The ftrace setup function correctly configures kernel tracing with proper sequencing (disable → clear → enable event → set filter) and appropriate error handling that cancels the test if ftrace is unavailable or permissions are insufficient.
105-117: LGTM!The environment setup properly retrieves test parameters with appropriate defaults and handles the cpuid tool prerequisite by building from source if not already installed.
118-138: LGTM!The CPUID feature detection correctly queries leaf 0x8000000A, parses the EDX register, and validates bit 30 for Idle HLT Intercept support. The test appropriately cancels if the feature is unavailable.
167-183: LGTM!The ftrace cleanup logic in the finally block is well-structured, properly resetting all tracing components (tracing_on, kvm_exit event, filter, trace buffer) and gracefully handling cleanup errors with warnings rather than failures.
150-150: The functionverify_dmesg()is properly imported at the top of the file fromvirttest.utils_misc(line 7) and called correctly without arguments at line 150. This usage pattern is consistent across multiple test files in the codebase (e.g.,sgx_cpu.py,sev_basic_config.py,snp_basic_config.py), confirming that the function signature supports being called with no arguments.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@qemu/tests/idlehlt.py`:
- Around line 1-8: The module references os.path.exists, os.path.join and
os.path.basename but never imports the os module, causing NameError at runtime;
fix by adding "import os" to the top-level imports in idlehlt.py so all
references to os.path.* resolve correctly (look for usages of os.path.exists,
os.path.join, os.path.basename to verify there are no other missing imports).
Introduces a test case to verify the Idle HLT Intercept feature in QEMU, using cpuid to check support and ftrace to monitor idle-halt exits. Supports secure guest types (SEV, SEV-ES, SNP) with configurable parameters. Signed-off-by: Srikanth Aithal <srikanth.aithal@amd.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@qemu/tests/cfg/idlehlt.cfg`:
- Line 15: Update the cpuid download URL constant url_cpuid_tool to use HTTPS
instead of HTTP so the tool is fetched securely; locate the declaration of
url_cpuid_tool and replace
"http://www.etallen.com/cpuid/cpuid-20250513.src.tar.gz" with
"https://www.etallen.com/cpuid/cpuid-20250513.src.tar.gz".
In `@qemu/tests/idlehlt.py`:
- Around line 123-126: Update the cpuid download to use HTTPS and add checksum
verification: change the default in params.get for url_cpuid_tool to the HTTPS
URL (https://www.etallen.com/cpuid/cpuid-20250513.src.tar.gz), add a new param
(e.g., asset_hash_cpuid_tool) with the expected hash as its default, and pass
that value into test.fetch_asset via its asset_hash parameter when fetching the
cpuid archive (references: url_cpuid_tool, params.get, and the call to
test.fetch_asset).
| virtio_dev_disable_legacy = on | ||
| trace_dir = "/sys/kernel/tracing" | ||
| hlt_exit_reason = "0x0a6" | ||
| url_cpuid_tool = "http://www.etallen.com/cpuid/cpuid-20250513.src.tar.gz" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Verify the file exists and check its contents
find . -name "idlehlt.cfg" -type f 2>/dev/null | head -5Repository: autotest/tp-qemu
Length of output: 88
🏁 Script executed:
# If file exists, check the exact content around line 15
if [ -f "qemu/tests/cfg/idlehlt.cfg" ]; then
sed -n '10,20p' qemu/tests/cfg/idlehlt.cfg
fiRepository: autotest/tp-qemu
Length of output: 397
🌐 Web query:
www.etallen.com cpuid HTTPS support
💡 Result:
etallen.com / www.etallen.com does support HTTPS for the cpuid project page and downloads.
- The cpuid homepage is available at
https://www.etallen.com/cpuid.html(also works withoutwwwashttps://etallen.com/cpuid.html). [1] - Accessing the page over plain HTTP (
http://www.etallen.com/cpuid.htmlorhttp://etallen.com/cpuid.html) resolves to the HTTPS version (i.e., it effectively redirects/serves as HTTPS). [1][2] - The cpuid release files linked from the page use HTTPS URLs (e.g., under
https://www.etallen.com/cpuid/). [1]
Sources: [1] [2]
Switch to HTTPS for the cpuid tool download.
The host www.etallen.com fully supports HTTPS, and the cpuid releases are served securely. Update the URL to prevent potential MITM tampering over HTTP.
Fix
- url_cpuid_tool = "http://www.etallen.com/cpuid/cpuid-20250513.src.tar.gz"
+ url_cpuid_tool = "https://www.etallen.com/cpuid/cpuid-20250513.src.tar.gz"📝 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.
| url_cpuid_tool = "http://www.etallen.com/cpuid/cpuid-20250513.src.tar.gz" | |
| url_cpuid_tool = "https://www.etallen.com/cpuid/cpuid-20250513.src.tar.gz" |
🤖 Prompt for AI Agents
In `@qemu/tests/cfg/idlehlt.cfg` at line 15, Update the cpuid download URL
constant url_cpuid_tool to use HTTPS instead of HTTP so the tool is fetched
securely; locate the declaration of url_cpuid_tool and replace
"http://www.etallen.com/cpuid/cpuid-20250513.src.tar.gz" with
"https://www.etallen.com/cpuid/cpuid-20250513.src.tar.gz".
| url_cpuid_tool = params.get( | ||
| "url_cpuid_tool", | ||
| default="http://www.etallen.com/cpuid/cpuid-20250513.src.tar.gz", | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and the specific code
head -n 150 qemu/tests/idlehlt.py | tail -n 40Repository: autotest/tp-qemu
Length of output: 1726
🏁 Script executed:
# Check the full context around lines 123-126 and look for checksum/integrity patterns
cat -n qemu/tests/idlehlt.py | sed -n '100,150p'Repository: autotest/tp-qemu
Length of output: 2529
🏁 Script executed:
# Search for checksum, integrity, or verification patterns in the file
rg -i "checksum|sha|md5|verify|integrity|hash" qemu/tests/idlehlt.pyRepository: autotest/tp-qemu
Length of output: 386
🏁 Script executed:
# Search for how the cpuid_tool is used (downloaded, executed, etc.)
rg -A 10 "url_cpuid_tool" qemu/tests/idlehlt.pyRepository: autotest/tp-qemu
Length of output: 1869
🏁 Script executed:
# Search for cpuid_tool_build function definition
rg -A 30 "def cpuid_tool_build" qemu/tests/idlehlt.pyRepository: autotest/tp-qemu
Length of output: 1354
🏁 Script executed:
# Check if there are any integrity checks in the entire cpuid_tool_build context
rg -B 5 -A 40 "test.fetch_asset" qemu/tests/idlehlt.pyRepository: autotest/tp-qemu
Length of output: 1986
🏁 Script executed:
# Look for any checksum-related configuration in test files or params
fd -name "*.py" qemu/tests/ | xargs rg -l "fetch_asset.*checksum|checksum.*fetch_asset|fetch_asset.*sha" | head -5Repository: autotest/tp-qemu
Length of output: 290
🏁 Script executed:
# Search for fetch_asset usage patterns to see if checksum is ever used
rg "fetch_asset.*checksum|checksum.*fetch_asset" --type pyRepository: autotest/tp-qemu
Length of output: 42
🏁 Script executed:
# Search for fetch_asset function definition
rg -B 5 -A 15 "def fetch_asset" --type pyRepository: autotest/tp-qemu
Length of output: 42
🏁 Script executed:
# Look for any checksum/hash parameter usage with fetch_asset
rg "fetch_asset\(" --type py -A 2 | grep -E "fetch_asset|checksum|sha|md5|hash" | head -20Repository: autotest/tp-qemu
Length of output: 136
🏁 Script executed:
# Check if there are examples of HTTPS URLs in test files
rg "https://" qemu/tests/idlehlt.pyRepository: autotest/tp-qemu
Length of output: 42
🏁 Script executed:
# Check imports at the top of idlehlt.py
head -n 50 qemu/tests/idlehlt.pyRepository: autotest/tp-qemu
Length of output: 1972
🏁 Script executed:
# Search for fetch_asset across the entire repo to find where it's defined
rg "def fetch_asset" --type pyRepository: autotest/tp-qemu
Length of output: 42
🏁 Script executed:
# Check if there are any other test files that show patterns for URL verification/checksums
rg "test.fetch_asset" qemu/tests/ -A 3 | head -30Repository: autotest/tp-qemu
Length of output: 299
🏁 Script executed:
# Look for HTTPS URLs across all test files to see if HTTPS is commonly used
rg "https://" qemu/tests/ | head -10Repository: autotest/tp-qemu
Length of output: 1518
🌐 Web query:
etallen.com cpuid HTTPS support
💡 Result:
etallen.com’s cpuid page is available over HTTPS, and the download links shown on that page resolve to HTTPS URLs (e.g., https://www.etallen.com/cpuid/...). [1]
Also, requesting the page over HTTP (e.g., http://etallen.com/cpuid.html or http://www.etallen.com/cpuid.html) lands on the HTTPS version, indicating an HTTP→HTTPS redirect (at least for this page). [2]
Sources: [1], [2]
🌐 Web query:
avocado test framework fetch_asset checksum verification integrity
💡 Result:
In Avocado, asset integrity checking for fetch_asset() is optional and driven by the asset_hash parameter:
- Enable checksum verification: pass
asset_hash=...toTest.fetch_asset(name, asset_hash=..., algorithm=...). Ifasset_hashis not given, Avocado skips hash verification. [1] - Hash algorithm: controlled by
algorithm(defaults to SHA-1). [1][2] - How verification works (cache-aware):
- Avocado first searches the configured asset cache directories (
[datadir.paths] cache_dirs). If not found, it downloads from the providedlocationsand stores it in the first writable cache dir. [1] - When
asset_hashis provided, Avocado prefers using an existing hash sidecar file in the cache (e.g.file.tar.gz.sha1). If the hash file is missing, it computes the hash and writes the hash file for future runs, then compares againstasset_hash. [1][3] - If download or verification fails,
fetch_asset()raises an error (or can cancel the test ifcancel_on_missing=True). [1][4]
- Avocado first searches the configured asset cache directories (
- Important edge case (algorithm mismatch): historically, if an asset had a cached checksum file using one algorithm (e.g. SHA-1) but the caller requested another (e.g. SHA-256), Avocado could treat it as a mismatch and re-download; Avocado 101.0 is noted as including a fix to recompute/handle checksum algorithm differences more correctly to avoid unnecessary re-downloads. [5]
Sources: [1] fetch_asset() docs and parameters, including asset_hash behavior and default algorithm [2] Asset utility API (algorithm parameter/default context) [3] older but explicit description of .sha1 sidecar behavior [4] fetch_asset() raising/cancel semantics [5] Avocado 101.0 checksum/algorithm mismatch fix noted in qemu-devel thread.
🏁 Script executed:
# Search for any examples of checksum/sha validation in test code patterns
rg "sha256|sha1|md5.*download|download.*sha256" qemu/tests/ -A 2 -B 2 | head -30Repository: autotest/tp-qemu
Length of output: 2549
🏁 Script executed:
# Look for any configuration files that might specify checksums
find qemu/tests -name "*.cfg" -exec grep -l "cpuid\|checksum\|sha" {} \; | head -5Repository: autotest/tp-qemu
Length of output: 4093
Use HTTPS and add checksum verification for the cpuid download.
The default URL uses HTTP when HTTPS is available at https://www.etallen.com/cpuid/. Additionally, test.fetch_asset() supports optional checksum verification via the asset_hash parameter, but none is currently used. Since the downloaded source is extracted and installed with elevated privileges (sudo make install), add both HTTPS and integrity verification to mitigate supply-chain risks.
🤖 Prompt for AI Agents
In `@qemu/tests/idlehlt.py` around lines 123 - 126, Update the cpuid download to
use HTTPS and add checksum verification: change the default in params.get for
url_cpuid_tool to the HTTPS URL
(https://www.etallen.com/cpuid/cpuid-20250513.src.tar.gz), add a new param
(e.g., asset_hash_cpuid_tool) with the expected hash as its default, and pass
that value into test.fetch_asset via its asset_hash parameter when fetching the
cpuid archive (references: url_cpuid_tool, params.get, and the call to
test.fetch_asset).
|
Latest revision test case result: This testcase is using avocado-vt utils introduced in avocado-framework/avocado-vt#4232 |
1 participant
Introduces a test case to verify the Idle HLT Intercept feature in QEMU, using cpuid to check support and ftrace to monitor idle-halt exits. Supports secure guest types (SEV, SEV-ES, SNP) with configurable parameters.
Summary by CodeRabbit
New Features
Tests