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

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

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@xxxxxxx>
> >>> ---
> >>> 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. 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.

Are you enumerating it for completeness or suggesting it should be done?

Maybe I'm missing something here (and please, do tell me what if so), but why
would this option be desirable at all? What would benefit from such semantics
(as opposed to any of the others)? It would break the lifetime dependency
between PF and VFs, and that doesn't strike me as a feature. It also turns
kernel bugs into a fine implementations by making promises about how state is
persisted, but the consequences of that appear to be too far reaching to know
for sure it's 100% ok.

>From afar, it sounds like trying to turn a bug into a feature. And that cannot
always be done sanely. But again, maybe I might very well be missing
something...

>
> Jan
>
> > [0] 
> > https://lore.kernel.org/xen-devel/20240827035929.118003-1-stewart.hildebrand@xxxxxxx/T/#t

Cheers,
Alejandro