add RtcState object #65

lauralt · 2021-09-13T07:39:47Z

This PR is adding support for saving and restoring the state of the RTC device. We are starting to add this support in rust-vmm to enable snapshot use cases, such as live migration, and to unblock the consumption of components (e.g. virtio devices, legacy devices).

This is also converting vm-superio to a workspace, in which the base vm-superio provides the functionality required to get/set the state of the device, and vm-superio-ser defines the RtcStateSer object (and in the future the ...StateSer objects for the other devices). RtcStateSer mirrors the state structure from vm-superio, and derives the required (de)serialization/versioning traits (i.e. Serialize, Deserialize, Versionize). If we later change the state of the component, this one will have to be updated as well (by also adding the required version constraints on it).

andreeaflorescu · 2021-09-23T08:20:23Z

src/rtc_pl031.rs

+    /// Creates a new `AMBA PL031 RTC` instance from a given `state` and invokes the `rtc_events`
+    /// implementation of `RtcEvents` during operation.


I think we should also add a comment about wanting to start from a clean state, in which case people should use with_events.

andreeaflorescu · 2021-09-23T08:24:59Z

src/rtc_pl031.rs

+    /// Creates a new `AMBA PL031 RTC` instance and invokes the `rtc_events`
+    /// implementation of `RtcEvents` during operation.


The part about "invokes the rtc_events" sounds like a implementation detail, and it shouldn't be part of the interface documentation. How about: "Creates a new AMBA PL031 RTC instance that is able to track events during operation using the passed rtc_events object." We should also add a paragraph about the state being the default one or something like that.

Updated the documentation for with_events and new.

andreeaflorescu · 2021-09-23T08:26:49Z

src/rtc_pl031.rs

+        // Check that the restored `Rtc` doesn't keep the state of the old `metrics` object.
+        assert_eq!(rtc_from_state.events.invalid_write_count.count(), 0);


nit: For the sake of the example, maybe it would be a good idea to have a "test as an example" where the metrics are saved as well.

Added an example, it's a bit weird though :-?.

crates/vm-superio/Cargo.toml

andreeaflorescu · 2021-09-23T08:34:09Z

crates/vm-superio/src/rtc_pl031.rs

