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
