New virtio blk driver #96

Arsering · 2023-06-13T12:56:23Z

No description provided.

Arsering

add several new features to virtio-blk driver, including:

packed queue
indirect descriptor for packed queue and split queue
complete two device commands: VIRTIO_BLK_T_IN and VIRTIO_BLK_T_OUT (supporting read/write multiple blocks in one request)
add six new commands: VIRTIO_BLK_T_DISCARD, VIRTIO_BLK_T_WRITE_ZEROES, VIRTIO_BLK_T_FLUSH, VIRTIO_BLK_T_GET_ID, VIRTIO_BLK_T_SECURE_ERASE, VIRTIO_BLK_T_GET_LIFETIME (polling and interrupt)
support asynchronous I/O (future in Rust)

qwandor

Thanks for the PR! There's a lot going on here, so it's going to take a while to review it all. For a start, I've reviewed the indirect descriptor support and a few related parts.

If you'd like to get this merged more quickly it might be better to split out the other changes (packed queue, more block device features) into separate PRs.

qwandor · 2023-06-19T16:22:36Z

src/device/console.rs

@@ -65,15 +65,18 @@ pub struct ConsoleInfo {
 impl<H: Hal, T: Transport> VirtIOConsole<H, T> {
    /// Creates a new VirtIO console driver.
    pub fn new(mut transport: T) -> Result<Self> {
+        // TODO:


Did you mean to do something or write something more here?

My primary focus is on improving the virtio-blk driver, and the usage of "TODO" here is to notify someone that the indirect descriptor is not supported in the virtio-console driver. I will make the necessary modifications to clearly convey my intention.

qwandor · 2023-06-19T16:24:00Z

src/lib.rs

 pub mod transport;
+mod virtio_queue;


Why rename the module? Given that this is the virtio-drivers crate, it seems like it should be fairly clear that the queue module is for virt queues.

Yes, the rename is unnecessary and I will fix it

qwandor · 2023-06-21T09:06:01Z

src/virtio_queue/split_queue.rs

@@ -1,4 +1,4 @@
-#![deny(unsafe_op_in_unsafe_fn)]


Let's keep this deny please, it helps keep unsafe code a bit more contained.

qwandor · 2023-06-21T09:07:39Z

src/virtio_queue/packed_queue.rs

+use core::sync::atomic::{fence, Ordering};
+use log::info;
+use zerocopy::FromBytes;
+// use log::{debug, info};


Remove this line, there's no point keeping it commented-out.

OK, I will fix it

qwandor · 2023-06-21T09:16:46Z

src/virtio_queue/split_queue.rs

 /// The mechanism for bulk data transport on virtio devices.
 ///
 /// Each device can have zero or more virtqueues.
 ///
-/// * `SIZE`: The size of the queue. This is both the number of descriptors, and the number of slots
-///   in the available and used rings.
+/// * `SIZE`: The size of the queue. This is both the number of descriptors, and the number of slots in the available and used rings. Queue Size value is always a power of 2. The maximum Queue Size value is 32768. This value is specified in a bus-specific way.


Nit: wrap comments to 100 columns, to match the rest of the file. (Both this and the rest of the comments lower down.)

OK, I will fix it

qwandor · 2023-06-21T11:15:35Z

src/virtio_queue/split_queue.rs

+    ///
+    /// Ref: virtio_ring.c virtqueue_kick_prepare_split
+    /// This will be false if the device has supressed notifications.
+    pub fn should_notify_old(&self) -> bool {


Likewise, please just change the callers to pass false rather than having two versions of the method.

qwandor · 2023-06-21T11:17:33Z

src/virtio_queue/split_queue.rs

@@ -1,4 +1,4 @@
-#![deny(unsafe_op_in_unsafe_fn)]
+extern crate alloc;


This shouldn't be necessary here, only in the top-level lib.rs.

Yes, I will fix it

qwandor · 2023-06-21T11:46:08Z

src/virtio_queue/split_queue.rs

@@ -280,11 +409,57 @@ impl<H: Hal, const SIZE: usize> VirtQueue<H, SIZE> {
        self.free_head = head;
        let mut next = Some(head);

-        for (buffer, direction) in InputOutputIter::new(inputs, outputs) {
+        if self.indirect_desc {


You still need to loop through all the buffers and unshare them with H::unshare, to avoid leaking DMA memory on platforms that allocate it separately (and potentially missing the response entirely, as unshare may also copy the data back to the original buffer).

Yes, let me fix it

qwandor · 2023-06-21T11:48:12Z

src/virtio_queue/split_queue.rs

+    // TODO: will be deleted in the further
+    /// The buffers in `inputs` and `outputs` must match the set of buffers originally added to the
+    /// queue by `add`.
+    unsafe fn recycle_descriptors_async<'a>(&mut self, head: u16) {


This also needs to call H::unshare on each buffer.

Yes, I will fix it

qwandor · 2023-06-21T11:52:16Z

src/virtio_queue/split_queue.rs

@@ -553,6 +794,15 @@ impl Descriptor {
            None
        }
    }
+
+    fn new() -> Self {


Nit: you could just derive Default instead.

Yes, nice suggestion

qwandor · 2023-07-11T16:15:37Z

I've split out some of the new block device driver features into #100 so we can get that in first. Please take a look. The support for packed queues and indirect descriptors is going to require more thought, as the approach taken in this PR doesn't handle DMA memory properly. The async IO parts of this PR have some soundness issues.

qwandor · 2023-07-13T16:10:35Z

@Arsering I've pulled out the indirect descriptor support from this PR into #102 and fixed the DMA allocation issues and a few other things, and written some tests. Please take a look at that and let me know what you think.

qwandor · 2023-07-13T17:13:06Z

src/virtio_queue/split_queue.rs

        // Read barrier, so we read a fresh value from the device.
        fence(Ordering::SeqCst);

        // Safe because self.used points to a valid, aligned, initialised, dereferenceable, readable
        // instance of UsedRing.
+        if support_event {
+            unsafe { (*self.avail.as_ptr()).idx == (*self.used.as_ptr()).avail_event }


This should use self.avail_idx rather than reading from the available ring directly, because we don't trust the device not to modify the available ring (even though it shouldn't).

Also, from my reading of the spec, it sounds like we should be checking for avail_idx == avail_event.wrapping_add(1), as should_notify is called after avail_idx is updated. See 2.7.10.2: "equivalently, until idx in the available ring will reach the value avail_event + 1".

I'll send a separate PR to add event_idx support with this fixed.

Let me read the spec again to figure it out

Arsering added 14 commits June 13, 2023 13:06

add packedqueue

f7ec09c

Add packed queue

bb5b057

Add packed queue

37b55dd

delete redundant import of alloc inside the code

9ecfd08

delete redundant import of alloc inside the code

cba0181

fix bugs

baab177

fix warning

3b9fd2e

Add packed queue

b7e8dec

Add packed queue

a48ee9f

Add packed queue

69412cd

Add packed queue

c581c6c

Add packed queue

33e4560

Add packed queue

18880b7

Add packed queue

f5873f8

Arsering commented Jun 14, 2023

View reviewed changes

qwandor self-requested a review June 19, 2023 16:51

qwandor reviewed Jun 21, 2023

View reviewed changes

qwandor mentioned this pull request Jul 11, 2023

Add support for flushing block devices and getting their IDs #100

Merged

qwandor mentioned this pull request Jul 13, 2023

Add support for indirect descriptors in VirtQueue with alloc feature #102

Merged

qwandor reviewed Jul 13, 2023

View reviewed changes

qwandor mentioned this pull request Jul 24, 2023

Add support for VIRTIO_F_EVENT_IDX buffer notification suppression. #106

Merged

----

9fca5c9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New virtio blk driver #96

New virtio blk driver #96

Arsering commented Jun 13, 2023

Arsering left a comment

qwandor left a comment

qwandor Jun 19, 2023

Arsering Jul 13, 2023

qwandor Jun 19, 2023

Arsering Jul 13, 2023

qwandor Jun 21, 2023

Arsering Jul 13, 2023

qwandor Jun 21, 2023

Arsering Jul 13, 2023

qwandor Jun 21, 2023

Arsering Jul 13, 2023

qwandor Jun 21, 2023

Arsering Aug 8, 2023

qwandor Jun 21, 2023

Arsering Jul 14, 2023

qwandor Jun 21, 2023

Arsering Jul 14, 2023

qwandor Jun 21, 2023

Arsering Aug 8, 2023

qwandor Jun 21, 2023

Arsering Aug 8, 2023

qwandor commented Jul 11, 2023

qwandor commented Jul 13, 2023

qwandor Jul 13, 2023

qwandor Jul 13, 2023

Arsering Jul 14, 2023

		@@ -1,4 +1,4 @@
		#![deny(unsafe_op_in_unsafe_fn)]
		extern crate alloc;

New virtio blk driver #96

Are you sure you want to change the base?

New virtio blk driver #96

Conversation

Arsering commented Jun 13, 2023

Arsering left a comment

Choose a reason for hiding this comment

qwandor 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

Choose a reason for hiding this comment

qwandor commented Jul 11, 2023

qwandor commented Jul 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment