Re: [PATCH v4 02/11] vpci: cancel pending map/unmap on vpci removal

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 18.11.21 11:15, Jan Beulich wrote:
> On 18.11.2021 09:54, Oleksandr Andrushchenko wrote:
>> On 18.11.21 10:36, Jan Beulich wrote:
>>> On 18.11.2021 08:49, Oleksandr Andrushchenko wrote:
>>>> On 17.11.21 10:28, Jan Beulich wrote:
>>>>> On 05.11.2021 07:56, Oleksandr Andrushchenko wrote:
>>>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>>>>>
>>>>>> When a vPCI is removed for a PCI device it is possible that we have
>>>>>> scheduled a delayed work for map/unmap operations for that device.
>>>>>> For example, the following scenario can illustrate the problem:
>>>>>>
>>>>>> pci_physdev_op
>>>>>>       pci_add_device
>>>>>>           init_bars -> modify_bars -> defer_map -> 
>>>>>> raise_softirq(SCHEDULE_SOFTIRQ)
>>>>>>       iommu_add_device <- FAILS
>>>>>>       vpci_remove_device -> xfree(pdev->vpci)
>>>>>>
>>>>>> leave_hypervisor_to_guest
>>>>>>       vpci_process_pending: v->vpci.mem != NULL; v->vpci.pdev->vpci == 
>>>>>> NULL
>>>>>>
>>>>>> For the hardware domain we continue execution as the worse that
>>>>>> could happen is that MMIO mappings are left in place when the
>>>>>> device has been deassigned
>>>>>>
>>>>>> For unprivileged domains that get a failure in the middle of a vPCI
>>>>>> {un}map operation we need to destroy them, as we don't know in which
>>>>>> state the p2m is. This can only happen in vpci_process_pending for
>>>>>> DomUs as they won't be allowed to call pci_add_device.
>>>>>>
>>>>>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>>>> Thinking about it some more, I'm not convinced any of this is really
>>>>> needed in the presented form.
>>>> The intention of this patch was to handle error conditions which are
>>>> abnormal, e.g. when iommu_add_device fails and we are in the middle
>>>> of initialization. So, I am trying to cancel all pending work which might
>>>> already be there and not to crash.
>>> Only Dom0 may be able to prematurely access the device during "add".
>>> Yet unlike for DomU-s we generally expect Dom0 to be well-behaved.
>>> Hence I'm not sure I see the need for dealing with these.
>> Probably I don't follow you here. The issue I am facing is Dom0
>> related, e.g. Xen was not able to initialize during "add" and thus
>> wanted to clean up the leftovers. As the result the already
>> scheduled work crashes as it was not neither canceled nor interrupted
>> in some safe manner. So, this sounds like something we need to take
>> care of, thus this patch.
> But my point was the question of why there would be any pending work
> in the first place in this case, when we expect Dom0 to be well-behaved.
I am not saying Dom0 misbehaves here. This is my real use-case
(as in the commit message):

pci_physdev_op
      pci_add_device
          init_bars -> modify_bars -> defer_map -> 
raise_softirq(SCHEDULE_SOFTIRQ)
      iommu_add_device <- FAILS
      vpci_remove_device -> xfree(pdev->vpci)

leave_hypervisor_to_guest
      vpci_process_pending: v->vpci.mem != NULL; v->vpci.pdev->vpci == NULL

So, this made me implement the patch. Then we decided that it is also
possible that other vCPUs may also have some pending work and I
agreed that this is a good point and we want to remove all pending work
for all vCPUs.

So, if you doubt the patch and we still have the scenario above, what
would you suggest in order to make sure we do not crash?
> Jan
>
Thank you,
Oleksandr