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

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

Hi Jan,

Jan Beulich <jbeulich@suse.com> writes:

> On 21.04.2023 13:00, Volodymyr Babchuk wrote:
>> 
>> Hello Roger,
>> 
>> Roger Pau Monné <roger.pau@citrix.com> 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.
>
> So maybe Linux'es issuing of the call needs moving elsewhere? Or we need
> a new sub-op, such that PHYSDEVOP_manage_pci_remove can remain purely a
> last-moment notification?

From Linux point of view, it already cleaned up all the device resources
and it is ready to hot-unplug the device. Xen PCI driver in Linux just
gets a notification that device is being removed.

BTW, xen_pciback (AKA pci_stub) driver in Linux tracks that device is
assigned to another domain, but all it can do is to loudly complain in
kernel log if device is being removed without being deassigned from
another domain.

>
>> As I workaround, I can create hypercall continuation in case if
>> pci_remove_device() returns -EBUSY. What is your opinion?
>
> How would that help? You'd then spin perhaps for hours or days ...

Are you implying the case when we increase refcounter when we assign a
PCI device to a domain? In this case yes, it is quite possible that we
will spin there for any arbitrary amount of time...

-- 
WBR, Volodymyr