Kernel: Add virtio-blk devices to DeviceManager

[sleirsgoevy]

This was erroneously not done, because the VirtIOBlockDevice is created using a raw new statement, and not using DeviceManager::try_create_device.

[BuggieBot]

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.

[supercomputer7]

This looks like a terrible hack to me. Why do we have to do this?

Also, this is not DeviceManager, but DeviceManagement.

[sleirsgoevy]

Because in StorageDevice this method is private, and we can't call it from a subclass. And in Device it's public, but calls the same method because it's virtual.

[supercomputer7]

This is happening because we don't follow the usual pattern of construction here.

Fix this by looking how we construct other StorageDevice please. For example, you could do this here:

UNMAP_AFTER_INIT ErrorOr<NonnullLockRefPtr<VirtIOBlockDevice>> VirtIOBlockDevice::try_create( ... )
{
    auto device = TRY(DeviceManagement::try_create_device<VirtIOBlockDevice>( ... ));
    TRY(device->initialize_virtio_resources());
    return device;
}

And then on VirtIOBlockController code, do this:

    ...
    auto device = TRY(VirtIOBlockDevice::try_create( ... ));
    TRY(device->initialize_virtio_resources());
    ...

[sleirsgoevy]

It seems that this happens because the constructor is assumed to be infallible, and here the "constructor" (initialize_virtio_resources) can fail. I guess a better way to fix this would be to:

1. Override after_inserting in VirtIOBlockDevice, move logic from initialize_virtio_resources there, then tailcall to the original method.
2. Make VirtIOBlockController use try_create.

The obvious issue with try_create is that it would insert a partially-initialized device into a global hash table, which is probably not what we want.

[supercomputer7]

The C++ constructor should not have anything that can logically fail. Failing can happen on a static construction method though, which is probably what we want.
Please go forward with option 1, because that's what we do essentially for any other Device-derived class in the kernel codebase.

If you want to ask for help with this, I'm on the discord server on #kernel channel, so feel free to tag me on any question and I'd gladly help if I can.

EDIT: Option 1 could be OK. but I'd like to review your changes again to see what you come up with because I'm not sure now this would be actually what I thought.

[supercomputer7]

Another idea - why don't we make VirtIOBlockController class the VirtIO::Device? After all, the block device on itself is not really the VirtIO device, right?
That would make it much more reasonable to let the creation of an exposed char device to fail, and the actual handling of VirtIO stuff will move to VirtIOBlockController class instead which makes far more sense to me.

[supercomputer7]

Because I'd like to make things go further, I wrote a small patch demonstrating what I suggested here in the previous comment:
https://github.com/supercomputer7/serenity/tree/fix-virtio-block-wrong-init-path

Specifically it has only one commit here:
supercomputer7@22dbd68

It has some rough edges (presumably I didn't really care about changing the debug messages), but you get the idea what I want.

I think this is the way forward. I also really don't like that we hardcode the max size (i.e. max LBA) of the VirtIO device, but that's a problem for another PR, not this one.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!