Skip to content

feat: add virtio-blk definitions - #33

Merged
mkroening merged 1 commit into
mainfrom
virtio-blk
Sep 7, 2026
Merged

mkroening merged 1 commit into
mainfrom
virtio-blk

Conversation

@mkroening

Copy link
Copy Markdown
Member

This PR adds the virtio-blk definitions. This is a combination of Kreb216@3f41c1e and stlankes@94d7608.

I have reworked the code to adhere to the style and naming conventions of this project. I have also added more definitions. I have tested this with the driver of @Kreb216 and the driver of @stlankes and both compile after adjusting a few names.

I have removed RequestHeader from @stlankes. Ideally, we would have a virtio::blk::Req struct corresponding to the definition in the spec:

struct virtio_blk_req { 
        le32 type; 
        le32 reserved; 
        le64 sector; 
        u8 data[]; 
        u8 status; 
};

I did model this, and it works well from a Rust type perspective, but the first three fields and the last one are written by the driver, and the data is written by the device, though. This requires having three descriptors for one type, which does not work in the Hermit driver at the moment. Since it should be possible in principle, though, I would like to keep any implementation-specific helper types such as a header out of this project. It works fine to define the type in the Hermit driver, though.

@Kreb216, I cannot request a review from you through GitHub, but you are welcome to give this a try and leave comments if you have any. :)

This PR does not conflict with, but depends on, the following PRs:

Closes #28.

Co-authored-by: Stefan Lankes <slankes@eonerc.rwth-aachen.de>
Co-authored-by: Martin Kröning <martin.kroening@eonerc.rwth-aachen.de>
@mkroening
mkroening requested review from phip1611 and stlankes August 21, 2026 07:44
@mkroening mkroening self-assigned this Aug 21, 2026

@phip1611 phip1611 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but please consider adding context (why you performed certain changes) to the commit messages.

Comment thread src/blk.rs
@stlankes

stlankes commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

It looks good to me. It's just the CI errors that bother me.

@mkroening

Copy link
Copy Markdown
Member Author

The CI status is expected. It's just cargo-semver-checks telling us that we need to do a major release.

I'll merge this later. :)

@Kreb216

Kreb216 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

It looks good to me as well.

@stlankes

stlankes commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

The CI status is expected. It's just cargo-semver-checks telling us that we need to do a major release.

I'll merge this later. :)

Hm, I was confused by the error message:

failure trait_added_supertrait: non-sealed trait added new supertraits

For me, it doesn't sound that we have to increase the version number. :-)

@mkroening

Copy link
Copy Markdown
Member Author

failure trait_added_supertrait: non-sealed trait added new supertraits

For me, it doesn't sound that we have to increase the version number. :-)

You are wrong. ^^

https://doc.rust-lang.org/cargo/reference/semver.html#generic-bounds-tighten

In this case, they were only moved, but other changes are breaking anyway.

@stlankes

stlankes commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

failure trait_added_supertrait: non-sealed trait added new supertraits

For me, it doesn't sound that we have to increase the version number. :-)

You are wrong. ^^

I know. Yesterday I just read the message. Today, I clicked on the link :-)

@mkroening
mkroening added this pull request to the merge queue Sep 7, 2026
Merged via the queue into main with commit 40e4abf Sep 7, 2026
12 of 14 checks passed
@phip1611

phip1611 commented Sep 7, 2026

Copy link
Copy Markdown
Member

🚀

@phip1611
phip1611 deleted the virtio-blk branch September 7, 2026 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants