Hi Andre,
On 11/05/17 18:53, Andre Przywara wrote:
When LPIs get unmapped by a guest, they might still be in some LR of
some VCPU. Nevertheless we remove the corresponding pending_irq
(possibly freeing it), and detect this case (irq_to_pending() returns
NULL) when the LR gets cleaned up later.
However a *new* LPI may get mapped with the same number while the old
LPI is *still* in some LR. To avoid getting the wrong state, we mark
every newly mapped LPI as PRISTINE, which means: has never been in an
LR before. If we detect the LPI in an LR anyway, it must have been an
older one, which we can simply retire.
Before inserting such a PRISTINE LPI into an LR, we must make sure that
it's not already in another LR, as the architecture forbids two
interrupts with the same virtual IRQ number on one CPU.
Signed-off-by: Andre Przywara <andre.przyw...@arm.com>
---
xen/arch/arm/gic.c | 55 +++++++++++++++++++++++++++++++++++++++++-----
xen/include/asm-arm/vgic.h | 6 +++++
2 files changed, 56 insertions(+), 5 deletions(-)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index fd3fa05..8bf0578 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -375,6 +375,8 @@ static inline void gic_set_lr(int lr, struct pending_irq *p,
{
ASSERT(!local_irq_is_enabled());
+ clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status);
+
gic_hw_ops->update_lr(lr, p, state);
set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
@@ -442,12 +444,41 @@ void gic_raise_inflight_irq(struct vcpu *v, unsigned int
virtual_irq)
#endif
}
+/*
+ * Find an unused LR to insert an IRQ into. If this new interrupt is a
+ * PRISTINE LPI, scan the other LRs to avoid inserting the same IRQ twice.
This replicate here a part of the commit message regarding the
pending_irq structure. So we have background in the code of why going
through the LRs.
+ */
+static int gic_find_unused_lr(struct vcpu *v, struct pending_irq *p, int lr)
This should be unsigned for both the return and the lr variable. Also
please explain the goal of the variable because it is not clear from the
callers.
+{
+ unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
+ unsigned long *lr_mask = (unsigned long *) &this_cpu(lr_mask);
+ struct gic_lr lr_val;
+
+ ASSERT(spin_is_locked(&v->arch.vgic.lock));
+
+ if ( test_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status) )
+ {
+ int used_lr = 0;
find_next_bit return unsigned long. So likely you want to use either
unsigned int or unsigned long here.
+
+ while ( (used_lr = find_next_bit(lr_mask, nr_lrs, used_lr)) < nr_lrs )
+ {
+ gic_hw_ops->read_lr(used_lr, &lr_val);
+ if ( lr_val.virq == p->irq )
+ return used_lr;
+ }
+ }
+
+ lr = find_next_zero_bit(lr_mask, nr_lrs, lr);
+
+ return lr;
+}
+
void gic_raise_guest_irq(struct vcpu *v, unsigned int virtual_irq,
unsigned int priority)
{
- int i;
- unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
struct pending_irq *p = irq_to_pending(v, virtual_irq);
+ unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
Why did you move around the variable? Please avoid such things, it is
not like the ITS series is easy to review...
+ int i = nr_lrs;
I don't understand why you initialize i to nr_lrs. i will always have
the value reset by the return of gic_find_unused_lr before been used.
ASSERT(spin_is_locked(&v->arch.vgic.lock));
@@ -457,7 +488,8 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int
virtual_irq,
if ( v == current && list_empty(&v->arch.vgic.lr_pending) )
{
- i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs);
+ i = gic_find_unused_lr(v, p, 0);
+
if (i < nr_lrs) {
set_bit(i, &this_cpu(lr_mask));
gic_set_lr(i, p, GICH_LR_PENDING);
@@ -509,7 +541,17 @@ static void gic_update_one_lr(struct vcpu *v, int i)
}
else if ( lr_val.state & GICH_LR_PENDING )
{
- int q __attribute__ ((unused)) = test_and_clear_bit(GIC_IRQ_GUEST_QUEUED,
&p->status);
+ int q __attribute__ ((unused));
+
+ if ( test_and_clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status) )
+ {
+ gic_hw_ops->clear_lr(i);
+ clear_bit(i, &this_cpu(lr_mask));
+
+ return;
+ }
This code is very similar to what you do at the beginning when
pending_irq is NULL. I would prefer if you stick the check there rather
than try to address in all the different paths.
I know it is not necessary to do it for the active path as LPI does not
have active state, though this would have require an explanation in the
commit message). But it is easier to maintain the test_and_clear_bit in
a single place rather than spreading in 2 different one for little benefits.
+
+ q = test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
#ifdef GIC_DEBUG
if ( q )
gdprintk(XENLOG_DEBUG, "trying to inject irq=%d into d%dv%d, when it is
already pending in LR%d\n",
@@ -521,6 +563,9 @@ static void gic_update_one_lr(struct vcpu *v, int i)
gic_hw_ops->clear_lr(i);
clear_bit(i, &this_cpu(lr_mask));
+ if ( test_and_clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status) )
+ return;
+
This would avoid this check too.
if ( p->desc != NULL )
clear_bit(_IRQ_INPROGRESS, &p->desc->status);
clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
@@ -591,7 +636,7 @@ static void gic_restore_pending_irqs(struct vcpu *v)
inflight_r = &v->arch.vgic.inflight_irqs;
list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
{
- lr = find_next_zero_bit(&this_cpu(lr_mask), nr_lrs, lr);
+ lr = gic_find_unused_lr(v, p, lr);
if ( lr >= nr_lrs )
{
/* No more free LRs: find a lower priority irq to evict */
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 02732db..3fc4ceb 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -60,12 +60,18 @@ struct pending_irq
* vcpu while it is still inflight and on an GICH_LR register on the
* old vcpu.
*
+ * GIC_IRQ_GUEST_PRISTINE_LPI: the IRQ is a newly mapped LPI, which
+ * has never been in an LR before. This means that any trace of an
+ * LPI with the same number in an LR must be from an older LPI, which
+ * has been unmapped before.
+ *
*/
#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
+#define GIC_IRQ_GUEST_PRISTINE_LPI 5
unsigned long status;
struct irq_desc *desc; /* only set it the irq corresponds to a physical
irq */
unsigned int irq;
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
