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@suse.com>
---
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?

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

Should we perhaps also log a warning if we exit the loop with non-zero
"pos"?

--- 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 )
+        {
+            if ( pos != PCI_CFG_SPACE_SIZE )
+                printk(XENLOG_WARNING
+                       "%pd %pp: broken extended cap list, offset %#x\n",
+                       pdev->domain, &pdev->sbdf, pos);
+            return 0;
+        }
+
         rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
                                pos, 4, (void *)(uintptr_t)header);
         if ( rc == -EEXIST )

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@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

With the U suffix added to the constant, as noted by Stewart.

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

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

> Should we perhaps also log a warning if we exit the loop with non-zero
> "pos"?

Possibly?  The spec says no next capability must be signaled by 00,
returning any other value below the extended space is a bug in the
device, but I have no idea whether that would be a common bug, and
whether things would get too noisy.

Thanks, Roger.

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@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> 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

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@suse.com>
> > 
> > Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> > 
> > 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?

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

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

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

I see, I was missing your point that there are older devices that
might not implement the extended space at all, and for those the
natural value to return would be ~0 instead of 0.

Xen could read the value the device returns for PCI_CFG_SPACE_SIZE
accesses, and whether it's ~0 or something else in order to device
whether the dummy handler added in vpci_init_ext_capability_list()
should return 0 or ~0, but that also risks possible side-effects if
the device aliases accesses >= PCI_CFG_SPACE_SIZE to registers in the
legacy config space region.

Thanks,

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@suse.com>
>>>
>>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>>>
>>> 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

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@suse.com>
> ---
> 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.

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

> 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. This assumes that ECAM
will be enabled for domUs so they can access the extended config space at all.
We have patches in our downstream to enable ECAM for domUs, intending to
eventually send upstream.

> Should we perhaps also log a warning if we exit the loop with non-zero
> "pos"?

Hm. I don't have any particularly strong opinion on this.

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

> +        {
> +            if ( pos != PCI_CFG_SPACE_SIZE )
> +                printk(XENLOG_WARNING
> +                       "%pd %pp: broken extended cap list, offset %#x\n",
> +                       pdev->domain, &pdev->sbdf, pos);
> +            return 0;
> +        }
> +
>          rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL,
>                                 pos, 4, (void *)(uintptr_t)header);
>          if ( rc == -EEXIST )
>

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@suse.com>
>> ---
>> 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

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@suse.com>
>>> ---
>>> 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

On 18.12.2025 19:56, Stewart Hildebrand wrote:
> 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:
>>>> 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().

Oh, indeed - I keep forgetting that "read hardware" as indicated by the name
doesn't really mean "read hardware" in all cases. (IOW I find this confusing.)

Jan