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: `fd051c2`	Previous: `caef4ad`	Ratio
`Dhrystone`	`1299` Average DMIPS over 10 runs	`1336` Average DMIPS over 10 runs	`1.03`
`Coremark`	`959.33` Average iterations/sec over 10 runs	`974.943` Average iterations/sec over 10 runs	`1.02`

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

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.

Can we simply use cpio or ar instead?

Can we simply use cpio or ar instead?

Neither cpio, ar nor zcat recognizes the image file, and after a quick review of their man pages, I couldn’t find any useful options to address this issue.

We can verify the success of filesystem creation by checking the return code from mkfs.ext4 directly, rather than using additional tools like 7z. Since 7z is not included in standard Linux distributions, avoiding such external dependencies makes the testing process more accessible to other developers and simplifies collaboration.

We can verify the success of filesystem creation by checking the return code from mkfs.ext4 directly, rather than using additional tools like 7z. Since 7z is not included in standard Linux distributions, avoiding such external dependencies makes the testing process more accessible to other developers and simplifies collaboration.

I completely agree that avoiding extra tools like 7z would significantly improve portability and make the testing process more accessible. However, checkin the return code of mkfs.ext4 merely verifies that the filesystem was created, but it does not confirm that Virtio-blk is functioning correctly.
For example:

$ dd if=/dev/zero of=disk.img bs=4M count=32 32+0 records in 32+0 records out 134217728 bytes (134 MB, 128 MiB) copied, 0.314109 s, 427 MB/s $ mkfs.ext4 disk.img mke2fs 1.47.0 (5-Feb-2023) Discarding device blocks: done Creating filesystem with 32768 4k blocks and 32768 inodes Allocating group tables: done Writing inode tables: done Creating journal (4096 blocks): done Writing superblocks and filesystem accounting information: done $ mkfs.ext4 disk.img mke2fs 1.47.0 (5-Feb-2023) disk.img contains a ext4 file system created on Mon Jan 27 23:22:21 2025 Proceed anyway? (y,N) n $ echo $? 1

As shown above, the second call to mkfs.ext4 returns 1 because the filesystem was already created by the first call, even though the emulator was never started.

src/devices/virtio-blk.c

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?

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

Certainly, multiple virtual block devices can be attached. However, after a quick review, it appears that each vblk device requires an independent virtqueue at least. Additionally, unlike QEMU, rv32emu’s current implementation uses a hardcoded device tree at build time. This could lead to significant complications with MMIO register mapping and negatively impact performance due to the extra polling required.

If possible, I would prefer to leave this for future extension, when the device tree and MMIO mapping can be adjusted at runtime.

I would prefer to leave this for future extension, when the device tree and MMIO mapping can be adjusted at runtime.

Add corresponding FIXME/TODO comments.

src/devices/virtio-blk.c

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


.ci/boot-linux.sh

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.

.ci/boot-linux.sh

jserv

Rebase the latest master branch to resolve Arm64 host breakage on CI.

src/devices/virtio-blk.c

otteryc · 2025-01-30T16:47:21Z

HELP WANTED

I found that the current implementation is incorrect when HAVE_MMAP is set to 0. While testing, I encountered a compiler error due to an undeclared function in lines 53 and 80 related to getpagesize(). This function is declared in unistd.h in Unix-like systems, but unistd.h is not included when HAVE_MMAP is 0.

I am currently not in Tainan and do not have access to an MSVC environment. Therefore, I am unsure whether this is a bug or if getpagesize() is properly defined in other MSVC headers. Any insights or verification would be greatly appreciated!

jserv · 2025-01-30T16:54:12Z

I am currently not in Tainan and do not have access to an MSVC environment. Therefore, I am unsure whether this is a bug or if getpagesize() is properly defined in other MSVC headers. Any insights or verification would be greatly appreciated!

MSVC support can be disregarded as it is not functional. While initial testing was performed on Windows 8 during the my original code snapshot in 2020, there has never been a working Windows build of rv32emu. Of course, if you are interested in a Windows port, pull requests are always welcome. At present, concentrate on the condition HAVE_MMAP=1.

ChinYikMing · 2025-01-31T07:22:37Z

.ci/boot-linux.sh

+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
+    OPTS+=" -x vblk:build/disk.img "


build/disk.img is repeated in the changes. Replace them with a variable such as VBLK_IMG=build/disk.img and access it via $(VBLK_IMG).

ChinYikMing · 2025-01-31T07:24:36Z

.ci/boot-linux.sh

+printf "\nBoot Linux Test: [ ${COLOR_G}${MESSAGES[$ret]}${COLOR_N} ]\n"
+if [ "$ENABLE_VBLK" -eq "1" ]; then 
+    file_list=$(7z l build/disk.img)
+    (echo $file_list | grep emu.txt >/dev/null 2>&1) || ret=4


Since file_list is only accessed here, is it better to streamline them into one line by removing file_list variable?

ChinYikMing · 2025-01-31T14:41:58Z

.ci/boot-linux.sh

 ret=$?
 cleanup

 MESSAGES=("OK!" \
     "Fail to boot" \
     "Fail to login" \
     "Fail to run commands" \
+     "Fail to find emu.txt in $VBLK_IMG"\
 )

 COLOR_G='\e[32;01m' # Green


Introduce red color here such that: COLOR_R='\e[31;01m' # Red

ChinYikMing · 2025-01-31T14:44:54Z

.ci/boot-linux.sh

+printf "\nBoot Linux Test: [ ${COLOR_G}${MESSAGES[$ret]}${COLOR_N} ]\n"
+if [ "$ENABLE_VBLK" -eq "1" ]; then 
+    7z l $VBLK_IMG | grep emu.txt >/dev/null 2>&1 || ret=4
+    printf "Virtio-blk Test: [ ${COLOR_G}${MESSAGES[$ret]}${COLOR_N} ]\n"


Show the VirtIO block error message (ret=4) in red instead of green since it is typically used for success.

Show the VirtIO block error message (ret=4) in red instead of green since it is typically used for success.

Shall other error messages be changed into red as well?

Show the VirtIO block error message (ret=4) in red instead of green since it is typically used for success.

Shall other error messages be changed into red as well?

Yes, let's make them consistent. Thanks!

jserv

Rebase the latest master branch and fix conflicts.

@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 5. Fix the size of addr in virtq_desc structure. 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. This commit also removes -b argument when starting the emulator, as it is refined to be used to accept custom bootargs.

jserv · 2025-02-01T15:44:10Z

Thank @otteryc for contributing!

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

ChinYikMing reviewed Jan 25, 2025

View reviewed changes

README.md Outdated Show resolved Hide resolved

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

.ci/boot-linux.sh Outdated Show resolved Hide resolved

shengwen-tw reviewed Jan 26, 2025

View reviewed changes

jserv reviewed Jan 27, 2025

View reviewed changes

.ci/boot-linux.sh Outdated Show resolved Hide resolved

jserv requested changes Jan 27, 2025

View reviewed changes

otteryc force-pushed the vblk branch 3 times, most recently from 8583029 to bce4c85 Compare January 28, 2025 17:53

jserv reviewed Jan 29, 2025

View reviewed changes

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

otteryc force-pushed the vblk branch from bce4c85 to 268988f Compare January 30, 2025 16:31

ChinYikMing reviewed Jan 31, 2025

View reviewed changes

otteryc force-pushed the vblk branch 2 times, most recently from 5770932 to d0ecf72 Compare January 31, 2025 13:25

ChinYikMing reviewed Jan 31, 2025

View reviewed changes

otteryc force-pushed the vblk branch 2 times, most recently from 803d241 to caef4ad Compare February 1, 2025 15:17

This comment was marked as resolved.

Sign in to view

jserv requested changes Feb 1, 2025

View reviewed changes

otteryc and others added 2 commits February 1, 2025 23:39

otteryc force-pushed the vblk branch from caef4ad to fd051c2 Compare February 1, 2025 15:40

jserv merged commit 6e6cfb8 into sysprog21:master Feb 1, 2025
8 of 9 checks passed

Implement VirtIO block device #539

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

Choose a reason for hiding this comment

otteryc Jan 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otteryc Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otteryc Jan 27, 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

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

shengwen-tw Jan 26, 2025 • edited Loading

Choose a reason for hiding this comment

jserv left a comment

Choose a reason for hiding this comment

otteryc commented Jan 30, 2025

jserv commented Jan 30, 2025

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

This comment was marked as resolved.

jserv left a comment

Choose a reason for hiding this comment

jserv commented Feb 1, 2025

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

otteryc Jan 26, 2025 •

edited

Loading

otteryc Jan 27, 2025 •

edited

Loading

otteryc Jan 27, 2025 •

edited

Loading

ChinYikMing Jan 26, 2025 •

edited

Loading

ChinYikMing Jan 26, 2025 •

edited

Loading

shengwen-tw Jan 26, 2025 •

edited

Loading