Re: [PATCH v3 2/6] xen: pci: introduce reference counting for pdev

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

On Fri, Apr 21, 2023 at 11:00:23AM +0000, Volodymyr Babchuk wrote:
> 
> Hello Roger,
> 
> Roger Pau Monné <roger.pau@xxxxxxxxxx> writes:
> 
> > On Mon, Apr 17, 2023 at 12:34:31PM +0200, Jan Beulich wrote:
> >> On 17.04.2023 12:17, Roger Pau Monné wrote:
> >> > On Fri, Apr 14, 2023 at 01:30:39AM +0000, Volodymyr Babchuk wrote:
> >> >> Above I have proposed another view on this. I hope, it will work for
> >> >> you. Just to reiterate, idea is to allow "harmless" refcounts to be left
> >> >> after returning from pci_remove_device(). By "harmless" I mean that
> >> >> owners of those refcounts will not try to access the physical PCI
> >> >> device if pci_remove_device() is already finished.
> >> > 
> >> > I'm not strictly a maintainer of this piece code, albeit I have an
> >> > opinion.  I will like to also hear Jans opinion, since he is the
> >> > maintainer.
> >> 
> >> I'm afraid I can't really appreciate the term "harmless refcounts". Whoever
> >> holds a ref is entitled to access the device. As stated before, I see only
> >> two ways of getting things consistent: Either pci_remove_device() is
> >> invoked upon dropping of the last ref,
> >
> > With this approach, what would be the implementation of
> > PHYSDEVOP_manage_pci_remove?  Would it just check whether the pdev
> > exist and either return 0 or -EBUSY?
> >
> 
> Okay, I am preparing patches with the behavior you proposed. To test it,
> I artificially set refcount to 2 and indeed PHYSDEVOP_manage_pci_remove
> returned -EBUSY, which propagated to the linux driver. Problem is that
> Linux driver can't do anything with this. It just displayed an error
> message and removed device anyways. This is because Linux sends
> PHYSDEVOP_manage_pci_remove in device_remove() call path and there is no
> way to prevent the device removal. So, admin is not capable to try this
> again.

Ideally Linux won't remove the device, and then the admin would get to
retry.  Maybe the way the Linux hook is placed is not the best one?
The hypervisor should be authoritative on whether a device can be
removed or not, and hence PHYSDEVOP_manage_pci_remove returning an
error (EBUSY or otherwise) shouldn't allow the device unplug in Linux
to continue.

We could add a PHYSDEVOP_manage_pci_test or similar that could be
programmatically used to check whether a device has a matching pdev in
the hypervisor, but I have no idea how that could be used by Linux so
it's exposed to the user, and it seems to just make the interface more
complicated for noo real benefit, when the same could be accomplished
by PHYSDEVOP_manage_pci_remove.

Maybe the only feasible solution is for pci_remove_device() to drop a
reference expecting it would be the last one, and print a warning
message if it's not and return -EBUSY.  Expecting any remaining
references to be dropped and the backing pdev to be freed.

Thanks, Roger.