Xen project Mailing List

[Xen-devel] [PATCH][RFC] fix some spinlock issues in vmsi

From: Kouya Shimura <kouya@xxxxxxxxxxxxxx>

Date: Fri, 6 Mar 2009 11:55:06 +0900

Cc: Isaku Yamahata <yamahata@xxxxxxxxxxxxx>, Keir Fraser <keir.fraser@xxxxxxxxxxxxx>, Qing He <qing.he@xxxxxxxxx>

Delivery-date: Thu, 05 Mar 2009 18:56:08 -0800

List-id: Xen developer discussion <xen-devel.lists.xensource.com>

Hi, This patch fixes some spinlock issues related to changeset: 19246:9bc5799566be "passthrough MSI-X mask bit acceleration" 19263:9c5b4efc934d "hvm: passthrough MSI-X: fix ia64 link and MSI-X clean up" Without this patch, I got: (XEN) Assertion '_spin_is_locked(&pcidevs_lock)' failed at vmsi.c:389 or (XEN) Xen BUG at spinlock.c:25 I'm not sure this fix is right. Please review it. - d->arch.hvm_domain.msixtbl_list_lock is redundant. irq_desc->lock covers it, thus remove the spinlock. - ASSERT(spin_is_locked(&pcidevs_lock)) is not needed. At first, I intended to add the enclosure of pcidevs_lock. But this assertion seems pointless. pcidevs_lock is for alldevs_list and msixtbl_pt_xxx functions never refer it. - In "debug=y" environment, xmalloc must not be called from both irq_enable and irq_disable state. Otherwise, the assertion failure occurs in check_lock(). So, in msixtbl_pt_register, xmalloc is called beforehand. I thinks this is ugly, though. Thanks, Kouya Signed-off-by: Kouya Shimura <kouya@xxxxxxxxxxxxxx>

