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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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

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/20240827035929.118003-1-stewart.hildebrand@xxxxxxx/T/#t
>