Re: [PATCH] drm/amdgpu: Guard reads in pcie state readout

[Lazar, Lijo]

On 17-Jun-26 4:01 PM, Wang, Yang(Kevin) wrote:
AMD General

The original intent of this function is to collect the real hardware PCIe state 
from the AMD GPU's internal upstream/downstream switch ports.

However, under QEMU/VFIO passthrough, the guest-visible PCIe topology and PCI 
config space is virtualized.
QEMU/VFIO can intercept, emulate, or selectively forward PCI config accesses, 
so the upstream bridge information seen by the guest does not necessarily 
represent the real hardware topology or state.

Therefore, in a VFIO guest this function no longer has practical meaning for 
retrieving real HW PCIe state.
At most, it reads the virtual/emulated PCIe state exposed by QEMU/VFIO, which may 
not reflect the physical device (which depend on the implementation of QEMU/VFIO 
& kernel VFIO Driver source code)


This function reads mostly the endpoint registers (mostly SMN) which I 
think are not intercepted (driver will have trouble otherwise for other 
reg reads). The particular reads addressed in this patch are related to 
link status for which it needs the real physical link. This is to 
avoiding that particular path.

Thanks,
Lijo

So, I suggest that it is better to hide these interfaces under PT or SRIOV mode.

Best Regards,
Kevin

-----Original Message-----
From: amd-gfx <amd-gfx-boun...@lists.freedesktop.org> On Behalf Of Lijo Lazar
Sent: Wednesday, June 17, 2026 17:35
To: amd-gfx@lists.freedesktop.org
Cc: Zhang, Hawking <hawking.zh...@amd.com>; Deucher, Alexander
<alexander.deuc...@amd.com>; Kamal, Asad <asad.ka...@amd.com>; Li,
Candice <candice...@amd.com>
Subject: [PATCH] drm/amdgpu: Guard reads in pcie state readout

Internal US/DS switch may not be exposed in passthrough. Guard the upstream port
reads to avoid a NULL dereference.

Signed-off-by: Lijo Lazar <lijo.la...@amd.com>
---
  drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c | 59 +++++++++++++++-------
  1 file changed, 42 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c
b/drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c
index 72ea37dbfea8..5f1389901504 100644
--- a/drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c
+++ b/drivers/gpu/drm/amd/amdgpu/aqua_vanjaram.c
@@ -589,6 +589,29 @@ static struct aqua_reg_list pcie_reg_addrs[] = {
       { smreg_0x1A380088, 6, DW_ADDR_INCR },  };

+/*
+ * Return the GPU's internal US switch port, or NULL if it is not
+visible
+ * (e.g. passthrough) or the EP is parented under an unrelated bridge.
+ */
+static struct pci_dev *aqua_vanjaram_get_us_pdev(struct amdgpu_device
+*adev) {
+     struct pci_dev *ds_pdev, *us_pdev;
+
+     ds_pdev = pci_upstream_bridge(adev->pdev);
+     if (!ds_pdev || ds_pdev->vendor != PCI_VENDOR_ID_ATI ||
+         pci_pcie_type(ds_pdev) != PCI_EXP_TYPE_DOWNSTREAM)
+             return NULL;
+
+     us_pdev = pci_upstream_bridge(ds_pdev);
+     if (!us_pdev ||
+         (us_pdev->vendor != PCI_VENDOR_ID_ATI &&
+          us_pdev->vendor != PCI_VENDOR_ID_AMD) ||
+         pci_pcie_type(us_pdev) != PCI_EXP_TYPE_UPSTREAM)
+             return NULL;
+
+     return us_pdev;
+}
+
  static ssize_t aqua_vanjaram_read_pcie_state(struct amdgpu_device *adev,
                                            void *buf, size_t max_size)
  {
@@ -596,7 +619,7 @@ static ssize_t aqua_vanjaram_read_pcie_state(struct
amdgpu_device *adev,
       uint32_t start_addr, incrx, num_regs, szbuf;
       struct amdgpu_regs_pcie_v1_0 *pcie_regs;
       struct amdgpu_smn_reg_data *reg_data;
-     struct pci_dev *us_pdev, *ds_pdev;
+     struct pci_dev *us_pdev;
       int aer_cap, r, n;

       if (!buf || !max_size)
@@ -628,25 +651,27 @@ static ssize_t aqua_vanjaram_read_pcie_state(struct
amdgpu_device *adev,
               }
       }

-     ds_pdev = pci_upstream_bridge(adev->pdev);
-     us_pdev = pci_upstream_bridge(ds_pdev);
+     us_pdev = aqua_vanjaram_get_us_pdev(adev);
+     if (us_pdev) {
+             pcie_capability_read_word(us_pdev, PCI_EXP_DEVSTA,
+                                       &pcie_regs->device_status);
+             pcie_capability_read_word(us_pdev, PCI_EXP_LNKSTA,
+                                       &pcie_regs->link_status);
+
+             aer_cap = pci_find_ext_capability(us_pdev,
PCI_EXT_CAP_ID_ERR);
+             if (aer_cap) {
+                     pci_read_config_dword(us_pdev,
+                                           aer_cap + PCI_ERR_COR_STATUS,
+                                           &pcie_regs->pcie_corr_err_status);
+                     pci_read_config_dword(us_pdev,
+                                           aer_cap + PCI_ERR_UNCOR_STATUS,
+                                           &pcie_regs->pcie_uncorr_err_status);
+             }

-     pcie_capability_read_word(us_pdev, PCI_EXP_DEVSTA,
-                               &pcie_regs->device_status);
-     pcie_capability_read_word(us_pdev, PCI_EXP_LNKSTA,
-                               &pcie_regs->link_status);
-
-     aer_cap = pci_find_ext_capability(us_pdev, PCI_EXT_CAP_ID_ERR);
-     if (aer_cap) {
-             pci_read_config_dword(us_pdev, aer_cap +
PCI_ERR_COR_STATUS,
-                                   &pcie_regs->pcie_corr_err_status);
-             pci_read_config_dword(us_pdev, aer_cap +
PCI_ERR_UNCOR_STATUS,
-                                   &pcie_regs->pcie_uncorr_err_status);
+             pci_read_config_dword(us_pdev, PCI_PRIMARY_BUS,
+                                   &pcie_regs->sub_bus_number_latency);
       }

-     pci_read_config_dword(us_pdev, PCI_PRIMARY_BUS,
-                           &pcie_regs->sub_bus_number_latency);
-
       pcie_reg_state->common_header.structure_size = szbuf;
       pcie_reg_state->common_header.format_revision = 1;
       pcie_reg_state->common_header.content_revision = 0;
--
2.49.0