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 <[email protected]>
>> ---
>> 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
