From 23545998896c4a4f0ac043862a11b960e6a5914e Mon Sep 17 00:00:00 2001 From: Mikhail Chusavitin Date: Fri, 28 Aug 2026 09:12:02 +0300 Subject: [PATCH] fix(pcie): account for downstream link capability --- audit/internal/platform/pcie_link_check.go | 142 ++++++++++++++++-- .../internal/platform/pcie_link_check_test.go | 78 ++++++++++ 2 files changed, 205 insertions(+), 15 deletions(-) diff --git a/audit/internal/platform/pcie_link_check.go b/audit/internal/platform/pcie_link_check.go index e4951e2..262dc15 100644 --- a/audit/internal/platform/pcie_link_check.go +++ b/audit/internal/platform/pcie_link_check.go @@ -22,20 +22,22 @@ const pcieLinkRetrainTimeout = 2 * time.Second // pcieLinkFinding is one device's before/after link-speed retrain result. type pcieLinkFinding struct { - BDF string - Description string - VendorID string - ClassCode string - IsGPU bool - GPUVendor string // "nvidia" or "amd", only set when IsGPU - Skipped string // non-empty reason this device wasn't retrained - BeforeSpeed string - AfterSpeed string - MaxSpeed string - Width int - MaxWidth int - Degraded bool - NotPresent bool // true when the slot trained to zero lanes: nothing is plugged in (or it fell off the bus), not a speed regression + BDF string + Description string + VendorID string + ClassCode string + IsGPU bool + GPUVendor string // "nvidia" or "amd", only set when IsGPU + Skipped string // non-empty reason this device wasn't retrained + BeforeSpeed string + AfterSpeed string + MaxSpeed string + PortMaxSpeed string // bridge's own capability when MaxSpeed is limited by its downstream peer + Width int + MaxWidth int + PortMaxWidth int // bridge's own capability when MaxWidth is limited by its downstream peer + Degraded bool + NotPresent bool // true when the slot trained to zero lanes: nothing is plugged in (or it fell off the bus), not a speed regression } // RunPCIeLinkCheckPack forces every enabled PCIe device to retrain its link @@ -154,6 +156,22 @@ func retrainAndSamplePCIeDevice(ctx context.Context, verboseLog, bdf string, log return f } + // A bridge/root port reports its own maximum capability in sysfs, not + // the maximum mutually supported by the device at the other end of the + // link. Comparing a Gen4 x16 root port directly with a Gen3 x8 or Gen2 + // x4 endpoint therefore produces a false degradation even though the + // link is running at the fastest rate the endpoint supports. The child + // device is tested separately, so use its advertised capability to + // calculate the real target for this bridge-side view of the same link. + if isPCIeBridgeClass(class) { + if peerSpeed, peerWidth, ok := downstreamPCIeLinkCapability(bdf); ok { + f.PortMaxSpeed = f.MaxSpeed + f.PortMaxWidth = f.MaxWidth + f.MaxSpeed = minPCIeLinkSpeed(f.MaxSpeed, peerSpeed) + f.MaxWidth = minPositiveInt(f.MaxWidth, peerWidth) + } + } + if err := retrainPCIeLink(ctx, verboseLog, bdf, logFunc); err != nil { f.Skipped = "retrain failed: " + err.Error() f.AfterSpeed = before @@ -271,6 +289,96 @@ func readPCIeSysfsHex(bdf, attr string) (string, bool) { return strings.TrimSpace(string(raw)), true } +// isPCIeBridgeClass matches PCI class 0x0604xx (PCI-to-PCI bridge). These +// functions describe the upstream side of a downstream link, so their own +// max_link_* values must be capped by the peer's capability. +func isPCIeBridgeClass(classHex string) bool { + c := strings.TrimPrefix(strings.ToLower(strings.TrimSpace(classHex)), "0x") + return len(c) >= 4 && c[:4] == "0604" +} + +// downstreamPCIeLinkCapability returns the strongest capability advertised +// by a bridge's immediate child functions. In sysfs those functions are +// direct entries below the bridge device directory. Multifunction devices +// expose several children for one physical link; taking the strongest values +// avoids understating the link because one auxiliary function omitted data. +func downstreamPCIeLinkCapability(bdf string) (speed string, width int, ok bool) { + bridgeDir := filepath.Join("/sys/bus/pci/devices", bdf) + return downstreamPCIeLinkCapabilityAt(bridgeDir) +} + +func downstreamPCIeLinkCapabilityAt(bridgeDir string) (speed string, width int, ok bool) { + entries, err := os.ReadDir(bridgeDir) + if err != nil { + return "", 0, false + } + for _, entry := range entries { + if !isFullPCIBDF(entry.Name()) { + continue + } + childDir := filepath.Join(bridgeDir, entry.Name()) + rawSpeed, speedErr := os.ReadFile(filepath.Join(childDir, "max_link_speed")) + if speedErr != nil { + continue + } + childSpeed := collector.NormalizePCILinkSpeed(strings.TrimSpace(string(rawSpeed))) + if pcieGeneration(childSpeed) > pcieGeneration(speed) { + speed = childSpeed + } + if rawWidth, widthErr := os.ReadFile(filepath.Join(childDir, "max_link_width")); widthErr == nil { + if childWidth, parseErr := strconv.Atoi(strings.TrimSpace(string(rawWidth))); parseErr == nil && childWidth > width { + width = childWidth + } + } + ok = true + } + return speed, width, ok && speed != "" +} + +func isFullPCIBDF(s string) bool { + if len(s) != len("0000:00:00.0") || s[4] != ':' || s[7] != ':' || s[10] != '.' { + return false + } + for i, r := range s { + if i == 4 || i == 7 || i == 10 { + continue + } + if !((r >= '0' && r <= '9') || (r >= 'a' && r <= 'f') || (r >= 'A' && r <= 'F')) { + return false + } + } + return true +} + +func minPCIeLinkSpeed(a, b string) string { + ga, gb := pcieGeneration(a), pcieGeneration(b) + switch { + case ga == 0: + return b + case gb == 0 || ga <= gb: + return a + default: + return b + } +} + +func pcieGeneration(speed string) int { + v := strings.TrimPrefix(strings.TrimSpace(speed), "Gen") + gen, _ := strconv.Atoi(v) + return gen +} + +func minPositiveInt(a, b int) int { + switch { + case a <= 0: + return b + case b <= 0 || a <= b: + return a + default: + return b + } +} + // classifyGPUFromVendorClass reports whether a device is a GPU die itself // (PCI base class 0x03 — Display Controller — under NVIDIA/AMD's vendor // ID), as opposed to a same-vendor companion device (NIC, storage @@ -378,8 +486,12 @@ func renderPCIeLinkCheckReport(findings []pcieLinkFinding) string { case f.Degraded: verdict = "DEGRADED" } - fmt.Fprintf(&b, " %s: before=%s after=%s max=%s width=%d/%d\n", + fmt.Fprintf(&b, " %s: before=%s after=%s max=%s width=%d/%d", verdict, f.BeforeSpeed, f.AfterSpeed, f.MaxSpeed, f.Width, f.MaxWidth) + if f.PortMaxSpeed != "" && (f.PortMaxSpeed != f.MaxSpeed || f.PortMaxWidth != f.MaxWidth) { + fmt.Fprintf(&b, " (port capability %s x%d, limited by downstream device)", f.PortMaxSpeed, f.PortMaxWidth) + } + fmt.Fprintln(&b) if f.Skipped != "" { fmt.Fprintf(&b, " note: %s\n", f.Skipped) } diff --git a/audit/internal/platform/pcie_link_check_test.go b/audit/internal/platform/pcie_link_check_test.go index 273b56c..e162ef4 100644 --- a/audit/internal/platform/pcie_link_check_test.go +++ b/audit/internal/platform/pcie_link_check_test.go @@ -1,6 +1,8 @@ package platform import ( + "os" + "path/filepath" "strings" "testing" ) @@ -30,6 +32,82 @@ func TestClassifyGPUFromVendorClass(t *testing.T) { } } +func TestPCIeBridgeUsesDownstreamCapability(t *testing.T) { + cases := []struct { + name string + portSpeed string + portWidth int + endpointSpeed string + endpointWidth int + wantSpeed string + wantWidth int + }{ + {"ConnectX-5 behind Gen4 root port", "Gen4", 16, "Gen3", 8, "Gen3", 8}, + {"Adaptec SAS behind Gen4 root port", "Gen4", 16, "Gen3", 8, "Gen3", 8}, + {"I350 behind Gen4 root port", "Gen4", 16, "Gen2", 4, "Gen2", 4}, + {"faster endpoint remains port-limited", "Gen3", 8, "Gen4", 16, "Gen3", 8}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + gotSpeed := minPCIeLinkSpeed(tc.portSpeed, tc.endpointSpeed) + gotWidth := minPositiveInt(tc.portWidth, tc.endpointWidth) + if gotSpeed != tc.wantSpeed || gotWidth != tc.wantWidth { + t.Fatalf("effective capability = %s x%d, want %s x%d", gotSpeed, gotWidth, tc.wantSpeed, tc.wantWidth) + } + }) + } +} + +func TestPCIeBridgeAtEndpointMaximumPasses(t *testing.T) { + maxSpeed := minPCIeLinkSpeed("Gen4", "Gen3") + maxWidth := minPositiveInt(16, 8) + finding := pcieLinkFinding{ + BDF: "0000:4a:02.0", + Description: "Intel root port to ConnectX-5", + BeforeSpeed: "Gen3", + AfterSpeed: "Gen3", + MaxSpeed: maxSpeed, + PortMaxSpeed: "Gen4", + Width: 8, + MaxWidth: maxWidth, + PortMaxWidth: 16, + Degraded: "Gen3" != maxSpeed, + } + + summary := renderPCIeLinkCheckSummary([]pcieLinkFinding{finding}) + if !strings.Contains(summary, "overall_status=OK") { + t.Fatalf("endpoint-limited bridge should pass, got:\n%s", summary) + } + report := renderPCIeLinkCheckReport([]pcieLinkFinding{finding}) + if !strings.Contains(report, "limited by downstream device") { + t.Fatalf("report should explain the effective maximum, got:\n%s", report) + } +} + +func TestDownstreamPCIeLinkCapabilityFromSysfsTopology(t *testing.T) { + bridgeDir := t.TempDir() + childDir := filepath.Join(bridgeDir, "0000:4b:00.0") + if err := os.Mkdir(childDir, 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(childDir, "max_link_speed"), []byte("8.0 GT/s PCIe\n"), 0644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(childDir, "max_link_width"), []byte("8\n"), 0644); err != nil { + t.Fatal(err) + } + // A non-BDF sysfs entry must not influence peer discovery. + if err := os.WriteFile(filepath.Join(bridgeDir, "max_link_speed"), []byte("16.0 GT/s PCIe\n"), 0644); err != nil { + t.Fatal(err) + } + + speed, width, ok := downstreamPCIeLinkCapabilityAt(bridgeDir) + if !ok || speed != "Gen3" || width != 8 { + t.Fatalf("downstream capability = (%q, %d, %v), want (Gen3, 8, true)", speed, width, ok) + } +} + func TestRenderPCIeLinkCheckSummaryDegradedGPU(t *testing.T) { findings := []pcieLinkFinding{ {BDF: "0000:0d:00.0", Description: "NVIDIA GPU", IsGPU: true, GPUVendor: "nvidia",