Repository navigation
Rework GAS accesses to fixed register blocks - #373
Conversation
martin-hughes
left a comment
martin-hughes
left a comment
There was a problem hiding this comment.
Nice one @IsaacWoods, I think this looks very sensible. I can't see why we shouldn't split the register into two MappedGass and I think it improves the ergonomics a lot.
One minor question in the review comments, but no reason not to merge if you're still happy with this.
| let block = GenericAddress::from_raw(raw)?; | ||
| let sts = GenericAddress { | ||
| address_space: AddressSpace::SystemIo, | ||
| bit_width: block.bit_width / 2, |
There was a problem hiding this comment.
Is there a minimum width specified in the spec? (I can't see one...) What happens if the width ends up less than the size of PM1a_STS? (Also the same question for the other registers)
There was a problem hiding this comment.
I think yes - it's mentioned in §4.8.1.1 of the 6.6 spec that "The PM1 event register block has a minimum size of 4 bytes", so each register will have a minimum size of the 16 currently-defined bits.
We could add some extra checks around this (?assume block is 4 bytes if size seems incorrect etc.) to deal with especially broken FADTs but could wait for a broken firmware to demonstrate that's an issue?
There was a problem hiding this comment.
it's mentioned in §4.8.1.1 of the 6.6 spec that "The PM1 event register block has a minimum size of 4 bytes"
Good spot! Think I saw that and read it as "minimum size of 4 bits" 🤦
I think waiting for a broken firmware is fair - at some point you have to trust the firmware isn't completely broken!
| address: block.address + (block.bit_width / 2 / 8) as u64, | ||
| }; | ||
| Ok((sts, en)) | ||
| } else { |
There was a problem hiding this comment.
Nice bit of extra compatibility, like it 😄
|
Actually, I did have a thought. Is it possible for the GAS Access Size to specify that both registers (both parts of pm1a for example) need to be written in one access? |
Good thought - I think my reading infers that these are two registers within a register block and so would ordinarily be separate. I think they just put them in a single GAS to save a field in the FADT. I want to get my kernel booting on some real hardware in the near future to give this a test but until then assume this will be fine. Edit: I've checked and uACPI also splits the accesses to these registers. This feels like the most resilient approach to me so will get this merged! Thanks for your review Martin! |
This removes the
read_u16andwrite_u16methods onGenericAddress, and fixes accesses to thePM1register blocks by splitting them into their componentSTSandENregisters.This does throw in a few unrelated changes that I have had locally on my
mainbranch from poorgitting I'm afraid - I have not rebased these off as I don't think they reduce reviewability and I'd like them to go in at some point.cc @martin-hughes - we've discussed complexities around handling these accesses. I'd be interested in your thoughts on this solution.