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.hildebr...@amd.com>
I'm okay with this, so in principle
Reviewed-by: Jan Beulich <jbeul...@suse.com>
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
