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. Like with Linux'es CONFIG_PCI_QUIRKS, only the alias
detection logic is covered by the new "pci=no-quirks". The singular access
at PCI_CFG_SPACE_SIZE is left unconditional.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
---
The warning near the bottom of pci_check_extcfg() may be issued multiple
times for a single device now. Should we try to avoid that?
Note that no vPCI adjustments are done here, but they're going to be
needed: Whatever requires extended capabilities will need re-
evaluating / newly establishing / tearing down in case an invocation of
PHYSDEVOP_pci_mmcfg_reserved alters global state.
---
v3: Add command line (sub-)option.
v2: Major re-work to also check upon PHYSDEVOP_pci_mmcfg_reserved
invocation.
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2009,12 +2009,21 @@ Only effective if CONFIG_PARTIAL_EMULATI
behavior.**
### pci
- = List of [ serr=<bool>, perr=<bool> ]
+ = List of [ serr=<bool>, perr=<bool>, quirks=<bool> ]
+
+* `serr` and `perr`
Default: Signaling left as set by firmware.
-Override the firmware settings, and explicitly enable or disable the
-signalling of PCI System and Parity errors.
+ Override the firmware settings, and explicitly enable or disable the
+ signalling of PCI System and Parity errors.
+
+* `quirks`
+
+ Default: `on`
+
+ In its negative form, allows to suppress certain quirk workarounds, in case
+ they cause issues.
### pci-phantom
> `=[<seg>:]<bus>:<device>,<stride>`
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -22,6 +22,8 @@ int physdev_map_pirq(struct domain *d, i
struct msi_info *msi);
int physdev_unmap_pirq(struct domain *d, int pirq);
+int cf_check physdev_check_pci_extcfg(struct pci_dev *pdev, void *arg);
+
#include "x86_64/mmconfig.h"
#ifndef COMPAT
@@ -160,6 +162,17 @@ int physdev_unmap_pirq(struct domain *d,
return ret;
}
+
+int cf_check physdev_check_pci_extcfg(struct pci_dev *pdev, void *arg)
+{
+ const struct physdev_pci_mmcfg_reserved *info = arg;
+
+ ASSERT(pdev->seg == info->segment);
+ if ( pdev->bus >= info->start_bus && pdev->bus <= info->end_bus )
+ pci_check_extcfg(pdev);
+
+ return 0;
+}
#endif /* COMPAT */
ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
@@ -511,6 +524,11 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
ret = pci_mmcfg_reserved(info.address, info.segment,
info.start_bus, info.end_bus, info.flags);
+
+ if ( !ret )
+ ret = pci_segment_iterate(info.segment, physdev_check_pci_extcfg,
+ &info);
+
if ( !ret && has_vpci(currd) && (info.flags & XEN_PCI_MMCFG_RESERVED) )
{
/*
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -183,6 +183,7 @@ custom_param("pci-phantom", parse_phanto
static u16 __read_mostly command_mask;
static u16 __read_mostly bridge_ctl_mask;
+static bool __ro_after_init opt_pci_quirks = true;
static int __init cf_check parse_pci_param(const char *s)
{
@@ -207,6 +208,8 @@ static int __init cf_check parse_pci_par
cmd_mask = PCI_COMMAND_PARITY;
brctl_mask = PCI_BRIDGE_CTL_PARITY;
}
+ else if ( (val = parse_boolean("quirks", s, ss)) >= 0 )
+ opt_pci_quirks = val;
else
rc = -EINVAL;
@@ -422,6 +425,9 @@ static struct pci_dev *alloc_pdev(struct
}
apply_quirks(pdev);
+
+ pci_check_extcfg(pdev);
+
check_pdev(pdev);
return pdev;
@@ -719,6 +725,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);
}
}
@@ -1042,6 +1053,79 @@ enum pdev_type pdev_type(u16 seg, u8 bus
return pos ? DEV_TYPE_PCIe_ENDPOINT : DEV_TYPE_PCI;
}
+void pci_check_extcfg(struct pci_dev *pdev)
+{
+ unsigned int pos;
+
+ pdev->ext_cfg = false;
+
+ switch ( pdev->type )
+ {
+ case DEV_TYPE_PCIe_ENDPOINT:
+ case DEV_TYPE_PCIe_BRIDGE:
+ case DEV_TYPE_PCI_HOST_BRIDGE:
+ case DEV_TYPE_PCIe2PCI_BRIDGE:
+ case DEV_TYPE_PCI2PCIe_BRIDGE:
+ break;
+
+ case DEV_TYPE_LEGACY_PCI_BRIDGE:
+ 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)) )
+ return;
+ break;
+
+ default:
+ return;
+ }
+
+ /*
+ * 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.
+ */
+ if ( pci_conf_read32(pdev->sbdf, PCI_CFG_SPACE_SIZE) == 0xffffffffU )
+ return;
+
+ if ( opt_pci_quirks )
+ {
+ /*
+ * 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)
+ */
+ unsigned int sig = pci_conf_read32(pdev->sbdf, PCI_VENDOR_ID);
+
+ for ( pos = PCI_CFG_SPACE_SIZE;
+ pos < PCI_CFG_SPACE_EXP_SIZE; pos += PCI_CFG_SPACE_SIZE )
+ if ( pci_conf_read32(pdev->sbdf, pos) != sig )
+ break;
+
+ if ( pos >= PCI_CFG_SPACE_EXP_SIZE )
+ {
+ printk(XENLOG_WARNING "%pp: extended config space aliases base one\n",
+ &pdev->sbdf);
+ return;
+ }
+ }
+
+ pdev->ext_cfg = true;
+}
+
/*
* find the upstream PCIe-to-PCI/PCIX bridge or PCI legacy bridge
* return 0: the device is integrated PCI device or PCIe
@@ -1842,6 +1926,29 @@ int pci_iterate_devices(int (*handler)(s
return pci_segments_iterate(iterate_all, &iter) ?: iter.rc;
}
+/* Iterate a single PCI segment, with locking but without preemption. */
+int pci_segment_iterate(unsigned int segment,
+ int (*handler)(struct pci_dev *pdev, void *arg),
+ void *arg)
+{
+ struct pci_seg *seg = get_pseg(segment);
+ struct segment_iter iter = {
+ .handler = handler,
+ .arg = arg,
+ };
+
+ if ( !seg )
+ return -ENODEV;
+
+ pcidevs_lock();
+
+ iter.rc = iterate_all(seg, &iter) ?: iter.rc;
+
+ pcidevs_unlock();
+
+ return iter.rc;
+}
+
/*
* Local variables:
* mode: C
--- 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 (accessible) extended config space. */
+ bool ext_cfg;
+
/* Device to be quarantined, don't automatically re-assign to dom0 */
bool quarantine;
@@ -242,6 +245,11 @@ void pci_check_disable_device(u16 seg, u
int pci_iterate_devices(int (*handler)(struct pci_dev *pdev, void *arg),
void *arg);
+/* Iterate a single PCI segment, with locking but without preemption. */
+int pci_segment_iterate(unsigned int segment,
+ int (*handler)(struct pci_dev *pdev, void *arg),
+ void *arg);
+
uint8_t pci_conf_read8(pci_sbdf_t sbdf, unsigned int reg);
uint16_t pci_conf_read16(pci_sbdf_t sbdf, unsigned int reg);
uint32_t pci_conf_read32(pci_sbdf_t sbdf, unsigned int reg);
@@ -260,6 +268,7 @@ unsigned int pci_find_next_cap_ttl(pci_s
unsigned int *ttl);
unsigned int pci_find_next_cap(pci_sbdf_t sbdf, unsigned int pos,
unsigned int cap);
+void pci_check_extcfg(struct pci_dev *pdev);
unsigned int pci_find_ext_capability(const struct pci_dev *pdev,
unsigned int cap);
unsigned int pci_find_next_ext_capability(const struct pci_dev *pdev,
On 29.01.2026 14:08, Jan Beulich wrote:
> @@ -1042,6 +1053,79 @@ enum pdev_type pdev_type(u16 seg, u8 bus
> return pos ? DEV_TYPE_PCIe_ENDPOINT : DEV_TYPE_PCI;
> }
>
> +void pci_check_extcfg(struct pci_dev *pdev)
> +{
> + unsigned int pos;
> +
> + pdev->ext_cfg = false;
> +
> + switch ( pdev->type )
> + {
> + case DEV_TYPE_PCIe_ENDPOINT:
> + case DEV_TYPE_PCIe_BRIDGE:
> + case DEV_TYPE_PCI_HOST_BRIDGE:
> + case DEV_TYPE_PCIe2PCI_BRIDGE:
> + case DEV_TYPE_PCI2PCIe_BRIDGE:
> + break;
> +
> + case DEV_TYPE_LEGACY_PCI_BRIDGE:
> + 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)) )
To not violate Misra rule 7.2 I'll fold in the change below. I guess I'll
further follow up with a patch adjusting other problematic #define-s in
that header, too.
Jan
--- a/xen/include/xen/pci_regs.h
+++ b/xen/include/xen/pci_regs.h
@@ -382,7 +382,7 @@
#define PCI_X_STATUS_MAX_CUM 0x1c000000 /* Designed Max Cumulative Read Size */
#define PCI_X_STATUS_SPL_ERR 0x20000000 /* Rcvd Split Completion Error Msg */
#define PCI_X_STATUS_266MHZ 0x40000000 /* 266 MHz capable */
-#define PCI_X_STATUS_533MHZ 0x80000000 /* 533 MHz capable */
+#define PCI_X_STATUS_533MHZ 0x80000000U /* 533 MHz capable */
/* PCI Express capability registers */
On Mon, Feb 02, 2026 at 10:14:57AM +0100, Jan Beulich wrote:
> On 29.01.2026 14:08, Jan Beulich wrote:
> > @@ -1042,6 +1053,79 @@ enum pdev_type pdev_type(u16 seg, u8 bus
> > return pos ? DEV_TYPE_PCIe_ENDPOINT : DEV_TYPE_PCI;
> > }
> >
> > +void pci_check_extcfg(struct pci_dev *pdev)
> > +{
> > + unsigned int pos;
> > +
> > + pdev->ext_cfg = false;
> > +
> > + switch ( pdev->type )
> > + {
> > + case DEV_TYPE_PCIe_ENDPOINT:
> > + case DEV_TYPE_PCIe_BRIDGE:
> > + case DEV_TYPE_PCI_HOST_BRIDGE:
> > + case DEV_TYPE_PCIe2PCI_BRIDGE:
> > + case DEV_TYPE_PCI2PCIe_BRIDGE:
> > + break;
> > +
> > + case DEV_TYPE_LEGACY_PCI_BRIDGE:
> > + 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)) )
>
> To not violate Misra rule 7.2 I'll fold in the change below. I guess I'll
> further follow up with a patch adjusting other problematic #define-s in
> that header, too.
I'm fine if you want to do a pre-patch fixing all the problematic
defines in the header and put it ahead of the series. Maybe it makes
more sense to fix them all in a single commit than fixing one here and
the rest separately?
Thanks, Roger.
On Thu, Jan 29, 2026 at 02:08:27PM +0100, Jan Beulich wrote: > 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. Like with Linux'es CONFIG_PCI_QUIRKS, only the alias > detection logic is covered by the new "pci=no-quirks". The singular access > at PCI_CFG_SPACE_SIZE is left unconditional. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Acked-by: Roger Pau Monné <roger.pau@citrix.com> > --- > The warning near the bottom of pci_check_extcfg() may be issued multiple > times for a single device now. Should we try to avoid that? > > Note that no vPCI adjustments are done here, but they're going to be > needed: Whatever requires extended capabilities will need re- > evaluating / newly establishing / tearing down in case an invocation of > PHYSDEVOP_pci_mmcfg_reserved alters global state. > --- > v3: Add command line (sub-)option. > v2: Major re-work to also check upon PHYSDEVOP_pci_mmcfg_reserved > invocation. > > --- a/docs/misc/xen-command-line.pandoc > +++ b/docs/misc/xen-command-line.pandoc > @@ -2009,12 +2009,21 @@ Only effective if CONFIG_PARTIAL_EMULATI > behavior.** > > ### pci > - = List of [ serr=<bool>, perr=<bool> ] > + = List of [ serr=<bool>, perr=<bool>, quirks=<bool> ] > + > +* `serr` and `perr` > > Default: Signaling left as set by firmware. > > -Override the firmware settings, and explicitly enable or disable the > -signalling of PCI System and Parity errors. > + Override the firmware settings, and explicitly enable or disable the > + signalling of PCI System and Parity errors. > + > +* `quirks` > + > + Default: `on` > + > + In its negative form, allows to suppress certain quirk workarounds, in case > + they cause issues. Not that I oppose to this, but I've assumed that you would introduce an option to fallback to the previous behavior where Xen would just assume extended space to be accessible. I'm fine with this, just assumed it would be a proper fallback to the previous behavior (which doesn't make much sense as it's partially bogus when used with vPCI). Regards, Roger.
On 29.01.2026 15:09, Roger Pau Monné wrote: > On Thu, Jan 29, 2026 at 02:08:27PM +0100, Jan Beulich wrote: >> 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. Like with Linux'es CONFIG_PCI_QUIRKS, only the alias >> detection logic is covered by the new "pci=no-quirks". The singular access >> at PCI_CFG_SPACE_SIZE is left unconditional. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> Acked-by: Roger Pau Monné <roger.pau@citrix.com> >> --- >> The warning near the bottom of pci_check_extcfg() may be issued multiple >> times for a single device now. Should we try to avoid that? >> >> Note that no vPCI adjustments are done here, but they're going to be >> needed: Whatever requires extended capabilities will need re- >> evaluating / newly establishing / tearing down in case an invocation of >> PHYSDEVOP_pci_mmcfg_reserved alters global state. >> --- >> v3: Add command line (sub-)option. >> v2: Major re-work to also check upon PHYSDEVOP_pci_mmcfg_reserved >> invocation. >> >> --- a/docs/misc/xen-command-line.pandoc >> +++ b/docs/misc/xen-command-line.pandoc >> @@ -2009,12 +2009,21 @@ Only effective if CONFIG_PARTIAL_EMULATI >> behavior.** >> >> ### pci >> - = List of [ serr=<bool>, perr=<bool> ] >> + = List of [ serr=<bool>, perr=<bool>, quirks=<bool> ] >> + >> +* `serr` and `perr` >> >> Default: Signaling left as set by firmware. >> >> -Override the firmware settings, and explicitly enable or disable the >> -signalling of PCI System and Parity errors. >> + Override the firmware settings, and explicitly enable or disable the >> + signalling of PCI System and Parity errors. >> + >> +* `quirks` >> + >> + Default: `on` >> + >> + In its negative form, allows to suppress certain quirk workarounds, in case >> + they cause issues. > > Not that I oppose to this, but I've assumed that you would introduce > an option to fallback to the previous behavior where Xen would just > assume extended space to be accessible. But that would be wrong. It didn't even occur to me that you could have wanted that. Jan
© 2016 - 2026 Red Hat, Inc.