Guest vs host features check #4884

ShadowCurse · 2024-10-30T15:27:15Z

Changes

Add tests comparing host and guest default cpu features.

Reason

We want to know what is the difference between cpu features on the host and in the guest.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

If a specific issue led to this PR, this PR closes the issue.
The description of changes is clear and encompassing.
Any required documentation changes (code and docs) are included in this
PR.
API changes follow the Runbook for Firecracker API changes.
User-facing changes are mentioned in CHANGELOG.md.
All added/changed functionality is tested.
New TODOs link to an issue.
Commits meet
contribution quality standards.

This functionality cannot be added in rust-vmm.

codecov · 2024-10-30T15:37:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.10%. Comparing base (3d0421f) to head (9cb0669).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4884   +/-   ##
=======================================
  Coverage   84.10%   84.10%           
=======================================
  Files         251      251           
  Lines       28080    28080           
=======================================
  Hits        23616    23616           
  Misses       4464     4464

Flag	Coverage Δ
5.10-c5n.metal	`84.67% <ø> (ø)`
5.10-m5n.metal	`84.65% <ø> (ø)`
5.10-m6a.metal	`83.96% <ø> (ø)`
5.10-m6g.metal	`80.78% <ø> (ø)`
5.10-m6i.metal	`84.65% <ø> (ø)`
5.10-m7g.metal	`80.78% <ø> (ø)`
6.1-c5n.metal	`84.67% <ø> (-0.01%)`	⬇️
6.1-m5n.metal	`84.65% <ø> (-0.01%)`	⬇️
6.1-m6a.metal	`83.96% <ø> (ø)`
6.1-m6g.metal	`80.78% <ø> (ø)`
6.1-m6i.metal	`84.65% <ø> (ø)`
6.1-m7g.metal	`80.78% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tests/integration_tests/functional/test_cpu_features_aarch64.py

pb8o · 2024-10-31T13:53:13Z

tests/integration_tests/functional/test_cpu_features_aarch64.py

+@pytest.mark.skipif(
+    PLATFORM != "aarch64",
+    reason="This is aarch64 specific test.",
+)


We could make this test more generic and support x86_64. Maybe it belongs in a generic test_cpu_features.py (and move the current test_cpu_features.py to test_cpu_features_x86.py)

I would prefer to keep our current separation where test_cpu_feautres is for x86 and test_cpu_features_aarch64 is for aarch64. No reason for creating some new file for a singe test.
I don't mind renaming test_cpu_features to test_cpu_features_x86 though.

On the other hand, we should look for any opportunities to consolidate and unify those tests. I think this test could be the first step in that direction.

These are 2 different architectures, we don't need to unify tests for them.

pb8o · 2024-10-31T13:54:07Z

tests/framework/utils_cpuid.py

@@ -10,6 +10,8 @@
 from framework.utils import check_output
 from framework.utils_imdsv2 import imdsv2_get

+CPU_FEATURES_CMD = r"lscpu |grep -oP '^Flags:\s+\K.+'"


Since right now it is used in a single test, I think it should stay closer to where it's used.

Idea is that it might be used in x86 version of this test. I did not implement it here though.

In that case we can wait until that happens, but in general this test is simple enough that could work for both architectures.

I added x86_64 test as well, so this global constant is now used in 2 places.

roypat · 2024-10-31T14:24:29Z

tests/integration_tests/functional/test_cpu_features_aarch64.py

+            }
+            assert guest_feats - host_feats == {"ssbs"}
+        case _:
+            assert False, f"Cpu model {cpu_model} is not supported"


Let's not have tests that fail unconditionally locally. Can we have this be behind a sort of if env.get("buildkite") is not None:?

what is the problem with this test failing on an instance for which we don't have cpu feature diff?

kalyazin · 2024-10-31T17:54:59Z

tests/integration_tests/functional/test_cpu_features_aarch64.py

+    match cpu_model:
+        case CpuModel.ARM_NEOVERSE_N1:
+            assert host_feats - guest_feats == set()
+            assert guest_feats - host_feats == {"ssbs"}


Ideally we should have explanations for why we have those differences (possibly with pointers to our docs).

added notes to the aarch64 test. Not sure how to describe the diffs for x86_64 test though.

Add test comparing host and guest default cpu features. Signed-off-by: Egor Lazarchuk <[email protected]>

ShadowCurse force-pushed the guest_host_features_check branch 6 times, most recently from 64c7e72 to e81248e Compare October 31, 2024 13:09

ShadowCurse changed the title ~~Guest host features check~~ Guest vs host features check for aarch64 Oct 31, 2024

ShadowCurse marked this pull request as ready for review October 31, 2024 13:20

ShadowCurse self-assigned this Oct 31, 2024

ShadowCurse added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Oct 31, 2024

ShadowCurse force-pushed the guest_host_features_check branch from e81248e to 41b2748 Compare October 31, 2024 13:26

pb8o reviewed Oct 31, 2024

View reviewed changes

roypat reviewed Oct 31, 2024

View reviewed changes

ShadowCurse force-pushed the guest_host_features_check branch from 41b2748 to e813c62 Compare October 31, 2024 15:24

kalyazin reviewed Oct 31, 2024

View reviewed changes

ShadowCurse force-pushed the guest_host_features_check branch 2 times, most recently from 9593771 to 6ce3d2b Compare November 1, 2024 14:43

ShadowCurse changed the title ~~Guest vs host features check for aarch64~~ Guest vs host features check Nov 1, 2024

feat(aarch64): add host vs guest cpu feature test

1907824

Add test comparing host and guest default cpu features. Signed-off-by: Egor Lazarchuk <[email protected]>

ShadowCurse force-pushed the guest_host_features_check branch 3 times, most recently from efdf8ee to f56f348 Compare November 1, 2024 16:43

feat(x86)64: add host vs guest cpu feature test

9cb0669

Add test comparing host and guest default cpu features. Signed-off-by: Egor Lazarchuk <[email protected]>

ShadowCurse force-pushed the guest_host_features_check branch from f56f348 to 9cb0669 Compare November 1, 2024 16:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Guest vs host features check #4884

Guest vs host features check #4884

ShadowCurse commented Oct 30, 2024 •

edited

Loading

codecov bot commented Oct 30, 2024 •

edited

Loading

pb8o Oct 31, 2024

ShadowCurse Oct 31, 2024

pb8o Oct 31, 2024

ShadowCurse Nov 1, 2024

pb8o Oct 31, 2024 •

edited

Loading

ShadowCurse Oct 31, 2024

pb8o Oct 31, 2024

ShadowCurse Nov 1, 2024

roypat Oct 31, 2024

ShadowCurse Oct 31, 2024

kalyazin Oct 31, 2024

ShadowCurse Nov 1, 2024

Guest vs host features check #4884

Are you sure you want to change the base?

Guest vs host features check #4884

Conversation

ShadowCurse commented Oct 30, 2024 • edited Loading

Changes

Reason

License Acceptance

PR Checklist

codecov bot commented Oct 30, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pb8o Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShadowCurse commented Oct 30, 2024 •

edited

Loading

codecov bot commented Oct 30, 2024 •

edited

Loading

pb8o Oct 31, 2024 •

edited

Loading