Re: [PATCH] vPCI: avoid bogus "overlap in extended cap list" warnings

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.12.2025 16:37, Stewart Hildebrand wrote:
> On 12/18/25 02:56, Jan Beulich wrote:
>> Legacy PCI devices don't have any extended config space. Reading any part
>> thereof may very well return all ones. That then necessarily means we
>> would think we found a "loop", when there simply is nothing.
>>
>> Fixes: a845b50c12f3 ("vpci/header: Emulate extended capability list for 
>> dom0")
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> This is the minimalistic change to get rid of "overlap in extended cap
>> list" warnings I'm observing. We may want to avoid any attempt to access
>> extended config space when there is none
> 
> Agreed.

First - I realize only now that I should have Cc-ed you on both vPCI patches;
sorry.

>> - see Linux'es
>> pci_cfg_space_size() and it helper pci_cfg_space_size_ext(). This would
>> then also avoid us interpreting as an extended cap list what isn't one at
>> all (some legacy PCI devices don't decode register address bits 9-11, some
>> return other non-0, non-all-ones data). Including the risk of reading a
>> register with read side effects. Thoughts?
> 
> I couldn't find any mention in the PCIe spec how reads of extended config 
> space
> should behave for legacy PCI devices. So, you're right, reading all 1s may not
> be a guarantee. The PCIe spec seems to imply that a PCI Express Capability is
> required for devices that have extended config space. How about adding 
> something
> like this at the top of vpci_init_ext_capability_list() (untested)?
> 
> if ( !pci_find_cap_offset(pdev->sbdf, PCI_CAP_ID_EXP) )
>     return 0;
> 
> This would seem to me like a reasonable effort to handle the situation
> (according to spec), without the complexities/quirks/cruft that Linux has.

But it wouldn't be sufficient. Host bridges are special, and PCI-X also
needs handling.

>> The DomU part of the function worries me as well. Rather than making it
>> "read 0, write ignore" for just the first 32 bits, shouldn't we make it so
>> for the entire extended config space, and shouldn't we also make it "read
>> all ones, write ignore" when there is no extended config space in the
>> first place (then in particular also for the first 32 bits)?
> 
> Hm, yes, perhaps. If we simply omit the call to vpci_add_register(), it should
> default to the "read all ones, write ignore" behavior.

But it doesn't right now, unless I'm mistaken?

>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -839,6 +839,15 @@ static int vpci_init_ext_capability_list
>>          uint32_t header = pci_conf_read32(pdev->sbdf, pos);
>>          int rc;
>>  
>> +        if ( header == 0xffffffff )
> 
> This constant should have a U suffix.

Oh, of course - thanks for spotting. If we go the more sophisticated route,
this would disappear again anyway, though.

Jan