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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Roger,

Roger Pau Monné <roger.pau@xxxxxxxxxx> writes:

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

Yes, it would be ideally, but Linux driver/device model is written in a
such way, that PCI subsystem tracks all the PCI device usage, so it can
be certain that it can remove the device. Thus, functions in the device
removal path either return void or 0. Of course, kernel does not know that
hypervisor has additional uses for the device, so there is no mechanisms
to prevent removal.

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

We can ignore the kernel behavior and just call
PHYSDEVOP_manage_pci_remove from toolstack. Something like "xl
pci-hotunplug SBFD". But yes, this will make interface more complicated.

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

So, basically in the same way as I proposed initially?

-- 
WBR, Volodymyr