Implement VirtIO block device #539

otteryc · 2025-01-23T05:22:31Z

This commit migrates @shengwen-tw's brilliant virtio implementation from semu, with the following modification:

Rename virtio_blk_reg_read/write to virtio_blk_read/write The original virtio_blk_read verified whether the virtio MMIO was aligned to 4. Since rv32emu does not enforce alignment checks for UART and PLIC devices or raise misalignment exceptions, this verification has been omitted.
Implement MMIO_VIRTIOBLK
Implement vblk_new() and vblk_delete()
These functions align with the conventions used for UART and PLIC devices.
Introduce new argument '-x vblk:<disk_image>' for virtio-blk disk image

This pull request has been tested on:

Linux 6.8.0 @ Intel core i7-7700K
Mac OS Sequoia 15.1 @ Apple Silicon M2

To try out virtio-blk:
Generate ext4 image file for virtio block device in Unix-like system:

$ dd if=/dev/zero of=disk.img bs=4M count=32
$ mkfs.ext4 disk.img

Boot the Guest OS

$ ./build/rv32emu -k <kernel_img_path> -i <rootfs_img_path> -x vblk:disk.img

Mount the virtual block device and create a test file after booting:

$ mkdir mnt
$ mount /dev/vda mnt
$ echo "rv32emu" > mnt/emu.txt
$ umount mnt

Reboot the system and re-mount the virtual block device, the written file should remain existing.

jserv

Benchmarks

Benchmark suite	Current: `b2d8304`	Previous: `613cd0a`	Ratio
`Dhrystone`	`1334` Average DMIPS over 10 runs	`1346` Average DMIPS over 10 runs	`1.01`
`Coremark`	`964.709` Average iterations/sec over 10 runs	`972.884` Average iterations/sec over 10 runs	`1.01`

This comment was automatically generated by workflow using github-action-benchmark.

src/devices/virtio-blk.c

src/system.c

jserv

Specify Shengwen Cheng as co-author in git commit messages. See https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors
Consolidate GitHub Actions to automate virtio-blk test procedure.

src/devices/virtio-blk.c

src/main.c

vacantron · 2025-01-23T07:54:02Z

Does it have any instructions / documentations for demonstrating or testing of this newly added feature?

ChinYikMing · 2025-01-23T08:45:29Z

rv32emu use snake_case naming scheme (check CONTRIBUTING.md), but I see CamelCase in the implementation. @otteryc Can you unify the naming scheme or any strong reason to use CamelCase?

otteryc · 2025-01-23T14:08:50Z

@vacantron @ChinYikMing Thank you for the insightful comments.
CamelCase names have been replaced with snake_case, and a new section has been added to the README to demonstrate virtio-blk.

src/devices/virtio-blk.c

src/devices/virtio.h

src/main.c

src/devices/virtio.h

src/devices/virtio-blk.c

ChinYikMing

Change the description of this pull request, adding information for others to build the system emulator and test the virtio-blk feature. This will make it easier for others to access the information without needing to refer to the README but also keep them in README :) .

jserv · 2025-01-23T15:44:54Z

Compilation warnings reported by GCC:

