Skip to content
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

Merged
merged 1 commit into from May 23, 2024
Merged

Conversation

nichtverstehen
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label May 13, 2024
@BuggieBot
Copy link
Member

Hello!

One or more of the commit messages in this PR do not match the SerenityOS code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

static constexpr u64 VIRTIO_BLK_S_IOERR = 1;
static constexpr u64 VIRTIO_BLK_S_UNSUPP = 2;

struct [[gnu::packed]] virtio_blk_config {
Copy link
Member

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:

  1. Move these definitions into a Defintions.h with the license comment header reproduced in full
  2. Add an exception for that file in our check-license-headers linter script
  3. Change the struct names to follow our style? (i.e. PascalCase rather than snake_case)

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@ADKaster ADKaster added ⏳ pr-waiting-for-author PR is blocked by feedback / code changes from the author and removed 👀 pr-needs-review PR needs review from a maintainer or community member labels May 14, 2024
@github-actions github-actions bot added 👀 pr-needs-review PR needs review from a maintainer or community member and removed ⏳ pr-waiting-for-author PR is blocked by feedback / code changes from the author labels May 14, 2024
Copy link
Member

@ADKaster ADKaster left a 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 ™️

@ADKaster ADKaster merged commit 99f6528 into SerenityOS:master May 23, 2024
13 checks passed
@github-actions github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label May 23, 2024
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.

None yet

4 participants