Hi Andre,
On 21/07/17 21:00, Andre Przywara wrote:
The VCPU a shared virtual IRQ is targeting is currently stored in the
irq_rank structure.
For LPIs we already store the target VCPU in struct pending_irq, so
move SPIs over as well.
The ITS code, which was using this field already, was so far using the
VCPU lock to protect the pending_irq, so move this over to the new lock.
Signed-off-by: Andre Przywara <andre.przyw...@arm.com>
---
xen/arch/arm/vgic-v2.c | 56 +++++++++++++++--------------------
xen/arch/arm/vgic-v3-its.c | 9 +++---
xen/arch/arm/vgic-v3.c | 69 ++++++++++++++++++++-----------------------
xen/arch/arm/vgic.c | 73 +++++++++++++++++++++-------------------------
xen/include/asm-arm/vgic.h | 13 +++------
5 files changed, 96 insertions(+), 124 deletions(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 0c8a598..c7ed3ce 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -66,19 +66,22 @@ void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t
csize,
*
* Note the byte offset will be aligned to an ITARGETSR<n> boundary.
*/
-static uint32_t vgic_fetch_itargetsr(struct vgic_irq_rank *rank,
- unsigned int offset)
+static uint32_t vgic_fetch_itargetsr(struct vcpu *v, unsigned int offset)
{
uint32_t reg = 0;
unsigned int i;
+ unsigned long flags;
- ASSERT(spin_is_locked(&rank->lock));
-
- offset &= INTERRUPT_RANK_MASK;
offset &= ~(NR_TARGETS_PER_ITARGETSR - 1);
for ( i = 0; i < NR_TARGETS_PER_ITARGETSR; i++, offset++ )
- reg |= (1 << read_atomic(&rank->vcpu[offset])) << (i *
NR_BITS_PER_TARGET);
+ {
+ struct pending_irq *p = irq_to_pending(v, offset);
+
+ vgic_irq_lock(p, flags);
+ reg |= (1 << p->vcpu_id) << (i * NR_BITS_PER_TARGET);
+ vgic_irq_unlock(p, flags);
+ }
return reg;
}
@@ -89,32 +92,29 @@ static uint32_t vgic_fetch_itargetsr(struct vgic_irq_rank
*rank,
*
* Note the byte offset will be aligned to an ITARGETSR<n> boundary.
*/
-static void vgic_store_itargetsr(struct domain *d, struct vgic_irq_rank *rank,
+static void vgic_store_itargetsr(struct domain *d,
unsigned int offset, uint32_t itargetsr)
{
unsigned int i;
unsigned int virq;
- ASSERT(spin_is_locked(&rank->lock));
-
/*
* The ITARGETSR0-7, used for SGIs/PPIs, are implemented RO in the
* emulation and should never call this function.
*
- * They all live in the first rank.
+ * They all live in the first four bytes of ITARGETSR.
*/
- BUILD_BUG_ON(NR_INTERRUPT_PER_RANK != 32);
- ASSERT(rank->index >= 1);
+ ASSERT(offset >= 4);
- offset &= INTERRUPT_RANK_MASK;
+ virq = offset;
offset &= ~(NR_TARGETS_PER_ITARGETSR - 1);
- virq = rank->index * NR_INTERRUPT_PER_RANK + offset;
-
for ( i = 0; i < NR_TARGETS_PER_ITARGETSR; i++, offset++, virq++ )
{
unsigned int new_target, old_target;
+ unsigned long flags;
uint8_t new_mask;
+ struct pending_irq *p = spi_to_pending(d, virq);
/*
* Don't need to mask as we rely on new_mask to fit for only one
@@ -151,16 +151,14 @@ static void vgic_store_itargetsr(struct domain *d, struct
vgic_irq_rank *rank,
/* The vCPU ID always starts from 0 */
new_target--;
- old_target = read_atomic(&rank->vcpu[offset]);
+ vgic_irq_lock(p, flags);
+ old_target = p->vcpu_id;
/* Only migrate the vIRQ if the target vCPU has changed */
if ( new_target != old_target )
- {
- if ( vgic_migrate_irq(d->vcpu[old_target],
- d->vcpu[new_target],
- virq) )
- write_atomic(&rank->vcpu[offset], new_target);
- }
+ vgic_migrate_irq(p, &flags, d->vcpu[new_target]);
Why do you need to pass a pointer on the flags and not directly the value?
+ else
+ vgic_irq_unlock(p, flags);
}
}
@@ -264,11 +262,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v,
mmio_info_t *info,
uint32_t itargetsr;
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 read_as_zero;
- vgic_lock_rank(v, rank, flags);
- itargetsr = vgic_fetch_itargetsr(rank, gicd_reg - GICD_ITARGETSR);
- vgic_unlock_rank(v, rank, flags);
+ itargetsr = vgic_fetch_itargetsr(v, gicd_reg - GICD_ITARGETSR);
You need a check on the IRQ to avoid calling vgic_fetch_itargetsr with
an IRQ not handled.
*r = vreg_reg32_extract(itargetsr, info);
return 1;
@@ -498,14 +492,10 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v,
mmio_info_t *info,
uint32_t itargetsr;
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;
- vgic_lock_rank(v, rank, flags);
- itargetsr = vgic_fetch_itargetsr(rank, gicd_reg - GICD_ITARGETSR);
+ itargetsr = vgic_fetch_itargetsr(v, gicd_reg - GICD_ITARGETSR);
Ditto.
vreg_reg32_update(&itargetsr, r, info);
- vgic_store_itargetsr(v->domain, rank, gicd_reg - GICD_ITARGETSR,
+ vgic_store_itargetsr(v->domain, gicd_reg - GICD_ITARGETSR,
itargetsr);
The itargetsr is updated using read-modify-write and should be atomic.
This was protected by the rank lock that you now dropped. So what would
be the locking here?
- vgic_unlock_rank(v, rank, flags);
return 1;
}
diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index 682ce10..1020ebe 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -628,7 +628,7 @@ static int its_discard_event(struct virt_its *its,
/* Cleanup the pending_irq and disconnect it from the LPI. */
gic_remove_irq_from_queues(vcpu, p);
- vgic_init_pending_irq(p, INVALID_LPI);
+ vgic_init_pending_irq(p, INVALID_LPI, INVALID_VCPU_ID);
spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags);
@@ -768,7 +768,7 @@ static int its_handle_mapti(struct virt_its *its, uint64_t
*cmdptr)
if ( !pirq )
goto out_remove_mapping;
- vgic_init_pending_irq(pirq, intid);
+ vgic_init_pending_irq(pirq, intid, vcpu->vcpu_id);
/*
* Now read the guest's property table to initialize our cached state.
@@ -781,7 +781,6 @@ static int its_handle_mapti(struct virt_its *its, uint64_t
*cmdptr)
if ( ret )
goto out_remove_host_entry;
- pirq->vcpu_id = vcpu->vcpu_id;
/*
* Mark this LPI as new, so any older (now unmapped) LPI in any LR
* can be easily recognised as such.
@@ -852,9 +851,9 @@ static int its_handle_movi(struct virt_its *its, uint64_t
*cmdptr)
*/
spin_lock_irqsave(&ovcpu->arch.vgic.lock, flags);
+ vgic_irq_lock(p, flags);
p->vcpu_id = nvcpu->vcpu_id;
-
- spin_unlock_irqrestore(&ovcpu->arch.vgic.lock, flags);
+ vgic_irq_unlock(p, flags);
/*
* TODO: Investigate if and how to migrate an already pending LPI. This
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index e9e36eb..e9d46af 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -100,18 +100,21 @@ static struct vcpu *vgic_v3_irouter_to_vcpu(struct domain
*d, uint64_t irouter)
*
* Note the byte offset will be aligned to an IROUTER<n> boundary.
*/
-static uint64_t vgic_fetch_irouter(struct vgic_irq_rank *rank,
- unsigned int offset)
+static uint64_t vgic_fetch_irouter(struct vcpu *v, unsigned int offset)
{
- ASSERT(spin_is_locked(&rank->lock));
+ struct pending_irq *p;
+ unsigned long flags;
+ uint64_t aff;
/* There is exactly 1 vIRQ per IROUTER */
offset /= NR_BYTES_PER_IROUTER;
- /* Get the index in the rank */
- offset &= INTERRUPT_RANK_MASK;
+ p = irq_to_pending(v, offset);
+ vgic_irq_lock(p, flags);
+ aff = vcpuid_to_vaffinity(p->vcpu_id);
+ vgic_irq_unlock(p, flags);
- return vcpuid_to_vaffinity(read_atomic(&rank->vcpu[offset]));
+ return aff;
}
/*
@@ -120,10 +123,12 @@ static uint64_t vgic_fetch_irouter(struct vgic_irq_rank
*rank,
*
* Note the offset will be aligned to the appropriate boundary.
*/
-static void vgic_store_irouter(struct domain *d, struct vgic_irq_rank *rank,
+static void vgic_store_irouter(struct domain *d,
unsigned int offset, uint64_t irouter)
{
- struct vcpu *new_vcpu, *old_vcpu;
+ struct vcpu *new_vcpu;
+ struct pending_irq *p;
+ unsigned long flags;
unsigned int virq;
/* There is 1 vIRQ per IROUTER */
@@ -135,11 +140,10 @@ static void vgic_store_irouter(struct domain *d, struct
vgic_irq_rank *rank,
*/
ASSERT(virq >= 32);
- /* Get the index in the rank */
- offset &= virq & INTERRUPT_RANK_MASK;
+ p = spi_to_pending(d, virq);
+ vgic_irq_lock(p, flags);
new_vcpu = vgic_v3_irouter_to_vcpu(d, irouter);
- old_vcpu = d->vcpu[read_atomic(&rank->vcpu[offset])];
/*
* From the spec (see 8.9.13 in IHI 0069A), any write with an
@@ -149,16 +153,13 @@ static void vgic_store_irouter(struct domain *d, struct
vgic_irq_rank *rank,
* invalid vCPU. So for now, just ignore the write.
*
* TODO: Respect the spec
+ *
+ * Only migrate the IRQ if the target vCPU has changed
*/
- if ( !new_vcpu )
- return;
-
- /* Only migrate the IRQ if the target vCPU has changed */
- if ( new_vcpu != old_vcpu )
- {
- if ( vgic_migrate_irq(old_vcpu, new_vcpu, virq) )
- write_atomic(&rank->vcpu[offset], new_vcpu->vcpu_id);
- }
+ if ( new_vcpu && new_vcpu->vcpu_id != p->vcpu_id )
+ vgic_migrate_irq(p, &flags, new_vcpu);
+ else
+ vgic_irq_unlock(p, flags);
}
static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
@@ -1061,8 +1062,6 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v,
mmio_info_t *info,
register_t *r, void *priv)
{
struct hsr_dabt dabt = info->dabt;
- struct vgic_irq_rank *rank;
- unsigned long flags;
int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
perfc_incr(vgicd_reads);
@@ -1190,15 +1189,12 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v,
mmio_info_t *info,
case VRANGE64(GICD_IROUTER32, GICD_IROUTER1019):
{
uint64_t irouter;
+ unsigned int irq;
if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
- rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER,
- DABT_DOUBLE_WORD);
- if ( rank == NULL ) goto read_as_zero;
- vgic_lock_rank(v, rank, flags);
- irouter = vgic_fetch_irouter(rank, gicd_reg - GICD_IROUTER);
- vgic_unlock_rank(v, rank, flags);
-
+ irq = (gicd_reg - GICD_IROUTER) / 8;
+ if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto read_as_zero;
+ irouter = vgic_fetch_irouter(v, gicd_reg - GICD_IROUTER);
*r = vreg_reg64_extract(irouter, info);
return 1;
@@ -1264,8 +1260,6 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v,
mmio_info_t *info,
register_t r, void *priv)
{
struct hsr_dabt dabt = info->dabt;
- struct vgic_irq_rank *rank;
- unsigned long flags;
int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
perfc_incr(vgicd_writes);
@@ -1379,16 +1373,15 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v,
mmio_info_t *info,
case VRANGE64(GICD_IROUTER32, GICD_IROUTER1019):
{
uint64_t irouter;
+ unsigned int irq;
if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
- rank = vgic_rank_offset(v, 64, gicd_reg - GICD_IROUTER,
- DABT_DOUBLE_WORD);
- if ( rank == NULL ) goto write_ignore;
- vgic_lock_rank(v, rank, flags);
- irouter = vgic_fetch_irouter(rank, gicd_reg - GICD_IROUTER);
+ irq = (gicd_reg - GICD_IROUTER) / 8;
+ if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto write_ignore;
+
+ irouter = vgic_fetch_irouter(v, gicd_reg - GICD_IROUTER);
vreg_reg64_update(&irouter, r, info);
- vgic_store_irouter(v->domain, rank, gicd_reg - GICD_IROUTER, irouter);
- vgic_unlock_rank(v, rank, flags);
+ vgic_store_irouter(v->domain, gicd_reg - GICD_IROUTER, irouter);
Same here for the locking issue.
return 1;
}
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 0e6dfe5..f6532ee 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -61,7 +61,8 @@ struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned
int irq)
return vgic_get_rank(v, rank);
}
-void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq)
+void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq,
+ unsigned int vcpu_id)
{
/* The vcpu_id field must be big enough to hold a VCPU ID. */
BUILD_BUG_ON(BIT(sizeof(p->vcpu_id) * 8) < MAX_VIRT_CPUS);
@@ -71,27 +72,15 @@ void vgic_init_pending_irq(struct pending_irq *p, unsigned
int virq)
INIT_LIST_HEAD(&p->lr_queue);
spin_lock_init(&p->lock);
p->irq = virq;
- p->vcpu_id = INVALID_VCPU_ID;
+ p->vcpu_id = vcpu_id;
}
static void vgic_rank_init(struct vgic_irq_rank *rank, uint8_t index,
unsigned int vcpu)
{
- unsigned int i;
-
- /*
- * Make sure that the type chosen to store the target is able to
- * store an VCPU ID between 0 and the maximum of virtual CPUs
- * supported.
- */
- BUILD_BUG_ON((1 << (sizeof(rank->vcpu[0]) * 8)) < MAX_VIRT_CPUS);
-
spin_lock_init(&rank->lock);
rank->index = index;
-
- for ( i = 0; i < NR_INTERRUPT_PER_RANK; i++ )
- write_atomic(&rank->vcpu[i], vcpu);
}
int domain_vgic_register(struct domain *d, int *mmio_count)
@@ -142,9 +131,9 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
if ( d->arch.vgic.pending_irqs == NULL )
return -ENOMEM;
+ /* SPIs are routed to VCPU0 by default */
for (i=0; i<d->arch.vgic.nr_spis; i++)
- vgic_init_pending_irq(&d->arch.vgic.pending_irqs[i], i + 32);
-
+ vgic_init_pending_irq(&d->arch.vgic.pending_irqs[i], i + 32, 0);
/* SPIs are routed to VCPU0 by default */
for ( i = 0; i < DOMAIN_NR_RANKS(d); i++ )
vgic_rank_init(&d->arch.vgic.shared_irqs[i], i + 1, 0);
@@ -208,8 +197,9 @@ int vcpu_vgic_init(struct vcpu *v)
v->domain->arch.vgic.handler->vcpu_init(v);
memset(&v->arch.vgic.pending_irqs, 0, sizeof(v->arch.vgic.pending_irqs));
+ /* SGIs/PPIs are always routed to this VCPU */
for (i = 0; i < 32; i++)
- vgic_init_pending_irq(&v->arch.vgic.pending_irqs[i], i);
+ vgic_init_pending_irq(&v->arch.vgic.pending_irqs[i], i, v->vcpu_id);
INIT_LIST_HEAD(&v->arch.vgic.inflight_irqs);
INIT_LIST_HEAD(&v->arch.vgic.lr_pending);
@@ -268,10 +258,7 @@ struct vcpu *vgic_lock_vcpu_irq(struct vcpu *v, struct
pending_irq *p,
struct vcpu *vgic_get_target_vcpu(struct vcpu *v, struct pending_irq *p)
{
- struct vgic_irq_rank *rank = vgic_rank_irq(v, p->irq);
- int target = read_atomic(&rank->vcpu[p->irq & INTERRUPT_RANK_MASK]);
-
- return v->domain->vcpu[target];
+ return v->domain->vcpu[p->vcpu_id];
Do you need p to be locked for reading vcpu_id? If so, then an ASSERT
should be added. If not, then maybe you need an ACCESS_ONCE/read-atomic.
}
#define MAX_IRQS_PER_IPRIORITYR 4
@@ -360,57 +347,65 @@ void vgic_store_irq_config(struct vcpu *v, unsigned int
first_irq,
local_irq_restore(flags);
}
-bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
+bool vgic_migrate_irq(struct pending_irq *p, unsigned long *flags,
+ struct vcpu *new)
{
- unsigned long flags;
- struct pending_irq *p;
+ unsigned long vcpu_flags;
+ struct vcpu *old;
+ bool ret = false;
/* This will never be called for an LPI, as we don't migrate them. */
- ASSERT(!is_lpi(irq));
+ ASSERT(!is_lpi(p->irq));
- spin_lock_irqsave(&old->arch.vgic.lock, flags);
-
- p = irq_to_pending(old, irq);
+ ASSERT(spin_is_locked(&p->lock));
/* nothing to do for virtual interrupts */
if ( p->desc == NULL )
{
- spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
- return true;
+ ret = true;
+ goto out_unlock;
}
/* migration already in progress, no need to do anything */
if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
{
- gprintk(XENLOG_WARNING, "irq %u migration failed: requested while in
progress\n", irq);
- spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
- return false;
+ gprintk(XENLOG_WARNING, "irq %u migration failed: requested while in
progress\n", p->irq);
+ goto out_unlock;
}
+ p->vcpu_id = new->vcpu_id;
Something is wrong here. You update p->vcpu_id quite early. This means
if the IRQ fire whilst you are in vgic_migrate_irq, then you will call
use the new vCPU in vgic_vcpu_inject_irq but potential still in the old
list.
+
perfc_incr(vgic_irq_migrates);
if ( list_empty(&p->inflight) )
I was kind of expecting the old vCPU lock to be taken given that you
check p->inflight.
{
irq_set_affinity(p->desc, cpumask_of(new->processor));
- spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
- return true;
+ goto out_unlock;
}
+
/* If the IRQ is still lr_pending, re-inject it to the new vcpu */
if ( !list_empty(&p->lr_queue) )
{
+ old = vgic_lock_vcpu_irq(new, p, &vcpu_flags);
I may miss something here. The vCPU returned should be new, not old, right?
gic_remove_irq_from_queues(old, p);
irq_set_affinity(p->desc, cpumask_of(new->processor));
- spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
- vgic_vcpu_inject_irq(new, irq);
+
+ vgic_irq_unlock(p, *flags);
+ spin_unlock_irqrestore(&old->arch.vgic.lock, vcpu_flags);
+
+ vgic_vcpu_inject_irq(new, p->irq);
return true;
}
+
/* 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);
- return true;
+out_unlock:
+ vgic_irq_unlock(p, *flags);
+
+ return false;
}
void arch_move_irqs(struct vcpu *v)
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index ffd9a95..4b47a9b 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -112,13 +112,6 @@ struct vgic_irq_rank {
uint32_t ienable;
- /*
- * It's more convenient to store a target VCPU per vIRQ
- * than the register ITARGETSR/IROUTER itself.
- * Use atomic operations to read/write the vcpu fields to avoid
- * taking the rank lock.
- */
- uint8_t vcpu[32];
};
struct sgi_target {
@@ -217,7 +210,8 @@ extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v,
struct pending_irq *p);
extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq);
extern void vgic_vcpu_inject_spi(struct domain *d, unsigned int virq);
extern void vgic_clear_pending_irqs(struct vcpu *v);
-extern void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq);
+extern void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq,
+ unsigned int vcpu_id);
extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
extern struct pending_irq *spi_to_pending(struct domain *d, unsigned int irq);
extern struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v, int b, int n,
int s);
@@ -237,7 +231,8 @@ extern int vcpu_vgic_free(struct vcpu *v);
extern bool vgic_to_sgi(struct vcpu *v, register_t sgir,
enum gic_sgi_mode irqmode, int virq,
const struct sgi_target *target);
-extern bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int
irq);
+extern bool vgic_migrate_irq(struct pending_irq *p,
+ unsigned long *flags, struct vcpu *new);
/* Reserve a specific guest vIRQ */
extern bool vgic_reserve_virq(struct domain *d, unsigned int virq);
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
