Allow BufferField -> Integer conversion#286
Allow BufferField -> Integer conversion#286martin-hughes wants to merge 6 commits intorust-osdev:mainfrom
Conversation
|
I tested this on my real hardware (an MSI X570 motherboard) and this fix enables it to parse all the tables, and my tester was able to enumerate all devices 😊 |
IsaacWoods
left a comment
IsaacWoods
left a comment
There was a problem hiding this comment.
Thanks for the PR!
I like the move to using to_integer for both bits of logic - I'm not sure why I didn't do that in the first place.
Apologies for the merge conflicts with merging #276 but actually there is some overlap of approach as mentioned below.
| // TODO: how should we handle invalid inputs? What does NT do here? | ||
| Object::String(value) => Ok(value.parse::<u64>().unwrap_or(0)), | ||
| _ => Ok(0), | ||
| Object::BufferField { .. } => { |
There was a problem hiding this comment.
I think we've both realised this case, but approached it in different ways. In #276, I added a case to TermArg resolution that reads all buffer fields as either Integers or Buffers depending on their size. In operands of ToInteger or anywhere that will be doing an implicit conversion, we should therefore never have encounter a BufferField.
Could you test that change (and removing this case here) to see if that fixes the problem instead of repeating this logic here?
There was a problem hiding this comment.
Yup, for sure.
There was a problem hiding this comment.
So from a first pass it doesn't work - but I'm trying to understand the argument resolution behaviour. To me, it looks like it only happens if you have one of the "name" opcodes Opcode::DualNamePrefix | Opcode::MultiNamePrefix | Opcode::Digit(_) | Opcode::NameChar(_) | Opcode::RootChar | Opcode::ParentPrefixChar, but not if an argument is pushed using contribute_arg.
a) is that true? and
b) is that correct in all cases? Of the top of my head, what would happen for:
Local0 = Local1 + DeRefOf(/* some buffer field */)
Would DeRefOf contribute_arg() a buffer field reference to the Addition op-in-flight?
There was a problem hiding this comment.
Hm yes apologies, you're completely right! Something like a Local0 = RefOf(/* buffer field*/) would of course be resolved through a SuperName etc. and we would need this logic too. Ignore me!
|
|
||
|
|
||
| #[test] | ||
| fn buffer_field_to_integer() { |
There was a problem hiding this comment.
With the above, these tests will not work (but I think that's fine).
There was a problem hiding this comment.
Tests left in since they do still work
| /// To avoid the cast, use [`Object::as_integer`] instead. | ||
| pub fn to_integer(&self, allowed_bytes: usize) -> Result<u64, AmlError> { | ||
| // This check shouldn't hit, but it protects the `to_interpret` buffer below from panicking. | ||
| if allowed_bytes > size_of::<u64>() { |
There was a problem hiding this comment.
I have previously been lazy and assumed allowed_bytes must only ever be 4 or 8.
#276 introduced an Interpreter::integer_size helper - I know you're keener on strong types that I've utilised here - what we could do is introduce an IntegerSize enum returned by that helper that could be constrained to FourBytes or EightBytes (names bike-sheddable) and use that in places like this.
This would then remove the new error case, as it should never be a user-surfacable error (it is an internal logic error in the interpreter if this value is not 4 or 8).
There was a problem hiding this comment.
Yup, love the types! 😆 I'll do that and see what it looks like.
|
Sounds good Isaac - give me a few days, the warmer weather means more garden work needs doing! |
Both explicit and some implicit conversions are supported.
martin-hughes
force-pushed
the
i272-deref-of-reference
branch
from
a6de117 to
d0ecd30
Compare
|
I've pushed a very naive rebase - once I've understood the argument parsing more (see my comment) then I'll make the other updates and squash to remove the naff commits. |
This removes some repetitive calculations and unreachable branches.
|
|
||
| #[derive(Debug, Clone)] | ||
| pub struct DsdtInfo { | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
It turns out that the only use for the DSDT revision on the interpreter is to figure out the integer length... So if you prefer I could strip out this structure entirely - just let me know.
I don't know though - will you need access to the DSDT revision number in future?
|
That's my updates completed. If you're happy just let me know and - assuming you want me to - I'll squash it into one commit. |
2 participants
Both explicit and some implicit conversions are supported.
Originally this was all about fixing #272 - and I think the changes to
mod.rsdo that. But in order to test it, I had to enable some conversion of BufferField to any other type, so I implementedToIntegerfor it.do_to_integerwasn't usingObject::to_integer- and they were both slightly different. But I think the same conversion rules apply for both uses, so I've merged them.