| Message ID | 20220831141040.13231-8-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rework PCI locking | expand |
On Wed, 31 Aug 2022, Volodymyr Babchuk wrote: > Spinlock in struct pci_device will be used to protect access to device > itself. Right now it is used mostly by MSI code. > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> There are 2 instances of: BUG_ON(list_empty(&dev->msi_list)); in xen/arch/x86/msi.c:__pci_disable_msi and xen/arch/x86/msi.c:__pci_disable_msix which are not protected by pcidev_lock. However list_empty needs to be protected. (pci_disable_msi can also be called from xen/arch/x86/irq.c where it is not surrounded by pcidev_lock.) Given that they are BUG_ON, I wonder if we could remove them instead of adding locks there. It would make things simpler. > --- > xen/arch/x86/hvm/vmsi.c | 6 +++++- > xen/arch/x86/msi.c | 16 ++++++++++++++++ > xen/drivers/passthrough/msi.c | 8 +++++++- > xen/drivers/passthrough/pci.c | 2 ++ > xen/include/xen/pci.h | 12 ++++++++++++ > 5 files changed, 42 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c > index 7fb1075673..c9e5f279c5 100644 > --- a/xen/arch/x86/hvm/vmsi.c > +++ b/xen/arch/x86/hvm/vmsi.c > @@ -203,10 +203,14 @@ static struct msi_desc *msixtbl_addr_to_desc( > > nr_entry = (addr - entry->gtable) / PCI_MSIX_ENTRY_SIZE; > > + pcidev_lock(entry->pdev); > list_for_each_entry( desc, &entry->pdev->msi_list, list ) > if ( desc->msi_attrib.type == PCI_CAP_ID_MSIX && > - desc->msi_attrib.entry_nr == nr_entry ) > + desc->msi_attrib.entry_nr == nr_entry ) { > + pcidev_unlock(entry->pdev); code style > return desc; > + } > + pcidev_unlock(entry->pdev); > > return NULL; > } > diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c > index bccaccb98b..6b62c4f452 100644 > --- a/xen/arch/x86/msi.c > +++ b/xen/arch/x86/msi.c > @@ -389,6 +389,7 @@ static bool msi_set_mask_bit(struct irq_desc *desc, bool host, bool guest) > default: > return 0; > } > + spurious change > entry->msi_attrib.host_masked = host; > entry->msi_attrib.guest_masked = guest; > > @@ -585,12 +586,17 @@ static struct msi_desc *find_msi_entry(struct pci_dev *dev, > { > struct msi_desc *entry; > > + pcidev_lock(dev); > list_for_each_entry( entry, &dev->msi_list, list ) > { > if ( entry->msi_attrib.type == cap_id && > (irq == -1 || entry->irq == irq) ) > + { > + pcidev_unlock(dev); > return entry; > + } > } > + pcidev_unlock(dev); > > return NULL; > } > @@ -661,7 +667,9 @@ static int msi_capability_init(struct pci_dev *dev, > maskbits |= ~(uint32_t)0 >> (32 - dev->msi_maxvec); > pci_conf_write32(dev->sbdf, mpos, maskbits); > } > + pcidev_lock(dev); > list_add_tail(&entry->list, &dev->msi_list); > + pcidev_unlock(dev); > > *desc = entry; > /* Restore the original MSI enabled bits */ > @@ -946,7 +954,9 @@ static int msix_capability_init(struct pci_dev *dev, > > pcidev_get(dev); > > + pcidev_lock(dev); > list_add_tail(&entry->list, &dev->msi_list); > + pcidev_unlock(dev); > *desc = entry; > } > > @@ -1231,11 +1241,13 @@ static void msi_free_irqs(struct pci_dev* dev) > { > struct msi_desc *entry, *tmp; > > + pcidev_lock(dev); > list_for_each_entry_safe( entry, tmp, &dev->msi_list, list ) > { > pci_disable_msi(entry); > msi_free_irq(entry); > } > + pcidev_unlock(dev); > } > > void pci_cleanup_msi(struct pci_dev *pdev) > @@ -1354,6 +1366,7 @@ int pci_restore_msi_state(struct pci_dev *pdev) > if ( ret ) > return ret; > > + pcidev_lock(pdev); > list_for_each_entry_safe( entry, tmp, &pdev->msi_list, list ) > { > unsigned int i = 0, nr = 1; > @@ -1371,6 +1384,7 @@ int pci_restore_msi_state(struct pci_dev *pdev) > dprintk(XENLOG_ERR, "Restore MSI for %pp entry %u not set?\n", > &pdev->sbdf, i); > spin_unlock_irqrestore(&desc->lock, flags); > + pcidev_unlock(pdev); > if ( type == PCI_CAP_ID_MSIX ) > pci_conf_write16(pdev->sbdf, msix_control_reg(pos), > control & ~PCI_MSIX_FLAGS_ENABLE); > @@ -1393,6 +1407,7 @@ int pci_restore_msi_state(struct pci_dev *pdev) > if ( unlikely(!memory_decoded(pdev)) ) > { > spin_unlock_irqrestore(&desc->lock, flags); > + pcidev_unlock(pdev); > pci_conf_write16(pdev->sbdf, msix_control_reg(pos), > control & ~PCI_MSIX_FLAGS_ENABLE); > return -ENXIO; > @@ -1438,6 +1453,7 @@ int pci_restore_msi_state(struct pci_dev *pdev) > pci_conf_write16(pdev->sbdf, msix_control_reg(pos), > control | PCI_MSIX_FLAGS_ENABLE); > > + pcidev_unlock(pdev); > return 0; > } > > diff --git a/xen/drivers/passthrough/msi.c b/xen/drivers/passthrough/msi.c > index ce1a450f6f..98f4d2721a 100644 > --- a/xen/drivers/passthrough/msi.c > +++ b/xen/drivers/passthrough/msi.c > @@ -22,6 +22,7 @@ int pdev_msi_init(struct pci_dev *pdev) > { > unsigned int pos; > > + pcidev_lock(pdev); > INIT_LIST_HEAD(&pdev->msi_list); > > pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn), > @@ -41,7 +42,10 @@ int pdev_msi_init(struct pci_dev *pdev) > uint16_t ctrl; > > if ( !msix ) > - return -ENOMEM; > + { > + pcidev_unlock(pdev); > + return -ENOMEM; > + } > > spin_lock_init(&msix->table_lock); > > @@ -51,6 +55,8 @@ int pdev_msi_init(struct pci_dev *pdev) > pdev->msix = msix; > } > > + pcidev_unlock(pdev); > + > return 0; > } > > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > index c8da80b981..c83397211b 100644 > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -1383,7 +1383,9 @@ static int cf_check _dump_pci_devices(struct pci_seg *pseg, void *arg) > printk("%pd", pdev->domain); > printk(" - node %-3d refcnt %d", (pdev->node != NUMA_NO_NODE) ? pdev->node : -1, > atomic_read(&pdev->refcnt)); > + pcidev_lock(pdev); > pdev_dump_msi(pdev); > + pcidev_unlock(pdev); > printk("\n"); > } > spin_unlock(&pseg->alldevs_lock); > diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h > index e71a180ef3..d0a7339d84 100644 > --- a/xen/include/xen/pci.h > +++ b/xen/include/xen/pci.h > @@ -106,6 +106,8 @@ struct pci_dev { > uint8_t msi_maxvec; > uint8_t phantom_stride; > > + /* Device lock */ > + spinlock_t lock; > nodeid_t node; /* NUMA node */ > > /* Device to be quarantined, don't automatically re-assign to dom0 */ > @@ -235,6 +237,16 @@ int msixtbl_pt_register(struct domain *, struct pirq *, uint64_t gtable); > void msixtbl_pt_unregister(struct domain *, struct pirq *); > void msixtbl_pt_cleanup(struct domain *d); > > +static inline void pcidev_lock(struct pci_dev *pdev) > +{ > + spin_lock(&pdev->lock); > +} > + > +static inline void pcidev_unlock(struct pci_dev *pdev) > +{ > + spin_unlock(&pdev->lock); > +} > + > #ifdef CONFIG_HVM > int arch_pci_clean_pirqs(struct domain *d); > #else > -- > 2.36.1 >
Hi Stefano, Stefano Stabellini <sstabellini@kernel.org> writes: > On Wed, 31 Aug 2022, Volodymyr Babchuk wrote: >> Spinlock in struct pci_device will be used to protect access to device >> itself. Right now it is used mostly by MSI code. >> >> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > > There are 2 instances of: > > BUG_ON(list_empty(&dev->msi_list)); > > in xen/arch/x86/msi.c:__pci_disable_msi and > xen/arch/x86/msi.c:__pci_disable_msix which are not protected by > pcidev_lock. However list_empty needs to be protected. (pci_disable_msi > can also be called from xen/arch/x86/irq.c where it is not surrounded by > pcidev_lock.) I checked all call paths. pci_disable_msi() is called from three places in xen/arch/x86/irq.c. As I can see, all three are "protected" with ASSERT(pcidevs_locked()), or am I missing something? > > Given that they are BUG_ON, I wonder if we could remove them instead of > adding locks there. It would make things simpler. Well, I will be happy to remove them, if there are no objections. > > >> --- >> xen/arch/x86/hvm/vmsi.c | 6 +++++- >> xen/arch/x86/msi.c | 16 ++++++++++++++++ >> xen/drivers/passthrough/msi.c | 8 +++++++- >> xen/drivers/passthrough/pci.c | 2 ++ >> xen/include/xen/pci.h | 12 ++++++++++++ >> 5 files changed, 42 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c >> index 7fb1075673..c9e5f279c5 100644 >> --- a/xen/arch/x86/hvm/vmsi.c >> +++ b/xen/arch/x86/hvm/vmsi.c >> @@ -203,10 +203,14 @@ static struct msi_desc *msixtbl_addr_to_desc( >> >> nr_entry = (addr - entry->gtable) / PCI_MSIX_ENTRY_SIZE; >> >> + pcidev_lock(entry->pdev); >> list_for_each_entry( desc, &entry->pdev->msi_list, list ) >> if ( desc->msi_attrib.type == PCI_CAP_ID_MSIX && >> - desc->msi_attrib.entry_nr == nr_entry ) >> + desc->msi_attrib.entry_nr == nr_entry ) { >> + pcidev_unlock(entry->pdev); > > code style > > >> return desc; >> + } >> + pcidev_unlock(entry->pdev); >> >> return NULL; >> } >> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c >> index bccaccb98b..6b62c4f452 100644 >> --- a/xen/arch/x86/msi.c >> +++ b/xen/arch/x86/msi.c >> @@ -389,6 +389,7 @@ static bool msi_set_mask_bit(struct irq_desc *desc, bool host, bool guest) >> default: >> return 0; >> } >> + > > spurious change > > >> entry->msi_attrib.host_masked = host; >> entry->msi_attrib.guest_masked = guest; >> >> @@ -585,12 +586,17 @@ static struct msi_desc *find_msi_entry(struct pci_dev *dev, >> { >> struct msi_desc *entry; >> >> + pcidev_lock(dev); >> list_for_each_entry( entry, &dev->msi_list, list ) >> { >> if ( entry->msi_attrib.type == cap_id && >> (irq == -1 || entry->irq == irq) ) >> + { >> + pcidev_unlock(dev); >> return entry; >> + } >> } >> + pcidev_unlock(dev); >> >> return NULL; >> } >> @@ -661,7 +667,9 @@ static int msi_capability_init(struct pci_dev *dev, >> maskbits |= ~(uint32_t)0 >> (32 - dev->msi_maxvec); >> pci_conf_write32(dev->sbdf, mpos, maskbits); >> } >> + pcidev_lock(dev); >> list_add_tail(&entry->list, &dev->msi_list); >> + pcidev_unlock(dev); >> >> *desc = entry; >> /* Restore the original MSI enabled bits */ >> @@ -946,7 +954,9 @@ static int msix_capability_init(struct pci_dev *dev, >> >> pcidev_get(dev); >> >> + pcidev_lock(dev); >> list_add_tail(&entry->list, &dev->msi_list); >> + pcidev_unlock(dev); >> *desc = entry; >> } >> >> @@ -1231,11 +1241,13 @@ static void msi_free_irqs(struct pci_dev* dev) >> { >> struct msi_desc *entry, *tmp; >> >> + pcidev_lock(dev); >> list_for_each_entry_safe( entry, tmp, &dev->msi_list, list ) >> { >> pci_disable_msi(entry); >> msi_free_irq(entry); >> } >> + pcidev_unlock(dev); >> } >> >> void pci_cleanup_msi(struct pci_dev *pdev) >> @@ -1354,6 +1366,7 @@ int pci_restore_msi_state(struct pci_dev *pdev) >> if ( ret ) >> return ret; >> >> + pcidev_lock(pdev); >> list_for_each_entry_safe( entry, tmp, &pdev->msi_list, list ) >> { >> unsigned int i = 0, nr = 1; >> @@ -1371,6 +1384,7 @@ int pci_restore_msi_state(struct pci_dev *pdev) >> dprintk(XENLOG_ERR, "Restore MSI for %pp entry %u not set?\n", >> &pdev->sbdf, i); >> spin_unlock_irqrestore(&desc->lock, flags); >> + pcidev_unlock(pdev); >> if ( type == PCI_CAP_ID_MSIX ) >> pci_conf_write16(pdev->sbdf, msix_control_reg(pos), >> control & ~PCI_MSIX_FLAGS_ENABLE); >> @@ -1393,6 +1407,7 @@ int pci_restore_msi_state(struct pci_dev *pdev) >> if ( unlikely(!memory_decoded(pdev)) ) >> { >> spin_unlock_irqrestore(&desc->lock, flags); >> + pcidev_unlock(pdev); >> pci_conf_write16(pdev->sbdf, msix_control_reg(pos), >> control & ~PCI_MSIX_FLAGS_ENABLE); >> return -ENXIO; >> @@ -1438,6 +1453,7 @@ int pci_restore_msi_state(struct pci_dev *pdev) >> pci_conf_write16(pdev->sbdf, msix_control_reg(pos), >> control | PCI_MSIX_FLAGS_ENABLE); >> >> + pcidev_unlock(pdev); >> return 0; >> } >> >> diff --git a/xen/drivers/passthrough/msi.c b/xen/drivers/passthrough/msi.c >> index ce1a450f6f..98f4d2721a 100644 >> --- a/xen/drivers/passthrough/msi.c >> +++ b/xen/drivers/passthrough/msi.c >> @@ -22,6 +22,7 @@ int pdev_msi_init(struct pci_dev *pdev) >> { >> unsigned int pos; >> >> + pcidev_lock(pdev); >> INIT_LIST_HEAD(&pdev->msi_list); >> >> pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn), >> @@ -41,7 +42,10 @@ int pdev_msi_init(struct pci_dev *pdev) >> uint16_t ctrl; >> >> if ( !msix ) >> - return -ENOMEM; >> + { >> + pcidev_unlock(pdev); >> + return -ENOMEM; >> + } >> >> spin_lock_init(&msix->table_lock); >> >> @@ -51,6 +55,8 @@ int pdev_msi_init(struct pci_dev *pdev) >> pdev->msix = msix; >> } >> >> + pcidev_unlock(pdev); >> + >> return 0; >> } >> >> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c >> index c8da80b981..c83397211b 100644 >> --- a/xen/drivers/passthrough/pci.c >> +++ b/xen/drivers/passthrough/pci.c >> @@ -1383,7 +1383,9 @@ static int cf_check _dump_pci_devices(struct pci_seg *pseg, void *arg) >> printk("%pd", pdev->domain); >> printk(" - node %-3d refcnt %d", (pdev->node != NUMA_NO_NODE) ? pdev->node : -1, >> atomic_read(&pdev->refcnt)); >> + pcidev_lock(pdev); >> pdev_dump_msi(pdev); >> + pcidev_unlock(pdev); >> printk("\n"); >> } >> spin_unlock(&pseg->alldevs_lock); >> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h >> index e71a180ef3..d0a7339d84 100644 >> --- a/xen/include/xen/pci.h >> +++ b/xen/include/xen/pci.h >> @@ -106,6 +106,8 @@ struct pci_dev { >> uint8_t msi_maxvec; >> uint8_t phantom_stride; >> >> + /* Device lock */ >> + spinlock_t lock; >> nodeid_t node; /* NUMA node */ >> >> /* Device to be quarantined, don't automatically re-assign to dom0 */ >> @@ -235,6 +237,16 @@ int msixtbl_pt_register(struct domain *, struct pirq *, uint64_t gtable); >> void msixtbl_pt_unregister(struct domain *, struct pirq *); >> void msixtbl_pt_cleanup(struct domain *d); >> >> +static inline void pcidev_lock(struct pci_dev *pdev) >> +{ >> + spin_lock(&pdev->lock); >> +} >> + >> +static inline void pcidev_unlock(struct pci_dev *pdev) >> +{ >> + spin_unlock(&pdev->lock); >> +} >> + >> #ifdef CONFIG_HVM >> int arch_pci_clean_pirqs(struct domain *d); >> #else >> -- >> 2.36.1 >>
On 31.08.2022 16:11, Volodymyr Babchuk wrote: > --- a/xen/arch/x86/hvm/vmsi.c > +++ b/xen/arch/x86/hvm/vmsi.c > @@ -203,10 +203,14 @@ static struct msi_desc *msixtbl_addr_to_desc( > > nr_entry = (addr - entry->gtable) / PCI_MSIX_ENTRY_SIZE; > > + pcidev_lock(entry->pdev); > list_for_each_entry( desc, &entry->pdev->msi_list, list ) > if ( desc->msi_attrib.type == PCI_CAP_ID_MSIX && > - desc->msi_attrib.entry_nr == nr_entry ) > + desc->msi_attrib.entry_nr == nr_entry ) { > + pcidev_unlock(entry->pdev); > return desc; This is a potentially problematic pattern: desc has a backlink to pdev, just like "entry" has. _If_ locking is required here (and the refcounting is insufficient), then it is questionable whether the lock can actually be dropped before returning. The idea with refcounting was, though, that entities holding a reference can be sure the pdev won't go away. But of course there's also the question what "access to device itself" (as stated in the description) does (or does not) constitute. I think it is pretty crucial that for every new lock it is spelled out clearly what it protects. Seeing the list iteration pattern here (and at least once below) another question is whether a lock like the one here may want to be a read/write one. Jan
diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c index 7fb1075673..c9e5f279c5 100644 --- a/xen/arch/x86/hvm/vmsi.c +++ b/xen/arch/x86/hvm/vmsi.c @@ -203,10 +203,14 @@ static struct msi_desc *msixtbl_addr_to_desc( nr_entry = (addr - entry->gtable) / PCI_MSIX_ENTRY_SIZE; + pcidev_lock(entry->pdev); list_for_each_entry( desc, &entry->pdev->msi_list, list ) if ( desc->msi_attrib.type == PCI_CAP_ID_MSIX && - desc->msi_attrib.entry_nr == nr_entry ) + desc->msi_attrib.entry_nr == nr_entry ) { + pcidev_unlock(entry->pdev); return desc; + } + pcidev_unlock(entry->pdev); return NULL; } diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c index bccaccb98b..6b62c4f452 100644 --- a/xen/arch/x86/msi.c +++ b/xen/arch/x86/msi.c @@ -389,6 +389,7 @@ static bool msi_set_mask_bit(struct irq_desc *desc, bool host, bool guest) default: return 0; } + entry->msi_attrib.host_masked = host; entry->msi_attrib.guest_masked = guest; @@ -585,12 +586,17 @@ static struct msi_desc *find_msi_entry(struct pci_dev *dev, { struct msi_desc *entry; + pcidev_lock(dev); list_for_each_entry( entry, &dev->msi_list, list ) { if ( entry->msi_attrib.type == cap_id && (irq == -1 || entry->irq == irq) ) + { + pcidev_unlock(dev); return entry; + } } + pcidev_unlock(dev); return NULL; } @@ -661,7 +667,9 @@ static int msi_capability_init(struct pci_dev *dev, maskbits |= ~(uint32_t)0 >> (32 - dev->msi_maxvec); pci_conf_write32(dev->sbdf, mpos, maskbits); } + pcidev_lock(dev); list_add_tail(&entry->list, &dev->msi_list); + pcidev_unlock(dev); *desc = entry; /* Restore the original MSI enabled bits */ @@ -946,7 +954,9 @@ static int msix_capability_init(struct pci_dev *dev, pcidev_get(dev); + pcidev_lock(dev); list_add_tail(&entry->list, &dev->msi_list); + pcidev_unlock(dev); *desc = entry; } @@ -1231,11 +1241,13 @@ static void msi_free_irqs(struct pci_dev* dev) { struct msi_desc *entry, *tmp; + pcidev_lock(dev); list_for_each_entry_safe( entry, tmp, &dev->msi_list, list ) { pci_disable_msi(entry); msi_free_irq(entry); } + pcidev_unlock(dev); } void pci_cleanup_msi(struct pci_dev *pdev) @@ -1354,6 +1366,7 @@ int pci_restore_msi_state(struct pci_dev *pdev) if ( ret ) return ret; + pcidev_lock(pdev); list_for_each_entry_safe( entry, tmp, &pdev->msi_list, list ) { unsigned int i = 0, nr = 1; @@ -1371,6 +1384,7 @@ int pci_restore_msi_state(struct pci_dev *pdev) dprintk(XENLOG_ERR, "Restore MSI for %pp entry %u not set?\n", &pdev->sbdf, i); spin_unlock_irqrestore(&desc->lock, flags); + pcidev_unlock(pdev); if ( type == PCI_CAP_ID_MSIX ) pci_conf_write16(pdev->sbdf, msix_control_reg(pos), control & ~PCI_MSIX_FLAGS_ENABLE); @@ -1393,6 +1407,7 @@ int pci_restore_msi_state(struct pci_dev *pdev) if ( unlikely(!memory_decoded(pdev)) ) { spin_unlock_irqrestore(&desc->lock, flags); + pcidev_unlock(pdev); pci_conf_write16(pdev->sbdf, msix_control_reg(pos), control & ~PCI_MSIX_FLAGS_ENABLE); return -ENXIO; @@ -1438,6 +1453,7 @@ int pci_restore_msi_state(struct pci_dev *pdev) pci_conf_write16(pdev->sbdf, msix_control_reg(pos), control | PCI_MSIX_FLAGS_ENABLE); + pcidev_unlock(pdev); return 0; } diff --git a/xen/drivers/passthrough/msi.c b/xen/drivers/passthrough/msi.c index ce1a450f6f..98f4d2721a 100644 --- a/xen/drivers/passthrough/msi.c +++ b/xen/drivers/passthrough/msi.c @@ -22,6 +22,7 @@ int pdev_msi_init(struct pci_dev *pdev) { unsigned int pos; + pcidev_lock(pdev); INIT_LIST_HEAD(&pdev->msi_list); pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn), @@ -41,7 +42,10 @@ int pdev_msi_init(struct pci_dev *pdev) uint16_t ctrl; if ( !msix ) - return -ENOMEM; + { + pcidev_unlock(pdev); + return -ENOMEM; + } spin_lock_init(&msix->table_lock); @@ -51,6 +55,8 @@ int pdev_msi_init(struct pci_dev *pdev) pdev->msix = msix; } + pcidev_unlock(pdev); + return 0; } diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index c8da80b981..c83397211b 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -1383,7 +1383,9 @@ static int cf_check _dump_pci_devices(struct pci_seg *pseg, void *arg) printk("%pd", pdev->domain); printk(" - node %-3d refcnt %d", (pdev->node != NUMA_NO_NODE) ? pdev->node : -1, atomic_read(&pdev->refcnt)); + pcidev_lock(pdev); pdev_dump_msi(pdev); + pcidev_unlock(pdev); printk("\n"); } spin_unlock(&pseg->alldevs_lock); diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h index e71a180ef3..d0a7339d84 100644 --- a/xen/include/xen/pci.h +++ b/xen/include/xen/pci.h @@ -106,6 +106,8 @@ struct pci_dev { uint8_t msi_maxvec; uint8_t phantom_stride; + /* Device lock */ + spinlock_t lock; nodeid_t node; /* NUMA node */ /* Device to be quarantined, don't automatically re-assign to dom0 */ @@ -235,6 +237,16 @@ int msixtbl_pt_register(struct domain *, struct pirq *, uint64_t gtable); void msixtbl_pt_unregister(struct domain *, struct pirq *); void msixtbl_pt_cleanup(struct domain *d); +static inline void pcidev_lock(struct pci_dev *pdev) +{ + spin_lock(&pdev->lock); +} + +static inline void pcidev_unlock(struct pci_dev *pdev) +{ + spin_unlock(&pdev->lock); +} + #ifdef CONFIG_HVM int arch_pci_clean_pirqs(struct domain *d); #else
Spinlock in struct pci_device will be used to protect access to device itself. Right now it is used mostly by MSI code. Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> --- xen/arch/x86/hvm/vmsi.c | 6 +++++- xen/arch/x86/msi.c | 16 ++++++++++++++++ xen/drivers/passthrough/msi.c | 8 +++++++- xen/drivers/passthrough/pci.c | 2 ++ xen/include/xen/pci.h | 12 ++++++++++++ 5 files changed, 42 insertions(+), 2 deletions(-)