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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 12/18/25 11:14, Jan Beulich wrote:
> 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.

I see. Perhaps we ought to introduce some form of a pci_cfg_space_size()
function in Xen. pci_find_next_ext_capability() likely could benefit from such a
check, too.

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

For !is_hardware_domain(d), any access that isn't explicitly handled with
vpci_add_register{,*}() will default to "read all 1s, write ignore". See
vpci_{read,write}_hw().

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