Conversation
stefano-garzarella
left a comment
There was a problem hiding this comment.
Thanks for this PR, I did a quick review and left some comments.
I'll think a bit more about Transport implementation and Vsock, since I'd like to have more abstraction (e.g. be ready to support a new driver for HyperV vsock)
e0d157e to
82976c4
Compare
|
v2:
TODO:
|
82976c4 to
08ff85f
Compare
|
v3:
TODO:
|
08ff85f to
9f5553e
Compare
|
v4:
TODO:
|
ca7e076 to
3bc1d0b
Compare
|
v5:
TODO:
|
|
Looks like CI runner fails to create a vsock listening socket using ncat |
3bc1d0b to
f9a59be
Compare
|
Fixed CI failure. |
f9a59be to
31df19c
Compare
|
ef04aef to
a14dca2
Compare
|
Addressed all @stefano-garzarella comments:
|
| for _ in 0..MAX_RETRIES { | ||
| let candidate_port = | ||
| self.first_free_port | ||
| .fetch_update(Ordering::Relaxed, Ordering::Relaxed, |port| { |
There was a problem hiding this comment.
I'm not sure about this, I'd like some feedback from someone with more experience
a14dca2 to
0dd8620
Compare
|
3dcc259 to
831ed1c
Compare
|
Fixed CI failure because no-cc by default is now run as non-root and |
Define a VsockTransport trait to abstract vsock operations and enable multiple backend implementations. Signed-off-by: Luigi Leonardi <leonardi@redhat.com>
Commit c4719d3 dropped driver support for virtio-vsock because it was not being used. This reverts this commit because we are introducing vsock support. This revert does not add back the examples/aarch64/main.rs file as it is not needed. Signed-off-by: Luigi Leonardi <leonardi@redhat.com>
Apply lints suggested by clippy. Signed-off-by: Luigi Leonardi <leonardi@redhat.com>
The connection status is not tracked in the `Connection` struct, so there is no way to check whether the handshake has completed. Introduce the `established` field and set it when a `Connected` or `ConnectionRequest` event is successfully handled. Expose it via `is_connection_established()`. Signed-off-by: Luigi Leonardi <leonardi@redhat.com>
`send()` and `update_credit()` do not check whether the peer has requested shutdown, allowing operations on a connection that the peer considers closed. Check `peer_requested_shutdown` before proceeding in both methods. Signed-off-by: Luigi Leonardi <leonardi@redhat.com>
Add a function that checks if a local port is in use, either for listening or for a connection. This will be used by VsockDriver to allocate local ports when creating new streams. Signed-off-by: Luigi Leonardi <leonardi@redhat.com>
831ed1c to
6283c7c
Compare
| /// - The VSOCK device is not available (`VsockError::DriverError`) | ||
| /// - The connection is already shutdown (`VsockError::SocketShutdown`) | ||
| /// - The shutdown operation fails | ||
| pub fn shutdown(&mut self) -> Result<(), SvsmError> { |
There was a problem hiding this comment.
Are there any users that require manually shutting down the stream? If not, then I would suggest moving all of this into Drop. This would make it impossible to use the stream after it is closed. This should also help deduplicate the logic that is already present in Drop and in this method (e.g. VSOCK_DEVICE.shutdown()).
There was a problem hiding this comment.
My idea was to replicate this https://docs.rs/vsock/latest/vsock/struct.VsockStream.html
But yes, it would simplify the logic indeed.
There was a problem hiding this comment.
I guess it's also a matter of taste, but a big thing in Rust is to make invalid states unrepresentable. Another option would be to make shutdown() take self instead of &mut self, having the method consume the type. But then again, I don't know if there are any valid use cases where you want to use again the struct after shutdown.
There was a problem hiding this comment.
I'll keep this open for now, so other reviewers can share their feedback
6283c7c to
2e119f4
Compare
|
Addressed comments from @00xc
|
Add a virtio-vsock wrapper around virtio-drivers. It provides blocking functions for connect, send, recv, shutdown and force_shutdown. To use it you need the `vsock` feature. Signed-off-by: Luigi Leonardi <leonardi@redhat.com>
Add VsockStream struct that provides a high-level stream-oriented interface over the vsock driver API. Signed-off-by: Luigi Leonardi <leonardi@redhat.com>
Add an optional `--vsock cid` parameter. This will attach a virtio-vsock device to the VM. Signed-off-by: Luigi Leonardi <leonardi@redhat.com>
In preparation to vsock support, QEMU needs to be configured with vhost support enabled. Without the flag `--enable-vhost-kernel`, QEMU would fail to launch with error: `qemu-system-x86_64: -device vhost-vsock-device,guest-cid=3: 'vhost-vsock-device' is not a valid device model name` This flag is disabled by default because of `--without-default-features` Because no-cc is run as non-root, this causes QEMU to fail with the error: `vhost-vsock: failed to open vhost device: Permission denied"` To fix this, setup permissions to /dev/vhost-vsock for non-root. Add the flag to enable support for vsock in QEMU and install netcat Co-Developed-by: Luigi Leonardi <leonardi@redhat.com> Signed-off-by: Luigi Leonardi <leonardi@redhat.com> Signed-off-by: Nicola Ramacciotti <niko.ramak@gmail.com>
Add a test that performs some basic checks on vsock functionalities: - double connection - recv a buffer from the host - send a buffer to the host - recv a buffer after local shutdown - send a buffer after local shutdown The vsock server is created on the host using ncat. Signed-off-by: Luigi Leonardi <leonardi@redhat.com>
2e119f4 to
97efe54
Compare
This PR introduces a wrapper to the virtio-drivers crate like what has been done for virtio-blk. This provides blocking calls to
connectclosesendandrecv.Vsock is more flexible than the serial port as it has the concept of port, and we can reuse vsock for anything else.
I had to make 2 changes to the virtio-drivers crate, the idea is to open a PR in that repo. Before proceeding with the PR, I'd like to take some feedback here, maybe more changes are needed.
To compile it you need to enable the feature
vsockandvirtio-drivers.To test it you can use
make test-in-svsmNote: If a vsock device is not attached, svsm will not crash.