-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Kernel: Implement virtio-blk driver. #24308
Conversation
Hello! One or more of the commit messages in this PR do not match the SerenityOS code submission policy, please check the |
1382d8f
to
9be0c84
Compare
static constexpr u64 VIRTIO_BLK_S_IOERR = 1; | ||
static constexpr u64 VIRTIO_BLK_S_UNSUPP = 2; | ||
|
||
struct [[gnu::packed]] virtio_blk_config { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These definitions look very similar to those from the Linux UAPI.
https://elixir.bootlin.com/linux/v6.9/source/include/uapi/linux/virtio_blk.h
https://github.com/qemu/qemu/blob/9360070196789cc8b9404b2efaf319384e64b107/include/standard-headers/linux/virtio_blk.h
Which is fine because the header explicit says it's licensed as BSD 3-clause so that other projects can implement drivers, but those headers are third-party and need their license intact if we are going to goink them.
Can you:
- Move these definitions into a
Defintions.h
with the license comment header reproduced in full - Add an exception for that file in our check-license-headers linter script
- Change the struct names to follow our style? (i.e. PascalCase rather than snake_case)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those definitions look like they probably are just from the virtio spec.
I'm not sure under what license the spec code snippets are though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review!
The definitions are in fact not copied from any other project. I didn't look at the Linux kernel implementation at all and did not copy QEMU definitions (although did look at them).
The definitions come directly from the spec here:
https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-2740002
Added a reference to the spec.
I am not 100% sure that we don't need to add the copyright notice of the spec authors (belonging to OASIS Open). But neither QEMU not Linux kernel reference OASIS copyright. So I guess, that's fine?..
Now about the names. I strived to keep modifications to the definitions from the spec to the minimum. I felt like it would be unnecessarily confusing if we change the naming style compared to the spec. Note, that the "non-compliant names" are localised to the VirtIO namespace.
However vaguely remember having the same discussion in #19757 and I ended up updating to PascalCase. So did the same here.
206ed26
to
4533fc1
Compare
6342e6e
to
af6e18e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad to see more work on VirtIO devices! Hopefully using these guys makes it more pleasant to work with QEMU on non-x86 systems soon ™️
No description provided.