On 11/8/24 10:19, Roger Pau Monné wrote: > On Fri, Nov 08, 2024 at 02:17:40PM +0100, Jan Beulich wrote: >> On 08.11.2024 13:42, Alejandro Vallejo wrote: >>> On Mon Nov 4, 2024 at 7:44 AM GMT, 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. > > I'm thinking probably this is the least bad option, and just force the > owner of the PF to ensure there are no VFs left when removing the PF. > > What sense does it make anyway to allow removing a PF with VFs still > present? Not sure exactly what the owner of the PF will do before > calling pci_remove_device(), but it would seem to me the device should > be ready for unplug (so SR-IOV disabled). Calling pci_remove_device() > with VFs still active points to an error to do proper cleanup by the > owner of the PF.
In normal, correct operation, right. The PF driver is indeed expected to disable SR-IOV (i.e. remove VFs) during its removal, prior to calling PHYSDEVOP_pci_device_remove for the PF. > Returning error from pci_remove_device() and doing nothing would seem > fine to me. There should be no stale PF or VFs in that case, as the > caller has been notified the device has failed to be removed, so > should treat the device as still present. But software has no way to guarantee there won't be a physical device removal. In test scenario #2 described in the first patch [1], the PF (the whole device, actually) has already been physically unplugged, and dom0 invokes PHYSDEVOP_pci_device_remove to inform Xen about it. [1] https://lore.kernel.org/xen-devel/[email protected]/ That said, test scenario #2 would only happen when a buggy PF driver failed to properly clean up the VFs before the PF. But the point is that returning an error does not guarantee there won't be a stale pdev in case of a buggy dom0. I guess as long as we trust the owner of the PF, this approach is fine.
