Implement Virtio Block Driver #108

bayedieng · 2021-11-26T18:50:31Z

This PR works towards resolving #74.

Checklist Towards Completion

Complete function to handle interrupts
Handle PCI and MMIO
Enable block device from qemu side
Register Device
- Resolve kernel panic upon block driver initialization
Ensure Driver works

exts/virtio_blk/lib.rs

exts/virtio_blk/Cargo.toml

michalfita · 2021-11-28T16:34:03Z

exts/virtio_blk/Cargo.toml

+[package]
+name = "virtio_blk"
+version = "0.0.1"
+authors = ["The Kerla Authors"]


As it's mostly your authorship at the moment, I think it's perfectly legal to claim your name here. There is no legal entity behind Kerla at the moment other than individual contributors and definitely one under such name don't exist.

michalfita · 2021-11-28T16:35:43Z

exts/virtio_blk/lib.rs

+
+
+pub struct VirtioBlk {
+    virtio: Virtio,


Please confirm it always have to claim ownership of underlying Virtio instance?

I'm not sure what you mean by claim ownership of "it".

nuta · 2021-11-29T14:41:02Z

tools/run-qemu.py

@@ -21,6 +22,9 @@
            "-device", "virtio-net,netdev=net0,disable-legacy=on,disable-modern=off",
            "-netdev", "user,id=net0,hostfwd=tcp:127.0.0.1:20022-:22,hostfwd=tcp:127.0.0.1:20080-:80",
            "-object", "filter-dump,id=fiter0,netdev=net0,file=virtio-net.pcap",
+
+            "-device", "virtio-blk-pci,drive=drive0,id=virtblk0,disable-legacy=on,disable-modern=off",
+            "-drive", "file=vdisk,if=none,id=drive0",


Tip: QEMU has a virtual FAT formatted disk and you may find it convenient
https://en.wikibooks.org/wiki/QEMU/Devices/Storage

hey @nuta. I'm confused as to what exact qemu argument I should pass. The latest commit to run-qemu.py exits to status 0. I've also tried the following:

"-drive", "file=vdisk,if=none,format=raw,id=hd", "-device", "virtio-blk-pci,drive=hd",

The strings above compile however the virtioblock::new() method isn't being instantiated. Help would be much appreciated.

disable-legacy=on,disable-modern=off is essential because the current virtio implementation supports the virtio modern spec only.

"-drive", "file=vdisk,if=none,format=raw,id=hd", "-device", "virtio-blk-pci,drive=hd,disable-legacy=on,disable-modern=off",

I see thanks, in regards to the qemu failure status I think it's likely the request_to_device() method that isn't well fleshed out. I'll be looking over the virtio spec some more and hopefully have the driver ready to merge by the end of the weekend.

nuta · 2021-12-12T00:47:19Z

exts/virtio_block/lib.rs

+        let chain = &[VirtqDescBuffer::ReadOnlyFromDevice {
+            addr: addr.as_paddr(),
+            len: request_len + frame.len()
+        }];


I guess you need to add 3 descriptors instead of a single ReadOnlyFromDevice one:

Descriptor 1: type, reserved, and sector (ReadOnlyFromDevice).

Descriptor 2: The memory buffer to be read (ReadOnlyFromDevice) or written (ReadOnlyFromDevice) from the device for RequestType::Write and RequestType:: RequestType::Read respectively.

Descriptor 3: status as defined in 5.2.6 Device Operation in the virtio spec (WritableFromDevice).

Thank you! I've tried to do that, but I am still getting an exit with failure status. Perhaps the memory and status buffers are wrong. Am I supposed to allocate pages for descriptor 2 and 3 or will an array buffer suffice? Right now I have:

fn request_to_device(&mut self, request_type: RequestType, sector: u64, frame: &[u8]) { let request_addr = self.request_buffer.add(MAX_BLK_SIZE); let request_len = size_of::<VirtioBlockRequest>(); let read_addr = unsafe {PAddr::new(*frame.as_ptr() as usize)}; let read_len = frame.len(); let status_buffer = [0]; let status_addr =unsafe { PAddr::new(*status_buffer.as_ptr() as usize)}; let status_len = status_buffer.len(); // Fill block request let block_request = unsafe { &mut *request_addr.as_mut_ptr::<VirtioBlockRequest>()}; block_request.type_ = request_type as u32; block_request.sector = sector; block_request.reserved = 0; // Copy data into buffers unsafe { request_addr.as_mut_ptr::<u8>() .add(request_len) .copy_from_nonoverlapping(frame.as_ptr(), frame.len()); } // Chain Descriptor let chain = &[VirtqDescBuffer::ReadOnlyFromDevice { addr: request_addr.as_paddr(), len: request_len + frame.len() }, VirtqDescBuffer::ReadOnlyFromDevice { addr: read_addr, len: read_len }, VirtqDescBuffer::WritableFromDevice { addr: status_addr, len: status_len } ]; // enqueue to data to send let request_virtq = self.virtio.virtq_mut(VIRTIO_REQUEST_QUEUE); request_virtq.enqueue(chain); request_virtq.notify();

Thank you! I've tried to do that, but I am still getting an exit with failure status.

What do you mean by "exit with failure status"? Does QEMU stop without a kernel panic message?

One thing I noticed is that read_addr and status_addr point to possibly the stack area (frame and status_buffer respectively). Since the virtio-blk device may process the request asynchronously, after returning from this method, in other words after status_buffer is freed, the device could modify other variables in the stack. If it modies the return address, it could be the cause of your mysterious crash.

FYI, virtio-net device driver allocates memory buffers for network packets beforehand and copy into/from them (just like self.request_buffer in your virtio-blk driver) not to use the stack. It incurs additional memory copies, but it just works for now.

What do you mean by "exit with failure status"? Does QEMU stop without a kernel panic message?

Yes, as of the latest commit to the PR, this is the output I get when running the kernel unsure whether it's my qemu settings for the block driver or something wrong with my implementation somewhere.

Booting from ROM.. cmdline: /tmp/tmp64lfyxuw cmdline: unsupported option, ignoring: '' available RAM: base=669000, size=505MiB Booting Kerla... initramfs: loaded 73 files and directories (1MiB) kext: Loading virtio_net... kext: Loading virtio_block... virtio-net: MAC address is 52:54:00:12:34:56 Block size is 0 run-qemu.py: qemu exited with failure status (status=0) make: *** [Makefile:113: run] Error 1

JFYI: From my experience, if the kernel stops without showing a panic message, I suspect memory corruption by newly added code. For example, use-after-free.

Can you push the latest changes? Let me try later 😃

Sorry for the late reply. I just pushed the latest commit and addressed merge conflicts. I am still getting kernel panics without messages. Haven't been able to figure out why yet.

nuta · 2021-12-19T13:50:17Z

kernel/main.rs

@@ -136,6 +136,10 @@ impl KernelOps for ApiOps {
        register_ethernet_driver(driver)
    }

+    fn register_block_driver(&self, driver: Box<dyn kerla_api::driver::block::BlockDriver>) {
+        register_block_driver(driver)


This causes an infinite recursion. I think this is the root cause of the kernel panic.

Since Kerla still doesn't have the block I/O layer, let's leave this method empty.

Thank you! Would've took a while to catch that. All that's seems to be left now is interrupt handling and reading the block size properly.

… subsystem is complete

nuta · 2021-12-19T23:24:11Z

@nuta What exactly does the read_device_config8 trait method in the virto device.rs module return as output? An 8-bit feature?

It reads a byte from the device's configuration space. For virtio-blk, you'll see the structure you've already defined as VirtioBlockConfig.

My naive idea is to allocate a memory page (alloc_pages), read configulation space using read_device_config8, and then transmute the buffer into VirtioBlockConfig.

bayedieng · 2021-12-22T12:27:51Z

While attempting to attach irq kernel panics with the following message:

panicked at 'handler for IRQ #11 is already attached', kernel/interrupt.rs:28:23
    0: ffff80000028095a  kerla_runtime::backtrace::backtrace()+0x1a
    1: ffff8000001bbcdc  rust_begin_unwind()+0x13c
    2: ffff8000002b089a  core::panicking::panic_fmt()+0x3a
    3: ffff80000013bc2a  kerla_kernel::interrupt::attach_irq()+0x16a
    4: ffff8000001e2ad4  <kerla_kernel::ApiOps as k...s::KernelOps>::attach_irq()+0x34
    5: ffff8000001f9c98  kerla_api::driver::attach_irq()+0x68
    6: ffff8000001f82f6  <virtio_block::VirtioBlock...:DeviceProber>::probe_pci()+0x796
    7: ffff8000002678f5  kerla_api::driver::init()+0x805
    8: ffff800000269d21  kerla_api::kernel_ops::init_drivers()+0x31
    9: ffff8000001e2fe8  boot_kernel()+0x458
    10: ffff800000283be9  bsp_early_init()+0xf9
    11: ffff8000001002e5  halt()+0x0
preparing a crash dump...
prepared crash dump: log_len=1549
booting boot2dump...

bayedieng · 2021-12-22T12:31:40Z

While attempting to attach irq kernel panics with the following message:

panicked at 'handler for IRQ #11 is already attached', kernel/interrupt.rs:28:23
    0: ffff80000028095a  kerla_runtime::backtrace::backtrace()+0x1a
    1: ffff8000001bbcdc  rust_begin_unwind()+0x13c
    2: ffff8000002b089a  core::panicking::panic_fmt()+0x3a
    3: ffff80000013bc2a  kerla_kernel::interrupt::attach_irq()+0x16a
    4: ffff8000001e2ad4  <kerla_kernel::ApiOps as k...s::KernelOps>::attach_irq()+0x34
    5: ffff8000001f9c98  kerla_api::driver::attach_irq()+0x68
    6: ffff8000001f82f6  <virtio_block::VirtioBlock...:DeviceProber>::probe_pci()+0x796
    7: ffff8000002678f5  kerla_api::driver::init()+0x805
    8: ffff800000269d21  kerla_api::kernel_ops::init_drivers()+0x31
    9: ffff8000001e2fe8  boot_kernel()+0x458
    10: ffff800000283be9  bsp_early_init()+0xf9
    11: ffff8000001002e5  halt()+0x0
preparing a crash dump...
prepared crash dump: log_len=1549
booting boot2dump...

bayedieng · 2021-12-25T09:57:34Z

hey @nuta what specific action do you take to make sure the driver works properly?

nuta · 2021-12-26T05:35:09Z

You should just try using the driver: write a sector into the disk and see if the attached image file has changed.

bayedieng · 2021-12-26T15:12:07Z

You should just try using the driver: write a sector into the disk and see if the attached image file has changed.

Not sure how to write a sector within the context of the kernel. Looked through vfat but didn't manage to get it running. Would be helpful to show a demonstration. I apologize for all the questions.

nuta · 2021-12-27T12:16:19Z

exts/virtio_block/lib.rs

+        attach_irq(mmio_device.irq, move || {
+            device.lock().handle_irq();
+        });
+    }


If I were you, I would add the following line here:

device.write_to_disk(0 /* the first sector */, [0x5a; 256].as_slice());

Then, run it on QEMU and see if the first sector has been modified:

$ hexdump -C vdisk.img | head

bayedieng · 2022-01-05T15:49:15Z

I applied some changes but am not sure whether my buffer allocation is correct.

 pub fn new(transport: Arc<dyn VirtioTransport>) -> Result<VirtioBlock, VirtioAttachError> {
     let mut virtio = Virtio::new(transport);
        virtio.initialize(VIRTIO_BLK_F_SIZE, 1)?;
        // Read the block size
        let block_size =
            virtio.read_device_config64(offset_of!(VirtioBlockConfig, capacity) as u16);

        info!(
            "The disk capacity is {}",
            ByteSize::new(block_size as usize * 512)
        );

        let ring_len = virtio.virtq(VIRTIO_REQUEST_QUEUE).num_descs() as usize;

        let request_buffer = alloc_pages(
            (align_up(MAX_BLK_SIZE * ring_len, PAGE_SIZE)) / PAGE_SIZE,
            AllocPageFlags::KERNEL,
        )
        .unwrap()
        .as_vaddr();

        let status_buffer = alloc_pages(
            (align_up((MAX_BLK_SIZE * ring_len) + 1, PAGE_SIZE)) / PAGE_SIZE,
            AllocPageFlags::KERNEL,
        )
        .unwrap()
        .as_vaddr();

        let read_buffer = alloc_pages(
            (align_up((MAX_BLK_SIZE * ring_len) + 2, PAGE_SIZE)) / PAGE_SIZE,
            AllocPageFlags::KERNEL,
        )
        .unwrap()
        .as_vaddr();

        Ok(VirtioBlock {
            virtio: virtio,
            request_buffer: request_buffer,
            status_buffer: status_buffer,
            read_buffer: read_buffer,
            request_buffer_index: 0,
            ring_len: ring_len,
        })
    }

    fn write_to_disk(&mut self, sector: u64, buf: &[u8]) {
        let i = self.request_buffer_index % self.ring_len;
        let request_addr = self.request_buffer.add(MAX_BLK_SIZE * i);
        let status_addr = self.status_buffer.add((MAX_BLK_SIZE * i) + 1);
        let read_addr = self.read_buffer.add((MAX_BLK_SIZE * i) + 2);
        let request_header_len = size_of::<VirtioBlockRequest>();

        // Fill block request
        let block_request = unsafe { &mut *request_addr.as_mut_ptr::<VirtioBlockRequest>() };
        block_request.type_ = RequestType::Write as u32;
        block_request.sector = sector;
        block_request.reserved = 0;

        // Chain Descriptor
        let chain = &[
            VirtqDescBuffer::ReadOnlyFromDevice {
                addr: request_addr.as_paddr(),
                len: request_header_len,
            },
            VirtqDescBuffer::ReadOnlyFromDevice {
                addr: read_addr.as_paddr(),
                len: buf.len(),
            },
            VirtqDescBuffer::WritableFromDevice {
                addr: status_addr.as_paddr(),
                len: 1,
            },
        ];

        // enqueue to data to send
        let request_virtq = self.virtio.virtq_mut(VIRTIO_REQUEST_QUEUE);
        request_virtq.enqueue(chain);
        request_virtq.notify();
    }

nuta · 2022-01-07T11:34:27Z

I applied some changes but am not sure whether my buffer allocation is correct.

Which specific part are you unsure about? Perhaps appropriate length?

bayedieng · 2022-01-07T12:41:14Z

More specifically, I'm not sure whether I allotted the correct number of pages required for each buffer. Also is my offsetting correct in the add buffer methods?

nuta · 2022-01-07T13:24:23Z

I think you should read the virtio spec again to understand how struct virtio_blk_req is structured and its relationship with VirtqDescBuffer. 2.6.4 Message Framing might be helpful.

nuta

Out of curiosity, how did you tested this code?

exts/virtio_block/lib.rs

nuta · 2022-01-18T00:39:17Z

exts/virtio_block/lib.rs

+                len: request_header_len,
+            },
+            VirtqDescBuffer::ReadOnlyFromDevice {
+                addr: read_addr.as_paddr(),


Who initializes read_addr?

bayedieng · 2022-01-18T19:59:24Z

In order to test the driver I do the following it still does not write though

        ///attach_irq(pci_device.config().interrupt_line(), move || {
           // device.lock().handle_irq();
        //});

        device.clone().lock().write_to_disk(0, [0x5a].as_slice())
    }

bayedieng · 2022-01-18T20:11:32Z

Who initializes read_addr?

I'm not sure what you mean I allocated memory and created an address just as I did for the header and the status

nuta · 2022-01-18T21:25:28Z

Who initializes read_addr?

I'm not sure what you mean I allocated memory and created an address just as I did for the header and the status

OK let me rephrase my question. What's the purpose of read_addr? Have you tested this code?

bayedieng · 2022-01-18T21:44:15Z

I see what you mean it's a misnomer. It is a virtqueue request for the buffer parameter. As you have previously mentioned there are 3 buffers to be made: request header, request buffer, and status. I will be changing the name.

nuta · 2022-01-19T00:02:34Z

How did you test this code?

bayedieng · 2022-01-19T09:44:36Z

How did you test this code?

By attempting to write to vdisk.img. Also printing the values of each buffer making sure they're appropriate.

bayedieng · 2022-02-12T00:45:57Z

I have tried to write to disk multiple times in a plethora of ways to no avail. If anyone else is intent on continuing this work or writing their own virtio block driver for the kernel you are free to do so. Will be closing this pr.

bayedieng added 7 commits November 16, 2021 19:17

Write Virtio Block instance

1ae5a4f

Merge remote-tracking branch 'upstream/main' into main

8fbb49e

Add traits for block driver

85d4924

Rough layout of block device driver

a336f6c

Handle of PCI and MMIO

5551b39

Virtio Block device id is 2

e5c9d1d

Add block device to qemu

377224c

michalfita reviewed Nov 28, 2021

View reviewed changes

bayedieng added 3 commits November 28, 2021 17:07

Register block device and fix bad formatting

0205689

better formatting

aacc9a3

Resolve virtio_blk bad naming

921d8f2

nuta reviewed Nov 29, 2021

View reviewed changes

bayedieng added 2 commits December 9, 2021 10:07

Merge remote-tracking branch 'upstream/main' into main

5e99c6a

Initialize virtio

7a504c4

nuta reviewed Dec 12, 2021

View reviewed changes

bayedieng added 4 commits December 13, 2021 16:12

Merge remote-tracking branch 'upstream/main' into main

a2dc476

Resolve merge conflict

7a7bb05

fix warning

4ea566a

add 3 descriptor buffers and address latest changes to kernel

1ffaec1

nuta reviewed Dec 19, 2021

View reviewed changes

bayedieng added 3 commits December 19, 2021 14:15

remove block driver trait method call implementation untill block I/O…

4e3f970

… subsystem is complete

Read device config for virtio block driver

dc07f2b

fix warning

1fce2e3

Handle interrupts

bc23f2f

bayedieng marked this pull request as ready for review December 22, 2021 12:03

bayedieng closed this Dec 22, 2021

bayedieng reopened this Dec 22, 2021

fix qemu args

526ce90

bayedieng added 4 commits December 25, 2021 19:51

Remove while loop from interrupt handler

42a4a3c

Update buffers

8273337

fix warnings

0058083

rustfmt

229c0ed

nuta reviewed Dec 27, 2021

View reviewed changes

bayedieng added 2 commits January 14, 2022 11:30

Merge remote-tracking branch 'upstream/main' into main

a3bdd96

No need for offsets upon page allocation

cf601c5

bayedieng requested a review from nuta January 16, 2022 22:27

nuta requested changes Jan 18, 2022

View reviewed changes

bayedieng added 2 commits January 18, 2022 19:46

update buffer offsets

9bb7976

request header offset update

5504382

update comments

f818086

nuta self-requested a review January 26, 2022 16:00

Merge branch 'nuta:main' into main

79ef02e

bayedieng closed this Feb 12, 2022

Implement Virtio Block Driver #108

Implement Virtio Block Driver #108

Conversation

bayedieng commented Nov 26, 2021 • edited Loading

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

bayedieng Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nuta Dec 12, 2021 • edited Loading

Choose a reason for hiding this comment

bayedieng Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bayedieng Dec 13, 2021 • edited Loading

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

bayedieng Dec 19, 2021 • edited Loading

Choose a reason for hiding this comment

nuta commented Dec 19, 2021

bayedieng commented Dec 22, 2021

bayedieng commented Dec 22, 2021

bayedieng commented Dec 25, 2021

nuta commented Dec 26, 2021

bayedieng commented Dec 26, 2021

nuta Dec 27, 2021 • edited Loading

Choose a reason for hiding this comment

bayedieng commented Jan 5, 2022

nuta commented Jan 7, 2022

bayedieng commented Jan 7, 2022

nuta commented Jan 7, 2022

nuta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bayedieng commented Jan 18, 2022 • edited Loading

bayedieng commented Jan 18, 2022

nuta commented Jan 18, 2022

bayedieng commented Jan 18, 2022 • edited Loading

nuta commented Jan 19, 2022

bayedieng commented Jan 19, 2022 • edited Loading

bayedieng commented Feb 12, 2022

bayedieng commented Nov 26, 2021 •

edited

Loading

bayedieng Dec 10, 2021 •

edited

Loading

nuta Dec 12, 2021 •

edited

Loading

bayedieng Dec 13, 2021 •

edited

Loading

bayedieng Dec 13, 2021 •

edited

Loading

bayedieng Dec 19, 2021 •

edited

Loading

nuta Dec 27, 2021 •

edited

Loading

bayedieng commented Jan 18, 2022 •

edited

Loading

bayedieng commented Jan 18, 2022 •

edited

Loading

bayedieng commented Jan 19, 2022 •

edited

Loading