Re: vpci: Need for vpci_cancel_pending

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Oct 28, 2021 at 10:25:28AM +0000, Oleksandr Andrushchenko wrote:
> 
> 
> On 28.10.21 13:17, Roger Pau Monné wrote:
> > On Thu, Oct 28, 2021 at 10:04:20AM +0000, Oleksandr Andrushchenko wrote:
> >> Hi, all!
> >>
> >> While working on PCI passthrough on Arm I stepped onto a crash
> >> with the following call chain:
> >>
> >> 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)
> >>
> >> Then:
> >> leave_hypervisor_to_guest
> >>     vpci_process_pending: v->vpci.mem != NULL; v->vpci.pdev->vpci == NULL
> >>
> >> Which results in the crash below:
> >>
> >> (XEN) Data Abort Trap. Syndrome=0x6
> >> (XEN) Walking Hypervisor VA 0x10 on CPU0 via TTBR 0x00000000481dd000
> >> (XEN) 0TH[0x0] = 0x00000000481dcf7f
> >> (XEN) 1ST[0x0] = 0x00000000481d9f7f
> >> (XEN) 2ND[0x0] = 0x0000000000000000
> >> (XEN) CPU0: Unexpected Trap: Data Abort
> >> ...
> >> (XEN) Xen call trace:
> >> (XEN)    [<00000000002246d8>] _spin_lock+0x40/0xa4 (PC)
> >> (XEN)    [<00000000002246c0>] _spin_lock+0x28/0xa4 (LR)
> >> (XEN)    [<000000000024f6d0>] vpci_process_pending+0x78/0x128
> >> (XEN)    [<000000000027f7e8>] leave_hypervisor_to_guest+0x50/0xcc
> >> (XEN)    [<0000000000269c5c>] entry.o#guest_sync_slowpath+0xa8/0xd4
> >>
> >> So, it seems that if pci_add_device fails and calls vpci_remove_device
> >> the later needs to cancel any pending work.
> > Indeed, you will need to check that v->vpci.pdev == pdev before
> > canceling the pending work though, or else you could be canceling
> > pending work from a different device.
> How about:
> 
> void vpci_cancel_pending(struct pci_dev *pdev)
> {
>      struct vcpu *v = current;
> 
>      if ( v->vpci.mem && v->vpci.pdev == pdev)
>      {
>          rangeset_destroy(v->vpci.mem);
>          v->vpci.mem = NULL;
>      }
> }
> 
> This will effectively prevent the pending work from running

Can't you just place this in vpci_remove_device?

Or is there a need to cancel pending work without removing the device?

Thanks, Roger.