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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.12.2025 16:19, Roger Pau Monné wrote:
> On Mon, Dec 22, 2025 at 09:39:38AM +0100, Jan Beulich wrote:
>> 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 <jbeulich@xxxxxxxx>
>>>
>>> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>>
>>> 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 ...
> 
> What about using ~0U instead of the longish 0xfff... hex literal?

Oh, sorry, my reply apparently was ambiguous. I wasn't questioning the U, but
whether to commit the change after adding the U (as expanded upon further
down).

And no, I specifically replaced an earlier form where I also made assumptions
about unsigned int being 32 bits wide. ~0U would make the same assumption.

> Am I misremembering that we had a coding style rule to write hex
> numbers all in uppercase letters?  I don't seem to find it anywhere

I would have hoped you didn't find any: I, for one, consider them harder to
read and harder to distinguish from #define-s. Plus with the U in upper case
that also better separates from the digits.

>>>> ---
>>>> 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.
> 
> Hm, yes, it would be better for Xen to use a logic similar to Linux's
> helpers to find the config space size.  This would need extending to
> pci_find_next_ext_capability(), as Xen does read the extended space
> without any checks there also.
> 
>> 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.
> 
> Yes, it probably wants to be there in addition to the extended checks
> for extended space presence.  If we have a pre-check that ensures the
> extended space is available, Xen should return an error also if
> reading from PCI_CFG_SPACE_SIZE returns ~0, as in that case the device
> must handle at least that access and return 0 to signal no extended
> capabilities.

Okay, so I'll commit the change as is and then make another change along
those lines. Or perhaps two, a 2nd one to deal with the DomU aspect. That
may only be in the new year, though.

Jan