Skip to content

Conversation

@jackschefer-msft
Copy link
Contributor

@jackschefer-msft jackschefer-msft commented Sep 10, 2025

This change adds currently unused MCFG definitions and parsing to the acpi_spec crate. The MCFG table describes the memory-mapped configuration space base address for each PCIe root complex device on a system.

Future changes will actually utilize these MCFG definitions to expose enhanced configuration access mechanism (ECAM) addresses to guests. See #1976 for a preview.

Copilot AI review requested due to automatic review settings September 10, 2025 18:02
@jackschefer-msft jackschefer-msft requested a review from a team as a code owner September 10, 2025 18:02
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds MCFG (Memory Mapped Configuration) table definitions and parsing to the acpi_spec crate. The MCFG table describes memory-mapped configuration space base addresses for PCIe root complex devices, enabling Enhanced Configuration Access Mechanism (ECAM) support.

Key changes:

  • Adds complete MCFG table structure definitions with proper zerocopy traits
  • Implements parsing functionality with comprehensive error handling
  • Provides both borrowed and owned parsing variants for different use cases

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
vm/acpi_spec/src/mcfg.rs New module implementing MCFG table structures, parsing logic, and error handling
vm/acpi_spec/src/lib.rs Exports the new mcfg module

@jackschefer-msft jackschefer-msft requested a review from a team September 10, 2025 18:03
Comment on lines +39 to +45
pub struct McfgSegmentBusRange {
pub ecam_base: u64_ne,
pub segment: u16_ne,
pub start_bus: u8,
pub end_bus: u8,
pub rsvd: u32_ne,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be a bitfield(u128), and then you're guaranteed that the size will be correct.

In general, I prefer bitfields for structures that are up to 128 bits in length that need to match some hardware spec. That is perhaps stylistic without any real basis. Interested what you / others think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally, I think using a bitfield to represent a structure where all members are on byte boundaries is confusing, so I would lean towards keeping it as-is with the const assert. However, I am still in the process of refining my personal stylistic preferences in Rust

Copy link
Member

Choose a reason for hiding this comment

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

my rule of thumb is generally to use bitfields if there are actual bit fields, but if all members are standard integers then it's not usually something I use. I don't think we codify that anywhere, but regardless as long as you assert the size correctly I don't think either approach is wrong.

@jackschefer-msft jackschefer-msft merged commit 730bbd9 into microsoft:main Sep 19, 2025
29 checks passed
@jackschefer-msft jackschefer-msft deleted the acpi-pcie-mcfg branch September 19, 2025 16:47
andyplank-msft pushed a commit to andyplank-msft/openvmm that referenced this pull request Nov 4, 2025
This change adds currently unused MCFG definitions and parsing to the
`acpi_spec` crate. The MCFG table describes the memory-mapped
configuration space base address for each PCIe root complex device on a
system.

Future changes will actually utilize these MCFG definitions to expose
enhanced configuration access mechanism (ECAM) addresses to guests. See
microsoft#1976 for a preview.
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.

4 participants