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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.01.2026 22:07, Stewart Hildebrand wrote:
> On 1/6/26 08:47, Jan Beulich wrote:
>> ---
>> 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).
> 
> I initially read "set" as in "set to true", but I think you mean that ext_cfg
> isn't assigned at all.

Both are the same really, due to alloc_pdev() using xzalloc().

> Though perhaps it should actually be set to true, because ...
> 
>> 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)?
> 
> ... in pdev_type(), we will only reach DEV_TYPE_PCI2PCIe_BRIDGE if it has
> PCI_CAP_ID_EXP, which would indicate the device has extended config. So maybe 
> it
> would be better to handle it similar to DEV_TYPE_PCIe2PCI_BRIDGE in
> alloc_pdev().

Hence my question. Since apparently you agree, I'll make that change. Maybe
in a separate, prereq patch.

>> @@ -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;
> 
> I'm using a machine where
> xen/arch/x86/x86_64/mmconfig-shared.c:is_mmconf_reserved() will initially 
> return
> false during Xen boot:
> 
> (XEN) PCI: MCFG configuration 0: base f0000000 segment 0000 buses 00 - 3f
> (XEN) PCI: Not using MCFG for segment 0000 bus 00-3f
> 
> Then, during dom0 boot, dom0 will call PHYSDEVOP_pci_mmcfg_reserved, after 
> which
> MCFG becomes enabled in Xen:
> 
> (XEN) PCI: Using MCFG for segment 0000 bus 00-3f
> 
> On such machines where mmcfg/ECAM is initially disabled, this will effectively
> set ->ext_cfg to false for all devices discovered at Xen boot.
> 
> I'm not really sure if I have any good suggestions, but perhaps we could add a
> macro or small function that returns something like
>    ( pdev->ext_cfg && pci_conf_read32(pdev->sbdf, PCI_CFG_SPACE_SIZE) != 
> 0xffffffffU )
> to allow this checking to happen dynamically (but this still wouldn't handle 
> the
> aliasing quirk). Maybe re-run the ext_cfg detection logic at the end of
> PHYSDEVOP_pci_mmcfg_reserved?
> 
> Regardless, I'd be happy to provide my R-b without this addressed, but I am
> curious if others think this as an issue?

Hmm, no, I forgot to consider that case (which in principle I'm well aware of).
Will need to fix in v2.

>> --- 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. */
> 
> Nit: it would be nice to clearly state if this means the extended config is
> accessible, or whether the device merely has it (but might not be accessible).

Well. Would the indicator be of any use if it wasn't accessible? Because if it
isn't accessible, we may not even be certain it exists. But yes, I can surely
make it "... has (accessible) extended ...".

Jan