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

[Julien Grall <julien@xxxxxxx>]

Hi Oleksandr,

On 16/11/2021 14:24, Oleksandr Andrushchenko wrote:
> On 16.11.21 16:12, Jan Beulich wrote:
> > On 16.11.2021 14:41, Oleksandr Andrushchenko wrote:
> > > On 16.11.21 10:23, Oleksandr Andrushchenko wrote:
> > > > On 16.11.21 10:01, Jan Beulich wrote:
> > > > > On 16.11.2021 08:32, Oleksandr Andrushchenko wrote:
> > > > > > On 15.11.21 18:56, 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
> > > > > > > Is continuing safe in this case? I.e. isn't there the risk of a NULL
> > > > > > > deref?
> > > > > > I think it is safe to continue
> > > > > And why do you think so? I.e. why is there no race for Dom0 when there
> > > > > is one for DomU?
> > > > Well, then we need to use a lock to synchronize the two.
> > > > I guess this needs to be pci devs lock unfortunately
> > > The parties involved in deferred work and its cancellation:
> > >
> > > MMIO trap -> vpci_write -> vpci_cmd_write -> modify_bars -> defer_map
> > >
> > > Arm: leave_hypervisor_to_guest -> check_for_vcpu_work -> vpci_process_pending
> > >
> > > x86: two places -> hvm_do_resume -> vpci_process_pending
> > >
> > > So, both defer_map and vpci_process_pending need to be synchronized with
> > > pcidevs_{lock|unlock).
> > If I was an Arm maintainer, I'm afraid I would object to the pcidevs lock
> > getting used in leave_hypervisor_to_guest.
> I do agree this is really not good, but it seems I am limited in choices.
> @Stefano, @Julien, do you see any better way of doing that?
I agree with Jan about using the pcidevs_{lock|unlock}. The lock is not 
fine-grained enough for been call in vpci_process_pending().
I haven't yet looked at the rest of the series to be able to suggest the 
exact lock. But we at least want a per-domain spinlock.
> We were thinking about introducing a dedicated lock for vpci [1],
> but finally decided to use pcidevs_lock for now
Skimming through the thread, you decided to use pcidevs_lock because it 
was simpler and sufficient for the use case discussed back then. Now, we 
have a use case where it would be a problem to use pcidevs_lock. So I 
think the extra complexity is justified.
Cheers,

--
Julien Grall