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: Fix VIOT endpoint_id remapping #34

Merged
merged 1 commit into from
Mar 26, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions src/viot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,14 +148,6 @@ impl PciDevice {
fn as_bdf(&self) -> u16 {
(self.bus as u16) << 8 | (self.device as u16) << 3 | self.function as u16
}

fn as_segment(&self) -> u16 {
self.segment
}

fn as_endpoint(&self) -> u32 {
(self.as_segment() as u32) << 16 | self.as_bdf() as u32
}
}

/// This structure describes a range of PCI endpoints
Expand Down Expand Up @@ -188,7 +180,7 @@ impl Aml for PciRange {
sink.byte(ViotEntryType::PciRange as u8);
sink.byte(0); // reserved
sink.word(Self::len() as u16);
sink.dword(self.first.as_endpoint());
sink.dword(self.first.as_bdf() as u32);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on the ACPI spec, and particularly the section about the PCI range node structure here, the Endpoint Start is the first Endpoint ID, and an Endpoint ID is described as follows:

The corresponding endpoint ID is obtained by combining segment and BDF:
Endpoint ID = ((segment - Segment Start) << 16) + BDF - BDF Start + Endpoint Start

I'm a bit confused because what does Endpoint Start stand for in this formula? And if we substract both the Segment Start and BDF Start, does that mean we end up with the endpoint ID being a simple index corresponding to Endpoint Start?

My point is, I think you're patching this in the right direction but we're still missing the BDF substraction. Should we simply always hardcode the Endpoint Start as 0?

/cc @timw-rivos @rbradford

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think 99% of the time Endpoint Start will likely be 0, but the "correct" thing to do is to probably use the corresponding formula from the spec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For most simple mappings Endpoint Start this will be 0, however for more complex mappings in the IOMMU range assuming the most common case (unity mapping in the segment) Endpoint Start should represent First BDF in the range. This change will provide simple unity mapping for non-contiguous PCI ranges, without adding segment number.
Eg. Mapping example for single IOMMU handle:

0001:00:01.0 - 0000:01.7  --> 0x008
0001:00:06.0 - 0000:06.7  --> 0x030

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for clarifying @tjeznach
It's a bit of a shame the ACPI spec isn't clear on that aspect, but I believe you've looked into the Linux implementation of the VIOT table, which is the "actual" spec :)

sink.word(self.first.segment);
sink.word(self.last.segment);
sink.word(self.first.as_bdf());
Expand Down
Loading