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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.11.2024 17:29, Stewart Hildebrand wrote:
> On 11/8/24 10:19, Roger Pau Monné wrote:
>> 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@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.
>>
>> 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.
> 
> In normal, correct operation, right. The PF driver is indeed expected to
> disable SR-IOV (i.e. remove VFs) during its removal, prior to calling
> PHYSDEVOP_pci_device_remove for 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.

Imo really that's another case that would best be addressed by proper
ref-counting. Each VF would hold a ref to its PF, and hence the PF would
go away when the last VF is removed, or when PF removal is (properly)
last. Just that this likely is too complex a change to be warranted for
the purpose here.

> But software has no way to guarantee there won't be a physical device
> removal.
> 
> In test scenario #2 described in the first patch [1], the PF (the whole
> device, actually) has already been physically unplugged, and dom0
> invokes PHYSDEVOP_pci_device_remove to inform Xen about it.

I don't think that's how it's supposed to work. Physical removal should
occur only after software has done all "soft removal". I'd view the
notification to Xen as part of that.

Jan

> [1] 
> https://lore.kernel.org/xen-devel/20241018203913.1162962-2-stewart.hildebrand@xxxxxxx/
> 
> That said, test scenario #2 would only happen when a buggy PF driver
> failed to properly clean up the VFs before the PF. But the point is that
> returning an error does not guarantee there won't be a stale pdev in
> case of a buggy dom0.
> 
> I guess as long as we trust the owner of the PF, this approach is fine.