Re: [PATCH v2 2/4] PCI: determine whether a device has extended config space

[Jan Beulich <jbeulich@xxxxxxxx>]

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 <jbeulich@xxxxxxxx>
>> ---
>> 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.

>> 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.

>> --- 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).

>> @@ -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.

>> @@ -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.

>  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.

>> +    /*
>> +     * 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.

Jan