Hi Roger, Roger Pau Monné <[email protected]> writes:
> On Wed, Jul 26, 2023 at 01:17:58AM +0000, Volodymyr Babchuk wrote: >> >> Hi Roger, >> >> Roger Pau Monné <[email protected]> writes: >> >> > On Thu, Jul 20, 2023 at 12:32:31AM +0000, Volodymyr Babchuk wrote: >> >> From: Oleksandr Andrushchenko <[email protected]> >> >> @@ -498,6 +537,7 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, >> >> unsigned int size, >> >> ASSERT(data_offset < size); >> >> } >> >> spin_unlock(&pdev->vpci->lock); >> >> + unlock_locks(d); >> > >> > There's one issue here, some handlers will cal pcidevs_lock(), which >> > will result in a lock over inversion, as in the previous patch we >> > agreed that the locking order was pcidevs_lock first, d->pci_lock >> > after. >> > >> > For example the MSI control_write() handler will call >> > vpci_msi_arch_enable() which takes the pcidevs lock. I think I will >> > have to look into using a dedicated lock for MSI related handling, as >> > that's the only place where I think we have this pattern of taking the >> > pcidevs_lock after the d->pci_lock. >> >> I'll mention this in the commit message. Is there something else that I >> should do right now? > > Well, I don't think we want to commit this as-is with a known lock > inversion. > > The functions that require the pcidevs lock are: > > pt_irq_{create,destroy}_bind() > unmap_domain_pirq() > > AFAICT those functions require the lock in order to assert that the > underlying device doesn't go away, as they do also use d->event_lock > in order to get exclusive access to the data fields. Please double > check that I'm not mistaken. You are right, all three function does not access any of PCI state directly. However... > If that's accurate you will have to check the call tree that spawns > from those functions in order to modify the asserts to check for > either the pcidevs or the per-domain pci_list lock being taken. ... I checked calls for PT_IRQ_TYPE_MSI case, there is only call that bothers me: hvm_pi_update_irte(), which calls IO-MMU code via vmx_pi_update_irte(): amd_iommu_msi_msg_update_ire() or msi_msg_write_remap_rte(). Both functions read basic pdev fields like sbfd or type. I see no problem there, as values of those fields are not supposed to be changed. Also those function use own locks to protect shared state. But as IO-MMU code is quite convoluted it is hard to be sure that it is safe to call those functions without holding pdevs_lock. All I can say is that those functions and their callees have no ASSERT(pcidevs_locked()). -- WBR, Volodymyr
