rust-vmm / vm-virtio

virtio implementation
Apache License 2.0
372 stars 89 forks source link

Provide a single Virtio Queue implementation #143

Closed andreeaflorescu closed 2 years ago

andreeaflorescu commented 2 years ago

Currently we have 2 objects in vm-virtio that are providing mostly the same implementation: Queue and QueueGuard. The later was added in the context of supporting multi threaded queues, but its implementation is fairly similar to the one of the Queue.

This issue tracks having a single implementation that can be used by all customers. Currently Alibaba uses QueueGuard, while Cloud Hypervisor uses Queue.

More details TBD.

alexandruag commented 2 years ago

For a bit more context, Queue and QueueState are in principle wrappers around some QueueState information and a handle to a memory object to perform the relevant accesses, with some differences: Queue owns the inner state object (i.e. it contains an S: QueueStateT) and the M in queue is an M: GuestAddressSpace (which means that whenever an operation in Queue needs an M: GuestMemory it will go through the indirection of calling the GuestAddressSpace::memory to get an eventually-ephemeral handle). On the other hand, QueueGuard right now behaves as a sort of both wrapper and reference; the M within is just something that derefs to a target type that implements GuestMemory, and the S within is just something that derefs to a QueueState ( in particular, it cannot own a state object unless it’s wrapped in something like an Arc, Rc, etc.).

Looks like what we want to do a bunch of stuff here. First see, out of all these abstractions, which ones do we want/need to keep. For example, there were discussions about getting rid of the current Queue, and turning what is now QueueState into Queue. We should also try to improve naming as part of this discussion. Last but not least, we need to improve the documentation to provide a clearer picture of the design and how abstractions are supposed to be used and interact with each other.

andreeaflorescu commented 2 years ago

FIxed by #150