[RFC] Block device proposal #32
Conversation
Use [patch] sections in the workspace Cargo.toml file to redirect dependencies to custom repos/branches that are not upstream yet. Also switched to abort on panic for both debug and release builds. Enabled lto and codegen-unit = 1 for release builds (and they take longer now). Signed-off-by: Alexandru Agache <[email protected]>
The virtio device implementations are going to be added to a local devices crate, so renaming the module with the same name within vmm to serial (because there's only a serial device there for the time being). Going foward, we should move the serial console to the devices crate as well. Signed-off-by: Alexandru Agache <[email protected]>
Signed-off-by: Alexandru Agache <[email protected]>
Applied some changes to the vmm to support the creation of block devices. There's not cmdline support to configure them yet so there's one hardcoded block device for demo purposes for now. It will always attempt to open a file named 'disk.ext4` from the current working directory when running the VMM. Signed-off-by: Alexandru Agache <[email protected]>
|
|
||
| let irqfd = EventFd::new(EFD_NONBLOCK).map_err(Error::EventFd)?; | ||
| args.vm_fd | ||
| .register_irqfd(&irqfd, args.mmio_cfg.gsi) |
There was a problem hiding this comment.
In the spirit of #14 I'd like to see this done elsewhere (run()?). The empty Block can hang around inert without a VmFd until it really needs to become a live device.
There was a problem hiding this comment.
I would really like to try out this approach because it helps delegate all the setup logic to the device, as long as the necessary "resources" (such as the VmFd for this particular implementation) are provided by the environment.
| Unsupported(u32), | ||
| } | ||
|
|
||
| impl From<u32> for RequestType { |
There was a problem hiding this comment.
I think it'd make more sense to couple this RFC with the other block device-related RFC rust-vmm/vm-virtio#29 and use the abstractions from there.
There was a problem hiding this comment.
It could be a good moment to do this, indeed. Do we plan to do this in the same PR or another one?
There was a problem hiding this comment.
I've done this (by pulling the changes from your PR in the vm-virtio branch used as a depenency right now) in #41.
| // will be ignored. | ||
| let num_sectors = file_size >> SECTOR_SHIFT; | ||
| // This has to be in little endian btw. | ||
| Ok(num_sectors.to_be_bytes().to_vec()) |
There was a problem hiding this comment.
definitely! my bad
| } | ||
|
|
||
| // Arguments required when building a block device. | ||
| // TODO: Add read-only operation support as a quick next step. |
There was a problem hiding this comment.
In the future we might want to have some sort of array for the supported operations since there are a few more other than read-only.
There was a problem hiding this comment.
Yes, that makes sense; we should def keep it in mind when adding all that stuff.
| Unsupported(u32), | ||
| } | ||
|
|
||
| impl From<u32> for RequestType { |
There was a problem hiding this comment.
It could be a good moment to do this, indeed. Do we plan to do this in the same PR or another one?
| match Request::parse(&mut chain) { | ||
| Ok(request) => { | ||
| let len; | ||
| // TODO: The executor could actually write the status itself, right? |
There was a problem hiding this comment.
It should, still trying to figure out how to do this in a nice way :D.
There was a problem hiding this comment.
That's cool; we can def live with this as is for the time being.
There was a problem hiding this comment.
Thanks for the comments! I've added answers here, and addressed them as part of #41 (which is meant to supersede this RFC PR). We can close this after that gets resolved/merged.
|
|
||
| let irqfd = EventFd::new(EFD_NONBLOCK).map_err(Error::EventFd)?; | ||
| args.vm_fd | ||
| .register_irqfd(&irqfd, args.mmio_cfg.gsi) |
There was a problem hiding this comment.
I would really like to try out this approach because it helps delegate all the setup logic to the device, as long as the necessary "resources" (such as the VmFd for this particular implementation) are provided by the environment.
| // will be ignored. | ||
| let num_sectors = file_size >> SECTOR_SHIFT; | ||
| // This has to be in little endian btw. | ||
| Ok(num_sectors.to_be_bytes().to_vec()) |
There was a problem hiding this comment.
definitely! my bad
| } | ||
|
|
||
| // Arguments required when building a block device. | ||
| // TODO: Add read-only operation support as a quick next step. |
There was a problem hiding this comment.
Yes, that makes sense; we should def keep it in mind when adding all that stuff.
| Unsupported(u32), | ||
| } | ||
|
|
||
| impl From<u32> for RequestType { |
There was a problem hiding this comment.
I've done this (by pulling the changes from your PR in the vm-virtio branch used as a depenency right now) in #41.
| match Request::parse(&mut chain) { | ||
| Ok(request) => { | ||
| let len; | ||
| // TODO: The executor could actually write the status itself, right? |
There was a problem hiding this comment.
That's cool; we can def live with this as is for the time being.
|
Closing as the discussion moved over to #41. |
This PR proposes a block device implementation based on abstraction from upstream (merged or under review) and some local logic as well (parts of which can become reusable at some point themselves). The block device is not properly tied to the command line yet; for now it's just hardcoded in the Vmm, which tries to open a file named "disk.ext4" from the current working directory when being run. There are some comments in the last commit regarding the order in which we create the irqchip relative to the devices; would be great to see what kind of restriction we're dealing with there (if any).
The request parsing and execution part is taken from Firecracker, until we can reuse something from rust-vmm. The next steps here are to see what kind of user interface changes we want to add to the application for block devices, and to start creating more tests if things look reasonable.