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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.11.2024 21:53, 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 add a PF first before adding
> associated VFs. Similarly, the hardware domain is expected to remove the
> associated VFs before removing the PF. If adding/removing 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 call to pci_get_pdev() down to avoid dropping and re-acquiring
> the pcidevs_lock(). Drop the call to pci_add_device() as it is invalid
> to add a VF without an existing PF.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>

I'm okay with this, so in principle
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

A few comments nevertheless, which may result in there wanting to be
another revision.

> ---
> Candidate for backport to 4.19 (the next patch depends on this one)

With this dependency (we definitely want to backport the other patch) ...

> v6->v7:
> * cope with multiple invocations of pci_add_device for VFs
> * get rid of enclosing struct for single member
> * during PF removal attempt with VFs still present:
>     * keep PF
>     * mark broken
>     * don't unlink
>     * return error
> * during VF add:
>     * initialize pf_pdev in declaration
>     * remove logic catering to adding VFs without PF

... I'd like to point out that this change has an at least theoretical
risk of causing regressions. I therefore wonder whether that wouldn't
better be a separate change, not to be backported.

> @@ -703,7 +696,38 @@ 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 = pci_get_pdev(NULL,
> +                                                   PCI_SBDF(seg,
> +                                                           info->physfn.bus,
> +                                                           
> info->physfn.devfn));
> +            struct pci_dev *vf_pdev;

const?

> +            bool already_added = false;
> +
> +            if ( !pf_pdev )
> +            {
> +                printk(XENLOG_WARNING
> +                       "Attempted to add SR-IOV device VF %pp without PF 
> %pp\n",

I'd omit "device" here.

(These changes alone I'd be happy to do while committing.)

> +                       &pdev->sbdf,
> +                       &PCI_SBDF(seg, info->physfn.bus, info->physfn.devfn));
> +                free_pdev(pseg, pdev);
> +                ret = -ENODEV;
> +                goto out;
> +            }
> +
> +            pdev->info.is_extfn = pf_pdev->info.is_extfn;

There's a comment related to this, partly visible in patch context above.
That comment imo needs moving here. And correcting while moving (it's
inverted imo, or at least worded ambiguously).

> +            pdev->pf_pdev = pf_pdev;
> +            list_for_each_entry(vf_pdev, &pf_pdev->vf_list, vf_list)
> +            {
> +                if ( vf_pdev == pdev )
> +                {
> +                    already_added = true;
> +                    break;
> +                }
> +            }
> +            if ( !already_added )
> +                list_add(&pdev->vf_list, &pf_pdev->vf_list);
> +        }
>      }

Personally, as I have a dislike for excess variables, I'd have gotten away
without "already_added". Instead of setting it to true, vf_pdev could be
set to NULL. Others may, however, consider this "obfuscation" or alike.

Jan