@@ -210,7 +210,7 @@ impl<EV: RtcEvents> Rtc<EV> {
            ris: state.ris,
            // A struct implementing RtcEvents for tracking the occurrence of
            // significant events.
-            events,
+            events


nit: this should not be part of the commit.

removed it.

crates/vm-superio-ser/Cargo.toml

andreeaflorescu · 2021-10-01T11:55:43Z

crates/vm-superio/src/rtc_pl031.rs

@@ -134,6 +134,20 @@ pub struct Rtc<EV: RtcEvents> {
    events: EV,
 }

+/// The state of the Rtc device.
+pub struct RtcState {


Can we derive Debug, Clone, and PartialEq on this structure? It would help with testing.

I think it might also help to have a derive Default, but I am not sure what should be the implementation of that. Maybe we can implement default ourselves and have everything as 0s expect the LR which we can initialize as the current time on host.

Default is already implemented for RtcState. I was using derive initially, but switched to a manual implementation to be on the safe side, and it also helped to have a clearer documentation.

Added derives for Debug, Clone and PartialEq. I also realized now that RtcStateSer was deriving Default. I removed the derive, we can add a manual implementation if we think it's useful, but it doesn't seem so, at least for now.

I also added a Default impl, it would look a bit awkward to convert with From from an RtcState::default() in the consumer code in case a default RtcStateSer is needed.

andreeaflorescu · 2021-10-01T11:56:25Z

crates/vm-superio/src/rtc_pl031.rs

+impl Rtc<NoEvents> {
+    /// Creates a new `AMBA PL031 RTC` instance without any metric capabilities. The instance is
+    /// created from the default state.
+    pub fn new() -> Self {


Is it possible to also add a from state constructor here?

As discussed offline, we can postpone this a bit until we have a nice idea to avoid weird naming.

andreeaflorescu · 2021-10-04T18:36:10Z

src/rtc_pl031.rs

+        assert_eq!(rtc_from_state.events.invalid_write_count.count(), 1);
+
+        let state2 = rtc_from_state.state();
+        let metrics2 = metrics;


Maybe we can rename this to saved_metrics and add a comment here that we are mimicking saving the metrics for the sake of the example so that it is a bit less weird 😆

andreeaflorescu · 2021-10-04T18:38:17Z

crates/vm-superio-ser/src/rtc_pl031.rs

+    }
+}
+
+impl RtcStateSer {


Is there a reason for implementing new here, and from for the reversed operation?

Hmm, not really, I think I was under the impression I can't implement From here. I replaced with From.

RtcState represents the state of the Rtc device, and is storing the state needed for restoring the device (no implementation detailis). The generic `events` object is not stored due to complexity reasons. More details in the design proposal: rust-vmm/community#118. Signed-off-by: Laura Loghin <lauralg@amazon.com>

This crate provides state objects that add serialization and versionize capabilities to the ones from vm-superio. Signed-off-by: Laura Loghin <lauralg@amazon.com>

This provides serialization capabilities to RTCState, and is defined in the vm-superio-ser crate. RtcStateSer mirrors the state structure from vm-superio and derives the required (de)serialization/versioning traits (i.e. Serialize, Deserialize, Versionize). Signed-off-by: Laura Loghin <lauralg@amazon.com>

Signed-off-by: Laura Loghin <lauralg@amazon.com>

Since now the offset & LR can be set from outside of the Rtc implementation, we need to add a checked add when computing the value of the RTC. Added a test & fix for the overflow. Signed-off-by: Andreea Florescu <fandree@amazon.com>

lauralt force-pushed the state_no_ev branch from 09c5d73 to 43c33f3 Compare September 13, 2021 10:25

lauralt mentioned this pull request Sep 13, 2021

[RFC] Save/restore state design proposal rust-vmm/community#118

Open

lauralt force-pushed the state_no_ev branch 3 times, most recently from b938fd1 to ef4d66f Compare September 16, 2021 15:28

alexandruag previously approved these changes Sep 20, 2021

View reviewed changes

andreeaflorescu reviewed Sep 23, 2021

View reviewed changes

crates/vm-superio-ser/Cargo.toml Show resolved Hide resolved

crates/vm-superio-ser/Cargo.toml Show resolved Hide resolved

lauralt dismissed alexandruag’s stale review via 1d2b5f9 September 24, 2021 13:45

lauralt force-pushed the state_no_ev branch 2 times, most recently from 1d2b5f9 to 52ec6a3 Compare September 24, 2021 13:49

andreeaflorescu reviewed Oct 4, 2021

View reviewed changes

lauralt force-pushed the state_no_ev branch 2 times, most recently from 531b958 to 2e8dbb0 Compare October 4, 2021 17:14

andreeaflorescu reviewed Oct 4, 2021

View reviewed changes

lauralt added 4 commits October 4, 2021 22:16

create vm-superio-ser

b7cd601

This crate provides state objects that add serialization and versionize capabilities to the ones from vm-superio. Signed-off-by: Laura Loghin <lauralg@amazon.com>

update README with the save/restore state support

ab6d86c

Signed-off-by: Laura Loghin <lauralg@amazon.com>

lauralt force-pushed the state_no_ev branch from 2e8dbb0 to ab6d86c Compare October 4, 2021 19:58

[rtc] fix potential overflow caused by offset

b45d19c

Since now the offset & LR can be set from outside of the Rtc implementation, we need to add a checked add when computing the value of the RTC. Added a test & fix for the overflow. Signed-off-by: Andreea Florescu <fandree@amazon.com>

lauralt force-pushed the state_no_ev branch from 17a2798 to b45d19c Compare October 5, 2021 08:20

andreeaflorescu approved these changes Oct 5, 2021

View reviewed changes

alexandruag approved these changes Oct 5, 2021

View reviewed changes

lauralt merged commit 9b98030 into rust-vmm:main Oct 5, 2021

lauralt mentioned this pull request Oct 12, 2021

prepare for publish vm-superio(-ser) #70

Merged

sabin-rapan mentioned this pull request Nov 16, 2021

Add save/restore for Serial device #73

Merged

		/// Creates a new `AMBA PL031 RTC` instance from a given `state` and invokes the `rtc_events`
		/// implementation of `RtcEvents` during operation.

		// Check that the restored `Rtc` doesn't keep the state of the old `metrics` object.
		assert_eq!(rtc_from_state.events.invalid_write_count.count(), 0);

add RtcState object #65

add RtcState object #65

Uh oh!

Conversation

lauralt commented Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lauralt commented Sep 13, 2021 •

edited

Loading