Integrate initial virtio console implementation #217

RaduNiculae · 2022-05-17T12:27:06Z

Added a virtio console device to replace the UART serial console.
The implementation is divided between vm-virtio and vmm-reference.

Signed-off-by: Niculae Radu [email protected]

Summary of the PR

Added a virtio console device with similar queue handling as the block and
network devices in vmm-reference. Part of the implementation is found in
the vm-virtio repository. For now it is linked to my personal repo until it
will be merged.

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

All commits in this PR are signed (with git commit -s), and the commit
message has max 60 characters for the summary and max 75 characters for each
description line.
All added/changed functionality has a corresponding unit/integration
test.
Any newly added unsafe code is properly documented.

lauralt

Nice work, I'll finish the review tomorrow.

Cargo.lock

src/devices/src/virtio/console/mod.rs

src/devices/src/virtio/console/inorder_handler.rs

src/devices/src/virtio/console/mod.rs

src/devices/src/virtio/console/device.rs

src/devices/src/virtio/console/mod.rs

src/vmm/src/config/mod.rs

lauralt

Left a few other comments. Will finish the review early next week.

src/devices/src/virtio/console/inorder_handler.rs

lauralt · 2022-06-17T14:45:30Z

src/devices/src/virtio/console/inorder_handler.rs

+                    Ok(used_len) => used_len,
+                    Err(e) => {
+                        self.receiveq.state.go_to_previous_position();
+                        return Err(Error::Console(e));
+                    }
+                };
+                if used_len == 0 {
+                    self.receiveq.state.go_to_previous_position();
+                    break;
+                }


We have to think a bit more about this. I think process_receiveq_chain can fail even if there were some bytes written to the chain, in which case we shouldn't go back one position in the available ring.

I think this can be achieved by using a write method in process_receiveq_chain that also returns the numbers of bytes successfully written to memory, and return that value even when we fail with an error there (there is something similar done for block https://github.com/rust-vmm/vm-virtio/blob/main/crates/devices/virtio-blk/src/stdio_executor.rs#L339). This is a bit more complicated and requires some dive deep, I think we can postpone it for a future PR of virtio-console.

src/devices/src/virtio/console/inorder_handler.rs

lauralt · 2022-06-20T14:12:34Z

src/devices/src/virtio/console/inorder_handler.rs

+                    Ok(used_len) => used_len,
+                    Err(e) => {
+                        self.receiveq.state.go_to_previous_position();
+                        return Err(Error::Console(e));
+                    }
+                };
+                if used_len == 0 {
+                    self.receiveq.state.go_to_previous_position();
+                    break;
+                }


