Implement hypervisor specific pause/resume API #6441

russell-islam · 2024-05-03T21:01:35Z

No description provided.

rbradford · 2024-05-04T08:30:55Z

hypervisor/src/vm.rs

@@ -390,6 +395,17 @@ pub trait Vm: Send + Sync + Any {
    ) -> Result<()> {
        unimplemented!()
    }
+
+    /// Freeze the Partiton/VM


I would prefer reuse the pause / resume terminology. There is not need for the _vm suffix as this is on the Vm trait.

Please also make it unconditional at build time and simply provide an empty implementation in the trait.

Are you refering to rename the function name or implementing the pausable trait? I skipped implementing the pauasable trait it it requires mutable reference to vm. I can easily rename the functions and make it unconditional. Let me know. Thanks

Just the name - I don't expect you to implement the trait.

hypervisor/src/mshv/mod.rs

NunoDasNeves · 2024-05-07T23:49:24Z

vmm/src/vm.rs

+        // Unfreeze the vm for MSHV
+        #[cfg(feature = "mshv")]
+        if current_state == VmState::Created {
+            self.vm.resume().map_err(Error::UnfreezeVm)?;


Should this be MigratableError::Resume ?

boot() does not return MigratableError so we can't return.

NunoDasNeves · 2024-05-07T23:49:49Z

vmm/src/vm.rs

@@ -319,6 +319,9 @@ pub enum Error {

    #[error("Error injecting NMI")]
    ErrorNmi,
+    #[cfg(feature = "mshv")]
+    #[error("Error unfreezing the VM: {0}")]
+    UnfreezeVm(#[source] hypervisor::HypervisorVmError),


Since you renamed freeze/unfreeze to pause/resume maybe we don't need this anymore

NunoDasNeves · 2024-05-07T23:55:08Z

hypervisor/src/vm.rs

+    /// Failed to set partition property
+    ///
+    #[error("Failed to set partition property: {0}")]
+    SetPartitionProperty(#[source] anyhow::Error),


I suspect we don't really want to define an error type for all the lower-level things that could go wrong here in the generic code.

IMO, only the mshv hypervisor-related code should know about "partition"s, etc... The generic CLH code can just see this error as 'failed to pause/resume', and the wrapped inner error will contain the additional details from the mshv code - I believe this is the main point of using the anyhow crate.

Also worth noting: we already have a SetPartitionProperty error in hypervisor/src/hypervisor.rs, which is clearly in the wrong place. Maybe we could fix just that, for a start, and clean up any other errors that shouldn't be in the generic code later on in some cleanup patch.

I can cleanup in another PR

Yes, please don't have hypervisor specific errors - if you can avoid it at the top level.

NunoDasNeves

LGTM

rbradford · 2024-05-08T21:49:35Z

hypervisor/src/mshv/mod.rs

+
+    /// Pause the VM
+    #[cfg(feature = "mshv")]
+    fn pause(&self) -> vm::Result<()> {


This is in the mshv file - is this #[cfg(feature = "mshv")] needed?

rbradford · 2024-05-08T21:50:48Z

hypervisor/src/vm.rs

+    /// Failed to set partition property
+    ///
+    #[error("Failed to set partition property: {0}")]
+    SetPartitionProperty(#[source] anyhow::Error),


Yes, please don't have hypervisor specific errors - if you can avoid it at the top level.

rbradford · 2024-05-08T21:51:16Z

vmm/src/vm.rs

+        #[cfg(feature = "mshv")]
+        if current_state == VmState::Paused {
+            self.vm.resume().map_err(|e| {
+                MigratableError::Resume(anyhow!("Could not unfreeze the VM: {}", e))


Old terminology - unfreeze?

rbradford · 2024-05-08T21:51:26Z

vmm/src/vm.rs

+        #[cfg(feature = "mshv")]
+        self.vm
+            .pause()
+            .map_err(|e| MigratableError::Pause(anyhow!("Could not freeze the VM: {}", e)))?;


Ditto - old terminology.

rbradford · 2024-05-08T21:52:04Z

vmm/src/vm.rs

@@ -2487,6 +2495,11 @@ impl Pausable for Vm {
        self.cpu_manager.lock().unwrap().pause()?;
        self.device_manager.lock().unwrap().pause()?;

+        #[cfg(feature = "mshv")]


Since there is a default implementation - you don't need to make this conditional - applies to the other changes in this file too.

Add Pause/Resume functions for VM trait. For KVM it will be empty implementations. For MSHV it needs to freeze and unfreeze the partition. Signed-off-by: Muminul Islam <muislam@microsoft.com>

Implementing pause/Resume API for MSHV. Here we set/reset the partition property(TIME_FREEZE) Signed-off-by: Muminul Islam <muislam@microsoft.com>

Create a partition frozen always, then unfreeze the partition during boot phase or resume phase. We also freeze the partition during pause event. Time is freeze during the time between freeze and unfreeze. Signed-off-by: Muminul Islam <muislam@microsoft.com>

For MSHV we always create frozen partition, so we resume the VM during boot. Also during pause and resume VM events we call hypervisor specific API. Signed-off-by: Muminul Islam <muislam@microsoft.com>

russell-islam · 2024-05-09T22:40:57Z

@rbradford PTAL

jinankjain

LGTM

rbradford · 2024-05-14T10:38:07Z

vmm/src/vm.rs

@@ -2495,6 +2507,7 @@ impl Pausable for Vm {

    fn resume(&mut self) -> std::result::Result<(), MigratableError> {
        event!("vm", "resuming");
+        let current_state = self.get_state().unwrap();


This can cause a panic - please propagate properly using ?

Now I look at it - handling of this seems inconsistent in this file...

russell-islam requested a review from a team as a code owner May 3, 2024 21:01

russell-islam requested review from jinankjain, NunoDasNeves and liuw and removed request for a team May 3, 2024 21:01

russell-islam force-pushed the muislam/partition-freeze branch from d71d66c to 3a76d90 Compare May 3, 2024 22:58

rbradford reviewed May 4, 2024

View reviewed changes

NunoDasNeves reviewed May 6, 2024

View reviewed changes

hypervisor/src/mshv/mod.rs Show resolved Hide resolved

russell-islam force-pushed the muislam/partition-freeze branch from 3a76d90 to 75dd690 Compare May 7, 2024 20:50

russell-islam changed the title ~~freeze/unfreeze vm during pause/resume for MSHV~~ Implement hypervisor specific pause/resume API May 7, 2024

NunoDasNeves reviewed May 7, 2024

View reviewed changes

russell-islam force-pushed the muislam/partition-freeze branch from 75dd690 to 98a8fb0 Compare May 8, 2024 19:42

NunoDasNeves approved these changes May 8, 2024

View reviewed changes

rbradford reviewed May 8, 2024

View reviewed changes

hypervisor: Add pause/resume definitions to VM trait

1dac5c7

Add Pause/Resume functions for VM trait. For KVM it will be empty implementations. For MSHV it needs to freeze and unfreeze the partition. Signed-off-by: Muminul Islam <muislam@microsoft.com>

russell-islam force-pushed the muislam/partition-freeze branch from 98a8fb0 to 63a874b Compare May 8, 2024 22:14

russell-islam added 3 commits May 9, 2024 15:03

hypervisor: implement pause/resume API for MSHV

d8b0145

Implementing pause/Resume API for MSHV. Here we set/reset the partition property(TIME_FREEZE) Signed-off-by: Muminul Islam <muislam@microsoft.com>

vmm: pause/resume VM during the VM events

0461c30

For MSHV we always create frozen partition, so we resume the VM during boot. Also during pause and resume VM events we call hypervisor specific API. Signed-off-by: Muminul Islam <muislam@microsoft.com>

russell-islam force-pushed the muislam/partition-freeze branch from 63a874b to 0461c30 Compare May 9, 2024 22:03

jinankjain approved these changes May 13, 2024

View reviewed changes

liuw approved these changes May 16, 2024

View reviewed changes

rbradford reviewed May 16, 2024

View reviewed changes

rbradford approved these changes May 16, 2024

View reviewed changes

rbradford added this pull request to the merge queue May 16, 2024

Merged via the queue into cloud-hypervisor:main with commit 860939d May 16, 2024
32 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement hypervisor specific pause/resume API #6441

Implement hypervisor specific pause/resume API #6441

russell-islam commented May 3, 2024

rbradford May 4, 2024

russell-islam May 6, 2024

rbradford May 7, 2024

NunoDasNeves May 7, 2024

russell-islam May 8, 2024

NunoDasNeves May 7, 2024 •

edited

NunoDasNeves May 7, 2024 •

edited

russell-islam May 8, 2024

rbradford May 8, 2024

NunoDasNeves left a comment

rbradford May 8, 2024

rbradford May 8, 2024

rbradford May 8, 2024

rbradford May 8, 2024

rbradford May 8, 2024

russell-islam commented May 9, 2024

jinankjain left a comment

rbradford May 14, 2024

rbradford May 16, 2024

Implement hypervisor specific pause/resume API #6441

Implement hypervisor specific pause/resume API #6441

Conversation

russell-islam commented May 3, 2024

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

NunoDasNeves May 7, 2024 • edited

Choose a reason for hiding this comment

NunoDasNeves May 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NunoDasNeves left a 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

russell-islam commented May 9, 2024

jinankjain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NunoDasNeves May 7, 2024 •

edited

NunoDasNeves May 7, 2024 •

edited