Cui, Dexuan
2008-Sep-26 04:44 UTC
[Xen-devel] RE: [Xen-changelog] [xen-unstable] x86: Properly synchronise updates to pirq-to-vector mapping.
@@ -491,16 +512,15 @@ int pirq_guest_bind(struct vcpu *v, int
int rc = 0;
cpumask_t cpumask = CPU_MASK_NONE;
+ WARN_ON(!spin_is_locked(&v->domain->evtchn_lock));
I find this WARN_ON() is triggered harmlessly when I assign device to HVM guest.
The call trace is XEN_DOMCTL_bind_pt_irq -> pt_irq_create_bind_vtd() ->
pirq_guest_bind().
Should we remove the WARN_ON here?
Thanks,
-- Dexuan
-----Original Message-----
From: xen-changelog-bounces@lists.xensource.com
[mailto:xen-changelog-bounces@lists.xensource.com] On Behalf Of Xen
patchbot-unstable
Sent: 2008年9月26日 11:20
To: xen-changelog@lists.xensource.com
Subject: [Xen-changelog] [xen-unstable] x86: Properly synchronise updates to
pirq-to-vector mapping.
# HG changeset patch
# User Keir Fraser <keir.fraser@citrix.com>
# Date 1222256215 -3600
# Node ID 31f09a5e24cf8eb8a9d73acc6c23262fe9d463d7
# Parent 7750906b06b3ebbba529e6d1042d7a2a2712623c
x86: Properly synchronise updates to pirq-to-vector mapping.
Per-domain irq mappings are now protected by d->evtchn_lock and by the
per-vector irq_desc lock.
Signed-off-by: Jan Beulich <jbeulich@novell.com>
Signed-off-by: Keir Fraser <keir.fraser@citrix.com>
---
xen/arch/ia64/xen/irq.c | 16 ++++--
xen/arch/x86/domain.c | 2
xen/arch/x86/io_apic.c | 11 ----
xen/arch/x86/irq.c | 112 ++++++++++++++++++++++++++++---------------
xen/arch/x86/msi.c | 1
xen/arch/x86/physdev.c | 96 +++++++++++++++++++-----------------
xen/common/event_channel.c | 3 -
xen/include/asm-x86/domain.h | 2
xen/include/asm-x86/irq.h | 5 +
xen/include/asm-x86/msi.h | 2
xen/include/xen/irq.h | 3 -
11 files changed, 142 insertions(+), 111 deletions(-)
diff -r 7750906b06b3 -r 31f09a5e24cf xen/arch/ia64/xen/irq.c
--- a/xen/arch/ia64/xen/irq.c Wed Sep 24 10:23:51 2008 +0100
+++ b/xen/arch/ia64/xen/irq.c Wed Sep 24 12:36:55 2008 +0100
@@ -459,20 +459,24 @@ int pirq_guest_bind(struct vcpu *v, int
return rc;
}
-void pirq_guest_unbind(struct domain *d, int irq)
+int pirq_guest_unbind(struct domain *d, int irq)
{
irq_desc_t *desc = &irq_desc[irq];
irq_guest_action_t *action;
unsigned long flags;
- int i;
+ int i, rc = 0;
spin_lock_irqsave(&desc->lock, flags);
action = (irq_guest_action_t *)desc->action;
- i = 0;
- while ( action->guest[i] && (action->guest[i] != d) )
- i++;
+ for ( i = 0; (i < action->nr_guests) && (action->guest[i]
!= d); i++ )
+ continue;
+ if ( i == action->nr_guests )
+ {
+ rc = -EINVAL;
+ goto out;
+ }
memmove(&action->guest[i], &action->guest[i+1],
IRQ_MAX_GUESTS-i-1);
action->nr_guests--;
@@ -492,7 +496,9 @@ void pirq_guest_unbind(struct domain *d,
desc->handler->shutdown(irq);
}
+ out:
spin_unlock_irqrestore(&desc->lock, flags);
+ return rc;
}
void
diff -r 7750906b06b3 -r 31f09a5e24cf xen/arch/x86/domain.c
--- a/xen/arch/x86/domain.c Wed Sep 24 10:23:51 2008 +0100
+++ b/xen/arch/x86/domain.c Wed Sep 24 12:36:55 2008 +0100
@@ -414,8 +414,6 @@ int arch_domain_create(struct domain *d,
goto fail;
}
- spin_lock_init(&d->arch.irq_lock);
-
if ( is_hvm_domain(d) )
{
if ( (rc = hvm_domain_initialise(d)) != 0 )
diff -r 7750906b06b3 -r 31f09a5e24cf xen/arch/x86/io_apic.c
--- a/xen/arch/x86/io_apic.c Wed Sep 24 10:23:51 2008 +0100
+++ b/xen/arch/x86/io_apic.c Wed Sep 24 12:36:55 2008 +0100
@@ -45,16 +45,6 @@ int (*ioapic_renumber_irq)(int ioapic, i
int (*ioapic_renumber_irq)(int ioapic, int irq);
atomic_t irq_mis_count;
-int domain_irq_to_vector(struct domain *d, int irq)
-{
- return d->arch.pirq_vector[irq];
-}
-
-int domain_vector_to_irq(struct domain *d, int vector)
-{
- return d->arch.vector_pirq[vector];
-}
-
/* Where if anywhere is the i8259 connect in external int mode */
static struct { int pin, apic; } ioapic_i8259 = { -1, -1 };
@@ -721,7 +711,6 @@ next:
static struct hw_interrupt_type ioapic_level_type;
static struct hw_interrupt_type ioapic_edge_type;
-struct hw_interrupt_type pci_msi_type;
#define IOAPIC_AUTO -1
#define IOAPIC_EDGE 0
diff -r 7750906b06b3 -r 31f09a5e24cf xen/arch/x86/irq.c
--- a/xen/arch/x86/irq.c Wed Sep 24 10:23:51 2008 +0100
+++ b/xen/arch/x86/irq.c Wed Sep 24 12:36:55 2008 +0100
@@ -277,6 +277,35 @@ static void __do_IRQ_guest(int vector)
}
}
+/*
+ * Retrieve Xen irq-descriptor corresponding to a domain-specific irq.
+ * The descriptor is returned locked. This function is safe against changes
+ * to the per-domain irq-to-vector mapping.
+ */
+static irq_desc_t *domain_spin_lock_irq_desc(
+ struct domain *d, int irq, unsigned long *pflags)
+{
+ unsigned int vector;
+ unsigned long flags;
+ irq_desc_t *desc;
+
+ for ( ; ; )
+ {
+ vector = domain_irq_to_vector(d, irq);
+ if ( vector <= 0 )
+ return NULL;
+ desc = &irq_desc[vector];
+ spin_lock_irqsave(&desc->lock, flags);
+ if ( vector == domain_irq_to_vector(d, irq) )
+ break;
+ spin_unlock_irqrestore(&desc->lock, flags);
+ }
+
+ if ( pflags != NULL )
+ *pflags = flags;
+ return desc;
+}
+
/* Flush all ready EOIs from the top of this CPU's pending-EOI stack. */
static void flush_ready_eoi(void *unused)
{
@@ -342,11 +371,13 @@ static void __pirq_guest_eoi(struct doma
cpumask_t cpu_eoi_map;
int vector;
- vector = domain_irq_to_vector(d, irq);
- desc = &irq_desc[vector];
+ ASSERT(local_irq_is_enabled());
+ desc = domain_spin_lock_irq_desc(d, irq, NULL);
+ if ( desc == NULL )
+ return;
+
action = (irq_guest_action_t *)desc->action;
-
- spin_lock_irq(&desc->lock);
+ vector = desc - irq_desc;
ASSERT(!test_bit(irq, d->pirq_mask) ||
(action->ack_type != ACKTYPE_NONE));
@@ -418,7 +449,7 @@ int pirq_acktype(struct domain *d, int i
unsigned int vector;
vector = domain_irq_to_vector(d, irq);
- if ( vector == 0 )
+ if ( vector <= 0 )
return ACKTYPE_NONE;
desc = &irq_desc[vector];
@@ -447,13 +478,6 @@ int pirq_acktype(struct domain *d, int i
if ( !strcmp(desc->handler->typename, "XT-PIC") )
return ACKTYPE_UNMASK;
- if ( strstr(desc->handler->typename, "MPIC") )
- {
- if ( desc->status & IRQ_LEVEL )
- return (desc->status & IRQ_PER_CPU) ? ACKTYPE_EOI :
ACKTYPE_UNMASK;
- return ACKTYPE_NONE; /* edge-triggered => no final EOI */
- }
-
printk("Unknown PIC type '%s' for IRQ %d\n",
desc->handler->typename, irq);
BUG();
@@ -462,21 +486,18 @@ int pirq_acktype(struct domain *d, int i
int pirq_shared(struct domain *d, int irq)
{
- unsigned int vector;
irq_desc_t *desc;
irq_guest_action_t *action;
unsigned long flags;
int shared;
- vector = domain_irq_to_vector(d, irq);
- if ( vector == 0 )
+ desc = domain_spin_lock_irq_desc(d, irq, &flags);
+ if ( desc == NULL )
return 0;
- desc = &irq_desc[vector];
-
- spin_lock_irqsave(&desc->lock, flags);
action = (irq_guest_action_t *)desc->action;
shared = ((desc->status & IRQ_GUEST) &&
(action->nr_guests > 1));
+
spin_unlock_irqrestore(&desc->lock, flags);
return shared;
@@ -491,16 +512,15 @@ int pirq_guest_bind(struct vcpu *v, int
int rc = 0;
cpumask_t cpumask = CPU_MASK_NONE;
+ WARN_ON(!spin_is_locked(&v->domain->evtchn_lock));
+
retry:
- vector = domain_irq_to_vector(v->domain, irq);
- if ( vector == 0 )
+ desc = domain_spin_lock_irq_desc(v->domain, irq, &flags);
+ if ( desc == NULL )
return -EINVAL;
- desc = &irq_desc[vector];
-
- spin_lock_irqsave(&desc->lock, flags);
-
action = (irq_guest_action_t *)desc->action;
+ vector = desc - irq_desc;
if ( !(desc->status & IRQ_GUEST) )
{
@@ -575,26 +595,39 @@ int pirq_guest_bind(struct vcpu *v, int
return rc;
}
-void pirq_guest_unbind(struct domain *d, int irq)
-{
- unsigned int vector;
+int pirq_guest_unbind(struct domain *d, int irq)
+{
+ int vector;
irq_desc_t *desc;
irq_guest_action_t *action;
cpumask_t cpu_eoi_map;
unsigned long flags;
- int i;
-
- vector = domain_irq_to_vector(d, irq);
- desc = &irq_desc[vector];
- BUG_ON(vector == 0);
-
- spin_lock_irqsave(&desc->lock, flags);
+ int i, rc = 0;
+
+ WARN_ON(!spin_is_locked(&d->evtchn_lock));
+
+ desc = domain_spin_lock_irq_desc(d, irq, &flags);
+ if ( unlikely(desc == NULL) )
+ {
+ if ( (vector = -domain_irq_to_vector(d, irq)) == 0 )
+ return -EINVAL;
+ BUG_ON(vector <= 0);
+ desc = &irq_desc[vector];
+ spin_lock_irqsave(&desc->lock, flags);
+ d->arch.pirq_vector[irq] = d->arch.vector_pirq[vector] = 0;
+ goto out;
+ }
action = (irq_guest_action_t *)desc->action;
-
- i = 0;
- while ( action->guest[i] && (action->guest[i] != d) )
- i++;
+ vector = desc - irq_desc;
+
+ for ( i = 0; (i < action->nr_guests) && (action->guest[i]
!= d); i++ )
+ continue;
+ if ( i == action->nr_guests )
+ {
+ rc = -EINVAL;
+ goto out;
+ }
memmove(&action->guest[i], &action->guest[i+1],
IRQ_MAX_GUESTS-i-1);
action->nr_guests--;
@@ -661,7 +694,8 @@ void pirq_guest_unbind(struct domain *d,
desc->handler->shutdown(vector);
out:
- spin_unlock_irqrestore(&desc->lock, flags);
+ spin_unlock_irqrestore(&desc->lock, flags);
+ return rc;
}
extern void dump_ioapic_irq_info(void);
diff -r 7750906b06b3 -r 31f09a5e24cf xen/arch/x86/msi.c
--- a/xen/arch/x86/msi.c Wed Sep 24 10:23:51 2008 +0100
+++ b/xen/arch/x86/msi.c Wed Sep 24 12:36:55 2008 +0100
@@ -727,7 +727,6 @@ void pci_disable_msi(int vector)
__pci_disable_msix(vector);
}
-extern struct hw_interrupt_type pci_msi_type;
static void msi_free_vectors(struct pci_dev* dev)
{
struct msi_desc *entry, *tmp;
diff -r 7750906b06b3 -r 31f09a5e24cf xen/arch/x86/physdev.c
--- a/xen/arch/x86/physdev.c Wed Sep 24 10:23:51 2008 +0100
+++ b/xen/arch/x86/physdev.c Wed Sep 24 12:36:55 2008 +0100
@@ -26,17 +26,11 @@ ioapic_guest_write(
ioapic_guest_write(
unsigned long physbase, unsigned int reg, u32 pval);
-
-extern struct hw_interrupt_type pci_msi_type;
-
static int get_free_pirq(struct domain *d, int type, int index)
{
int i;
- if ( d == NULL )
- return -EINVAL;
-
- ASSERT(spin_is_locked(&d->arch.irq_lock));
+ ASSERT(spin_is_locked(&d->evtchn_lock));
if ( type == MAP_PIRQ_TYPE_GSI )
{
@@ -64,11 +58,10 @@ static int map_domain_pirq(struct domain
int ret = 0;
int old_vector, old_pirq;
struct msi_info msi;
-
- if ( d == NULL )
- return -EINVAL;
-
- ASSERT(spin_is_locked(&d->arch.irq_lock));
+ irq_desc_t *desc;
+ unsigned long flags;
+
+ ASSERT(spin_is_locked(&d->evtchn_lock));
if ( !IS_PRIV(current->domain) )
return -EPERM;
@@ -88,8 +81,7 @@ static int map_domain_pirq(struct domain
{
dprintk(XENLOG_G_ERR, "dom%d: pirq %d or vector %d already
mapped\n",
d->domain_id, pirq, vector);
- ret = -EINVAL;
- goto done;
+ return -EINVAL;
}
ret = irq_permit_access(d, pirq);
@@ -97,17 +89,14 @@ static int map_domain_pirq(struct domain
{
dprintk(XENLOG_G_ERR, "dom%d: could not permit access to irq
%d\n",
d->domain_id, pirq);
- goto done;
- }
+ return ret;
+ }
+
+ desc = &irq_desc[vector];
+ spin_lock_irqsave(&desc->lock, flags);
if ( map && MAP_PIRQ_TYPE_MSI == map->type )
{
- irq_desc_t *desc;
- unsigned long flags;
-
- desc = &irq_desc[vector];
-
- spin_lock_irqsave(&desc->lock, flags);
if ( desc->handler != &no_irq_type )
dprintk(XENLOG_G_ERR, "dom%d: vector %d in use\n",
d->domain_id, vector);
@@ -120,8 +109,6 @@ static int map_domain_pirq(struct domain
msi.vector = vector;
ret = pci_enable_msi(&msi);
-
- spin_unlock_irqrestore(&desc->lock, flags);
if ( ret )
goto done;
}
@@ -130,6 +117,7 @@ static int map_domain_pirq(struct domain
d->arch.vector_pirq[vector] = pirq;
done:
+ spin_unlock_irqrestore(&desc->lock, flags);
return ret;
}
@@ -139,18 +127,18 @@ static int unmap_domain_pirq(struct doma
unsigned long flags;
irq_desc_t *desc;
int vector, ret = 0;
-
- if ( d == NULL || pirq < 0 || pirq >= NR_PIRQS )
+ bool_t forced_unbind;
+
+ if ( (pirq < 0) || (pirq >= NR_PIRQS) )
return -EINVAL;
if ( !IS_PRIV(current->domain) )
return -EINVAL;
- ASSERT(spin_is_locked(&d->arch.irq_lock));
+ ASSERT(spin_is_locked(&d->evtchn_lock));
vector = d->arch.pirq_vector[pirq];
-
- if ( !vector )
+ if ( vector <= 0 )
{
dprintk(XENLOG_G_ERR, "dom%d: pirq %d not mapped\n",
d->domain_id, pirq);
@@ -158,20 +146,34 @@ static int unmap_domain_pirq(struct doma
goto done;
}
+ forced_unbind = (pirq_guest_unbind(d, pirq) == 0);
+ if ( forced_unbind )
+ dprintk(XENLOG_G_WARNING, "dom%d: forcing unbind of pirq
%d\n",
+ d->domain_id, pirq);
+
desc = &irq_desc[vector];
spin_lock_irqsave(&desc->lock, flags);
+
+ BUG_ON(vector != d->arch.pirq_vector[pirq]);
+
if ( desc->msi_desc )
pci_disable_msi(vector);
if ( desc->handler == &pci_msi_type )
- {
- /* MSI is not shared, so should be released already */
- BUG_ON(desc->status & IRQ_GUEST);
- irq_desc[vector].handler = &no_irq_type;
- }
+ desc->handler = &no_irq_type;
+
+ if ( !forced_unbind )
+ {
+ d->arch.pirq_vector[pirq] = 0;
+ d->arch.vector_pirq[vector] = 0;
+ }
+ else
+ {
+ d->arch.pirq_vector[pirq] = -vector;
+ d->arch.vector_pirq[vector] = -pirq;
+ }
+
spin_unlock_irqrestore(&desc->lock, flags);
-
- d->arch.pirq_vector[pirq] = d->arch.vector_pirq[vector] = 0;
ret = irq_deny_access(d, pirq);
if ( ret )
@@ -186,7 +188,6 @@ static int physdev_map_pirq(struct physd
{
struct domain *d;
int vector, pirq, ret = 0;
- unsigned long flags;
if ( !IS_PRIV(current->domain) )
return -EPERM;
@@ -243,8 +244,8 @@ static int physdev_map_pirq(struct physd
goto free_domain;
}
- spin_lock_irqsave(&d->arch.irq_lock, flags);
- if ( map->pirq == -1 )
+ spin_lock(&d->evtchn_lock);
+ if ( map->pirq < 0 )
{
if ( d->arch.vector_pirq[vector] )
{
@@ -252,6 +253,11 @@ static int physdev_map_pirq(struct physd
d->domain_id, map->index, map->pirq,
d->arch.vector_pirq[vector]);
pirq = d->arch.vector_pirq[vector];
+ if ( pirq < 0 )
+ {
+ ret = -EBUSY;
+ goto done;
+ }
}
else
{
@@ -284,7 +290,7 @@ static int physdev_map_pirq(struct physd
if ( !ret )
map->pirq = pirq;
done:
- spin_unlock_irqrestore(&d->arch.irq_lock, flags);
+ spin_unlock(&d->evtchn_lock);
free_domain:
rcu_unlock_domain(d);
return ret;
@@ -293,7 +299,6 @@ static int physdev_unmap_pirq(struct phy
static int physdev_unmap_pirq(struct physdev_unmap_pirq *unmap)
{
struct domain *d;
- unsigned long flags;
int ret;
if ( !IS_PRIV(current->domain) )
@@ -307,9 +312,9 @@ static int physdev_unmap_pirq(struct phy
if ( d == NULL )
return -ESRCH;
- spin_lock_irqsave(&d->arch.irq_lock, flags);
+ spin_lock(&d->evtchn_lock);
ret = unmap_domain_pirq(d, unmap->pirq);
- spin_unlock_irqrestore(&d->arch.irq_lock, flags);
+ spin_unlock(&d->evtchn_lock);
rcu_unlock_domain(d);
@@ -416,7 +421,6 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
case PHYSDEVOP_alloc_irq_vector: {
struct physdev_irq irq_op;
- unsigned long flags;
ret = -EFAULT;
if ( copy_from_guest(&irq_op, arg, 1) != 0 )
@@ -437,9 +441,9 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
irq_op.vector = assign_irq_vector(irq);
- spin_lock_irqsave(&dom0->arch.irq_lock, flags);
+ spin_lock(&dom0->evtchn_lock);
ret = map_domain_pirq(dom0, irq_op.irq, irq_op.vector, NULL);
- spin_unlock_irqrestore(&dom0->arch.irq_lock, flags);
+ spin_unlock(&dom0->evtchn_lock);
if ( copy_to_guest(arg, &irq_op, 1) != 0 )
ret = -EFAULT;
diff -r 7750906b06b3 -r 31f09a5e24cf xen/common/event_channel.c
--- a/xen/common/event_channel.c Wed Sep 24 10:23:51 2008 +0100
+++ b/xen/common/event_channel.c Wed Sep 24 12:36:55 2008 +0100
@@ -387,7 +387,8 @@ static long __evtchn_close(struct domain
break;
case ECS_PIRQ:
- pirq_guest_unbind(d1, chn1->u.pirq);
+ if ( pirq_guest_unbind(d1, chn1->u.pirq) != 0 )
+ BUG();
d1->pirq_to_evtchn[chn1->u.pirq] = 0;
break;
diff -r 7750906b06b3 -r 31f09a5e24cf xen/include/asm-x86/domain.h
--- a/xen/include/asm-x86/domain.h Wed Sep 24 10:23:51 2008 +0100
+++ b/xen/include/asm-x86/domain.h Wed Sep 24 12:36:55 2008 +0100
@@ -235,7 +235,7 @@ struct arch_domain
/* Shadow translated domain: P2M mapping */
pagetable_t phys_table;
- spinlock_t irq_lock;
+ /* NB. protected by d->evtchn_lock and by irq_desc[vector].lock */
int vector_pirq[NR_VECTORS];
int pirq_vector[NR_PIRQS];
diff -r 7750906b06b3 -r 31f09a5e24cf xen/include/asm-x86/irq.h
--- a/xen/include/asm-x86/irq.h Wed Sep 24 10:23:51 2008 +0100
+++ b/xen/include/asm-x86/irq.h Wed Sep 24 12:36:55 2008 +0100
@@ -52,6 +52,7 @@ int pirq_acktype(struct domain *d, int i
int pirq_acktype(struct domain *d, int irq);
int pirq_shared(struct domain *d , int irq);
-extern int domain_irq_to_vector(struct domain *d, int irq);
-extern int domain_vector_to_irq(struct domain *d, int vector);
+#define domain_irq_to_vector(d, irq) ((d)->arch.pirq_vector[(irq)])
+#define domain_vector_to_irq(d, vec) ((d)->arch.vector_pirq[(vec)])
+
#endif /* _ASM_HW_IRQ_H */
diff -r 7750906b06b3 -r 31f09a5e24cf xen/include/asm-x86/msi.h
--- a/xen/include/asm-x86/msi.h Wed Sep 24 10:23:51 2008 +0100
+++ b/xen/include/asm-x86/msi.h Wed Sep 24 12:36:55 2008 +0100
@@ -106,7 +106,7 @@ struct msi_desc {
*/
#define NR_HP_RESERVED_VECTORS 20
-extern int vector_irq[NR_VECTORS];
+extern struct hw_interrupt_type pci_msi_type;
/*
* MSI-X Address Register
diff -r 7750906b06b3 -r 31f09a5e24cf xen/include/xen/irq.h
--- a/xen/include/xen/irq.h Wed Sep 24 10:23:51 2008 +0100
+++ b/xen/include/xen/irq.h Wed Sep 24 12:36:55 2008 +0100
@@ -22,7 +22,6 @@ struct irqaction
#define IRQ_PENDING 4 /* IRQ pending - replay on enable */
#define IRQ_REPLAY 8 /* IRQ has been replayed but not acked yet */
#define IRQ_GUEST 16 /* IRQ is handled by guest OS(es) */
-#define IRQ_LEVEL 64 /* IRQ level triggered */
#define IRQ_PER_CPU 256 /* IRQ is per CPU */
/*
@@ -78,7 +77,7 @@ extern int pirq_guest_eoi(struct domain
extern int pirq_guest_eoi(struct domain *d, int irq);
extern int pirq_guest_unmask(struct domain *d);
extern int pirq_guest_bind(struct vcpu *v, int irq, int will_share);
-extern void pirq_guest_unbind(struct domain *d, int irq);
+extern int pirq_guest_unbind(struct domain *d, int irq);
static inline void set_native_irq_info(int irq, cpumask_t mask)
{
_______________________________________________
Xen-changelog mailing list
Xen-changelog@lists.xensource.com
http://lists.xensource.com/xen-changelog
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Keir Fraser
2008-Sep-26 06:58 UTC
[Xen-devel] Re: [Xen-changelog] [xen-unstable] x86: Properly synchronise updates to pirq-to-vector mapping.
On 26/9/08 05:44, "Cui, Dexuan" <dexuan.cui@intel.com> wrote:> @@ -491,16 +512,15 @@ int pirq_guest_bind(struct vcpu *v, int > int rc = 0; > cpumask_t cpumask = CPU_MASK_NONE; > > + WARN_ON(!spin_is_locked(&v->domain->evtchn_lock)); > > I find this WARN_ON() is triggered harmlessly when I assign device to HVM > guest. > The call trace is XEN_DOMCTL_bind_pt_irq -> pt_irq_create_bind_vtd() -> > pirq_guest_bind(). > Should we remove the WARN_ON here?I put in that WARN_ON() deliberately, because I think HVM pt_irq locking needs some work. Yunhong Jiang is looking into it I believe (cc''ed). Obviously the current situation is temporary -- either the locking in the passthrough code will be fixed as I envisage, or we''ll agree some other fix and the WARN_ON() will be removed or changed. Appended is a relevant section of en email I sent a couple of days ago. -- Keir ------------------ * I decided to WARN_ON(!spin_is_locked(&d->evtchn_lock)) in pirq_guest_[un]bind(). The reason is that in any case those functions do not expect to be re-entered -- they really want to be per-domain serialised. Furthermore I am pretty certain that the HVM passthrough code is not synchronising properly with changes to the pirq-to-vector mapping (it uses domain_irq_to_vector() in many places with no care for locking) nor is it synchronised with other users of pirq_guest_bind() --- a reasonable semantics here would be that a domain pirq can be bound to once, either via an event channel, or through a virtual PIC in HVM emulation context. I therefore think that careful locking is required -- it may suffice to get rid of (or at least make less use of) the hvm_domain.irq_lock and replace its use with evtchn_lock (only consideration is that the latter is not IRQ-safe). The WARN_ON() is a nice reminder of work to be done here. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jiang, Yunhong
2008-Sep-26 09:34 UTC
[Xen-devel] RE: [Xen-changelog] [xen-unstable] x86: Properly synchronise updates to pirq-to-vector mapping.
Yes, I''m trying to fix this issue. Yunhong Jiang Keir Fraser <mailto:keir.fraser@eu.citrix.com> wrote:> On 26/9/08 05:44, "Cui, Dexuan" <dexuan.cui@intel.com> wrote: > >> @@ -491,16 +512,15 @@ int pirq_guest_bind(struct vcpu *v, int >> int rc = 0; >> cpumask_t cpumask = CPU_MASK_NONE; >> >> + WARN_ON(!spin_is_locked(&v->domain->evtchn_lock)); >> >> I find this WARN_ON() is triggered harmlessly when I assign device to HVM >> guest. The call trace is XEN_DOMCTL_bind_pt_irq -> >> pt_irq_create_bind_vtd() -> pirq_guest_bind(). Should we remove the >> WARN_ON here? > > I put in that WARN_ON() deliberately, because I think HVM > pt_irq locking > needs some work. Yunhong Jiang is looking into it I believe (cc''ed). > Obviously the current situation is temporary -- either the > locking in the > passthrough code will be fixed as I envisage, or we''ll agree > some other fix > and the WARN_ON() will be removed or changed. Appended is a > relevant section > of en email I sent a couple of days ago. > > -- Keir > > ------------------ > > * I decided to WARN_ON(!spin_is_locked(&d->evtchn_lock)) in > pirq_guest_[un]bind(). The reason is that in any case those > functions do not > expect to be re-entered -- they really want to be per-domain serialised. > Furthermore I am pretty certain that the HVM passthrough code is not > synchronising properly with changes to the pirq-to-vector > mapping (it uses > domain_irq_to_vector() in many places with no care for > locking) nor is it > synchronised with other users of pirq_guest_bind() --- a reasonable > semantics here would be that a domain pirq can be bound to > once, either via > an event channel, or through a virtual PIC in HVM emulation context. I > therefore think that careful locking is required -- it may > suffice to get > rid of (or at least make less use of) the hvm_domain.irq_lock > and replace > its use with evtchn_lock (only consideration is that the latter is not > IRQ-safe). The WARN_ON() is a nice reminder of work to be done here._______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Possibly Parallel Threads
- Xen crash on dom0 shutdown
- [patch] define and use cpu_test_and_clear() and some type checking fixes
- [PATCH v3] Clear IRQ_GUEST in irq_desc->status when setting action to NULL.
- [PATCH] ia64: fix build (next instance)
- [PATCH] avoid duplication of domain ID in messages