add abstraction for the vsock packet

[lauralt]

We were thinking to start upstreaming the virtio vsock device, starting with the vsock packet. We already have some abstractions for the block device request, and we can go the same way for the vsock packet, i.e. have a new crate in vm-virtio: virtio-vsock.

For a bit of context, the virtio vsock packet is defined in the standard as having a header of type virtio_vsock_hdr and an optional data array of bytes. There are multiple operations that can be requested within a packet, e.g. VIRTIO_VSOCK_OP_RST for resetting the connection, VIRTIO_VSOCK_OP_RW for sending payload. Most operations are of the VIRTIO_VSOCK_OP_RW type, which means for data transfer, and the other ones are used for connection and buffer space management. data is non-empty only for the VIRTIO_VSOCK_OP_RW operations.

struct virtio_vsock_packet {
    struct virtio_vsock_hdr hdr;
    u8 data[];
};

Our proposal is to use vm-memory’s VolatileSlice, and define theVsockPacket structure as follows:

pub struct VsockPacket<'a, B: BitmapSlice> {
    header_slice: VolatileSlice<'a, B>,
    header: PacketHeader,
    data_slice: Option<VolatileSlice<'a, B>>,
}

The advantage with this approach is that we don’t tie the packet implementation to the GuestMemory interface (which would happen if we would use GuestAddresses instead of VolatileSlices). A packet can be configured from simply pointers to a slice, not necessarily from a GuestMemory object (e.g. the vm-virtio’s DescriptorChain), so with this approach we would support this use case too. A limitation of this approach is that it doesn’t cover the scenario where the header or data buffer doesn't fit in a single VolatileSlice because the guest memory regions of the buffer are contiguous in the guest physical address space, but not in the host virtual one as well. This is not currently happening for the known customers (Firecracker, Cloud Hypervisor); the vsock packet corresponds to only one region of the host virtual space. For the multiple VolatileSlices use case, we can later extend this solution to use an array of VolatileSlices for the header and data.

Design

When using a GuestMemory object, a VsockPacket instance is created by parsing a descriptor chain from either the TX virtqueue via from_tx_virtq_chain or the RX virtqueue via from_rx_virtq_chain. The VsockPacket API is also providing setters and getters for each virtio_vsock_hdr field (e.g. src_cid, dst_port, op), which are using methods of the VolatileSlice’s Bytes implementation for accessing parts of the header slice.

impl<'a, B: BitmapSlice> VsockPacket<'a, B> {
    /// Return a reference to the `header_slice` of the packet.
    pub fn header_slice(&self) -> &VolatileSlice<'a, B>  {}

    /// Return a reference to the `data_slice` buffer of the packet.
    pub fn data_slice(&self) -> Option<&VolatileSlice<'a, B>> {}

    /// Create the packet from a TX virtqueue chain.
    pub fn from_tx_virtq_chain<M, T>(mem: &'a M, desc_chain: &DescriptorChain<T>)
        -> Result<Self>
    where
        M: GuestMemory,
        <<M as GuestMemory>::R as GuestMemoryRegion>::B: WithBitmapSlice<'a, S = B>,
        T: Deref,    
        T::Target: GuestMemory,   
    {}

    /// Create the packet from an RX virtqueue chain.
    pub fn from_rx_virtq_chain<M, T>(mem: &'a M, desc_chain: &DescriptorChain<T>)
        -> Result<Self>
    where
        M: GuestMemory,
        <<M as GuestMemory>::R as GuestMemoryRegion>::B: WithBitmapSlice<'a, S = B>,
        T: Deref,    
        T::Target: GuestMemory
    {}

    /// Return the `src_cid` of the header.
    pub fn src_cid(&self) -> u64 {}

    /// Set the `src_cid` of the header.
    pub fn set_src_cid(&mut self, cid: u64) -> &mut Self {}

    ...
}

POC here. I'll add tests and more documentation, and open a PR with the proposal once we agree on the high level design.

Thoughts?

[imaginezz]

Is this crate has only the abstraction for VsocketPacket, or there are other abstractions?

[lauralt]

Is this crate has only the abstraction for VsocketPacket, or there are other abstractions?

By crate you mean virtio-vsock? There are no abstractions in it so far, but we plan on upstreaming more vsock building blocks (starting with the packet).

[imaginezz]

Is this crate has only the abstraction for VsocketPacket, or there are other abstractions?

By crate you mean virtio-vsock? There are no abstractions in it so far, but we plan on upstreaming more vsock building blocks (starting with the packet).

Sure, the packet is the common component of virtio-vsock, so move it to virtio-vsock is reasonable, but the behavior of other components (like muxer or csm in firecracker) may be different in other vmm, would you plan to move them to this crate?

[lauralt]

Is this crate has only the abstraction for VsocketPacket, or there are other abstractions?

By crate you mean virtio-vsock? There are no abstractions in it so far, but we plan on upstreaming more vsock building blocks (starting with the packet).

Sure, the packet is the common component of virtio-vsock, so move it to virtio-vsock is reasonable, but the behavior of other components (like muxer or csm in firecracker) may be different in other vmm, would you plan to move them to this crate?

Yeah these are more debatable, I have to think a bit more what would be the best approach with these, for the backend things I wouldn't add something in vm-virtio (or maybe some traits make sense, but anyway no implementation), and for the connection we could probably define some traits and have a default implementation for them (the connection module is pretty similar in firecracker and cloud-hypervisor). I'll dive deep a bit more into these in the following weeks, but I don't think we should block the packet upstreaming until that point.

[imaginezz]

Is this crate has only the abstraction for VsocketPacket, or there are other abstractions?

By crate you mean virtio-vsock? There are no abstractions in it so far, but we plan on upstreaming more vsock building blocks (starting with the packet).

Sure, the packet is the common component of virtio-vsock, so move it to virtio-vsock is reasonable, but the behavior of other components (like muxer or csm in firecracker) may be different in other vmm, would you plan to move them to this crate?

Yeah these are more debatable, I have to think a bit more what would be the best approach with these, for the backend things I wouldn't add something in vm-virtio (or maybe some traits make sense, but anyway no implementation), and for the connection we could probably define some traits and have a default implementation for them (the connection module is pretty similar in firecracker and cloud-hypervisor). I'll dive deep a bit more into these in the following weeks, but I don't think we should block the packet upstreaming until that point.

I agree, we can move on. And different vmm implementation can select some parts if it to use.

[sboeuf]

Looks good! I think you can go ahead and proceed!

[stsquad]

Can we document the lifetime requirements? Is there anything special going on here (given every part of the structure has the same lifetime) or is this just syntactic sugar to keep the compiler happy?

[lauralt]

Thank you everyone for having a look, and for the comments!

Regarding documenting the lifetime requirements, I'm thinking whether it would make sense to have this kind of documentation around how the dirty page tracking and all the bitmap objects interact with the VolatileSlices lifetime 🤔 (probably somewhere in vm-memory). In the meantime I opened https://github.com/rust-vmm/vm-virtio/pull/131. It still needs some improvements to docs, but I plan to do that in a separate PR.