-
Notifications
You must be signed in to change notification settings - Fork 105
add QueueStateSer #149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
add QueueStateSer #149
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
[package] | ||
name = "virtio-queue-ser" | ||
version = "0.1.0" | ||
authors = ["rust-vmm AWS maintainers <[email protected]>"] | ||
description = "Serialization for virtio queue state" | ||
repository = "https://github.com/rust-vmm/vm-virtio" | ||
keywords = ["queue", "serialization", "versioning"] | ||
readme = "README.md" | ||
license = "Apache-2.0 OR BSD-3-Clause" | ||
edition = "2018" | ||
|
||
[dependencies] | ||
serde = { version = ">=1.0.27", features = ["derive"] } | ||
versionize = ">=0.1.6" | ||
versionize_derive = ">=0.1.3" | ||
virtio-queue = { path = "../../crates/virtio-queue" } | ||
vm-memory = "0.7.0" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
// Copyright 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
// | ||
// SPDX-License-Identifier: Apache-2.0 OR BSD-3-Clause | ||
|
||
//! Adds serialization capabilities to the state objects from `virtio-queue`. | ||
//! | ||
//! Provides wrappers over the state objects from `virtio-queue` crate that | ||
//! implement the `Serialize`, `Deserialize` and `Versionize` traits. | ||
|
||
#![deny(missing_docs)] | ||
|
||
mod state; | ||
|
||
pub use state::QueueStateSer; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,126 @@ | ||
// Copyright 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
// | ||
// SPDX-License-Identifier: Apache-2.0 OR BSD-3-Clause | ||
|
||
use std::num::Wrapping; | ||
|
||
use serde::{Deserialize, Serialize}; | ||
use versionize::{VersionMap, Versionize, VersionizeResult}; | ||
use versionize_derive::Versionize; | ||
use virtio_queue::QueueState; | ||
use vm_memory::GuestAddress; | ||
|
||
/// Wrapper over a `QueueState` that has serialization capabilities. | ||
#[derive(Clone, Debug, Deserialize, PartialEq, Serialize, Versionize)] | ||
pub struct QueueStateSer { | ||
/// The maximum size in elements offered by the device. | ||
pub max_size: u16, | ||
/// Tail position of the available ring. | ||
pub next_avail: u16, | ||
/// Head position of the used ring. | ||
pub next_used: u16, | ||
/// VIRTIO_F_RING_EVENT_IDX negotiated. | ||
pub event_idx_enabled: bool, | ||
/// The last used value when using VIRTIO_F_EVENT_IDX. | ||
pub signalled_used: Option<u16>, | ||
/// The queue size in elements the driver selected. | ||
pub size: u16, | ||
/// Indicates if the queue finished with the configuration. | ||
pub ready: bool, | ||
/// Guest physical address of the descriptor table. | ||
pub desc_table: u64, | ||
/// Guest physical address of the available ring. | ||
pub avail_ring: u64, | ||
/// Guest physical address of the used ring. | ||
pub used_ring: u64, | ||
} | ||
|
||
// The following `From` implementations can be used to convert from a `QueueStateSer` to the | ||
// `QueueState` from the base crate and vice versa. | ||
// WARNING: They don't check for any invalid data, that would otherwise be checked when initializing | ||
// a queue object from scratch (for example setting a queue size greater than its max possible | ||
// size). The problem with the current `QueueState` implementation is that it can be used as the | ||
// queue object itself. `QueueState`'s fields are public, which allows the user to set up and use an | ||
// invalid queue. Once we fix https://github.com/rust-vmm/vm-virtio/issues/143, `QueueState` will be | ||
// renamed to `Queue`, its fields will no longer be public, and `QueueState` will consist of the | ||
// actual state. This way we can also check against any invalid data when trying to get a `Queue` | ||
// from the state object. | ||
// Nevertheless, we don't make any assumptions in the virtio-queue code about the queue's state that | ||
// would otherwise result in a panic, when initialized with random data, so from this point of view | ||
// these conversions are safe to use. | ||
impl From<&QueueStateSer> for QueueState { | ||
fn from(state: &QueueStateSer) -> Self { | ||
QueueState { | ||
max_size: state.max_size, | ||
next_avail: Wrapping(state.next_avail), | ||
next_used: Wrapping(state.next_used), | ||
event_idx_enabled: state.event_idx_enabled, | ||
signalled_used: state.signalled_used.map(Wrapping), | ||
size: state.size, | ||
ready: state.ready, | ||
desc_table: GuestAddress(state.desc_table), | ||
avail_ring: GuestAddress(state.avail_ring), | ||
used_ring: GuestAddress(state.used_ring), | ||
} | ||
} | ||
} | ||
|
||
impl From<&QueueState> for QueueStateSer { | ||
fn from(state: &QueueState) -> Self { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if there is anything that we need to check here. Is there any invalid state that would otherwise not be able to get into when just initializing a Queue from scratch? |
||
QueueStateSer { | ||
max_size: state.max_size, | ||
next_avail: state.next_avail.0, | ||
next_used: state.next_used.0, | ||
event_idx_enabled: state.event_idx_enabled, | ||
signalled_used: state.signalled_used.map(|value| value.0), | ||
size: state.size, | ||
ready: state.ready, | ||
desc_table: state.desc_table.0, | ||
avail_ring: state.avail_ring.0, | ||
used_ring: state.used_ring.0, | ||
} | ||
} | ||
} | ||
|
||
impl Default for QueueStateSer { | ||
fn default() -> Self { | ||
QueueStateSer::from(&QueueState::default()) | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
|
||
#[test] | ||
fn test_state_ser() { | ||
const SOME_VALUE: u16 = 16; | ||
|
||
let state = QueueState { | ||
max_size: SOME_VALUE * 2, | ||
next_avail: Wrapping(SOME_VALUE - 1), | ||
next_used: Wrapping(SOME_VALUE + 1), | ||
event_idx_enabled: false, | ||
signalled_used: None, | ||
size: SOME_VALUE, | ||
ready: true, | ||
desc_table: GuestAddress(SOME_VALUE as u64), | ||
avail_ring: GuestAddress(SOME_VALUE as u64 * 2), | ||
used_ring: GuestAddress(SOME_VALUE as u64 * 4), | ||
}; | ||
|
||
let ser_state = QueueStateSer::from(&state); | ||
let state_from_ser = QueueState::from(&ser_state); | ||
|
||
// Check that the old and the new state are identical when using the intermediate | ||
// `QueueStateSer` object. | ||
assert_eq!(state, state_from_ser); | ||
|
||
// Test the `Default` implementation of `QueueStateSer`. | ||
let default_queue_state_ser = QueueStateSer::default(); | ||
assert_eq!( | ||
QueueState::from(&default_queue_state_ser), | ||
QueueState::default() | ||
); | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there is anything that we need to check here. Is there any invalid state that would otherwise not be able to get into when just initializing a Queue from scratch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When initializing a Queue from scratch, there are some checks that are done on the queue size, on the addresses of the queue components (in their corresponding setters). So yes, the state here can be invalid if the user is initializing it with random values.
This is actually a problem at the
QueueState
level and related to all that confusion around the queue interfaces. TheQueueState
s fields are public, which is okay for a state object, but since theQueueState
is actually theQueue
, the fact that its fields are public is no longer good.I guess for now I can bring the logic from those setters here as well, but that won't fix the entire problem, and is not a clean fix either. I think the clean fix would be to have the
QueueState
->Queue
interface update (and not having the queue fields public), and when trying to get aQueue
from aQueueState
, do all of the checks from those setters. That way, the code in this file wouldn't require any checks.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why just wonder and not reason about it?
For example, my thinking is even if some of the fields could be sanity checked (
size <= max_size
for example), I suggest deferring validations to when the actualQueue
is built, since withoutGuestMemory
most of the state doesn't make any sense.Once you have the
Queue
, you can rightly call its is_valid() method to make sure all is kosher.I too am lazy to point out exactly what extra checks
is_valid()
would need when also considering deserialization, but once you get there you can look at Firecracker impl to get an idea.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is sort of my suggestion as well. The thing here is the way the queue interfaces are right now, you can more easily use
QueueState
as the Queue object. The queue object doesn't need to have a memory handle in its composition. This is basically one of the things we want to update, and after that thewhere should I check stuff
discussion would be much easier to have.In virtio-queue,
is_valid
is no longer doing all those checks that are done in Firecracker. The checks that are part of the spec were moved to the corresponding setters of those fields, because as opposed tois_valid
, which is optional for the user to call, the setters are not (when starting from scratch). But yes, this is the way to do the validations after deserialization.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how I imagine the conversion would look like after the interface change that was mentioned:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm! But as a heads-up, you might have an easier time if you created a
QueueBuilder
object where you'd havefn with_x(self, x) -> Self
instead of setters you mention above.The advantage is that ultimately you have
fn build(self) -> Result<Queue, Error<Self>>
that can do more complex validations that cannot be done in simple setters.Validations depending on
X
andY
cannot be done inset_x
orset_y
(for example this check can't be done in a setter, but can be done inqbuilder.build()
orqueue.is_valid()
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We thought about using the builder pattern before 0.1.0, but it's a bit incompatible with how the queue is constructed when using for example MMIO writes (and there won't be any
build()
in this case; callingbuild()
will still be optional).Not all de validations are supposed to happen in the setters, but what we are aiming with this is not having the MUSTs from the spec as optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! Validating in the setters against the spec is great!
I'm just saying as a heads-up that, on the slightly orthogonal subject of deserialization, you will need some extra validations that you can't do in setters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally dislike the pattern with
is_valid
. The callers of the interface should not know that they have to callis_valid
. But this should be part of a bigger refactoring effort.If you're using the current interface correctly (i.e. you don't set arbitrary values to the queue fields, but just use the setters), then the Queue that you are getting is a valid one. From my point of view, this is something that we need to check when initializing from a saved state as well. We can split the checks in 2 categories:
Right now with the current interface is really hard to guard against programming errors because the interface is too open (i.e. with the public fields). Nevertheless, we should have checks for things that can be initialized by malicious actors. As per the threat model the deserialized state is not trusted because we cannot know where it originates from. If we cannot address this in this PR it's fine, but we should know about this shortcoming and at least document it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some more clarifications in code comments.