-
-
Notifications
You must be signed in to change notification settings - Fork 181
Add DiskIo and DiskIo2 protocols
#467
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
Conversation
nicholasbishop
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
I think the DiskIo part looks good, but we should consider the safety of the DiskIo2 interface. Since the operations are asynchronous if an Event is passed in via DiskIo2Token, the buffer and token parameters needs to remain valid for the whole lifetime of the request, but this isn't currently enforced. I'm not quite sure what a good safe interface would look like yet. As an intermediate step, we could change the current functions to be unsafe, maybe also rename them to something like read_disk_raw/write_disk_raw, and change the buffer and token parameters to be raw pointers. Alternatively we could try to jump right into figuring out what a safe interface looks like, I'm interested to hear ideas.
Also, it would be good to add some tests for the new protocols. I think these could go in uefi-test-runner/src/proto/media/known_disk.rs.
|
|
nicholasbishop
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates and added tests! I added some comments on the tests.
src/proto/media/disk.rs
Outdated
| #[repr(C)] | ||
| pub struct DiskIo2Token { | ||
| /// Event to be signalled when an asynchronous disk I/O operation completes. | ||
| pub event: Option<Event>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In trying to track down why the Event never gets signaled, I realized that this struct does not currently have the correct layout. We aren't using NonNull in Event like we do with Handle, so Option<Event> doesn't get the same layout as a raw pointer.
I filed #477 to fix this, but for now this line should be changed to pub event: Event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in this case this actually isn't necessary because I interpreted the specification wrong. The spec talks about the token "field" to be nullable, but it seems what it means is that the token argument to ReadDiskEx/WriteDiskEx can be NULL - meaning I could simply change the parameter type.
6ebe7f4 to
5f67272
Compare
|
Looks good, thanks! |
Adds the
EFI_DISK_IO_PROTOCOLandEFI_DISK_IO2_PROTOCOLprotocols from the UEFI specification that build on top of the block I/O protocol to provide byte-oriented disk I/O.