add QueueStateSer

[lauralt]

Summary of the PR

Based on the design agreed here (and similar to what is already done in vm-superio), created a new crate, virtio-queue-ser, and added QueueStateSer. Once we have a fix for #143, this PR will probably need some updates, but high level it should remain the same. Marking this as draft until that.

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

• All commits in this PR are signed (with git commit -s), and the commit
  message has max 60 characters for the summary and max 75 characters for each
  description line.
• All added/changed functionality has a corresponding unit/integration
  test.
• Any newly added unsafe code is properly documented.

[lauralt]

Since it is actually going to take some time to agree on the path forward with #150, I propose we review/merge this one before, and I can take care afterwards of any other changes to the state (if there will be any) that can be needed after the interface changes. Marking this as ready for review.

[andreeaflorescu]

I think we should completely separate state from functionality, as we already stated in the generic save/restore documentation. The fact that this is wrapping is a a matter of function, not state. In theory we should be able to save this directly as an u16.

[lauralt]

done

[andreeaflorescu]

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?

[lauralt]

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. The QueueStates fields are public, which is okay for a state object, but since the QueueState is actually the Queue, 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 a Queue from a QueueState, do all of the checks from those setters. That way, the code in this file wouldn't require any checks.

[acatangiu]

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 actual Queue is built, since without GuestMemory 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.

[lauralt]

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 actual Queue is built, since without GuestMemory most of the state doesn't make any sense.

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 the where should I check stuff discussion would be much easier to have.

Then 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.

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 to is_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.

[lauralt]

This is how I imagine the conversion would look like after the interface change that was mentioned:

impl Queue {
    pub fn from_state(state: &QueueState) -> Self {
        let mut queue = Queue {
            max_size: state.max_size,
            next_avail: state.next_avail,
            next_used: state.next_used,
            event_idx_enabled: state.event_idx_enabled,
            signalled_used: state.signalled_used,
            size: state.max_size,
            ready: state.ready,
            desc_table: GuestAddress(DEFAULT_DESC_TABLE_ADDR),
            avail_ring: GuestAddress(DEFAULT_AVAIL_RING_ADDR),
            used_ring: GuestAddress(DEFAULT_USED_RING_ADDR),
        };
        queue.set_size(state.size);
        queue.set_desc_table_address(..);
         ...
        queue
    }
}

[acatangiu]

Lgtm! But as a heads-up, you might have an easier time if you created a QueueBuilder object where you'd have fn 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 and Y cannot be done in set_x or set_y (for example this check can't be done in a setter, but can be done in qbuilder.build() or queue.is_valid()).

[lauralt]

Lgtm! But as a heads-up, you might have an easier time if you created a QueueBuilder object where you'd have fn 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.

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; calling build() will still be optional).

Validations depending on X and Y cannot be done in set_x or set_y (for example this check can't be done in a setter, but can be done in qbuilder.build() or queue.is_valid()).

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.

[acatangiu]

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.

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.

[andreeaflorescu]

I personally dislike the pattern with is_valid. The callers of the interface should not know that they have to call is_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:

• what can malicious actors do
• what can happen because of programming errors

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.

[lauralt]

Added some more clarifications in code comments.

[andreeaflorescu]

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?