I think this can be achieved by using a write method in process_receiveq_chain that also returns the numbers of bytes successfully written to memory, and return that value even when we fail with an error there (there is something similar done for block https://github.com/rust-vmm/vm-virtio/blob/main/crates/devices/virtio-blk/src/stdio_executor.rs#L339). This is a bit more complicated and requires some dive deep, I think we can postpone it for a future PR of virtio-console.

andreeaflorescu · 2022-06-21T12:15:57Z

src/api/src/lib.rs

+                    .long("console")
+                    .required(false)
+                    .takes_value(true)
+                    .help("Console configuration. \n\tFormat: \"type=<string>\"")


What are the possible types here? For the other ones it is self explanatory what the value of the configuration is (i.e. a path, kernel_load_addr, etc), but for this one it is not. We should write the possible values in the help here, and also when we are parsing it, we should check that the value is one of the supported ones.

andreeaflorescu · 2022-06-21T12:17:50Z

src/api/src/lib.rs

@@ -236,6 +244,7 @@ mod tests {
                vcpu_config: VcpuConfig { num: 1 },
                block_config: None,
                net_config: None,
+                console_config: None


We should update the test to have a console_config that's being parsed as well. Best would be to have it with all possible values.

andreeaflorescu · 2022-06-21T12:18:40Z

src/devices/src/virtio/console/device.rs

+        ];
+
+        let config_space = Vec::new();
+        let virtio_cfg = VirtioConfig::new(device_features, queues, config_space);


nit: too many whitespaces. Why do we need them?

andreeaflorescu · 2022-06-21T12:23:48Z

src/devices/src/virtio/console/inorder_handler.rs

+    T: Write,
+{
+    pub fn process_transmitq(&mut self) -> result::Result<(), Error> {
+        // To see why this is done in a loop, please look at the `Queue::enable_notification`


This is very likely to be outdated in the future. Can you add a short description at least for why we do the loop here? You can say that more details can be found in the Queue:enable_notification function, but we should have at least a short description here as well.

src/devices/src/virtio/console/mod.rs

andreeaflorescu · 2022-06-21T12:25:43Z

src/devices/src/virtio/console/queue_handler.rs

+const TRANSMITQ_EVENT: u32 = 1;
+const RECEIVEQ_EVENT: u32 = 2;
+
+const STDIN_BUFFER_SIZE: usize = 1024;


Where is this taken from?

This is just a random value. To figure out a good one we should run some tests. I tried with some python tests but I could not measure the time it takes to send data to the driver accurately.

andreeaflorescu · 2022-06-21T12:28:47Z

src/devices/src/virtio/console/queue_handler.rs

+                        }
+                    }
+                }
+                if !failed {


Why do we need this as opposed to just handling the errors as they happen? I think this can be significantly shorten by just returning the error directly.

andreeaflorescu · 2022-06-21T12:35:21Z

src/devices/src/virtio/console/queue_handler.rs

+                if let Err(e) = self.transmitqfd.read() {
+                    error!("Could not read transmitq event fd: {:?}", e);
+                } else if let Err(e) = self.inner.process_transmitq() {
+                    error!("Transmitq processing failed: {:?}", e);
+                }


Can you try to use and_then to reduce some of these lines of code?

I tried using and_then/or_else to keep the error messages and it made the code more convoluted. I think this is easier to understand. I will change it if you think it is better that way.

andreeaflorescu · 2022-06-21T12:36:31Z

src/vmm/src/config/mod.rs

+#[derive(Clone, Debug, PartialEq)]
+pub struct ConsoleConfig {
+    /// Type of console.
+    pub console_type: String,


This should be an enum instead of a String. In the enum we would then have 2 variants: uart and virtio.

Added a virtio console device to replace the UART serial console. The implementation is divided between vm-virtio and vmm-reference. Signed-off-by: Niculae Radu <[email protected]>

lauralt · 2022-07-07T13:34:20Z

src/devices/src/virtio/console/device.rs

+            driver_notify,
+            receiveq: self.cfg.virtio.queues.remove(0),
+            transmitq: self.cfg.virtio.queues.remove(0),
+            console: console::Console::new_with_capacity(console::DEFAULT_CAPACITY, stdout())


I think we can use here the new constructor or even the Default implementation.

lauralt · 2022-07-07T13:49:27Z

src/devices/src/virtio/console/device.rs

+// SPDX-License-Identifier: Apache-2.0 OR BSD-3-Clause
+
+use crate::virtio::console::CONSOLE_DEVICE_ID;
+use crate::virtio::features::{VIRTIO_F_IN_ORDER, VIRTIO_F_RING_EVENT_IDX, VIRTIO_F_VERSION_1};


nit: can we group together the crate imports, same comment for the external crates (see use virtio_console::console below)?

lauralt · 2022-07-07T14:12:12Z

src/vmm/src/config/mod.rs

+    type Error = ConversionError;
+
+    fn try_from(console_cfg_str: &str) -> Result<Self, Self::Error> {
+        // Supported options: `type=String`


This is not correct, we support only type=virtio/console.

lauralt · 2022-07-07T14:19:23Z

src/api/src/lib.rs

@@ -252,6 +306,7 @@ mod tests {
                vcpu_config: VcpuConfig { num: 1 },
                block_config: None,
                net_config: None,
+                console_config: None


Shouldn't this be uart?

lauralt · 2022-07-07T14:23:08Z

src/vmm/src/config/mod.rs

+        let console_type_str: String = arg_parser
+            .value_of("type")
+            .map_err(ConversionError::new_console)?
+            .ok_or_else(|| ConversionError::new_console("Missing required argument: type"))?;


I think type is not required, right? So if the user doesn't pass it, then we set here the default value, or am I missing something?

lauralt · 2022-07-07T14:24:40Z

tests/test_run_reference_vmm.py

-            output_lines = data.split(b'\r\n')
-            last = output_lines[-1].strip()
-            if prompt in last:
+            total_data += data


Would be nice to move this fix to a separate commit and explain a bit what is it about.

RaduNiculae force-pushed the virtio-console branch 3 times, most recently from 9fc51ca to 5cb7cef Compare May 17, 2022 17:25

lauralt reviewed May 19, 2022

View reviewed changes

Cargo.lock Outdated Show resolved Hide resolved

lauralt reviewed May 20, 2022

View reviewed changes

RaduNiculae force-pushed the virtio-console branch from 5cb7cef to f06f594 Compare June 14, 2022 09:56

lauralt changed the title ~~Init virtio console.~~ Integrate initial virtio console implementation Jun 15, 2022

RaduNiculae force-pushed the virtio-console branch 5 times, most recently from dcf40ba to dbbcf17 Compare June 17, 2022 11:03

lauralt reviewed Jun 17, 2022

View reviewed changes

src/vmm/src/config/mod.rs Show resolved Hide resolved

lauralt reviewed Jun 17, 2022

View reviewed changes

lauralt reviewed Jun 20, 2022

View reviewed changes

src/devices/src/virtio/console/inorder_handler.rs Show resolved Hide resolved

lauralt reviewed Jun 20, 2022

View reviewed changes

andreeaflorescu reviewed Jun 21, 2022

View reviewed changes

Init virtio console.

9e220e5

Added a virtio console device to replace the UART serial console. The implementation is divided between vm-virtio and vmm-reference. Signed-off-by: Niculae Radu <[email protected]>

RaduNiculae force-pushed the virtio-console branch from dbbcf17 to 9e220e5 Compare June 30, 2022 10:34

lauralt reviewed Jul 7, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate initial virtio console implementation #217

Integrate initial virtio console implementation #217

RaduNiculae commented May 17, 2022

lauralt left a comment

lauralt left a comment

lauralt Jun 17, 2022

lauralt Jun 20, 2022

lauralt Jun 20, 2022

andreeaflorescu Jun 21, 2022

andreeaflorescu Jun 21, 2022

andreeaflorescu Jun 21, 2022

andreeaflorescu Jun 21, 2022

andreeaflorescu Jun 21, 2022

RaduNiculae Jun 30, 2022

andreeaflorescu Jun 21, 2022

andreeaflorescu Jun 21, 2022

RaduNiculae Jun 30, 2022

andreeaflorescu Jun 21, 2022

lauralt Jul 7, 2022

lauralt Jul 7, 2022

lauralt Jul 7, 2022

lauralt Jul 7, 2022

lauralt Jul 7, 2022

lauralt Jul 7, 2022

Integrate initial virtio console implementation #217

Are you sure you want to change the base?

Integrate initial virtio console implementation #217

Conversation

RaduNiculae commented May 17, 2022

Summary of the PR

Requirements

lauralt left a comment

Choose a reason for hiding this comment

lauralt 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

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