virtio-blk storage via mmio using virtio-drivers [v2] #542
Conversation
osteffenrh
force-pushed
the
virtio-pr-2
branch
from
6554c77 to
4ab8ac5
Compare
osteffenrh
force-pushed
the
virtio-pr-2
branch
5 times, most recently
from
8f52f24 to
3aaa640
Compare
Use the latest version of the zerocopy crate, to match the one used by the virtio-drivers crate. Signed-off-by: Oliver Steffen <[email protected]>
Add mmio read and write functions. ToDo: Improve read_buffer() to be similar to write_buffer(). Signed-off-by: Oliver Steffen <[email protected]>
osteffenrh
force-pushed
the
virtio-pr-2
branch
2 times, most recently
from
0383221 to
1f1362b
Compare
Add the virtio-drivers crate; implement a Hal for Coconut that uses the ghcb mmio functions, and manages bounce-buffers in host-shared memory. The buffers use SharedBox and are currently limited to 1 page in size. The SharedBoxes need to be kept around between the share() and unshare() calls, thus a global manager was added that holds them. ToDo: Allow arbitrary buffer sizes. Signed-off-by: Oliver Steffen <[email protected]>
Add a simple block layer api trait. Co-developed-by: Oliver Steffen <[email protected]> Signed-off-by: Oliver Steffen <[email protected]> Signed-off-by: Stefano Garzarella <[email protected]>
Add command line option to add a virtio-blk MMIO device to the CVM. Signed-off-by: Stefano Garzarella <[email protected]>
Add a block device driver that uses the virito-blk via mmio transport. ToDo: Return proper error codes. Co-developed-by: Oliver Steffen <[email protected]> Signed-off-by: Oliver Steffen <[email protected]> Signed-off-by: Stefano Garzarella <[email protected]>
Add simple write+readback tests for the VirtioBlkDriver. Signed-off-by: Oliver Steffen <[email protected]>
Add a virtio-blk device to the in-svsm test setup to allow the VirtioBlkDriver tests to run. Signed-off-by: Oliver Steffen <[email protected]>
osteffenrh
force-pushed
the
virtio-pr-2
branch
from
1f1362b to
5eb30db
Compare
There was a problem hiding this comment.
I think this is a great start for getting block-device support into COCONUT-SVSM. The PR uncovers some problems in the code base and I think we should seriously consider pulling the virtio-drivers directly into our code-base and change the interfaces to our needs.
| fn read_blocks(&self, block_id: usize, buf: &mut [u8]) -> Result<(), BlockDeviceError>; | ||
| fn write_blocks(&self, block_id: usize, buf: &[u8]) -> Result<(), BlockDeviceError>; |
There was a problem hiding this comment.
This interface makes the safe-unsafe memory boundary a bit blurry and looks unsafe in itself (due to the slice parameters). I have seen this interface somewhat resembles the interface of the virtio-blk driver crate. Maybe it is the best strategy to import the drivers into our code-base and improve their interfaces.
Some things I'd like to see:
- Asynchronous interface with a separate request tracking data structure and polling methods.
- Better abstractions for the source/target memory regions of block access.
There was a problem hiding this comment.
About async:
The virtio-drivers crate has non-blocking read/write functions, see https://github.com/osteffenrh/virtio-drivers/blob/63c5450f08bd557f609e0ffad7a5bd6281d6562e/src/device/blk.rs#L248
But I did not try that yet.
About the second point:
The buffer supplied to these functions is copied over to a new one, allocated via SharedBox when writing (and the reverse for reading). We are not sharing existing allocations with the host.
| { | ||
| use svsm::block::virtio_blk; | ||
| static MMIO_BASE: u64 = 0xfef03000; | ||
| let _blk = virtio_blk::VirtIOBlkDriver::new(PhysAddr::from(MMIO_BASE)); | ||
| } |
There was a problem hiding this comment.
We should start thinking about a proper detection mechanism for SVSM-assigned devices.
There was a problem hiding this comment.
@joergroedel yeah, should be some runtime discover mechanism (e.g. fw-cfg) or a static configuration through IGVM? Or both?
There was a problem hiding this comment.
The qemu patch adds four virtio-mmio transport slots at 0xfef00000 ... 0xfef03fff. So the very minimum would be to loop over these four slots, check whenever a virtio device is present, if so what kind of virtio device it is. Only after that attach a driver.
Separate question is whenever we'll just hardcode those four slots in both qemu and svsm, or whenever it makes sense to communicate the range from qemu to svsm via fw_cfg. I honestly don't see much benefit in making this configurable. There are not that many options to place the virtio-mmio transport slots anyway ...
There was a problem hiding this comment.
How about build time option that puts it into the igvm file as a start.
And use the range @kraxel picked as the default on both sides (Coconut and Qemu).
| struct PageStore { | ||
| pages: Vec<(PhysAddr, SharedBox<[u8; PAGE_SIZE]>)>, | ||
| } |
There was a problem hiding this comment.
Nothing wrong with this code, but I think this uncovers a fundamental problem with SharedBox which we need to solve separately. Reading any any memory in a SharedBox is UB, so it should have a read()/write() interface instead of direct data access.
joergroedel
added
in-review
This is version 2 of #343.
Add support for virtio-blk storage devices using the virtio-mmio transport.
MMIO accesses are done via explicit vmgexits. Interrupts are not used (-> polling).
The virtio-drivers crate from the rcore-os project is used. It required some modifications: osteffenrh/virtio-drivers#2
Crate License: MIT.
This PR requires a patched version of Qemu (coconut-svsm/qemu#13), adding virtio-mmio slots to the Q35 machine model.
This build on #531
How to test:
make test-in-svsmToDo:
Fixes since V1:
Things for future PRs:
CC: @stefano-garzarella ;-)