* [PATCH v10 01/10] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs
2014-08-05 11:25 [PATCH v10 00/10] gic and vgic fixes and improvements Stefano Stabellini
@ 2014-08-05 11:26 ` Stefano Stabellini
2014-08-05 11:26 ` [PATCH v10 02/10] xen/arm: move setting GIC_IRQ_GUEST_QUEUED earlier Stefano Stabellini
` (8 subsequent siblings)
9 siblings, 0 replies; 18+ messages in thread
From: Stefano Stabellini @ 2014-08-05 11:26 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, Ian.Campbell, Stefano Stabellini
vgic_enable_irqs should enable irq delivery to the vcpu specified by
GICD_ITARGETSR, rather than the vcpu that wrote to GICD_ISENABLER.
Similarly vgic_disable_irqs should use the target vcpu specified by
itarget to disable irqs.
itargets can be set to a mask but vgic_get_target_vcpu always returns
the lower vcpu in the mask.
Correctly initialize itargets for SPIs.
Ignore bits in GICD_ITARGETSR corresponding to invalid vcpus.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
Changes in v9:
- move vgic_get_target_vcpu declaration to vgic.h;
- move _vgic_get_target_vcpu to vgic-v2.c and name it
vgic_v2_get_target_vcpu;
- introduce get_target_vcpu to vgic_ops.
Changes in v8:
- rebase on ab78724fc5628318b172b4344f7280621a151e1b.
Changes in v7:
- add ASSERT to _vgic_get_target_vcpu;
- add comment to vgic_distr_mmio_write.
Changes in v6:
- add assert and bug_on;
- add in-code comments;
- move additional check on itargets writing from the following patch to
this patch;
- sizeof(itargets) instead of 8*sizeof(itargets[0]);
- remove the unneeded cast of &target for find_first_bit.
Changes in v5:
- improve in-code comments;
- use vgic_rank_irq;
- use bit masks to write-ignore GICD_ITARGETSR;
- introduce an version of vgic_get_target_vcpu that doesn't take the
rank lock;
- keep the rank lock while enabling/disabling irqs;
- use find_first_bit instead of find_next_bit;
- check for zero writes to GICD_ITARGETSR.
Changes in v4:
- remove assert that could allow a guest to crash Xen;
- add itargets validation to vgic_distr_mmio_write;
- export vgic_get_target_vcpu.
Changes in v3:
- add assert in get_target_vcpu;
- rename get_target_vcpu to vgic_get_target_vcpu.
Changes in v2:
- refactor the common code in get_target_vcpu;
- unify PPI and SPI paths;
- correctly initialize itargets for SPI;
- use byte_read.
---
xen/arch/arm/vgic-v2.c | 42 ++++++++++++++++++++++++++++++++++++++----
xen/arch/arm/vgic.c | 43 ++++++++++++++++++++++++++++++++++---------
xen/include/asm-arm/vgic.h | 5 +++++
3 files changed, 77 insertions(+), 13 deletions(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 2102e43..63d4f65 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -298,12 +298,12 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
vgic_lock_rank(v, rank);
tr = rank->ienable;
rank->ienable |= *r;
- vgic_unlock_rank(v, rank);
/* The virtual irq is derived from register offset.
* The register difference is word difference. So divide by 2(DABT_WORD)
* to get Virtual irq number */
vgic_enable_irqs(v, (*r) & (~tr),
(gicd_reg - GICD_ISENABLER) >> DABT_WORD);
+ vgic_unlock_rank(v, rank);
return 1;
case GICD_ICENABLER ... GICD_ICENABLERN:
@@ -313,12 +313,12 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
vgic_lock_rank(v, rank);
tr = rank->ienable;
rank->ienable &= ~*r;
- vgic_unlock_rank(v, rank);
/* The virtual irq is derived from register offset.
* The register difference is word difference. So divide by 2(DABT_WORD)
* to get Virtual irq number */
vgic_disable_irqs(v, (*r) & tr,
(gicd_reg - GICD_ICENABLER) >> DABT_WORD);
+ vgic_unlock_rank(v, rank);
return 1;
case GICD_ISPENDR ... GICD_ISPENDRN:
@@ -359,13 +359,29 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR, DABT_WORD);
if ( rank == NULL) goto write_ignore;
+ /* 8-bit vcpu mask for this domain */
+ BUG_ON(v->domain->max_vcpus > 8);
+ tr = (1 << v->domain->max_vcpus) - 1;
+ if ( dabt.size == 2 )
+ tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
+ else
+ tr = (tr << (8 * (gicd_reg & 0x3)));
+ tr &= *r;
+ /* ignore zero writes */
+ if ( !tr )
+ goto write_ignore;
+ /* For word reads ignore writes where any single byte is zero */
+ if ( dabt.size == 2 &&
+ !((tr & 0xff) && (tr & (0xff << 8)) &&
+ (tr & (0xff << 16)) && (tr & (0xff << 24))))
+ goto write_ignore;
vgic_lock_rank(v, rank);
if ( dabt.size == DABT_WORD )
rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR,
- DABT_WORD)] = *r;
+ DABT_WORD)] = tr;
else
vgic_byte_write(&rank->itargets[REG_RANK_INDEX(8,
- gicd_reg - GICD_ITARGETSR, DABT_WORD)], *r, gicd_reg);
+ gicd_reg - GICD_ITARGETSR, DABT_WORD)], tr, gicd_reg);
vgic_unlock_rank(v, rank);
return 1;
@@ -460,6 +476,23 @@ static const struct mmio_handler_ops vgic_v2_distr_mmio_handler = {
.write_handler = vgic_v2_distr_mmio_write,
};
+static struct vcpu *vgic_v2_get_target_vcpu(struct vcpu *v, unsigned int irq)
+{
+ unsigned long target;
+ struct vcpu *v_target;
+ struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
+ ASSERT(spin_is_locked(&rank->lock));
+
+ target = vgic_byte_read(rank->itargets[(irq%32)/4], 0, irq % 4);
+ /* 1-N SPI should be delivered as pending to all the vcpus in the
+ * mask, but here we just return the first vcpu for simplicity and
+ * because it would be too slow to do otherwise. */
+ target = find_first_bit(&target, 8);
+ ASSERT(target >= 0 && target < v->domain->max_vcpus);
+ v_target = v->domain->vcpu[target];
+ return v_target;
+}
+
static int vgic_v2_vcpu_init(struct vcpu *v)
{
int i;
@@ -487,6 +520,7 @@ static int vgic_v2_domain_init(struct domain *d)
static const struct vgic_ops vgic_v2_ops = {
.vcpu_init = vgic_v2_vcpu_init,
.domain_init = vgic_v2_domain_init,
+ .get_target_vcpu = vgic_v2_get_target_vcpu,
};
int vgic_v2_init(struct domain *d)
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 1948316..ebfec83 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -48,7 +48,7 @@ struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v, int b, int n,
return NULL;
}
-static struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq)
+struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq)
{
return vgic_rank_offset(v, 8, irq, DABT_WORD);
}
@@ -96,7 +96,13 @@ int domain_vgic_init(struct domain *d)
INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].lr_queue);
}
for (i=0; i<DOMAIN_NR_RANKS(d); i++)
+ {
spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
+ /* By default deliver to CPU0 */
+ memset(d->arch.vgic.shared_irqs[i].itargets,
+ 0x1,
+ sizeof(d->arch.vgic.shared_irqs[i].itargets));
+ }
d->arch.vgic.handler->domain_init(d);
@@ -146,19 +152,35 @@ int vcpu_vgic_free(struct vcpu *v)
return 0;
}
+/* takes the rank lock */
+struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
+{
+ struct domain *d = v->domain;
+ struct vcpu *v_target;
+ struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
+
+ vgic_lock_rank(v, rank);
+ v_target = d->arch.vgic.handler->get_target_vcpu(v, irq);
+ vgic_unlock_rank(v, rank);
+ return v_target;
+}
+
void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
{
+ struct domain *d = v->domain;
const unsigned long mask = r;
struct pending_irq *p;
unsigned int irq;
unsigned long flags;
int i = 0;
+ struct vcpu *v_target;
while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
irq = i + (32 * n);
- p = irq_to_pending(v, irq);
+ v_target = d->arch.vgic.handler->get_target_vcpu(v, irq);
+ p = irq_to_pending(v_target, irq);
clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
- gic_remove_from_queues(v, irq);
+ gic_remove_from_queues(v_target, irq);
if ( p->desc != NULL )
{
spin_lock_irqsave(&p->desc->lock, flags);
@@ -171,29 +193,32 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
{
+ struct domain *d = v->domain;
const unsigned long mask = r;
struct pending_irq *p;
unsigned int irq;
unsigned long flags;
int i = 0;
+ struct vcpu *v_target;
while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
irq = i + (32 * n);
- p = irq_to_pending(v, irq);
+ v_target = d->arch.vgic.handler->get_target_vcpu(v, irq);
+ p = irq_to_pending(v_target, irq);
set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
/* We need to force the first injection of evtchn_irq because
* evtchn_upcall_pending is already set by common code on vcpu
* creation. */
- if ( irq == v->domain->arch.evtchn_irq &&
+ if ( irq == v_target->domain->arch.evtchn_irq &&
vcpu_info(current, evtchn_upcall_pending) &&
list_empty(&p->inflight) )
- vgic_vcpu_inject_irq(v, irq);
+ vgic_vcpu_inject_irq(v_target, irq);
else {
unsigned long flags;
- spin_lock_irqsave(&v->arch.vgic.lock, flags);
+ spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
- gic_raise_guest_irq(v, irq, p->priority);
- spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+ gic_raise_guest_irq(v_target, irq, p->priority);
+ spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
}
if ( p->desc != NULL )
{
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 19eed7e..81a3eef 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -91,6 +91,9 @@ struct vgic_ops {
int (*vcpu_init)(struct vcpu *v);
/* Domain specific initialization of vGIC */
int (*domain_init)(struct domain *d);
+ /* Get the target vcpu for a given virq. The rank lock is already taken
+ * when calling this. */
+ struct vcpu *(*get_target_vcpu)(struct vcpu *v, unsigned int irq);
};
/* Number of ranks of interrupt registers for a domain */
@@ -151,10 +154,12 @@ enum gic_sgi_mode;
extern int domain_vgic_init(struct domain *d);
extern void domain_vgic_free(struct domain *d);
extern int vcpu_vgic_init(struct vcpu *v);
+extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq);
extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq);
extern void vgic_clear_pending_irqs(struct vcpu *v);
extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
extern struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v, int b, int n, int s);
+extern struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq);
extern void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n);
extern void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n);
extern void register_vgic_ops(struct domain *d, const struct vgic_ops *ops);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v10 02/10] xen/arm: move setting GIC_IRQ_GUEST_QUEUED earlier
2014-08-05 11:25 [PATCH v10 00/10] gic and vgic fixes and improvements Stefano Stabellini
2014-08-05 11:26 ` [PATCH v10 01/10] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs Stefano Stabellini
@ 2014-08-05 11:26 ` Stefano Stabellini
2014-08-05 11:26 ` [PATCH v10 03/10] xen/arm: inflight irqs during migration Stefano Stabellini
` (7 subsequent siblings)
9 siblings, 0 replies; 18+ messages in thread
From: Stefano Stabellini @ 2014-08-05 11:26 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, Ian.Campbell, Stefano Stabellini
It makes the code cleaner, especially with the following patches.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
xen/arch/arm/vgic.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index ebfec83..9947e8c 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -321,13 +321,6 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
spin_lock_irqsave(&v->arch.vgic.lock, flags);
- if ( !list_empty(&n->inflight) )
- {
- set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
- gic_raise_inflight_irq(v, irq);
- goto out;
- }
-
/* vcpu offline */
if ( test_bit(_VPF_down, &v->pause_flags) )
{
@@ -335,10 +328,17 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
return;
}
+ set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
+
+ if ( !list_empty(&n->inflight) )
+ {
+ gic_raise_inflight_irq(v, irq);
+ goto out;
+ }
+
priority = vgic_byte_read(rank->ipriority[REG_RANK_INDEX(8, irq, DABT_WORD)], 0, irq & 0x3);
n->irq = irq;
- set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
n->priority = priority;
/* the irq is enabled */
--
1.7.10.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v10 03/10] xen/arm: inflight irqs during migration
2014-08-05 11:25 [PATCH v10 00/10] gic and vgic fixes and improvements Stefano Stabellini
2014-08-05 11:26 ` [PATCH v10 01/10] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs Stefano Stabellini
2014-08-05 11:26 ` [PATCH v10 02/10] xen/arm: move setting GIC_IRQ_GUEST_QUEUED earlier Stefano Stabellini
@ 2014-08-05 11:26 ` Stefano Stabellini
2014-08-05 12:28 ` Julien Grall
2014-08-05 11:26 ` [PATCH v10 04/10] xen/arm: support irq delivery to vcpu > 0 Stefano Stabellini
` (6 subsequent siblings)
9 siblings, 1 reply; 18+ messages in thread
From: Stefano Stabellini @ 2014-08-05 11:26 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, Ian.Campbell, Stefano Stabellini
We need to take special care when migrating irqs that are already
inflight from one vcpu to another. See "The effect of changes to an
GICD_ITARGETSR", part of chapter 4.3.12 of the ARM Generic Interrupt
Controller Architecture Specification.
The main issue from the Xen point of view is that the lr_pending and
inflight lists are per-vcpu. The lock we take to protect them is also
per-vcpu.
In order to avoid issues, if the irq is still lr_pending, we can
immediately move it to the new vcpu for injection.
Otherwise if it is in a GICH_LR register, set a new flag
GIC_IRQ_GUEST_MIGRATING. If GIC_IRQ_GUEST_MIGRATING is set we'll change
the affinity of the physical irq when clearing the LR (in a following
commit). Therefore if the irq is inflight in an LR when the guest writes
to itarget, we wait until the LR is cleared before changing physical
irq affinity. This guarantees that the old vcpu is going to be
interrupted and clear the LR, either because it has been descheduled
after EOIing the interrupt or because it is interrupted by the second
physical irq coming.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
Changes in v9:
- simplify the code: rely on "physical irq follow virtual irq" to ensure
that physical irqs are injected into the right pcpu.
Changes in v8:
- rebase on ab78724fc5628318b172b4344f7280621a151e1b;
- rename target to new_target to avoid conflicts.
Changes in v7:
- move the _VPF_down check before setting GIC_IRQ_GUEST_QUEUED;
- fix comments;
- rename trl to target;
- introduce vcpu_migrate_from;
- do not kick new vcpu on MIGRATING, kick the old vcpu instead;
- separate moving GIC_IRQ_GUEST_QUEUED earlier into a different patch.
Changes in v6:
- remove unnecessary casts to (const unsigned long *) to the arguments
of find_first_bit and find_next_bit;
- instroduce a corresponding smb_rmb call;
- deal with migrating an irq that is inflight and still lr_pending;
- replace the dsb with smb_wmb and smb_rmb, use them to ensure the order
of accesses to GIC_IRQ_GUEST_QUEUED and GIC_IRQ_GUEST_MIGRATING;
Changes in v5:
- pass unsigned long to find_next_bit for alignment on aarch64;
- call vgic_get_target_vcpu instead of gic_add_to_lr_pending to add the
irq in the right inflight queue;
- add barrier and bit tests to make sure that vgic_migrate_irq and
gic_update_one_lr can run simultaneously on different cpus without
issues;
- rework the loop to identify the new vcpu when ITARGETSR is written;
- use find_first_bit instead of find_next_bit.
---
xen/arch/arm/gic.c | 8 ++++++--
xen/arch/arm/vgic-v2.c | 43 ++++++++++++++++++++++++++++++++++---------
xen/arch/arm/vgic.c | 37 +++++++++++++++++++++++++++++++++++++
xen/include/asm-arm/vgic.h | 6 ++++++
4 files changed, 83 insertions(+), 11 deletions(-)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 256d9cf..3e75fc5 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -33,6 +33,7 @@
#include <asm/device.h>
#include <asm/io.h>
#include <asm/gic.h>
+#include <asm/vgic.h>
static void gic_restore_pending_irqs(struct vcpu *v);
@@ -375,10 +376,13 @@ static void gic_update_one_lr(struct vcpu *v, int i)
clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
p->lr = GIC_INVALID_LR;
if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
- test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) )
+ test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) &&
+ !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
gic_raise_guest_irq(v, irq, p->priority);
- else
+ else {
list_del_init(&p->inflight);
+ clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status);
+ }
}
}
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 63d4f65..f9e357c 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -356,34 +356,59 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
goto write_ignore;
case GICD_ITARGETSR + 8 ... GICD_ITARGETSRN:
+ {
+ /* unsigned long needed for find_next_bit */
+ unsigned long target;
+ int i;
if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR, DABT_WORD);
if ( rank == NULL) goto write_ignore;
/* 8-bit vcpu mask for this domain */
BUG_ON(v->domain->max_vcpus > 8);
- tr = (1 << v->domain->max_vcpus) - 1;
+ target = (1 << v->domain->max_vcpus) - 1;
if ( dabt.size == 2 )
- tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
+ target = target | (target << 8) | (target << 16) | (target << 24);
else
- tr = (tr << (8 * (gicd_reg & 0x3)));
- tr &= *r;
+ target = (target << (8 * (gicd_reg & 0x3)));
+ target &= *r;
/* ignore zero writes */
- if ( !tr )
+ if ( !target )
goto write_ignore;
/* For word reads ignore writes where any single byte is zero */
if ( dabt.size == 2 &&
- !((tr & 0xff) && (tr & (0xff << 8)) &&
- (tr & (0xff << 16)) && (tr & (0xff << 24))))
+ !((target & 0xff) && (target & (0xff << 8)) &&
+ (target & (0xff << 16)) && (target & (0xff << 24))))
goto write_ignore;
vgic_lock_rank(v, rank);
+ i = 0;
+ while ( (i = find_next_bit(&target, 32, i)) < 32 )
+ {
+ unsigned int irq, new_target, old_target;
+ unsigned long old_target_mask;
+ struct vcpu *v_target, *v_old;
+
+ new_target = i % 8;
+ old_target_mask = vgic_byte_read(rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR, DABT_WORD)], 0, i/8);
+ old_target = find_first_bit(&old_target_mask, 8);
+
+ if ( new_target != old_target )
+ {
+ irq = gicd_reg + (i / 8);
+ v_target = v->domain->vcpu[new_target];
+ v_old = v->domain->vcpu[old_target];
+ vgic_migrate_irq(v_old, v_target, irq);
+ }
+ i += 8 - new_target;
+ }
if ( dabt.size == DABT_WORD )
rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR,
- DABT_WORD)] = tr;
+ DABT_WORD)] = target;
else
vgic_byte_write(&rank->itargets[REG_RANK_INDEX(8,
- gicd_reg - GICD_ITARGETSR, DABT_WORD)], tr, gicd_reg);
+ gicd_reg - GICD_ITARGETSR, DABT_WORD)], target, gicd_reg);
vgic_unlock_rank(v, rank);
return 1;
+ }
case GICD_IPRIORITYR ... GICD_IPRIORITYRN:
if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 9947e8c..9e1dd49 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -165,6 +165,43 @@ struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
return v_target;
}
+void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
+{
+ unsigned long flags;
+ struct pending_irq *p = irq_to_pending(old, irq);
+
+ /* nothing to do for virtual interrupts */
+ if ( p->desc == NULL )
+ return;
+
+ /* migration already in progress, no need to do anything */
+ if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
+ return;
+
+ spin_lock_irqsave(&old->arch.vgic.lock, flags);
+
+ if ( list_empty(&p->inflight) )
+ {
+ spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
+ return;
+ }
+ /* If the IRQ is still lr_pending, re-inject it to the new vcpu */
+ if ( !list_empty(&p->lr_queue) )
+ {
+ list_del_init(&p->lr_queue);
+ list_del_init(&p->inflight);
+ spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
+ vgic_vcpu_inject_irq(new, irq);
+ return;
+ }
+ /* if the IRQ is in a GICH_LR register, set GIC_IRQ_GUEST_MIGRATING
+ * and wait for the EOI */
+ if ( !list_empty(&p->inflight) )
+ set_bit(GIC_IRQ_GUEST_MIGRATING, &p->status);
+
+ spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
+}
+
void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
{
struct domain *d = v->domain;
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 81a3eef..434a625 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -55,11 +55,16 @@ struct pending_irq
* GIC_IRQ_GUEST_ENABLED: the guest IRQ is enabled at the VGICD
* level (GICD_ICENABLER/GICD_ISENABLER).
*
+ * GIC_IRQ_GUEST_MIGRATING: the irq is being migrated to a different
+ * vcpu while it is still inflight and on an GICH_LR register on the
+ * old vcpu.
+ *
*/
#define GIC_IRQ_GUEST_QUEUED 0
#define GIC_IRQ_GUEST_ACTIVE 1
#define GIC_IRQ_GUEST_VISIBLE 2
#define GIC_IRQ_GUEST_ENABLED 3
+#define GIC_IRQ_GUEST_MIGRATING 4
unsigned long status;
struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */
int irq;
@@ -169,6 +174,7 @@ extern int vcpu_vgic_free(struct vcpu *v);
extern int vgic_to_sgi(struct vcpu *v, register_t sgir,
enum gic_sgi_mode irqmode, int virq,
unsigned long vcpu_mask);
+extern void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq);
#endif /* __ASM_ARM_VGIC_H__ */
/*
--
1.7.10.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v10 03/10] xen/arm: inflight irqs during migration
2014-08-05 11:26 ` [PATCH v10 03/10] xen/arm: inflight irqs during migration Stefano Stabellini
@ 2014-08-05 12:28 ` Julien Grall
2014-08-05 14:35 ` Stefano Stabellini
0 siblings, 1 reply; 18+ messages in thread
From: Julien Grall @ 2014-08-05 12:28 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel; +Cc: julien.grall, Ian.Campbell
Hi Stefano,
On 08/05/2014 12:26 PM, Stefano Stabellini wrote:
> We need to take special care when migrating irqs that are already
> inflight from one vcpu to another. See "The effect of changes to an
> GICD_ITARGETSR", part of chapter 4.3.12 of the ARM Generic Interrupt
> Controller Architecture Specification.
NIT: Can you give the reference of the book? ie: IHI 0048B
[..]
> + i = 0;
> + while ( (i = find_next_bit(&target, 32, i)) < 32 )
> + {
> + unsigned int irq, new_target, old_target;
> + unsigned long old_target_mask;
> + struct vcpu *v_target, *v_old;
> +
> + new_target = i % 8;
> + old_target_mask = vgic_byte_read(rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR, DABT_WORD)], 0, i/8);
Can you try to split this line?
> + old_target = find_first_bit(&old_target_mask, 8);
> +
> + if ( new_target != old_target )
> + {
> + irq = gicd_reg + (i / 8);
gicd_reg contains the register offset (i.e GICD_ITARGETSR + off). How
can you get the correct IRQ number here?
I think this should be
irq = gicd_reg - GICD_ITARGETSR + (i / 8);
--
Julien Grall
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v10 03/10] xen/arm: inflight irqs during migration
2014-08-05 12:28 ` Julien Grall
@ 2014-08-05 14:35 ` Stefano Stabellini
0 siblings, 0 replies; 18+ messages in thread
From: Stefano Stabellini @ 2014-08-05 14:35 UTC (permalink / raw)
To: Julien Grall; +Cc: julien.grall, xen-devel, Ian.Campbell, Stefano Stabellini
On Tue, 5 Aug 2014, Julien Grall wrote:
> Hi Stefano,
>
> On 08/05/2014 12:26 PM, Stefano Stabellini wrote:
> > We need to take special care when migrating irqs that are already
> > inflight from one vcpu to another. See "The effect of changes to an
> > GICD_ITARGETSR", part of chapter 4.3.12 of the ARM Generic Interrupt
> > Controller Architecture Specification.
>
> NIT: Can you give the reference of the book? ie: IHI 0048B
>
> [..]
>
> > + i = 0;
> > + while ( (i = find_next_bit(&target, 32, i)) < 32 )
> > + {
> > + unsigned int irq, new_target, old_target;
> > + unsigned long old_target_mask;
> > + struct vcpu *v_target, *v_old;
> > +
> > + new_target = i % 8;
> > + old_target_mask = vgic_byte_read(rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR, DABT_WORD)], 0, i/8);
>
> Can you try to split this line?
>
> > + old_target = find_first_bit(&old_target_mask, 8);
> > +
> > + if ( new_target != old_target )
> > + {
> > + irq = gicd_reg + (i / 8);
>
> gicd_reg contains the register offset (i.e GICD_ITARGETSR + off). How
> can you get the correct IRQ number here?
>
> I think this should be
>
> irq = gicd_reg - GICD_ITARGETSR + (i / 8);
You are right. I have no idea how I didn't spot it before.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v10 04/10] xen/arm: support irq delivery to vcpu > 0
2014-08-05 11:25 [PATCH v10 00/10] gic and vgic fixes and improvements Stefano Stabellini
` (2 preceding siblings ...)
2014-08-05 11:26 ` [PATCH v10 03/10] xen/arm: inflight irqs during migration Stefano Stabellini
@ 2014-08-05 11:26 ` Stefano Stabellini
2014-08-05 11:26 ` [PATCH v10 05/10] xen/arm: physical irq follow virtual irq Stefano Stabellini
` (5 subsequent siblings)
9 siblings, 0 replies; 18+ messages in thread
From: Stefano Stabellini @ 2014-08-05 11:26 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, Ian.Campbell, Stefano Stabellini
Use vgic_get_target_vcpu to retrieve the target vcpu from do_IRQ.
Remove in-code comments about missing implementation of SGI delivery to
vcpus other than 0.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
Changes in v7:
- improve in-code comment;
- use gic_number_lines in assert.
Changes in v6:
- add in-code comments;
- assert that the guest irq is an SPI.
Changes in v4:
- the mask in gic_route_irq_to_guest is a physical cpu mask, treat it as
such;
- export vgic_get_target_vcpu in a previous patch.
---
xen/arch/arm/gic.c | 3 ++-
xen/arch/arm/irq.c | 5 +++--
xen/arch/arm/vgic.c | 11 +++++++++++
xen/include/asm-arm/vgic.h | 1 +
4 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 3e75fc5..f5c7c91 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -137,7 +137,8 @@ void gic_route_irq_to_guest(struct domain *d, struct irq_desc *desc,
gic_set_irq_properties(desc, cpumask_of(smp_processor_id()), GIC_PRI_IRQ);
- /* TODO: do not assume delivery to vcpu0 */
+ /* Use vcpu0 to retrieve the pending_irq struct. Given that we only
+ * route SPIs to guests, it doesn't make any difference. */
p = irq_to_pending(d->vcpu[0], desc->irq);
p->desc = desc;
}
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 3a8acbf..49ca467 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -198,8 +198,9 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
desc->status |= IRQ_INPROGRESS;
desc->arch.eoi_cpu = smp_processor_id();
- /* XXX: inject irq into all guest vcpus */
- vgic_vcpu_inject_irq(d->vcpu[0], irq);
+ /* the irq cannot be a PPI, we only support delivery of SPIs to
+ * guests */
+ vgic_vcpu_inject_spi(d, irq);
goto out_no_end;
}
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 9e1dd49..4344c36 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -400,6 +400,17 @@ out:
smp_send_event_check_mask(cpumask_of(v->processor));
}
+void vgic_vcpu_inject_spi(struct domain *d, unsigned int irq)
+{
+ struct vcpu *v;
+
+ /* the IRQ needs to be an SPI */
+ ASSERT(irq >= 32 && irq <= gic_number_lines());
+
+ v = vgic_get_target_vcpu(d->vcpu[0], irq);
+ vgic_vcpu_inject_irq(v, irq);
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 434a625..9b1db04 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -161,6 +161,7 @@ extern void domain_vgic_free(struct domain *d);
extern int vcpu_vgic_init(struct vcpu *v);
extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq);
extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq);
+extern void vgic_vcpu_inject_spi(struct domain *d, unsigned int irq);
extern void vgic_clear_pending_irqs(struct vcpu *v);
extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
extern struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v, int b, int n, int s);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v10 05/10] xen/arm: physical irq follow virtual irq
2014-08-05 11:25 [PATCH v10 00/10] gic and vgic fixes and improvements Stefano Stabellini
` (3 preceding siblings ...)
2014-08-05 11:26 ` [PATCH v10 04/10] xen/arm: support irq delivery to vcpu > 0 Stefano Stabellini
@ 2014-08-05 11:26 ` Stefano Stabellini
2014-08-05 11:57 ` Jan Beulich
2014-08-05 11:26 ` [PATCH v10 06/10] xen: introduce sched_move_irqs Stefano Stabellini
` (4 subsequent siblings)
9 siblings, 1 reply; 18+ messages in thread
From: Stefano Stabellini @ 2014-08-05 11:26 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, Ian.Campbell, Stefano Stabellini
Migrate physical irqs to the same physical cpu that is running the vcpu
expected to receive the irqs. That is done when enabling irqs, when the
guest writes to GICD_ITARGETSR and when Xen migrates a vcpu to a
different pcpu.
In case of virq migration, if the virq is inflight and in a GICH_LR
register already, delay migrating the corresponding physical irq until
the virq is EOIed by the guest and the MIGRATING flag has been cleared.
This way we make sure that the pcpu running the old vcpu gets
interrupted with a new irq of the same kind, clearing the GICH_LR sooner.
Introduce a new arch specific function, arch_move_irqs, that is empty on
x86 and implements the vgic irq migration code on ARM.
arch_move_irqs is going to be called by from sched.c.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in v10:
- fix for loop over vgic.nr_lines.
Changes in v9:
- move arch_move_irqs declaration to irq.h.
Changes in v7:
- remove checks at the top of gic_irq_set_affinity, add assert instead;
- move irq_set_affinity to irq.c;
- delay setting the affinity of the physical irq when the virq is
MIGRATING until the virq is EOIed by the guest;
- do not set the affinity of MIGRATING irqs from arch_move_irqs.
Changes in v6:
- use vgic_get_target_vcpu instead of _vgic_get_target_vcpu in
arch_move_irqs.
Changes in v5:
- prettify vgic_move_irqs;
- rename vgic_move_irqs to arch_move_irqs;
- introduce helper function irq_set_affinity.
---
xen/arch/arm/gic-v2.c | 15 +++++++++++++--
xen/arch/arm/gic.c | 6 +++++-
xen/arch/arm/irq.c | 6 ++++++
xen/arch/arm/vgic.c | 21 +++++++++++++++++++++
xen/include/asm-arm/irq.h | 3 +++
xen/include/asm-x86/irq.h | 2 ++
6 files changed, 50 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 1305542..da60a41 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -569,9 +569,20 @@ static void gicv2_guest_irq_end(struct irq_desc *desc)
/* Deactivation happens in maintenance interrupt / via GICV */
}
-static void gicv2_irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
+static void gicv2_irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
{
- BUG();
+ unsigned int mask;
+
+ ASSERT(!cpumask_empty(cpu_mask));
+
+ spin_lock(&gicv2.lock);
+
+ mask = gicv2_cpu_mask(cpu_mask);
+
+ /* Set target CPU mask (RAZ/WI on uniprocessor) */
+ writeb_gicd(mask, GICD_ITARGETSR + desc->irq);
+
+ spin_unlock(&gicv2.lock);
}
/* XXX different for level vs edge */
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index f5c7c91..2aa9500 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -382,7 +382,11 @@ static void gic_update_one_lr(struct vcpu *v, int i)
gic_raise_guest_irq(v, irq, p->priority);
else {
list_del_init(&p->inflight);
- clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status);
+ if ( test_and_clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
+ {
+ struct vcpu *v_target = vgic_get_target_vcpu(v, irq);
+ irq_set_affinity(p->desc, cpumask_of(v_target->processor));
+ }
}
}
}
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 49ca467..7150c7a 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -134,6 +134,12 @@ static inline struct domain *irq_get_domain(struct irq_desc *desc)
return desc->action->dev_id;
}
+void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
+{
+ if ( desc != NULL )
+ desc->handler->set_affinity(desc, cpu_mask);
+}
+
int request_irq(unsigned int irq, unsigned int irqflags,
void (*handler)(int, void *, struct cpu_user_regs *),
const char *devname, void *dev_id)
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 4344c36..731d84d 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -182,6 +182,7 @@ void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
if ( list_empty(&p->inflight) )
{
+ irq_set_affinity(p->desc, cpumask_of(new->processor));
spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
return;
}
@@ -190,6 +191,7 @@ void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
{
list_del_init(&p->lr_queue);
list_del_init(&p->inflight);
+ irq_set_affinity(p->desc, cpumask_of(new->processor));
spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
vgic_vcpu_inject_irq(new, irq);
return;
@@ -202,6 +204,24 @@ void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
}
+void arch_move_irqs(struct vcpu *v)
+{
+ const cpumask_t *cpu_mask = cpumask_of(v->processor);
+ struct domain *d = v->domain;
+ struct pending_irq *p;
+ struct vcpu *v_target;
+ int i;
+
+ for ( i = 32; i < (d->arch.vgic.nr_lines + 32); i++ )
+ {
+ v_target = vgic_get_target_vcpu(v, i);
+ p = irq_to_pending(v_target, i);
+
+ if ( v_target == v && !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
+ irq_set_affinity(p->desc, cpu_mask);
+ }
+}
+
void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
{
struct domain *d = v->domain;
@@ -259,6 +279,7 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
}
if ( p->desc != NULL )
{
+ irq_set_affinity(p->desc, cpumask_of(v_target->processor));
spin_lock_irqsave(&p->desc->lock, flags);
p->desc->handler->enable(p->desc);
spin_unlock_irqrestore(&p->desc->lock, flags);
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index e567f71..e877334 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -42,12 +42,15 @@ void init_secondary_IRQ(void);
int route_irq_to_guest(struct domain *d, unsigned int irq,
const char *devname);
+void arch_move_irqs(struct vcpu *v);
/* Set IRQ type for an SPI */
int irq_set_spi_type(unsigned int spi, unsigned int type);
int platform_get_irq(const struct dt_device_node *device, int index);
+void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask);
+
#endif /* _ASM_HW_IRQ_H */
/*
* Local variables:
diff --git a/xen/include/asm-x86/irq.h b/xen/include/asm-x86/irq.h
index 9066d38..d3c55f3 100644
--- a/xen/include/asm-x86/irq.h
+++ b/xen/include/asm-x86/irq.h
@@ -197,4 +197,6 @@ void cleanup_domain_irq_mapping(struct domain *);
bool_t cpu_has_pending_apic_eoi(void);
+static inline void arch_move_irqs(struct vcpu *v) { }
+
#endif /* _ASM_HW_IRQ_H */
--
1.7.10.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v10 05/10] xen/arm: physical irq follow virtual irq
2014-08-05 11:26 ` [PATCH v10 05/10] xen/arm: physical irq follow virtual irq Stefano Stabellini
@ 2014-08-05 11:57 ` Jan Beulich
2014-08-05 12:01 ` Jan Beulich
0 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2014-08-05 11:57 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: julien.grall, xen-devel, Ian.Campbell
>>> On 05.08.14 at 13:26, <stefano.stabellini@eu.citrix.com> wrote:
> --- a/xen/include/asm-x86/irq.h
> +++ b/xen/include/asm-x86/irq.h
> @@ -197,4 +197,6 @@ void cleanup_domain_irq_mapping(struct domain *);
>
> bool_t cpu_has_pending_apic_eoi(void);
>
> +static inline void arch_move_irqs(struct vcpu *v) { }
So we discussed the alternative, and you still sent the unmodified
version? Please drop my ack without the discussed change.
Jan
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v10 05/10] xen/arm: physical irq follow virtual irq
2014-08-05 11:57 ` Jan Beulich
@ 2014-08-05 12:01 ` Jan Beulich
0 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2014-08-05 12:01 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: julien.grall, xen-devel, Ian.Campbell
>>> On 05.08.14 at 13:57, <JBeulich@suse.com> wrote:
>>>> On 05.08.14 at 13:26, <stefano.stabellini@eu.citrix.com> wrote:
>> --- a/xen/include/asm-x86/irq.h
>> +++ b/xen/include/asm-x86/irq.h
>> @@ -197,4 +197,6 @@ void cleanup_domain_irq_mapping(struct domain *);
>>
>> bool_t cpu_has_pending_apic_eoi(void);
>>
>> +static inline void arch_move_irqs(struct vcpu *v) { }
>
> So we discussed the alternative, and you still sent the unmodified
> version? Please drop my ack without the discussed change.
Please ignore this, I was being confused. Patch 7 is where the
intended change is.
Jan
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v10 06/10] xen: introduce sched_move_irqs
2014-08-05 11:25 [PATCH v10 00/10] gic and vgic fixes and improvements Stefano Stabellini
` (4 preceding siblings ...)
2014-08-05 11:26 ` [PATCH v10 05/10] xen/arm: physical irq follow virtual irq Stefano Stabellini
@ 2014-08-05 11:26 ` Stefano Stabellini
2014-08-05 11:26 ` [PATCH v10 07/10] xen: remove workaround to inject evtchn_irq on irq enable Stefano Stabellini
` (3 subsequent siblings)
9 siblings, 0 replies; 18+ messages in thread
From: Stefano Stabellini @ 2014-08-05 11:26 UTC (permalink / raw)
To: xen-devel
Cc: Ian.Campbell, Stefano Stabellini, tim, julien.grall, keir.xen,
jbeulich
Introduce sched_move_irqs: it calls arch_move_irqs and
evtchn_move_pirqs.
Replace calls to evtchn_move_pirqs with calls to sched_move_irqs.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
CC: jbeulich@suse.com
CC: tim@xen.org
CC: keir.xen@gmail.com
---
xen/common/schedule.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index e9eb0bc..8c1561f 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -226,6 +226,12 @@ int sched_init_vcpu(struct vcpu *v, unsigned int processor)
return 0;
}
+static void sched_move_irqs(struct vcpu *v)
+{
+ arch_move_irqs(v);
+ evtchn_move_pirqs(v);
+}
+
int sched_move_domain(struct domain *d, struct cpupool *c)
{
struct vcpu *v;
@@ -301,7 +307,7 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
v->sched_priv = vcpu_priv[v->vcpu_id];
if ( !d->is_dying )
- evtchn_move_pirqs(v);
+ sched_move_irqs(v);
new_p = cpumask_cycle(new_p, c->cpu_valid);
@@ -528,7 +534,7 @@ static void vcpu_migrate(struct vcpu *v)
spin_unlock_irqrestore(old_lock, flags);
if ( old_cpu != new_cpu )
- evtchn_move_pirqs(v);
+ sched_move_irqs(v);
/* Wake on new CPU. */
vcpu_wake(v);
@@ -1251,7 +1257,7 @@ static void schedule(void)
stop_timer(&prev->periodic_timer);
if ( next_slice.migrated )
- evtchn_move_pirqs(next);
+ sched_move_irqs(next);
vcpu_periodic_timer_work(next);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v10 07/10] xen: remove workaround to inject evtchn_irq on irq enable
2014-08-05 11:25 [PATCH v10 00/10] gic and vgic fixes and improvements Stefano Stabellini
` (5 preceding siblings ...)
2014-08-05 11:26 ` [PATCH v10 06/10] xen: introduce sched_move_irqs Stefano Stabellini
@ 2014-08-05 11:26 ` Stefano Stabellini
2014-08-05 12:00 ` Jan Beulich
2014-08-05 12:34 ` Julien Grall
2014-08-05 11:26 ` [PATCH v10 08/10] xen/arm: take the rank lock before accessing ipriority Stefano Stabellini
` (2 subsequent siblings)
9 siblings, 2 replies; 18+ messages in thread
From: Stefano Stabellini @ 2014-08-05 11:26 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, Ian.Campbell, Stefano Stabellini
evtchn_upcall_pending is already set by common code at vcpu creation,
therefore on ARM we also need to call vgic_vcpu_inject_irq for it.
Currently we do that from vgic_enable_irqs as a workaround.
Do this properly by introducing an appropriate arch specific hook:
arch_evtchn_inject. arch_evtchn_inject is called by map_vcpu_info to
inject the evtchn irq into the guest. On ARM is implemented by calling
vgic_vcpu_inject_irq.
On x86 guests typically don't call VCPUOP_register_vcpu_info on vcpu0,
therefore avoiding the issue. However theoretically they could call
VCPUOP_register_vcpu_info on vcpu0 and in that case Xen would need to
inject the event channel notification into the guest if the guest is
HVM. So implement arch_evtchn_inject on x86 by calling
hvm_assert_evtchn_irq.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
Changes in v10:
- provide an implementation for x86.
Changes in v9:
- use an arch hook.
Changes in v2:
- coding style fix;
- add comment;
- return an error if arch_set_info_guest is called without VGCF_online.
---
xen/arch/arm/vgic.c | 23 +++++++++--------------
xen/arch/x86/hvm/irq.c | 6 ++++++
xen/common/domain.c | 1 +
xen/include/xen/event.h | 3 +++
4 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 731d84d..ce4457e 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -263,20 +263,10 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
v_target = d->arch.vgic.handler->get_target_vcpu(v, irq);
p = irq_to_pending(v_target, irq);
set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
- /* We need to force the first injection of evtchn_irq because
- * evtchn_upcall_pending is already set by common code on vcpu
- * creation. */
- if ( irq == v_target->domain->arch.evtchn_irq &&
- vcpu_info(current, evtchn_upcall_pending) &&
- list_empty(&p->inflight) )
- vgic_vcpu_inject_irq(v_target, irq);
- else {
- unsigned long flags;
- spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
- if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
- gic_raise_guest_irq(v_target, irq, p->priority);
- spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
- }
+ spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
+ if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
+ gic_raise_guest_irq(v_target, irq, p->priority);
+ spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
if ( p->desc != NULL )
{
irq_set_affinity(p->desc, cpumask_of(v_target->processor));
@@ -432,6 +422,11 @@ void vgic_vcpu_inject_spi(struct domain *d, unsigned int irq)
vgic_vcpu_inject_irq(v, irq);
}
+void arch_evtchn_inject(struct vcpu *v)
+{
+ vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index 3b29678..35f4f94 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -468,6 +468,12 @@ int hvm_local_events_need_delivery(struct vcpu *v)
return !hvm_interrupt_blocked(v, intack);
}
+void arch_evtchn_inject(struct vcpu *v)
+{
+ if ( has_hvm_container_vcpu(v) )
+ hvm_assert_evtchn_irq(v);
+}
+
static void irq_dump(struct domain *d)
{
struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
diff --git a/xen/common/domain.c b/xen/common/domain.c
index cd64aea..05d0049 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1058,6 +1058,7 @@ int map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned offset)
vcpu_info(v, evtchn_upcall_pending) = 1;
for ( i = 0; i < BITS_PER_EVTCHN_WORD(d); i++ )
set_bit(i, &vcpu_info(v, evtchn_pending_sel));
+ arch_evtchn_inject(v);
return 0;
}
diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
index 06c0654..fe19165 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -69,6 +69,9 @@ int guest_enabled_event(struct vcpu *v, uint32_t virq);
/* Notify remote end of a Xen-attached event channel.*/
void notify_via_xen_event_channel(struct domain *ld, int lport);
+/* inject an event channel notification into the guest */
+void arch_evtchn_inject(struct vcpu *v);
+
/*
* Internal event channel object storage.
*
--
1.7.10.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v10 07/10] xen: remove workaround to inject evtchn_irq on irq enable
2014-08-05 11:26 ` [PATCH v10 07/10] xen: remove workaround to inject evtchn_irq on irq enable Stefano Stabellini
@ 2014-08-05 12:00 ` Jan Beulich
2014-08-05 12:34 ` Julien Grall
1 sibling, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2014-08-05 12:00 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: julien.grall, xen-devel, Ian.Campbell
>>> On 05.08.14 at 13:26, <stefano.stabellini@eu.citrix.com> wrote:
> evtchn_upcall_pending is already set by common code at vcpu creation,
> therefore on ARM we also need to call vgic_vcpu_inject_irq for it.
> Currently we do that from vgic_enable_irqs as a workaround.
>
> Do this properly by introducing an appropriate arch specific hook:
> arch_evtchn_inject. arch_evtchn_inject is called by map_vcpu_info to
> inject the evtchn irq into the guest. On ARM is implemented by calling
> vgic_vcpu_inject_irq.
>
> On x86 guests typically don't call VCPUOP_register_vcpu_info on vcpu0,
> therefore avoiding the issue. However theoretically they could call
> VCPUOP_register_vcpu_info on vcpu0 and in that case Xen would need to
> inject the event channel notification into the guest if the guest is
> HVM. So implement arch_evtchn_inject on x86 by calling
> hvm_assert_evtchn_irq.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
For the non-ARM parts
Acked-by: Jan Beulich <jbeulich@suse.com>
provided you make ...
> --- a/xen/include/xen/event.h
> +++ b/xen/include/xen/event.h
> @@ -69,6 +69,9 @@ int guest_enabled_event(struct vcpu *v, uint32_t virq);
> /* Notify remote end of a Xen-attached event channel.*/
> void notify_via_xen_event_channel(struct domain *ld, int lport);
>
> +/* inject an event channel notification into the guest */
> +void arch_evtchn_inject(struct vcpu *v);
... this comment match coding style.
Jan
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v10 07/10] xen: remove workaround to inject evtchn_irq on irq enable
2014-08-05 11:26 ` [PATCH v10 07/10] xen: remove workaround to inject evtchn_irq on irq enable Stefano Stabellini
2014-08-05 12:00 ` Jan Beulich
@ 2014-08-05 12:34 ` Julien Grall
1 sibling, 0 replies; 18+ messages in thread
From: Julien Grall @ 2014-08-05 12:34 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel; +Cc: julien.grall, Ian.Campbell
Hi Stefano,
On 08/05/2014 12:26 PM, Stefano Stabellini wrote:
> evtchn_upcall_pending is already set by common code at vcpu creation,
> therefore on ARM we also need to call vgic_vcpu_inject_irq for it.
> Currently we do that from vgic_enable_irqs as a workaround.
>
> Do this properly by introducing an appropriate arch specific hook:
> arch_evtchn_inject. arch_evtchn_inject is called by map_vcpu_info to
> inject the evtchn irq into the guest. On ARM is implemented by calling
> vgic_vcpu_inject_irq.
>
> On x86 guests typically don't call VCPUOP_register_vcpu_info on vcpu0,
> therefore avoiding the issue. However theoretically they could call
> VCPUOP_register_vcpu_info on vcpu0 and in that case Xen would need to
> inject the event channel notification into the guest if the guest is
> HVM. So implement arch_evtchn_inject on x86 by calling
> hvm_assert_evtchn_irq.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v10 08/10] xen/arm: take the rank lock before accessing ipriority
2014-08-05 11:25 [PATCH v10 00/10] gic and vgic fixes and improvements Stefano Stabellini
` (6 preceding siblings ...)
2014-08-05 11:26 ` [PATCH v10 07/10] xen: remove workaround to inject evtchn_irq on irq enable Stefano Stabellini
@ 2014-08-05 11:26 ` Stefano Stabellini
2014-08-05 11:26 ` [PATCH v10 09/10] xen: introduce bit access macros for the IRQ line status flags Stefano Stabellini
2014-08-05 11:26 ` [PATCH v10 10/10] xen/arm: make accesses to desc->status flags atomic Stefano Stabellini
9 siblings, 0 replies; 18+ messages in thread
From: Stefano Stabellini @ 2014-08-05 11:26 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, Ian.Campbell, Stefano Stabellini
Currently we read ipriority from vgic_vcpu_inject_irq without taking the
rank lock. Fix that by taking the rank lock and reading ipriority at the
beginning of the function.
As vgic_vcpu_inject_irq is called from the irq.c upon receiving an
interrupt, we need to change the implementation of vgic_lock/unlock_rank
to spin_lock_irqsave to make it safe in irq context.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
---
Changes in v9:
- add explicit flags paramter to vgic_lock_rank and vgic_unlock_rank.
Changes in v2:
- rebased on ab78724fc5628318b172b4344f7280621a151e1b;
- remove warning on changing priority of active irqs.
---
xen/arch/arm/vgic-v2.c | 74 +++++++++++++++++++++++---------------------
xen/arch/arm/vgic.c | 11 ++++---
xen/include/asm-arm/vgic.h | 4 +--
3 files changed, 47 insertions(+), 42 deletions(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index f9e357c..8b21a13 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -39,6 +39,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
register_t *r = select_user_reg(regs, dabt.reg);
struct vgic_irq_rank *rank;
int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
+ unsigned long flags;
switch ( gicd_reg )
{
@@ -77,54 +78,54 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
if ( dabt.size != DABT_WORD ) goto bad_width;
rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ISENABLER, DABT_WORD);
if ( rank == NULL) goto read_as_zero;
- vgic_lock_rank(v, rank);
+ vgic_lock_rank(v, rank, flags);
*r = rank->ienable;
- vgic_unlock_rank(v, rank);
+ vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ICENABLER ... GICD_ICENABLERN:
if ( dabt.size != DABT_WORD ) goto bad_width;
rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICENABLER, DABT_WORD);
if ( rank == NULL) goto read_as_zero;
- vgic_lock_rank(v, rank);
+ vgic_lock_rank(v, rank, flags);
*r = rank->ienable;
- vgic_unlock_rank(v, rank);
+ vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ISPENDR ... GICD_ISPENDRN:
if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ISPENDR, DABT_WORD);
if ( rank == NULL) goto read_as_zero;
- vgic_lock_rank(v, rank);
+ vgic_lock_rank(v, rank, flags);
*r = vgic_byte_read(rank->ipend, dabt.sign, gicd_reg);
- vgic_unlock_rank(v, rank);
+ vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ICPENDR ... GICD_ICPENDRN:
if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICPENDR, DABT_WORD);
if ( rank == NULL) goto read_as_zero;
- vgic_lock_rank(v, rank);
+ vgic_lock_rank(v, rank, flags);
*r = vgic_byte_read(rank->ipend, dabt.sign, gicd_reg);
- vgic_unlock_rank(v, rank);
+ vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ISACTIVER ... GICD_ISACTIVERN:
if ( dabt.size != DABT_WORD ) goto bad_width;
rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ISACTIVER, DABT_WORD);
if ( rank == NULL) goto read_as_zero;
- vgic_lock_rank(v, rank);
+ vgic_lock_rank(v, rank, flags);
*r = rank->iactive;
- vgic_unlock_rank(v, rank);
+ vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ICACTIVER ... GICD_ICACTIVERN:
if ( dabt.size != DABT_WORD ) goto bad_width;
rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICACTIVER, DABT_WORD);
if ( rank == NULL) goto read_as_zero;
- vgic_lock_rank(v, rank);
+ vgic_lock_rank(v, rank, flags);
*r = rank->iactive;
- vgic_unlock_rank(v, rank);
+ vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ITARGETSR ... GICD_ITARGETSRN:
@@ -132,12 +133,12 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR, DABT_WORD);
if ( rank == NULL) goto read_as_zero;
- vgic_lock_rank(v, rank);
+ vgic_lock_rank(v, rank, flags);
*r = rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR,
DABT_WORD)];
if ( dabt.size == DABT_BYTE )
*r = vgic_byte_read(*r, dabt.sign, gicd_reg);
- vgic_unlock_rank(v, rank);
+ vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_IPRIORITYR ... GICD_IPRIORITYRN:
@@ -145,21 +146,21 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
rank = vgic_rank_offset(v, 8, gicd_reg - GICD_IPRIORITYR, DABT_WORD);
if ( rank == NULL) goto read_as_zero;
- vgic_lock_rank(v, rank);
+ vgic_lock_rank(v, rank, flags);
*r = rank->ipriority[REG_RANK_INDEX(8, gicd_reg - GICD_IPRIORITYR,
DABT_WORD)];
if ( dabt.size == DABT_BYTE )
*r = vgic_byte_read(*r, dabt.sign, gicd_reg);
- vgic_unlock_rank(v, rank);
+ vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ICFGR ... GICD_ICFGRN:
if ( dabt.size != DABT_WORD ) goto bad_width;
rank = vgic_rank_offset(v, 2, gicd_reg - GICD_ICFGR, DABT_WORD);
if ( rank == NULL) goto read_as_zero;
- vgic_lock_rank(v, rank);
+ vgic_lock_rank(v, rank, flags);
*r = rank->icfg[REG_RANK_INDEX(2, gicd_reg - GICD_ICFGR, DABT_WORD)];
- vgic_unlock_rank(v, rank);
+ vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_NSACR ... GICD_NSACRN:
@@ -176,18 +177,18 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
rank = vgic_rank_offset(v, 1, gicd_reg - GICD_CPENDSGIR, DABT_WORD);
if ( rank == NULL) goto read_as_zero;
- vgic_lock_rank(v, rank);
+ vgic_lock_rank(v, rank, flags);
*r = vgic_byte_read(rank->pendsgi, dabt.sign, gicd_reg);
- vgic_unlock_rank(v, rank);
+ vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_SPENDSGIR ... GICD_SPENDSGIRN:
if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
rank = vgic_rank_offset(v, 1, gicd_reg - GICD_SPENDSGIR, DABT_WORD);
if ( rank == NULL) goto read_as_zero;
- vgic_lock_rank(v, rank);
+ vgic_lock_rank(v, rank, flags);
*r = vgic_byte_read(rank->pendsgi, dabt.sign, gicd_reg);
- vgic_unlock_rank(v, rank);
+ vgic_unlock_rank(v, rank, flags);
return 1;
/* Implementation defined -- read as zero */
@@ -269,6 +270,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
struct vgic_irq_rank *rank;
int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
uint32_t tr;
+ unsigned long flags;
switch ( gicd_reg )
{
@@ -295,7 +297,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
if ( dabt.size != DABT_WORD ) goto bad_width;
rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ISENABLER, DABT_WORD);
if ( rank == NULL) goto write_ignore;
- vgic_lock_rank(v, rank);
+ vgic_lock_rank(v, rank, flags);
tr = rank->ienable;
rank->ienable |= *r;
/* The virtual irq is derived from register offset.
@@ -303,14 +305,14 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
* to get Virtual irq number */
vgic_enable_irqs(v, (*r) & (~tr),
(gicd_reg - GICD_ISENABLER) >> DABT_WORD);
- vgic_unlock_rank(v, rank);
+ vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ICENABLER ... GICD_ICENABLERN:
if ( dabt.size != DABT_WORD ) goto bad_width;
rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICENABLER, DABT_WORD);
if ( rank == NULL) goto write_ignore;
- vgic_lock_rank(v, rank);
+ vgic_lock_rank(v, rank, flags);
tr = rank->ienable;
rank->ienable &= ~*r;
/* The virtual irq is derived from register offset.
@@ -318,7 +320,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
* to get Virtual irq number */
vgic_disable_irqs(v, (*r) & tr,
(gicd_reg - GICD_ICENABLER) >> DABT_WORD);
- vgic_unlock_rank(v, rank);
+ vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ISPENDR ... GICD_ISPENDRN:
@@ -337,18 +339,18 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
if ( dabt.size != DABT_WORD ) goto bad_width;
rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ISACTIVER, DABT_WORD);
if ( rank == NULL) goto write_ignore;
- vgic_lock_rank(v, rank);
+ vgic_lock_rank(v, rank, flags);
rank->iactive &= ~*r;
- vgic_unlock_rank(v, rank);
+ vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ICACTIVER ... GICD_ICACTIVERN:
if ( dabt.size != DABT_WORD ) goto bad_width;
rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICACTIVER, DABT_WORD);
if ( rank == NULL) goto write_ignore;
- vgic_lock_rank(v, rank);
+ vgic_lock_rank(v, rank, flags);
rank->iactive &= ~*r;
- vgic_unlock_rank(v, rank);
+ vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ITARGETSR ... GICD_ITARGETSR + 7:
@@ -379,7 +381,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
!((target & 0xff) && (target & (0xff << 8)) &&
(target & (0xff << 16)) && (target & (0xff << 24))))
goto write_ignore;
- vgic_lock_rank(v, rank);
+ vgic_lock_rank(v, rank, flags);
i = 0;
while ( (i = find_next_bit(&target, 32, i)) < 32 )
{
@@ -406,7 +408,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
else
vgic_byte_write(&rank->itargets[REG_RANK_INDEX(8,
gicd_reg - GICD_ITARGETSR, DABT_WORD)], target, gicd_reg);
- vgic_unlock_rank(v, rank);
+ vgic_unlock_rank(v, rank, flags);
return 1;
}
@@ -414,14 +416,14 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
rank = vgic_rank_offset(v, 8, gicd_reg - GICD_IPRIORITYR, DABT_WORD);
if ( rank == NULL) goto write_ignore;
- vgic_lock_rank(v, rank);
+ vgic_lock_rank(v, rank, flags);
if ( dabt.size == DABT_WORD )
rank->ipriority[REG_RANK_INDEX(8, gicd_reg - GICD_IPRIORITYR,
DABT_WORD)] = *r;
else
vgic_byte_write(&rank->ipriority[REG_RANK_INDEX(8,
gicd_reg - GICD_IPRIORITYR, DABT_WORD)], *r, gicd_reg);
- vgic_unlock_rank(v, rank);
+ vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_ICFGR: /* SGIs */
@@ -433,9 +435,9 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
if ( dabt.size != DABT_WORD ) goto bad_width;
rank = vgic_rank_offset(v, 2, gicd_reg - GICD_ICFGR, DABT_WORD);
if ( rank == NULL) goto write_ignore;
- vgic_lock_rank(v, rank);
+ vgic_lock_rank(v, rank, flags);
rank->icfg[REG_RANK_INDEX(2, gicd_reg - GICD_ICFGR, DABT_WORD)] = *r;
- vgic_unlock_rank(v, rank);
+ vgic_unlock_rank(v, rank, flags);
return 1;
case GICD_NSACR ... GICD_NSACRN:
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index ce4457e..f86a91b 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -158,10 +158,11 @@ struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
struct domain *d = v->domain;
struct vcpu *v_target;
struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
+ unsigned long flags;
- vgic_lock_rank(v, rank);
+ vgic_lock_rank(v, rank, flags);
v_target = d->arch.vgic.handler->get_target_vcpu(v, irq);
- vgic_unlock_rank(v, rank);
+ vgic_unlock_rank(v, rank, flags);
return v_target;
}
@@ -367,6 +368,10 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
unsigned long flags;
bool_t running;
+ vgic_lock_rank(v, rank, flags);
+ priority = vgic_byte_read(rank->ipriority[REG_RANK_INDEX(8, irq, DABT_WORD)], 0, irq & 0x3);
+ vgic_unlock_rank(v, rank, flags);
+
spin_lock_irqsave(&v->arch.vgic.lock, flags);
/* vcpu offline */
@@ -384,8 +389,6 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
goto out;
}
- priority = vgic_byte_read(rank->ipriority[REG_RANK_INDEX(8, irq, DABT_WORD)], 0, irq & 0x3);
-
n->irq = irq;
n->priority = priority;
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 9b1db04..338ba03 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -107,8 +107,8 @@ struct vgic_ops {
#define vgic_lock(v) spin_lock_irq(&(v)->domain->arch.vgic.lock)
#define vgic_unlock(v) spin_unlock_irq(&(v)->domain->arch.vgic.lock)
-#define vgic_lock_rank(v, r) spin_lock(&(r)->lock)
-#define vgic_unlock_rank(v, r) spin_unlock(&(r)->lock)
+#define vgic_lock_rank(v, r, flags) spin_lock_irqsave(&(r)->lock, flags)
+#define vgic_unlock_rank(v, r, flags) spin_unlock_irqrestore(&(r)->lock, flags)
/*
* Rank containing GICD_<FOO><n> for GICD_<FOO> with
--
1.7.10.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v10 09/10] xen: introduce bit access macros for the IRQ line status flags
2014-08-05 11:25 [PATCH v10 00/10] gic and vgic fixes and improvements Stefano Stabellini
` (7 preceding siblings ...)
2014-08-05 11:26 ` [PATCH v10 08/10] xen/arm: take the rank lock before accessing ipriority Stefano Stabellini
@ 2014-08-05 11:26 ` Stefano Stabellini
2014-08-05 11:26 ` [PATCH v10 10/10] xen/arm: make accesses to desc->status flags atomic Stefano Stabellini
9 siblings, 0 replies; 18+ messages in thread
From: Stefano Stabellini @ 2014-08-05 11:26 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, Ian.Campbell, Jan Beulich, Stefano Stabellini
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
CC: Jan Beulich <jbeulich@suse.com>
---
Changes in v9:
- do not rename IRQF_SHARED to IRQ_SHARED.
---
xen/include/xen/irq.h | 27 ++++++++++++++++++---------
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
index 40c0f3f..96d818f 100644
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -22,15 +22,24 @@ struct irqaction {
/*
* IRQ line status.
*/
-#define IRQ_INPROGRESS (1u<<0) /* IRQ handler active - do not enter! */
-#define IRQ_DISABLED (1u<<1) /* IRQ disabled - do not enter! */
-#define IRQ_PENDING (1u<<2) /* IRQ pending - replay on enable */
-#define IRQ_REPLAY (1u<<3) /* IRQ has been replayed but not acked yet */
-#define IRQ_GUEST (1u<<4) /* IRQ is handled by guest OS(es) */
-#define IRQ_MOVE_PENDING (1u<<5) /* IRQ is migrating to another CPUs */
-#define IRQ_PER_CPU (1u<<6) /* IRQ is per CPU */
-#define IRQ_GUEST_EOI_PENDING (1u<<7) /* IRQ was disabled, pending a guest EOI */
-#define IRQF_SHARED (1<<8) /* IRQ is shared */
+#define _IRQ_INPROGRESS 0 /* IRQ handler active - do not enter! */
+#define _IRQ_DISABLED 1 /* IRQ disabled - do not enter! */
+#define _IRQ_PENDING 2 /* IRQ pending - replay on enable */
+#define _IRQ_REPLAY 3 /* IRQ has been replayed but not acked yet */
+#define _IRQ_GUEST 4 /* IRQ is handled by guest OS(es) */
+#define _IRQ_MOVE_PENDING 5 /* IRQ is migrating to another CPUs */
+#define _IRQ_PER_CPU 6 /* IRQ is per CPU */
+#define _IRQ_GUEST_EOI_PENDING 7 /* IRQ was disabled, pending a guest EOI */
+#define _IRQF_SHARED 8 /* IRQ is shared */
+#define IRQ_INPROGRESS (1u<<_IRQ_INPROGRESS)
+#define IRQ_DISABLED (1u<<_IRQ_DISABLED)
+#define IRQ_PENDING (1u<<_IRQ_PENDING)
+#define IRQ_REPLAY (1u<<_IRQ_REPLAY)
+#define IRQ_GUEST (1u<<_IRQ_GUEST)
+#define IRQ_MOVE_PENDING (1u<<_IRQ_MOVE_PENDING)
+#define IRQ_PER_CPU (1u<<_IRQ_PER_CPU)
+#define IRQ_GUEST_EOI_PENDING (1u<<_IRQ_GUEST_EOI_PENDING)
+#define IRQF_SHARED (1u<<_IRQF_SHARED)
/* Special IRQ numbers. */
#define AUTO_ASSIGN_IRQ (-1)
--
1.7.10.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v10 10/10] xen/arm: make accesses to desc->status flags atomic
2014-08-05 11:25 [PATCH v10 00/10] gic and vgic fixes and improvements Stefano Stabellini
` (8 preceding siblings ...)
2014-08-05 11:26 ` [PATCH v10 09/10] xen: introduce bit access macros for the IRQ line status flags Stefano Stabellini
@ 2014-08-05 11:26 ` Stefano Stabellini
2014-08-05 12:35 ` Julien Grall
9 siblings, 1 reply; 18+ messages in thread
From: Stefano Stabellini @ 2014-08-05 11:26 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, Ian.Campbell, Stefano Stabellini
This way we don't need to take the desc->lock in order to access
desc->status in many of the gic and vgic functions.
Using *_bit manipulation functions on desc->status is safe on arm64:
status is an unsigned int but is the first field of a struct that
contains pointers, therefore the alignement of the struct is at least 8
bytes.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
Changes in v10:
- add in-code comment;
- fix _IRQF_SHARED renaming.
Changes in v2:
- rebase on ab78724fc5628318b172b4344f7280621a151e1b.
---
xen/arch/arm/gic-v2.c | 4 ++--
xen/arch/arm/gic.c | 6 +++---
xen/arch/arm/irq.c | 33 +++++++++++++++++----------------
xen/include/xen/irq.h | 5 +++++
4 files changed, 27 insertions(+), 21 deletions(-)
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index da60a41..78ad4de 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -516,7 +516,7 @@ static void gicv2_irq_enable(struct irq_desc *desc)
ASSERT(spin_is_locked(&desc->lock));
spin_lock_irqsave(&gicv2.lock, flags);
- desc->status &= ~IRQ_DISABLED;
+ clear_bit(_IRQ_DISABLED, &desc->status);
dsb(sy);
/* Enable routing */
writel_gicd((1u << (irq % 32)), GICD_ISENABLER + (irq / 32) * 4);
@@ -533,7 +533,7 @@ static void gicv2_irq_disable(struct irq_desc *desc)
spin_lock_irqsave(&gicv2.lock, flags);
/* Disable routing */
writel_gicd(1u << (irq % 32), GICD_ICENABLER + (irq / 32) * 4);
- desc->status |= IRQ_DISABLED;
+ set_bit(_IRQ_DISABLED, &desc->status);
spin_unlock_irqrestore(&gicv2.lock, flags);
}
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 2aa9500..6611ba0 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -115,7 +115,7 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
{
ASSERT(priority <= 0xff); /* Only 8 bits of priority */
ASSERT(desc->irq < gic_number_lines());/* Can't route interrupts that don't exist */
- ASSERT(desc->status & IRQ_DISABLED);
+ ASSERT(test_bit(_IRQ_DISABLED, &desc->status));
ASSERT(spin_is_locked(&desc->lock));
desc->handler = gic_hw_ops->gic_host_irq_type;
@@ -133,7 +133,7 @@ void gic_route_irq_to_guest(struct domain *d, struct irq_desc *desc,
ASSERT(spin_is_locked(&desc->lock));
desc->handler = gic_hw_ops->gic_guest_irq_type;
- desc->status |= IRQ_GUEST;
+ set_bit(_IRQ_GUEST, &desc->status);
gic_set_irq_properties(desc, cpumask_of(smp_processor_id()), GIC_PRI_IRQ);
@@ -369,7 +369,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
if ( p->desc != NULL )
{
- p->desc->status &= ~IRQ_INPROGRESS;
+ clear_bit(_IRQ_INPROGRESS, &p->desc->status);
if ( platform_has_quirk(PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI) )
gic_hw_ops->deactivate_irq(p->desc);
}
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 7150c7a..25ecf1d 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -126,7 +126,7 @@ static inline struct domain *irq_get_domain(struct irq_desc *desc)
{
ASSERT(spin_is_locked(&desc->lock));
- if ( !(desc->status & IRQ_GUEST) )
+ if ( !test_bit(_IRQ_GUEST, &desc->status) )
return dom_xen;
ASSERT(desc->action != NULL);
@@ -195,13 +195,13 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
goto out;
}
- if ( desc->status & IRQ_GUEST )
+ if ( test_bit(_IRQ_GUEST, &desc->status) )
{
struct domain *d = irq_get_domain(desc);
desc->handler->end(desc);
- desc->status |= IRQ_INPROGRESS;
+ set_bit(_IRQ_INPROGRESS, &desc->status);
desc->arch.eoi_cpu = smp_processor_id();
/* the irq cannot be a PPI, we only support delivery of SPIs to
@@ -210,22 +210,23 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
goto out_no_end;
}
- desc->status |= IRQ_PENDING;
+ set_bit(_IRQ_PENDING, &desc->status);
/*
* Since we set PENDING, if another processor is handling a different
* instance of this same irq, the other processor will take care of it.
*/
- if ( desc->status & (IRQ_DISABLED | IRQ_INPROGRESS) )
+ if ( test_bit(_IRQ_DISABLED, &desc->status) ||
+ test_bit(_IRQ_INPROGRESS, &desc->status) )
goto out;
- desc->status |= IRQ_INPROGRESS;
+ set_bit(_IRQ_INPROGRESS, &desc->status);
- while ( desc->status & IRQ_PENDING )
+ while ( test_bit(_IRQ_PENDING, &desc->status) )
{
struct irqaction *action;
- desc->status &= ~IRQ_PENDING;
+ clear_bit(_IRQ_PENDING, &desc->status);
action = desc->action;
spin_unlock_irq(&desc->lock);
@@ -239,7 +240,7 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
spin_lock_irq(&desc->lock);
}
- desc->status &= ~IRQ_INPROGRESS;
+ clear_bit(_IRQ_INPROGRESS, &desc->status);
out:
desc->handler->end(desc);
@@ -282,13 +283,13 @@ void release_irq(unsigned int irq, const void *dev_id)
if ( !desc->action )
{
desc->handler->shutdown(desc);
- desc->status &= ~IRQ_GUEST;
+ clear_bit(_IRQ_GUEST, &desc->status);
}
spin_unlock_irqrestore(&desc->lock,flags);
/* Wait to make sure it's not being used on another CPU */
- do { smp_mb(); } while ( desc->status & IRQ_INPROGRESS );
+ do { smp_mb(); } while ( test_bit(_IRQ_INPROGRESS, &desc->status) );
if ( action->free_on_release )
xfree(action);
@@ -305,13 +306,13 @@ static int __setup_irq(struct irq_desc *desc, unsigned int irqflags,
* - if the IRQ is marked as shared
* - dev_id is not NULL when IRQF_SHARED is set
*/
- if ( desc->action != NULL && (!(desc->status & IRQF_SHARED) || !shared) )
+ if ( desc->action != NULL && (!test_bit(_IRQF_SHARED, &desc->status) || !shared) )
return -EINVAL;
if ( shared && new->dev_id == NULL )
return -EINVAL;
if ( shared )
- desc->status |= IRQF_SHARED;
+ set_bit(_IRQF_SHARED, &desc->status);
new->next = desc->action;
dsb(ish);
@@ -332,7 +333,7 @@ int setup_irq(unsigned int irq, unsigned int irqflags, struct irqaction *new)
spin_lock_irqsave(&desc->lock, flags);
- if ( desc->status & IRQ_GUEST )
+ if ( test_bit(_IRQ_GUEST, &desc->status) )
{
struct domain *d = irq_get_domain(desc);
@@ -396,10 +397,10 @@ int route_irq_to_guest(struct domain *d, unsigned int irq,
{
struct domain *ad = irq_get_domain(desc);
- if ( (desc->status & IRQ_GUEST) && d == ad )
+ if ( test_bit(_IRQ_GUEST, &desc->status) && d == ad )
goto out;
- if ( desc->status & IRQ_GUEST )
+ if ( test_bit(_IRQ_GUEST, &desc->status) )
printk(XENLOG_ERR "ERROR: IRQ %u is already used by domain %u\n",
irq, ad->domain_id);
else
diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
index 96d818f..ffb5932 100644
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -76,6 +76,11 @@ struct msi_desc;
* This is the "IRQ descriptor", which contains various information
* about the irq, including what kind of hardware handling it has,
* whether it is disabled etc etc.
+ *
+ * Note: on ARMv8 we can use normal bit manipulation functions to access
+ * the status field because struct irq_desc contains pointers, therefore
+ * the alignment of the struct is at least 8 bytes and status is the
+ * first field.
*/
typedef struct irq_desc {
unsigned int status; /* IRQ status */
--
1.7.10.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v10 10/10] xen/arm: make accesses to desc->status flags atomic
2014-08-05 11:26 ` [PATCH v10 10/10] xen/arm: make accesses to desc->status flags atomic Stefano Stabellini
@ 2014-08-05 12:35 ` Julien Grall
0 siblings, 0 replies; 18+ messages in thread
From: Julien Grall @ 2014-08-05 12:35 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel; +Cc: julien.grall, Ian.Campbell
Hi Stefano,
On 08/05/2014 12:26 PM, Stefano Stabellini wrote:
> This way we don't need to take the desc->lock in order to access
> desc->status in many of the gic and vgic functions.
>
> Using *_bit manipulation functions on desc->status is safe on arm64:
> status is an unsigned int but is the first field of a struct that
> contains pointers, therefore the alignement of the struct is at least 8
> bytes.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 18+ messages in thread