Skip to content

Conversation

JinLiul
Copy link
Contributor

@JinLiul JinLiul commented Sep 3, 2025

ID: 4377
After applying avocado-framework/avocado-vt#4212 , need to change snp policy to hex format.

Summary by CodeRabbit

  • New Features
    • Tests now accept SNP policy values in both hexadecimal and decimal formats for greater flexibility.
  • Bug Fixes
    • Prevents misinterpretation of SNP policy values during test runs by auto-detecting numeric base.
  • Tests
    • Updated snp_basic_config variants to use unquoted policy values and hex literals.
    • Parsing logic now converts policy strings with automatic base detection for accurate comparison with guest info.
  • Chores
    • Minor configuration cleanup to keep fields consistent across variants.

@JinLiul
Copy link
Contributor Author

JinLiul commented Sep 3, 2025

(1/3) Host_RHEL.m10.u1.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.1.x86_64.io-github-autotest-qemu.snp_basic_config.policy_default.q35: STARTED
(1/3) Host_RHEL.m10.u1.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.1.x86_64.io-github-autotest-qemu.snp_basic_config.policy_default.q35: PASS (89.59 s)
(2/3) Host_RHEL.m10.u1.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.1.x86_64.io-github-autotest-qemu.snp_basic_config.policy_debug.q35: STARTED
(2/3) Host_RHEL.m10.u1.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.1.x86_64.io-github-autotest-qemu.snp_basic_config.policy_debug.q35: PASS (91.63 s)

@JinLiul
Copy link
Contributor Author

JinLiul commented Sep 3, 2025

@zixi-chen @zhencliu could you please help to review? Thanks.

@zixi-chen
Copy link
Contributor

@JinLiul there's a mistake I made before, I think we can fix this with this patch. For the single socket SNP policy, it should be 0x130000, in binary 100110000000000000000, bit 20 stands for single socket. By the way, policy = 30000 or 0x30000 both of the methods should work.

@bssrikanth
Copy link

0x130000

@zixi-chen I had that fixed in decimal already in my PR https://github.com/autotest/tp-qemu/pull/4351/files#diff-aa2e3ba229ed25005ee285fbac830a765e8e69bdfef6c16b1cc012d783a7052dR37
If you get a chance please review that PR, TiA!

@JinLiul
Copy link
Contributor Author

JinLiul commented Sep 4, 2025

By the way, policy = 30000 or 0x30000 both of the methods should work.

The policy doesn't match if change the 0x30000 to 30000.
avocado.core.exceptions.TestFail: QMP snp policy doesn't match 30000.

@JinLiul
Copy link
Contributor Author

JinLiul commented Sep 4, 2025

Updated the policy_singlesocket to 0x130000

@zhencliu
Copy link
Contributor

zhencliu commented Sep 11, 2025

Just suggest to change the header of the commit to:
snp_basic_config: Change SNP policy to hex format

So we can easily know which tc/module is modified from the commit

I have to remove the LGTM, for avocado-framework/avocado-vt#4212 is to be reviewed, there might be some code update

Copy link

coderabbitai bot commented Sep 12, 2025

Walkthrough

Updated SNP policy values in a config to hex and adjusted quoting for policy substitution. In test logic, replaced numeric helper with string-based int parsing using base 0 to support hex/decimal. No other logic, structure, or public API changes.

Changes

Cohort / File(s) Summary of changes
SNP policy config updates
qemu/tests/cfg/snp_basic_config.cfg
Changed snp_policy values from decimals to hex literals; switched vm_secure_guest_object_options to use unquoted policy=${snp_policy}.
Policy parsing logic
qemu/tests/snp_basic_config.py
Replaced get_numeric("snp_policy") with int(vm.params.get("snp_policy"), 0) to auto-detect base; retained subsequent comparisons.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks (3 passed)

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "Change SNP policy to hex format" succinctly and accurately summarizes the primary change in the PR — switching SNP policy values to hexadecimal and updating parsing — and is concise, specific, and relevant to the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

Poem

I twitch my whiskers at hex’s glow,
From 30000h to B0000—on we go!
A nibble here, a base that knows,
Parse by zero, truth now shows.
In fields of bits I hop with glee,
Config and tests in harmony.

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
qemu/tests/cfg/snp_basic_config.cfg (1)

32-33: Single-socket policy value is right; minor maintainability nit.

0x130000 = 0x100000 (bit20 single-socket) + 0x30000 (base). Consider an inline comment to document the bit meaning.

Example:

-            snp_policy = 0x130000
+            snp_policy = 0x130000  # bit20 (single-socket) | base 0x30000
qemu/tests/snp_basic_config.py (1)

51-55: Parse robustness and clearer mismatch diagnostics.

Guard against missing/invalid params and include both hex/dec in the failure to speed up triage.

Apply:

-    vm_policy = int(vm.params.get("snp_policy"), 0)
+    vm_policy_str = vm.params.get("snp_policy")
+    if not vm_policy_str:
+        test.cancel("Missing 'snp_policy' param in test params.")
+    try:
+        vm_policy = int(vm_policy_str, 0)
+    except Exception as e:
+        test.cancel(f"Invalid snp_policy '{vm_policy_str}': {e}")
@@
-    if sev_guest_info["snp-policy"] != vm_policy:
-        test.fail("QMP snp policy doesn't match %s." % vm_policy)
+    if sev_guest_info["snp-policy"] != vm_policy:
+        test.fail(
+            "QMP snp policy mismatch: expected %s (%d), got %s (%d)." %
+            (hex(vm_policy), vm_policy, hex(sev_guest_info["snp-policy"]),
+             sev_guest_info["snp-policy"])
+        )
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8ad6b9c and 714aaf1.

📒 Files selected for processing (2)
  • qemu/tests/cfg/snp_basic_config.cfg (1 hunks)
  • qemu/tests/snp_basic_config.py (1 hunks)
🔇 Additional comments (2)
qemu/tests/cfg/snp_basic_config.cfg (2)

25-26: Hex default policy and unquoted object option look correct.

0x30000 matches the prior 196608; unquoting policy=${snp_policy} is fine for QEMU option assembly.


28-29: Debug policy conversion is accurate.

0xB0000 equals the previous 720896; no issues anticipated.

@JinLiul
Copy link
Contributor Author

JinLiul commented Sep 15, 2025

Updated the commit title.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants