Add save/restore for Serial device #73
Conversation
crates/vm-superio-ser/src/serial.rs
Outdated
| } | ||
|
|
||
| #[test] | ||
| fn test_state_ser_constructor() { |
There was a problem hiding this comment.
It is, but without it kcov doesn't consider the public fields of the struct as covered by the test_state_ser_default() test above
There was a problem hiding this comment.
Hmm looks like this test doesn't even check some setters or some new() constructor, so it is indeed superfluous. I don't see how it could ever fail, so I would just remove it if it's okay for you too.
There was a problem hiding this comment.
I double checked and it turns out I was wrong. Removing the superfluous tests doesn't affect coverage. Will drop the commit.
crates/vm-superio/src/serial.rs
Outdated
| @@ -914,4 +1013,110 @@ mod tests { | |||
| // THR empty interrupt was raised nevertheless. | |||
| assert_eq!(iir & IIR_THR_EMPTY_BIT, IIR_THR_EMPTY_BIT); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_serial_state_constructor() { | |||
crates/vm-superio/src/serial.rs
Outdated
| }; | ||
|
|
||
| if serial.is_thr_interrupt_enabled() && serial.is_thr_interrupt_set() { | ||
| serial.trigger_interrupt().unwrap_or_default(); |
There was a problem hiding this comment.
Maybe a small comment why it's safe to unwrap and ignore?
There was a problem hiding this comment.
If we cannot trigger this interrupt, isn't the guest going to be stuck? I wonder if in this case it's not better to just return the Error so we can fail fast. Same for the following if.
There was a problem hiding this comment.
That's was my initial instinct as well, but I wasn't sure what's more desirable for Serial::from_state(): to fail fast or cope with programming errors. Thanks for the feedback. Will update the PR so that from_state() returns errors instead of being best effort.
crates/vm-superio/src/serial.rs
Outdated
| scratch: DEFAULT_SCRATCH, | ||
| in_buffer: VecDeque::new(), | ||
| /// * `state` - A reference to the state from which the `Serial` is constructed. | ||
| pub fn from_state(trigger: T, serial_evts: EV, out: W, state: &SerialState) -> Self { |
There was a problem hiding this comment.
The state should be the first parameter so that it's in line with the RTC device.
crates/vm-superio/src/serial.rs
Outdated
| /// * `state` - A reference to the state from which the `Serial` is constructed. | ||
| pub fn from_state(trigger: T, serial_evts: EV, out: W, state: &SerialState) -> Self { | ||
| let mut buffer = state.in_buffer.clone(); | ||
| buffer.truncate(FIFO_SIZE); |
There was a problem hiding this comment.
Nice way of insuring the buffer length 👍. We should document this behavior somewhere. One option is either in SerialState or in this constructor. If we end up returning errors from this function, we should also consider returning an error instead.
There was a problem hiding this comment.
Will change to return an error instead so that we're consistent in behaviour with trigger() failures.
crates/vm-superio/src/serial.rs
Outdated
| }; | ||
|
|
||
| if serial.is_thr_interrupt_enabled() && serial.is_thr_interrupt_set() { | ||
| serial.trigger_interrupt().unwrap_or_default(); |
There was a problem hiding this comment.
If we cannot trigger this interrupt, isn't the guest going to be stuck? I wonder if in this case it's not better to just return the Error so we can fail fast. Same for the following if.
crates/vm-superio/src/serial.rs
Outdated
| modem_control: DEFAULT_MODEM_CONTROL, | ||
| modem_status: DEFAULT_MODEM_STATUS, | ||
| scratch: DEFAULT_SCRATCH, | ||
| in_buffer: Vec::new(), |
There was a problem hiding this comment.
We can unify this test with test_from_state_with_too_many_bytes.
crates/vm-superio/src/serial.rs
Outdated
|
|
||
| #[test] | ||
| fn test_serial_state_default() { | ||
| let state = SerialState::default(); |
There was a problem hiding this comment.
I think it would be more useful if we instead create the serial from the default state (using with_events), then call save_state, and check that the state is the default_state using assert_eq (because SerialState derives PartialEq). I don't think we need to check the actual values because this is a duplication of the code.
sabin-rapan
force-pushed
the
main
branch
2 times, most recently
from
54abfbd
to
c83fb75
Compare
crates/vm-superio/src/serial.rs
Outdated
| @@ -304,7 +304,7 @@ impl<T: Trigger, W: Write> Serial<T, NoEvents, W> { | |||
| /// | |||
| /// You can see an example of how to use this function in the | |||
| /// [`Example` section from `Serial`](struct.Serial.html#example). | |||
| pub fn new(trigger: T, out: W) -> Serial<T, NoEvents, W> { | |||
| pub fn new(trigger: T, out: W) -> Result<Serial<T, NoEvents, W>, Error<T::E>> { | |||
There was a problem hiding this comment.
I think this should use unwrap and comment why the unwrap is safe so we don't need to change the existing API.
The SerialState struct will help aid us in instantiating Serial objects from a predefined state. It will be used in following commits for adding save/restore support. At the moment, we do not check if the state we are restoring from has an obscenely large buffer or if the interrupt should be asserted. These issues will be addressed in subsequent commits. Signed-off-by: Sabin Rapan <sabrapan@amazon.com>
This struct implements both Serialize/Deserialize and Versionize to aid saving/restoring the serial state in multiple formats. The data is then converted to SerialState which ultimately is consumed when creating another Serial object via its from_state() method. Signed-off-by: Sabin Rapan <sabrapan@amazon.com>
... by returning an Error if the input buffer is larger than FIFO_SIZE. Signed-off-by: Sabin Rapan <sabrapan@amazon.com>
To trigger a specific interrupt one needs first to enable said interrupt (in the Interrupt Enable Register), then the logic that triggers the interrupt will set the corresponding bit in Interrupt Identification Register, so that the driver can figure out what interrupt to handle. When it reads IIR, this causes the respective bit to be reset. We can leverage this same logic to retrigger interrupts when restoring from a given state by checking that both IER and IIR have been set for each supported interrupt kind. This way we avoid adding unnecessary fields in SerialState to do the bookkeeping of these pending interrupts. Signed-off-by: Sabin Rapan <sabrapan@amazon.com>
Just some basic tests, nothing fancy, to make sure our dependencies work with our structs like we would expect (and improve our code coverage). Signed-off-by: Sabin Rapan <sabrapan@amazon.com>
Signed-off-by: Sabin Rapan <sabrapan@amazon.com>
Add support for saving and restoring the state (registers) of the serial device. This follows the same pattern in RTC (#65) by adding
SerialStateandSerialStateSerstructs, where the former is a plain container for the data and the later mirrors it and derivesSerialize,DeserializeandVersionize.Both structures gives the caller full access to their fields. Apart from handling input buffers larger than
FIFO_SIZEand retriggering interrupts that haven't been consumed by the driver, the current proposal makes no sanity checks on the fields in the state struct. This means, for example, one could set the upper four bits inIERregister which is invalid.If such sanity checks are desired, we can adjust the PR with a follow-up commit.