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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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.

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