rust-osdev / acpi

Rust library for parsing ACPI tables and interpreting AML
Apache License 2.0
203 stars 65 forks source link

acpi: Add missing and new fields to GIC CPU interface structure #156

Closed rcerc closed 1 year ago

rcerc commented 1 year ago

This adds the previously missing 'GICV' field to GiccEntry (under the name gic_virtual_registers_address, renaming gic_control_block_address to gic_hypervisor_registers_address to match) and appends the two more recent 'SPE overflow Interrupt' and 'TRBE Interrupt' fields.

A question on appending fields that are introduced in later versions of the specification: Is it reasonable to be concerned about Rust implicitly accessing the new fields on a platform that does not provide them? I don't have an idea of the internals of the compiler, but it seems like the MADT entries might very well end at a page boundary and one risks accessing unmapped memory in the case that any follows. Please let me know if my question is unclear. If this situation is possible, what is the solution? Should the extra fields only be made available through accessor methods that verify the length of the entry, or could a new struct be made for every addition that includes the struct for the previous version of the entry?

rcerc commented 1 year ago

Ah, thanks for the explanation and mention of ExtendedField. I attached a question about it below.

In my previous comment, I was more worried about the (arguably unlikely) case that a supported field and an unsupported field are neighbouring and (if there is such a compiler optimisation; I have no idea) they are both accessed by one instruction for simplicity (even if the value read from the second field is not used because the field is not supported), possibly causing a page fault if the two fields are on different pages and the second page is unmapped. This is an imaginary scenario though; maybe it is not a problem because of how the compiler works.

How should I specify a minor version for ExtendedField? The 'SPE overflow Interrupt' field was added in 6.3 (following 6.2 (Errata B)) and, as you mentioned, the 'TRBE Interrupt' was added in 6.5 (following 6.4 (Errata A)). On a related note, would it not be possible to rely on the 'Length' field (edit: and checking that a field is not zero) to detect supported fields and have the structs include getters that succeed accordingly? This way, the user would not have to check the FADT for the major and minor ACPI versions (as I understand must be done)? Thank you for helping me out with these changes.

IsaacWoods commented 1 year ago

supported field and an unsupported field are neighbouring and (if there is such a compiler optimisation; I have no idea) they are both accessed by one instruction for simplicity

This should definitely not happen with ExtendedField, as the MaybeUninit should prevent optimizations that assume the memory is initialized from happening. I'm not entirely sure if it would be allowable without the wrapper - my guess would be yes, so another reason to avoid that.

How should I specify a minor version for ExtendedField?

ExtendedField does not operate off the ACPI version, but the revision of the table itself (the revision is a field of the SDT header). If you look in the description of the field on the first version of the spec that supports the field, they should (the universe being merciful, hopefully we don't have to cover them forgetting to do this) have incremented this revision each time they make a change to the table. That is the number that should be used in ExtendedField.

On a related note, would it not be possible to rely on the 'Length' field

I hadn't thought of this, but yes I don't see why this wouldn't work. It feels a little less proper than going off the table revision though, so I don't think we should go down this avenue unless required.

Thank you for helping me out with these changes.

No worries at all! It's great to have people interested in making contributions.

rcerc commented 1 year ago

Sure. I squashed the commits for brevity.

IsaacWoods commented 1 year ago

Note to self: if we don't manage to get the new alloc changes finalised + released relatively soon, we could backport this into the old major version.