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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.04.2023 16:13, Volodymyr Babchuk wrote:
> 
> 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.

Could pciback obtain a reference on behalf of the hypervisor, dropping it
when device removal is requested (i.e. much closer to the start of that
operation), and only if it finds that no guests use the device anymore?

Jan