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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.11.2024 17:09, Stewart Hildebrand wrote:
> Add links between a VF's struct pci_dev and its associated PF struct
> pci_dev.
> 
> The hardware domain is expected to remove the associated VFs before
> removing the PF. If removal happens out of order, print a warning and
> return an error. This means that VFs can only exist with an associated
> PF.
> 
> Additionally, if the hardware domain attempts to remove a PF with VFs
> still present, mark the PF and VFs broken, because Linux Dom0 has been
> observed to not respect the error returned.
> 
> Move the calls to pci_get_pdev() and pci_add_device() down to avoid
> dropping and re-acquiring the pcidevs_lock().
> 
> Check !pdev->pf_pdev before adding the VF to the list to guard against
> adding it multiple times.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

> @@ -698,12 +691,48 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>      if ( info )
>      {
>          pdev->info = *info;
> -        /*
> -         * VF's 'is_extfn' field is used to indicate whether its PF is an
> -         * extended function.
> -         */
>          if ( pdev->info.is_virtfn )
> -            pdev->info.is_extfn = pf_is_extfn;
> +        {
> +            struct pci_dev *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 )
> +                {
> +                    printk(XENLOG_ERR
> +                           "Inconsistent PCI state: failed to find newly 
> added PF %pp for VF %pp\n",
> +                           &PCI_SBDF(seg, info->physfn.bus, 
> info->physfn.devfn),
> +                           &pdev->sbdf);
> +                    ASSERT_UNREACHABLE();
> +                    free_pdev(pseg, pdev);
> +                    ret = -EILSEQ;
> +                    goto out;
> +                }
> +            }
> +
> +            if ( !pdev->pf_pdev )
> +            {
> +                /* VF inherits its 'is_extfn' from PF */
> +                pdev->info.is_extfn = pf_pdev->info.is_extfn;
> +                list_add(&pdev->vf_list, &pf_pdev->vf_list);
> +                pdev->pf_pdev = pf_pdev;

As a general pattern, I think filling fields before adding to lists is 
preferable.
For now it doesn't really matter here (lock held here and while removing), yet
still: Thoughts towards flipping the last two lines above, perhaps while 
committing?

Jan