Skip to content

Conversation

timrobertsdev
Copy link
Contributor

@timrobertsdev timrobertsdev commented Sep 15, 2021

I don't believe this is quite ready to merge, there are a few things that I'm looking for guidance on:

  1. Should the functions in the impl block validate parameters before calling into firmware? Such as ensuring that processor_index <= maximum_processor_index?
  2. Would it make sense to turn exception.rs into a series of newtype_enum for each arch? The caller could still mix up the EBC and real ISA exception types, but it'd be a bit safer than what I have now.
  3. Should the callback registration functions deregister the current callback(if exists) and then register the new callback automatically, or should we return an error and let the caller retry themselves?
  4. Is it okay to use #[allow(unused_must_use)] on the individual test functions? All that really matters is that the calls return EFI_SUCCESS, we don't really do anything with the completion.
  5. Are the structs and constants specific to Itanium worth having? Rust/LLVM doesn't support Itanium and it doesn't seem likely in the future.

Thank you for taking the time to look at this :)

@nicholasbishop
Copy link
Member

Thanks for the patch! I haven't quite wrapped my head around the DebugSupport protocol, but I'll try to answer some of the questions.

1. Should the functions in the `impl` block validate parameters before calling into firmware? Such as ensuring that `processor_index <= maximum_processor_index`?

I guess since the spec says that it's up to the caller to ensure that params are correct, it's probably a good idea to do whatever level of validation on the parameters we can. Assuming it doesn't have an unreasonable cost of course, but I don't see why it would here.

2. Would it make sense to turn `exception.rs` into a series of `newtype_enum` for each arch? The caller could still mix up the EBC and real ISA exception types, but it'd be a bit safer than what I have now.

Since there are repeating values the newtype_enum macro won't quite do the right thing, but you could make your own newtype with an impl containing consts like this: https://github.com/rust-osdev/uefi-rs/blob/master/src/proto/device_path.rs#L164

3. Should the callback registration functions deregister the current callback(if exists) and then register the new callback automatically, or should we return an error and let the caller retry themselves?

I don't have a strong opinion either way, as long as the behavior is described in the docstring.

4. Is it okay to use `#[allow(unused_must_use)]` on the individual test functions? All that really matters is that the calls return `EFI_SUCCESS`, we don't really do anything with the completion.

An alternative: add use uefi::ResultExt;, and then change .expect() to .expect_success().

5. Are the structs and constants specific to Itanium worth having? Rust/LLVM doesn't support Itanium and it doesn't seem likely in the future.

I don't have a strong opinion either way. On the one hand, it's nice for completeness and isn't a big amount of code. On the other hand, as you say it's not likely to actually be used.

Finish implementing `DebugSupport::ExceptionTypes`
Adjust `DebugSupport` tests for currently supported architectures
@timrobertsdev timrobertsdev marked this pull request as ready for review September 24, 2021 21:29
assert_ne!(
maximum_processor_index,
usize::MAX,
"get_maximum_processor_index() returning garbage, unless you really have 18,446,744,073,709,551,615 processors"
Copy link
Member

Choose a reason for hiding this comment

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

😀

@nicholasbishop
Copy link
Member

nicholasbishop commented Sep 26, 2021

Left a couple more comments, but otherwise this seems good to merge to me.

@nicholasbishop nicholasbishop merged commit 87db820 into rust-osdev:master Sep 27, 2021
@timrobertsdev timrobertsdev deleted the implement_debug_proto branch October 18, 2021 05:01
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