Re: [PATCH v8 02/13] vpci: use per-domain PCI lock to protect vpci structure

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

Hi Roger,

Roger Pau Monné <roger.pau@citrix.com> writes:

> On Wed, Jul 26, 2023 at 01:17:58AM +0000, Volodymyr Babchuk wrote:
>> 
>> Hi Roger,
>> 
>> Roger Pau Monné <roger.pau@citrix.com> writes:
>> 
>> > On Thu, Jul 20, 2023 at 12:32:31AM +0000, Volodymyr Babchuk wrote:
>> >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>> >> @@ -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