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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan

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