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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Nov 11, 2024 at 07:40:14AM +0100, Jan Beulich wrote:
> 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.

That would be my expectation also, albeit IIRC we had other instances
where the calling of the removal hypercall was not done in a vetoing
manner by Linux, so it was mostly a notification that the device was
going away, rather than a request for permission to removal.

Thanks, Roger.