Add snapshot compatibility checks for snapshot create/restore #2596

georgepisaltu · 2021-05-27T16:44:57Z

Reason for This PR

VMs restored from a snapshot might have the TSC frequency misaligned with the original, which could lead to problems further down the line. We took this as an opportunity to add snapshot compatibility checks, which start with adding the original TSC in the state file and scaling it for resumed VMs.

Description of Changes

Added compatibility checks for snapshot create/restore, starting with TSC scaling.

This functionality can be added in rust-vmm.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

All commits in this PR are signed (git commit -s).
The reason for this PR is clearly provided (issue no. or explanation).
The description of changes is clear and encompassing.
Any required documentation changes (code and docs) are included in this PR.
Any newly added unsafe code is properly documented.
Any API changes are reflected in firecracker/swagger.yaml.
Any user-facing changes are mentioned in CHANGELOG.md.
All added/changed functionality is tested.

src/api_server/swagger/firecracker.yaml

dianpopa · 2021-05-31T11:01:58Z

Should we close this one?

src/cpuid/src/common.rs

sandreim · 2021-06-04T11:42:50Z

src/vmm/src/vstate/vcpu/x86_64.rs

+    pub fn set_tsc_khz(&self, state_tsc: Option<u32>, cpuid: &CpuId) -> Result<()> {
+        // Check if CPU models are the same.
+        if let Ok(curr_cpuid) = cpuid::common::get_cpuid(0x1, 0) {
+            if is_same_model(cpuid, curr_cpuid.eax) {


This allows snapshots to be loaded on the same CPU, even if there is no constant TSC or TSC scaling support. Are we sure the checks we do here guarantee that TSC freq is the same ?

If we as a process never change the TSC frequency outside the scope of a snapshot save/restore, and the CPU model is the same, I expect the TSC frequency to be the same in the clone as in the original VM. If this is not true, then checking the CPU model is actually useless, since we still need to get/set the TSC frequency regardless.

On variant TSC systems, I don't think users even expect high precision from the TSC, but I might have a bad understanding of this whole story.

src/vmm/src/vstate/vcpu/x86_64.rs

georgepisaltu

@sandreim PTAL!

src/vmm/src/vstate/vcpu/x86_64.rs

georgepisaltu

@sandreim @serban300 I updated the PR, also added unit tests.

The next step is to add integration tests using snapshots generated with the final form of the binary after merging this.

PTAL!

src/vmm/src/vstate/vcpu/x86_64.rs

src/vmm/src/builder.rs

src/cpuid/src/common.rs

src/vmm/src/builder.rs

src/vmm/src/vstate/vcpu/x86_64.rs

sandreim · 2021-06-14T11:00:33Z

tests/integration_tests/functional/test_snapshot_advanced.py

+    # Resume microvm using current build of FC/Jailer.
+    # The resume should be successful because the CPU model
+    # in the snapshot state is the same as this host's.
+    microvm, _ = builder.build_from_snapshot(


Please also add an additional issue to cover negative testing on different CPU models.

Will do, after this PR is merged.

src/vmm/src/builder.rs

src/vmm/src/vstate/vcpu/x86_64.rs

georgepisaltu

@serban300 @sandreim PTAL!

Signed-off-by: George Pisaltu <gpl@amazon.com>

georgepisaltu self-assigned this May 27, 2021

acatangiu reviewed May 31, 2021

View reviewed changes

src/api_server/swagger/firecracker.yaml Outdated Show resolved Hide resolved

dianpopa added the Status: Author label May 31, 2021

georgepisaltu force-pushed the tsc_scaling branch from bdac2af to 89eb708 Compare June 4, 2021 09:25

serban300 reviewed Jun 4, 2021

View reviewed changes

src/cpuid/src/common.rs Outdated Show resolved Hide resolved

src/cpuid/src/common.rs Outdated Show resolved Hide resolved

src/cpuid/src/common.rs Outdated Show resolved Hide resolved

src/cpuid/src/common.rs Outdated Show resolved Hide resolved

sandreim reviewed Jun 4, 2021

View reviewed changes

georgepisaltu force-pushed the tsc_scaling branch 2 times, most recently from 7540cbe to 8f316de Compare June 4, 2021 13:10

georgepisaltu added Status: Awaiting review Indicates that a pull request is ready to be reviewed and removed Status: Author labels Jun 4, 2021

georgepisaltu commented Jun 7, 2021

View reviewed changes

src/vmm/src/vstate/vcpu/x86_64.rs Outdated Show resolved Hide resolved

src/vmm/src/vstate/vcpu/x86_64.rs Outdated Show resolved Hide resolved

georgepisaltu force-pushed the tsc_scaling branch from 8f316de to f07a80a Compare June 7, 2021 08:34

sandreim reviewed Jun 7, 2021

View reviewed changes

georgepisaltu force-pushed the tsc_scaling branch from f07a80a to ced7adf Compare June 7, 2021 14:43

georgepisaltu commented Jun 8, 2021

View reviewed changes

georgepisaltu marked this pull request as ready for review June 8, 2021 15:27

georgepisaltu force-pushed the tsc_scaling branch 5 times, most recently from 2407cad to 85cce2c Compare June 10, 2021 07:45

serban300 suggested changes Jun 11, 2021

View reviewed changes

src/vmm/src/vstate/vcpu/x86_64.rs Show resolved Hide resolved

src/vmm/src/vstate/vcpu/x86_64.rs Outdated Show resolved Hide resolved

src/vmm/src/builder.rs Show resolved Hide resolved

src/cpuid/src/common.rs Outdated Show resolved Hide resolved

georgepisaltu force-pushed the tsc_scaling branch 3 times, most recently from ad0eeec to 59501b4 Compare June 14, 2021 10:10

serban300 reviewed Jun 14, 2021

View reviewed changes

src/vmm/src/builder.rs Show resolved Hide resolved

sandreim reviewed Jun 14, 2021

View reviewed changes

src/vmm/src/vstate/vcpu/x86_64.rs Outdated Show resolved Hide resolved

georgepisaltu force-pushed the tsc_scaling branch from 59501b4 to fef3adf Compare June 14, 2021 14:32

georgepisaltu force-pushed the tsc_scaling branch from fef3adf to d9f8344 Compare June 14, 2021 14:41

georgepisaltu commented Jun 14, 2021

View reviewed changes

cpuid: add function for identifying same cpu model

551ac00

Signed-off-by: George Pisaltu <gpl@amazon.com>

georgepisaltu force-pushed the tsc_scaling branch from d9f8344 to 7a8f435 Compare June 15, 2021 08:39

georgepisaltu added 3 commits June 15, 2021 15:33

vmm: add compatibility check to vcpus

c5bd62c

Signed-off-by: George Pisaltu <gpl@amazon.com>

upgrade to kvm-ioctls v0.9.0

c7266f0

Signed-off-by: George Pisaltu <gpl@amazon.com>

added integ negative test for TSC scaling

4f53367

Signed-off-by: George Pisaltu <gpl@amazon.com>

georgepisaltu force-pushed the tsc_scaling branch from 7a8f435 to 4f53367 Compare June 15, 2021 12:33

serban300 approved these changes Jun 15, 2021

View reviewed changes

sandreim approved these changes Jun 15, 2021

View reviewed changes

georgepisaltu merged commit 7edec88 into firecracker-microvm:main Jun 15, 2021

georgepisaltu mentioned this pull request Jun 15, 2021

Add negative test for TSC snapshot restore on different CPU models #2624

Closed

georgepisaltu deleted the tsc_scaling branch September 30, 2021 09:18

Add snapshot compatibility checks for snapshot create/restore #2596

Add snapshot compatibility checks for snapshot create/restore #2596

Uh oh!

Conversation

georgepisaltu commented May 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reason for This PR

Description of Changes

License Acceptance

PR Checklist

Uh oh!

Uh oh!

dianpopa commented May 31, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sandreim Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgepisaltu Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

georgepisaltu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

georgepisaltu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sandreim Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

georgepisaltu Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

georgepisaltu left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

georgepisaltu commented May 27, 2021 •

edited

Loading

sandreim Jun 4, 2021 •

edited

Loading