src/devices/virtio-blk.c: In function ‘vblk_preprocess’:
src/devices/virtio-blk.c:80:60: error: unused parameter ‘vblk’ [-Werror=unused-parameter]
   80 | static inline uint[32](https://github.com/sysprog21/rv32emu/actions/runs/12930766138/job/36065876824?pr=539#step:9:33)_t vblk_preprocess(virtio_blk_state_t *vblk, uint32_t addr)
      |                                        ~~~~~~~~~~~~~~~~~~~~^~~~

Simply add UNUSED (see src/common.h) to get rid of the above.

README.md

src/devices/virtio-blk.c

RinHizakura · 2025-01-23T16:54:40Z

src/main.c

@@ -118,6 +120,13 @@ static bool parse_args(int argc, char **args)
            opt_bootargs = optarg;
            emu_argc++;
            break;
+        case 'x':
+            if (!strncmp("vblk:", optarg, 5))


Why must we introduce such complexity with the vblk: prefix? The brilliant way is to use just the disk file name here and consider adding the long option for different block devices if required.

Why must we introduce such complexity with the vblk: prefix?

VirtIO features like disk, network, and sound can be dynamically enabled using the -x option, inspired by Oracle/Sun Java virtual machine. For example: -x vblk:<disk> -x vnet:tap0 -x vsnd:alsa. The long command options might be confusing if we want to specify more VirtIO devices, such as a secondary virtio-blk.

Okay. Thanks

RinHizakura · 2025-01-23T17:11:34Z

src/riscv.h

+
+    /* virtio-blk device */
+    uint32_t *disk;
+    virtio_blk_state_t *vblk;


Ok it seems like UART and VIRTIO_BLK come from semu, and PLIC may have a little bit different design here......

I am just thinking of wrapping these things in virtio_blk_t, but maybe we can refactor these in other PR if required. This is not a critical issue.

README.md

Makefile

ChinYikMing · 2025-01-24T06:16:42Z

.ci/virtio-blk.sh

@@ -0,0 +1,54 @@
+#!/usr/bin/env bash


Can you reuse the script of boot Linux instead of creating a new one? By adjusting the script to accept a CLI parameter that determines whether to test the virtio-blk feature.

jserv · 2025-01-24T06:53:48Z

.ci/virtio-blk.sh

+RUN_LINUX="build/rv32emu ${OPTS}"
+
+dd if=/dev/zero of=build/disk.img bs=4M count=32
+mkfs.ext4 build/disk.img


Check the presence of mkfs.ext4. That is, something like https://github.com/sysprog21/semu/blob/master/Makefile#L20-L32

README.md

@shengwen-tw

This commit migrates @shengwen-tw's brilliant virtio implementation from semu, with the following modification: 1. Rename virtio_blk_reg_read/write to virtio_blk_read/write The original virtio_blk_read verified whether the virtio MMIO was aligned to 4. Since rv32emu does not enforce alignment checks for UART and PLIC devices or raise misalignment exceptions, this verification has been omitted. 2. Implement MMIO_VIRTIOBLK 3. Implement vblk_new() and vblk_delete() These functions align with the conventions used for UART and PLIC devices. 4. Introduce new argument '-x vblk:<disk_image>' for virtio-blk disk image Co-authored-by: Shengwen Cheng <[email protected]>

Virtio-blk verification has been integrated into boot-linux test, and is enabled by default. The p7zip-full package, providing command '7z', is added to dependencies in order to validate the success of writes to the Virtio-blk device.

jserv · 2025-01-26T06:30:27Z

.ci/boot-linux.sh

+ENABLE_VBLK=1
+type dd >/dev/null 2>&1 || ENABLE_VBLK=0
+(type mkfs.ext4 >/dev/null 2>&1 || type $(brew --prefix e2fsprogs)/sbin/mkfs.ext4) >/dev/null 2>&1 || ENABLE_VBLK=0
+type 7z >/dev/null 2>&1 || ENABLE_VBLK=0


Why is 7z required?

I believe using 7z to list files in disk.img is the simplest and most efficient method to verify whether the file emu.txt written by the guest OS exists.
Other approaches have limitations:

Mounting disk.img requires root privileges, which may not always be feasible.

Inspecting the image with debugfs provides the desired result but involves navigating a complex interactive interface.

dumpe2fs fail with an error, "Wrong magic number for Ext2 Image Header while trying to open disk.img" when attempting to process disk.img.

ChinYikMing · 2025-01-26T06:33:12Z

src/devices/virtio-blk.c

+    case _(QueueNumMax):
+        return VBLK_QUEUE_NUM_MAX;
+    case _(QueueReady):
+        return VBLK_QUEUE.ready ? 1 : 0;


Rewrite as: return (uint32_t) VBLK_QUEUE.ready since VBLK_QUEUE.ready is a boolean that already evaluates to either true or false.

ChinYikMing · 2025-01-26T06:37:08Z

src/devices/virtio-blk.c

+
+#define DISK_BLK_SIZE 512
+
+#define VBLK_DEV_CNT_MAX 1


Hmm, does this refer to the virtio block limit? I believe it can attach multiple virtual block devices, can't it?

ChinYikMing · 2025-01-26T06:38:38Z

src/devices/virtio-blk.c

+#define _(reg) VIRTIO_##reg
+    switch (addr) {
+    case _(MagicValue):
+        return 0x74726976;


Define these magic value to a semantic name, just like VIRTIO_VENDOR_ID indicates the vendor ID.

ChinYikMing · 2025-01-26T06:38:45Z

src/devices/virtio-blk.c

+    case _(MagicValue):
+        return 0x74726976;
+    case _(Version):
+        return 2;


ChinYikMing · 2025-01-26T06:38:50Z

src/devices/virtio-blk.c

+    case _(Version):
+        return 2;
+    case _(DeviceID):
+        return 2;


ChinYikMing · 2025-01-26T06:39:01Z

src/devices/virtio-blk.c

+    case _(Status):
+        return vblk->status;
+    case _(ConfigGeneration):
+        return 0;


ChinYikMing · 2025-01-26T06:50:43Z

.ci/boot-linux.sh

 TIMEOUT=50
 OPTS=" -k build/linux-image/Image "
 OPTS+=" -i build/linux-image/rootfs.cpio "
 OPTS+=" -b build/minimal.dtb "
+if [ "$ENABLE_VBLK" -eq "1" ]; then
+    dd if=/dev/zero of=build/disk.img bs=4M count=32
+    mkfs.ext4 build/disk.img || $(brew --prefix e2fsprogs)/sbin/mkfs.ext4 build/disk.img


No macOS CI is integrated yet. Is it suitable to include brew utility since we do not actually use it for now?

This was added based on this review. While we do not currently utilize the brew utility, its inclusion is harmless and imposes minimal runtime overhead.

ChinYikMing · 2025-01-26T06:51:21Z

.ci/boot-linux.sh

@@ -16,31 +16,62 @@ function ASSERT {

 cleanup

+ENABLE_VBLK=1
+type dd >/dev/null 2>&1 || ENABLE_VBLK=0
+(type mkfs.ext4 >/dev/null 2>&1 || type $(brew --prefix e2fsprogs)/sbin/mkfs.ext4) >/dev/null 2>&1 || ENABLE_VBLK=0


ChinYikMing · 2025-01-26T06:55:11Z

.ci/boot-linux.sh

-printf "\n[ ${COLOR_G}${MESSAGES[$ret]}${COLOR_N} ]\n"
+printf "\nBoot Linux Test: [ ${COLOR_G}${MESSAGES[$ret]}${COLOR_N} ]\n"
+if [ "$ENABLE_VBLK" -eq "1" ]; then 
+    file_list=`7z l build/disk.img`


Rewrite as file_list= $(7z l build/disk.img)

ChinYikMing · 2025-01-26T07:13:28Z

.ci/boot-linux.sh

+type dd >/dev/null 2>&1 || ENABLE_VBLK=0
+(type mkfs.ext4 >/dev/null 2>&1 || type $(brew --prefix e2fsprogs)/sbin/mkfs.ext4) >/dev/null 2>&1 || ENABLE_VBLK=0
+type 7z >/dev/null 2>&1 || ENABLE_VBLK=0
+
 TIMEOUT=50
 OPTS=" -k build/linux-image/Image "
 OPTS+=" -i build/linux-image/rootfs.cpio "
 OPTS+=" -b build/minimal.dtb "


I believe this can be remove since -b has been refined to be used to accept custom bootargs. Cc. @RinHizakura

Should it be removed within this pull request?

Should it be removed within this pull request?

Yes, go ahead. Please also mention the rationale in the commit message.

shengwen-tw · 2025-01-26T08:43:16Z

src/devices/virtio.h

+};
+
+struct virtq_desc {
+    uint32_t addr;


According to Virtual I/O Device (VIRTIO) Version 1.3

2.7.5 The Virtqueue Descriptor Table

struct virtq_desc { /* Address (guest-physical). */ le64 addr; /* Length. */ le32 len; /* The flags as indicated above. */ le16 flags; /* We chain unused descriptors via this, too */ le16 next; };

We mistakenly defined virtq_desc.addr as 32 bits.
Please consider correcting it here and submitting a pull request for semu.

jserv reviewed Jan 23, 2025

View reviewed changes

jserv requested review from RinHizakura, ChinYikMing and vacantron January 23, 2025 05:50

jserv reviewed Jan 23, 2025

View reviewed changes

src/devices/virtio-blk.c Show resolved Hide resolved

otteryc force-pushed the vblk branch from b6bd4b2 to 9a4ed2b Compare January 23, 2025 05:56

jserv reviewed Jan 23, 2025

View reviewed changes

src/system.c Outdated Show resolved Hide resolved

jserv requested changes Jan 23, 2025

View reviewed changes

jserv reviewed Jan 23, 2025

View reviewed changes

src/devices/virtio-blk.c Outdated Show resolved Hide resolved

otteryc force-pushed the vblk branch from 9a4ed2b to 202fbd9 Compare January 23, 2025 06:44

jserv reviewed Jan 23, 2025

View reviewed changes

src/main.c Outdated Show resolved Hide resolved

jserv changed the title ~~Migrate virtio-blk from semu~~ Implement VirtIO block device Jan 23, 2025

jserv added this to the release-2025.1 milestone Jan 23, 2025

otteryc force-pushed the vblk branch from 202fbd9 to 55c592f Compare January 23, 2025 13:59

otteryc commented Jan 23, 2025

View reviewed changes

src/devices/virtio-blk.c Outdated Show resolved Hide resolved

ChinYikMing reviewed Jan 23, 2025

View reviewed changes

otteryc force-pushed the vblk branch 3 times, most recently from cb15ab4 to f513ee7 Compare January 23, 2025 16:23

ChinYikMing reviewed Jan 23, 2025

View reviewed changes

README.md Outdated Show resolved Hide resolved

jserv reviewed Jan 23, 2025

View reviewed changes

src/devices/virtio-blk.c Outdated Show resolved Hide resolved

otteryc force-pushed the vblk branch from f513ee7 to 382ccf7 Compare January 23, 2025 16:53

RinHizakura reviewed Jan 23, 2025

View reviewed changes

ChinYikMing reviewed Jan 23, 2025

View reviewed changes

README.md Outdated Show resolved Hide resolved

RinHizakura approved these changes Jan 23, 2025

View reviewed changes

otteryc force-pushed the vblk branch 2 times, most recently from 8355697 to 40bca99 Compare January 24, 2025 04:40

ChinYikMing reviewed Jan 24, 2025

View reviewed changes

README.md Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

otteryc force-pushed the vblk branch 2 times, most recently from 48536bf to f475a45 Compare January 24, 2025 05:54

ChinYikMing reviewed Jan 24, 2025

View reviewed changes

jserv reviewed Jan 24, 2025

View reviewed changes

ChinYikMing mentioned this pull request Jan 24, 2025

Support access to block devices in the /dev directory #544

Open

otteryc force-pushed the vblk branch 2 times, most recently from d46348c to 15a105c Compare January 25, 2025 13:59

ChinYikMing reviewed Jan 25, 2025

View reviewed changes

README.md Outdated Show resolved Hide resolved

otteryc and others added 2 commits January 26, 2025 10:24

CI: Add virtio-blk verification

b2d8304

Virtio-blk verification has been integrated into boot-linux test, and is enabled by default. The p7zip-full package, providing command '7z', is added to dependencies in order to validate the success of writes to the Virtio-blk device.

otteryc force-pushed the vblk branch from 15a105c to b2d8304 Compare January 26, 2025 02:25

jserv requested a review from ChinYikMing January 26, 2025 06:29

jserv reviewed Jan 26, 2025

View reviewed changes

ChinYikMing reviewed Jan 26, 2025

View reviewed changes

shengwen-tw reviewed Jan 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement VirtIO block device #539

Implement VirtIO block device #539

otteryc commented Jan 23, 2025 •

edited

Loading

jserv left a comment •

edited

Loading

jserv left a comment

vacantron commented Jan 23, 2025

ChinYikMing commented Jan 23, 2025

otteryc commented Jan 23, 2025

ChinYikMing left a comment

jserv commented Jan 23, 2025 •

edited

Loading

RinHizakura Jan 23, 2025

jserv Jan 23, 2025

RinHizakura Jan 23, 2025

RinHizakura Jan 23, 2025 •

edited

Loading

ChinYikMing Jan 24, 2025 •

edited

Loading

jserv Jan 24, 2025

jserv Jan 26, 2025

otteryc Jan 26, 2025 •

edited

Loading

ChinYikMing Jan 26, 2025 •

edited

Loading

ChinYikMing Jan 26, 2025

ChinYikMing Jan 26, 2025

ChinYikMing Jan 26, 2025

ChinYikMing Jan 26, 2025

ChinYikMing Jan 26, 2025

ChinYikMing Jan 26, 2025 •

edited

Loading

otteryc Jan 26, 2025

ChinYikMing Jan 26, 2025 •

edited

Loading

ChinYikMing Jan 26, 2025

ChinYikMing Jan 26, 2025

otteryc Jan 26, 2025

ChinYikMing Jan 27, 2025

shengwen-tw Jan 26, 2025 •

edited

Loading

Implement VirtIO block device #539

Are you sure you want to change the base?

Implement VirtIO block device #539

Conversation

otteryc commented Jan 23, 2025 • edited Loading

jserv left a comment • edited Loading

Choose a reason for hiding this comment

Benchmarks

jserv left a comment

Choose a reason for hiding this comment

vacantron commented Jan 23, 2025

ChinYikMing commented Jan 23, 2025

otteryc commented Jan 23, 2025

ChinYikMing left a comment

Choose a reason for hiding this comment

jserv commented Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RinHizakura Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

ChinYikMing Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otteryc Jan 26, 2025 • edited Loading

Choose a reason for hiding this comment

ChinYikMing Jan 26, 2025 • 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

Choose a reason for hiding this comment

ChinYikMing Jan 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChinYikMing Jan 26, 2025 • 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

shengwen-tw Jan 26, 2025 • edited Loading

Choose a reason for hiding this comment

otteryc commented Jan 23, 2025 •

edited

Loading

jserv left a comment •

edited

Loading

jserv commented Jan 23, 2025 •

edited

Loading

RinHizakura Jan 23, 2025 •

edited

Loading

ChinYikMing Jan 24, 2025 •

edited

Loading

otteryc Jan 26, 2025 •

edited

Loading

ChinYikMing Jan 26, 2025 •

edited

Loading

ChinYikMing Jan 26, 2025 •

edited

Loading

ChinYikMing Jan 26, 2025 •

edited

Loading

shengwen-tw Jan 26, 2025 •

edited

Loading