[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v12.2 01/15] vpci: use per-domain PCI lock to protect vpci structure
On 23.01.2024 16:23, Roger Pau Monné wrote: > On Tue, Jan 23, 2024 at 03:26:26PM +0100, Jan Beulich wrote: >> On 15.01.2024 20:43, Stewart Hildebrand wrote: >>> --- a/xen/arch/x86/hvm/vmsi.c >>> +++ b/xen/arch/x86/hvm/vmsi.c >>> @@ -468,7 +468,7 @@ int msixtbl_pt_register(struct domain *d, struct pirq >>> *pirq, uint64_t gtable) >>> struct msixtbl_entry *entry, *new_entry; >>> int r = -EINVAL; >>> >>> - ASSERT(pcidevs_locked()); >>> + ASSERT(pcidevs_locked() || rw_is_locked(&d->pci_lock)); >>> ASSERT(rw_is_write_locked(&d->event_lock)); >>> >>> if ( !msixtbl_initialised(d) ) >>> @@ -538,7 +538,7 @@ void msixtbl_pt_unregister(struct domain *d, struct >>> pirq *pirq) >>> struct pci_dev *pdev; >>> struct msixtbl_entry *entry; >>> >>> - ASSERT(pcidevs_locked()); >>> + ASSERT(pcidevs_locked() || rw_is_locked(&d->pci_lock)); >>> ASSERT(rw_is_write_locked(&d->event_lock)); >> >> I was hoping to just ack this patch, but the two changes above look >> questionable to me: How can it be that holding _either_ lock is okay? >> It's not obvious in this context that consumers have to hold both >> locks now. In fact consumers looks to be the callers of >> msixtbl_find_entry(), yet the list is RCU-protected. Whereas races >> against themselves or against one another are avoided by holding >> d->event_lock. > > The reason for the change here is that msixtbl_pt_{un,}register() gets > called by pt_irq_{create,destroy}_bind(), which is in turn called by > vPCI code (pcidevs_locked()) that has been switched to not take the > pcidevs lock anymore, and hence the ASSERT would trigger. I understand this is the motivation for the change, but that doesn't (alone) render the construct above sensible / correct. >> My only guess then for the original need of holding pcidevs_lock is >> the use of msi_desc->dev, with the desire for the device to not go >> away. Yet the description doesn't talk about interactions of the per- >> domain PCI lock with that one at all; it all circles around the >> domain'd vPCI lock. > > I do agree that it looks like the original intention of holding > pcidevs_lock is to prevent msi_desc->dev from being removed - yet I'm > not sure it's possible for the device to go away while the domain > event_lock is hold, as device removal would need to take that same > lock in order to destroy the irq_desc. Yes, that matches an observation of mine as well. If we can simplify (rather then complicate) locking, I'd prefer if we did. May need to be a separate (prereq) patch, though. >> Feels like I'm missing something that's obvious to everyone else. >> Or maybe this part of the patch is actually unrelated, and should be >> split off (with its own [proper] justification)? Or wouldn't it then >> be better to also change the other paths leading here to acquire the >> per-domain PCI lock? > > Other paths in vPCI vpci_msi_update(), vpci_msi_arch_update(), > vpci_msi_arch_enable()... are switched in this patch to use the > per-domain pci_lock instead of pcidevs lock. Hence my question: Can't we consolidate to all paths only using the per-domain lock? That would make these odd-looking assertions become normal-looking again. Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |