add support of a virtio socket device #826

stlankes · 2023-08-07T19:14:49Z

The current interface is able to work as server. System calls like bind, listen, accept and connect are supported.

The hermit-os/hermit-rs#450 is an example to use the interface.

egrimley-arm · 2023-09-11T13:01:02Z

I'm interested in testing this, but what will be the way to access vsock from the application?

stlankes · 2023-09-11T19:16:55Z

It isn't finished and in the beginning. At the end it should work like https://gist.github.com/mcastelino/9a57d00ccf245b98de2129f0efe39857

stlankes · 2024-08-08T07:00:08Z

@mkroening The CI error doesn't depend to this PR

mkroening · 2024-08-08T15:23:16Z

@mkroening The CI error doesn't depend to this PR

Yes, thats rust-lang/rust#128808. It should be resolved tomorrow.

After accepting a connection, the new socket has to use the same port like the listening stream. => Accepting of new connection is not possible.

mkroening

Looks good to me!

@cagatay-y will take a look at the virtqueue-related bits.

Is this ready to merge from your side, @stlankes? Should I squash some commits before merging? :)

src/executor/vsock.rs

Cargo.toml

mkroening · 2024-08-12T14:50:07Z

src/drivers/virtio/transport/mmio.rs

 					#[cfg(not(target_arch = "riscv64"))]
-					add_irq_name(irq_no, "virtio_net");
+					add_irq_name(irq_no, "virtio");
+					let _ = VIRTIO_IRQ.try_insert(irq_no);


Suggested change

let _ = VIRTIO_IRQ.try_insert(irq_no);

if let Err((final_value, _)) = VIRTIO_IRQ.try_insert(irq_no) {

assert_eq!(*final_value, irq_no);

}

mkroening · 2024-08-12T14:52:09Z

src/drivers/virtio/transport/mmio.rs

+					irq_install_handler(irq_no, virtio_irqhandler);
+					#[cfg(not(target_arch = "riscv64"))]
+					add_irq_name(irq_no, "virtio");
+					let _ = VIRTIO_IRQ.try_insert(irq_no);


Suggested change

let _ = VIRTIO_IRQ.try_insert(irq_no);

if let Err((final_value, _)) = VIRTIO_IRQ.try_insert(irq_no) {

assert_eq!(*final_value, irq_no);

}

mkroening · 2024-08-12T14:52:20Z

src/drivers/virtio/transport/pci.rs

+					use crate::drivers::virtio::transport::VIRTIO_IRQ;
+
+					let irq = device.get_irq().unwrap();
+					let _ = VIRTIO_IRQ.try_insert(irq);


Suggested change

let _ = VIRTIO_IRQ.try_insert(irq);

if let Err((final_value, _)) = VIRTIO_IRQ.try_insert(irq_no) {

assert_eq!(*final_value, irq_no);

}

mkroening · 2024-08-12T14:52:26Z

src/drivers/virtio/transport/pci.rs

 					let irq = device.get_irq().unwrap();
+					let _ = VIRTIO_IRQ.try_insert(irq);


Suggested change

let _ = VIRTIO_IRQ.try_insert(irq);

if let Err((final_value, _)) = VIRTIO_IRQ.try_insert(irq_no) {

assert_eq!(*final_value, irq_no);

}

cagatay-y · 2024-08-19T15:08:56Z

src/drivers/vsock/mod.rs

+	fn add(&mut self, mut vq: Box<dyn Virtq>) {
+		const BUFF_PER_PACKET: u16 = 2;
+		let num_packets: u16 = u16::from(vq.size()) / BUFF_PER_PACKET;
+		fill_queue(


Do device events have headers? If not, fill_queue cannot be reused for EventQueue, since it allocates and puts before the payload a header structure. Also, if the function is adapted for the EventQueue use case, num_packets will not need to be divided by two since the buffers in the queue will consist of a single element.

The EventQueue is currently a dummy queue. The standard want that we register such a queue. For instance, a migration signal can be sent to the kernel. Currently, we don't support it. => Just a dummy registration.

cagatay-y · 2024-08-19T15:12:16Z

src/drivers/vsock/mod.rs

+			vq: None,
+			poll_sender,
+			poll_receiver,
+			packet_size: crate::VSOCK_PACKET_SIZE + mem::size_of::<Hdr>() as u32,


packet_size does not need to include the header size since fill_queue allocates the header separately and uses the packet_size parameter only for the payload vector.

Ok, I will fix it.

cagatay-y · 2024-08-19T15:44:27Z

src/drivers/vsock/mod.rs

+	pub fn handle_interrupt(&mut self) {
+		let _ = self.isr_stat.is_interrupt();
+
+		if self.isr_stat.is_cfg_change() {


If I understand correctly, the way we currently handle IsrStatus for PCIe transport is incorrect. The specification says that "To avoid an extra access, simply reading this register resets it to 0 and causes the device to de-assert the interrupt" (v1.2 § 4.1.4.5), so the is_interrupt read may be resetting the status and cause the is_cfg_change to always return false. I think it should be fixed in the IsrStatus impl but I couldn't write a review comment for an unchanged line.

Ok, I will revise it.

cagatay-y · 2024-08-19T15:48:00Z

src/drivers/vsock/mod.rs

+	}
+
+	pub fn handle_interrupt(&mut self) {
+		let _ = self.isr_stat.is_interrupt();


We should rename is_interrupt to something like is_queue_interrupt to clarify that it does not check for configuration change interrupts and is specific to used buffer interrupts. It would also match the naming used in the specification.

You are right, I will do it.

cagatay-y · 2024-08-19T15:58:22Z

src/syscalls/socket.rs

-				let handle = nic.create_udp_handle().unwrap();
-				drop(guard);
-				let socket = udp::Socket::new(handle);
+		#[cfg(feature = "vsock")]


Shouldn't we have a match on domain here? It seems to me that if "vsock" is enabled, the function will always early return a socket regardless of domain.

You are right, I will fix it.

stlankes marked this pull request as draft August 7, 2023 19:16

egrimley-arm mentioned this pull request Aug 14, 2023

src/unix/hermit/ is dead code as hermit is no longer cfg(unix) rust-lang/libc#3318

Closed

stlankes force-pushed the vsock branch 22 times, most recently from 442cdc4 to 77a0ff7 Compare July 25, 2024 18:05

stlankes added 4 commits August 6, 2024 00:01

add draft of a vsock driver

ef3e473

allow dead code in vsock driver

db20f73

allow enum_variant_names to avoid clippy warnings

12474e2

define vsock driver as pci driver

055c627

revise vsock driver to support the new virtio interface

7c24305

stlankes force-pushed the vsock branch from c2adfae to 7c24305 Compare August 6, 2024 19:48

stlankes marked this pull request as ready for review August 6, 2024 19:49

stlankes changed the title ~~add draft of a vsock driver~~ add support of a virtio socket device Aug 6, 2024

stlankes mentioned this pull request Aug 6, 2024

add example to test vsock interface hermit-os/hermit-rs#450

Merged

stlankes requested review from cagatay-y and mkroening August 6, 2024 20:33

stlankes assigned mkroening Aug 6, 2024

stlankes added 2 commits August 7, 2024 00:03

add support of Vsock shutdown and credit update command

d82fba8

revise buffer management compliant with the virtio standard

7190b40

stlankes force-pushed the vsock branch from 0029dc2 to 5632208 Compare August 7, 2024 21:18

add support of the system call poll for the vsock interface

36375c6

stlankes force-pushed the vsock branch from 5632208 to 36375c6 Compare August 8, 2024 06:28

add support of getpeername and getsockname

ebf94b7

stlankes force-pushed the vsock branch from 554e1da to ebf94b7 Compare August 8, 2024 10:30

stlankes added 2 commits August 8, 2024 18:19

In case of vsock, cloning of the socket is invalid

8b80a6d

After accepting a connection, the new socket has to use the same port like the listening stream. => Accepting of new connection is not possible.

check the source of the received message

27d5c7a

stlankes force-pushed the vsock branch from c49b72e to 27d5c7a Compare August 11, 2024 22:26

add support of the system call connect

8aee66c

mkroening approved these changes Aug 12, 2024

View reviewed changes

stlankes added 2 commits August 13, 2024 08:40

use busy waiting in case of connect

ae48d4e

remove dependency to the crate endian-num

affd88c

mkroening added this pull request to the merge queue Aug 22, 2024

Merged via the queue into hermit-os:main with commit 6dac572 Aug 22, 2024
13 checks passed

cagatay-y reviewed Aug 22, 2024

View reviewed changes

stlankes deleted the vsock branch August 22, 2024 17:57

stlankes mentioned this pull request Aug 22, 2024

improve the compatibility to the virtio standard #1365

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support of a virtio socket device #826

add support of a virtio socket device #826

stlankes commented Aug 7, 2023 •

edited

Loading

egrimley-arm commented Sep 11, 2023

stlankes commented Sep 11, 2023

stlankes commented Aug 8, 2024

mkroening commented Aug 8, 2024 •

edited

Loading

mkroening left a comment

mkroening Aug 12, 2024

mkroening Aug 12, 2024

mkroening Aug 12, 2024

mkroening Aug 12, 2024

cagatay-y Aug 19, 2024

stlankes Aug 22, 2024

cagatay-y Aug 19, 2024

stlankes Aug 22, 2024

cagatay-y Aug 19, 2024

stlankes Aug 22, 2024

cagatay-y Aug 19, 2024

stlankes Aug 22, 2024

cagatay-y Aug 19, 2024

stlankes Aug 22, 2024

-					let _ = VIRTIO_IRQ.try_insert(irq_no);
+					if let Err((final_value, _)) = VIRTIO_IRQ.try_insert(irq_no) {
+					    assert_eq!(*final_value, irq_no);
+					}

		let irq = device.get_irq().unwrap();
		let _ = VIRTIO_IRQ.try_insert(irq);

add support of a virtio socket device #826

add support of a virtio socket device #826

Conversation

stlankes commented Aug 7, 2023 • edited Loading

egrimley-arm commented Sep 11, 2023

stlankes commented Sep 11, 2023

stlankes commented Aug 8, 2024

mkroening commented Aug 8, 2024 • edited Loading

mkroening left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stlankes commented Aug 7, 2023 •

edited

Loading

mkroening commented Aug 8, 2024 •

edited

Loading