Re: [PATCH v6 1/8] vpci/header: Emulate extended capability list for dom0

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.06.2025 11:29, Jiqian Chen wrote:
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -836,6 +836,42 @@ static int vpci_init_capability_list(struct pci_dev 
> *pdev)
>                                    PCI_STATUS_RSVDZ_MASK);
>  }
>  
> +static int vpci_init_ext_capability_list(struct pci_dev *pdev)
> +{
> +    unsigned int pos = PCI_CFG_SPACE_SIZE;
> +
> +    if ( !is_hardware_domain(pdev->domain) )
> +        /* Extended capabilities read as zero, write ignore for guest */

s/guest/DomU/ ?

> +        return vpci_add_register(pdev->vpci, vpci_read_val, NULL,
> +                                 pos, 4, (void *)0);
> +
> +    while ( pos >= PCI_CFG_SPACE_SIZE )
> +    {
> +        uint32_t header = pci_conf_read32(pdev->sbdf, pos);
> +        int rc;
> +
> +        if ( !header )
> +            return 0;

Is this a valid check to make for anything other than the first read? And even
if valid for the first one, shouldn't that also go through ...

> +        rc = vpci_add_register(pdev->vpci, vpci_read_val, vpci_hw_write32,
> +                               pos, 4, (void *)(uintptr_t)header);

... here?

> +        if ( rc == -EEXIST )
> +        {
> +            printk(XENLOG_WARNING
> +                   "%pd %pp: overlap in extended cap list, offset %#x\n",
> +                   pdev->domain, &pdev->sbdf, pos);
> +            return 0;
> +        }
> +
> +        if ( rc )
> +            return rc;
> +
> +        pos = PCI_EXT_CAP_NEXT(header);
> +    }

As a more general remark - this is imo the kind of situation where using
do ... while() would be better.

> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -267,6 +267,12 @@ void cf_check vpci_hw_write16(
>      pci_conf_write16(pdev->sbdf, reg, val);
>  }
>  
> +void cf_check vpci_hw_write32(
> +    const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data)
> +{
> +    pci_conf_write32(pdev->sbdf, reg, val);
> +}

Iirc we've been there before, yet I continue to wonder whether we're doing
ourselves any good in allowing writes to something that certainly better
wouldn't change. Even if we limit this to Dom0.

Jan