MsvmPkg: Support multiple PCIe host bridges per segment#87
Conversation
mebersol
left a comment
mebersol
left a comment
There was a problem hiding this comment.
[Copilot] Both fixes are correct, mutually consistent, and the root-cause analysis matches the code. I traced the target multi-bridge scenario (apertures seg0 bus0–31 and seg0 bus64–255 against split MCFG entries) and confirmed the old code discarded all bridges while the new code matches each aperture to its correct entry. Also verified no single-entry regression, that the zero-init segment-0 slot can't false-match in the coalesce inner loop (it only scans k < SegIdx), and that the failure path (ASSERT + FreePool + NULL + *Count = 0) is clean in both DEBUG and RELEASE. Two non-blocking suggestions left inline (a diagnostic-message regression and a clarifying comment). Ship-ready.
| if (McfgEntries[j].PciSegmentGroupNumber == Segment) { | ||
| if (McfgEntries[j].PciSegmentGroupNumber == Segment && | ||
| StartBus >= McfgEntries[j].StartBusNumber && | ||
| EndBus <= McfgEntries[j].EndBusNumber) { |
There was a problem hiding this comment.
[Copilot] The match fix is correct — folding bus-range containment into the predicate avoids the old first-match-then-validate bug that discarded all bridges. One diagnostic regression to consider: the removed branch emitted a precise message on a segment-match/bus-mismatch (Aperture bus range %u..%u exceeds MCFG range %u..%u for segment %u). Now any bus-range problem (e.g. an aperture that straddles two MCFG entries or exceeds them all) falls through to the generic !McfgFound path, which reports Aperture segment %u has no matching MCFG entry — misleading for a range mismatch. Suggest adding the bus range to that downstream message, e.g. Aperture seg %u bus %u..%u has no containing MCFG entry, to preserve debuggability.
| // same-segment host bridges with distinct, discontinuous ECAM base | ||
| // addresses. We do not support that here: this platform's VMM always lays | ||
| // same-segment host bridges over a single contiguous ECAM, so every | ||
| // same-segment entry carries the same bus-0-relative BaseAddress. We assert |
There was a problem hiding this comment.
[Copilot] The coalesce-by-segment + bus-range-union logic is correct, and the raw BaseAddress conflict check is valid because this VMM's MCFG BaseAddress is bus-0-relative (consistent with PciHostBridgeLib, which computes EcamBase = BaseAddress + StartBusNumber * PCIE_ECAM_BYTES_PER_BUS). Minor: a reviewer familiar with the standard MCFG convention — where BaseAddress is the physical base of StartBusNumber rather than bus 0 — might expect the conflict check to normalize by StartBusNumber before comparing. Consider stating explicitly in this comment that this VMM always emits the bus-0-relative base, so a raw != comparison is equivalent to a bus-0-relative comparison. The ASSERT(FALSE) correctly guards the case where that assumption is ever violated.
There was a problem hiding this comment.
Uh, no, what you've described is never how MCFG works.
The PCI Firmware Spec (Rev 3.3, §4.1.2 Table 4-3) permits multiple MCFG entries for the same PCI Segment Group with disjoint bus ranges. The VMM emits this configuration when a guest has two or more PCIe host bridges on segment 0 (e.g. bridge A on bus 0-31, bridge B on bus 64-255). Two bugs prevented such guests from booting: PciHostBridgeLib: The MCFG lookup matched by segment number alone with a first-match break. When the second aperture (higher bus range) matched the first MCFG entry (lower bus range), it failed the bus-range check and goto Cleanup discarded ALL root bridges, causing 'no boot devices'. Fix: match by segment AND bus-range containment so each aperture finds its correct MCFG entry. PciSegmentInfoLib: Emitted one PCI_SEGMENT_INFO per MCFG entry, but the upstream consumer (PciSegmentLibCommon.c) expects exactly one entry per segment and matches by segment number alone. Two same-segment entries caused config-space accesses to the upper bus range to resolve against the wrong entry. Fix: coalesce same-segment MCFG entries into a single PCI_SEGMENT_INFO with the union of their bus ranges and their shared ECAM base address. Assert and fail if same-segment entries ever carry different base addresses (not supported by this platform).
2 participants
The PCI Firmware Spec (Rev 3.3, §4.1.2 Table 4-3) permits multiple MCFG entries for the same PCI Segment Group with disjoint bus ranges. The VMM emits this configuration when a guest has two or more PCIe host bridges on segment 0 (e.g. bridge A on bus 0-31, bridge B on bus 64-255). Two bugs prevented such guests from booting:
PciHostBridgeLib: The MCFG lookup matched by segment number alone with a first-match break. When the second aperture (higher bus range) matched the first MCFG entry (lower bus range), it failed the bus-range check and goto Cleanup discarded ALL root bridges, causing 'no boot devices'. Fix: match by segment AND bus-range containment so each aperture finds its correct MCFG entry.
PciSegmentInfoLib: Emitted one PCI_SEGMENT_INFO per MCFG entry, but the upstream consumer (PciSegmentLibCommon.c) expects exactly one entry per segment and matches by segment number alone. Two same-segment entries caused config-space accesses to the upper bus range to resolve against the wrong entry. Fix: coalesce same-segment MCFG entries into a single PCI_SEGMENT_INFO with the union of their bus ranges and their shared ECAM base address. Assert and fail if same-segment entries ever carry different base addresses (not supported by this platform).