Skip to content

Conversation

raccog
Copy link
Contributor

@raccog raccog commented Dec 21, 2022

Also revised some existing documentation to match the current style of how most errors are documented.

Checklist

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

///
/// Although [`INVALID_PARAMETER`] is not defined as an error in the UEFI Specification,
/// it can be returned if the opened filename exceeds the maximum length of 255 UCS-2
/// characters (not including the null terminator).
Copy link
Member

Choose a reason for hiding this comment

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

Nit: let's drop the ## Note subsection, I don't think separating out that info is needed. Also, the phrasing "not defined as an error in the UEFI Specification" is a little misleading; INVALID_PARAMETER is defined as an error, it's just not mentioned as a possible error returned by EFI_FILE_PROTOCOL.Open. From a quick look at the edk2 source code it can be returned in more cases than just the path being too long. That's one of the difficulties of trying to document errors for UEFI functions; the errors listed in the spec do not necessarily encompass all errors used by actual implementations. And even the full list of STATUS values is not necessarily complete, as the implementation is allowed to return vendor-specific errors.

Maybe something like this:

/// See section `EFI_FILE_PROTOCOL.Open()` in the UEFI Specification for more
/// details. Note that [`INVALID_PARAMETER`] is not listed in the specification
/// as one of the errors returned by this function, but some implementations
/// (such as EDK2) perform additional validation and may return that status for
/// invalid inputs.
///
/// * [`uefi::Status::INVALID_PARAMETER`]
/// * [`uefi::Status::NOT_FOUND`]
/// ...rest of the errors...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that looks much better to me, too. I'll fix it real quick.

That's one of the difficulties of trying to document errors for UEFI functions; the errors listed in the spec do not necessarily encompass all errors used by actual implementations. And even the full list of STATUS values is not necessarily complete, as the implementation is allowed to return vendor-specific errors.

That's interesting, do you mean that vendors can even define their own custom error codes (that aren't in the spec) in some cases?

Also, I'm curious, have you found cases where vendors don't properly implement every error listed in the spec? Using EFI_FILE_PROTOCOL.Open() as an example, is it possible that some vendor implementations will never return EFI_NOT_FOUND?

I don't have much experience with firmware in general, so I really appreciate this feedback. :)

Copy link
Member

Choose a reason for hiding this comment

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

That's interesting, do you mean that vendors can even define their own custom error codes (that aren't in the spec) in some cases?

Yes, the spec reserves ranges of EFI_STATUS for OEM warnings and errors:
https://uefi.org/specs/UEFI/2.10/Apx_D_Status_Codes.html#efi-status-success-codes-high-bit-clear-apx-d-status-codes

I am not sure how often OEMs make use of these ranges in practice. As far as I understand it, the best practice is to always check the EFI_STATUS and handle any possible value. In the majority of cases, all you have to handle is EFI_SUCCESS and treat all error cases the same. Occasionally some special handling is needed for particular error codes, but even then you should usually have a fallback case to handle any unexpected error or warning.

Also, I'm curious, have you found cases where vendors don't properly implement every error listed in the spec? Using EFI_FILE_PROTOCOL.Open() as an example, is it possible that some vendor implementations will never return EFI_NOT_FOUND?

I haven't personally seen any real-world examples of this, no. Hopefully implementations are well tested, but the reality (as with any software) is that there will be some bugs and unexpected quirks. And especially with older devices, the firmware is often not updated beyond whatever was flashed on at the factory, so even if the bug is patched, that patch doesn't make it out to all the physical devices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this, I really appreciate the time you put into this answer.

Yes, the spec reserves ranges of EFI_STATUS for OEM warnings and errors:

Seems I gotta dig deeper into the manual, because I honestly didn't know this. Feels like there's always new weird implementation details to learn with UEFI, I love it. :) Also, very good to know for documenting the returned errors in this crate.

In the majority of cases, all you have to handle is EFI_SUCCESS and treat all error cases the same.

I agree, that does seem like the best way to handle such a large range of unknown error codes. Gonna be using this strategy in my own projects, as I can think of a few cases where I didn't leave room for the possibility of unknown error codes.

Hopefully implementations are well tested, but the reality (as with any software) is that there will be some bugs and unexpected quirks.

Thanks for linking the edk2 tests, I've never seen those either. Surprisingly easy to get running! I only have a few old PCs running UEFI firmware that I can test this on, but I'm very interested to see the results compared to OVMF.

Also revised some existing documentation to match the current style of
how most errors are documented.
@nicholasbishop nicholasbishop merged commit 7b099d5 into rust-osdev:main Dec 28, 2022
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.

2 participants