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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@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?

I was meaning to suggest that it should at least be considered.

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

My main point is that the other suggested options have weaknesses, too.
Leaving stale VFs around forever isn't, imo, any better than trying to
reuse their structs.

Jan