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.

Sign up for GitHub

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

Jump to bottom

Kernel: Add virtio-blk devices to DeviceManager #24692

Closed
wants to merge 1 commit into from
Closed

Kernel: Add virtio-blk devices to DeviceManager #24692

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion Kernel/Devices/Storage/VirtIO/VirtIOBlockDevice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,10 @@ UNMAP_AFTER_INIT ErrorOr<void> VirtIOBlockDevice::initialize_virtio_resources()
}));
TRY(setup_queues(1)); // REQUESTQ
finish_init();
return {};

// This device is not added to DeviceManagement, as we've bypassed try_create_device, so we need to add it ourselves
// We can't call the method directly, as it is redeclared as private in the StorageDevice class, thus the cast
return static_cast<Kernel::Device*>(this)->after_inserting();
sleirsgoevy marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

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

Also, this is not DeviceManager, but DeviceManagement.

Copy link
Contributor Author

@sleirsgoevy sleirsgoevy Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@supercomputer7 supercomputer7 Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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());
    ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@supercomputer7 supercomputer7 Jul 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@supercomputer7 supercomputer7 Jul 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

}

ErrorOr<void> VirtIOBlockDevice::handle_device_config_change()
Expand Down
Loading