Re: [PATCH v5 2/3] xen/pci: introduce PF<->VF links

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.10.2024 17:27, Stewart Hildebrand wrote:
> Add links between a VF's struct pci_dev and its associated PF struct
> pci_dev. Move the calls to pci_get_pdev()/pci_add_device() down to avoid
> dropping and re-acquiring the pcidevs_lock().
> 
> During PF removal, unlink VF from PF and mark the VF broken. As before,
> VFs may exist without a corresponding PF, although now only with
> pdev->broken = true. If the PF is removed and re-added, dom0 is expected
> to also remove and re-add the VFs.

Right, or else the VF struct instance would remain orphaned the way you've
implemented this. Question is whether it is a reasonable assumption that a
Dom0 which failed to remove the VFs during PF removal might later
"remember" that it still needs to report VFs removed. I for one doubt that.

> @@ -703,7 +696,44 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>           * extended function.
>           */
>          if ( pdev->info.is_virtfn )
> -            pdev->info.is_extfn = pf_is_extfn;
> +        {
> +            struct pci_dev *pf_pdev;
> +
> +            pf_pdev = pci_get_pdev(NULL,
> +                                   PCI_SBDF(seg, info->physfn.bus,
> +                                            info->physfn.devfn));
> +
> +            if ( !pf_pdev )
> +            {
> +                ret = pci_add_device(seg, info->physfn.bus, 
> info->physfn.devfn,
> +                                     NULL, node);
> +                if ( ret )
> +                {
> +                    printk(XENLOG_WARNING "Failed to add SR-IOV device PF 
> %pp for VF %pp\n",
> +                           &PCI_SBDF(seg, info->physfn.bus, 
> info->physfn.devfn),
> +                           &pdev->sbdf);
> +                    free_pdev(pseg, pdev);
> +                    goto out;
> +                }
> +                pf_pdev = pci_get_pdev(NULL,
> +                                       PCI_SBDF(seg, info->physfn.bus,
> +                                                info->physfn.devfn));
> +                if ( !pf_pdev )
> +                {
> +                    ASSERT_UNREACHABLE();
> +                    printk(XENLOG_ERR "Failed to find SR-IOV device PF %pp 
> for VF %pp\n",
> +                           &PCI_SBDF(seg, info->physfn.bus, 
> info->physfn.devfn),
> +                           &pdev->sbdf);
> +                    free_pdev(pseg, pdev);
> +                    ret = -EILSEQ;
> +                    goto out;

Might be helpful to have the printk() ahead of the ASSERT_UNREACHABLE(), in the
unlikely event that the assertion would actually trigger. Positioning doesn't
make a difference for release builds anyway.

Jan