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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.11.2021 10:32, Oleksandr Andrushchenko wrote:
> 
> 
> 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

First of all I'm sorry for having lost track of that particular case in
the course of the discussion.

I wonder though whether that's something we really need to take care of.
At boot (on x86) modify_bars() wouldn't call defer_map() anyway, but
use apply_map() instead. I wonder whether this wouldn't be appropriate
generally in the context of init_bars() when used for Dom0 (not sure
whether init_bars() would find some form of use for DomU-s as well).
This is even more so as it would better be the exception that devices
discovered post-boot start out with memory decoding enabled (which is a
prereq for modify_bars() to be called in the first place).

Jan