Skip to content

uefi: Add safe protocol wrapper for EFI_ATA_PASS_THRU_PROTOCOL #1595

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

Merged
merged 3 commits into from
Apr 14, 2025

Conversation

seijikun
Copy link
Contributor

@seijikun seijikun commented Mar 27, 2025

Update Latest API discussions


Implemented a safe API wrapper for EFI_ATA_PASS_THRU_PROTOCOL [1d3de7f0-0807-424f-aa69-11a54e19a46f].
Added an integration-test for it.

This contains a copy of the AlignedBuffer commit from #1589.

This uses the iterator/mutable design I suggested here: #1589 (comment) where the iter method only captures the protocol (AtaPassThru) immutably, but requires you to use the produced element (AtaDevice) in a mutable way for meaningful methods like execute_command.

This closes #244

Checklist

  • Sensible git history (for example, squash "typo" or "fix" commits). See the Rewriting History guide for help.
  • Update the changelog (if necessary)

@seijikun seijikun force-pushed the mr-atapt branch 14 times, most recently from 5d20e04 to 17ff660 Compare March 28, 2025 01:19
Copy link
Member

@phip1611 phip1611 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! Left a few remarks

@seijikun seijikun force-pushed the mr-atapt branch 2 times, most recently from 2ccfb33 to 6a82a32 Compare March 28, 2025 10:43
@nicholasbishop nicholasbishop mentioned this pull request Apr 1, 2025
2 tasks
@phip1611 phip1611 mentioned this pull request Apr 6, 2025
13 tasks
@seijikun seijikun force-pushed the mr-atapt branch 2 times, most recently from 4e64767 to 9a64ab4 Compare April 8, 2025 09:55
@phip1611 phip1611 changed the title Add safe protocol wrapper for EFI_ATA_PASS_THRU_PROTOCOL uefi: Add safe protocol wrapper for EFI_ATA_PASS_THRU_PROTOCOL Apr 8, 2025
Copy link
Member

@phip1611 phip1611 left a comment

Choose a reason for hiding this comment

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

We're almost there!

@seijikun seijikun force-pushed the mr-atapt branch 6 times, most recently from dd50f86 to 2844372 Compare April 10, 2025 08:51
@seijikun seijikun marked this pull request as draft April 10, 2025 16:40
@seijikun seijikun force-pushed the mr-atapt branch 4 times, most recently from cffd9af to 24cdd18 Compare April 12, 2025 19:13
@seijikun
Copy link
Contributor Author

Like with NVMe, I just rebased this MR and split it into more / smaller commits.

One note on the integration test:
I currently don't run the ATA integration tests on the aarch64 and arm architectures. This is because the EDK2 / OVMF UEFI firmware for these targets does not seem to actually support SATA controllers (or at least not the ATA pass thru protocol).
However: Even though I don't run the ATA test on these two platforms currently, I properly configured the qemu hardware with a manually installed SATA controller and an attached test-disk anyhow. So enabling the test later on is just removing the #[cfg] switches

@seijikun seijikun marked this pull request as ready for review April 12, 2025 19:20
@seijikun seijikun requested a review from phip1611 April 12, 2025 19:20
@nicholasbishop
Copy link
Member

Added a couple minor comments, otherwise lgtm

@seijikun seijikun force-pushed the mr-atapt branch 2 times, most recently from 2bf6b69 to 827bbc6 Compare April 14, 2025 19:28
@seijikun
Copy link
Contributor Author

@nicholasbishop I hope I have addressed all of your comments.

The Rust / Test Coverage failure doesn't look like something I can fix 🤔

@phip1611
Copy link
Member

phip1611 commented Apr 14, 2025

I fixed the CI in #1631

@nicholasbishop nicholasbishop added this pull request to the merge queue Apr 14, 2025
Merged via the queue into rust-osdev:main with commit 27487c2 Apr 14, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implementing pass-through protocols
3 participants