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

["Roger Pau Monné" <roger.pau@citrix.com>]

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 <stewart.hildebrand@amd.com>
> >>>>> ---
> >>>>> 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.

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.

Thanks. Roger.