|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 1/6] PCI: determine whether a device has extended config space
Le 06/01/2026 à 14:50, Jan Beulich a écrit :
> 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;
> + }
> + }
Given that it only appears to be the case for PCIe to PCI/PCI-X bridges,
do we want to check this for all devices ?
> }
>
> 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;
>
>
>
--
Teddy Astie | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |