[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [RFC PATCH 07/10] xen: pci: add per-device locking
Hi Stefano, Stefano Stabellini <sstabellini@xxxxxxxxxx> 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@xxxxxxxx> > > 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 >>
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |