* [PATCH 0/2] Make the pcidevs_lock a recursive one
@ 2016-03-08 11:09 Quan Xu
2016-03-08 11:09 ` [PATCH 1/2] IOMMU/spinlock: Fix a bug found in AMD IOMMU initialization Quan Xu
2016-03-08 11:09 ` [PATCH 2/2] IOMMU/spinlock: Make the pcidevs_lock a recursive one Quan Xu
0 siblings, 2 replies; 10+ messages in thread
From: Quan Xu @ 2016-03-08 11:09 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Keir Fraser, Jan Beulich, Andrew Cooper,
Dario Faggioli, Aravind Gopalakrishnan, Suravee Suthikulpanit,
Quan Xu, Feng Wu
This patch set makes the pcidevs_lock a recursive one. It is a prereq
patch set for Patch:'VT-d Device-TLB flush issue', as the pcidevs_lock
may be recursively held for hiding the ATS device, when IOMMU Device-TLB
flush timed out.
In detail:
1. Fix a bug found in AMD IOMMU initialization.
Doing what we do serves as a fix for a bug found in AMD IOMMU initialization.
The current code is using spin_lock{_irqsave(), _irqrestore()} to
protect pci_get_dev() in the set_iommu_interrupt_handler(). However,
this can only be called during AMD IOMMU initialization, with interrupt
enabled, so at least it is not necessary to disable interrupts, or
save/restore interrupt flag.
In order to fix this, we can use just plain spin{_lock(),_unlock()},
instead of spin_lock{_irqsave(),_irqrestore()}.
2. Make the pcidevs_lock a recursive one.
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
CC: Feng Wu <feng.wu@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Dario Faggioli <dario.faggioli@citrix.com>
Quan Xu (2):
IOMMU/spinlock: Fix a bug found in AMD IOMMU initialization.
IOMMU/spinlock: Make the pcidevs_lock a recursive one
xen/arch/x86/domctl.c | 8 +--
xen/arch/x86/hvm/vmsi.c | 4 +-
xen/arch/x86/irq.c | 8 +--
xen/arch/x86/msi.c | 16 ++---
xen/arch/x86/pci.c | 4 +-
xen/arch/x86/physdev.c | 16 ++---
xen/common/sysctl.c | 4 +-
xen/drivers/passthrough/amd/iommu_init.c | 9 ++-
xen/drivers/passthrough/amd/iommu_map.c | 2 +-
xen/drivers/passthrough/amd/pci_amd_iommu.c | 4 +-
xen/drivers/passthrough/pci.c | 96 +++++++++++++++++------------
xen/drivers/passthrough/vtd/iommu.c | 14 ++---
xen/drivers/video/vga.c | 4 +-
xen/include/xen/pci.h | 5 +-
14 files changed, 108 insertions(+), 86 deletions(-)
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] IOMMU/spinlock: Fix a bug found in AMD IOMMU initialization.
2016-03-08 11:09 [PATCH 0/2] Make the pcidevs_lock a recursive one Quan Xu
@ 2016-03-08 11:09 ` Quan Xu
2016-03-08 12:12 ` Dario Faggioli
2016-03-08 13:54 ` Jan Beulich
2016-03-08 11:09 ` [PATCH 2/2] IOMMU/spinlock: Make the pcidevs_lock a recursive one Quan Xu
1 sibling, 2 replies; 10+ messages in thread
From: Quan Xu @ 2016-03-08 11:09 UTC (permalink / raw)
To: xen-devel
Cc: Dario Faggioli, Jan Beulich, Aravind Gopalakrishnan,
Suravee Suthikulpanit, Quan Xu
Doing what we do serves as a fix for a bug found in AMD IOMMU initialization.
The current code is using spin_lock{_irqsave(), _irqrestore()} to
protect pci_get_dev() in the set_iommu_interrupt_handler(). However,
this can only be called during AMD IOMMU initialization, with interrupt
enabled, so at least it is not necessary to disable interrupts, or
save/restore interrupt flag.
In order to fix this, we can use just plain spin{_lock(),_unlock()},
instead of spin_lock{_irqsave(),_irqrestore()}.
Signed-off-by: Quan Xu <quan.xu@intel.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
CC: Dario Faggioli <dario.faggioli@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
---
xen/drivers/passthrough/amd/iommu_init.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index d90a2d2..a400497 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -778,7 +778,6 @@ static bool_t __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
{
int irq, ret;
hw_irq_controller *handler;
- unsigned long flags;
u16 control;
irq = create_irq(NUMA_NO_NODE);
@@ -788,10 +787,10 @@ static bool_t __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
return 0;
}
- spin_lock_irqsave(&pcidevs_lock, flags);
+ spin_lock(&pcidevs_lock);
iommu->msi.dev = pci_get_pdev(iommu->seg, PCI_BUS(iommu->bdf),
PCI_DEVFN2(iommu->bdf));
- spin_unlock_irqrestore(&pcidevs_lock, flags);
+ spin_unlock(&pcidevs_lock);
if ( !iommu->msi.dev )
{
AMD_IOMMU_DEBUG("IOMMU: no pdev for %04x:%02x:%02x.%u\n",
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] IOMMU/spinlock: Make the pcidevs_lock a recursive one
2016-03-08 11:09 [PATCH 0/2] Make the pcidevs_lock a recursive one Quan Xu
2016-03-08 11:09 ` [PATCH 1/2] IOMMU/spinlock: Fix a bug found in AMD IOMMU initialization Quan Xu
@ 2016-03-08 11:09 ` Quan Xu
2016-03-08 12:29 ` Dario Faggioli
1 sibling, 1 reply; 10+ messages in thread
From: Quan Xu @ 2016-03-08 11:09 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Keir Fraser, Jan Beulich, Andrew Cooper,
Dario Faggioli, Aravind Gopalakrishnan, Suravee Suthikulpanit,
Quan Xu, Feng Wu
Signed-off-by: Quan Xu <quan.xu@intel.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
CC: Feng Wu <feng.wu@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Dario Faggioli <dario.faggioli@citrix.com>
---
xen/arch/x86/domctl.c | 8 +--
xen/arch/x86/hvm/vmsi.c | 4 +-
xen/arch/x86/irq.c | 8 +--
xen/arch/x86/msi.c | 16 ++---
xen/arch/x86/pci.c | 4 +-
xen/arch/x86/physdev.c | 16 ++---
xen/common/sysctl.c | 4 +-
xen/drivers/passthrough/amd/iommu_init.c | 8 +--
xen/drivers/passthrough/amd/iommu_map.c | 2 +-
xen/drivers/passthrough/amd/pci_amd_iommu.c | 4 +-
xen/drivers/passthrough/pci.c | 96 +++++++++++++++++------------
xen/drivers/passthrough/vtd/iommu.c | 14 ++---
xen/drivers/video/vga.c | 4 +-
xen/include/xen/pci.h | 5 +-
14 files changed, 108 insertions(+), 85 deletions(-)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index bf62a88..21cc161 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -427,9 +427,9 @@ long arch_do_domctl(
ret = -ESRCH;
if ( iommu_enabled )
{
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
ret = pt_irq_create_bind(d, bind);
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
}
if ( ret < 0 )
printk(XENLOG_G_ERR "pt_irq_create_bind failed (%ld) for dom%d\n",
@@ -452,9 +452,9 @@ long arch_do_domctl(
if ( iommu_enabled )
{
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
ret = pt_irq_destroy_bind(d, bind);
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
}
if ( ret < 0 )
printk(XENLOG_G_ERR "pt_irq_destroy_bind failed (%ld) for dom%d\n",
diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index ac838a9..8e0817b 100644
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -388,7 +388,7 @@ int msixtbl_pt_register(struct domain *d, struct pirq *pirq, uint64_t gtable)
struct msixtbl_entry *entry, *new_entry;
int r = -EINVAL;
- ASSERT(spin_is_locked(&pcidevs_lock));
+ ASSERT(pcidevs_is_locked());
ASSERT(spin_is_locked(&d->event_lock));
/*
@@ -443,7 +443,7 @@ void msixtbl_pt_unregister(struct domain *d, struct pirq *pirq)
struct pci_dev *pdev;
struct msixtbl_entry *entry;
- ASSERT(spin_is_locked(&pcidevs_lock));
+ ASSERT(pcidevs_is_locked());
ASSERT(spin_is_locked(&d->event_lock));
irq_desc = pirq_spin_lock_irq_desc(pirq, NULL);
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index bf2e822..68bdf19 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -1955,7 +1955,7 @@ int map_domain_pirq(
struct pci_dev *pdev;
unsigned int nr = 0;
- ASSERT(spin_is_locked(&pcidevs_lock));
+ ASSERT(pcidevs_is_locked());
ret = -ENODEV;
if ( !cpu_has_apic )
@@ -2100,7 +2100,7 @@ int unmap_domain_pirq(struct domain *d, int pirq)
if ( (pirq < 0) || (pirq >= d->nr_pirqs) )
return -EINVAL;
- ASSERT(spin_is_locked(&pcidevs_lock));
+ ASSERT(pcidevs_is_locked());
ASSERT(spin_is_locked(&d->event_lock));
info = pirq_info(d, pirq);
@@ -2226,7 +2226,7 @@ void free_domain_pirqs(struct domain *d)
{
int i;
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
spin_lock(&d->event_lock);
for ( i = 0; i < d->nr_pirqs; i++ )
@@ -2234,7 +2234,7 @@ void free_domain_pirqs(struct domain *d)
unmap_domain_pirq(d, i);
spin_unlock(&d->event_lock);
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
}
static void dump_irqs(unsigned char key)
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index 3dbb84d..6e5e33e 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -694,7 +694,7 @@ static int msi_capability_init(struct pci_dev *dev,
u8 slot = PCI_SLOT(dev->devfn);
u8 func = PCI_FUNC(dev->devfn);
- ASSERT(spin_is_locked(&pcidevs_lock));
+ ASSERT(pcidevs_is_locked());
pos = pci_find_cap_offset(seg, bus, slot, func, PCI_CAP_ID_MSI);
if ( !pos )
return -ENODEV;
@@ -852,7 +852,7 @@ static int msix_capability_init(struct pci_dev *dev,
u8 func = PCI_FUNC(dev->devfn);
bool_t maskall = msix->host_maskall;
- ASSERT(spin_is_locked(&pcidevs_lock));
+ ASSERT(pcidevs_is_locked());
control = pci_conf_read16(seg, bus, slot, func, msix_control_reg(pos));
/*
@@ -1042,7 +1042,7 @@ static int __pci_enable_msi(struct msi_info *msi, struct msi_desc **desc)
struct pci_dev *pdev;
struct msi_desc *old_desc;
- ASSERT(spin_is_locked(&pcidevs_lock));
+ ASSERT(pcidevs_is_locked());
pdev = pci_get_pdev(msi->seg, msi->bus, msi->devfn);
if ( !pdev )
return -ENODEV;
@@ -1103,7 +1103,7 @@ static int __pci_enable_msix(struct msi_info *msi, struct msi_desc **desc)
u8 func = PCI_FUNC(msi->devfn);
struct msi_desc *old_desc;
- ASSERT(spin_is_locked(&pcidevs_lock));
+ ASSERT(pcidevs_is_locked());
pdev = pci_get_pdev(msi->seg, msi->bus, msi->devfn);
pos = pci_find_cap_offset(msi->seg, msi->bus, slot, func, PCI_CAP_ID_MSIX);
if ( !pdev || !pos )
@@ -1205,7 +1205,7 @@ int pci_prepare_msix(u16 seg, u8 bus, u8 devfn, bool_t off)
if ( !pos )
return -ENODEV;
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
pdev = pci_get_pdev(seg, bus, devfn);
if ( !pdev )
rc = -ENODEV;
@@ -1224,7 +1224,7 @@ int pci_prepare_msix(u16 seg, u8 bus, u8 devfn, bool_t off)
rc = msix_capability_init(pdev, pos, NULL, NULL,
multi_msix_capable(control));
}
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
return rc;
}
@@ -1235,7 +1235,7 @@ int pci_prepare_msix(u16 seg, u8 bus, u8 devfn, bool_t off)
*/
int pci_enable_msi(struct msi_info *msi, struct msi_desc **desc)
{
- ASSERT(spin_is_locked(&pcidevs_lock));
+ ASSERT(pcidevs_is_locked());
if ( !use_msi )
return -EPERM;
@@ -1351,7 +1351,7 @@ int pci_restore_msi_state(struct pci_dev *pdev)
unsigned int type = 0, pos = 0;
u16 control = 0;
- ASSERT(spin_is_locked(&pcidevs_lock));
+ ASSERT(pcidevs_is_locked());
if ( !use_msi )
return -EOPNOTSUPP;
diff --git a/xen/arch/x86/pci.c b/xen/arch/x86/pci.c
index 5bcecbb..892278d 100644
--- a/xen/arch/x86/pci.c
+++ b/xen/arch/x86/pci.c
@@ -82,13 +82,13 @@ int pci_conf_write_intercept(unsigned int seg, unsigned int bdf,
if ( reg < 64 || reg >= 256 )
return 0;
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
pdev = pci_get_pdev(seg, PCI_BUS(bdf), PCI_DEVFN2(bdf));
if ( pdev )
rc = pci_msi_conf_write_intercept(pdev, reg, size, data);
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
return rc;
}
diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index 57b7800..1cb9b58 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -167,7 +167,7 @@ int physdev_map_pirq(domid_t domid, int type, int *index, int *pirq_p,
goto free_domain;
}
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
/* Verify or get pirq. */
spin_lock(&d->event_lock);
pirq = domain_irq_to_pirq(d, irq);
@@ -237,7 +237,7 @@ int physdev_map_pirq(domid_t domid, int type, int *index, int *pirq_p,
done:
spin_unlock(&d->event_lock);
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
if ( ret != 0 )
switch ( type )
{
@@ -275,11 +275,11 @@ int physdev_unmap_pirq(domid_t domid, int pirq)
goto free_domain;
}
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
spin_lock(&d->event_lock);
ret = unmap_domain_pirq(d, pirq);
spin_unlock(&d->event_lock);
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
free_domain:
rcu_unlock_domain(d);
@@ -689,10 +689,10 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
if ( copy_from_guest(&restore_msi, arg, 1) != 0 )
break;
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
pdev = pci_get_pdev(0, restore_msi.bus, restore_msi.devfn);
ret = pdev ? pci_restore_msi_state(pdev) : -ENODEV;
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
break;
}
@@ -704,10 +704,10 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
if ( copy_from_guest(&dev, arg, 1) != 0 )
break;
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
pdev = pci_get_pdev(dev.seg, dev.bus, dev.devfn);
ret = pdev ? pci_restore_msi_state(pdev) : -ENODEV;
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
break;
}
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 85e853f..401edb9 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -426,7 +426,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
break;
}
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
pdev = pci_get_pdev(dev.seg, dev.bus, dev.devfn);
if ( !pdev )
node = XEN_INVALID_DEV;
@@ -434,7 +434,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
node = XEN_INVALID_NODE_ID;
else
node = pdev->node;
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
if ( copy_to_guest_offset(ti->nodes, i, &node, 1) )
{
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index a400497..4536106 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -673,9 +673,9 @@ void parse_ppr_log_entry(struct amd_iommu *iommu, u32 entry[])
bus = PCI_BUS(device_id);
devfn = PCI_DEVFN2(device_id);
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
pdev = pci_get_real_pdev(iommu->seg, bus, devfn);
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
if ( pdev )
guest_iommu_add_ppr_log(pdev->domain, entry);
@@ -787,10 +787,10 @@ static bool_t __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
return 0;
}
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
iommu->msi.dev = pci_get_pdev(iommu->seg, PCI_BUS(iommu->bdf),
PCI_DEVFN2(iommu->bdf));
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
if ( !iommu->msi.dev )
{
AMD_IOMMU_DEBUG("IOMMU: no pdev for %04x:%02x:%02x.%u\n",
diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
index 78862c9..9d91dfc 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -593,7 +593,7 @@ static int update_paging_mode(struct domain *d, unsigned long gfn)
hd->arch.paging_mode = level;
hd->arch.root_table = new_root;
- if ( !spin_is_locked(&pcidevs_lock) )
+ if ( !pcidevs_is_locked() )
AMD_IOMMU_DEBUG("%s Try to access pdev_list "
"without aquiring pcidevs_lock.\n", __func__);
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index c1c0b6b..dc3bfab 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -158,7 +158,7 @@ static void amd_iommu_setup_domain_device(
spin_unlock_irqrestore(&iommu->lock, flags);
- ASSERT(spin_is_locked(&pcidevs_lock));
+ ASSERT(pcidevs_is_locked());
if ( pci_ats_device(iommu->seg, bus, pdev->devfn) &&
!pci_ats_enabled(iommu->seg, bus, pdev->devfn) )
@@ -345,7 +345,7 @@ void amd_iommu_disable_domain_device(struct domain *domain,
}
spin_unlock_irqrestore(&iommu->lock, flags);
- ASSERT(spin_is_locked(&pcidevs_lock));
+ ASSERT(pcidevs_is_locked());
if ( devfn == pdev->devfn &&
pci_ats_device(iommu->seg, bus, devfn) &&
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 27b3ca7..539518c 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -48,7 +48,7 @@ struct pci_seg {
} bus2bridge[MAX_BUSES];
};
-spinlock_t pcidevs_lock = SPIN_LOCK_UNLOCKED;
+static spinlock_t _pcidevs_lock = SPIN_LOCK_UNLOCKED;
static struct radix_tree_root pci_segments;
static inline struct pci_seg *get_pseg(u16 seg)
@@ -103,6 +103,26 @@ static int pci_segments_iterate(
return rc;
}
+void pcidevs_lock(void)
+{
+ spin_lock_recursive(&_pcidevs_lock);
+}
+
+void pcidevs_unlock(void)
+{
+ spin_unlock_recursive(&_pcidevs_lock);
+}
+
+int pcidevs_is_locked(void)
+{
+ return spin_is_locked(&_pcidevs_lock);
+}
+
+int pcidevs_trylock(void)
+{
+ return spin_trylock_recursive(&_pcidevs_lock);
+}
+
void __init pt_pci_init(void)
{
radix_tree_init(&pci_segments);
@@ -412,14 +432,14 @@ int __init pci_hide_device(int bus, int devfn)
struct pci_dev *pdev;
int rc = -ENOMEM;
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
pdev = alloc_pdev(get_pseg(0), bus, devfn);
if ( pdev )
{
_pci_hide_device(pdev);
rc = 0;
}
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
return rc;
}
@@ -456,7 +476,7 @@ struct pci_dev *pci_get_pdev(int seg, int bus, int devfn)
struct pci_seg *pseg = get_pseg(seg);
struct pci_dev *pdev = NULL;
- ASSERT(spin_is_locked(&pcidevs_lock));
+ ASSERT(pcidevs_is_locked());
ASSERT(seg != -1 || bus == -1);
ASSERT(bus != -1 || devfn == -1);
@@ -581,9 +601,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
pdev_type = "extended function";
else if (info->is_virtfn)
{
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
pdev = pci_get_pdev(seg, info->physfn.bus, info->physfn.devfn);
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
if ( !pdev )
pci_add_device(seg, info->physfn.bus, info->physfn.devfn,
NULL, node);
@@ -601,7 +621,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
ret = -ENOMEM;
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
pseg = alloc_pseg(seg);
if ( !pseg )
goto out;
@@ -703,7 +723,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
pci_enable_acs(pdev);
out:
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
if ( !ret )
{
printk(XENLOG_DEBUG "PCI add %s %04x:%02x:%02x.%u\n", pdev_type,
@@ -735,7 +755,7 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
if ( !pseg )
return -ENODEV;
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
if ( pdev->bus == bus && pdev->devfn == devfn )
{
@@ -749,7 +769,7 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
break;
}
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
return ret;
}
@@ -807,11 +827,11 @@ int pci_release_devices(struct domain *d)
u8 bus, devfn;
int ret;
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
ret = pci_clean_dpci_irqs(d);
if ( ret )
{
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
return ret;
}
while ( (pdev = pci_get_pdev_by_domain(d, -1, -1, -1)) )
@@ -823,7 +843,7 @@ int pci_release_devices(struct domain *d)
d->domain_id, pdev->seg, bus,
PCI_SLOT(devfn), PCI_FUNC(devfn));
}
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
return 0;
}
@@ -920,7 +940,7 @@ void pci_check_disable_device(u16 seg, u8 bus, u8 devfn)
s_time_t now = NOW();
u16 cword;
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
pdev = pci_get_real_pdev(seg, bus, devfn);
if ( pdev )
{
@@ -931,7 +951,7 @@ void pci_check_disable_device(u16 seg, u8 bus, u8 devfn)
if ( ++pdev->fault.count < PT_FAULT_THRESHOLD )
pdev = NULL;
}
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
if ( !pdev )
return;
@@ -988,9 +1008,9 @@ int __init scan_pci_devices(void)
{
int ret;
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
ret = pci_segments_iterate(_scan_pci_devices, NULL);
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
return ret;
}
@@ -1054,17 +1074,17 @@ static int __hwdom_init _setup_hwdom_pci_devices(struct pci_seg *pseg, void *arg
if ( iommu_verbose )
{
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
process_pending_softirqs();
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
}
}
if ( !iommu_verbose )
{
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
process_pending_softirqs();
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
}
}
@@ -1076,9 +1096,9 @@ void __hwdom_init setup_hwdom_pci_devices(
{
struct setup_hwdom ctxt = { .d = d, .handler = handler };
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
pci_segments_iterate(_setup_hwdom_pci_devices, &ctxt);
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
}
#ifdef CONFIG_ACPI
@@ -1206,9 +1226,9 @@ static int _dump_pci_devices(struct pci_seg *pseg, void *arg)
static void dump_pci_devices(unsigned char ch)
{
printk("==== PCI devices ====\n");
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
pci_segments_iterate(_dump_pci_devices, NULL);
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
}
struct keyhandler dump_pci_devices_keyhandler = {
@@ -1248,7 +1268,7 @@ int iommu_add_device(struct pci_dev *pdev)
if ( !pdev->domain )
return -EINVAL;
- ASSERT(spin_is_locked(&pcidevs_lock));
+ ASSERT(pcidevs_is_locked());
hd = domain_hvm_iommu(pdev->domain);
if ( !iommu_enabled || !hd->platform_ops )
@@ -1277,7 +1297,7 @@ int iommu_enable_device(struct pci_dev *pdev)
if ( !pdev->domain )
return -EINVAL;
- ASSERT(spin_is_locked(&pcidevs_lock));
+ ASSERT(pcidevs_is_locked());
hd = domain_hvm_iommu(pdev->domain);
if ( !iommu_enabled || !hd->platform_ops ||
@@ -1326,9 +1346,9 @@ static int device_assigned(u16 seg, u8 bus, u8 devfn)
{
struct pci_dev *pdev;
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
pdev = pci_get_pdev_by_domain(hardware_domain, seg, bus, devfn);
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
return pdev ? 0 : -EBUSY;
}
@@ -1350,13 +1370,13 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
p2m_get_hostp2m(d)->global_logdirty)) )
return -EXDEV;
- if ( !spin_trylock(&pcidevs_lock) )
+ if ( !pcidevs_trylock() )
return -ERESTART;
rc = iommu_construct(d);
if ( rc )
{
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
return rc;
}
@@ -1387,7 +1407,7 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
done:
if ( !has_arch_pdevs(d) && need_iommu(d) )
iommu_teardown(d);
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
return rc;
}
@@ -1402,7 +1422,7 @@ int deassign_device(struct domain *d, u16 seg, u8 bus, u8 devfn)
if ( !iommu_enabled || !hd->platform_ops )
return -EINVAL;
- ASSERT(spin_is_locked(&pcidevs_lock));
+ ASSERT(pcidevs_is_locked());
pdev = pci_get_pdev_by_domain(d, seg, bus, devfn);
if ( !pdev )
return -ENODEV;
@@ -1457,7 +1477,7 @@ static int iommu_get_device_group(
group_id = ops->get_device_group_id(seg, bus, devfn);
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
for_each_pdev( d, pdev )
{
if ( (pdev->seg != seg) ||
@@ -1476,14 +1496,14 @@ static int iommu_get_device_group(
if ( unlikely(copy_to_guest_offset(buf, i, &bdf, 1)) )
{
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
return -1;
}
i++;
}
}
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
return i;
}
@@ -1611,9 +1631,9 @@ int iommu_do_pci_domctl(
bus = PCI_BUS(machine_sbdf);
devfn = PCI_DEVFN2(machine_sbdf);
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
ret = deassign_device(d, seg, bus, devfn);
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
if ( ret )
printk(XENLOG_G_ERR
"deassign %04x:%02x:%02x.%u from dom%d failed (%d)\n",
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index dd13865..30f007e 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1286,7 +1286,7 @@ int domain_context_mapping_one(
u16 seg = iommu->intel->drhd->segment;
int agaw;
- ASSERT(spin_is_locked(&pcidevs_lock));
+ ASSERT(pcidevs_is_locked());
spin_lock(&iommu->lock);
maddr = bus_to_context_maddr(iommu, bus);
context_entries = (struct context_entry *)map_vtd_domain_page(maddr);
@@ -1428,7 +1428,7 @@ static int domain_context_mapping(
if ( !drhd )
return -ENODEV;
- ASSERT(spin_is_locked(&pcidevs_lock));
+ ASSERT(pcidevs_is_locked());
switch ( pdev->type )
{
@@ -1510,7 +1510,7 @@ int domain_context_unmap_one(
u64 maddr;
int iommu_domid;
- ASSERT(spin_is_locked(&pcidevs_lock));
+ ASSERT(pcidevs_is_locked());
spin_lock(&iommu->lock);
maddr = bus_to_context_maddr(iommu, bus);
@@ -1820,7 +1820,7 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map,
struct mapped_rmrr *mrmrr;
struct hvm_iommu *hd = domain_hvm_iommu(d);
- ASSERT(spin_is_locked(&pcidevs_lock));
+ ASSERT(pcidevs_is_locked());
ASSERT(rmrr->base_address < rmrr->end_address);
/*
@@ -1885,7 +1885,7 @@ static int intel_iommu_add_device(u8 devfn, struct pci_dev *pdev)
u16 bdf;
int ret, i;
- ASSERT(spin_is_locked(&pcidevs_lock));
+ ASSERT(pcidevs_is_locked());
if ( !pdev->domain )
return -EINVAL;
@@ -2113,7 +2113,7 @@ static void __hwdom_init setup_hwdom_rmrr(struct domain *d)
u16 bdf;
int ret, i;
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
for_each_rmrr_device ( rmrr, bdf, i )
{
/*
@@ -2127,7 +2127,7 @@ static void __hwdom_init setup_hwdom_rmrr(struct domain *d)
dprintk(XENLOG_ERR VTDPREFIX,
"IOMMU: mapping reserved region failed\n");
}
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
}
int __init intel_vtd_setup(void)
diff --git a/xen/drivers/video/vga.c b/xen/drivers/video/vga.c
index 40e5963..61c6b13 100644
--- a/xen/drivers/video/vga.c
+++ b/xen/drivers/video/vga.c
@@ -117,9 +117,9 @@ void __init video_endboot(void)
const struct pci_dev *pdev;
u8 b = bus, df = devfn, sb;
- spin_lock(&pcidevs_lock);
+ pcidevs_lock();
pdev = pci_get_pdev(0, bus, devfn);
- spin_unlock(&pcidevs_lock);
+ pcidevs_unlock();
if ( !pdev ||
pci_conf_read16(0, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index a5aef55..b87571d 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -94,7 +94,10 @@ struct pci_dev {
* interrupt handling related (the mask bit register).
*/
-extern spinlock_t pcidevs_lock;
+void pcidevs_lock(void);
+void pcidevs_unlock(void);
+int pcidevs_is_locked(void);
+int pcidevs_trylock(void);
bool_t pci_known_segment(u16 seg);
bool_t pci_device_detect(u16 seg, u8 bus, u8 dev, u8 func);
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] IOMMU/spinlock: Fix a bug found in AMD IOMMU initialization.
2016-03-08 11:09 ` [PATCH 1/2] IOMMU/spinlock: Fix a bug found in AMD IOMMU initialization Quan Xu
@ 2016-03-08 12:12 ` Dario Faggioli
2016-03-08 12:35 ` Xu, Quan
2016-03-08 13:54 ` Jan Beulich
1 sibling, 1 reply; 10+ messages in thread
From: Dario Faggioli @ 2016-03-08 12:12 UTC (permalink / raw)
To: Quan Xu, xen-devel; +Cc: Jan Beulich, Suravee Suthikulpanit
[-- Attachment #1.1: Type: text/plain, Size: 1319 bytes --]
On Tue, 2016-03-08 at 19:09 +0800, Quan Xu wrote:
> Doing what we do serves as a fix for a bug found in AMD IOMMU
> initialization.
>
This first line is rather useless, if not worse. :-)
I don't know (provided a new version is not necessary, and provided
maintainers agree with me :-)) whether you need to repost or it can be
removed when code is committed.
> Signed-off-by: Quan Xu <quan.xu@intel.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>
get_maintainer.pl gives me only Suravee, as Aravind stepped down a few
days ago, so he shouldn't be bothered (and, in fact, I'm moving him
from Cc to Bcc).
> CC: Dario Faggioli <dario.faggioli@citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
>
(BTW, it's of course fine to include me and Jan, despite what
get_maintainer.pl's output, as we've been involved in previous rounds
of review.)
All that being said:
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
Thanks and Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] IOMMU/spinlock: Make the pcidevs_lock a recursive one
2016-03-08 11:09 ` [PATCH 2/2] IOMMU/spinlock: Make the pcidevs_lock a recursive one Quan Xu
@ 2016-03-08 12:29 ` Dario Faggioli
2016-03-08 12:39 ` Xu, Quan
0 siblings, 1 reply; 10+ messages in thread
From: Dario Faggioli @ 2016-03-08 12:29 UTC (permalink / raw)
To: Quan Xu, xen-devel
Cc: Kevin Tian, Feng Wu, Jan Beulich, Andrew Cooper,
Aravind Gopalakrishnan, Suravee Suthikulpanit, Keir Fraser
[-- Attachment #1.1: Type: text/plain, Size: 5249 bytes --]
On Tue, 2016-03-08 at 19:09 +0800, Quan Xu wrote:
> Signed-off-by: Quan Xu <quan.xu@intel.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> CC: Feng Wu <feng.wu@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Dario Faggioli <dario.faggioli@citrix.com>
>
I've gone through the code, and it looks fine.
However, when trying to apply the patch, on top of this morning's
staging, I got this:
[dario@Solace xen.git] $ patch -p1 < \[PATCH_2_2\]_IOMMU_spinlock\:_Make_the_pcidevs_lock_a_recursive_one.mbox
patching file xen/arch/x86/domctl.c
Hunk #1 succeeded at 472 (offset 45 lines).
Hunk #2 succeeded at 497 (offset 45 lines).
patching file xen/arch/x86/hvm/vmsi.c
Hunk #1 succeeded at 388 with fuzz 1.
Hunk #2 succeeded at 446 with fuzz 1 (offset 3 lines).
patching file xen/arch/x86/irq.c
Hunk #1 succeeded at 1960 (offset 5 lines).
Hunk #2 succeeded at 2105 (offset 5 lines).
Hunk #3 succeeded at 2231 (offset 5 lines).
Hunk #4 succeeded at 2239 (offset 5 lines).
patching file xen/arch/x86/msi.c
patching file xen/arch/x86/pci.c
Hunk #1 succeeded at 88 (offset 6 lines).
patching file xen/arch/x86/physdev.c
patching file xen/common/sysctl.c
patching file xen/drivers/passthrough/amd/iommu_init.c
patching file xen/drivers/passthrough/amd/iommu_map.c
patching file xen/drivers/passthrough/amd/pci_amd_iommu.c
patching file xen/drivers/passthrough/pci.c
Hunk #17 succeeded at 1226 with fuzz 1.
Hunk #18 succeeded at 1262 (offset -6 lines).
Hunk #19 succeeded at 1291 (offset -6 lines).
Hunk #20 succeeded at 1340 (offset -6 lines).
Hunk #21 succeeded at 1364 (offset -6 lines).
Hunk #22 succeeded at 1401 (offset -6 lines).
Hunk #23 succeeded at 1416 (offset -6 lines).
Hunk #24 succeeded at 1471 (offset -6 lines).
Hunk #25 succeeded at 1490 (offset -6 lines).
Hunk #26 succeeded at 1625 (offset -6 lines).
patching file xen/drivers/passthrough/vtd/iommu.c
Hunk #1 succeeded at 1282 (offset -4 lines).
Hunk #2 succeeded at 1424 (offset -4 lines).
Hunk #3 succeeded at 1506 (offset -4 lines).
Hunk #4 succeeded at 1816 (offset -4 lines).
Hunk #5 succeeded at 1881 (offset -4 lines).
Hunk #6 succeeded at 2109 (offset -4 lines).
Hunk #7 succeeded at 2123 (offset -4 lines).
patching file xen/drivers/video/vga.c
patching file xen/include/xen/pci.h
And, when building:
gcc -O2 -fomit-frame-pointer -m64 -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -DNDEBUG -I/home/SOURCES/xen/xen/xen.git/xen/include -I/home/SOURCES/xen/xen/xen.git/xen/include/asm-x86/mach-generic -I/home/SOURCES/xen/xen/xen.git/xen/include/asm-x86/mach-default '-D__OBJECT_LABEL__=drivers$passthrough$vtd$intremap.o' -msoft-float -fno-stack-protector -fno-exceptions -Wnested-externs -DHAVE_GAS_VMX -DHAVE_GAS_EPT -DHAVE_GAS_FSGSBASE -U__OBJECT_LABEL__ -DHAVE_GAS_QUOTED_SYM '-D__OBJECT_LABEL__=drivers/passthrough/vtd/intremap.o' -mno-red-zone -mno-sse -fpic -fno-asynchronous-unwind-tables -DGCC_HAS_VISIBILITY_ATTRIBUTE -nostdinc -fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith -pipe -g -D__XEN__ -include /home/SOURCES/xen/xen/xen.git/xen/include/xen/config.h '-D__OBJECT_FILE__="intremap.o"' -DPERF_COUNTERS -DPERF_ARRAYS -MMD -MF ./.intremap.o.d -c intremap.c -o intremap.o
In file included from /home/SOURCES/xen/xen/xen.git/xen/include/xen/bitmap.h:6:0,
from /home/SOURCES/xen/xen/xen.git/xen/include/xen/cpumask.h:78,
from /home/SOURCES/xen/xen/xen.git/xen/include/xen/irq.h:4,
from intremap.c:20:
intremap.c: In function 'pi_update_irte':
intremap.c:987:27: error: passing argument 1 of '_spin_is_locked' from incompatible pointer type [-Werror]
ASSERT(spin_is_locked(&pcidevs_lock));
^
/home/SOURCES/xen/xen/xen.git/xen/include/xen/lib.h:35:35: note: in definition of macro 'ASSERT'
#define ASSERT(p) do { if ( 0 && (p) ) {} } while (0)
^
intremap.c:987:12: note: in expansion of macro 'spin_is_locked'
ASSERT(spin_is_locked(&pcidevs_lock));
^
In file included from /home/SOURCES/xen/xen/xen.git/xen/include/xen/rcupdate.h:35:0,
from /home/SOURCES/xen/xen/xen.git/xen/include/xen/irq.h:5,
from intremap.c:20:
/home/SOURCES/xen/xen/xen.git/xen/include/xen/spinlock.h:163:5: note: expected 'struct spinlock_t *' but argument is of type 'void (*)(void)'
int _spin_is_locked(spinlock_t *lock);
So, I think a refresh is necessary.
Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] IOMMU/spinlock: Fix a bug found in AMD IOMMU initialization.
2016-03-08 12:12 ` Dario Faggioli
@ 2016-03-08 12:35 ` Xu, Quan
0 siblings, 0 replies; 10+ messages in thread
From: Xu, Quan @ 2016-03-08 12:35 UTC (permalink / raw)
To: Dario Faggioli, xen-devel@lists.xen.org
Cc: Jan Beulich, Suravee Suthikulpanit
On March 08, 2016 8:13pm, <dario.faggioli@citrix.com> wrote:
> On Tue, 2016-03-08 at 19:09 +0800, Quan Xu wrote:
> > Doing what we do serves as a fix for a bug found in AMD IOMMU
> > initialization.
> >
> This first line is rather useless, if not worse. :-)
>
I will remove it in next v2. :)
> I don't know (provided a new version is not necessary, and provided maintainers
> agree with me :-)) whether you need to repost or it can be removed when code
> is committed.
>
I think I'd better send out v2. :)
> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> > CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> > CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> >
> get_maintainer.pl gives me only Suravee, as Aravind stepped down a few days
> ago, so he shouldn't be bothered (and, in fact, I'm moving him from Cc to Bcc).
>
Got it, thanks for your advice.
> > CC: Dario Faggioli <dario.faggioli@citrix.com>
> > CC: Jan Beulich <jbeulich@suse.com>
> >
> (BTW, it's of course fine to include me and Jan, despite what get_maintainer.pl's
> output, as we've been involved in previous rounds of review.)
>
> All that being said:
>
> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
>
Dario, thanks.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] IOMMU/spinlock: Make the pcidevs_lock a recursive one
2016-03-08 12:29 ` Dario Faggioli
@ 2016-03-08 12:39 ` Xu, Quan
2016-03-08 13:48 ` Jan Beulich
0 siblings, 1 reply; 10+ messages in thread
From: Xu, Quan @ 2016-03-08 12:39 UTC (permalink / raw)
To: Dario Faggioli, xen-devel@lists.xen.org
Cc: Tian, Kevin, Wu, Feng, Jan Beulich, Andrew Cooper,
Suravee Suthikulpanit, Keir Fraser
On March 08, 2016 8:29pm, <dario.faggioli@citrix.com> wrote:
> On Tue, 2016-03-08 at 19:09 +0800, Quan Xu wrote:
> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> > CC: Keir Fraser <keir@xen.org>
> > CC: Jan Beulich <jbeulich@suse.com>
> > CC: Andrew Cooper <andrew.cooper3@citrix.com>
> > CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> > CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> > CC: Feng Wu <feng.wu@intel.com>
> > CC: Kevin Tian <kevin.tian@intel.com>
> > CC: Dario Faggioli <dario.faggioli@citrix.com>
> >
> I've gone through the code, and it looks fine.
>
> However, when trying to apply the patch, on top of this morning's staging, I got
> this:
>
Oh, sorry, it is not against this morning's staging.
I would try to send out patch against this morning's staging soon. Thanks.
-Quan
> [dario@Solace xen.git] $ patch -p1 <
> \[PATCH_2_2\]_IOMMU_spinlock\:_Make_the_pcidevs_lock_a_recursive_one.
> mbox
> patching file xen/arch/x86/domctl.c
> Hunk #1 succeeded at 472 (offset 45 lines).
> Hunk #2 succeeded at 497 (offset 45 lines).
> patching file xen/arch/x86/hvm/vmsi.c
> Hunk #1 succeeded at 388 with fuzz 1.
> Hunk #2 succeeded at 446 with fuzz 1 (offset 3 lines).
> patching file xen/arch/x86/irq.c
> Hunk #1 succeeded at 1960 (offset 5 lines).
> Hunk #2 succeeded at 2105 (offset 5 lines).
> Hunk #3 succeeded at 2231 (offset 5 lines).
> Hunk #4 succeeded at 2239 (offset 5 lines).
> patching file xen/arch/x86/msi.c
> patching file xen/arch/x86/pci.c
> Hunk #1 succeeded at 88 (offset 6 lines).
> patching file xen/arch/x86/physdev.c
> patching file xen/common/sysctl.c
> patching file xen/drivers/passthrough/amd/iommu_init.c
> patching file xen/drivers/passthrough/amd/iommu_map.c
> patching file xen/drivers/passthrough/amd/pci_amd_iommu.c
> patching file xen/drivers/passthrough/pci.c Hunk #17 succeeded at 1226 with
> fuzz 1.
> Hunk #18 succeeded at 1262 (offset -6 lines).
> Hunk #19 succeeded at 1291 (offset -6 lines).
> Hunk #20 succeeded at 1340 (offset -6 lines).
> Hunk #21 succeeded at 1364 (offset -6 lines).
> Hunk #22 succeeded at 1401 (offset -6 lines).
> Hunk #23 succeeded at 1416 (offset -6 lines).
> Hunk #24 succeeded at 1471 (offset -6 lines).
> Hunk #25 succeeded at 1490 (offset -6 lines).
> Hunk #26 succeeded at 1625 (offset -6 lines).
> patching file xen/drivers/passthrough/vtd/iommu.c
> Hunk #1 succeeded at 1282 (offset -4 lines).
> Hunk #2 succeeded at 1424 (offset -4 lines).
> Hunk #3 succeeded at 1506 (offset -4 lines).
> Hunk #4 succeeded at 1816 (offset -4 lines).
> Hunk #5 succeeded at 1881 (offset -4 lines).
> Hunk #6 succeeded at 2109 (offset -4 lines).
> Hunk #7 succeeded at 2123 (offset -4 lines).
> patching file xen/drivers/video/vga.c
> patching file xen/include/xen/pci.h
>
> And, when building:
>
> gcc -O2 -fomit-frame-pointer -m64 -fno-strict-aliasing -std=gnu99 -Wall
> -Wstrict-prototypes -Wdeclaration-after-statement
> -Wno-unused-but-set-variable -Wno-unused-local-typedefs -DNDEBUG
> -I/home/SOURCES/xen/xen/xen.git/xen/include
> -I/home/SOURCES/xen/xen/xen.git/xen/include/asm-x86/mach-generic
> -I/home/SOURCES/xen/xen/xen.git/xen/include/asm-x86/mach-default
> '-D__OBJECT_LABEL__=drivers$passthrough$vtd$intremap.o' -msoft-float
> -fno-stack-protector -fno-exceptions -Wnested-externs -DHAVE_GAS_VMX
> -DHAVE_GAS_EPT -DHAVE_GAS_FSGSBASE -U__OBJECT_LABEL__
> -DHAVE_GAS_QUOTED_SYM
> '-D__OBJECT_LABEL__=drivers/passthrough/vtd/intremap.o' -mno-red-zone
> -mno-sse -fpic -fno-asynchronous-unwind-tables
> -DGCC_HAS_VISIBILITY_ATTRIBUTE -nostdinc -fno-builtin -fno-common
> -Werror -Wredundant-decls -Wno-pointer-arith -pipe -g -D__XEN__ -include
> /home/SOURCES/xen/xen/xen.git/xen/include/xen/config.h
> '-D__OBJECT_FILE__="intremap.o"' -DPERF_COUNTERS -DPERF_ARRAYS -MMD
> -MF ./.intremap.o.d -c intremap.c -o intremap.o In file included from
> /home/SOURCES/xen/xen/xen.git/xen/include/xen/bitmap.h:6:0,
> from
> /home/SOURCES/xen/xen/xen.git/xen/include/xen/cpumask.h:78,
> from
> /home/SOURCES/xen/xen/xen.git/xen/include/xen/irq.h:4,
> from intremap.c:20:
> intremap.c: In function 'pi_update_irte':
> intremap.c:987:27: error: passing argument 1 of '_spin_is_locked' from
> incompatible pointer type [-Werror]
> ASSERT(spin_is_locked(&pcidevs_lock));
> ^
> /home/SOURCES/xen/xen/xen.git/xen/include/xen/lib.h:35:35: note: in
> definition of macro 'ASSERT'
> #define ASSERT(p) do { if ( 0 && (p) ) {} } while (0)
> ^
> intremap.c:987:12: note: in expansion of macro 'spin_is_locked'
> ASSERT(spin_is_locked(&pcidevs_lock));
> ^
> In file included from
> /home/SOURCES/xen/xen/xen.git/xen/include/xen/rcupdate.h:35:0,
> from
> /home/SOURCES/xen/xen/xen.git/xen/include/xen/irq.h:5,
> from intremap.c:20:
> /home/SOURCES/xen/xen/xen.git/xen/include/xen/spinlock.h:163:5: note:
> expected 'struct spinlock_t *' but argument is of type 'void (*)(void)'
> int _spin_is_locked(spinlock_t *lock);
>
> So, I think a refresh is necessary.
>
> Regards,
> Dario
> --
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer,
> Citrix Systems R&D Ltd., Cambridge (UK)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] IOMMU/spinlock: Make the pcidevs_lock a recursive one
2016-03-08 12:39 ` Xu, Quan
@ 2016-03-08 13:48 ` Jan Beulich
2016-03-08 13:58 ` Xu, Quan
0 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2016-03-08 13:48 UTC (permalink / raw)
To: Quan Xu
Cc: Kevin Tian, Feng Wu, Andrew Cooper, Dario Faggioli,
xen-devel@lists.xen.org, Suravee Suthikulpanit, Keir Fraser
>>> On 08.03.16 at 13:39, <quan.xu@intel.com> wrote:
> On March 08, 2016 8:29pm, <dario.faggioli@citrix.com> wrote:
>> On Tue, 2016-03-08 at 19:09 +0800, Quan Xu wrote:
>> > Signed-off-by: Quan Xu <quan.xu@intel.com>
>> > CC: Keir Fraser <keir@xen.org>
>> > CC: Jan Beulich <jbeulich@suse.com>
>> > CC: Andrew Cooper <andrew.cooper3@citrix.com>
>> > CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> > CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>> > CC: Feng Wu <feng.wu@intel.com>
>> > CC: Kevin Tian <kevin.tian@intel.com>
>> > CC: Dario Faggioli <dario.faggioli@citrix.com>
>> >
>> I've gone through the code, and it looks fine.
>>
>> However, when trying to apply the patch, on top of this morning's staging, I got
>> this:
>>
> Oh, sorry, it is not against this morning's staging.
> I would try to send out patch against this morning's staging soon. Thanks.
Well, with e.g.
>> [dario@Solace xen.git] $ patch -p1 <
>> \[PATCH_2_2\]_IOMMU_spinlock\:_Make_the_pcidevs_lock_a_recursive_one.
>> mbox
>> patching file xen/arch/x86/domctl.c
>> Hunk #1 succeeded at 472 (offset 45 lines).
>> Hunk #2 succeeded at 497 (offset 45 lines).
>> patching file xen/arch/x86/hvm/vmsi.c
>> Hunk #1 succeeded at 388 with fuzz 1.
>> Hunk #2 succeeded at 446 with fuzz 1 (offset 3 lines).
... this it must have been quite old a tree - this file didn't change
in the last 4 months. I consider it rather unfriendly to post such
a patch without RFC tag, and without stating that it's against a
stale tree. Was the recent v6 of the 5-patch series this way too?
If so, I'm glad I didn't spend time looking at it yet.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] IOMMU/spinlock: Fix a bug found in AMD IOMMU initialization.
2016-03-08 11:09 ` [PATCH 1/2] IOMMU/spinlock: Fix a bug found in AMD IOMMU initialization Quan Xu
2016-03-08 12:12 ` Dario Faggioli
@ 2016-03-08 13:54 ` Jan Beulich
1 sibling, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2016-03-08 13:54 UTC (permalink / raw)
To: Quan Xu; +Cc: Dario Faggioli, Suravee Suthikulpanit, xen-devel
>>> On 08.03.16 at 12:09, <quan.xu@intel.com> wrote:
> Doing what we do serves as a fix for a bug found in AMD IOMMU initialization.
>
> The current code is using spin_lock{_irqsave(), _irqrestore()} to
> protect pci_get_dev() in the set_iommu_interrupt_handler(). However,
> this can only be called during AMD IOMMU initialization, with interrupt
> enabled, so at least it is not necessary to disable interrupts, or
> save/restore interrupt flag.
On top of what Dario said: This description isn't very accurate either:
If the code in question runs with interrupts enabled (which I'm not
sure it does; would take me following back up the call chain to see
whether this happens before or after interrupts get enabled for the
first time), then it may very well be necessary to disable interrupts
for a particular purpose - from an abstract pov. That's not the case
here, but the description of such a change shouldn't make incorrect
claims or statements.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] IOMMU/spinlock: Make the pcidevs_lock a recursive one
2016-03-08 13:48 ` Jan Beulich
@ 2016-03-08 13:58 ` Xu, Quan
0 siblings, 0 replies; 10+ messages in thread
From: Xu, Quan @ 2016-03-08 13:58 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, Wu, Feng, Andrew Cooper, Dario Faggioli,
xen-devel@lists.xen.org, Suravee Suthikulpanit, Keir Fraser
On March 08, 2016 9:49pm, <JBeulich@suse.com> wrote:
> >>> On 08.03.16 at 13:39, <quan.xu@intel.com> wrote:
> > On March 08, 2016 8:29pm, <dario.faggioli@citrix.com> wrote:
> >> On Tue, 2016-03-08 at 19:09 +0800, Quan Xu wrote:
> >> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> >> > CC: Keir Fraser <keir@xen.org>
> >> > CC: Jan Beulich <jbeulich@suse.com>
> >> > CC: Andrew Cooper <andrew.cooper3@citrix.com>
> >> > CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> >> > CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> >> > CC: Feng Wu <feng.wu@intel.com>
> >> > CC: Kevin Tian <kevin.tian@intel.com>
> >> > CC: Dario Faggioli <dario.faggioli@citrix.com>
> >> >
> >> I've gone through the code, and it looks fine.
> >>
> >> However, when trying to apply the patch, on top of this morning's
> >> staging, I got
> >> this:
> >>
> > Oh, sorry, it is not against this morning's staging.
> > I would try to send out patch against this morning's staging soon. Thanks.
>
> Well, with e.g.
>
> >> [dario@Solace xen.git] $ patch -p1 <
> >>
> \[PATCH_2_2\]_IOMMU_spinlock\:_Make_the_pcidevs_lock_a_recursive_one.
> >> mbox
> >> patching file xen/arch/x86/domctl.c
> >> Hunk #1 succeeded at 472 (offset 45 lines).
> >> Hunk #2 succeeded at 497 (offset 45 lines).
> >> patching file xen/arch/x86/hvm/vmsi.c Hunk #1 succeeded at 388 with
> >> fuzz 1.
> >> Hunk #2 succeeded at 446 with fuzz 1 (offset 3 lines).
>
> ... this it must have been quite old a tree - this file didn't change in the last 4
> months. I consider it rather unfriendly to post such a patch without RFC tag, and
> without stating that it's against a stale tree. Was the recent v6 of the 5-patch
> series this way too?
Yes,
sorry, I didn't rebase since from v1. :(:(
I will try to rebase against current staging and send out new patch sets.
Quan
> If so, I'm glad I didn't spend time looking at it yet.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-03-08 13:58 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-08 11:09 [PATCH 0/2] Make the pcidevs_lock a recursive one Quan Xu
2016-03-08 11:09 ` [PATCH 1/2] IOMMU/spinlock: Fix a bug found in AMD IOMMU initialization Quan Xu
2016-03-08 12:12 ` Dario Faggioli
2016-03-08 12:35 ` Xu, Quan
2016-03-08 13:54 ` Jan Beulich
2016-03-08 11:09 ` [PATCH 2/2] IOMMU/spinlock: Make the pcidevs_lock a recursive one Quan Xu
2016-03-08 12:29 ` Dario Faggioli
2016-03-08 12:39 ` Xu, Quan
2016-03-08 13:48 ` Jan Beulich
2016-03-08 13:58 ` Xu, Quan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).