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
