Skip to content

Comments

lib: implement reference TSC enlightenment#856

Merged
gjcolombo merged 21 commits intomasterfrom
gjcolombo/reference-tsc
Feb 25, 2025
Merged

lib: implement reference TSC enlightenment#856
gjcolombo merged 21 commits intomasterfrom
gjcolombo/reference-tsc

Conversation

@gjcolombo
Copy link
Contributor

@gjcolombo gjcolombo commented Feb 12, 2025

Add a Hyper-V reference time enlightenment to the Hyper-V stack. See the new tsc module's doc comment for details about how the enlightenment operates.

Add PHD tests that try to verify both that the enlightenment is present (by querying the current clocksource) and that it keeps time roughly correctly (by comparing host and guest time readings). The latter test also takes readings over a migration to help get some coverage of the time data phase of live migration (though the migration is not currently cross-machine, so this is a little less interesting than it might otherwise be).

Tests: cargo test; PHD w/Alpine, Debian 11, WS2022 guests.

Fixes #328.

Copy link
Contributor

@jordanhendricks jordanhendricks left a comment

Choose a reason for hiding this comment

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

initial pass; want to learn a bit more about this feature in depth and I'll take another one

@gjcolombo gjcolombo force-pushed the gjcolombo/overlay-pages-part-2 branch from 78057ac to 2e8e741 Compare February 14, 2025 18:40
@gjcolombo gjcolombo force-pushed the gjcolombo/reference-tsc branch from 24be46e to c450002 Compare February 14, 2025 18:40
@gjcolombo gjcolombo force-pushed the gjcolombo/reference-tsc branch from a5dd761 to 9b3c294 Compare February 21, 2025 18:36
@gjcolombo gjcolombo changed the base branch from gjcolombo/overlay-pages-part-2 to master February 21, 2025 18:36
@gjcolombo
Copy link
Contributor Author

gjcolombo commented Feb 21, 2025

I've rebased this to the latest master. This was unfortunately a bit of a mess, because #861 obsoleted a fair amount of the migration logic that was present previously. It may help to look at just the last five commits to get a sense of what's actually changed (beyond merge conflict resolutions during the rebase).

Now that the Hyper-V stack doesn't migrate overlay page contents directly, it needs to be more careful to ensure that it generates TSC overlays that are based on the correct guest frequency. The challenges here are

  • we can't read the guest frequency during enlightenment creation because we don't have a VmmHdl yet
  • during import, we do have a VmmHdl, but we'd like to avoid assuming that the time-data phase of migration has already happened (which has to happen for us to read the correct guest frequency out of the kernel VM)

Most of the new code involves a new ReferenceTsc helper type that keeps track of whether the enlightenment is on, whether it's initialized, and what guest frequency it's supposed to use when constructing the reference page. A lot of the newest code is meant to work with this type and to make sure its configuration data doesn't get blown away during VM reset.

EDIT: Things that blessedly did not change are the reference TSC page's contents (and the math we use to compute them) and the new PHD tests.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Hm, the introduction of the Uninitialized state etc is not my favorite thing, but I agree that this is necessary due to the changes to how we migrate overlays, and I can't think up a solution that avoids it. This seems good to me!

Copy link
Member

@iximeow iximeow left a comment

Choose a reason for hiding this comment

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

more nits about the panics :) seems good, very excited for this!

@gjcolombo gjcolombo force-pushed the gjcolombo/reference-tsc branch from 9b3c294 to 736e595 Compare February 24, 2025 16:53
@gjcolombo
Copy link
Contributor Author

Thanks for re-reviewing! ae5cfac should take care of the latest batch of comments.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

new commentary looks lovely, thank you!

Comment on lines +193 to +195
/// The caller must call [`attach`] to finish initializing this
/// enlightenment stack before starting any VM components that depend on it.
/// Otherwise the stack may panic while the VM is running.
Copy link
Member

Choose a reason for hiding this comment

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

mmmmmaybe this ought to have a big leading "WARNING:" on it? not a big deal though.

self.vmm_hdl.get().unwrap()
self.vmm_hdl.get().expect(
"a fully-initialized Hyper-V enlightenment always has a \
VMM handle (did the library user remember to call `attach`?)",
Copy link
Member

Choose a reason for hiding this comment

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

good call with these notes!

Copy link
Contributor

@jordanhendricks jordanhendricks left a comment

Choose a reason for hiding this comment

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

🚢

@gjcolombo gjcolombo merged commit 6c1c2f4 into master Feb 25, 2025
11 checks passed
@gjcolombo gjcolombo deleted the gjcolombo/reference-tsc branch February 25, 2025 19:27
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.

Hyper-V enlightenment reset needs to reset MSR contents want paravirtualized clock enlightenment

4 participants