On Thu, Jan 29, 2026 at 09:15:30AM +0100, Jan Beulich wrote: > On 28.01.2026 18:49, Roger Pau Monné wrote: > > On Mon, Jan 19, 2026 at 03:46:55PM +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. > >> > >> Signed-off-by: Jan Beulich <[email protected]>
Acked-by: Roger Pau Monné <[email protected]> > >> --- > >> Should we skip re-evaluation when pci_mmcfg_arch_enable() takes its early > >> exit path? > > > > Possibly - we expect no change in that case. However it would need > > to propagate some extra information into the callers. I could see > > that as a followup optimization. > > Okay, with Stewart also saying so I'll make this a follow-on then. > > >> 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. > > > > Hm, you probably want to do something similar to re-scanning the > > capability list, but avoid tearing down and re-setting the vPCI header > > logic to prevent unneeded p2m manipulations. We have no easy way to > > preempt this rescanning from the context of a > > PHYSDEVOP_pci_mmcfg_reserved call. > > Yes, definitely only re-evaluation of extended capabilities. Note, however, > that once we expose more of them, there might be a knock-on effects on the > P2M. Preemption in that case will be complicated, as we would have to defer p2m operations from multiple devices in the context of an hypercall. I guess we will cross that bridge when we get there. > >> 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? > > > > I would be fine with just a command line to disable the newly added > > behavior in case it causes issues. > > Can do. Will need to get creative as to the name of such an option. pci=check-ext-cfg=<bool>? Kind of a mouthful. > >> --- 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); > > > > I'm not sure why you need the forward declaration here, the function > > (in this patch) is just used after it's already defined. > > Well, this is needed for the same reason that the two decls just above are: > The file is also used for the COMPAT variant of the hypercall, and hence > the declaration needs to be visible there, while ... > > >> @@ -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) > > > > You can make this static I think? > > ... the definition doesn't need building a 2nd time (which hence also > can't be static). Oh, I see. > >> @@ -718,6 +721,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); > > > > You possibly also want to check that the PF (pf_pdev in this context I > > think) also has ext_cfg == true. > > I don't think so. No extended config space on a PF means no PF in that sense > in the first place, for then there not being any SR-IOV capability. Right, but won't it be possible for Xen to not be aware of the ECAM region for that device, yet the hardware domain somehow managed to enable SR-IOV it and request to register a VF? I'm not saying it's common, but it might be a useful sanity check. > >> @@ -1041,6 +1049,75 @@ 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, sig; > >> + > >> + pdev->ext_cfg = false; > > > > I think I would prefer if the ext_cfg field is only modified once Xen > > know the correct value to put there. > > Well, my main point of doing it this way is that the code ends up being a > little easier to follow. Especially without the optimization talked about > near the top, there inevitably will be a window in time where what the > field says is wrong. With the optimization there'll be two main cases: > - MCFG becoming newly available: The field starts out false in this case, > i.e. the write above is a no-op. > - MCFG disappearing (largely hypothetical, I think): The field may start > out true in this case, but will go false unless we have another access > mechanism for extended config space. It then can as well be set to > false as early as possible. Yes, with the optimization to not re-parse existing MMCFGs there's no transient windows where the filed is wrongly set. I also think the registering of MMCFG ares with Xen should be done ahead of the OS attempting to access the config space, and hence it's not possible for there to be in-flight accesses that could see transient invalid pdev->ext_cfg values. > > It would also be nice to detect > > cases where the device has pdev->ext_cfg == true but a new scan makes > > it switch to false. Which would signal something has likely gone very > > wrong, and we should print a warning. > > Why "very wrong"? If Dom0 tells us that MCFG shouldn't be used, there's > nothing "very wrong" with that. It's simply what firmware / ACPI are > telling us. There's also a message printed by `pci_mmcfg_arch_disable()` when the MMCFG is disabled, so likely we don't need a message printed by each device. > >> + /* > >> + * 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) > >> + */ > >> + 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); > > > > Hm, I think this shouldn't be very common as it seems limited to a > > short list of bridges. However every device under such bridge would > > be affected and repeatedly print the message. I wonder whether we > > should make this XENLOG_DEBUG instead, there isn't much the user can > > do to fix it. More a rant than a request though. > > XENLOG_DEBUG feels too weak for indicating a potential problem with a device. > I also don't see us marking bridges to limit the verbosity here, as the > issue may or may not be due to a bridge in between. Imo we can defer thinking > about limiting verbosity here until we see reports of this actually getting > overly verbose. OK, let's try with the current level then. Thanks, Roger.
