vsock saga, episode 1: vhost-free vsock device #1106

dhrgit · 2019-05-26T17:09:23Z

Issue: #650

Description of changes

Removed experimental vhost-based vsock support;
Added a vhost-less vsock device;
Added a generic vsock backend trait;
Implemented the vsock packet assembly.

Note: the vsock series is now split across this PR, together with #1175 and #1176, for a leaner review process. Considering that the experimental, vhost-based, vsock functionality gets removed here, the entire series should get merged as closely as possible.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

petreeftime · 2019-05-27T07:57:20Z

Cargo.toml

@@ -28,7 +28,4 @@ panic = "abort"
 lto = true
 panic = "abort"

-[features]
-vsock = ["api_server/vsock", "jailer/vsock"]


How difficult would it be to keep the vsock-vhost implementation as an alternative to vsock-unixsockets?

Well, aside from the considerable implementation and maintenance effort, supporting two different implementations for the same feature would not really align well with our tenets.

petreeftime · 2019-05-27T08:03:33Z

devices/src/virtio/vsock/mod.rs

+    pub const NUM_QUEUES: usize = 3;
+    /// Virtio queue sizes, in number of descriptor chain heads.
+    /// There are 3 queues for a virtio device (in this order): RX, TX, Event
+    pub const QUEUE_SIZES: &[u16] = &[256; NUM_QUEUES];


The queue sizes shoud be [256, 256, 2]. There's no reason for allocating 256 descriptors for the reset event.

We aren't allocating anything, though. Those are just the max sizes that the driver can choose to allocate.

devices/src/virtio/vsock/mod.rs

petreeftime · 2019-05-27T08:04:23Z

devices/src/virtio/vsock/mod.rs

+        ///
+        /// The device processes available buffers in the same order in which the device
+        /// offers them.
+        pub const VIRTIO_F_IN_ORDER: usize = 35;


Is this required?

This one and the next are virtio device independent feature bits, so maybe we should get them out of the Vsock device related modules (they might already be defined somewhere else as well) for neatness reasons.

Yeah, I considered that, especially since all these UAPI defs should be neatly stored somewhere else, but that would require some clean up effort throughout all our devices.

nit: at least leave a TODO for this, someone might do it when they're bored if they see it when browsing code

petreeftime · 2019-05-27T09:39:20Z

devices/src/virtio/vsock/device.rs

+                defs::NUM_QUEUES,
+                queues.len()
+            );
+            return Err(ActivateError::BadActivate);


I think this leas to the VMM exiting. I am not sure this is the expected behavior on driver initialization failure. I think the device can be reset and re-initialized.

Yea we don't support device initialization failures/resets/etc right now, and you're right it error causes an exit. We plan to get rid of it as we ingest more rust-vmm components, but that might change if there's a pressing need along the way.

petreeftime · 2019-05-27T09:41:16Z

devices/src/virtio/vsock/device.rs

+        mut queue_evts: Vec<EventFd>,
+    ) -> ActivateResult {
+        if queues.len() != defs::NUM_QUEUES || queue_evts.len() != defs::NUM_QUEUES {
+            error!(


I would argue that this is misbehavior from the guest and might be worth a warning, but not an error. I think errors should be reserved for non-recoverable states of the VMM.

The only current practical implication of the log level is whether a message actually shows up as you restrict what actually gets written to the log file. We see this as pretty important given the use cases we know of, so we've made it (and similar events) an error. Things might definitely change in the future.

devices/src/virtio/vsock/epoll_handler.rs

devices/src/virtio/mod.rs

petreeftime · 2019-05-29T12:12:41Z

vmm/src/vmm_config/vsock.rs

@@ -13,6 +13,8 @@ pub struct VsockDeviceConfig {
    pub id: String,
    /// A 32-bit Context Identifier (CID) used to identify the guest.
    pub guest_cid: u32,
+    /// Path to local unix socket.
+    pub uds_path: String,


This should be a VsockBackend instead.

I agree. However, currently, there's no clean way of handling these config structs properly. The time is close when we'll have to, though, and this will get solved.

vmm/src/lib.rs

petreeftime · 2019-05-29T12:15:03Z

vmm/src/vmm_config/vsock.rs

@@ -13,6 +13,8 @@ pub struct VsockDeviceConfig {
    pub id: String,


Not clear what the purpose of this is.

It's just a device identifier, since currently there's support for multiple devices, even if the Linux driver doesn't.

petreeftime · 2019-05-29T13:22:26Z

devices/src/virtio/vsock/device.rs

+    backend: Option<B>,
+    avail_features: u64,
+    acked_features: u64,
+    config_space: Vec<u8>,


Why is this needed? There's no reason for a guest to write random data to the config_space of this device.

Indeed, the device specific configuration space for vsock appears to only contain the guest_cid. If we don't want to allow guests to change that (which is also the simpler thing to do for now), we can remove this member (as it seems the read_config function only makes use of cid anyway), and not do any actual change for write_config.

alexandruag

Posting/updating some comments from a while back regarding the first part of the PR.

alexandruag · 2019-05-28T09:33:56Z

tests/integration_tests/build/test_style.py

@@ -102,13 +102,3 @@ def test_firecracker_swagger():
        os.path.join(os.getcwd(), '../api_server/swagger/firecracker.yaml')
    )
    check_swagger_style(yaml_spec)
-
-
-def test_experimental_firecracker_swagger():


Maybe it would make sense for the test to declare itself as skipped if the experimental swagger file does not exist. I imagine we might bring it back in the future if we try out new experimental features, and we'll want the check to happen again at that point.

I agree we may bring an experimental swagger file back at some point, but I think that's not enough reason to keep an unused test hanging around. I think it's a pretty straight-forward test to write, once we'll need it (mostly copy-paste from the stable version).

api_server/swagger/firecracker.yaml

devices/src/virtio/vsock/device.rs

alexandruag · 2019-06-22T11:22:25Z

devices/src/virtio/mod.rs

@@ -49,6 +51,7 @@ pub const NOTIFY_REG_OFFSET: u32 = 0x50;
 pub enum ActivateError {
    EpollCtl(IOError),
    BadActivate,
+    VsockError(self::vsock::VsockError),


Sorry, couldn't notice from the code, why is VsockError a special variant of ActivateError?

Oops, that's a leftover from an old version of the code - will remove it.

vmm/src/lib.rs

alexandruag · 2019-06-22T11:39:07Z

devices/src/virtio/vsock/epoll_handler.rs

+//                } else {
+//                    warn!("vsock: unexpected backend event flags={:08x}", evset_bits);
+//                }
+                self.backend.notify(epoll::Events::EPOLLIN);


Shouldn't it be self.backend.notify(evset); here?

Yup, it's fixed.

alexandruag · 2019-06-22T12:09:27Z

devices/src/virtio/vsock/packet.rs

+    ///
+    /// Warning: Both `ptr` and `len` must be insured as valid by the caller.
+    ///
+    fn from_fat_ptr_unchecked(ptr: *const u8, len: usize) -> Self {


I think we can remove the _fat part from the name, as there's no risk for confusion.

This whole code is gone now.

devices/src/virtio/vsock/packet.rs

petreeftime · 2019-07-16T08:30:57Z

devices/src/virtio/vsock/packet.rs

+///
+pub struct VsockPacket {
+    hdr: *mut u8,
+    buf: Option<*mut u8>,


I have some general concerns with these pointers to memory: the backing memory could definitely go away at some point before the VsockPacket disappears. VsockPacket should not exceed the lifetime of the backing memory or all the operations done to it should be otherwise unsafe.

There is a comment around the unsafe code, but it's not entirely accurate.
It's true that you're using it in a way that's safe, as far as I can tell, but in the general case, this breaks memory safety rules, as the compiler can't tell if hdr and buf still point to valid memory.

// This is safe since bound checks have already been performed when creating the packet // from the virtq descriptor.

I think adding a lifetime parameter to this type would improve things.

Are you referring to the backing memory going away once the virtio buffers are marked used, and thusly their ownership returned to the driver? If that's so, I think a rigorously correct solution would have us synchronize the lifetime of virtio buffer wrappers (i.e. DescriptorChain) and VsockPacket in a non-trivial manner. That would require a considerable amount of work on our virtio handling code.

If you were referring to something else, could you please dive into a bit more detail? Thanks!

No, that's exactly right. The lifetime of both hdr and buf depends on the logical lifetime of associated virtqueue descriptors. I haven't looked into an exact solution, but there should be some lifetime dependency between the VsockPacket and the memory it has pointers to. Not sure how easy that would be considering you hold direct pointers to memory instead of references.
Maybe something like this would work?

pub struct VsockPacket<'a, 'b> { hdr: UnsafeCell<u8>, buf: Option<UnsafeCell<u8>>, phantom_hdr: PhantomData<&'a u8>, phantom_buf: PhantomData<&'b u8>, }

There's probably something even simpler than this.

Well, then I don't think rust lifetimes are going to help, since descriptor lifetimes are not known at compilation time. I.e. the data pointed to by a descriptor lives up until the moment we add that descriptor to the used vring.

If I understand you correctly, you are referring to the rust lifetimes explicitly set for DescriptorChain (please correct me if I misunderstood). Those are actually not what they might appear to be, though. I.e. they aren't connected to the data lifetime (since, as I was saying, that can only be known at runtime), but are just there so DescriptorChain can hold a reference to GuestMemory. That pattern itself needs to be cleaned up, since references to GuestMemory make little sense (GuestMemory is just an Arc, so a reference to it is basically a statically-tracked reference to a dynamically-tracked reference).

The descriptors themselves no, but the wrapping structure does have a lifetime.

let used_len = match VsockPacket::from_rx_virtq_head(&head)

This creates a new VsockPacket from &head by extracting the internal memory region of head and the next descriptor. However, after this creation succeeds, VsockPacket is completely disconnected from head, instead of owning a reference to it with a proper lifetime to it. So, you could add it to the used queue and the memory gets reclaimed by the guest, while the VsockPacket is still alive and usable from the VMM.

Speaking of which:

self.rxvq.add_used(&self.mem, head.index, used_len);

This needs to consume head and expire its lifetime instead of using head.index.

I think I'll turn this into a larger issue instead of commenting on this particular PR.

dianpopa

vsock saga: Review v1
(Reached final commit)

tests/conftest.py

tests/integration_tests/functional/test_api.py

vmm/src/default_syscalls/mod.rs

vmm/src/lib.rs

dianpopa · 2019-08-09T15:33:49Z

devices/src/virtio/vsock/packet.rs

+        LittleEndian::read_u64(&self.hdr()[HDROFF_SRC_CID..])
+    }
+
+    pub fn set_src_cid(&mut self, cid: u64) -> &mut Self {


I would use references here (i.e cid: &u64) to avoid copying of u64 and then dereferencing.
It applies to all of the above args passed by value.

A reference to an u64 would be another u64 (pointers are 64-bit wide on 64 bit architectures). Even on 32-bit, though, we still wouldn't want to force passing 64-bit args by reference, since that would imply a memory access. Large args can be passed by value more efficiently, via various regs, without touching the memory. E.g. the compiler may choose to use AVX-512 registers to pass an argument that's 512-bit wide.

Passing by value vs by reference is actually decided by the compiler, when applying compile-time optimizations. Using & is a higher-level abstraction, telling rust we want to borrow, not pass ownership. The compiler then decides the optimal way to pass the argument.

devices/src/virtio/vsock/epoll_handler.rs

dianpopa · 2019-08-12T14:36:09Z

devices/src/virtio/vsock/epoll_handler.rs

@@ -142,6 +213,10 @@ where
            }
        }

+        if raise_irq {
+            self.signal_used_queue().unwrap_or_default();


Why not self.signal_used_queue()?

Rust warns you when you're not handling Results in any way. This seems like a hack to silence the compiler about it. However, is there any good reason why this shouldn't return the error? At least logging such an event would be a better solution than simply failing silently.

The error is already logged by signal_used_queue().

I was not referring to logging the error but to propagating it. Right now the unwrap_or_default is silencing it in case it returns error.
Is that what you want?
The net device is propagating it: https://github.com/firecracker-microvm/firecracker/blob/master/devices/src/virtio/net.rs#L581.

devices/src/virtio/vsock/packet.rs

dianpopa · 2019-08-13T14:12:45Z

devices/src/virtio/vsock/device.rs

+        assert_eq!(data, [0u8, 1, 2, 3, 4, 5, 6, 7]);
+
+        // Just covering lines here.
+        ctx.device.write_config(0, &data[..4]);


I'd actually check that read_config returns the same value after calling write_config.

The vsock device has no writable config data. The only config available is the device CID, and that's not writable by the guest driver. However, the VirtioDevice trait does require a write_config() function, hence the "just covering lines" comment. I've amended the comment to make this more clear.

Yes, yes, no writable config data for vsock which is exactly what I was implying with my comment. Since the write_config is supposed to not change the config, I would do:

x = read_config; write_config(smth); y = read_config; assert_eq(x,y);

You are checking that write_config for the vsock device does not actually write the config data (which in this case consists only of the CID).

dianpopa · 2019-08-13T14:24:27Z

devices/src/virtio/vsock/device.rs

+        }
+
+        // Test a correct activation.
+        ctx.device


Since these are unit tests why not use a explicit assert(i.e

assert!(ctx.device .activate( ctx.mem.clone(), EventFd::new().unwrap(), Arc::new(AtomicUsize::new(0)), vec![ VirtQueue::new(256), VirtQueue::new(256), VirtQueue::new(256), ], vec![ EventFd::new().unwrap(), EventFd::new().unwrap(), EventFd::new().unwrap(), ], ) .is_ok());

)?

I don't really see how the two are different. Can you elaborate on why this is preferable, please?

I prefer to use asserts in unit tests for verbosity. See below.
Output when test fails and you use unwrap

---- virtio::vsock::device::tests::test_virtio_device stdout ---- thread 'virtio::vsock::device::tests::test_virtio_device' panicked at 'called `Result::unwrap()` on an `Err` value: BadActivate', src/libcore/result.rs:997:5

Output when test fails and you use assert!:

---- virtio::vsock::device::tests::test_virtio_device stdout ---- thread 'virtio::vsock::device::tests::test_virtio_device' panicked at 'assertion failed: ctx.device.activate(ctx.mem.clone(), EventFd::new().unwrap(), Arc::new(AtomicUsize::new(0)), vec![VirtQueue :: new ( 256 ) , VirtQueue :: new ( 256 ) , VirtQueue :: new ( 256 ) ,], vec![EventFd :: new ( ) . unwrap ( ) , EventFd :: new ( ) . unwrap ( ) , EventFd :: new ( ) . unwrap ( ) ,]).is_ok()', devices/src/virtio/vsock/device.rs:301:9

However, this is a personal preference which I do not feel strongly about.

devices/src/virtio/vsock/device.rs

devices/src/virtio/vsock/packet.rs

devices/src/virtio/vsock/mod.rs

devices/src/virtio/vsock/packet.rs

alexandruag

Just a couple of nits I forgot about before. Looks good otherwise; needs rebasing.

alexandruag · 2019-08-14T17:03:00Z

tests/integration_tests/functional/test_api.py

-        vsock_id='vsock2',
-        guest_cid=16
+        guest_cid=15,
+        uds_path='vsock.sock'


v.sock sounds so much cooler. There are some other occurrences which could benefit from this change as well.

alexandruag · 2019-08-24T18:32:10Z

devices/src/virtio/vsock/epoll_handler.rs

@@ -111,6 +119,20 @@ where
            }
            defs::BACKEND_EVENT => {
                debug!("vsock: backend event");
+//                if let Some(evset) = epoll::Events::from_bits(evset_bits) {


Would be a nice touch to remove these unused lines from the current commit as well.

Prepared the way for the new AF_UNIX-backed vsock device by: * removing old vhost code; and * removing the "vsock" feature. Signed-off-by: Dan Horobeanu <[email protected]>

Updated the devices crate to use Rust edition 2018 features. This was done in preparation for the new vsock device, the code for which will make use of some nifty 2018 features, such as non-lexical lifetimes. Signed-off-by: Dan Horobeanu <[email protected]>

Added the skeleton for a new vsock device, that will use Unix sockets as a backend, instead of vhost. Signed-off-by: Dan Horobeanu <[email protected]>

Added the definition for a generic backend trait, that could be used to possibly offload vsock traffic to something other than Unix sockets. Signed-off-by: Dan Horobeanu <[email protected]>

Added code to handle reading and writing vsock packets from/to the virtio queues. Signed-off-by: Dan Horobeanu <[email protected]>

Added unit tests for: - the vsock virtio device implementation - the vsock epoll handler - the vsock packet handling interface Signed-off-by: Dan Horobeanu <[email protected]>

dhrgit · 2019-09-02T16:26:16Z

Closed as a subset of #1176 .

dhrgit self-assigned this May 26, 2019

This was referenced May 26, 2019

[RFC] vsock Design Doc #1044

Closed

[Devices] virtio vsock #650

Closed

petreeftime reviewed May 27, 2019

View reviewed changes

devices/src/virtio/mod.rs Show resolved Hide resolved

petreeftime reviewed May 29, 2019

View reviewed changes

vmm/src/lib.rs Show resolved Hide resolved

petreeftime reviewed May 29, 2019

View reviewed changes

dhrgit force-pushed the vsock branch 2 times, most recently from 4ccf155 to 636e4a9 Compare June 8, 2019 14:14

dhrgit force-pushed the vsock branch 3 times, most recently from 7c28a35 to 41e214d Compare June 20, 2019 08:03

alexandruag reviewed Jun 22, 2019

View reviewed changes

dhrgit force-pushed the vsock branch 2 times, most recently from 2861dcd to 4971ff2 Compare July 13, 2019 17:49

This was referenced Jul 13, 2019

vsock saga, episode 2: the connection state machine #1175

Closed

vsock saga, episode 3 (season finale): the unix sockets backend #1176

Merged

dhrgit added Feature: Emulation labels Jul 13, 2019

dhrgit force-pushed the vsock branch 3 times, most recently from 260f5ef to 60efa35 Compare July 14, 2019 14:02

petreeftime reviewed Jul 16, 2019

View reviewed changes

acatangiu changed the title ~~Vsock saga: the code is here~~ vsock saga, episode 1: vhost-free vsock device Jul 18, 2019

dhrgit force-pushed the vsock branch from 60efa35 to 3ba39e6 Compare July 22, 2019 22:10

dhrgit force-pushed the vsock branch 2 times, most recently from b5f09d0 to 4038418 Compare August 3, 2019 06:48

dianpopa mentioned this pull request Aug 5, 2019

vsock: add config_space on the basis of virtio spec #1197

Closed

dhrgit force-pushed the vsock branch from 4038418 to dadd9cc Compare August 6, 2019 00:58

dianpopa self-requested a review August 6, 2019 14:31

dianpopa mentioned this pull request Aug 9, 2019

Add metrics for the vsock device #1210

Closed

dianpopa reviewed Aug 9, 2019

View reviewed changes

dianpopa reviewed Aug 13, 2019

View reviewed changes

dhrgit force-pushed the vsock branch from dadd9cc to 2e3f32f Compare August 14, 2019 14:16

alexandruag reviewed Aug 24, 2019

View reviewed changes

dhrgit added 6 commits August 28, 2019 10:44

devices/vsock: Removed old vhost code

3baffda

Prepared the way for the new AF_UNIX-backed vsock device by: * removing old vhost code; and * removing the "vsock" feature. Signed-off-by: Dan Horobeanu <[email protected]>

devices/vsock: new vhost-less vsock device stub

fadd811

Added the skeleton for a new vsock device, that will use Unix sockets as a backend, instead of vhost. Signed-off-by: Dan Horobeanu <[email protected]>

devices/vsock: Added vsock backend trait

ba5e972

Added the definition for a generic backend trait, that could be used to possibly offload vsock traffic to something other than Unix sockets. Signed-off-by: Dan Horobeanu <[email protected]>

devices/vsock: Added packet assembly logic

376aa85

Added code to handle reading and writing vsock packets from/to the virtio queues. Signed-off-by: Dan Horobeanu <[email protected]>

devices/vsock: device unit tests

cbc0670

Added unit tests for: - the vsock virtio device implementation - the vsock epoll handler - the vsock packet handling interface Signed-off-by: Dan Horobeanu <[email protected]>

dhrgit force-pushed the vsock branch from 2e3f32f to cbc0670 Compare August 28, 2019 10:58

dhrgit closed this Sep 2, 2019

		@@ -13,6 +13,8 @@ pub struct VsockDeviceConfig {
		pub id: String,

vsock saga, episode 1: vhost-free vsock device #1106

vsock saga, episode 1: vhost-free vsock device #1106

Uh oh!

Conversation

dhrgit commented May 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petreeftime May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexandruag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petreeftime Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dhrgit commented May 26, 2019 •

edited

Loading

petreeftime May 29, 2019 •

edited

Loading

petreeftime Jul 17, 2019 •

edited

Loading