[PATCH 1/6] PCI: determine whether a device has extended config space

[Jan Beulich <jbeulich@xxxxxxxx>]

Legacy PCI devices don't have any extended config space. Reading any part
thereof may return all ones or other arbitrary data, e.g. in some cases
base config space contents repeatedly.

Logic follows Linux 6.19-rc's pci_cfg_space_size(), albeit leveraging our
determination of device type; in particular some comments are taken
verbatim from there.

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
---
Note that alloc_pdev()'s switch doesn't handle DEV_TYPE_PCI2PCIe_BRIDGE at
all. Such bridges will therefore not have ->ext_cfg set (which is likely
wrong). Shouldn't we handle them like DEV_TYPE_LEGACY_PCI_BRIDGE (or
DEV_TYPE_PCI?) anyway (just like VT-d's set_msi_source_id() does)?

Linux also has CONFIG_PCI_QUIRKS, allowing to compile out the slightly
risky code (as reads may in principle have side effects). Should we gain
such, too?

--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -310,6 +310,41 @@ static void apply_quirks(struct pci_dev
              * from trying to size the BARs or add handlers to trap accesses.
              */
             pdev->ignore_bars = true;
+
+    if ( pdev->ext_cfg )
+    {
+        unsigned int pos;
+
+        /*
+         * PCI Express to PCI/PCI-X Bridge Specification, rev 1.0, 4.1.4 says
+         * that when forwarding a type1 configuration request the bridge must
+         * check that the extended register address field is zero.  The bridge
+         * is not permitted to forward the transactions and must handle it as
+         * an Unsupported Request.  Some bridges do not follow this rule and
+         * simply drop the extended register bits, resulting in the standard
+         * config space being aliased, every 256 bytes across the entire
+         * configuration space.  Test for this condition by comparing the first
+         * dword of each potential alias to the vendor/device ID.
+         * Known offenders:
+         *   ASM1083/1085 PCIe-to-PCI Reversible Bridge (1b21:1080, rev 01 & 
03)
+         *   AMD/ATI SBx00 PCI to PCI Bridge (1002:4384, rev 40)
+         */
+        for ( pos = PCI_CFG_SPACE_SIZE;
+              pos < PCI_CFG_SPACE_EXP_SIZE; pos += PCI_CFG_SPACE_SIZE )
+        {
+            if ( pci_conf_read16(pdev->sbdf, pos) != vendor ||
+                 pci_conf_read16(pdev->sbdf, pos + 2) != device )
+                break;
+        }
+
+        if ( pos >= PCI_CFG_SPACE_EXP_SIZE )
+        {
+            printk(XENLOG_WARNING
+                   "%pp: extended config space aliases base one\n",
+                   &pdev->sbdf);
+            pdev->ext_cfg = false;
+        }
+    }
 }
 
 static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
@@ -351,6 +386,8 @@ static struct pci_dev *alloc_pdev(struct
         unsigned long flags;
 
         case DEV_TYPE_PCIe2PCI_BRIDGE:
+            pdev->ext_cfg = true;
+            fallthrough;
         case DEV_TYPE_LEGACY_PCI_BRIDGE:
             sec_bus = pci_conf_read8(pdev->sbdf, PCI_SECONDARY_BUS);
             sub_bus = pci_conf_read8(pdev->sbdf, PCI_SUBORDINATE_BUS);
@@ -363,9 +400,19 @@ static struct pci_dev *alloc_pdev(struct
                 pseg->bus2bridge[sec_bus].devfn = devfn;
             }
             spin_unlock_irqrestore(&pseg->bus2bridge_lock, flags);
+
+            fallthrough;
+        case DEV_TYPE_PCI:
+            pos = pci_find_cap_offset(pdev->sbdf, PCI_CAP_ID_PCIX);
+            if ( pos &&
+                 (pci_conf_read32(pdev->sbdf, pos + PCI_X_STATUS) &
+                  (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)) )
+                pdev->ext_cfg = true;
             break;
 
         case DEV_TYPE_PCIe_ENDPOINT:
+            pdev->ext_cfg = true;
+
             pos = pci_find_cap_offset(pdev->sbdf, PCI_CAP_ID_EXP);
             BUG_ON(!pos);
             cap = pci_conf_read16(pdev->sbdf, pos + PCI_EXP_DEVCAP);
@@ -409,9 +456,9 @@ static struct pci_dev *alloc_pdev(struct
             }
             break;
 
-        case DEV_TYPE_PCI:
         case DEV_TYPE_PCIe_BRIDGE:
         case DEV_TYPE_PCI_HOST_BRIDGE:
+            pdev->ext_cfg = true;
             break;
 
         default:
@@ -420,6 +467,19 @@ static struct pci_dev *alloc_pdev(struct
             break;
     }
 
+    if ( pdev->ext_cfg &&
+         /*
+          * Regular PCI devices have 256 bytes, but PCI-X 2 and PCI Express
+          * devices have 4096 bytes.  Even if the device is capable, that
+          * doesn't mean we can access it.  Maybe we don't have a way to
+          * generate extended config space accesses, or the device is behind a
+          * reverse Express bridge.  So we try reading the dword at
+          * PCI_CFG_SPACE_SIZE which must either be 0 or a valid extended
+          * capability header.
+          */
+         pci_conf_read32(pdev->sbdf, PCI_CFG_SPACE_SIZE) == 0xffffffffU )
+        pdev->ext_cfg = false;
+
     apply_quirks(pdev);
     check_pdev(pdev);
 
@@ -717,6 +777,11 @@ int pci_add_device(u16 seg, u8 bus, u8 d
 
                 list_add(&pdev->vf_list, &pf_pdev->vf_list);
             }
+
+            if ( !pdev->ext_cfg )
+                printk(XENLOG_WARNING
+                       "%pp: VF without extended config space?\n",
+                       &pdev->sbdf);
         }
     }
 
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -126,6 +126,9 @@ struct pci_dev {
 
     nodeid_t node; /* NUMA node */
 
+    /* Whether the device has extended config space. */
+    bool ext_cfg;
+
     /* Device to be quarantined, don't automatically re-assign to dom0 */
     bool quarantine;