On 19.12.2025 09:39, Roger Pau Monné wrote:
> On Thu, Dec 18, 2025 at 08:56:24AM +0100, 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]>
>
> Acked-by: Roger Pau Monné <[email protected]>
>
> With the U suffix added to the constant, as noted by Stewart.
Thanks, albeit I'm not quite convinced I actually should put it in. Imo ...
>> ---
>> 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 - 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 think that's likely too much - for the hardware domain we want to
> allow the domain to access all the PCI config space, regardless of
> Xen's thinking there's nothing there.
... we really need to do better here, irrespective of this intended behavior
for hwdom. Us accessing the supposed extended capabilities list is already a
mistake when there's no extended config space. Us then calling
vpci_add_register() to "pin down" the value read is wrong too in that case.
Question here is whether even with that fixed the check being added here
would make sense to keep. In that case putting it in now and then doing the
other re-work would likely be the right thing to do.
>> 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)?
>
> If there's no explicitly handler added, the behavior for domU will
> already be to drop writes, and return reads as all 1s, which is fine
> for the rest of the extended config space? We just need to return 0
> for the first 32bits to avoid seeming to have extended capability
> support.
>
> Maybe we want to keep the same behavior as expected from native for
> legacy devices and just return all 1s consistency for the extended
> space?
>
> Hence we don't need to special case this region, as it's already
> covered by how unhandled accesses are resolved for domUs.
>
> Or is there something else I'm missing?
Imo correct behavior would be to return 0 for the first 32 bits when there
is extended config space, and ~0 when there isn't.
Jan