diff -r cff29d694a89 xen/arch/x86/hvm/hvm.c --- a/xen/arch/x86/hvm/hvm.c Thu Mar 05 17:50:05 2009 +0000 +++ b/xen/arch/x86/hvm/hvm.c Fri Mar 06 11:01:43 2009 +0900 @@ -309,7 +309,6 @@ int hvm_domain_initialise(struct domain spin_lock_init(&d->arch.hvm_domain.uc_lock); INIT_LIST_HEAD(&d->arch.hvm_domain.msixtbl_list); - spin_lock_init(&d->arch.hvm_domain.msixtbl_list_lock); hvm_init_guest_time(d); diff -r cff29d694a89 xen/arch/x86/hvm/vmsi.c --- a/xen/arch/x86/hvm/vmsi.c Thu Mar 05 17:50:05 2009 +0000 +++ b/xen/arch/x86/hvm/vmsi.c Fri Mar 06 11:01:43 2009 +0900 @@ -336,16 +336,12 @@ struct hvm_mmio_handler msixtbl_mmio_han .write_handler = msixtbl_write }; -static struct msixtbl_entry *add_msixtbl_entry(struct domain *d, - struct pci_dev *pdev, - uint64_t gtable) +static void add_msixtbl_entry(struct domain *d, + struct pci_dev *pdev, + uint64_t gtable, + struct msixtbl_entry *entry) { - struct msixtbl_entry *entry; u32 len; - - entry = xmalloc(struct msixtbl_entry); - if ( !entry ) - return NULL; memset(entry, 0, sizeof(struct msixtbl_entry)); @@ -359,8 +355,6 @@ static struct msixtbl_entry *add_msixtbl entry->gtable = (unsigned long) gtable; list_add_rcu(&entry->list, &d->arch.hvm_domain.msixtbl_list); - - return entry; } static void free_msixtbl_entry(struct rcu_head *rcu) @@ -383,12 +377,17 @@ int msixtbl_pt_register(struct domain *d irq_desc_t *irq_desc; struct msi_desc *msi_desc; struct pci_dev *pdev; - struct msixtbl_entry *entry; + struct msixtbl_entry *entry, *new_entry; int r = -EINVAL; - ASSERT(spin_is_locked(&pcidevs_lock)); + /* XXX: beforehand allocation is needed since check_lock() fails. */ + new_entry = xmalloc(struct msixtbl_entry); + if ( !new_entry ) + return r; irq_desc = domain_spin_lock_irq_desc(d, pirq, NULL); + if ( !irq_desc ) + return r; if ( irq_desc->handler != &pci_msi_type ) goto out; @@ -399,28 +398,22 @@ int msixtbl_pt_register(struct domain *d pdev = msi_desc->dev; - spin_lock(&d->arch.hvm_domain.msixtbl_list_lock); - list_for_each_entry( entry, &d->arch.hvm_domain.msixtbl_list, list ) if ( pdev == entry->pdev ) goto found; - entry = add_msixtbl_entry(d, pdev, gtable); - if ( !entry ) - { - spin_unlock(&d->arch.hvm_domain.msixtbl_list_lock); - goto out; - } + entry = new_entry; + new_entry = NULL; + add_msixtbl_entry(d, pdev, gtable, entry); found: atomic_inc(&entry->refcnt); - spin_unlock(&d->arch.hvm_domain.msixtbl_list_lock); r = 0; out: spin_unlock_irq(&irq_desc->lock); + xfree(new_entry); return r; - } void msixtbl_pt_unregister(struct domain *d, int pirq) @@ -430,9 +423,9 @@ void msixtbl_pt_unregister(struct domain struct pci_dev *pdev; struct msixtbl_entry *entry; - ASSERT(spin_is_locked(&pcidevs_lock)); - irq_desc = domain_spin_lock_irq_desc(d, pirq, NULL); + if ( !irq_desc ) + return; if ( irq_desc->handler != &pci_msi_type ) goto out; @@ -443,36 +436,33 @@ void msixtbl_pt_unregister(struct domain pdev = msi_desc->dev; - spin_lock(&d->arch.hvm_domain.msixtbl_list_lock); - list_for_each_entry( entry, &d->arch.hvm_domain.msixtbl_list, list ) if ( pdev == entry->pdev ) goto found; - spin_unlock(&d->arch.hvm_domain.msixtbl_list_lock); - - out: - spin_unlock(&irq_desc->lock); + spin_unlock_irq(&irq_desc->lock); return; found: if ( !atomic_dec_and_test(&entry->refcnt) ) del_msixtbl_entry(entry); - spin_unlock(&d->arch.hvm_domain.msixtbl_list_lock); - spin_unlock(&irq_desc->lock); + spin_unlock_irq(&irq_desc->lock); } void msixtbl_pt_cleanup(struct domain *d, int pirq) { + irq_desc_t *irq_desc; struct msixtbl_entry *entry, *temp; - spin_lock(&d->arch.hvm_domain.msixtbl_list_lock); + irq_desc = domain_spin_lock_irq_desc(d, pirq, NULL); + if ( !irq_desc ) + return; list_for_each_entry_safe( entry, temp, &d->arch.hvm_domain.msixtbl_list, list ) del_msixtbl_entry(entry); - spin_unlock(&d->arch.hvm_domain.msixtbl_list_lock); + spin_unlock_irq(&irq_desc->lock); } diff -r cff29d694a89 xen/include/asm-x86/hvm/domain.h --- a/xen/include/asm-x86/hvm/domain.h Thu Mar 05 17:50:05 2009 +0000 +++ b/xen/include/asm-x86/hvm/domain.h Fri Mar 06 11:01:43 2009 +0900 @@ -77,7 +77,6 @@ struct hvm_domain { /* hypervisor intercepted msix table */ struct list_head msixtbl_list; - spinlock_t msixtbl_list_lock; struct viridian_domain viridian;

_______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxx http://lists.xensource.com/xen-devel

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.