Hi Andre,
On 21/07/17 20:59, Andre Przywara wrote:
So far a virtual interrupt's priority is stored in the irq_rank
structure, which covers multiple IRQs and has a single lock for this
group.
Generalize the already existing priority variable in struct pending_irq
to not only cover LPIs, but every IRQ. Access to this value is protected
by the per-IRQ lock.
Signed-off-by: Andre Przywara <andre.przyw...@arm.com>
---
xen/arch/arm/vgic-v2.c | 34 ++++++----------------------------
xen/arch/arm/vgic-v3.c | 36 ++++++++----------------------------
xen/arch/arm/vgic.c | 41 +++++++++++++++++------------------------
xen/include/asm-arm/vgic.h | 10 ----------
4 files changed, 31 insertions(+), 90 deletions(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index cf4ab89..ed7ff3b 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -171,6 +171,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v,
mmio_info_t *info,
struct vgic_irq_rank *rank;
int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
unsigned long flags;
+ unsigned int irq;
I am going to repeat the comment I made on v1, s/irq/virq.
perfc_incr(vgicd_reads);
@@ -250,22 +251,10 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v,
mmio_info_t *info,
goto read_as_zero;
case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
- {
- uint32_t ipriorityr;
- uint8_t rank_index;
-
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 read_as_zero;
- rank_index = REG_RANK_INDEX(8, gicd_reg - GICD_IPRIORITYR, DABT_WORD);
-
- vgic_lock_rank(v, rank, flags);
- ipriorityr = ACCESS_ONCE(rank->ipriorityr[rank_index]);
- vgic_unlock_rank(v, rank, flags);
- *r = vreg_reg32_extract(ipriorityr, info);
-
+ irq = gicd_reg - GICD_IPRIORITYR; /* 8 bit per IRQ, so IRQ = offset */
+ *r = vgic_fetch_irq_priority(v, irq, (dabt.size == DABT_BYTE) ? 1 : 4);
vgic_fetch_irq_priority assumes that there is always a pending_irq
associated to a given vIRQ. However, this is not true for any vIRQ above
the maximum supported by the vGIC (depends on the configuration).
This was protected by the check rank == NULL that now disappears.
return 1;
- }
case VREG32(0x7FC):
goto read_reserved;
@@ -415,6 +404,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v,
mmio_info_t *info,
int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
uint32_t tr;
unsigned long flags;
+ unsigned int irq;
Same here for the name.
perfc_incr(vgicd_writes);
@@ -498,23 +488,11 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v,
mmio_info_t *info,
goto write_ignore_32;
case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
- {
- uint32_t *ipriorityr, priority;
-
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, flags);
- ipriorityr = &rank->ipriorityr[REG_RANK_INDEX(8,
- gicd_reg -
GICD_IPRIORITYR,
- DABT_WORD)];
- priority = ACCESS_ONCE(*ipriorityr);
- vreg_reg32_update(&priority, r, info);
- ACCESS_ONCE(*ipriorityr) = priority;
- vgic_unlock_rank(v, rank, flags);
+ irq = gicd_reg - GICD_IPRIORITYR; /* 8 bit per IRQ, so IRQ = offset */
+ vgic_store_irq_priority(v, (dabt.size == DABT_BYTE) ? 1 : 4, irq, r);
Same here for the check.
return 1;
- }
case VREG32(0x7FC):
goto write_reserved;
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index ad9019e..e58e77e 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -677,6 +677,7 @@ static int __vgic_v3_distr_common_mmio_read(const char
*name, struct vcpu *v,
struct hsr_dabt dabt = info->dabt;
struct vgic_irq_rank *rank;
unsigned long flags;
+ unsigned int irq;
Same here for the name.
switch ( reg )
{
@@ -714,23 +715,11 @@ static int __vgic_v3_distr_common_mmio_read(const char
*name, struct vcpu *v,
goto read_as_zero;
case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
- {
- uint32_t ipriorityr;
- uint8_t rank_index;
-
if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
- rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, DABT_WORD);
- if ( rank == NULL ) goto read_as_zero;
- rank_index = REG_RANK_INDEX(8, reg - GICD_IPRIORITYR, DABT_WORD);
-
- vgic_lock_rank(v, rank, flags);
- ipriorityr = ACCESS_ONCE(rank->ipriorityr[rank_index]);
- vgic_unlock_rank(v, rank, flags);
-
- *r = vreg_reg32_extract(ipriorityr, info);
-
+ irq = reg - GICD_IPRIORITYR; /* 8 bit per IRQ, so IRQ = offset */
+ if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto read_as_zero;
You want to use vgic_num_irqs(v->domain) here. It might be nice to have
an helper checking the validity of an interrupt as I suspect you will
need this in quite a few places now.
+ *r = vgic_fetch_irq_priority(v, irq, (dabt.size == DABT_BYTE) ? 1 : 4);
return 1;
- }
case VRANGE32(GICD_ICFGR, GICD_ICFGRN):
{
@@ -774,6 +763,7 @@ static int __vgic_v3_distr_common_mmio_write(const char
*name, struct vcpu *v,
struct vgic_irq_rank *rank;
uint32_t tr;
unsigned long flags;
+ unsigned int irq;
Same for the name.
switch ( reg )
{
@@ -831,21 +821,11 @@ static int __vgic_v3_distr_common_mmio_write(const char
*name, struct vcpu *v,
goto write_ignore_32;
case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
- {
- uint32_t *ipriorityr, priority;
-
if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
- rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, DABT_WORD);
- if ( rank == NULL ) goto write_ignore;
- vgic_lock_rank(v, rank, flags);
- ipriorityr = &rank->ipriorityr[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR,
- DABT_WORD)];
- priority = ACCESS_ONCE(*ipriorityr);
- vreg_reg32_update(&priority, r, info);
- ACCESS_ONCE(*ipriorityr) = priority;
- vgic_unlock_rank(v, rank, flags);
+ irq = reg - GICD_IPRIORITYR; /* 8 bit per IRQ, so IRQ = offset */
+ if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto write_ignore;
Ditto.
+ vgic_store_irq_priority(v, (dabt.size == DABT_BYTE) ? 1 : 4, irq, r);
return 1;
- }
case VREG32(GICD_ICFGR): /* Restricted to configure SGIs */
goto write_ignore_32;
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index b2c9632..ddcd99b 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -231,18 +231,6 @@ struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned
int virq)
return v->domain->vcpu[target];
}
-static int vgic_get_virq_priority(struct vcpu *v, unsigned int virq)
-{
- struct vgic_irq_rank *rank;
-
- /* LPIs don't have a rank, also store their priority separately. */
- if ( is_lpi(virq) )
- return v->domain->arch.vgic.handler->lpi_get_priority(v->domain, virq);
-
- rank = vgic_rank_irq(v, virq);
- return ACCESS_ONCE(rank->priority[virq & INTERRUPT_RANK_MASK]);
-}
-
#define MAX_IRQS_PER_IPRIORITYR 4
uint32_t vgic_fetch_irq_priority(struct vcpu *v, unsigned int nrirqs,
unsigned int first_irq)
@@ -567,37 +555,40 @@ void vgic_clear_pending_irqs(struct vcpu *v)
void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
{
- uint8_t priority;
struct pending_irq *iter, *n;
- unsigned long flags;
+ unsigned long flags, vcpu_flags;
This renaming of flags -> vcpu_flags seems unwarrant to me. But it looks
like to me that you need two set of flags because vgic_irq_lock requires
to take a flag.
Technically we don't care about the flags for the second has we know the
IRQ are disabled. So I would introduce a new helper that simply lock +
maybe an ASSERT to check the IRQ was previously disabled. Something like:
ASSERT(!local_irq_enabled());
spin_lock(....);
You would also need the counter-part to unlock it.
bool running;
- spin_lock_irqsave(&v->arch.vgic.lock, flags);
+ spin_lock_irqsave(&v->arch.vgic.lock, vcpu_flags);
n = irq_to_pending(v, virq);
/* If an LPI has been removed, there is nothing to inject here. */
if ( unlikely(!n) )
{
- spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+ spin_unlock_irqrestore(&v->arch.vgic.lock, vcpu_flags);
return;
}
/* vcpu offline */
if ( test_bit(_VPF_down, &v->pause_flags) )
{
- spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+ spin_unlock_irqrestore(&v->arch.vgic.lock, vcpu_flags);
return;
}
+ vgic_irq_lock(n, flags);
It looks like to me that this locking should have been introduced in a
separate patch with the associated description. Because it is not really
related to that patch (you protected more than the priority). And I
think both the rank and pending_irq could cope. None of the patch before
would make it worst.
+
set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
if ( !list_empty(&n->inflight) )
{
bool update = test_bit(GIC_IRQ_GUEST_ENABLED, &n->status) &&
list_empty(&n->lr_queue) && (v == current);
+ int lr = ACCESS_ONCE(n->lr);
Why do you need the ACCESS_ONCE here? This does not seem related to this
patch.
+ vgic_irq_unlock(n, flags);
if ( update )
- gic_update_one_lr(v, n->lr);
+ gic_update_one_lr(v, lr);
#ifdef GIC_DEBUG
else
gdprintk(XENLOG_DEBUG, "trying to inject irq=%u into d%dv%d, when it is
still lr_pending\n",
@@ -606,24 +597,26 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int
virq)
goto out;
}
- priority = vgic_get_virq_priority(v, virq);
- n->cur_priority = priority;
+ n->cur_priority = n->priority;
/* the irq is enabled */
if ( test_bit(GIC_IRQ_GUEST_ENABLED, &n->status) )
- gic_raise_guest_irq(v, virq, priority);
+ gic_raise_guest_irq(v, virq, n->cur_priority);
list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight )
{
- if ( iter->cur_priority > priority )
+ if ( iter->cur_priority > n->cur_priority )
If I am not mistaken cur_priority is protected by the vCPU lock and not
the pending_irq lock. If so, the comment in patch #1 should be updated.
{
list_add_tail(&n->inflight, &iter->inflight);
- goto out;
+ goto out_unlock_irq;
}
}
list_add_tail(&n->inflight, &v->arch.vgic.inflight_irqs);
+
+out_unlock_irq:
+ vgic_irq_unlock(n, flags);
out:
- spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+ spin_unlock_irqrestore(&v->arch.vgic.lock, vcpu_flags);
/* we have a new higher priority irq, inject it into the guest */
running = v->is_running;
vcpu_unblock(v);
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index f3791c8..59d52c6 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -113,16 +113,6 @@ struct vgic_irq_rank {
uint32_t icfg[2];
/*
- * Provide efficient access to the priority of an vIRQ while keeping
- * the emulation simple.
- * Note, this is working fine as long as Xen is using little endian.
- */
- union {
- uint8_t priority[32];
- uint32_t ipriorityr[8];
- };
-
- /*
* 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
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
