Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

acpi_tables: Update to latest RQSC specs #36

Merged
merged 1 commit into from
Jun 26, 2024
Merged

Conversation

lucido-simon
Copy link
Contributor

@lucido-simon lucido-simon commented Jun 21, 2024

The table definition for RQSC tables changed. This commits updates the RQSC implementation to match the new specs.

The new specs can be confusing to use, since some fields depends on others. Sometimes they should be set, sometimes not. To prevent mishaps, enums are used to force the developer to be coherent in what he defines.

Also, I'm not sure about how to do vendor specific resource, I think it could be improved. If you have better idea, lmk.

@lucido-simon lucido-simon marked this pull request as ready for review June 21, 2024 09:00
src/rqsc.rs Outdated
/// - 0: Cache
/// - 1: Memory
/// - 2-0x7F: Reserved
/// - 0x80-0xFF: Vendor Specific
resource_type: u8,
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about adding a ResourceType enum (that is #[repr(u8)]) with these values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reserved and vendor specific can take a range of value. I'm not sure how to code this range in an #[repr(u8)] enum

Copy link
Collaborator

Choose a reason for hiding this comment

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

For now all we probably care about is Cache and Memory. If others have later use cases, they can add more values to the enum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I've put back the ResourceType enum

src/rqsc.rs Outdated
/// The byte must be the Resource ID Type.
/// The Vec must be Resource ID 1, Resource ID 2 and Resource Specific Data
/// serialized as aml_bytes.
VendorSpecific((u8, Vec<u8>)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: this does not have to be a tuple, you can just use VendorSpecific(u8, Vec<u8>)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't know that, thanks!

src/rqsc.rs Outdated
Comment on lines 260 to 265
ResourceID::Cache(_) => core::mem::size_of::<CacheResource>(),
ResourceID::MemoryAffinityStructure(_) => {
core::mem::size_of::<MemoryAffinityStructureResource>()
}
ResourceID::ACPIDevice(_) => core::mem::size_of::<ACPIDeviceResource>(),
ResourceID::PCIDevice(_) => core::mem::size_of::<PCIDeviceResource>(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I see lots of core::mem::size_of, might be worthwhile to import that and just call size_of everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

assert_eq!(
bytes.len(),
TableHeader::len() + 4 + core::mem::size_of::<QoSController>()
assert_eq!(bytes.len(), TableHeader::len() + 4 + 28);
Copy link
Collaborator

Choose a reason for hiding this comment

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

28 is the expected size of the QoSController record?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there are no resource structure associated, yes

@sboeuf
Copy link
Collaborator

sboeuf commented Jun 21, 2024

LGTM

The table definition for RQSC tables changed[0]. This commits updates
the RQSC implementation to match the new specs.

The new specs can be confusing to use, since some fields depends on
others. Sometime they should be set, sometimes not. To prevent mishaps,
enums are used to force the developer to be coherent in what he defines.

[0] https://github.com/riscv-non-isa/riscv-rqsc

Signed-off-by: Simon Lucido <simonl@rivosinc.com>
@sboeuf
Copy link
Collaborator

sboeuf commented Jun 26, 2024

@lucido-simon you've introduced a merge commit. Please use git rebase to rebase your commit on top of latest main branch.

@sboeuf
Copy link
Collaborator

sboeuf commented Jun 26, 2024

@lucido-simon you've introduced a merge commit. Please use git rebase to rebase your commit on top of latest main branch.

Nevermind, you've already fixed it :)

@sboeuf sboeuf merged commit 969978b into rust-vmm:main Jun 26, 2024
14 checks passed
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.

3 participants