On 11/4/24 02:44, Jan Beulich wrote: > On 01.11.2024 21:16, Stewart Hildebrand wrote: >> +Daniel (XSM mention) >> >> On 10/28/24 13:02, Jan Beulich wrote: >>> On 18.10.2024 22:39, 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. >>>> >>>> The hardware domain is expected to remove the associated VFs before >>>> removing the PF. Print a warning in case a PF is removed with associated >>>> VFs still present. >>>> >>>> Signed-off-by: Stewart Hildebrand <[email protected]> >>>> --- >>>> Candidate for backport to 4.19 (the next patch depends on this one) >>>> >>>> v5->v6: >>>> * move printk() before ASSERT_UNREACHABLE() >>>> * warn about PF removal with VFs still present >>> >>> Hmm, maybe I didn't make this clear enough when commenting on v5: I wasn't >>> just after an adjustment to the commit message. I'm instead actively >>> concerned of the resulting behavior. Question is whether we can reasonably >>> do something about that. >> >> Right. My suggestion then is to go back to roughly how it was done in >> v4 [0]: >> >> * Remove the VFs right away during PF removal, so that we don't end up >> with stale VFs. Regarding XSM, assume that a domain with permission to >> remove the PF is also allowed to remove the VFs. We should probably also >> return an error from pci_remove_device in the case of removing the PF >> with VFs still present (and still perform the removals despite returning >> an error). Subsequent attempts by a domain to remove the VFs would >> return an error (as they have already been removed), but that's expected >> since we've taken a stance that PF-then-VF removal order is invalid >> anyway. > > Imo going back is not an option. > >> While the above is what I prefer, I just want to mention other options I >> considered for the scenario of PF removal with VFs still present: >> >> * Increase the "scariness" of the warning message added in v6. >> >> * Return an error from pci_remove_device (while still removing only the >> PF). We would be left with stale VFs in Xen. At least this would >> concretely inform dom0 that Xen takes issue with the PF-then-VF removal >> order. Subsequent attempts by a domain to remove VFs, however >> (un)likely, would succeed. > > Returning an error in such a case is a possibility, but comes with the > risk of confusion. Seeing such an error, a caller may itself assume the > device still is there, and retry its (with or without having removed the > VFs) removal at a later point. > >> * Return an error from pci_remove_device and keep the PF and VFs. This >> is IMO the worst option because then we would have a stale PF in >> addition to stale VFs. > > Yet this would at least be self-consistent, unlike the variant above. No > matter what, any failure to remove VFs and/or PFs correctly will need to > result in there being no attempt to physically remove the device. > > You didn't enumerate an option lightly mentioned before, perhaps because > of its anticipated intrusiveness: Re-associate stale VFs with their PF, > once the PF is re-reported.
I'll plan on this approach for v7. > Problem of course is that, aiui, the VFs > could in principle re-appear at a different BDF (albeit we have other > issues with potential bus-renumbering done by Dom0), and their count > could also change. > > Jan > >> [0] >> https://lore.kernel.org/xen-devel/[email protected]/T/#t >
