Re: [PATCH v4 5/5] vpci: add SR-IOV support for DomUs

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.10.2026 16:08, Mykyta Poturai wrote:
> --- a/xen/drivers/vpci/sriov.c
> +++ b/xen/drivers/vpci/sriov.c
> @@ -79,19 +79,80 @@ int vpci_vf_init_header(struct pci_dev *vf_pdev)
>      const struct pci_dev *pf_pdev;
>      int rc = 0;
>      uint16_t ctrl;
> +    unsigned int sriov_pos;
>  
>      ASSERT(rw_is_write_locked(&vf_pdev->domain->pci_lock));
>      ASSERT(vf_pdev->info.is_virtfn);
>  
>      pf_pdev = vf_pdev->pf_pdev;
>      ASSERT(pf_pdev);
> +    sriov_pos = pf_pdev->vpci->sriov->pos;

Can't this move ...

>      rc = vf_init_bars(vf_pdev);
>      if ( rc )
>          return rc;

... here?

> -    ctrl = pci_conf_read16(pf_pdev->sbdf,
> -                           pf_pdev->vpci->sriov->pos + PCI_SRIOV_CTRL);
> +    ctrl = pci_conf_read16(pf_pdev->sbdf, sriov_pos + PCI_SRIOV_CTRL);
> +
> +    if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) &&
> +         pf_pdev->domain != vf_pdev->domain )
> +    {
> +        struct vpci_bar *bars = vf_pdev->vpci->header.bars;
> +        /*
> +         * A VF's Vendor and Device ID registers read as 0xffff. The guest 
> has
> +         * no PF to synthesise them from, so take the vendor from the PF and
> +         * the device from its SR-IOV capability.
> +         */
> +        uint16_t vid = pci_conf_read16(pf_pdev->sbdf, PCI_VENDOR_ID);
> +        uint16_t did = pci_conf_read16(pf_pdev->sbdf,
> +                                       sriov_pos + PCI_SRIOV_VF_DID);
> +
> +        rc = vpci_add_register(vf_pdev->vpci, vpci_read_val, NULL,
> +                               PCI_VENDOR_ID, 2, (void *)(uintptr_t)vid);

This effectively open-codes _p().

> +        if ( rc )
> +            return rc;
> +
> +        rc = vpci_add_register(vf_pdev->vpci, vpci_read_val, NULL,
> +                               PCI_DEVICE_ID, 2, (void *)(uintptr_t)did);

Same here.

> +        if ( rc )
> +            return rc;
> +
> +        /* Hardcode multi-function device bit to 0 */
> +        rc = vpci_add_register(vf_pdev->vpci, vpci_read_val, NULL,
> +                               PCI_HEADER_TYPE, 1,
> +                               (void *)PCI_HEADER_TYPE_NORMAL);

And this works only because PCI_HEADER_TYPE_NORMAL happens to be 0. Imo
_p() wants using here as well.

> +        if ( rc )
> +            return rc;
> +
> +        rc = vpci_add_register(vf_pdev->vpci, vpci_hw_read32, NULL,
> +                               PCI_CLASS_REVISION, 4, NULL);
> +        if ( rc )
> +            return rc;

How is this SR-IOV-related?

> +        for ( unsigned int i = 0; i < PCI_SRIOV_NUM_BARS; i++ )
> +        {
> +            switch ( pf_pdev->vpci->sriov->vf_bars[i].type )
> +            {
> +            case VPCI_BAR_MEM32:
> +            case VPCI_BAR_MEM64_LO:
> +            case VPCI_BAR_MEM64_HI:
> +                rc = vpci_add_register(vf_pdev->vpci, 
> vpci_guest_mem_bar_read,
> +                                       vpci_guest_mem_bar_write,
> +                                       PCI_BASE_ADDRESS_0 + i * 4, 4, 
> &bars[i]);
> +                if ( rc )
> +                    return rc;
> +                break;
> +
> +            default:
> +                rc = vpci_add_register(vf_pdev->vpci, vpci_read_val, NULL,
> +                                       PCI_BASE_ADDRESS_0 + i * 4, 4,
> +                                       (void *)0);

Unlike above here I'm less certain - we deliberately use this pattern
elsewhere already. Yet maybe those would best be switched to _p() as
well.

Jan