* [RFC PATCH 00/10] ARM VGIC rework: remove rank, introduce per-IRQ lock
@ 2017-05-04 15:31 Andre Przywara
2017-05-04 15:31 ` [RFC PATCH 01/10] ARM: vGIC: remove rank lock from IRQ routing functions Andre Przywara
` (9 more replies)
0 siblings, 10 replies; 31+ messages in thread
From: Andre Przywara @ 2017-05-04 15:31 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini; +Cc: xen-devel
Hi,
this is a sketch of the first step of some ARM VGIC rework.
While working more closely with the code during the ITS patch series, we
identified some shortcomings with the existing code which should be fixed.
This series addresses two, somewhat related, fields:
It introduces a per-IRQ lock to protect our struct pending_irq, and moves
the data from the separate irq_rank structure into pending_irq. The latter
is useful because we don't have to deal with two locks, also it helps with
the ITS/LPI emulation.
The first two patches are somewhat fixes, which help to simplify the
following patches. Patch 03/10 introduces a lock to cover some members
of struct pending_irq, patch 04/10 introduces the proper lock/unlock
calls in functions dealing with the structure. At this point the lock
doesn't really protect anything, but the following four patches change that.
They move the member of struct irq_rank over to struct pending_irq.
We give up our optimized MMIO access, but in fact this was quite pointless
given the extremly low frequency of these traps compared to far more
dominant code paths dealing with IRQ injection and the entry/exit path.
Patch 09/10 introduces some put/get functionality for getting a struct
pending_irq pointer. For now this is pretty pointless on its own, but allows
to introduce ref-counting for LPIs more easily later on.
The final patch 10/10 then removes all remaining code having dealt with
the irq_rank, because we don't need this structure at all anymore.
This is more of a suggestion to pave the way for proper ITS emulation and
to get the discussion going. I am sure I missed some places where we need
the lock and introduces some other bugs, so please have a look and tell me
what you think.
There is more (admittedly controversial) rework in my pipe, which helps
to simplify the locking scheme.
The code can also be found on the vgic-rework/rfc branch here:
git://linux-arm.org/xen-ap.git
http://www.linux-arm.org/git?p=xen-ap.git;a=shortlog;h=refs/heads/vgic-rework/rfc
Cheers,
Andre
Andre Przywara (10):
ARM: vGIC: remove rank lock from IRQ routing functions
ARM: vGIC: rework gic_raise_*_irq() functions
ARM: vGIC: introduce and initialize pending_irq lock
ARM: vGIC: add struct pending_irq locking
ARM: vGIC: move priority from irq_rank to struct pending_irq
ARM: vGIC: move config from irq_rank to struct pending_irq
ARM: vGIC: move enable status from irq_rank to struct pending_irq
ARM: vGIC: move target vcpu from irq_rank to struct pending_irq
ARM: vGIC: introduce vgic_get/put_pending_irq
ARM: vGIC: remove struct irq_rank and support functions
xen/arch/arm/gic.c | 84 +++++++------
xen/arch/arm/vgic-v2.c | 157 +++++++++--------------
xen/arch/arm/vgic-v3.c | 190 +++++++++++++---------------
xen/arch/arm/vgic.c | 287 +++++++++++++++++++++----------------------
xen/include/asm-arm/domain.h | 6 +-
xen/include/asm-arm/event.h | 20 ++-
xen/include/asm-arm/gic.h | 5 +-
xen/include/asm-arm/vgic.h | 107 +++++-----------
8 files changed, 382 insertions(+), 474 deletions(-)
--
2.9.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* [RFC PATCH 01/10] ARM: vGIC: remove rank lock from IRQ routing functions
2017-05-04 15:31 [RFC PATCH 00/10] ARM VGIC rework: remove rank, introduce per-IRQ lock Andre Przywara
@ 2017-05-04 15:31 ` Andre Przywara
2017-05-04 15:53 ` Julien Grall
2017-05-04 15:31 ` [RFC PATCH 02/10] ARM: vGIC: rework gic_raise_*_irq() functions Andre Przywara
` (8 subsequent siblings)
9 siblings, 1 reply; 31+ messages in thread
From: Andre Przywara @ 2017-05-04 15:31 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini; +Cc: xen-devel
gic_route_irq_to_guest() and gic_remove_irq_from_guest() take the rank
lock, however never actually access the rank structure.
Avoid taking the lock in those two functions and remove some more
unneeded code on the way.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
xen/arch/arm/gic.c | 28 ++++------------------------
1 file changed, 4 insertions(+), 24 deletions(-)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index da19130..c734f66 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -136,25 +136,19 @@ void gic_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
struct irq_desc *desc, unsigned int priority)
{
- unsigned long flags;
/* Use vcpu0 to retrieve the pending_irq struct. Given that we only
* route SPIs to guests, it doesn't make any difference. */
- struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
- struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
- struct pending_irq *p = irq_to_pending(v_target, virq);
- int res = -EBUSY;
+ struct pending_irq *p = irq_to_pending(d->vcpu[0], virq);
ASSERT(spin_is_locked(&desc->lock));
/* Caller has already checked that the IRQ is an SPI */
ASSERT(virq >= 32);
ASSERT(virq < vgic_num_irqs(d));
- vgic_lock_rank(v_target, rank, flags);
-
if ( p->desc ||
/* The VIRQ should not be already enabled by the guest */
test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
- goto out;
+ return -EBUSY;
desc->handler = gic_hw_ops->gic_guest_irq_type;
set_bit(_IRQ_GUEST, &desc->status);
@@ -164,29 +158,20 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
gic_set_irq_priority(desc, priority);
p->desc = desc;
- res = 0;
-out:
- vgic_unlock_rank(v_target, rank, flags);
-
- return res;
+ return 0;
}
/* This function only works with SPIs for now */
int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
struct irq_desc *desc)
{
- struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
- struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
- struct pending_irq *p = irq_to_pending(v_target, virq);
- unsigned long flags;
+ struct pending_irq *p = irq_to_pending(d->vcpu[0], virq);
ASSERT(spin_is_locked(&desc->lock));
ASSERT(test_bit(_IRQ_GUEST, &desc->status));
ASSERT(p->desc == desc);
- vgic_lock_rank(v_target, rank, flags);
-
if ( d->is_dying )
{
desc->handler->shutdown(desc);
@@ -204,10 +189,7 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
*/
if ( test_bit(_IRQ_INPROGRESS, &desc->status) ||
!test_bit(_IRQ_DISABLED, &desc->status) )
- {
- vgic_unlock_rank(v_target, rank, flags);
return -EBUSY;
- }
}
clear_bit(_IRQ_GUEST, &desc->status);
@@ -215,8 +197,6 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
p->desc = NULL;
- vgic_unlock_rank(v_target, rank, flags);
-
return 0;
}
--
2.9.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [RFC PATCH 02/10] ARM: vGIC: rework gic_raise_*_irq() functions
2017-05-04 15:31 [RFC PATCH 00/10] ARM VGIC rework: remove rank, introduce per-IRQ lock Andre Przywara
2017-05-04 15:31 ` [RFC PATCH 01/10] ARM: vGIC: remove rank lock from IRQ routing functions Andre Przywara
@ 2017-05-04 15:31 ` Andre Przywara
2017-05-04 15:31 ` [RFC PATCH 03/10] ARM: vGIC: introduce and initialize pending_irq lock Andre Przywara
` (7 subsequent siblings)
9 siblings, 0 replies; 31+ messages in thread
From: Andre Przywara @ 2017-05-04 15:31 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini; +Cc: xen-devel
Currently gic_raise_inflight_irq() and gic_raise_guest_irq() are used
to queue interrupts to a VCPU, for which they are given a VCPU and an
interrupt number.
To allow proper locking and simplify further changes, change their prototypes
to take a struct pending_irq directly (since the callers have the
pointer already anyway).
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
xen/arch/arm/gic.c | 15 ++++++---------
xen/arch/arm/vgic.c | 6 +++---
xen/include/asm-arm/gic.h | 5 ++---
3 files changed, 11 insertions(+), 15 deletions(-)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index c734f66..67375a2 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -391,10 +391,8 @@ void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq)
spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
}
-void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq)
+void gic_raise_inflight_irq(struct vcpu *v, struct pending_irq *n)
{
- struct pending_irq *n = irq_to_pending(v, virtual_irq);
-
ASSERT(spin_is_locked(&v->arch.vgic.lock));
if ( list_empty(&n->lr_queue) )
@@ -405,12 +403,11 @@ void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq)
#ifdef GIC_DEBUG
else
gdprintk(XENLOG_DEBUG, "trying to inject irq=%u into d%dv%d, when it is still lr_pending\n",
- virtual_irq, v->domain->domain_id, v->vcpu_id);
+ n->irq, v->domain->domain_id, v->vcpu_id);
#endif
}
-void gic_raise_guest_irq(struct vcpu *v, unsigned int virtual_irq,
- unsigned int priority)
+void gic_raise_guest_irq(struct vcpu *v, struct pending_irq *p)
{
int i;
unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
@@ -422,12 +419,12 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int virtual_irq,
i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs);
if (i < nr_lrs) {
set_bit(i, &this_cpu(lr_mask));
- gic_set_lr(i, irq_to_pending(v, virtual_irq), GICH_LR_PENDING);
+ gic_set_lr(i, p, GICH_LR_PENDING);
return;
}
}
- gic_add_to_lr_pending(v, irq_to_pending(v, virtual_irq));
+ gic_add_to_lr_pending(v, p);
}
static void gic_update_one_lr(struct vcpu *v, int i)
@@ -480,7 +477,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
if ( test_bit(GIC_IRQ_GUEST_ENABLED, &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);
+ gic_raise_guest_irq(v, p);
else {
list_del_init(&p->inflight);
/*
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 83569b0..f359ecb 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -359,7 +359,7 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
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);
+ gic_raise_guest_irq(v_target, p);
spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
if ( p->desc != NULL )
{
@@ -485,7 +485,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
if ( !list_empty(&n->inflight) )
{
- gic_raise_inflight_irq(v, virq);
+ gic_raise_inflight_irq(v, n);
goto out;
}
@@ -493,7 +493,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
/* 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, n);
list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight )
{
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 836a103..8872934 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -240,9 +240,8 @@ extern void gic_clear_pending_irqs(struct vcpu *v);
extern int gic_events_need_delivery(void);
extern void init_maintenance_interrupt(void);
-extern void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
- unsigned int priority);
-extern void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq);
+extern void gic_raise_guest_irq(struct vcpu *v, struct pending_irq *p);
+extern void gic_raise_inflight_irq(struct vcpu *v, struct pending_irq *p);
extern void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq);
/* Accept an interrupt from the GIC and dispatch its handler */
--
2.9.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [RFC PATCH 03/10] ARM: vGIC: introduce and initialize pending_irq lock
2017-05-04 15:31 [RFC PATCH 00/10] ARM VGIC rework: remove rank, introduce per-IRQ lock Andre Przywara
2017-05-04 15:31 ` [RFC PATCH 01/10] ARM: vGIC: remove rank lock from IRQ routing functions Andre Przywara
2017-05-04 15:31 ` [RFC PATCH 02/10] ARM: vGIC: rework gic_raise_*_irq() functions Andre Przywara
@ 2017-05-04 15:31 ` Andre Przywara
2017-05-04 15:31 ` [RFC PATCH 04/10] ARM: vGIC: add struct pending_irq locking Andre Przywara
` (6 subsequent siblings)
9 siblings, 0 replies; 31+ messages in thread
From: Andre Przywara @ 2017-05-04 15:31 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini; +Cc: xen-devel
Currently we protect the pending_irq structure with the corresponding
VGIC VCPU lock. For future extensions this leads to problems (for
instance if an IRQ is migrating), so let's introduce a per-IRQ lock,
which protects the consistency of this structure independent from
any VCPU.
This patch just introduces and intializes the lock, it is not used
anywhere right now.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
xen/arch/arm/vgic.c | 1 +
xen/include/asm-arm/vgic.h | 8 ++++++++
2 files changed, 9 insertions(+)
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index f359ecb..f4ae454 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -64,6 +64,7 @@ static void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq)
{
INIT_LIST_HEAD(&p->inflight);
INIT_LIST_HEAD(&p->lr_queue);
+ spin_lock_init(&p->lock);
p->irq = virq;
}
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 544867a..e7322fc 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -81,6 +81,14 @@ struct pending_irq
* TODO: when implementing irq migration, taking only the current
* vgic lock is not going to be enough. */
struct list_head lr_queue;
+ /* The lock protects the consistency of this structure. A single status bit
+ * can be read and/or set without holding the lock using the atomic
+ * set_bit/clear_bit/test_bit functions, however accessing multiple bits or
+ * relating to other members in this struct requires the lock.
+ * The list_head members are protected by their corresponding VCPU lock,
+ * it is not sufficient to hold this pending_irq lock here to query or
+ * change list order or affiliation. */
+ spinlock_t lock;
};
#define NR_INTERRUPT_PER_RANK 32
--
2.9.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [RFC PATCH 04/10] ARM: vGIC: add struct pending_irq locking
2017-05-04 15:31 [RFC PATCH 00/10] ARM VGIC rework: remove rank, introduce per-IRQ lock Andre Przywara
` (2 preceding siblings ...)
2017-05-04 15:31 ` [RFC PATCH 03/10] ARM: vGIC: introduce and initialize pending_irq lock Andre Przywara
@ 2017-05-04 15:31 ` Andre Przywara
2017-05-04 16:21 ` Julien Grall
2017-05-04 16:54 ` Julien Grall
2017-05-04 15:31 ` [RFC PATCH 05/10] ARM: vGIC: move priority from irq_rank to struct pending_irq Andre Przywara
` (5 subsequent siblings)
9 siblings, 2 replies; 31+ messages in thread
From: Andre Przywara @ 2017-05-04 15:31 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini; +Cc: xen-devel
Introduce the proper locking sequence for the new pending_irq lock.
This takes the lock around multiple accesses to struct members,
also makes sure we observe the locking order (VGIC VCPU lock first,
then pending_irq lock).
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
xen/arch/arm/gic.c | 26 ++++++++++++++++++++++++++
xen/arch/arm/vgic.c | 12 +++++++++++-
2 files changed, 37 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 67375a2..e175e9b 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -351,6 +351,7 @@ void gic_disable_cpu(void)
static inline void gic_set_lr(int lr, struct pending_irq *p,
unsigned int state)
{
+ ASSERT(spin_is_locked(&p->lock));
ASSERT(!local_irq_is_enabled());
gic_hw_ops->update_lr(lr, p, state);
@@ -413,6 +414,7 @@ void gic_raise_guest_irq(struct vcpu *v, struct pending_irq *p)
unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
ASSERT(spin_is_locked(&v->arch.vgic.lock));
+ ASSERT(spin_is_locked(&p->lock));
if ( v == current && list_empty(&v->arch.vgic.lr_pending) )
{
@@ -439,6 +441,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
gic_hw_ops->read_lr(i, &lr_val);
irq = lr_val.virq;
p = irq_to_pending(v, irq);
+ spin_lock(&p->lock);
if ( lr_val.state & GICH_LR_ACTIVE )
{
set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
@@ -495,6 +498,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
}
}
}
+ spin_unlock(&p->lock);
}
void gic_clear_lrs(struct vcpu *v)
@@ -545,14 +549,30 @@ static void gic_restore_pending_irqs(struct vcpu *v)
/* No more free LRs: find a lower priority irq to evict */
list_for_each_entry_reverse( p_r, inflight_r, inflight )
{
+ if ( p_r->irq < p->irq )
+ {
+ spin_lock(&p_r->lock);
+ spin_lock(&p->lock);
+ }
+ else
+ {
+ spin_lock(&p->lock);
+ spin_lock(&p_r->lock);
+ }
if ( p_r->priority == p->priority )
+ {
+ spin_unlock(&p->lock);
+ spin_unlock(&p_r->lock);
goto out;
+ }
if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status) &&
!test_bit(GIC_IRQ_GUEST_ACTIVE, &p_r->status) )
goto found;
}
/* We didn't find a victim this time, and we won't next
* time, so quit */
+ spin_unlock(&p->lock);
+ spin_unlock(&p_r->lock);
goto out;
found:
@@ -562,12 +582,18 @@ found:
clear_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status);
gic_add_to_lr_pending(v, p_r);
inflight_r = &p_r->inflight;
+
+ spin_unlock(&p_r->lock);
}
+ else
+ spin_lock(&p->lock);
gic_set_lr(lr, p, GICH_LR_PENDING);
list_del_init(&p->lr_queue);
set_bit(lr, &this_cpu(lr_mask));
+ spin_unlock(&p->lock);
+
/* We can only evict nr_lrs entries */
lrs--;
if ( lrs == 0 )
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index f4ae454..44363bb 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -356,11 +356,16 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
irq = i + (32 * n);
v_target = vgic_get_target_vcpu(v, irq);
+
+ spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
p = irq_to_pending(v_target, irq);
+ spin_lock(&p->lock);
+
set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
- 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, p);
+ spin_unlock(&p->lock);
spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
if ( p->desc != NULL )
{
@@ -482,10 +487,12 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
return;
}
+ spin_lock(&n->lock);
set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
if ( !list_empty(&n->inflight) )
{
+ spin_unlock(&n->lock);
gic_raise_inflight_irq(v, n);
goto out;
}
@@ -501,10 +508,13 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
if ( iter->priority > priority )
{
list_add_tail(&n->inflight, &iter->inflight);
+ spin_unlock(&n->lock);
goto out;
}
}
list_add_tail(&n->inflight, &v->arch.vgic.inflight_irqs);
+ spin_unlock(&n->lock);
+
out:
spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
/* we have a new higher priority irq, inject it into the guest */
--
2.9.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [RFC PATCH 05/10] ARM: vGIC: move priority from irq_rank to struct pending_irq
2017-05-04 15:31 [RFC PATCH 00/10] ARM VGIC rework: remove rank, introduce per-IRQ lock Andre Przywara
` (3 preceding siblings ...)
2017-05-04 15:31 ` [RFC PATCH 04/10] ARM: vGIC: add struct pending_irq locking Andre Przywara
@ 2017-05-04 15:31 ` Andre Przywara
2017-05-04 16:39 ` Julien Grall
2017-05-04 15:31 ` [RFC PATCH 06/10] ARM: vGIC: move config " Andre Przywara
` (4 subsequent siblings)
9 siblings, 1 reply; 31+ messages in thread
From: Andre Przywara @ 2017-05-04 15:31 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini; +Cc: xen-devel
Currently we store the priority for newly triggered IRQs in the rank
structure. To get eventually rid of this structure, move this value
into the struct pending_irq. This one already contains a priority value,
but we have to keep the two apart, as the priority for guest visible
IRQs must not change while they are in a VCPU.
This patch introduces a framework to get some per-IRQ information for a
number of interrupts and collate them into one register. Similarily
there is the opposite function to spread values from one register into
multiple pending_irq's.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
xen/arch/arm/vgic-v2.c | 33 +++++++++--------------------
xen/arch/arm/vgic-v3.c | 33 ++++++++++-------------------
xen/arch/arm/vgic.c | 53 ++++++++++++++++++++++++++++++++++------------
xen/include/asm-arm/vgic.h | 17 ++++++---------
4 files changed, 67 insertions(+), 69 deletions(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index dc9f95b..5c59fb4 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;
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;
-
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;
-
- vgic_lock_rank(v, rank, flags);
- ipriorityr = rank->ipriorityr[REG_RANK_INDEX(8,
- gicd_reg - GICD_IPRIORITYR,
- DABT_WORD)];
- vgic_unlock_rank(v, rank, flags);
- *r = vgic_reg32_extract(ipriorityr, info);
-
+ irq = gicd_reg - GICD_IPRIORITYR;
+ *r = vgic_reg32_extract(gather_irq_info_priority(v, irq), info);
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;
perfc_incr(vgicd_writes);
@@ -499,17 +489,14 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
{
- uint32_t *ipriorityr;
+ uint32_t ipriorityr;
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)];
- vgic_reg32_update(ipriorityr, r, info);
- vgic_unlock_rank(v, rank, flags);
+ irq = gicd_reg - GICD_IPRIORITYR;
+
+ ipriorityr = gather_irq_info_priority(v, irq);
+ vgic_reg32_update(&ipriorityr, r, info);
+ scatter_irq_info_priority(v, irq, ipriorityr);
return 1;
}
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index d10757a..10db939 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -476,6 +476,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;
switch ( reg )
{
@@ -513,22 +514,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;
-
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;
-
- vgic_lock_rank(v, rank, flags);
- ipriorityr = rank->ipriorityr[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR,
- DABT_WORD)];
- vgic_unlock_rank(v, rank, flags);
-
- *r = vgic_reg32_extract(ipriorityr, info);
-
+ irq = reg - GICD_IPRIORITYR;
+ if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto read_as_zero;
+ *r = vgic_reg32_extract(gather_irq_info_priority(v, irq), info);
return 1;
- }
case VRANGE32(GICD_ICFGR, GICD_ICFGRN):
{
@@ -572,6 +562,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;
switch ( reg )
{
@@ -630,16 +621,14 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
{
- uint32_t *ipriorityr;
+ uint32_t ipriorityr;
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)];
- vgic_reg32_update(ipriorityr, r, info);
- vgic_unlock_rank(v, rank, flags);
+ irq = reg - GICD_IPRIORITYR;
+ if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto write_ignore;
+ ipriorityr = gather_irq_info_priority(v, irq);
+ vgic_reg32_update(&ipriorityr, r, info);
+ scatter_irq_info_priority(v, irq, ipriorityr);
return 1;
}
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 44363bb..68eef47 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -225,19 +225,49 @@ 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)
+static uint8_t extract_priority(struct pending_irq *p)
{
- struct vgic_irq_rank *rank = vgic_rank_irq(v, virq);
- unsigned long flags;
- int priority;
+ return p->new_priority;
+}
+
+static void set_priority(struct pending_irq *p, uint8_t prio)
+{
+ p->new_priority = prio;
+}
+
- vgic_lock_rank(v, rank, flags);
- priority = rank->priority[virq & INTERRUPT_RANK_MASK];
- vgic_unlock_rank(v, rank, flags);
+#define DEFINE_GATHER_IRQ_INFO(name, get_val, shift) \
+uint32_t gather_irq_info_##name(struct vcpu *v, unsigned int irq) \
+{ \
+ uint32_t ret = 0, i; \
+ for ( i = 0; i < (32 / shift); i++ ) \
+ { \
+ struct pending_irq *p = irq_to_pending(v, irq + i); \
+ spin_lock(&p->lock); \
+ ret |= get_val(p) << (shift * i); \
+ spin_unlock(&p->lock); \
+ } \
+ return ret; \
+}
- return priority;
+#define DEFINE_SCATTER_IRQ_INFO(name, set_val, shift) \
+void scatter_irq_info_##name(struct vcpu *v, unsigned int irq, \
+ unsigned int value) \
+{ \
+ unsigned int i; \
+ for ( i = 0; i < (32 / shift); i++ ) \
+ { \
+ struct pending_irq *p = irq_to_pending(v, irq + i); \
+ spin_lock(&p->lock); \
+ set_val(p, (value >> (shift * i)) & ((1 << shift) - 1)); \
+ spin_unlock(&p->lock); \
+ } \
}
+/* grep fodder: gather_irq_info_priority, scatter_irq_info_priority below */
+DEFINE_GATHER_IRQ_INFO(priority, extract_priority, 8)
+DEFINE_SCATTER_IRQ_INFO(priority, set_priority, 8)
+
bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
{
unsigned long flags;
@@ -471,13 +501,10 @@ 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 = irq_to_pending(v, virq);
unsigned long flags;
bool running;
- priority = vgic_get_virq_priority(v, virq);
-
spin_lock_irqsave(&v->arch.vgic.lock, flags);
/* vcpu offline */
@@ -497,7 +524,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
goto out;
}
- n->priority = priority;
+ n->priority = n->new_priority;
/* the irq is enabled */
if ( test_bit(GIC_IRQ_GUEST_ENABLED, &n->status) )
@@ -505,7 +532,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight )
{
- if ( iter->priority > priority )
+ if ( iter->priority > n->priority )
{
list_add_tail(&n->inflight, &iter->inflight);
spin_unlock(&n->lock);
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index e7322fc..38a5e76 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -71,7 +71,8 @@ struct pending_irq
unsigned int irq;
#define GIC_INVALID_LR (uint8_t)~0
uint8_t lr;
- uint8_t priority;
+ uint8_t priority; /* the priority of the currently inflight IRQ */
+ uint8_t new_priority; /* the priority of newly triggered IRQs */
/* inflight is used to append instances of pending_irq to
* vgic.inflight_irqs */
struct list_head inflight;
@@ -104,16 +105,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
@@ -179,6 +170,10 @@ static inline int REG_RANK_NR(int b, uint32_t n)
}
}
+uint32_t gather_irq_info_priority(struct vcpu *v, unsigned int irq);
+void scatter_irq_info_priority(struct vcpu *v, unsigned int irq,
+ unsigned int value);
+
#define VGIC_REG_MASK(size) ((~0UL) >> (BITS_PER_LONG - ((1 << (size)) * 8)))
/*
--
2.9.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [RFC PATCH 06/10] ARM: vGIC: move config from irq_rank to struct pending_irq
2017-05-04 15:31 [RFC PATCH 00/10] ARM VGIC rework: remove rank, introduce per-IRQ lock Andre Przywara
` (4 preceding siblings ...)
2017-05-04 15:31 ` [RFC PATCH 05/10] ARM: vGIC: move priority from irq_rank to struct pending_irq Andre Przywara
@ 2017-05-04 15:31 ` Andre Przywara
2017-05-04 16:55 ` Julien Grall
2017-05-04 15:31 ` [RFC PATCH 07/10] ARM: vGIC: move enable status " Andre Przywara
` (3 subsequent siblings)
9 siblings, 1 reply; 31+ messages in thread
From: Andre Przywara @ 2017-05-04 15:31 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini; +Cc: xen-devel
Currently we store the interrupt type configuration (level or edge
triggered) in the rank structure. Move this value into struct pending_irq.
We just need one bit (edge or level), so use one of the status bits.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
xen/arch/arm/vgic-v2.c | 29 ++++++++++-------------------
xen/arch/arm/vgic-v3.c | 31 ++++++++++++-------------------
xen/arch/arm/vgic.c | 39 ++++++++++++++++++++-------------------
xen/include/asm-arm/vgic.h | 7 ++++++-
4 files changed, 48 insertions(+), 58 deletions(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 5c59fb4..795173c 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -278,20 +278,10 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
goto read_reserved;
case VRANGE32(GICD_ICFGR, GICD_ICFGRN):
- {
- uint32_t icfgr;
-
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, flags);
- icfgr = rank->icfg[REG_RANK_INDEX(2, gicd_reg - GICD_ICFGR, DABT_WORD)];
- vgic_unlock_rank(v, rank, flags);
-
- *r = vgic_reg32_extract(icfgr, info);
-
+ irq = (gicd_reg - GICD_ICFGR) * 4;
+ *r = vgic_reg32_extract(gather_irq_info_config(v, irq), info);
return 1;
- }
case VRANGE32(0xD00, 0xDFC):
goto read_impl_defined;
@@ -534,15 +524,16 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
goto write_ignore_32;
case VRANGE32(GICD_ICFGR2, GICD_ICFGRN): /* SPIs */
+ {
+ uint32_t icfgr;
+
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, flags);
- vgic_reg32_update(&rank->icfg[REG_RANK_INDEX(2, gicd_reg - GICD_ICFGR,
- DABT_WORD)],
- r, info);
- vgic_unlock_rank(v, rank, flags);
+ irq = (gicd_reg - GICD_ICFGR) * 4;
+ icfgr = gather_irq_info_config(v, irq);
+ vgic_reg32_update(&icfgr, r, info);
+ scatter_irq_info_config(v, irq, icfgr);
return 1;
+ }
case VRANGE32(0xD00, 0xDFC):
goto write_impl_defined;
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 10db939..7989989 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -521,20 +521,11 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
return 1;
case VRANGE32(GICD_ICFGR, GICD_ICFGRN):
- {
- uint32_t icfgr;
-
if ( dabt.size != DABT_WORD ) goto bad_width;
- rank = vgic_rank_offset(v, 2, reg - GICD_ICFGR, DABT_WORD);
- if ( rank == NULL ) goto read_as_zero;
- vgic_lock_rank(v, rank, flags);
- icfgr = rank->icfg[REG_RANK_INDEX(2, reg - GICD_ICFGR, DABT_WORD)];
- vgic_unlock_rank(v, rank, flags);
-
- *r = vgic_reg32_extract(icfgr, info);
-
+ irq = (reg - GICD_IPRIORITYR) * 4;
+ if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto read_as_zero;
+ *r = vgic_reg32_extract(gather_irq_info_config(v, irq), info);
return 1;
- }
default:
printk(XENLOG_G_ERR
@@ -636,17 +627,19 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
goto write_ignore_32;
case VRANGE32(GICD_ICFGR + 4, GICD_ICFGRN): /* PPI + SPIs */
+ {
+ uint32_t icfgr;
+
/* ICFGR1 for PPI's, which is implementation defined
if ICFGR1 is programmable or not. We chose to program */
if ( dabt.size != DABT_WORD ) goto bad_width;
- rank = vgic_rank_offset(v, 2, reg - GICD_ICFGR, DABT_WORD);
- if ( rank == NULL ) goto write_ignore;
- vgic_lock_rank(v, rank, flags);
- vgic_reg32_update(&rank->icfg[REG_RANK_INDEX(2, reg - GICD_ICFGR,
- DABT_WORD)],
- r, info);
- vgic_unlock_rank(v, rank, flags);
+ irq = (reg - GICD_ICFGR) * 4;
+ if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto write_ignore;
+ icfgr = gather_irq_info_config(v, irq);
+ vgic_reg32_update(&icfgr, r, info);
+ scatter_irq_info_config(v, irq, icfgr);
return 1;
+ }
default:
printk(XENLOG_G_ERR
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 68eef47..02c1d12 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -235,6 +235,19 @@ static void set_priority(struct pending_irq *p, uint8_t prio)
p->new_priority = prio;
}
+unsigned int extract_config(struct pending_irq *p)
+{
+ return test_bit(GIC_IRQ_GUEST_EDGE, &p->status) ? 2 : 0;
+}
+
+static void set_config(struct pending_irq *p, unsigned int config)
+{
+ if ( config < 2 )
+ clear_bit(GIC_IRQ_GUEST_EDGE, &p->status);
+ else
+ set_bit(GIC_IRQ_GUEST_EDGE, &p->status);
+}
+
#define DEFINE_GATHER_IRQ_INFO(name, get_val, shift) \
uint32_t gather_irq_info_##name(struct vcpu *v, unsigned int irq) \
@@ -267,6 +280,8 @@ void scatter_irq_info_##name(struct vcpu *v, unsigned int irq, \
/* grep fodder: gather_irq_info_priority, scatter_irq_info_priority below */
DEFINE_GATHER_IRQ_INFO(priority, extract_priority, 8)
DEFINE_SCATTER_IRQ_INFO(priority, set_priority, 8)
+DEFINE_GATHER_IRQ_INFO(config, extract_config, 2)
+DEFINE_SCATTER_IRQ_INFO(config, set_config, 2)
bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
{
@@ -357,27 +372,11 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
}
}
-#define VGIC_ICFG_MASK(intr) (1 << ((2 * ((intr) % 16)) + 1))
-
-/* The function should be called with the rank lock taken */
-static inline unsigned int vgic_get_virq_type(struct vcpu *v, int n, int index)
-{
- struct vgic_irq_rank *r = vgic_get_rank(v, n);
- uint32_t tr = r->icfg[index >> 4];
-
- ASSERT(spin_is_locked(&r->lock));
-
- if ( tr & VGIC_ICFG_MASK(index) )
- return IRQ_TYPE_EDGE_RISING;
- else
- return IRQ_TYPE_LEVEL_HIGH;
-}
-
void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
{
const unsigned long mask = r;
struct pending_irq *p;
- unsigned int irq;
+ unsigned int irq, int_type;
unsigned long flags;
int i = 0;
struct vcpu *v_target;
@@ -392,6 +391,8 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
spin_lock(&p->lock);
set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
+ int_type = test_bit(GIC_IRQ_GUEST_EDGE, &p->status) ?
+ IRQ_TYPE_EDGE_RISING : IRQ_TYPE_LEVEL_HIGH;
if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
gic_raise_guest_irq(v_target, p);
@@ -399,15 +400,15 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
if ( p->desc != NULL )
{
- irq_set_affinity(p->desc, cpumask_of(v_target->processor));
spin_lock_irqsave(&p->desc->lock, flags);
+ irq_set_affinity(p->desc, cpumask_of(v_target->processor));
/*
* The irq cannot be a PPI, we only support delivery of SPIs
* to guests.
*/
ASSERT(irq >= 32);
if ( irq_type_set_by_domain(d) )
- gic_set_irq_type(p->desc, vgic_get_virq_type(v, n, i));
+ gic_set_irq_type(p->desc, int_type);
p->desc->handler->enable(p->desc);
spin_unlock_irqrestore(&p->desc->lock, flags);
}
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 38a5e76..931a672 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -60,12 +60,15 @@ struct pending_irq
* vcpu while it is still inflight and on an GICH_LR register on the
* old vcpu.
*
+ * GIC_IRQ_GUEST_EDGE: the IRQ is an edge triggered interrupt.
+ *
*/
#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_EDGE 5
unsigned long status;
struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */
unsigned int irq;
@@ -102,7 +105,6 @@ struct vgic_irq_rank {
uint8_t index;
uint32_t ienable;
- uint32_t icfg[2];
/*
* It's more convenient to store a target VCPU per vIRQ
@@ -173,6 +175,9 @@ static inline int REG_RANK_NR(int b, uint32_t n)
uint32_t gather_irq_info_priority(struct vcpu *v, unsigned int irq);
void scatter_irq_info_priority(struct vcpu *v, unsigned int irq,
unsigned int value);
+uint32_t gather_irq_info_config(struct vcpu *v, unsigned int irq);
+void scatter_irq_info_config(struct vcpu *v, unsigned int irq,
+ unsigned int value);
#define VGIC_REG_MASK(size) ((~0UL) >> (BITS_PER_LONG - ((1 << (size)) * 8)))
--
2.9.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [RFC PATCH 07/10] ARM: vGIC: move enable status from irq_rank to struct pending_irq
2017-05-04 15:31 [RFC PATCH 00/10] ARM VGIC rework: remove rank, introduce per-IRQ lock Andre Przywara
` (5 preceding siblings ...)
2017-05-04 15:31 ` [RFC PATCH 06/10] ARM: vGIC: move config " Andre Przywara
@ 2017-05-04 15:31 ` Andre Przywara
2017-05-04 15:31 ` [RFC PATCH 08/10] ARM: vGIC: move target vcpu " Andre Przywara
` (2 subsequent siblings)
9 siblings, 0 replies; 31+ messages in thread
From: Andre Przywara @ 2017-05-04 15:31 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini; +Cc: xen-devel
Currently we store the enable bit of an interrupt in the rank structure.
Remove it from there and let the MMIO emulation use the already existing
GIC_IRQ_GUEST_ENABLED in the status bits of struct pending_irq.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
xen/arch/arm/vgic-v2.c | 38 ++++++++++++--------------------
xen/arch/arm/vgic-v3.c | 55 ++++++++++++++++++++++------------------------
xen/arch/arm/vgic.c | 27 +++++++++++++----------
xen/include/asm-arm/vgic.h | 7 +++---
4 files changed, 58 insertions(+), 69 deletions(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 795173c..22c679c 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -224,20 +224,15 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
case VRANGE32(GICD_ISENABLER, GICD_ISENABLERN):
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, flags);
- *r = vgic_reg32_extract(rank->ienable, info);
- vgic_unlock_rank(v, rank, flags);
+ irq = (gicd_reg - GICD_ISENABLER) * 8;
+ if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto read_as_zero;
+ *r = vgic_reg32_extract(gather_irq_info_enabled(v, irq), info);
return 1;
case VRANGE32(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, flags);
- *r = vgic_reg32_extract(rank->ienable, info);
- vgic_unlock_rank(v, rank, flags);
+ irq = (gicd_reg - GICD_ISENABLER) * 8;
+ *r = vgic_reg32_extract(gather_irq_info_enabled(v, irq), info);
return 1;
/* Read the pending status of an IRQ via GICD is not supported */
@@ -430,24 +425,19 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
case VRANGE32(GICD_ISENABLER, GICD_ISENABLERN):
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, flags);
- tr = rank->ienable;
- vgic_reg32_setbits(&rank->ienable, r, info);
- vgic_enable_irqs(v, (rank->ienable) & (~tr), rank->index);
- vgic_unlock_rank(v, rank, flags);
+ irq = (gicd_reg - GICD_ISENABLER) * 8;
+ if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto write_ignore;
+ tr = gather_irq_info_enabled(v, irq);
+ vgic_reg32_setbits(&tr, r, info);
+ vgic_enable_irqs(v, irq, tr);
return 1;
case VRANGE32(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, flags);
- tr = rank->ienable;
- vgic_reg32_clearbits(&rank->ienable, r, info);
- vgic_disable_irqs(v, (~rank->ienable) & tr, rank->index);
- vgic_unlock_rank(v, rank, flags);
+ irq = (gicd_reg - GICD_ISENABLER) * 8;
+ tr = gather_irq_info_enabled(v, irq);
+ vgic_reg32_clearbits(&tr, r, info);
+ vgic_disable_irqs(v, irq, tr);
return 1;
case VRANGE32(GICD_ISPENDR, GICD_ISPENDRN):
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 7989989..01764c9 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -474,8 +474,6 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
register_t *r)
{
struct hsr_dabt dabt = info->dabt;
- struct vgic_irq_rank *rank;
- unsigned long flags;
unsigned int irq;
switch ( reg )
@@ -487,20 +485,16 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
case VRANGE32(GICD_ISENABLER, GICD_ISENABLERN):
if ( dabt.size != DABT_WORD ) goto bad_width;
- rank = vgic_rank_offset(v, 1, reg - GICD_ISENABLER, DABT_WORD);
- if ( rank == NULL ) goto read_as_zero;
- vgic_lock_rank(v, rank, flags);
- *r = vgic_reg32_extract(rank->ienable, info);
- vgic_unlock_rank(v, rank, flags);
+ irq = (reg - GICD_ISENABLER) * 8;
+ if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto read_as_zero;
+ *r = vgic_reg32_extract(gather_irq_info_enabled(v, irq), info);
return 1;
case VRANGE32(GICD_ICENABLER, GICD_ICENABLERN):
if ( dabt.size != DABT_WORD ) goto bad_width;
- rank = vgic_rank_offset(v, 1, reg - GICD_ICENABLER, DABT_WORD);
- if ( rank == NULL ) goto read_as_zero;
- vgic_lock_rank(v, rank, flags);
- *r = vgic_reg32_extract(rank->ienable, info);
- vgic_unlock_rank(v, rank, flags);
+ irq = (reg - GICD_ISENABLER) * 8;
+ if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto read_as_zero;
+ *r = vgic_reg32_extract(gather_irq_info_enabled(v, irq), info);
return 1;
/* Read the pending status of an IRQ via GICD/GICR is not supported */
@@ -550,9 +544,6 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
register_t r)
{
struct hsr_dabt dabt = info->dabt;
- struct vgic_irq_rank *rank;
- uint32_t tr;
- unsigned long flags;
unsigned int irq;
switch ( reg )
@@ -562,26 +553,32 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
goto write_ignore_32;
case VRANGE32(GICD_ISENABLER, GICD_ISENABLERN):
+ {
+ uint32_t new_reg, tr;
+
if ( dabt.size != DABT_WORD ) goto bad_width;
- rank = vgic_rank_offset(v, 1, reg - GICD_ISENABLER, DABT_WORD);
- if ( rank == NULL ) goto write_ignore;
- vgic_lock_rank(v, rank, flags);
- tr = rank->ienable;
- vgic_reg32_setbits(&rank->ienable, r, info);
- vgic_enable_irqs(v, (rank->ienable) & (~tr), rank->index);
- vgic_unlock_rank(v, rank, flags);
+ irq = (reg - GICD_ISENABLER) * 8;
+ if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto write_ignore;
+ new_reg = gather_irq_info_enabled(v, irq);
+ tr = new_reg;
+ vgic_reg32_setbits(&new_reg, r, info);
+ vgic_enable_irqs(v, irq, new_reg & (~tr));
return 1;
+ }
case VRANGE32(GICD_ICENABLER, GICD_ICENABLERN):
+ {
+ uint32_t new_reg, tr;
+
if ( dabt.size != DABT_WORD ) goto bad_width;
- rank = vgic_rank_offset(v, 1, reg - GICD_ICENABLER, DABT_WORD);
- if ( rank == NULL ) goto write_ignore;
- vgic_lock_rank(v, rank, flags);
- tr = rank->ienable;
- vgic_reg32_clearbits(&rank->ienable, r, info);
- vgic_disable_irqs(v, (~rank->ienable) & tr, rank->index);
- vgic_unlock_rank(v, rank, flags);
+ irq = (reg - GICD_ISENABLER) * 8;
+ if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto write_ignore;
+ new_reg = gather_irq_info_enabled(v, irq);
+ tr = new_reg;
+ vgic_reg32_clearbits(&new_reg, r, info);
+ vgic_disable_irqs(v, irq, (~new_reg) & tr);
return 1;
+ }
case VRANGE32(GICD_ISPENDR, GICD_ISPENDRN):
if ( dabt.size != DABT_WORD ) goto bad_width;
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 02c1d12..a23079a 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -235,6 +235,11 @@ static void set_priority(struct pending_irq *p, uint8_t prio)
p->new_priority = prio;
}
+unsigned int extract_enabled(struct pending_irq *p)
+{
+ return test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ? 1 : 0;
+}
+
unsigned int extract_config(struct pending_irq *p)
{
return test_bit(GIC_IRQ_GUEST_EDGE, &p->status) ? 2 : 0;
@@ -280,6 +285,7 @@ void scatter_irq_info_##name(struct vcpu *v, unsigned int irq, \
/* grep fodder: gather_irq_info_priority, scatter_irq_info_priority below */
DEFINE_GATHER_IRQ_INFO(priority, extract_priority, 8)
DEFINE_SCATTER_IRQ_INFO(priority, set_priority, 8)
+DEFINE_GATHER_IRQ_INFO(enabled, extract_enabled, 1)
DEFINE_GATHER_IRQ_INFO(config, extract_config, 2)
DEFINE_SCATTER_IRQ_INFO(config, set_config, 2)
@@ -347,21 +353,19 @@ void arch_move_irqs(struct vcpu *v)
}
}
-void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
+void vgic_disable_irqs(struct vcpu *v, unsigned int irq, uint32_t r)
{
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);
- v_target = vgic_get_target_vcpu(v, irq);
- p = irq_to_pending(v_target, irq);
+ v_target = vgic_get_target_vcpu(v, irq + i);
+ p = irq_to_pending(v_target, irq + i);
clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
- gic_remove_from_queues(v_target, irq);
+ gic_remove_from_queues(v_target, irq + i);
if ( p->desc != NULL )
{
spin_lock_irqsave(&p->desc->lock, flags);
@@ -372,22 +376,21 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
}
}
-void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
+void vgic_enable_irqs(struct vcpu *v, unsigned int irq, uint32_t r)
{
const unsigned long mask = r;
struct pending_irq *p;
- unsigned int irq, int_type;
+ unsigned int int_type;
unsigned long flags;
int i = 0;
struct vcpu *v_target;
struct domain *d = v->domain;
while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
- irq = i + (32 * n);
- v_target = vgic_get_target_vcpu(v, irq);
+ v_target = vgic_get_target_vcpu(v, irq + i);
spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
- p = irq_to_pending(v_target, irq);
+ p = irq_to_pending(v_target, irq + i);
spin_lock(&p->lock);
set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
@@ -406,7 +409,7 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
* The irq cannot be a PPI, we only support delivery of SPIs
* to guests.
*/
- ASSERT(irq >= 32);
+ ASSERT(irq + i >= 32);
if ( irq_type_set_by_domain(d) )
gic_set_irq_type(p->desc, int_type);
p->desc->handler->enable(p->desc);
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 931a672..fe09fb8 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -104,8 +104,6 @@ struct vgic_irq_rank {
uint8_t index;
- uint32_t ienable;
-
/*
* It's more convenient to store a target VCPU per vIRQ
* than the register ITARGETSR/IROUTER itself.
@@ -178,6 +176,7 @@ void scatter_irq_info_priority(struct vcpu *v, unsigned int irq,
uint32_t gather_irq_info_config(struct vcpu *v, unsigned int irq);
void scatter_irq_info_config(struct vcpu *v, unsigned int irq,
unsigned int value);
+uint32_t gather_irq_info_enabled(struct vcpu *v, unsigned int irq);
#define VGIC_REG_MASK(size) ((~0UL) >> (BITS_PER_LONG - ((1 << (size)) * 8)))
@@ -311,8 +310,8 @@ 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);
extern struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq);
extern bool vgic_emulate(struct cpu_user_regs *regs, union hsr hsr);
-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 vgic_disable_irqs(struct vcpu *v, unsigned int irq, uint32_t r);
+extern void vgic_enable_irqs(struct vcpu *v, unsigned int irq, uint32_t r);
extern void register_vgic_ops(struct domain *d, const struct vgic_ops *ops);
int vgic_v2_init(struct domain *d, int *mmio_count);
int vgic_v3_init(struct domain *d, int *mmio_count);
--
2.9.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [RFC PATCH 08/10] ARM: vGIC: move target vcpu from irq_rank to struct pending_irq
2017-05-04 15:31 [RFC PATCH 00/10] ARM VGIC rework: remove rank, introduce per-IRQ lock Andre Przywara
` (6 preceding siblings ...)
2017-05-04 15:31 ` [RFC PATCH 07/10] ARM: vGIC: move enable status " Andre Przywara
@ 2017-05-04 15:31 ` Andre Przywara
2017-05-04 17:20 ` Julien Grall
2017-05-04 15:31 ` [RFC PATCH 09/10] ARM: vGIC: introduce vgic_get/put_pending_irq Andre Przywara
2017-05-04 15:31 ` [RFC PATCH 10/10] ARM: vGIC: remove struct irq_rank and support functions Andre Przywara
9 siblings, 1 reply; 31+ messages in thread
From: Andre Przywara @ 2017-05-04 15:31 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini; +Cc: xen-devel
So far we kept the target VCPU for SPIs in the rank structure.
Move that information over into pending_irq.
This changes vgic_get_target_vcpu(), which now takes only a domain
and a struct pending_irq to get the target vCPU, in a way that does not
necessarily require the pending_irq lock to be held.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
xen/arch/arm/gic.c | 3 +-
xen/arch/arm/vgic-v2.c | 57 ++++++++++++++---------------------
xen/arch/arm/vgic-v3.c | 71 ++++++++++++++++++++++----------------------
xen/arch/arm/vgic.c | 74 ++++++++++++++++++++--------------------------
xen/include/asm-arm/vgic.h | 15 ++++------
5 files changed, 97 insertions(+), 123 deletions(-)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index e175e9b..737da6b 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -492,7 +492,8 @@ static void gic_update_one_lr(struct vcpu *v, int i)
smp_wmb();
if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
{
- struct vcpu *v_target = vgic_get_target_vcpu(v, irq);
+ struct vcpu *v_target = vgic_get_target_vcpu(v->domain, p);
+
irq_set_affinity(p->desc, cpumask_of(v_target->processor));
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 22c679c..bf755ae 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -66,19 +66,21 @@ 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;
- 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);
+
+ spin_lock(&p->lock);
+ reg |= (1 << p->vcpu_id) << (i * NR_BITS_PER_TARGET);
+ spin_unlock(&p->lock);
+ }
return reg;
}
@@ -89,32 +91,28 @@ 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;
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 +149,17 @@ 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]);
+ spin_lock(&p->lock);
+
+ 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);
+ if ( vgic_migrate_irq(p, d->vcpu[old_target], d->vcpu[new_target]) )
+ p->vcpu_id = new_target;
}
+ spin_unlock(&p->lock);
}
}
@@ -168,9 +167,7 @@ static int vgic_v2_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;
int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
- unsigned long flags;
unsigned int irq;
perfc_incr(vgicd_reads);
@@ -259,11 +256,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);
*r = vgic_reg32_extract(itargetsr, info);
return 1;
@@ -385,10 +378,8 @@ static int vgic_v2_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;
int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
uint32_t tr;
- unsigned long flags;
unsigned int irq;
perfc_incr(vgicd_writes);
@@ -492,14 +483,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);
vgic_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);
- vgic_unlock_rank(v, rank, flags);
return 1;
}
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 01764c9..15a512a 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -97,18 +97,20 @@ 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;
+ 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);
+ spin_lock(&p->lock);
+ aff = vcpuid_to_vaffinity(p->vcpu_id);
+ spin_unlock(&p->lock);
- return vcpuid_to_vaffinity(read_atomic(&rank->vcpu[offset]));
+ return aff;
}
/*
@@ -117,11 +119,13 @@ 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 pending_irq *p;
unsigned int virq;
+ bool reinject;
/* There is 1 vIRQ per IROUTER */
virq = offset / NR_BYTES_PER_IROUTER;
@@ -132,11 +136,11 @@ 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);
+ spin_lock(&p->lock);
new_vcpu = vgic_v3_irouter_to_vcpu(d, irouter);
- old_vcpu = d->vcpu[read_atomic(&rank->vcpu[offset])];
+ old_vcpu = d->vcpu[p->vcpu_id];
/*
* From the spec (see 8.9.13 in IHI 0069A), any write with an
@@ -146,16 +150,21 @@ 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 ( !new_vcpu || new_vcpu == old_vcpu )
{
- if ( vgic_migrate_irq(old_vcpu, new_vcpu, virq) )
- write_atomic(&rank->vcpu[offset], new_vcpu->vcpu_id);
+ spin_unlock(&p->lock);
+ return;
}
+
+ reinject = vgic_migrate_irq(p, old_vcpu, new_vcpu);
+
+ spin_unlock(&p->lock);
+
+ if ( reinject )
+ vgic_vcpu_inject_irq(new_vcpu, virq);
}
static inline bool vgic_reg64_check_access(struct hsr_dabt dabt)
@@ -869,8 +878,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);
@@ -996,15 +1003,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 = vgic_reg64_extract(irouter, info);
return 1;
@@ -1070,8 +1074,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);
@@ -1185,16 +1187,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);
vgic_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);
return 1;
}
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index a23079a..530ac55 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -60,32 +60,22 @@ struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq)
return vgic_get_rank(v, rank);
}
-static void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq)
+static void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq,
+ int vcpu_id)
{
INIT_LIST_HEAD(&p->inflight);
INIT_LIST_HEAD(&p->lr_queue);
spin_lock_init(&p->lock);
p->irq = virq;
+ 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)
@@ -136,8 +126,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++ )
@@ -202,8 +193,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);
@@ -218,11 +210,11 @@ int vcpu_vgic_free(struct vcpu *v)
return 0;
}
-struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int virq)
+struct vcpu *vgic_get_target_vcpu(struct domain *d, struct pending_irq *p)
{
- struct vgic_irq_rank *rank = vgic_rank_irq(v, virq);
- int target = read_atomic(&rank->vcpu[virq & INTERRUPT_RANK_MASK]);
- return v->domain->vcpu[target];
+ uint16_t vcpu_id = read_atomic(&p->vcpu_id);
+
+ return d->vcpu[vcpu_id];
}
static uint8_t extract_priority(struct pending_irq *p)
@@ -289,31 +281,29 @@ DEFINE_GATHER_IRQ_INFO(enabled, extract_enabled, 1)
DEFINE_GATHER_IRQ_INFO(config, extract_config, 2)
DEFINE_SCATTER_IRQ_INFO(config, set_config, 2)
-bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
+bool vgic_migrate_irq(struct pending_irq *p, struct vcpu *old, struct vcpu *new)
{
- unsigned long flags;
- struct pending_irq *p = irq_to_pending(old, irq);
-
- /* nothing to do for virtual interrupts */
- if ( p->desc == NULL )
- return true;
+ ASSERT(spin_is_locked(&p->lock));
/* 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);
+ gprintk(XENLOG_WARNING, "irq %u migration failed: requested while in progress\n", p->irq);
return false;
}
- perfc_incr(vgic_irq_migrates);
+ p->vcpu_id = new->vcpu_id;
+
+ /* nothing to do for virtual interrupts */
+ if ( p->desc == NULL )
+ return false;
- spin_lock_irqsave(&old->arch.vgic.lock, flags);
+ perfc_incr(vgic_irq_migrates);
if ( list_empty(&p->inflight) )
{
irq_set_affinity(p->desc, cpumask_of(new->processor));
- spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
- return true;
+ return false;
}
/* If the IRQ is still lr_pending, re-inject it to the new vcpu */
if ( !list_empty(&p->lr_queue) )
@@ -322,8 +312,6 @@ bool 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 true;
}
/* if the IRQ is in a GICH_LR register, set GIC_IRQ_GUEST_MIGRATING
@@ -331,8 +319,7 @@ bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
if ( !list_empty(&p->inflight) )
set_bit(GIC_IRQ_GUEST_MIGRATING, &p->status);
- spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
- return true;
+ return false;
}
void arch_move_irqs(struct vcpu *v)
@@ -345,11 +332,13 @@ void arch_move_irqs(struct vcpu *v)
for ( i = 32; i < vgic_num_irqs(d); i++ )
{
- v_target = vgic_get_target_vcpu(v, i);
- p = irq_to_pending(v_target, i);
+ p = irq_to_pending(d->vcpu[0], i);
+ spin_lock(&p->lock);
+ v_target = vgic_get_target_vcpu(d, p);
if ( v_target == v && !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
irq_set_affinity(p->desc, cpu_mask);
+ spin_unlock(&p->lock);
}
}
@@ -362,8 +351,8 @@ void vgic_disable_irqs(struct vcpu *v, unsigned int irq, uint32_t r)
struct vcpu *v_target;
while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
- v_target = vgic_get_target_vcpu(v, irq + i);
- p = irq_to_pending(v_target, irq + i);
+ p = irq_to_pending(v, irq + i);
+ v_target = vgic_get_target_vcpu(v->domain, p);
clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
gic_remove_from_queues(v_target, irq + i);
if ( p->desc != NULL )
@@ -387,10 +376,10 @@ void vgic_enable_irqs(struct vcpu *v, unsigned int irq, uint32_t r)
struct domain *d = v->domain;
while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
- v_target = vgic_get_target_vcpu(v, irq + i);
+ p = irq_to_pending(v, irq + i);
+ v_target = vgic_get_target_vcpu(d, p);
spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
- p = irq_to_pending(v_target, irq + i);
spin_lock(&p->lock);
set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
@@ -561,11 +550,12 @@ out:
void vgic_vcpu_inject_spi(struct domain *d, unsigned int virq)
{
struct vcpu *v;
+ struct pending_irq *p = irq_to_pending(d->vcpu[0], virq);
/* the IRQ needs to be an SPI */
ASSERT(virq >= 32 && virq <= vgic_num_irqs(d));
- v = vgic_get_target_vcpu(d->vcpu[0], virq);
+ v = vgic_get_target_vcpu(d, p);
vgic_vcpu_inject_irq(v, virq);
}
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index fe09fb8..186e6df 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -76,6 +76,7 @@ struct pending_irq
uint8_t lr;
uint8_t priority; /* the priority of the currently inflight IRQ */
uint8_t new_priority; /* the priority of newly triggered IRQs */
+ uint8_t vcpu_id;
/* inflight is used to append instances of pending_irq to
* vgic.inflight_irqs */
struct list_head inflight;
@@ -103,14 +104,6 @@ struct vgic_irq_rank {
spinlock_t lock; /* Covers access to all other members of this struct */
uint8_t index;
-
- /*
- * 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 {
@@ -301,7 +294,8 @@ enum gic_sgi_mode;
extern int domain_vgic_init(struct domain *d, unsigned int nr_spis);
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 virq);
+extern struct vcpu *vgic_get_target_vcpu(struct domain *d,
+ 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);
@@ -321,7 +315,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,
+ struct vcpu *old, struct vcpu *new);
/* Reserve a specific guest vIRQ */
extern bool vgic_reserve_virq(struct domain *d, unsigned int virq);
--
2.9.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [RFC PATCH 09/10] ARM: vGIC: introduce vgic_get/put_pending_irq
2017-05-04 15:31 [RFC PATCH 00/10] ARM VGIC rework: remove rank, introduce per-IRQ lock Andre Przywara
` (7 preceding siblings ...)
2017-05-04 15:31 ` [RFC PATCH 08/10] ARM: vGIC: move target vcpu " Andre Przywara
@ 2017-05-04 15:31 ` Andre Przywara
2017-05-04 17:36 ` Julien Grall
2017-05-04 15:31 ` [RFC PATCH 10/10] ARM: vGIC: remove struct irq_rank and support functions Andre Przywara
9 siblings, 1 reply; 31+ messages in thread
From: Andre Przywara @ 2017-05-04 15:31 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini; +Cc: xen-devel
So far there is always a statically allocated struct pending_irq for
each interrupt that we deal with.
To prepare for dynamically allocated LPIs, introduce a put/get wrapper
to get hold of a pending_irq pointer.
So far get() just returns the same pointer and put() is empty, but this
change allows to introduce ref-counting very easily, to prevent
use-after-free usage of struct pending_irq's once LPIs get unmapped from
a domain.
For convenience reasons we introduce a put_unlock() version, which also
drops the pending_irq lock before calling the actual put() function.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
xen/arch/arm/gic.c | 24 +++++++++++++++------
xen/arch/arm/vgic-v2.c | 4 ++--
xen/arch/arm/vgic-v3.c | 4 ++--
xen/arch/arm/vgic.c | 52 +++++++++++++++++++++++++++++++--------------
xen/include/asm-arm/event.h | 20 +++++++++++------
xen/include/asm-arm/vgic.h | 7 +++++-
6 files changed, 77 insertions(+), 34 deletions(-)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 737da6b..7147b6c 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -136,9 +136,7 @@ void gic_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
struct irq_desc *desc, unsigned int priority)
{
- /* Use vcpu0 to retrieve the pending_irq struct. Given that we only
- * route SPIs to guests, it doesn't make any difference. */
- struct pending_irq *p = irq_to_pending(d->vcpu[0], virq);
+ struct pending_irq *p = vgic_get_pending_irq(d, NULL, virq);
ASSERT(spin_is_locked(&desc->lock));
/* Caller has already checked that the IRQ is an SPI */
@@ -148,7 +146,10 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
if ( p->desc ||
/* The VIRQ should not be already enabled by the guest */
test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
+ {
+ vgic_put_pending_irq(d, p);
return -EBUSY;
+ }
desc->handler = gic_hw_ops->gic_guest_irq_type;
set_bit(_IRQ_GUEST, &desc->status);
@@ -159,6 +160,7 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
p->desc = desc;
+ vgic_put_pending_irq(d, p);
return 0;
}
@@ -166,7 +168,7 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
struct irq_desc *desc)
{
- struct pending_irq *p = irq_to_pending(d->vcpu[0], virq);
+ struct pending_irq *p = vgic_get_pending_irq(d, NULL, virq);
ASSERT(spin_is_locked(&desc->lock));
ASSERT(test_bit(_IRQ_GUEST, &desc->status));
@@ -189,7 +191,10 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
*/
if ( test_bit(_IRQ_INPROGRESS, &desc->status) ||
!test_bit(_IRQ_DISABLED, &desc->status) )
+ {
+ vgic_put_pending_irq(d, p);
return -EBUSY;
+ }
}
clear_bit(_IRQ_GUEST, &desc->status);
@@ -197,6 +202,8 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
p->desc = NULL;
+ vgic_put_pending_irq(d, p);
+
return 0;
}
@@ -383,13 +390,14 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, struct pending_irq *n)
void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq)
{
- struct pending_irq *p = irq_to_pending(v, virtual_irq);
+ struct pending_irq *p = vgic_get_pending_irq(v->domain, v, virtual_irq);
unsigned long flags;
spin_lock_irqsave(&v->arch.vgic.lock, flags);
if ( !list_empty(&p->lr_queue) )
list_del_init(&p->lr_queue);
spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+ vgic_put_pending_irq(v->domain, p);
}
void gic_raise_inflight_irq(struct vcpu *v, struct pending_irq *n)
@@ -406,6 +414,7 @@ void gic_raise_inflight_irq(struct vcpu *v, struct pending_irq *n)
gdprintk(XENLOG_DEBUG, "trying to inject irq=%u into d%dv%d, when it is still lr_pending\n",
n->irq, v->domain->domain_id, v->vcpu_id);
#endif
+ vgic_put_pending_irq(v->domain, n);
}
void gic_raise_guest_irq(struct vcpu *v, struct pending_irq *p)
@@ -440,8 +449,9 @@ static void gic_update_one_lr(struct vcpu *v, int i)
gic_hw_ops->read_lr(i, &lr_val);
irq = lr_val.virq;
- p = irq_to_pending(v, irq);
+ p = vgic_get_pending_irq(v->domain, v, irq);
spin_lock(&p->lock);
+
if ( lr_val.state & GICH_LR_ACTIVE )
{
set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
@@ -499,7 +509,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
}
}
}
- spin_unlock(&p->lock);
+ vgic_put_pending_irq_unlock(v->domain, p);
}
void gic_clear_lrs(struct vcpu *v)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index bf755ae..36ed04f 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -75,11 +75,11 @@ static uint32_t vgic_fetch_itargetsr(struct vcpu *v, unsigned int offset)
for ( i = 0; i < NR_TARGETS_PER_ITARGETSR; i++, offset++ )
{
- struct pending_irq *p = irq_to_pending(v, offset);
+ struct pending_irq *p = vgic_get_pending_irq(v->domain, v, offset);
spin_lock(&p->lock);
reg |= (1 << p->vcpu_id) << (i * NR_BITS_PER_TARGET);
- spin_unlock(&p->lock);
+ vgic_put_pending_irq_unlock(v->domain, p);
}
return reg;
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 15a512a..fff518e 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -105,10 +105,10 @@ static uint64_t vgic_fetch_irouter(struct vcpu *v, unsigned int offset)
/* There is exactly 1 vIRQ per IROUTER */
offset /= NR_BYTES_PER_IROUTER;
- p = irq_to_pending(v, offset);
+ p = vgic_get_pending_irq(v->domain, v, offset);
spin_lock(&p->lock);
aff = vcpuid_to_vaffinity(p->vcpu_id);
- spin_unlock(&p->lock);
+ vgic_put_pending_irq_unlock(v->domain, p);
return aff;
}
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 530ac55..c7d645e 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -245,17 +245,16 @@ static void set_config(struct pending_irq *p, unsigned int config)
set_bit(GIC_IRQ_GUEST_EDGE, &p->status);
}
-
#define DEFINE_GATHER_IRQ_INFO(name, get_val, shift) \
uint32_t gather_irq_info_##name(struct vcpu *v, unsigned int irq) \
{ \
uint32_t ret = 0, i; \
for ( i = 0; i < (32 / shift); i++ ) \
{ \
- struct pending_irq *p = irq_to_pending(v, irq + i); \
+ struct pending_irq *p = vgic_get_pending_irq(v->domain, v, irq + i); \
spin_lock(&p->lock); \
ret |= get_val(p) << (shift * i); \
- spin_unlock(&p->lock); \
+ vgic_put_pending_irq_unlock(v->domain, p); \
} \
return ret; \
}
@@ -267,10 +266,10 @@ void scatter_irq_info_##name(struct vcpu *v, unsigned int irq, \
unsigned int i; \
for ( i = 0; i < (32 / shift); i++ ) \
{ \
- struct pending_irq *p = irq_to_pending(v, irq + i); \
+ struct pending_irq *p = vgic_get_pending_irq(v->domain, v, irq + i); \
spin_lock(&p->lock); \
set_val(p, (value >> (shift * i)) & ((1 << shift) - 1)); \
- spin_unlock(&p->lock); \
+ vgic_put_pending_irq_unlock(v->domain, p); \
} \
}
@@ -332,13 +331,13 @@ void arch_move_irqs(struct vcpu *v)
for ( i = 32; i < vgic_num_irqs(d); i++ )
{
- p = irq_to_pending(d->vcpu[0], i);
+ p = vgic_get_pending_irq(d, NULL, i);
spin_lock(&p->lock);
v_target = vgic_get_target_vcpu(d, p);
if ( v_target == v && !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
irq_set_affinity(p->desc, cpu_mask);
- spin_unlock(&p->lock);
+ vgic_put_pending_irq_unlock(d, p);
}
}
@@ -351,7 +350,7 @@ void vgic_disable_irqs(struct vcpu *v, unsigned int irq, uint32_t r)
struct vcpu *v_target;
while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
- p = irq_to_pending(v, irq + i);
+ p = vgic_get_pending_irq(v->domain, v, irq + i);
v_target = vgic_get_target_vcpu(v->domain, p);
clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
gic_remove_from_queues(v_target, irq + i);
@@ -361,6 +360,7 @@ void vgic_disable_irqs(struct vcpu *v, unsigned int irq, uint32_t r)
p->desc->handler->disable(p->desc);
spin_unlock_irqrestore(&p->desc->lock, flags);
}
+ vgic_put_pending_irq(v->domain, p);
i++;
}
}
@@ -376,7 +376,7 @@ void vgic_enable_irqs(struct vcpu *v, unsigned int irq, uint32_t r)
struct domain *d = v->domain;
while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
- p = irq_to_pending(v, irq + i);
+ p = vgic_get_pending_irq(d, v, irq + i);
v_target = vgic_get_target_vcpu(d, p);
spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
@@ -404,6 +404,7 @@ void vgic_enable_irqs(struct vcpu *v, unsigned int irq, uint32_t r)
p->desc->handler->enable(p->desc);
spin_unlock_irqrestore(&p->desc->lock, flags);
}
+ vgic_put_pending_irq(v->domain, p);
i++;
}
}
@@ -461,23 +462,39 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode,
return true;
}
-struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq)
+struct pending_irq *spi_to_pending(struct domain *d, unsigned int irq)
+{
+ ASSERT(irq >= NR_LOCAL_IRQS);
+
+ return &d->arch.vgic.pending_irqs[irq - 32];
+}
+
+struct pending_irq *vgic_get_pending_irq(struct domain *d, struct vcpu *v,
+ unsigned int irq)
{
struct pending_irq *n;
+
/* Pending irqs allocation strategy: the first vgic.nr_spis irqs
* are used for SPIs; the rests are used for per cpu irqs */
if ( irq < 32 )
+ {
+ ASSERT(v);
n = &v->arch.vgic.pending_irqs[irq];
+ }
else
- n = &v->domain->arch.vgic.pending_irqs[irq - 32];
+ n = spi_to_pending(d, irq);
+
return n;
}
-struct pending_irq *spi_to_pending(struct domain *d, unsigned int irq)
+void vgic_put_pending_irq(struct domain *d, struct pending_irq *p)
{
- ASSERT(irq >= NR_LOCAL_IRQS);
+}
- return &d->arch.vgic.pending_irqs[irq - 32];
+void vgic_put_pending_irq_unlock(struct domain *d, struct pending_irq *p)
+{
+ spin_unlock(&p->lock);
+ vgic_put_pending_irq(d, p);
}
void vgic_clear_pending_irqs(struct vcpu *v)
@@ -494,7 +511,7 @@ void vgic_clear_pending_irqs(struct vcpu *v)
void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
{
- struct pending_irq *iter, *n = irq_to_pending(v, virq);
+ struct pending_irq *iter, *n = vgic_get_pending_irq(v->domain, v, virq);
unsigned long flags;
bool running;
@@ -504,6 +521,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
if ( test_bit(_VPF_down, &v->pause_flags) )
{
spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+ vgic_put_pending_irq(v->domain, n);
return;
}
@@ -536,6 +554,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
spin_unlock(&n->lock);
out:
+ vgic_put_pending_irq(v->domain, n);
spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
/* we have a new higher priority irq, inject it into the guest */
running = v->is_running;
@@ -550,12 +569,13 @@ out:
void vgic_vcpu_inject_spi(struct domain *d, unsigned int virq)
{
struct vcpu *v;
- struct pending_irq *p = irq_to_pending(d->vcpu[0], virq);
+ struct pending_irq *p = vgic_get_pending_irq(d, NULL, virq);
/* the IRQ needs to be an SPI */
ASSERT(virq >= 32 && virq <= vgic_num_irqs(d));
v = vgic_get_target_vcpu(d, p);
+ vgic_put_pending_irq(d, p);
vgic_vcpu_inject_irq(v, virq);
}
diff --git a/xen/include/asm-arm/event.h b/xen/include/asm-arm/event.h
index 5330dfe..df672e2 100644
--- a/xen/include/asm-arm/event.h
+++ b/xen/include/asm-arm/event.h
@@ -16,8 +16,7 @@ static inline int vcpu_event_delivery_is_enabled(struct vcpu *v)
static inline int local_events_need_delivery_nomask(void)
{
- struct pending_irq *p = irq_to_pending(current,
- current->domain->arch.evtchn_irq);
+ int ret = 0;
/* XXX: if the first interrupt has already been delivered, we should
* check whether any other interrupts with priority higher than the
@@ -28,11 +27,20 @@ static inline int local_events_need_delivery_nomask(void)
* case.
*/
if ( gic_events_need_delivery() )
- return 1;
+ {
+ ret = 1;
+ }
+ else
+ {
+ struct pending_irq *p;
- if ( vcpu_info(current, evtchn_upcall_pending) &&
- list_empty(&p->inflight) )
- return 1;
+ p = vgic_get_pending_irq(current->domain, current,
+ current->domain->arch.evtchn_irq);
+ if ( vcpu_info(current, evtchn_upcall_pending) &&
+ list_empty(&p->inflight) )
+ ret = 1;
+ vgic_put_pending_irq(current->domain, p);
+ }
return 0;
}
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 186e6df..36e4de2 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -299,7 +299,12 @@ extern struct vcpu *vgic_get_target_vcpu(struct domain *d,
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 struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
+extern struct pending_irq *vgic_get_pending_irq(struct domain *d,
+ struct vcpu *v,
+ unsigned int irq);
+extern void vgic_put_pending_irq(struct domain *d, struct pending_irq *p);
+extern void vgic_put_pending_irq_unlock(struct domain *d,
+ struct pending_irq *p);
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);
extern struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq);
--
2.9.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [RFC PATCH 10/10] ARM: vGIC: remove struct irq_rank and support functions
2017-05-04 15:31 [RFC PATCH 00/10] ARM VGIC rework: remove rank, introduce per-IRQ lock Andre Przywara
` (8 preceding siblings ...)
2017-05-04 15:31 ` [RFC PATCH 09/10] ARM: vGIC: introduce vgic_get/put_pending_irq Andre Przywara
@ 2017-05-04 15:31 ` Andre Przywara
9 siblings, 0 replies; 31+ messages in thread
From: Andre Przywara @ 2017-05-04 15:31 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini; +Cc: xen-devel
Now that every information formerly stored in the irq_rank has been
transferred over to struct pending_irq, we can get rid of all dead code
declaring and initializing the structure and all the support functions.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
xen/arch/arm/vgic.c | 55 --------------------------------------------
xen/include/asm-arm/domain.h | 6 +----
xen/include/asm-arm/vgic.h | 48 --------------------------------------
3 files changed, 1 insertion(+), 108 deletions(-)
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index c7d645e..7680cd8 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -31,35 +31,6 @@
#include <asm/gic.h>
#include <asm/vgic.h>
-static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v, int rank)
-{
- if ( rank == 0 )
- return v->arch.vgic.private_irqs;
- else if ( rank <= DOMAIN_NR_RANKS(v->domain) )
- return &v->domain->arch.vgic.shared_irqs[rank - 1];
- else
- return NULL;
-}
-
-/*
- * Returns rank corresponding to a GICD_<FOO><n> register for
- * GICD_<FOO> with <b>-bits-per-interrupt.
- */
-struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v, int b, int n,
- int s)
-{
- int rank = REG_RANK_NR(b, (n >> s));
-
- return vgic_get_rank(v, rank);
-}
-
-struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq)
-{
- int rank = irq/32;
-
- return vgic_get_rank(v, rank);
-}
-
static void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq,
int vcpu_id)
{
@@ -70,14 +41,6 @@ static void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq,
p->vcpu_id = vcpu_id;
}
-static void vgic_rank_init(struct vgic_irq_rank *rank, uint8_t index,
- unsigned int vcpu)
-{
- spin_lock_init(&rank->lock);
-
- rank->index = index;
-}
-
int domain_vgic_register(struct domain *d, int *mmio_count)
{
switch ( d->arch.vgic.version )
@@ -116,11 +79,6 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
spin_lock_init(&d->arch.vgic.lock);
- d->arch.vgic.shared_irqs =
- xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_RANKS(d));
- if ( d->arch.vgic.shared_irqs == NULL )
- return -ENOMEM;
-
d->arch.vgic.pending_irqs =
xzalloc_array(struct pending_irq, d->arch.vgic.nr_spis);
if ( d->arch.vgic.pending_irqs == NULL )
@@ -130,10 +88,6 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
for (i=0; i<d->arch.vgic.nr_spis; i++)
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);
-
ret = d->arch.vgic.handler->domain_init(d);
if ( ret )
return ret;
@@ -174,7 +128,6 @@ void domain_vgic_free(struct domain *d)
}
d->arch.vgic.handler->domain_free(d);
- xfree(d->arch.vgic.shared_irqs);
xfree(d->arch.vgic.pending_irqs);
xfree(d->arch.vgic.allocated_irqs);
}
@@ -183,13 +136,6 @@ int vcpu_vgic_init(struct vcpu *v)
{
int i;
- v->arch.vgic.private_irqs = xzalloc(struct vgic_irq_rank);
- if ( v->arch.vgic.private_irqs == NULL )
- return -ENOMEM;
-
- /* SGIs/PPIs are always routed to this VCPU */
- vgic_rank_init(v->arch.vgic.private_irqs, 0, v->vcpu_id);
-
v->domain->arch.vgic.handler->vcpu_init(v);
memset(&v->arch.vgic.pending_irqs, 0, sizeof(v->arch.vgic.pending_irqs));
@@ -206,7 +152,6 @@ int vcpu_vgic_init(struct vcpu *v)
int vcpu_vgic_free(struct vcpu *v)
{
- xfree(v->arch.vgic.private_irqs);
return 0;
}
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 6de8082..19531b0 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -83,15 +83,12 @@ struct arch_domain
* shared_irqs where each member contains its own locking.
*
* If both class of lock is required then this lock must be
- * taken first. If multiple rank locks are required (including
- * the per-vcpu private_irqs rank) then they must be taken in
- * rank order.
+ * taken first.
*/
spinlock_t lock;
uint32_t ctlr;
int nr_spis; /* Number of SPIs */
unsigned long *allocated_irqs; /* bitmap of IRQs allocated */
- struct vgic_irq_rank *shared_irqs;
/*
* SPIs are domain global, SGIs and PPIs are per-VCPU and stored in
* struct arch_vcpu.
@@ -236,7 +233,6 @@ struct arch_vcpu
* struct arch_domain.
*/
struct pending_irq pending_irqs[32];
- struct vgic_irq_rank *private_irqs;
/* This list is ordered by IRQ priority and it is used to keep
* track of the IRQs that the VGIC injected into the guest.
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 36e4de2..72095b8 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -96,16 +96,6 @@ struct pending_irq
spinlock_t lock;
};
-#define NR_INTERRUPT_PER_RANK 32
-#define INTERRUPT_RANK_MASK (NR_INTERRUPT_PER_RANK - 1)
-
-/* Represents state corresponding to a block of 32 interrupts */
-struct vgic_irq_rank {
- spinlock_t lock; /* Covers access to all other members of this struct */
-
- uint8_t index;
-};
-
struct sgi_target {
uint8_t aff1;
uint16_t list;
@@ -130,39 +120,9 @@ struct vgic_ops {
const unsigned int max_vcpus;
};
-/* Number of ranks of interrupt registers for a domain */
-#define DOMAIN_NR_RANKS(d) (((d)->arch.vgic.nr_spis+31)/32)
-
#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, 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
- * <b>-bits-per-interrupt
- */
-static inline int REG_RANK_NR(int b, uint32_t n)
-{
- switch ( b )
- {
- /*
- * IRQ ranks are of size 32. So n cannot be shifted beyond 5 for 32
- * and above. For 64-bit n is already shifted DBAT_DOUBLE_WORD
- * by the caller
- */
- case 64:
- case 32: return n >> 5;
- case 16: return n >> 4;
- case 8: return n >> 3;
- case 4: return n >> 2;
- case 2: return n >> 1;
- case 1: return n;
- default: BUG();
- }
-}
-
uint32_t gather_irq_info_priority(struct vcpu *v, unsigned int irq);
void scatter_irq_info_priority(struct vcpu *v, unsigned int irq,
unsigned int value);
@@ -283,12 +243,6 @@ VGIC_REG_HELPERS(32, 0x3);
enum gic_sgi_mode;
-/*
- * Offset of GICD_<FOO><n> with its rank, for GICD_<FOO> size <s> with
- * <b>-bits-per-interrupt.
- */
-#define REG_RANK_INDEX(b, n, s) ((((n) >> s) & ((b)-1)) % 32)
-
#define vgic_num_irqs(d) ((d)->arch.vgic.nr_spis + 32)
extern int domain_vgic_init(struct domain *d, unsigned int nr_spis);
@@ -306,8 +260,6 @@ extern void vgic_put_pending_irq(struct domain *d, struct pending_irq *p);
extern void vgic_put_pending_irq_unlock(struct domain *d,
struct pending_irq *p);
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);
-extern struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq);
extern bool vgic_emulate(struct cpu_user_regs *regs, union hsr hsr);
extern void vgic_disable_irqs(struct vcpu *v, unsigned int irq, uint32_t r);
extern void vgic_enable_irqs(struct vcpu *v, unsigned int irq, uint32_t r);
--
2.9.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 01/10] ARM: vGIC: remove rank lock from IRQ routing functions
2017-05-04 15:31 ` [RFC PATCH 01/10] ARM: vGIC: remove rank lock from IRQ routing functions Andre Przywara
@ 2017-05-04 15:53 ` Julien Grall
2017-05-08 9:15 ` Andre Przywara
0 siblings, 1 reply; 31+ messages in thread
From: Julien Grall @ 2017-05-04 15:53 UTC (permalink / raw)
To: Andre Przywara, Stefano Stabellini; +Cc: xen-devel
Hi Andre,
On 04/05/17 16:31, Andre Przywara wrote:
> gic_route_irq_to_guest() and gic_remove_irq_from_guest() take the rank
> lock, however never actually access the rank structure.
> Avoid taking the lock in those two functions and remove some more
> unneeded code on the way.
The rank is here to protect p->desc when checking that the virtual
interrupt was not yet routed to another physical interrupt.
Without this locking, you can have two concurrent call of
gic_route_irq_to_guest that will update the same virtual interrupt but
with different physical interrupts.
You would have to replace the rank lock by the per-pending_irq lock to
keep the code safe.
Cheers,
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> xen/arch/arm/gic.c | 28 ++++------------------------
> 1 file changed, 4 insertions(+), 24 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index da19130..c734f66 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -136,25 +136,19 @@ void gic_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
> int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
> struct irq_desc *desc, unsigned int priority)
> {
> - unsigned long flags;
> /* Use vcpu0 to retrieve the pending_irq struct. Given that we only
> * route SPIs to guests, it doesn't make any difference. */
> - struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
> - struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
> - struct pending_irq *p = irq_to_pending(v_target, virq);
> - int res = -EBUSY;
> + struct pending_irq *p = irq_to_pending(d->vcpu[0], virq);
>
> ASSERT(spin_is_locked(&desc->lock));
> /* Caller has already checked that the IRQ is an SPI */
> ASSERT(virq >= 32);
> ASSERT(virq < vgic_num_irqs(d));
>
> - vgic_lock_rank(v_target, rank, flags);
> -
> if ( p->desc ||
> /* The VIRQ should not be already enabled by the guest */
> test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
> - goto out;
> + return -EBUSY;
>
> desc->handler = gic_hw_ops->gic_guest_irq_type;
> set_bit(_IRQ_GUEST, &desc->status);
> @@ -164,29 +158,20 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
> gic_set_irq_priority(desc, priority);
>
> p->desc = desc;
> - res = 0;
>
> -out:
> - vgic_unlock_rank(v_target, rank, flags);
> -
> - return res;
> + return 0;
> }
>
> /* This function only works with SPIs for now */
> int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
> struct irq_desc *desc)
> {
> - struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
> - struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
> - struct pending_irq *p = irq_to_pending(v_target, virq);
> - unsigned long flags;
> + struct pending_irq *p = irq_to_pending(d->vcpu[0], virq);
>
> ASSERT(spin_is_locked(&desc->lock));
> ASSERT(test_bit(_IRQ_GUEST, &desc->status));
> ASSERT(p->desc == desc);
>
> - vgic_lock_rank(v_target, rank, flags);
> -
> if ( d->is_dying )
> {
> desc->handler->shutdown(desc);
> @@ -204,10 +189,7 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
> */
> if ( test_bit(_IRQ_INPROGRESS, &desc->status) ||
> !test_bit(_IRQ_DISABLED, &desc->status) )
> - {
> - vgic_unlock_rank(v_target, rank, flags);
> return -EBUSY;
> - }
> }
>
> clear_bit(_IRQ_GUEST, &desc->status);
> @@ -215,8 +197,6 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>
> p->desc = NULL;
>
> - vgic_unlock_rank(v_target, rank, flags);
> -
> return 0;
> }
>
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 04/10] ARM: vGIC: add struct pending_irq locking
2017-05-04 15:31 ` [RFC PATCH 04/10] ARM: vGIC: add struct pending_irq locking Andre Przywara
@ 2017-05-04 16:21 ` Julien Grall
2017-05-05 9:02 ` Andre Przywara
2017-05-05 23:42 ` Stefano Stabellini
2017-05-04 16:54 ` Julien Grall
1 sibling, 2 replies; 31+ messages in thread
From: Julien Grall @ 2017-05-04 16:21 UTC (permalink / raw)
To: Andre Przywara, Stefano Stabellini; +Cc: xen-devel
Hi Andre,
On 04/05/17 16:31, Andre Przywara wrote:
> Introduce the proper locking sequence for the new pending_irq lock.
> This takes the lock around multiple accesses to struct members,
> also makes sure we observe the locking order (VGIC VCPU lock first,
> then pending_irq lock).
This locking order should be explained in the code. Likely in vgic.h.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> xen/arch/arm/gic.c | 26 ++++++++++++++++++++++++++
> xen/arch/arm/vgic.c | 12 +++++++++++-
> 2 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 67375a2..e175e9b 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -351,6 +351,7 @@ void gic_disable_cpu(void)
> static inline void gic_set_lr(int lr, struct pending_irq *p,
> unsigned int state)
> {
> + ASSERT(spin_is_locked(&p->lock));
> ASSERT(!local_irq_is_enabled());
>
> gic_hw_ops->update_lr(lr, p, state);
> @@ -413,6 +414,7 @@ void gic_raise_guest_irq(struct vcpu *v, struct pending_irq *p)
> unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
>
> ASSERT(spin_is_locked(&v->arch.vgic.lock));
> + ASSERT(spin_is_locked(&p->lock));
>
> if ( v == current && list_empty(&v->arch.vgic.lr_pending) )
> {
> @@ -439,6 +441,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
> gic_hw_ops->read_lr(i, &lr_val);
> irq = lr_val.virq;
> p = irq_to_pending(v, irq);
> + spin_lock(&p->lock);
> if ( lr_val.state & GICH_LR_ACTIVE )
> {
> set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
> @@ -495,6 +498,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
> }
> }
> }
> + spin_unlock(&p->lock);
> }
>
> void gic_clear_lrs(struct vcpu *v)
> @@ -545,14 +549,30 @@ static void gic_restore_pending_irqs(struct vcpu *v)
> /* No more free LRs: find a lower priority irq to evict */
> list_for_each_entry_reverse( p_r, inflight_r, inflight )
> {
> + if ( p_r->irq < p->irq )
> + {
> + spin_lock(&p_r->lock);
> + spin_lock(&p->lock);
> + }
> + else
> + {
> + spin_lock(&p->lock);
> + spin_lock(&p_r->lock);
> + }
Please explain in the commit message and the code why this locking order.
> if ( p_r->priority == p->priority )
> + {
> + spin_unlock(&p->lock);
> + spin_unlock(&p_r->lock);
> goto out;
> + }
> if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status) &&
> !test_bit(GIC_IRQ_GUEST_ACTIVE, &p_r->status) )
> goto found;
> }
> /* We didn't find a victim this time, and we won't next
> * time, so quit */
> + spin_unlock(&p->lock);
> + spin_unlock(&p_r->lock);
> goto out;
>
> found:
> @@ -562,12 +582,18 @@ found:
> clear_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status);
> gic_add_to_lr_pending(v, p_r);
> inflight_r = &p_r->inflight;
> +
> + spin_unlock(&p_r->lock);
> }
> + else
> + spin_lock(&p->lock);
>
> gic_set_lr(lr, p, GICH_LR_PENDING);
> list_del_init(&p->lr_queue);
> set_bit(lr, &this_cpu(lr_mask));
>
> + spin_unlock(&p->lock);
> +
> /* We can only evict nr_lrs entries */
> lrs--;
> if ( lrs == 0 )
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index f4ae454..44363bb 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -356,11 +356,16 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
> while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
> irq = i + (32 * n);
> v_target = vgic_get_target_vcpu(v, irq);
> +
> + spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
> p = irq_to_pending(v_target, irq);
> + spin_lock(&p->lock);
> +
> set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> - spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
IHMO, this should belong to a separate patch as not strictly relate to
this one.
> +
Spurious change.
> if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
> gic_raise_guest_irq(v_target, p);
> + spin_unlock(&p->lock);
> spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
> if ( p->desc != NULL )
> {
> @@ -482,10 +487,12 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
> return;
> }
>
> + spin_lock(&n->lock);
> set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
>
> if ( !list_empty(&n->inflight) )
> {
> + spin_unlock(&n->lock);
> gic_raise_inflight_irq(v, n);
Any reason to not code gic_raise_inflight_irq with the lock? This would
also simplify quite a lot the function and avoid unlock in pretty much
all the exit path.
> goto out;
> }
> @@ -501,10 +508,13 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
> if ( iter->priority > priority )
> {
> list_add_tail(&n->inflight, &iter->inflight);
> + spin_unlock(&n->lock);
> goto out;
> }
> }
> list_add_tail(&n->inflight, &v->arch.vgic.inflight_irqs);
> + spin_unlock(&n->lock);
> +
> out:
> spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> /* we have a new higher priority irq, inject it into the guest */
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 05/10] ARM: vGIC: move priority from irq_rank to struct pending_irq
2017-05-04 15:31 ` [RFC PATCH 05/10] ARM: vGIC: move priority from irq_rank to struct pending_irq Andre Przywara
@ 2017-05-04 16:39 ` Julien Grall
2017-05-04 16:47 ` Julien Grall
0 siblings, 1 reply; 31+ messages in thread
From: Julien Grall @ 2017-05-04 16:39 UTC (permalink / raw)
To: Andre Przywara, Stefano Stabellini; +Cc: xen-devel
Hi Andre,
On 04/05/17 16:31, Andre Przywara wrote:
> Currently we store the priority for newly triggered IRQs in the rank
> structure. To get eventually rid of this structure, move this value
> into the struct pending_irq. This one already contains a priority value,
> but we have to keep the two apart, as the priority for guest visible
> IRQs must not change while they are in a VCPU.
> This patch introduces a framework to get some per-IRQ information for a
> number of interrupts and collate them into one register. Similarily
s/Similarily/Similarly/
> there is the opposite function to spread values from one register into
> multiple pending_irq's.
Also, the last paragraph is a call to split the patch in two:
1) Introducing the framework
2) Move priority from irq_rank to struct pending_irq
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> xen/arch/arm/vgic-v2.c | 33 +++++++++--------------------
> xen/arch/arm/vgic-v3.c | 33 ++++++++++-------------------
> xen/arch/arm/vgic.c | 53 ++++++++++++++++++++++++++++++++++------------
> xen/include/asm-arm/vgic.h | 17 ++++++---------
> 4 files changed, 67 insertions(+), 69 deletions(-)
>
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index dc9f95b..5c59fb4 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;
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;
> -
> 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;
> -
> - vgic_lock_rank(v, rank, flags);
> - ipriorityr = rank->ipriorityr[REG_RANK_INDEX(8,
> - gicd_reg - GICD_IPRIORITYR,
> - DABT_WORD)];
> - vgic_unlock_rank(v, rank, flags);
> - *r = vgic_reg32_extract(ipriorityr, info);
> -
> + irq = gicd_reg - GICD_IPRIORITYR;
This variable name does not make sense. This is not rely an irq but an
offset.
> + *r = vgic_reg32_extract(gather_irq_info_priority(v, irq), info);
> 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;
s/irq/virq/
>
> perfc_incr(vgicd_writes);
>
> @@ -499,17 +489,14 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
>
> case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
> {
> - uint32_t *ipriorityr;
> + uint32_t ipriorityr;
>
> 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)];
> - vgic_reg32_update(ipriorityr, r, info);
> - vgic_unlock_rank(v, rank, flags);
> + irq = gicd_reg - GICD_IPRIORITYR;
> +
> + ipriorityr = gather_irq_info_priority(v, irq);
> + vgic_reg32_update(&ipriorityr, r, info);
> + scatter_irq_info_priority(v, irq, ipriorityr);
> return 1;
> }
>
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index d10757a..10db939 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -476,6 +476,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;
s/irq/virq/
>
> switch ( reg )
> {
> @@ -513,22 +514,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;
> -
> 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;
> -
> - vgic_lock_rank(v, rank, flags);
> - ipriorityr = rank->ipriorityr[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR,
> - DABT_WORD)];
> - vgic_unlock_rank(v, rank, flags);
> -
> - *r = vgic_reg32_extract(ipriorityr, info);
> -
> + irq = reg - GICD_IPRIORITYR;
> + if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto read_as_zero;
This check will likely belong to an helper.
> + *r = vgic_reg32_extract(gather_irq_info_priority(v, irq), info);
> return 1;
> - }
>
> case VRANGE32(GICD_ICFGR, GICD_ICFGRN):
> {
> @@ -572,6 +562,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;
s/irq/virq/
>
> switch ( reg )
> {
> @@ -630,16 +621,14 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
>
> case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
> {
> - uint32_t *ipriorityr;
> + uint32_t ipriorityr;
>
> 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)];
> - vgic_reg32_update(ipriorityr, r, info);
> - vgic_unlock_rank(v, rank, flags);
> + irq = reg - GICD_IPRIORITYR;
> + if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto write_ignore;
Ditto.
> + ipriorityr = gather_irq_info_priority(v, irq);
> + vgic_reg32_update(&ipriorityr, r, info);
> + scatter_irq_info_priority(v, irq, ipriorityr);
> return 1;
> }
>
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 44363bb..68eef47 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -225,19 +225,49 @@ 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)
> +static uint8_t extract_priority(struct pending_irq *p)
Please append vgic_
> {
> - struct vgic_irq_rank *rank = vgic_rank_irq(v, virq);
> - unsigned long flags;
> - int priority;
> + return p->new_priority;
> +}
> +
> +static void set_priority(struct pending_irq *p, uint8_t prio)
Ditto.
> +{
> + p->new_priority = prio;
> +}
> +
>
> - vgic_lock_rank(v, rank, flags);
> - priority = rank->priority[virq & INTERRUPT_RANK_MASK];
> - vgic_unlock_rank(v, rank, flags);
> +#define DEFINE_GATHER_IRQ_INFO(name, get_val, shift) \
> +uint32_t gather_irq_info_##name(struct vcpu *v, unsigned int irq) \
The name of this function are too generic. This should at least contain
vgic.
Also I would like to keep the naming consistent with what we did with
irouter and itargetr. I.e fetch/store.
Lastly, irq is confusing? How many irqs will it gather/scatter?
> +{ \
> + uint32_t ret = 0, i; \
newline here.
> + for ( i = 0; i < (32 / shift); i++ ) \
Why 32?
> + { \
> + struct pending_irq *p = irq_to_pending(v, irq + i); \
> + spin_lock(&p->lock); \
I am fairly surprised that you don't need to disable the interrupts
here. the pending_irq lock will be used in vgic_inject_irq which will be
called in the interrupt context.
> + ret |= get_val(p) << (shift * i); \
> + spin_unlock(&p->lock); \
> + } \
> + return ret; \
> +}
>
> - return priority;
> +#define DEFINE_SCATTER_IRQ_INFO(name, set_val, shift) \
Why do you need to define two separate macros? Both could be done at the
same time.
> +void scatter_irq_info_##name(struct vcpu *v, unsigned int irq, \
Same remarks as above.
> + unsigned int value) \
> +{ \
> + unsigned int i; \
newline here.
> + for ( i = 0; i < (32 / shift); i++ ) \
> + { \
> + struct pending_irq *p = irq_to_pending(v, irq + i); \
> + spin_lock(&p->lock); \
> + set_val(p, (value >> (shift * i)) & ((1 << shift) - 1)); \
> + spin_unlock(&p->lock); \
> + } \
> }
I do think those functions have to be introduced in a separate patch.
>
> +/* grep fodder: gather_irq_info_priority, scatter_irq_info_priority below */
> +DEFINE_GATHER_IRQ_INFO(priority, extract_priority, 8)
> +DEFINE_SCATTER_IRQ_INFO(priority, set_priority, 8)
> +
> bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
> {
> unsigned long flags;
> @@ -471,13 +501,10 @@ 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 = irq_to_pending(v, virq);
> unsigned long flags;
> bool running;
>
> - priority = vgic_get_virq_priority(v, virq);
> -
> spin_lock_irqsave(&v->arch.vgic.lock, flags);
>
> /* vcpu offline */
> @@ -497,7 +524,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
> goto out;
> }
>
> - n->priority = priority;
> + n->priority = n->new_priority;
>
> /* the irq is enabled */
> if ( test_bit(GIC_IRQ_GUEST_ENABLED, &n->status) )
> @@ -505,7 +532,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
>
> list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight )
> {
> - if ( iter->priority > priority )
> + if ( iter->priority > n->priority )
> {
> list_add_tail(&n->inflight, &iter->inflight);
> spin_unlock(&n->lock);
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index e7322fc..38a5e76 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -71,7 +71,8 @@ struct pending_irq
> unsigned int irq;
> #define GIC_INVALID_LR (uint8_t)~0
> uint8_t lr;
> - uint8_t priority;
> + uint8_t priority; /* the priority of the currently inflight IRQ */
> + uint8_t new_priority; /* the priority of newly triggered IRQs */
> /* inflight is used to append instances of pending_irq to
> * vgic.inflight_irqs */
> struct list_head inflight;
> @@ -104,16 +105,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
> @@ -179,6 +170,10 @@ static inline int REG_RANK_NR(int b, uint32_t n)
> }
> }
>
> +uint32_t gather_irq_info_priority(struct vcpu *v, unsigned int irq);
> +void scatter_irq_info_priority(struct vcpu *v, unsigned int irq,
> + unsigned int value);
> +
> #define VGIC_REG_MASK(size) ((~0UL) >> (BITS_PER_LONG - ((1 << (size)) * 8)))
>
> /*
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 05/10] ARM: vGIC: move priority from irq_rank to struct pending_irq
2017-05-04 16:39 ` Julien Grall
@ 2017-05-04 16:47 ` Julien Grall
0 siblings, 0 replies; 31+ messages in thread
From: Julien Grall @ 2017-05-04 16:47 UTC (permalink / raw)
To: Andre Przywara, Stefano Stabellini; +Cc: xen-devel
On 04/05/17 17:39, Julien Grall wrote:
> Hi Andre,
>
> On 04/05/17 16:31, Andre Przywara wrote:
>> Currently we store the priority for newly triggered IRQs in the rank
>> structure. To get eventually rid of this structure, move this value
>> into the struct pending_irq. This one already contains a priority value,
>> but we have to keep the two apart, as the priority for guest visible
>> IRQs must not change while they are in a VCPU.
>> This patch introduces a framework to get some per-IRQ information for a
>> number of interrupts and collate them into one register. Similarily
>
> s/Similarily/Similarly/
>
>> there is the opposite function to spread values from one register into
>> multiple pending_irq's.
>
> Also, the last paragraph is a call to split the patch in two:
> 1) Introducing the framework
> 2) Move priority from irq_rank to struct pending_irq
>
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>> xen/arch/arm/vgic-v2.c | 33 +++++++++--------------------
>> xen/arch/arm/vgic-v3.c | 33 ++++++++++-------------------
>> xen/arch/arm/vgic.c | 53
>> ++++++++++++++++++++++++++++++++++------------
>> xen/include/asm-arm/vgic.h | 17 ++++++---------
>> 4 files changed, 67 insertions(+), 69 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
>> index dc9f95b..5c59fb4 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;
>
> 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;
>> -
>> 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;
>> -
>> - vgic_lock_rank(v, rank, flags);
>> - ipriorityr = rank->ipriorityr[REG_RANK_INDEX(8,
>> - gicd_reg -
>> GICD_IPRIORITYR,
>> - DABT_WORD)];
>> - vgic_unlock_rank(v, rank, flags);
>> - *r = vgic_reg32_extract(ipriorityr, info);
>> -
>> + irq = gicd_reg - GICD_IPRIORITYR;
Actually there are an issue with this code. You don't handle correctly
byte access and vgic_reg32_extract expects the interrupt
IHMO, this kind of problem could be handled directly in
gather_irq_info_priority by passing the offset. See how we deal in
vgic_fetch_itarger.
The behavior of those functions should really be explained the commit
message.
Also I don't see any check to prevent reading interrupts outside of the
one supported by the vGIC for this domain.
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 04/10] ARM: vGIC: add struct pending_irq locking
2017-05-04 15:31 ` [RFC PATCH 04/10] ARM: vGIC: add struct pending_irq locking Andre Przywara
2017-05-04 16:21 ` Julien Grall
@ 2017-05-04 16:54 ` Julien Grall
2017-05-05 23:26 ` Stefano Stabellini
1 sibling, 1 reply; 31+ messages in thread
From: Julien Grall @ 2017-05-04 16:54 UTC (permalink / raw)
To: Andre Przywara, Stefano Stabellini; +Cc: xen-devel
On 04/05/17 16:31, Andre Przywara wrote:
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index f4ae454..44363bb 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -356,11 +356,16 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
> while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
> irq = i + (32 * n);
> v_target = vgic_get_target_vcpu(v, irq);
> +
> + spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
> p = irq_to_pending(v_target, irq);
> + spin_lock(&p->lock);
> +
> set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> - 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, p);
> + spin_unlock(&p->lock);
Why does the lock not cover p->desc below?
> spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
> if ( p->desc != NULL )
> {
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 06/10] ARM: vGIC: move config from irq_rank to struct pending_irq
2017-05-04 15:31 ` [RFC PATCH 06/10] ARM: vGIC: move config " Andre Przywara
@ 2017-05-04 16:55 ` Julien Grall
0 siblings, 0 replies; 31+ messages in thread
From: Julien Grall @ 2017-05-04 16:55 UTC (permalink / raw)
To: Andre Przywara, Stefano Stabellini; +Cc: xen-devel
Hi Andre,
On 04/05/17 16:31, Andre Przywara wrote:
> Currently we store the interrupt type configuration (level or edge
> triggered) in the rank structure. Move this value into struct pending_irq.
> We just need one bit (edge or level), so use one of the status bits.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> xen/arch/arm/vgic-v2.c | 29 ++++++++++-------------------
> xen/arch/arm/vgic-v3.c | 31 ++++++++++++-------------------
> xen/arch/arm/vgic.c | 39 ++++++++++++++++++++-------------------
> xen/include/asm-arm/vgic.h | 7 ++++++-
> 4 files changed, 48 insertions(+), 58 deletions(-)
>
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 5c59fb4..795173c 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -278,20 +278,10 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
> goto read_reserved;
>
> case VRANGE32(GICD_ICFGR, GICD_ICFGRN):
> - {
> - uint32_t icfgr;
> -
> 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, flags);
> - icfgr = rank->icfg[REG_RANK_INDEX(2, gicd_reg - GICD_ICFGR, DABT_WORD)];
> - vgic_unlock_rank(v, rank, flags);
> -
> - *r = vgic_reg32_extract(icfgr, info);
> -
> + irq = (gicd_reg - GICD_ICFGR) * 4;
This would be nicer to have that handle directly in
gather_irq_info_config rather than open-coding everywhere. Also it
avoids this confusion of what irq actually meaning in this context.
I was also expecting some check on whether the interrupts is valid for
the vGIC.
> + *r = vgic_reg32_extract(gather_irq_info_config(v, irq), info);
> return 1;
> - }
>
> case VRANGE32(0xD00, 0xDFC):
> goto read_impl_defined;
> @@ -534,15 +524,16 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
> goto write_ignore_32;
>
> case VRANGE32(GICD_ICFGR2, GICD_ICFGRN): /* SPIs */
> + {
> + uint32_t icfgr;
> +
> 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, flags);
> - vgic_reg32_update(&rank->icfg[REG_RANK_INDEX(2, gicd_reg - GICD_ICFGR,
> - DABT_WORD)],
> - r, info);
> - vgic_unlock_rank(v, rank, flags);
> + irq = (gicd_reg - GICD_ICFGR) * 4;
Ditto for the irq and the check.
> + icfgr = gather_irq_info_config(v, irq);
> + vgic_reg32_update(&icfgr, r, info);
> + scatter_irq_info_config(v, irq, icfgr);
> return 1;
> + }
>
> case VRANGE32(0xD00, 0xDFC):
> goto write_impl_defined;
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 10db939..7989989 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -521,20 +521,11 @@ static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
> return 1;
>
> case VRANGE32(GICD_ICFGR, GICD_ICFGRN):
> - {
> - uint32_t icfgr;
> -
> if ( dabt.size != DABT_WORD ) goto bad_width;
> - rank = vgic_rank_offset(v, 2, reg - GICD_ICFGR, DABT_WORD);
> - if ( rank == NULL ) goto read_as_zero;
> - vgic_lock_rank(v, rank, flags);
> - icfgr = rank->icfg[REG_RANK_INDEX(2, reg - GICD_ICFGR, DABT_WORD)];
> - vgic_unlock_rank(v, rank, flags);
> -
> - *r = vgic_reg32_extract(icfgr, info);
> -
> + irq = (reg - GICD_IPRIORITYR) * 4;
> + if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto read_as_zero;
This should really be an helper.
> + *r = vgic_reg32_extract(gather_irq_info_config(v, irq), info);
> return 1;
> - }
>
> default:
> printk(XENLOG_G_ERR
> @@ -636,17 +627,19 @@ static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
> goto write_ignore_32;
>
> case VRANGE32(GICD_ICFGR + 4, GICD_ICFGRN): /* PPI + SPIs */
> + {
> + uint32_t icfgr;
> +
> /* ICFGR1 for PPI's, which is implementation defined
> if ICFGR1 is programmable or not. We chose to program */
> if ( dabt.size != DABT_WORD ) goto bad_width;
> - rank = vgic_rank_offset(v, 2, reg - GICD_ICFGR, DABT_WORD);
> - if ( rank == NULL ) goto write_ignore;
> - vgic_lock_rank(v, rank, flags);
> - vgic_reg32_update(&rank->icfg[REG_RANK_INDEX(2, reg - GICD_ICFGR,
> - DABT_WORD)],
> - r, info);
> - vgic_unlock_rank(v, rank, flags);
> + irq = (reg - GICD_ICFGR) * 4;
> + if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto write_ignore;
> + icfgr = gather_irq_info_config(v, irq);
> + vgic_reg32_update(&icfgr, r, info);
> + scatter_irq_info_config(v, irq, icfgr);
> return 1;
> + }
>
> default:
> printk(XENLOG_G_ERR
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 68eef47..02c1d12 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -235,6 +235,19 @@ static void set_priority(struct pending_irq *p, uint8_t prio)
> p->new_priority = prio;
> }
>
> +unsigned int extract_config(struct pending_irq *p)
Why this is exported? Also naming.
> +{
> + return test_bit(GIC_IRQ_GUEST_EDGE, &p->status) ? 2 : 0;
Please document 0, 2. Likely with a some define.
> +}
> +
> +static void set_config(struct pending_irq *p, unsigned int config)
Naming.
> +{
> + if ( config < 2 )
Ditto.
> + clear_bit(GIC_IRQ_GUEST_EDGE, &p->status);
> + else
> + set_bit(GIC_IRQ_GUEST_EDGE, &p->status);
> +}
> +
>
> #define DEFINE_GATHER_IRQ_INFO(name, get_val, shift) \
> uint32_t gather_irq_info_##name(struct vcpu *v, unsigned int irq) \
> @@ -267,6 +280,8 @@ void scatter_irq_info_##name(struct vcpu *v, unsigned int irq, \
> /* grep fodder: gather_irq_info_priority, scatter_irq_info_priority below */
> DEFINE_GATHER_IRQ_INFO(priority, extract_priority, 8)
> DEFINE_SCATTER_IRQ_INFO(priority, set_priority, 8)
> +DEFINE_GATHER_IRQ_INFO(config, extract_config, 2)
> +DEFINE_SCATTER_IRQ_INFO(config, set_config, 2)
>
> bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
> {
> @@ -357,27 +372,11 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
> }
> }
>
> -#define VGIC_ICFG_MASK(intr) (1 << ((2 * ((intr) % 16)) + 1))
> -
> -/* The function should be called with the rank lock taken */
> -static inline unsigned int vgic_get_virq_type(struct vcpu *v, int n, int index)
> -{
> - struct vgic_irq_rank *r = vgic_get_rank(v, n);
> - uint32_t tr = r->icfg[index >> 4];
> -
> - ASSERT(spin_is_locked(&r->lock));
> -
> - if ( tr & VGIC_ICFG_MASK(index) )
> - return IRQ_TYPE_EDGE_RISING;
> - else
> - return IRQ_TYPE_LEVEL_HIGH;
> -}
> -
> void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
> {
> const unsigned long mask = r;
> struct pending_irq *p;
> - unsigned int irq;
> + unsigned int irq, int_type;
> unsigned long flags;
> int i = 0;
> struct vcpu *v_target;
> @@ -392,6 +391,8 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
> spin_lock(&p->lock);
>
> set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> + int_type = test_bit(GIC_IRQ_GUEST_EDGE, &p->status) ?
> + IRQ_TYPE_EDGE_RISING : IRQ_TYPE_LEVEL_HIGH;
>
> if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
> gic_raise_guest_irq(v_target, p);
> @@ -399,15 +400,15 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
> spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
> if ( p->desc != NULL )
> {
> - irq_set_affinity(p->desc, cpumask_of(v_target->processor));
> spin_lock_irqsave(&p->desc->lock, flags);
> + irq_set_affinity(p->desc, cpumask_of(v_target->processor));
> /*
> * The irq cannot be a PPI, we only support delivery of SPIs
> * to guests.
> */
> ASSERT(irq >= 32);
> if ( irq_type_set_by_domain(d) )
> - gic_set_irq_type(p->desc, vgic_get_virq_type(v, n, i));
> + gic_set_irq_type(p->desc, int_type);
> p->desc->handler->enable(p->desc);
> spin_unlock_irqrestore(&p->desc->lock, flags);
> }
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 38a5e76..931a672 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -60,12 +60,15 @@ struct pending_irq
> * vcpu while it is still inflight and on an GICH_LR register on the
> * old vcpu.
> *
> + * GIC_IRQ_GUEST_EDGE: the IRQ is an edge triggered interrupt.
Also, explain that by default the interrupt will be level.
> + *
> */
> #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_EDGE 5
> unsigned long status;
> struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */
> unsigned int irq;
> @@ -102,7 +105,6 @@ struct vgic_irq_rank {
> uint8_t index;
>
> uint32_t ienable;
> - uint32_t icfg[2];
>
> /*
> * It's more convenient to store a target VCPU per vIRQ
> @@ -173,6 +175,9 @@ static inline int REG_RANK_NR(int b, uint32_t n)
> uint32_t gather_irq_info_priority(struct vcpu *v, unsigned int irq);
> void scatter_irq_info_priority(struct vcpu *v, unsigned int irq,
> unsigned int value);
> +uint32_t gather_irq_info_config(struct vcpu *v, unsigned int irq);
> +void scatter_irq_info_config(struct vcpu *v, unsigned int irq,
> + unsigned int value);
>
> #define VGIC_REG_MASK(size) ((~0UL) >> (BITS_PER_LONG - ((1 << (size)) * 8)))
>
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 08/10] ARM: vGIC: move target vcpu from irq_rank to struct pending_irq
2017-05-04 15:31 ` [RFC PATCH 08/10] ARM: vGIC: move target vcpu " Andre Przywara
@ 2017-05-04 17:20 ` Julien Grall
0 siblings, 0 replies; 31+ messages in thread
From: Julien Grall @ 2017-05-04 17:20 UTC (permalink / raw)
To: Andre Przywara, Stefano Stabellini; +Cc: xen-devel
Hi Andre,
On 04/05/17 16:31, Andre Przywara wrote:
> So far we kept the target VCPU for SPIs in the rank structure.
> Move that information over into pending_irq.
> This changes vgic_get_target_vcpu(), which now takes only a domain
> and a struct pending_irq to get the target vCPU, in a way that does not
> necessarily require the pending_irq lock to be held.
You don't explain half of the changes made in this patch. Most of them
would be better in separate patch as they don't necessarily depends on
the move from the rank to pending_irq.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> xen/arch/arm/gic.c | 3 +-
> xen/arch/arm/vgic-v2.c | 57 ++++++++++++++---------------------
> xen/arch/arm/vgic-v3.c | 71 ++++++++++++++++++++++----------------------
> xen/arch/arm/vgic.c | 74 ++++++++++++++++++++--------------------------
> xen/include/asm-arm/vgic.h | 15 ++++------
> 5 files changed, 97 insertions(+), 123 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index e175e9b..737da6b 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -492,7 +492,8 @@ static void gic_update_one_lr(struct vcpu *v, int i)
> smp_wmb();
> if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
> {
> - struct vcpu *v_target = vgic_get_target_vcpu(v, irq);
> + struct vcpu *v_target = vgic_get_target_vcpu(v->domain, p);
> +
> irq_set_affinity(p->desc, cpumask_of(v_target->processor));
> 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 22c679c..bf755ae 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -66,19 +66,21 @@ 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)
Why can't you directly use the macros you introduced in #5?
> {
> uint32_t reg = 0;
> unsigned int i;
>
> - 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);
> +
> + spin_lock(&p->lock);
> + reg |= (1 << p->vcpu_id) << (i * NR_BITS_PER_TARGET);
> + spin_unlock(&p->lock);
> + }
>
> return reg;
> }
> @@ -89,32 +91,28 @@ 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)
Please re-indent properly.
> {
> 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;
> 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 +149,17 @@ 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]);
> + spin_lock(&p->lock);
I suspect you need to disable interrupt here.
> +
> + 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);
> + if ( vgic_migrate_irq(p, d->vcpu[old_target], d->vcpu[new_target]) )
> + p->vcpu_id = new_target;
> }
> + spin_unlock(&p->lock);
> }
> }
>
> @@ -168,9 +167,7 @@ static int vgic_v2_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;
> int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
> - unsigned long flags;
> unsigned int irq;
>
> perfc_incr(vgicd_reads);
> @@ -259,11 +256,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);
> *r = vgic_reg32_extract(itargetsr, info);
>
> return 1;
> @@ -385,10 +378,8 @@ static int vgic_v2_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;
> int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
> uint32_t tr;
> - unsigned long flags;
> unsigned int irq;
>
> perfc_incr(vgicd_writes);
> @@ -492,14 +483,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);
> vgic_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);
> - vgic_unlock_rank(v, rank, flags);
> return 1;
> }
>
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 01764c9..15a512a 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -97,18 +97,20 @@ 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;
> + 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);
> + spin_lock(&p->lock);
Same remark for the interrupts.
> + aff = vcpuid_to_vaffinity(p->vcpu_id);
> + spin_unlock(&p->lock);
>
> - return vcpuid_to_vaffinity(read_atomic(&rank->vcpu[offset]));
> + return aff;
> }
>
> /*
> @@ -117,11 +119,13 @@ 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 pending_irq *p;
> unsigned int virq;
> + bool reinject;
>
> /* There is 1 vIRQ per IROUTER */
> virq = offset / NR_BYTES_PER_IROUTER;
> @@ -132,11 +136,11 @@ 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);
> + spin_lock(&p->lock);
>
> new_vcpu = vgic_v3_irouter_to_vcpu(d, irouter);
> - old_vcpu = d->vcpu[read_atomic(&rank->vcpu[offset])];
> + old_vcpu = d->vcpu[p->vcpu_id];
>
> /*
> * From the spec (see 8.9.13 in IHI 0069A), any write with an
> @@ -146,16 +150,21 @@ 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 ( !new_vcpu || new_vcpu == old_vcpu )
Why this change?
> {
> - if ( vgic_migrate_irq(old_vcpu, new_vcpu, virq) )
> - write_atomic(&rank->vcpu[offset], new_vcpu->vcpu_id);
> + spin_unlock(&p->lock);
> + return;
> }
> +
> + reinject = vgic_migrate_irq(p, old_vcpu, new_vcpu);
This change should really be a separate patch and a justification of why
this is necessary.
> +
> + spin_unlock(&p->lock);
> +
> + if ( reinject )
> + vgic_vcpu_inject_irq(new_vcpu, virq);
> }
>
> static inline bool vgic_reg64_check_access(struct hsr_dabt dabt)
> @@ -869,8 +878,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);
> @@ -996,15 +1003,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;
s/irq/virq/
>
> 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 = vgic_reg64_extract(irouter, info);
>
> return 1;
> @@ -1070,8 +1074,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);
> @@ -1185,16 +1187,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);
> vgic_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);
> return 1;
> }
>
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index a23079a..530ac55 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -60,32 +60,22 @@ struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq)
> return vgic_get_rank(v, rank);
> }
>
> -static void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq)
> +static void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq,
> + int vcpu_id)
> {
> INIT_LIST_HEAD(&p->inflight);
> INIT_LIST_HEAD(&p->lr_queue);
> spin_lock_init(&p->lock);
> p->irq = virq;
> + 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);
You dropped this BUILD_BUG_ON and never re-introduce a similar one to
prevent any issue in the future if we decide to extend MAX_VIRT_VCPUS.
> -
> 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)
> @@ -136,8 +126,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++ )
> @@ -202,8 +193,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);
> @@ -218,11 +210,11 @@ int vcpu_vgic_free(struct vcpu *v)
> return 0;
> }
>
> -struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int virq)
> +struct vcpu *vgic_get_target_vcpu(struct domain *d, struct pending_irq *p)
This kind of changes would have been better in a separate patch to help
the review.
> {
> - struct vgic_irq_rank *rank = vgic_rank_irq(v, virq);
> - int target = read_atomic(&rank->vcpu[virq & INTERRUPT_RANK_MASK]);
> - return v->domain->vcpu[target];
> + uint16_t vcpu_id = read_atomic(&p->vcpu_id);
> +
> + return d->vcpu[vcpu_id];
You could have do return d->vcpu[read_atomic(&p->vcpu_id)]. This even
likely can be turned into a static inline function.
> }
>
> static uint8_t extract_priority(struct pending_irq *p)
> @@ -289,31 +281,29 @@ DEFINE_GATHER_IRQ_INFO(enabled, extract_enabled, 1)
> DEFINE_GATHER_IRQ_INFO(config, extract_config, 2)
> DEFINE_SCATTER_IRQ_INFO(config, set_config, 2)
>
> -bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
> +bool vgic_migrate_irq(struct pending_irq *p, struct vcpu *old, struct vcpu *new)
Re-ordering the parameter should have been explained. Also, use
pending_irq could have been done in a separate patch.
Breaking down patchs in very small one really help the review. I admit
this one is quite complex to read it because of the mix of re-ordering,
introduction of new parameters, code modification not really related to
this patch...
> {
> - unsigned long flags;
> - struct pending_irq *p = irq_to_pending(old, irq);
> -
> - /* nothing to do for virtual interrupts */
> - if ( p->desc == NULL )
> - return true;
> + ASSERT(spin_is_locked(&p->lock));
>
> /* 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);
> + gprintk(XENLOG_WARNING, "irq %u migration failed: requested while in progress\n", p->irq);
> return false;
> }
>
> - perfc_incr(vgic_irq_migrates);
> + p->vcpu_id = new->vcpu_id;
Why did you move this code here?
> +
> + /* nothing to do for virtual interrupts */
> + if ( p->desc == NULL )
> + return false;
>
> - spin_lock_irqsave(&old->arch.vgic.lock, flags);
> + perfc_incr(vgic_irq_migrates);
>
> if ( list_empty(&p->inflight) )
> {
> irq_set_affinity(p->desc, cpumask_of(new->processor));
> - spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
> - return true;
> + return false;
> }
> /* If the IRQ is still lr_pending, re-inject it to the new vcpu */
> if ( !list_empty(&p->lr_queue) )
> @@ -322,8 +312,6 @@ bool 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 true;
> }
> /* if the IRQ is in a GICH_LR register, set GIC_IRQ_GUEST_MIGRATING
> @@ -331,8 +319,7 @@ bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
> if ( !list_empty(&p->inflight) )
> set_bit(GIC_IRQ_GUEST_MIGRATING, &p->status);
>
> - spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
> - return true;
> + return false;
> }
>
> void arch_move_irqs(struct vcpu *v)
> @@ -345,11 +332,13 @@ void arch_move_irqs(struct vcpu *v)
>
> for ( i = 32; i < vgic_num_irqs(d); i++ )
> {
> - v_target = vgic_get_target_vcpu(v, i);
> - p = irq_to_pending(v_target, i);
> + p = irq_to_pending(d->vcpu[0], i);
Why d->vcpu[0]?
> + spin_lock(&p->lock);
> + v_target = vgic_get_target_vcpu(d, p);
>
> if ( v_target == v && !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
> irq_set_affinity(p->desc, cpu_mask);
> + spin_unlock(&p->lock);
> }
> }
>
> @@ -362,8 +351,8 @@ void vgic_disable_irqs(struct vcpu *v, unsigned int irq, uint32_t r)
> struct vcpu *v_target;
>
> while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
> - v_target = vgic_get_target_vcpu(v, irq + i);
> - p = irq_to_pending(v_target, irq + i);
> + p = irq_to_pending(v, irq + i);
> + v_target = vgic_get_target_vcpu(v->domain, p);
This kind of re-ordering would have be nice to explain in the commit
message.
> clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> gic_remove_from_queues(v_target, irq + i);
> if ( p->desc != NULL )
> @@ -387,10 +376,10 @@ void vgic_enable_irqs(struct vcpu *v, unsigned int irq, uint32_t r)
> struct domain *d = v->domain;
>
> while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
> - v_target = vgic_get_target_vcpu(v, irq + i);
> + p = irq_to_pending(v, irq + i);
> + v_target = vgic_get_target_vcpu(d, p);
>
> spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
> - p = irq_to_pending(v_target, irq + i);
> spin_lock(&p->lock);
>
> set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> @@ -561,11 +550,12 @@ out:
> void vgic_vcpu_inject_spi(struct domain *d, unsigned int virq)
> {
> struct vcpu *v;
> + struct pending_irq *p = irq_to_pending(d->vcpu[0], virq);
>
> /* the IRQ needs to be an SPI */
> ASSERT(virq >= 32 && virq <= vgic_num_irqs(d));
>
> - v = vgic_get_target_vcpu(d->vcpu[0], virq);
> + v = vgic_get_target_vcpu(d, p);
> vgic_vcpu_inject_irq(v, virq);
> }
>
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index fe09fb8..186e6df 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -76,6 +76,7 @@ struct pending_irq
> uint8_t lr;
> uint8_t priority; /* the priority of the currently inflight IRQ */
> uint8_t new_priority; /* the priority of newly triggered IRQs */
> + uint8_t vcpu_id;
So this is pointing to the new vCPU and not the current. So this should
be clearly identified in the name and in a comment.
> /* inflight is used to append instances of pending_irq to
> * vgic.inflight_irqs */
> struct list_head inflight;
> @@ -103,14 +104,6 @@ struct vgic_irq_rank {
> spinlock_t lock; /* Covers access to all other members of this struct */
>
> uint8_t index;
> -
> - /*
> - * 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 {
> @@ -301,7 +294,8 @@ enum gic_sgi_mode;
> extern int domain_vgic_init(struct domain *d, unsigned int nr_spis);
> 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 virq);
> +extern struct vcpu *vgic_get_target_vcpu(struct domain *d,
> + 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);
> @@ -321,7 +315,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,
> + struct vcpu *old, 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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 09/10] ARM: vGIC: introduce vgic_get/put_pending_irq
2017-05-04 15:31 ` [RFC PATCH 09/10] ARM: vGIC: introduce vgic_get/put_pending_irq Andre Przywara
@ 2017-05-04 17:36 ` Julien Grall
2017-05-04 22:42 ` Stefano Stabellini
0 siblings, 1 reply; 31+ messages in thread
From: Julien Grall @ 2017-05-04 17:36 UTC (permalink / raw)
To: Andre Przywara, Stefano Stabellini; +Cc: xen-devel
Hi Andre,
On 04/05/17 16:31, Andre Przywara wrote:
> So far there is always a statically allocated struct pending_irq for
> each interrupt that we deal with.
> To prepare for dynamically allocated LPIs, introduce a put/get wrapper
> to get hold of a pending_irq pointer.
> So far get() just returns the same pointer and put() is empty, but this
> change allows to introduce ref-counting very easily, to prevent
> use-after-free usage of struct pending_irq's once LPIs get unmapped from
> a domain.
> For convenience reasons we introduce a put_unlock() version, which also
> drops the pending_irq lock before calling the actual put() function.
Please explain where get/put should be used in both the commit message
and the code.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> xen/arch/arm/gic.c | 24 +++++++++++++++------
> xen/arch/arm/vgic-v2.c | 4 ++--
> xen/arch/arm/vgic-v3.c | 4 ++--
> xen/arch/arm/vgic.c | 52 +++++++++++++++++++++++++++++++--------------
> xen/include/asm-arm/event.h | 20 +++++++++++------
> xen/include/asm-arm/vgic.h | 7 +++++-
> 6 files changed, 77 insertions(+), 34 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 737da6b..7147b6c 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -136,9 +136,7 @@ void gic_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
> int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
> struct irq_desc *desc, unsigned int priority)
> {
> - /* Use vcpu0 to retrieve the pending_irq struct. Given that we only
> - * route SPIs to guests, it doesn't make any difference. */
> - struct pending_irq *p = irq_to_pending(d->vcpu[0], virq);
> + struct pending_irq *p = vgic_get_pending_irq(d, NULL, virq);
This vgic_get_pending_irq would benefits of an explanation where is the
associated put.
>
> ASSERT(spin_is_locked(&desc->lock));
> /* Caller has already checked that the IRQ is an SPI */
> @@ -148,7 +146,10 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
> if ( p->desc ||
> /* The VIRQ should not be already enabled by the guest */
> test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
> + {
> + vgic_put_pending_irq(d, p);
> return -EBUSY;
> + }
>
> desc->handler = gic_hw_ops->gic_guest_irq_type;
> set_bit(_IRQ_GUEST, &desc->status);
> @@ -159,6 +160,7 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
>
> p->desc = desc;
>
> + vgic_put_pending_irq(d, p);
Newline.
> return 0;
> }
>
> @@ -166,7 +168,7 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
> int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
> struct irq_desc *desc)
> {
> - struct pending_irq *p = irq_to_pending(d->vcpu[0], virq);
> + struct pending_irq *p = vgic_get_pending_irq(d, NULL, virq);
>
> ASSERT(spin_is_locked(&desc->lock));
> ASSERT(test_bit(_IRQ_GUEST, &desc->status));
> @@ -189,7 +191,10 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
> */
> if ( test_bit(_IRQ_INPROGRESS, &desc->status) ||
> !test_bit(_IRQ_DISABLED, &desc->status) )
> + {
> + vgic_put_pending_irq(d, p);
> return -EBUSY;
> + }
> }
>
> clear_bit(_IRQ_GUEST, &desc->status);
> @@ -197,6 +202,8 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>
> p->desc = NULL;
>
> + vgic_put_pending_irq(d, p);
> +
> return 0;
> }
>
> @@ -383,13 +390,14 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, struct pending_irq *n)
>
> void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq)
> {
> - struct pending_irq *p = irq_to_pending(v, virtual_irq);
> + struct pending_irq *p = vgic_get_pending_irq(v->domain, v, virtual_irq);
> unsigned long flags;
>
> spin_lock_irqsave(&v->arch.vgic.lock, flags);
> if ( !list_empty(&p->lr_queue) )
> list_del_init(&p->lr_queue);
> spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> + vgic_put_pending_irq(v->domain, p);
> }
>
> void gic_raise_inflight_irq(struct vcpu *v, struct pending_irq *n)
> @@ -406,6 +414,7 @@ void gic_raise_inflight_irq(struct vcpu *v, struct pending_irq *n)
> gdprintk(XENLOG_DEBUG, "trying to inject irq=%u into d%dv%d, when it is still lr_pending\n",
> n->irq, v->domain->domain_id, v->vcpu_id);
> #endif
> + vgic_put_pending_irq(v->domain, n);
Why this one?
> }
>
> void gic_raise_guest_irq(struct vcpu *v, struct pending_irq *p)
> @@ -440,8 +449,9 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>
> gic_hw_ops->read_lr(i, &lr_val);
> irq = lr_val.virq;
> - p = irq_to_pending(v, irq);
> + p = vgic_get_pending_irq(v->domain, v, irq);
> spin_lock(&p->lock);
It sounds like to me that you want to introduce a
vgic_get_pending_irq_lock(...).
> +
Spurious change.
> if ( lr_val.state & GICH_LR_ACTIVE )
> {
> set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
> @@ -499,7 +509,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
> }
> }
> }
> - spin_unlock(&p->lock);
> + vgic_put_pending_irq_unlock(v->domain, p);
> }
>
> void gic_clear_lrs(struct vcpu *v)
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index bf755ae..36ed04f 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -75,11 +75,11 @@ static uint32_t vgic_fetch_itargetsr(struct vcpu *v, unsigned int offset)
>
> for ( i = 0; i < NR_TARGETS_PER_ITARGETSR; i++, offset++ )
> {
> - struct pending_irq *p = irq_to_pending(v, offset);
> + struct pending_irq *p = vgic_get_pending_irq(v->domain, v, offset);
>
> spin_lock(&p->lock);
> reg |= (1 << p->vcpu_id) << (i * NR_BITS_PER_TARGET);
> - spin_unlock(&p->lock);
> + vgic_put_pending_irq_unlock(v->domain, p);
> }
>
> return reg;
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 15a512a..fff518e 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -105,10 +105,10 @@ static uint64_t vgic_fetch_irouter(struct vcpu *v, unsigned int offset)
> /* There is exactly 1 vIRQ per IROUTER */
> offset /= NR_BYTES_PER_IROUTER;
>
> - p = irq_to_pending(v, offset);
> + p = vgic_get_pending_irq(v->domain, v, offset);
> spin_lock(&p->lock);
> aff = vcpuid_to_vaffinity(p->vcpu_id);
> - spin_unlock(&p->lock);
> + vgic_put_pending_irq_unlock(v->domain, p);
>
> return aff;
> }
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 530ac55..c7d645e 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -245,17 +245,16 @@ static void set_config(struct pending_irq *p, unsigned int config)
> set_bit(GIC_IRQ_GUEST_EDGE, &p->status);
> }
>
> -
This newline should not have been introduced at first hand.
> #define DEFINE_GATHER_IRQ_INFO(name, get_val, shift) \
> uint32_t gather_irq_info_##name(struct vcpu *v, unsigned int irq) \
> { \
> uint32_t ret = 0, i; \
> for ( i = 0; i < (32 / shift); i++ ) \
> { \
> - struct pending_irq *p = irq_to_pending(v, irq + i); \
> + struct pending_irq *p = vgic_get_pending_irq(v->domain, v, irq + i); \
> spin_lock(&p->lock); \
> ret |= get_val(p) << (shift * i); \
> - spin_unlock(&p->lock); \
> + vgic_put_pending_irq_unlock(v->domain, p); \
> } \
> return ret; \
> }
> @@ -267,10 +266,10 @@ void scatter_irq_info_##name(struct vcpu *v, unsigned int irq, \
> unsigned int i; \
> for ( i = 0; i < (32 / shift); i++ ) \
> { \
> - struct pending_irq *p = irq_to_pending(v, irq + i); \
> + struct pending_irq *p = vgic_get_pending_irq(v->domain, v, irq + i); \
> spin_lock(&p->lock); \
> set_val(p, (value >> (shift * i)) & ((1 << shift) - 1)); \
> - spin_unlock(&p->lock); \
> + vgic_put_pending_irq_unlock(v->domain, p); \
> } \
> }
>
> @@ -332,13 +331,13 @@ void arch_move_irqs(struct vcpu *v)
>
> for ( i = 32; i < vgic_num_irqs(d); i++ )
> {
> - p = irq_to_pending(d->vcpu[0], i);
> + p = vgic_get_pending_irq(d, NULL, i);
> spin_lock(&p->lock);
> v_target = vgic_get_target_vcpu(d, p);
>
> if ( v_target == v && !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
> irq_set_affinity(p->desc, cpu_mask);
> - spin_unlock(&p->lock);
> + vgic_put_pending_irq_unlock(d, p);
> }
> }
>
> @@ -351,7 +350,7 @@ void vgic_disable_irqs(struct vcpu *v, unsigned int irq, uint32_t r)
> struct vcpu *v_target;
>
> while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
> - p = irq_to_pending(v, irq + i);
> + p = vgic_get_pending_irq(v->domain, v, irq + i);
> v_target = vgic_get_target_vcpu(v->domain, p);
> clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> gic_remove_from_queues(v_target, irq + i);
> @@ -361,6 +360,7 @@ void vgic_disable_irqs(struct vcpu *v, unsigned int irq, uint32_t r)
> p->desc->handler->disable(p->desc);
> spin_unlock_irqrestore(&p->desc->lock, flags);
> }
> + vgic_put_pending_irq(v->domain, p);
> i++;
> }
> }
> @@ -376,7 +376,7 @@ void vgic_enable_irqs(struct vcpu *v, unsigned int irq, uint32_t r)
> struct domain *d = v->domain;
>
> while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
> - p = irq_to_pending(v, irq + i);
> + p = vgic_get_pending_irq(d, v, irq + i);
> v_target = vgic_get_target_vcpu(d, p);
>
> spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
> @@ -404,6 +404,7 @@ void vgic_enable_irqs(struct vcpu *v, unsigned int irq, uint32_t r)
> p->desc->handler->enable(p->desc);
> spin_unlock_irqrestore(&p->desc->lock, flags);
> }
> + vgic_put_pending_irq(v->domain, p);
> i++;
> }
> }
> @@ -461,23 +462,39 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode,
> return true;
> }
>
> -struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq)
> +struct pending_irq *spi_to_pending(struct domain *d, unsigned int irq)
> +{
> + ASSERT(irq >= NR_LOCAL_IRQS);
> +
> + return &d->arch.vgic.pending_irqs[irq - 32];
> +}
This re-ordering should have been made in a separate patch. Also the
change of taking a domain too.
But I don't understand why you keep it around.
> +
> +struct pending_irq *vgic_get_pending_irq(struct domain *d, struct vcpu *v,
> + unsigned int irq)
> {
> struct pending_irq *n;
> +
> /* Pending irqs allocation strategy: the first vgic.nr_spis irqs
> * are used for SPIs; the rests are used for per cpu irqs */
> if ( irq < 32 )
> + {
> + ASSERT(v);
> n = &v->arch.vgic.pending_irqs[irq];
> + }
> else
> - n = &v->domain->arch.vgic.pending_irqs[irq - 32];
> + n = spi_to_pending(d, irq);
> +
This change does not belong to this patch.
> return n;
> }
>
> -struct pending_irq *spi_to_pending(struct domain *d, unsigned int irq)
> +void vgic_put_pending_irq(struct domain *d, struct pending_irq *p)
> {
> - ASSERT(irq >= NR_LOCAL_IRQS);
> +}
>
> - return &d->arch.vgic.pending_irqs[irq - 32];
> +void vgic_put_pending_irq_unlock(struct domain *d, struct pending_irq *p)
> +{
> + spin_unlock(&p->lock);
> + vgic_put_pending_irq(d, p);
> }
>
> void vgic_clear_pending_irqs(struct vcpu *v)
> @@ -494,7 +511,7 @@ void vgic_clear_pending_irqs(struct vcpu *v)
>
> void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
> {
> - struct pending_irq *iter, *n = irq_to_pending(v, virq);
> + struct pending_irq *iter, *n = vgic_get_pending_irq(v->domain, v, virq);
> unsigned long flags;
> bool running;
>
> @@ -504,6 +521,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
> if ( test_bit(_VPF_down, &v->pause_flags) )
> {
> spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> + vgic_put_pending_irq(v->domain, n);
> return;
> }
>
> @@ -536,6 +554,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
> spin_unlock(&n->lock);
>
> out:
> + vgic_put_pending_irq(v->domain, n);
> spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> /* we have a new higher priority irq, inject it into the guest */
> running = v->is_running;
> @@ -550,12 +569,13 @@ out:
> void vgic_vcpu_inject_spi(struct domain *d, unsigned int virq)
> {
> struct vcpu *v;
> - struct pending_irq *p = irq_to_pending(d->vcpu[0], virq);
> + struct pending_irq *p = vgic_get_pending_irq(d, NULL, virq);
>
> /* the IRQ needs to be an SPI */
> ASSERT(virq >= 32 && virq <= vgic_num_irqs(d));
>
> v = vgic_get_target_vcpu(d, p);
> + vgic_put_pending_irq(d, p);
> vgic_vcpu_inject_irq(v, virq);
> }
>
> diff --git a/xen/include/asm-arm/event.h b/xen/include/asm-arm/event.h
> index 5330dfe..df672e2 100644
> --- a/xen/include/asm-arm/event.h
> +++ b/xen/include/asm-arm/event.h
> @@ -16,8 +16,7 @@ static inline int vcpu_event_delivery_is_enabled(struct vcpu *v)
>
> static inline int local_events_need_delivery_nomask(void)
> {
> - struct pending_irq *p = irq_to_pending(current,
> - current->domain->arch.evtchn_irq);
Limiting the scope of pending_irq should be a separate patch.
> + int ret = 0;
>
> /* XXX: if the first interrupt has already been delivered, we should
> * check whether any other interrupts with priority higher than the
> @@ -28,11 +27,20 @@ static inline int local_events_need_delivery_nomask(void)
> * case.
> */
> if ( gic_events_need_delivery() )
> - return 1;
> + {
> + ret = 1;
> + }
> + else
> + {
> + struct pending_irq *p;
>
> - if ( vcpu_info(current, evtchn_upcall_pending) &&
> - list_empty(&p->inflight) )
> - return 1;
> + p = vgic_get_pending_irq(current->domain, current,
> + current->domain->arch.evtchn_irq);
> + if ( vcpu_info(current, evtchn_upcall_pending) &&
> + list_empty(&p->inflight) )
> + ret = 1;
> + vgic_put_pending_irq(current->domain, p);
Looking at this code, I think there are a race condition. Because
nothing protect list_empty(&p->inflight) (this could be modified by
another physical vCPU at the same time).
Although, I don't know if this is really an issue. Stefano do you have
an opinion?
> + }
>
> return 0;
> }
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 186e6df..36e4de2 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -299,7 +299,12 @@ extern struct vcpu *vgic_get_target_vcpu(struct domain *d,
> 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 struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
> +extern struct pending_irq *vgic_get_pending_irq(struct domain *d,
> + struct vcpu *v,
> + unsigned int irq);
> +extern void vgic_put_pending_irq(struct domain *d, struct pending_irq *p);
> +extern void vgic_put_pending_irq_unlock(struct domain *d,
> + struct pending_irq *p);
> 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);
> extern struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq);
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 09/10] ARM: vGIC: introduce vgic_get/put_pending_irq
2017-05-04 17:36 ` Julien Grall
@ 2017-05-04 22:42 ` Stefano Stabellini
0 siblings, 0 replies; 31+ messages in thread
From: Stefano Stabellini @ 2017-05-04 22:42 UTC (permalink / raw)
To: Julien Grall; +Cc: Andre Przywara, Stefano Stabellini, xen-devel
On Thu, 4 May 2017, Julien Grall wrote:
> > @@ -28,11 +27,20 @@ static inline int
> > local_events_need_delivery_nomask(void)
> > * case.
> > */
> > if ( gic_events_need_delivery() )
> > - return 1;
> > + {
> > + ret = 1;
> > + }
> > + else
> > + {
> > + struct pending_irq *p;
> >
> > - if ( vcpu_info(current, evtchn_upcall_pending) &&
> > - list_empty(&p->inflight) )
> > - return 1;
> > + p = vgic_get_pending_irq(current->domain, current,
> > + current->domain->arch.evtchn_irq);
> > + if ( vcpu_info(current, evtchn_upcall_pending) &&
> > + list_empty(&p->inflight) )
> > + ret = 1;
> > + vgic_put_pending_irq(current->domain, p);
>
> Looking at this code, I think there are a race condition. Because nothing
> protect list_empty(&p->inflight) (this could be modified by another physical
> vCPU at the same time).
>
> Although, I don't know if this is really an issue. Stefano do you have an
> opinion?
I only gave a cursory look at the series, but I think you are right.
This access to inflight needs to be protected.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 04/10] ARM: vGIC: add struct pending_irq locking
2017-05-04 16:21 ` Julien Grall
@ 2017-05-05 9:02 ` Andre Przywara
2017-05-05 23:29 ` Stefano Stabellini
2017-05-06 6:41 ` Julien Grall
2017-05-05 23:42 ` Stefano Stabellini
1 sibling, 2 replies; 31+ messages in thread
From: Andre Przywara @ 2017-05-05 9:02 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini; +Cc: xen-devel
Hi,
On 04/05/17 17:21, Julien Grall wrote:
> Hi Andre,
>
> On 04/05/17 16:31, Andre Przywara wrote:
>> Introduce the proper locking sequence for the new pending_irq lock.
>> This takes the lock around multiple accesses to struct members,
>> also makes sure we observe the locking order (VGIC VCPU lock first,
>> then pending_irq lock).
>
> This locking order should be explained in the code. Likely in vgic.h.
>
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>> xen/arch/arm/gic.c | 26 ++++++++++++++++++++++++++
>> xen/arch/arm/vgic.c | 12 +++++++++++-
>> 2 files changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 67375a2..e175e9b 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -351,6 +351,7 @@ void gic_disable_cpu(void)
>> static inline void gic_set_lr(int lr, struct pending_irq *p,
>> unsigned int state)
>> {
>> + ASSERT(spin_is_locked(&p->lock));
>> ASSERT(!local_irq_is_enabled());
>>
>> gic_hw_ops->update_lr(lr, p, state);
>> @@ -413,6 +414,7 @@ void gic_raise_guest_irq(struct vcpu *v, struct
>> pending_irq *p)
>> unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
>>
>> ASSERT(spin_is_locked(&v->arch.vgic.lock));
>> + ASSERT(spin_is_locked(&p->lock));
>>
>> if ( v == current && list_empty(&v->arch.vgic.lr_pending) )
>> {
>> @@ -439,6 +441,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>> gic_hw_ops->read_lr(i, &lr_val);
>> irq = lr_val.virq;
>> p = irq_to_pending(v, irq);
>> + spin_lock(&p->lock);
>> if ( lr_val.state & GICH_LR_ACTIVE )
>> {
>> set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
>> @@ -495,6 +498,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>> }
>> }
>> }
>> + spin_unlock(&p->lock);
>> }
>>
>> void gic_clear_lrs(struct vcpu *v)
>> @@ -545,14 +549,30 @@ static void gic_restore_pending_irqs(struct vcpu
>> *v)
>> /* No more free LRs: find a lower priority irq to evict */
>> list_for_each_entry_reverse( p_r, inflight_r, inflight )
>> {
>> + if ( p_r->irq < p->irq )
>> + {
>> + spin_lock(&p_r->lock);
>> + spin_lock(&p->lock);
>> + }
>> + else
>> + {
>> + spin_lock(&p->lock);
>> + spin_lock(&p_r->lock);
>> + }
>
> Please explain in the commit message and the code why this locking order.
>
>> if ( p_r->priority == p->priority )
>> + {
>> + spin_unlock(&p->lock);
>> + spin_unlock(&p_r->lock);
>> goto out;
>> + }
>> if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status) &&
>> !test_bit(GIC_IRQ_GUEST_ACTIVE, &p_r->status) )
>> goto found;
>> }
>> /* We didn't find a victim this time, and we won't next
>> * time, so quit */
>> + spin_unlock(&p->lock);
>> + spin_unlock(&p_r->lock);
>> goto out;
>>
>> found:
>> @@ -562,12 +582,18 @@ found:
>> clear_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status);
>> gic_add_to_lr_pending(v, p_r);
>> inflight_r = &p_r->inflight;
>> +
>> + spin_unlock(&p_r->lock);
>> }
>> + else
>> + spin_lock(&p->lock);
>>
>> gic_set_lr(lr, p, GICH_LR_PENDING);
>> list_del_init(&p->lr_queue);
>> set_bit(lr, &this_cpu(lr_mask));
>>
>> + spin_unlock(&p->lock);
>> +
>> /* We can only evict nr_lrs entries */
>> lrs--;
>> if ( lrs == 0 )
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index f4ae454..44363bb 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -356,11 +356,16 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t
>> r, int n)
>> while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
>> irq = i + (32 * n);
>> v_target = vgic_get_target_vcpu(v, irq);
>> +
>> + spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
>> p = irq_to_pending(v_target, irq);
>> + spin_lock(&p->lock);
>> +
>> set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
>> - spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
>
> IHMO, this should belong to a separate patch as not strictly relate to
> this one.
I don't think it makes much sense to split this, as this change is
motivated by the introduction of the pending_irq lock, and we have to
move the VGIC VCPU lock due to the locking order.
>
>> +
>
> Spurious change.
Well, that helps to structure the code.
>> if ( !list_empty(&p->inflight) &&
>> !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
>> gic_raise_guest_irq(v_target, p);
>> + spin_unlock(&p->lock);
>> spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
>> if ( p->desc != NULL )
>> {
>> @@ -482,10 +487,12 @@ void vgic_vcpu_inject_irq(struct vcpu *v,
>> unsigned int virq)
>> return;
>> }
>>
>> + spin_lock(&n->lock);
>> set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
>>
>> if ( !list_empty(&n->inflight) )
>> {
>> + spin_unlock(&n->lock);
>> gic_raise_inflight_irq(v, n);
>
> Any reason to not code gic_raise_inflight_irq with the lock? This would
> also simplify quite a lot the function and avoid unlock in pretty much
> all the exit path.
gic_raise_inflight_irq() calls gic_update_one_lr(), which works out the
pending_irq from the LR number and then takes the lock.
Yes, there are other ways of solving this:
- remove gic_raise_inflight_irq() at all
- rework gic_update_one_lr() to take a (locked) pending_irq already
Both approaches have other issues that pop up, I think this solution
here is the least hideous and least intrusive.
Frankly I believe that removing gic_raise_inflight_irq() altogether is
the best solution, but this requires more rework (which Stefano hinted
at not liking too much) and I wanted to keep this series as simple as
possible.
Cheers,
Andre.
>> goto out;
>> }
>> @@ -501,10 +508,13 @@ void vgic_vcpu_inject_irq(struct vcpu *v,
>> unsigned int virq)
>> if ( iter->priority > priority )
>> {
>> list_add_tail(&n->inflight, &iter->inflight);
>> + spin_unlock(&n->lock);
>> goto out;
>> }
>> }
>> list_add_tail(&n->inflight, &v->arch.vgic.inflight_irqs);
>> + spin_unlock(&n->lock);
>> +
>> out:
>> spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>> /* we have a new higher priority irq, inject it into the guest */
>>
>
> Cheers,
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 04/10] ARM: vGIC: add struct pending_irq locking
2017-05-04 16:54 ` Julien Grall
@ 2017-05-05 23:26 ` Stefano Stabellini
0 siblings, 0 replies; 31+ messages in thread
From: Stefano Stabellini @ 2017-05-05 23:26 UTC (permalink / raw)
To: Julien Grall; +Cc: Andre Przywara, Stefano Stabellini, xen-devel
On Thu, 4 May 2017, Julien Grall wrote:
> On 04/05/17 16:31, Andre Przywara wrote:
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index f4ae454..44363bb 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -356,11 +356,16 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int
> > n)
> > while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
> > irq = i + (32 * n);
> > v_target = vgic_get_target_vcpu(v, irq);
> > +
> > + spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
> > p = irq_to_pending(v_target, irq);
> > + spin_lock(&p->lock);
> > +
> > set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> > - 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, p);
> > + spin_unlock(&p->lock);
>
> Why does the lock not cover p->desc below?
Indeed. The main problem with this patch is that it doesn't say what
this lock is supposed to cover. It is OK for the lock not to cover
everything pending_irq related as long as it is clear.
As it stands, it is not clear.
For example, why the lock is not added to following functions?
- gic_route_irq_to_guest
- gic_remove_irq_from_guest
- gic_remove_from_queues
- gic_raise_inflight_irq
- vgic_migrate_irq
- arch_move_irqs
- vgic_disable_irqs
I came up with this list by grepping for irq_to_pending and listing the
function where fields of the pending_irq struct are accessed.
> > spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
> > if ( p->desc != NULL )
> > {
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 04/10] ARM: vGIC: add struct pending_irq locking
2017-05-05 9:02 ` Andre Przywara
@ 2017-05-05 23:29 ` Stefano Stabellini
2017-05-06 6:41 ` Julien Grall
1 sibling, 0 replies; 31+ messages in thread
From: Stefano Stabellini @ 2017-05-05 23:29 UTC (permalink / raw)
To: Andre Przywara; +Cc: xen-devel, Julien Grall, Stefano Stabellini
On Fri, 5 May 2017, Andre Przywara wrote:
> Hi,
>
> On 04/05/17 17:21, Julien Grall wrote:
> > Hi Andre,
> >
> > On 04/05/17 16:31, Andre Przywara wrote:
> >> Introduce the proper locking sequence for the new pending_irq lock.
> >> This takes the lock around multiple accesses to struct members,
> >> also makes sure we observe the locking order (VGIC VCPU lock first,
> >> then pending_irq lock).
> >
> > This locking order should be explained in the code. Likely in vgic.h.
> >
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >> ---
> >> xen/arch/arm/gic.c | 26 ++++++++++++++++++++++++++
> >> xen/arch/arm/vgic.c | 12 +++++++++++-
> >> 2 files changed, 37 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> >> index 67375a2..e175e9b 100644
> >> --- a/xen/arch/arm/gic.c
> >> +++ b/xen/arch/arm/gic.c
> >> @@ -351,6 +351,7 @@ void gic_disable_cpu(void)
> >> static inline void gic_set_lr(int lr, struct pending_irq *p,
> >> unsigned int state)
> >> {
> >> + ASSERT(spin_is_locked(&p->lock));
> >> ASSERT(!local_irq_is_enabled());
> >>
> >> gic_hw_ops->update_lr(lr, p, state);
> >> @@ -413,6 +414,7 @@ void gic_raise_guest_irq(struct vcpu *v, struct
> >> pending_irq *p)
> >> unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
> >>
> >> ASSERT(spin_is_locked(&v->arch.vgic.lock));
> >> + ASSERT(spin_is_locked(&p->lock));
> >>
> >> if ( v == current && list_empty(&v->arch.vgic.lr_pending) )
> >> {
> >> @@ -439,6 +441,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
> >> gic_hw_ops->read_lr(i, &lr_val);
> >> irq = lr_val.virq;
> >> p = irq_to_pending(v, irq);
> >> + spin_lock(&p->lock);
> >> if ( lr_val.state & GICH_LR_ACTIVE )
> >> {
> >> set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
> >> @@ -495,6 +498,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
> >> }
> >> }
> >> }
> >> + spin_unlock(&p->lock);
> >> }
> >>
> >> void gic_clear_lrs(struct vcpu *v)
> >> @@ -545,14 +549,30 @@ static void gic_restore_pending_irqs(struct vcpu
> >> *v)
> >> /* No more free LRs: find a lower priority irq to evict */
> >> list_for_each_entry_reverse( p_r, inflight_r, inflight )
> >> {
> >> + if ( p_r->irq < p->irq )
> >> + {
> >> + spin_lock(&p_r->lock);
> >> + spin_lock(&p->lock);
> >> + }
> >> + else
> >> + {
> >> + spin_lock(&p->lock);
> >> + spin_lock(&p_r->lock);
> >> + }
> >
> > Please explain in the commit message and the code why this locking order.
> >
> >> if ( p_r->priority == p->priority )
> >> + {
> >> + spin_unlock(&p->lock);
> >> + spin_unlock(&p_r->lock);
> >> goto out;
> >> + }
> >> if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status) &&
> >> !test_bit(GIC_IRQ_GUEST_ACTIVE, &p_r->status) )
> >> goto found;
> >> }
> >> /* We didn't find a victim this time, and we won't next
> >> * time, so quit */
> >> + spin_unlock(&p->lock);
> >> + spin_unlock(&p_r->lock);
> >> goto out;
> >>
> >> found:
> >> @@ -562,12 +582,18 @@ found:
> >> clear_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status);
> >> gic_add_to_lr_pending(v, p_r);
> >> inflight_r = &p_r->inflight;
> >> +
> >> + spin_unlock(&p_r->lock);
> >> }
> >> + else
> >> + spin_lock(&p->lock);
> >>
> >> gic_set_lr(lr, p, GICH_LR_PENDING);
> >> list_del_init(&p->lr_queue);
> >> set_bit(lr, &this_cpu(lr_mask));
> >>
> >> + spin_unlock(&p->lock);
> >> +
> >> /* We can only evict nr_lrs entries */
> >> lrs--;
> >> if ( lrs == 0 )
> >> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> >> index f4ae454..44363bb 100644
> >> --- a/xen/arch/arm/vgic.c
> >> +++ b/xen/arch/arm/vgic.c
> >> @@ -356,11 +356,16 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t
> >> r, int n)
> >> while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
> >> irq = i + (32 * n);
> >> v_target = vgic_get_target_vcpu(v, irq);
> >> +
> >> + spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
> >> p = irq_to_pending(v_target, irq);
> >> + spin_lock(&p->lock);
> >> +
> >> set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> >> - spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
> >
> > IHMO, this should belong to a separate patch as not strictly relate to
> > this one.
>
> I don't think it makes much sense to split this, as this change is
> motivated by the introduction of the pending_irq lock, and we have to
> move the VGIC VCPU lock due to the locking order.
>
> >
> >> +
> >
> > Spurious change.
>
> Well, that helps to structure the code.
>
> >> if ( !list_empty(&p->inflight) &&
> >> !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
> >> gic_raise_guest_irq(v_target, p);
> >> + spin_unlock(&p->lock);
> >> spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
> >> if ( p->desc != NULL )
> >> {
> >> @@ -482,10 +487,12 @@ void vgic_vcpu_inject_irq(struct vcpu *v,
> >> unsigned int virq)
> >> return;
> >> }
> >>
> >> + spin_lock(&n->lock);
> >> set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
> >>
> >> if ( !list_empty(&n->inflight) )
> >> {
> >> + spin_unlock(&n->lock);
> >> gic_raise_inflight_irq(v, n);
> >
> > Any reason to not code gic_raise_inflight_irq with the lock? This would
> > also simplify quite a lot the function and avoid unlock in pretty much
> > all the exit path.
>
> gic_raise_inflight_irq() calls gic_update_one_lr(), which works out the
> pending_irq from the LR number and then takes the lock.
> Yes, there are other ways of solving this:
> - remove gic_raise_inflight_irq() at all
> - rework gic_update_one_lr() to take a (locked) pending_irq already
>
> Both approaches have other issues that pop up, I think this solution
> here is the least hideous and least intrusive.
> Frankly I believe that removing gic_raise_inflight_irq() altogether is
> the best solution, but this requires more rework (which Stefano hinted
> at not liking too much) and I wanted to keep this series as simple as
> possible.
Just to be clear: I like any rework/refactoring you can come up with,
*after* ITS :-)
> >> goto out;
> >> }
> >> @@ -501,10 +508,13 @@ void vgic_vcpu_inject_irq(struct vcpu *v,
> >> unsigned int virq)
> >> if ( iter->priority > priority )
> >> {
> >> list_add_tail(&n->inflight, &iter->inflight);
> >> + spin_unlock(&n->lock);
> >> goto out;
> >> }
> >> }
> >> list_add_tail(&n->inflight, &v->arch.vgic.inflight_irqs);
> >> + spin_unlock(&n->lock);
> >> +
> >> out:
> >> spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> >> /* we have a new higher priority irq, inject it into the guest */
> >>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 04/10] ARM: vGIC: add struct pending_irq locking
2017-05-04 16:21 ` Julien Grall
2017-05-05 9:02 ` Andre Przywara
@ 2017-05-05 23:42 ` Stefano Stabellini
1 sibling, 0 replies; 31+ messages in thread
From: Stefano Stabellini @ 2017-05-05 23:42 UTC (permalink / raw)
To: Julien Grall; +Cc: Andre Przywara, Stefano Stabellini, xen-devel
On Thu, 4 May 2017, Julien Grall wrote:
> > @@ -545,14 +549,30 @@ static void gic_restore_pending_irqs(struct vcpu *v)
> > /* No more free LRs: find a lower priority irq to evict */
> > list_for_each_entry_reverse( p_r, inflight_r, inflight )
> > {
> > + if ( p_r->irq < p->irq )
> > + {
> > + spin_lock(&p_r->lock);
> > + spin_lock(&p->lock);
> > + }
> > + else
> > + {
> > + spin_lock(&p->lock);
> > + spin_lock(&p_r->lock);
> > + }
>
> Please explain in the commit message and the code why this locking order.
Yes, please add a couple of lines of comment to the code too. However,
the code looks correct.
> > if ( p_r->priority == p->priority )
> > + {
> > + spin_unlock(&p->lock);
> > + spin_unlock(&p_r->lock);
> > goto out;
> > + }
> > if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status) &&
> > !test_bit(GIC_IRQ_GUEST_ACTIVE, &p_r->status) )
> > goto found;
> > }
> > /* We didn't find a victim this time, and we won't next
> > * time, so quit */
> > + spin_unlock(&p->lock);
> > + spin_unlock(&p_r->lock);
> > goto out;
> >
> > found:
> > @@ -562,12 +582,18 @@ found:
> > clear_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status);
> > gic_add_to_lr_pending(v, p_r);
> > inflight_r = &p_r->inflight;
> > +
> > + spin_unlock(&p_r->lock);
> > }
> > + else
> > + spin_lock(&p->lock);
> >
> > gic_set_lr(lr, p, GICH_LR_PENDING);
> > list_del_init(&p->lr_queue);
> > set_bit(lr, &this_cpu(lr_mask));
> >
> > + spin_unlock(&p->lock);
> > +
> > /* We can only evict nr_lrs entries */
> > lrs--;
> > if ( lrs == 0 )
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 04/10] ARM: vGIC: add struct pending_irq locking
2017-05-05 9:02 ` Andre Przywara
2017-05-05 23:29 ` Stefano Stabellini
@ 2017-05-06 6:41 ` Julien Grall
1 sibling, 0 replies; 31+ messages in thread
From: Julien Grall @ 2017-05-06 6:41 UTC (permalink / raw)
To: Andre Przywara, Stefano Stabellini; +Cc: xen-devel
Hi Andre,
On 05/05/2017 10:02 AM, Andre Przywara wrote:
> Hi,
>
> On 04/05/17 17:21, Julien Grall wrote:
>> Hi Andre,
>>
>> On 04/05/17 16:31, Andre Przywara wrote:
>>> Introduce the proper locking sequence for the new pending_irq lock.
>>> This takes the lock around multiple accesses to struct members,
>>> also makes sure we observe the locking order (VGIC VCPU lock first,
>>> then pending_irq lock).
>>
>> This locking order should be explained in the code. Likely in vgic.h.
>>
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>> xen/arch/arm/gic.c | 26 ++++++++++++++++++++++++++
>>> xen/arch/arm/vgic.c | 12 +++++++++++-
>>> 2 files changed, 37 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>> index 67375a2..e175e9b 100644
>>> --- a/xen/arch/arm/gic.c
>>> +++ b/xen/arch/arm/gic.c
>>> @@ -351,6 +351,7 @@ void gic_disable_cpu(void)
>>> static inline void gic_set_lr(int lr, struct pending_irq *p,
>>> unsigned int state)
>>> {
>>> + ASSERT(spin_is_locked(&p->lock));
>>> ASSERT(!local_irq_is_enabled());
>>>
>>> gic_hw_ops->update_lr(lr, p, state);
>>> @@ -413,6 +414,7 @@ void gic_raise_guest_irq(struct vcpu *v, struct
>>> pending_irq *p)
>>> unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
>>>
>>> ASSERT(spin_is_locked(&v->arch.vgic.lock));
>>> + ASSERT(spin_is_locked(&p->lock));
>>>
>>> if ( v == current && list_empty(&v->arch.vgic.lr_pending) )
>>> {
>>> @@ -439,6 +441,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>>> gic_hw_ops->read_lr(i, &lr_val);
>>> irq = lr_val.virq;
>>> p = irq_to_pending(v, irq);
>>> + spin_lock(&p->lock);
>>> if ( lr_val.state & GICH_LR_ACTIVE )
>>> {
>>> set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
>>> @@ -495,6 +498,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>>> }
>>> }
>>> }
>>> + spin_unlock(&p->lock);
>>> }
>>>
>>> void gic_clear_lrs(struct vcpu *v)
>>> @@ -545,14 +549,30 @@ static void gic_restore_pending_irqs(struct vcpu
>>> *v)
>>> /* No more free LRs: find a lower priority irq to evict */
>>> list_for_each_entry_reverse( p_r, inflight_r, inflight )
>>> {
>>> + if ( p_r->irq < p->irq )
>>> + {
>>> + spin_lock(&p_r->lock);
>>> + spin_lock(&p->lock);
>>> + }
>>> + else
>>> + {
>>> + spin_lock(&p->lock);
>>> + spin_lock(&p_r->lock);
>>> + }
>>
>> Please explain in the commit message and the code why this locking order.
>>
>>> if ( p_r->priority == p->priority )
>>> + {
>>> + spin_unlock(&p->lock);
>>> + spin_unlock(&p_r->lock);
>>> goto out;
>>> + }
>>> if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status) &&
>>> !test_bit(GIC_IRQ_GUEST_ACTIVE, &p_r->status) )
>>> goto found;
>>> }
>>> /* We didn't find a victim this time, and we won't next
>>> * time, so quit */
>>> + spin_unlock(&p->lock);
>>> + spin_unlock(&p_r->lock);
>>> goto out;
>>>
>>> found:
>>> @@ -562,12 +582,18 @@ found:
>>> clear_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status);
>>> gic_add_to_lr_pending(v, p_r);
>>> inflight_r = &p_r->inflight;
>>> +
>>> + spin_unlock(&p_r->lock);
>>> }
>>> + else
>>> + spin_lock(&p->lock);
>>>
>>> gic_set_lr(lr, p, GICH_LR_PENDING);
>>> list_del_init(&p->lr_queue);
>>> set_bit(lr, &this_cpu(lr_mask));
>>>
>>> + spin_unlock(&p->lock);
>>> +
>>> /* We can only evict nr_lrs entries */
>>> lrs--;
>>> if ( lrs == 0 )
>>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>>> index f4ae454..44363bb 100644
>>> --- a/xen/arch/arm/vgic.c
>>> +++ b/xen/arch/arm/vgic.c
>>> @@ -356,11 +356,16 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t
>>> r, int n)
>>> while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
>>> irq = i + (32 * n);
>>> v_target = vgic_get_target_vcpu(v, irq);
>>> +
>>> + spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
>>> p = irq_to_pending(v_target, irq);
>>> + spin_lock(&p->lock);
>>> +
>>> set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
>>> - spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
>>
>> IHMO, this should belong to a separate patch as not strictly relate to
>> this one.
>
> I don't think it makes much sense to split this, as this change is
> motivated by the introduction of the pending_irq lock, and we have to
> move the VGIC VCPU lock due to the locking order.
My point here is this change does not require to be specifically in this
patch. And moving it in a separate patch would allow you to justify this
change and simplify the patch.
>
>>
>>> +
>>
>> Spurious change.
>
> Well, that helps to structure the code.
I call that code clean-up and should be moved in separate patch. A
general rule to make the patches as small as possible. This makes easier
to review and justify changes.
>
>>> if ( !list_empty(&p->inflight) &&
>>> !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
>>> gic_raise_guest_irq(v_target, p);
>>> + spin_unlock(&p->lock);
>>> spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
>>> if ( p->desc != NULL )
>>> {
>>> @@ -482,10 +487,12 @@ void vgic_vcpu_inject_irq(struct vcpu *v,
>>> unsigned int virq)
>>> return;
>>> }
>>>
>>> + spin_lock(&n->lock);
>>> set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
>>>
>>> if ( !list_empty(&n->inflight) )
>>> {
>>> + spin_unlock(&n->lock);
>>> gic_raise_inflight_irq(v, n);
>>
>> Any reason to not code gic_raise_inflight_irq with the lock? This would
>> also simplify quite a lot the function and avoid unlock in pretty much
>> all the exit path.
>
> gic_raise_inflight_irq() calls gic_update_one_lr(), which works out the
> pending_irq from the LR number and then takes the lock.
> Yes, there are other ways of solving this:
> - remove gic_raise_inflight_irq() at all
> - rework gic_update_one_lr() to take a (locked) pending_irq already
>
> Both approaches have other issues that pop up, I think this solution
> here is the least hideous and least intrusive.
> Frankly I believe that removing gic_raise_inflight_irq() altogether is
> the best solution, but this requires more rework (which Stefano hinted
> at not liking too much) and I wanted to keep this series as simple as
> possible.
The problem I can see with this patch series is it is not going far
enough. Without the rest of the vgic rework, it is hard to say whether
the locking order or placement of vgic_{get,put}_* are valid.
For instance, as we discussed yesterday face to face. In this series you
are suggesting the locking order:
1) vgic vcpu lock
2) pending_irq lock
Because of this locking order, you may need in some places to take the
pending_irq lock temporarily, drop it then take the locking in the
correct order.
I do believe that we can limit the use of vgic vcpu lock (it mostly
protect the list in pending_irq) and we could use the locking order:
1) pending_irq lock
2) vgic vcpu lock
This would also simplify the locking afterwards. But the correct
ordering can only be decided with a full vGIC rework in mind.
What I want to avoid is getting merged a lock ordering today, but in a
couple of weeks we have a new series with a different locking ordering.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 01/10] ARM: vGIC: remove rank lock from IRQ routing functions
2017-05-04 15:53 ` Julien Grall
@ 2017-05-08 9:15 ` Andre Przywara
2017-05-08 14:26 ` Julien Grall
0 siblings, 1 reply; 31+ messages in thread
From: Andre Przywara @ 2017-05-08 9:15 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini; +Cc: xen-devel
Hi,
On 04/05/17 16:53, Julien Grall wrote:
> Hi Andre,
>
> On 04/05/17 16:31, Andre Przywara wrote:
>> gic_route_irq_to_guest() and gic_remove_irq_from_guest() take the rank
>> lock, however never actually access the rank structure.
>> Avoid taking the lock in those two functions and remove some more
>> unneeded code on the way.
>
> The rank is here to protect p->desc when checking that the virtual
> interrupt was not yet routed to another physical interrupt.
Really? To me that sounds quite surprising.
My understanding is that the VGIC VCPU lock protected the pending_irq
(and thus the desc pointer?) so far, and the desc itself has its own lock.
According to the comment in the struct irq_rank declaration the lock
protects the members of this struct only.
Looking briefly at users of pending_irq->desc (for instance
gicv[23]_update_lr() or gic_update_one_lr()) I can't see any hint that
they care about the lock.
So should that be fixed or at least documented?
> Without this locking, you can have two concurrent call of
> gic_route_irq_to_guest that will update the same virtual interrupt but
> with different physical interrupts.
>
> You would have to replace the rank lock by the per-pending_irq lock to
> keep the code safe.
That indeed sounds reasonable.
Cheers,
Andre.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>> xen/arch/arm/gic.c | 28 ++++------------------------
>> 1 file changed, 4 insertions(+), 24 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index da19130..c734f66 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -136,25 +136,19 @@ void gic_route_irq_to_xen(struct irq_desc *desc,
>> unsigned int priority)
>> int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
>> struct irq_desc *desc, unsigned int priority)
>> {
>> - unsigned long flags;
>> /* Use vcpu0 to retrieve the pending_irq struct. Given that we only
>> * route SPIs to guests, it doesn't make any difference. */
>> - struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
>> - struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
>> - struct pending_irq *p = irq_to_pending(v_target, virq);
>> - int res = -EBUSY;
>> + struct pending_irq *p = irq_to_pending(d->vcpu[0], virq);
>>
>> ASSERT(spin_is_locked(&desc->lock));
>> /* Caller has already checked that the IRQ is an SPI */
>> ASSERT(virq >= 32);
>> ASSERT(virq < vgic_num_irqs(d));
>>
>> - vgic_lock_rank(v_target, rank, flags);
>> -
>> if ( p->desc ||
>> /* The VIRQ should not be already enabled by the guest */
>> test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
>> - goto out;
>> + return -EBUSY;
>>
>> desc->handler = gic_hw_ops->gic_guest_irq_type;
>> set_bit(_IRQ_GUEST, &desc->status);
>> @@ -164,29 +158,20 @@ int gic_route_irq_to_guest(struct domain *d,
>> unsigned int virq,
>> gic_set_irq_priority(desc, priority);
>>
>> p->desc = desc;
>> - res = 0;
>>
>> -out:
>> - vgic_unlock_rank(v_target, rank, flags);
>> -
>> - return res;
>> + return 0;
>> }
>>
>> /* This function only works with SPIs for now */
>> int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>> struct irq_desc *desc)
>> {
>> - struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
>> - struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
>> - struct pending_irq *p = irq_to_pending(v_target, virq);
>> - unsigned long flags;
>> + struct pending_irq *p = irq_to_pending(d->vcpu[0], virq);
>>
>> ASSERT(spin_is_locked(&desc->lock));
>> ASSERT(test_bit(_IRQ_GUEST, &desc->status));
>> ASSERT(p->desc == desc);
>>
>> - vgic_lock_rank(v_target, rank, flags);
>> -
>> if ( d->is_dying )
>> {
>> desc->handler->shutdown(desc);
>> @@ -204,10 +189,7 @@ int gic_remove_irq_from_guest(struct domain *d,
>> unsigned int virq,
>> */
>> if ( test_bit(_IRQ_INPROGRESS, &desc->status) ||
>> !test_bit(_IRQ_DISABLED, &desc->status) )
>> - {
>> - vgic_unlock_rank(v_target, rank, flags);
>> return -EBUSY;
>> - }
>> }
>>
>> clear_bit(_IRQ_GUEST, &desc->status);
>> @@ -215,8 +197,6 @@ int gic_remove_irq_from_guest(struct domain *d,
>> unsigned int virq,
>>
>> p->desc = NULL;
>>
>> - vgic_unlock_rank(v_target, rank, flags);
>> -
>> return 0;
>> }
>>
>>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 01/10] ARM: vGIC: remove rank lock from IRQ routing functions
2017-05-08 9:15 ` Andre Przywara
@ 2017-05-08 14:26 ` Julien Grall
2017-05-08 21:53 ` Stefano Stabellini
0 siblings, 1 reply; 31+ messages in thread
From: Julien Grall @ 2017-05-08 14:26 UTC (permalink / raw)
To: Andre Przywara, Stefano Stabellini; +Cc: xen-devel
Hi Andre,
On 08/05/17 10:15, Andre Przywara wrote:
> On 04/05/17 16:53, Julien Grall wrote:
>> Hi Andre,
>>
>> On 04/05/17 16:31, Andre Przywara wrote:
>>> gic_route_irq_to_guest() and gic_remove_irq_from_guest() take the rank
>>> lock, however never actually access the rank structure.
>>> Avoid taking the lock in those two functions and remove some more
>>> unneeded code on the way.
>>
>> The rank is here to protect p->desc when checking that the virtual
>> interrupt was not yet routed to another physical interrupt.
>
> Really? To me that sounds quite surprising.
> My understanding is that the VGIC VCPU lock protected the pending_irq
> (and thus the desc pointer?) so far, and the desc itself has its own lock.
> According to the comment in the struct irq_rank declaration the lock
> protects the members of this struct only.
>
> Looking briefly at users of pending_irq->desc (for instance
> gicv[23]_update_lr() or gic_update_one_lr()) I can't see any hint that
> they care about the lock.
>
> So should that be fixed or at least documented?
My understanding is this rank lock is preventing race between two
updates of p->desc. This can happen if gic_route_irq_to_guest(...) is
called concurrently with the same vIRQ but different pIRQ.
If you drop this lock, nothing will protect that anymore.
>
>> Without this locking, you can have two concurrent call of
>> gic_route_irq_to_guest that will update the same virtual interrupt but
>> with different physical interrupts.
>>
>> You would have to replace the rank lock by the per-pending_irq lock to
>> keep the code safe.
>
> That indeed sounds reasonable.
As you mentioned IRL, the current code may lead to a deadlock due to
locking order.
Indeed routing an IRQ (route_irq_to_guest) will take:
1) desc lock (in route_irq_to_guest)
2) rank lock (in gic_route_irq_to_guest)
Whilst the MMIO emulation of ISENABLER_* will take:
1) rank lock
2) desc lock (in vgic_enable_irqs)
Using the per-pending_irq lock will not solve the deadlock. I though a
bit more to the code. I believe the routing of SPIs/PPIs after domain
creation time is a call to mistake and locking nightmare. Similarly an
interrupt should stay routed for the duration of the domain life.
So I would forbid IRQ routing after domain creation (see
d->creation_finished) and remove IRQ whilst routing (I think with
d->is_dying). This would have an race between the routing and the rest
of the vGIC code.
However, this would not prevent the routing function to race against
itself. For that I would take the vgic domain lock, it is fine because
routing is not something we expect to happen often.
Any opinions?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 01/10] ARM: vGIC: remove rank lock from IRQ routing functions
2017-05-08 14:26 ` Julien Grall
@ 2017-05-08 21:53 ` Stefano Stabellini
2017-05-08 22:13 ` Julien Grall
0 siblings, 1 reply; 31+ messages in thread
From: Stefano Stabellini @ 2017-05-08 21:53 UTC (permalink / raw)
To: Julien Grall; +Cc: Andre Przywara, Stefano Stabellini, xen-devel
On Mon, 8 May 2017, Julien Grall wrote:
> Hi Andre,
>
> On 08/05/17 10:15, Andre Przywara wrote:
> > On 04/05/17 16:53, Julien Grall wrote:
> > > Hi Andre,
> > >
> > > On 04/05/17 16:31, Andre Przywara wrote:
> > > > gic_route_irq_to_guest() and gic_remove_irq_from_guest() take the rank
> > > > lock, however never actually access the rank structure.
> > > > Avoid taking the lock in those two functions and remove some more
> > > > unneeded code on the way.
> > >
> > > The rank is here to protect p->desc when checking that the virtual
> > > interrupt was not yet routed to another physical interrupt.
> >
> > Really? To me that sounds quite surprising.
> > My understanding is that the VGIC VCPU lock protected the pending_irq
> > (and thus the desc pointer?) so far, and the desc itself has its own lock.
> > According to the comment in the struct irq_rank declaration the lock
> > protects the members of this struct only.
> >
> > Looking briefly at users of pending_irq->desc (for instance
> > gicv[23]_update_lr() or gic_update_one_lr()) I can't see any hint that
> > they care about the lock.
> >
> > So should that be fixed or at least documented?
>
> My understanding is this rank lock is preventing race between two updates of
> p->desc. This can happen if gic_route_irq_to_guest(...) is called concurrently
> with the same vIRQ but different pIRQ.
>
> If you drop this lock, nothing will protect that anymore.
The desc->lock in route_irq_to_guest is protecting against concurrent
changes to the same physical irq.
The vgic_lock_rank in gic_route_irq_to_guest is protecting against
concurrent changes to the same virtual irq.
In other words, the current code does:
1) lock physical irq
2) lock virtual irq
3) establish mapping virtual irq - physical irq
Andre, sorry for not seeing this earlier.
> > > Without this locking, you can have two concurrent call of
> > > gic_route_irq_to_guest that will update the same virtual interrupt but
> > > with different physical interrupts.
> > >
> > > You would have to replace the rank lock by the per-pending_irq lock to
> > > keep the code safe.
> >
> > That indeed sounds reasonable.
>
> As you mentioned IRL, the current code may lead to a deadlock due to locking
> order.
>
> Indeed routing an IRQ (route_irq_to_guest) will take:
> 1) desc lock (in route_irq_to_guest)
> 2) rank lock (in gic_route_irq_to_guest)
>
> Whilst the MMIO emulation of ISENABLER_* will take:
> 1) rank lock
> 2) desc lock (in vgic_enable_irqs)
Yes, you are right, but I don't think that is an issue because
route_irq_to_guest is expected to be called before the domain is
started, which is consistent with what you wrote below that routing
should be done *before* running the VM.
To be clear: I am not arguing for keeping the code as-is, just trying to
understand it.
> Using the per-pending_irq lock will not solve the deadlock. I though a bit
> more to the code. I believe the routing of SPIs/PPIs after domain creation
> time is a call to mistake and locking nightmare. Similarly an interrupt should
> stay routed for the duration of the domain life.
It looks like current routing code was already written with that assumption.
> So I would forbid IRQ routing after domain creation (see d->creation_finished)
> and remove IRQ whilst routing (I think with d->is_dying). This would have an
> race between the routing and the rest of the vGIC code.
I am fine with that.
> However, this would not prevent the routing function to race against itself.
> For that I would take the vgic domain lock, it is fine because routing is not
> something we expect to happen often.
>
> Any opinions?
The changes done by gic_route_irq_to_guest are specific to one virtual
irq and one physical irq. In fact, they establish the mapping between
the two.
Given that concurrent changes to a physical irq are protected by the
desc->lock in route_irq_to_guest, why do you think that taking the new
pending_irq lock in gic_route_irq_to_guest would not be sufficient?
(gic_route_irq_to_guest is always called by route_irq_to_guest.)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 01/10] ARM: vGIC: remove rank lock from IRQ routing functions
2017-05-08 21:53 ` Stefano Stabellini
@ 2017-05-08 22:13 ` Julien Grall
2017-05-09 0:47 ` Stefano Stabellini
0 siblings, 1 reply; 31+ messages in thread
From: Julien Grall @ 2017-05-08 22:13 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: Andre Przywara, nd, xen-devel
Hi Stefano,
On 08/05/2017 22:53, Stefano Stabellini wrote:
> On Mon, 8 May 2017, Julien Grall wrote:
>> Hi Andre,
>>
>> On 08/05/17 10:15, Andre Przywara wrote:
>>> On 04/05/17 16:53, Julien Grall wrote:
>>>> Hi Andre,
>>>>
>>>> On 04/05/17 16:31, Andre Przywara wrote:
>>>>> gic_route_irq_to_guest() and gic_remove_irq_from_guest() take the rank
>>>>> lock, however never actually access the rank structure.
>>>>> Avoid taking the lock in those two functions and remove some more
>>>>> unneeded code on the way.
>>>>
>>>> The rank is here to protect p->desc when checking that the virtual
>>>> interrupt was not yet routed to another physical interrupt.
>>>
>>> Really? To me that sounds quite surprising.
>>> My understanding is that the VGIC VCPU lock protected the pending_irq
>>> (and thus the desc pointer?) so far, and the desc itself has its own lock.
>>> According to the comment in the struct irq_rank declaration the lock
>>> protects the members of this struct only.
>>>
>>> Looking briefly at users of pending_irq->desc (for instance
>>> gicv[23]_update_lr() or gic_update_one_lr()) I can't see any hint that
>>> they care about the lock.
>>>
>>> So should that be fixed or at least documented?
>>
>> My understanding is this rank lock is preventing race between two updates of
>> p->desc. This can happen if gic_route_irq_to_guest(...) is called concurrently
>> with the same vIRQ but different pIRQ.
>>
>> If you drop this lock, nothing will protect that anymore.
>
> The desc->lock in route_irq_to_guest is protecting against concurrent
> changes to the same physical irq.
>
> The vgic_lock_rank in gic_route_irq_to_guest is protecting against
> concurrent changes to the same virtual irq.
>
> In other words, the current code does:
> 1) lock physical irq
> 2) lock virtual irq
> 3) establish mapping virtual irq - physical irq
>
> Andre, sorry for not seeing this earlier.
>
>
>>>> Without this locking, you can have two concurrent call of
>>>> gic_route_irq_to_guest that will update the same virtual interrupt but
>>>> with different physical interrupts.
>>>>
>>>> You would have to replace the rank lock by the per-pending_irq lock to
>>>> keep the code safe.
>>>
>>> That indeed sounds reasonable.
>>
>> As you mentioned IRL, the current code may lead to a deadlock due to locking
>> order.
>>
>> Indeed routing an IRQ (route_irq_to_guest) will take:
>> 1) desc lock (in route_irq_to_guest)
>> 2) rank lock (in gic_route_irq_to_guest)
>>
>> Whilst the MMIO emulation of ISENABLER_* will take:
>> 1) rank lock
>> 2) desc lock (in vgic_enable_irqs)
>
> Yes, you are right, but I don't think that is an issue because
> route_irq_to_guest is expected to be called before the domain is
> started, which is consistent with what you wrote below that routing
> should be done *before* running the VM.
I didn't consider as a big issue because of that and the fact it
currently can only happen via a domctl or during dom0 building.
>
> To be clear: I am not arguing for keeping the code as-is, just trying to
> understand it.
>
>
>> Using the per-pending_irq lock will not solve the deadlock. I though a bit
>> more to the code. I believe the routing of SPIs/PPIs after domain creation
>> time is a call to mistake and locking nightmare. Similarly an interrupt should
>> stay routed for the duration of the domain life.
>
> It looks like current routing code was already written with that assumption.
>
>
>> So I would forbid IRQ routing after domain creation (see d->creation_finished)
>> and remove IRQ whilst routing (I think with d->is_dying). This would have an
>> race between the routing and the rest of the vGIC code.
>
> I am fine with that.
>
>
>> However, this would not prevent the routing function to race against itself.
>> For that I would take the vgic domain lock, it is fine because routing is not
>> something we expect to happen often.
>>
>> Any opinions?
>
> The changes done by gic_route_irq_to_guest are specific to one virtual
> irq and one physical irq. In fact, they establish the mapping between
> the two.
>
> Given that concurrent changes to a physical irq are protected by the
> desc->lock in route_irq_to_guest, why do you think that taking the new
> pending_irq lock in gic_route_irq_to_guest would not be sufficient?
> (gic_route_irq_to_guest is always called by route_irq_to_guest.)
Because if we just replace the rank lock by the pending lock there
deadlock would still be there. I guess we can take the pending_irq lock
in route_irq_to_guest before the desc lock.
But I like the idea of hiding the vgic locking in gic_route_irq_to_guest
to keep irq.c fairly Interrupt Controller agnostic.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 01/10] ARM: vGIC: remove rank lock from IRQ routing functions
2017-05-08 22:13 ` Julien Grall
@ 2017-05-09 0:47 ` Stefano Stabellini
2017-05-09 15:24 ` Julien Grall
0 siblings, 1 reply; 31+ messages in thread
From: Stefano Stabellini @ 2017-05-09 0:47 UTC (permalink / raw)
To: Julien Grall; +Cc: Andre Przywara, nd, Stefano Stabellini, xen-devel
On Mon, 8 May 2017, Julien Grall wrote:
> Hi Stefano,
>
> On 08/05/2017 22:53, Stefano Stabellini wrote:
> > On Mon, 8 May 2017, Julien Grall wrote:
> > > Hi Andre,
> > >
> > > On 08/05/17 10:15, Andre Przywara wrote:
> > > > On 04/05/17 16:53, Julien Grall wrote:
> > > > > Hi Andre,
> > > > >
> > > > > On 04/05/17 16:31, Andre Przywara wrote:
> > > > > > gic_route_irq_to_guest() and gic_remove_irq_from_guest() take the
> > > > > > rank
> > > > > > lock, however never actually access the rank structure.
> > > > > > Avoid taking the lock in those two functions and remove some more
> > > > > > unneeded code on the way.
> > > > >
> > > > > The rank is here to protect p->desc when checking that the virtual
> > > > > interrupt was not yet routed to another physical interrupt.
> > > >
> > > > Really? To me that sounds quite surprising.
> > > > My understanding is that the VGIC VCPU lock protected the pending_irq
> > > > (and thus the desc pointer?) so far, and the desc itself has its own
> > > > lock.
> > > > According to the comment in the struct irq_rank declaration the lock
> > > > protects the members of this struct only.
> > > >
> > > > Looking briefly at users of pending_irq->desc (for instance
> > > > gicv[23]_update_lr() or gic_update_one_lr()) I can't see any hint that
> > > > they care about the lock.
> > > >
> > > > So should that be fixed or at least documented?
> > >
> > > My understanding is this rank lock is preventing race between two updates
> > > of
> > > p->desc. This can happen if gic_route_irq_to_guest(...) is called
> > > concurrently
> > > with the same vIRQ but different pIRQ.
> > >
> > > If you drop this lock, nothing will protect that anymore.
> >
> > The desc->lock in route_irq_to_guest is protecting against concurrent
> > changes to the same physical irq.
> >
> > The vgic_lock_rank in gic_route_irq_to_guest is protecting against
> > concurrent changes to the same virtual irq.
> >
> > In other words, the current code does:
> > 1) lock physical irq
> > 2) lock virtual irq
> > 3) establish mapping virtual irq - physical irq
> >
> > Andre, sorry for not seeing this earlier.
> >
> >
> > > > > Without this locking, you can have two concurrent call of
> > > > > gic_route_irq_to_guest that will update the same virtual interrupt but
> > > > > with different physical interrupts.
> > > > >
> > > > > You would have to replace the rank lock by the per-pending_irq lock to
> > > > > keep the code safe.
> > > >
> > > > That indeed sounds reasonable.
> > >
> > > As you mentioned IRL, the current code may lead to a deadlock due to
> > > locking
> > > order.
> > >
> > > Indeed routing an IRQ (route_irq_to_guest) will take:
> > > 1) desc lock (in route_irq_to_guest)
> > > 2) rank lock (in gic_route_irq_to_guest)
> > >
> > > Whilst the MMIO emulation of ISENABLER_* will take:
> > > 1) rank lock
> > > 2) desc lock (in vgic_enable_irqs)
> >
> > Yes, you are right, but I don't think that is an issue because
> > route_irq_to_guest is expected to be called before the domain is
> > started, which is consistent with what you wrote below that routing
> > should be done *before* running the VM.
>
> I didn't consider as a big issue because of that and the fact it currently can
> only happen via a domctl or during dom0 building.
>
> >
> > To be clear: I am not arguing for keeping the code as-is, just trying to
> > understand it.
> >
> >
> > > Using the per-pending_irq lock will not solve the deadlock. I though a bit
> > > more to the code. I believe the routing of SPIs/PPIs after domain creation
> > > time is a call to mistake and locking nightmare. Similarly an interrupt
> > > should
> > > stay routed for the duration of the domain life.
> >
> > It looks like current routing code was already written with that assumption.
> >
> >
> > > So I would forbid IRQ routing after domain creation (see
> > > d->creation_finished)
> > > and remove IRQ whilst routing (I think with d->is_dying). This would have
> > > an
> > > race between the routing and the rest of the vGIC code.
> >
> > I am fine with that.
> >
> >
> > > However, this would not prevent the routing function to race against
> > > itself.
> > > For that I would take the vgic domain lock, it is fine because routing is
> > > not
> > > something we expect to happen often.
> > >
> > > Any opinions?
> >
> > The changes done by gic_route_irq_to_guest are specific to one virtual
> > irq and one physical irq. In fact, they establish the mapping between
> > the two.
> >
> > Given that concurrent changes to a physical irq are protected by the
> > desc->lock in route_irq_to_guest, why do you think that taking the new
> > pending_irq lock in gic_route_irq_to_guest would not be sufficient?
> > (gic_route_irq_to_guest is always called by route_irq_to_guest.)
>
> Because if we just replace the rank lock by the pending lock there deadlock
> would still be there. I guess we can take the pending_irq lock in
> route_irq_to_guest before the desc lock.
I think that's right, it would work.
> But I like the idea of hiding the vgic locking in gic_route_irq_to_guest to
> keep irq.c fairly Interrupt Controller agnostic.
Did you suggest to take the vgic domain lock in gic_route_irq_to_guest,
instead of the rank lock?
It would work, but it would establish a new locking order constraint:
desc->lock first, then d->arch.vgic.lock, which is a bit unnatural. For
example, the d->arch.vgic.lock is taken by vgic-v2 and vgic-v3 in
response to guest MMIO traps. Today, they don't do much, but it is easy
to imagine that in the future you might want to take the desc->lock
after the d->arch.vgic.lock, the same way we take the desc->lock after
the rank lock today.
On the other end, if we take the pending_irq lock before desc->lock we
don't add any new order constraints. That's my preference
But maybe I misunderstood and you meant to suggest taking the
d->arch.vgic.lock before the desc->lock in route_irq_to_guest? I guess
that would work too.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 01/10] ARM: vGIC: remove rank lock from IRQ routing functions
2017-05-09 0:47 ` Stefano Stabellini
@ 2017-05-09 15:24 ` Julien Grall
0 siblings, 0 replies; 31+ messages in thread
From: Julien Grall @ 2017-05-09 15:24 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: Andre Przywara, xen-devel
Hi Stefano,
On 09/05/17 01:47, Stefano Stabellini wrote:
> On Mon, 8 May 2017, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 08/05/2017 22:53, Stefano Stabellini wrote:
>>> On Mon, 8 May 2017, Julien Grall wrote:
>>>> Hi Andre,
>>>>
>>>> On 08/05/17 10:15, Andre Przywara wrote:
>>>>> On 04/05/17 16:53, Julien Grall wrote:
>>>>>> Hi Andre,
>>>>>>
>>>>>> On 04/05/17 16:31, Andre Przywara wrote:
>>>>>>> gic_route_irq_to_guest() and gic_remove_irq_from_guest() take the
>>>>>>> rank
>>>>>>> lock, however never actually access the rank structure.
>>>>>>> Avoid taking the lock in those two functions and remove some more
>>>>>>> unneeded code on the way.
>>>>>>
>>>>>> The rank is here to protect p->desc when checking that the virtual
>>>>>> interrupt was not yet routed to another physical interrupt.
>>>>>
>>>>> Really? To me that sounds quite surprising.
>>>>> My understanding is that the VGIC VCPU lock protected the pending_irq
>>>>> (and thus the desc pointer?) so far, and the desc itself has its own
>>>>> lock.
>>>>> According to the comment in the struct irq_rank declaration the lock
>>>>> protects the members of this struct only.
>>>>>
>>>>> Looking briefly at users of pending_irq->desc (for instance
>>>>> gicv[23]_update_lr() or gic_update_one_lr()) I can't see any hint that
>>>>> they care about the lock.
>>>>>
>>>>> So should that be fixed or at least documented?
>>>>
>>>> My understanding is this rank lock is preventing race between two updates
>>>> of
>>>> p->desc. This can happen if gic_route_irq_to_guest(...) is called
>>>> concurrently
>>>> with the same vIRQ but different pIRQ.
>>>>
>>>> If you drop this lock, nothing will protect that anymore.
>>>
>>> The desc->lock in route_irq_to_guest is protecting against concurrent
>>> changes to the same physical irq.
>>>
>>> The vgic_lock_rank in gic_route_irq_to_guest is protecting against
>>> concurrent changes to the same virtual irq.
>>>
>>> In other words, the current code does:
>>> 1) lock physical irq
>>> 2) lock virtual irq
>>> 3) establish mapping virtual irq - physical irq
>>>
>>> Andre, sorry for not seeing this earlier.
>>>
>>>
>>>>>> Without this locking, you can have two concurrent call of
>>>>>> gic_route_irq_to_guest that will update the same virtual interrupt but
>>>>>> with different physical interrupts.
>>>>>>
>>>>>> You would have to replace the rank lock by the per-pending_irq lock to
>>>>>> keep the code safe.
>>>>>
>>>>> That indeed sounds reasonable.
>>>>
>>>> As you mentioned IRL, the current code may lead to a deadlock due to
>>>> locking
>>>> order.
>>>>
>>>> Indeed routing an IRQ (route_irq_to_guest) will take:
>>>> 1) desc lock (in route_irq_to_guest)
>>>> 2) rank lock (in gic_route_irq_to_guest)
>>>>
>>>> Whilst the MMIO emulation of ISENABLER_* will take:
>>>> 1) rank lock
>>>> 2) desc lock (in vgic_enable_irqs)
>>>
>>> Yes, you are right, but I don't think that is an issue because
>>> route_irq_to_guest is expected to be called before the domain is
>>> started, which is consistent with what you wrote below that routing
>>> should be done *before* running the VM.
>>
>> I didn't consider as a big issue because of that and the fact it currently can
>> only happen via a domctl or during dom0 building.
>>
>>>
>>> To be clear: I am not arguing for keeping the code as-is, just trying to
>>> understand it.
>>>
>>>
>>>> Using the per-pending_irq lock will not solve the deadlock. I though a bit
>>>> more to the code. I believe the routing of SPIs/PPIs after domain creation
>>>> time is a call to mistake and locking nightmare. Similarly an interrupt
>>>> should
>>>> stay routed for the duration of the domain life.
>>>
>>> It looks like current routing code was already written with that assumption.
>>>
>>>
>>>> So I would forbid IRQ routing after domain creation (see
>>>> d->creation_finished)
>>>> and remove IRQ whilst routing (I think with d->is_dying). This would have
>>>> an
>>>> race between the routing and the rest of the vGIC code.
>>>
>>> I am fine with that.
>>>
>>>
>>>> However, this would not prevent the routing function to race against
>>>> itself.
>>>> For that I would take the vgic domain lock, it is fine because routing is
>>>> not
>>>> something we expect to happen often.
>>>>
>>>> Any opinions?
>>>
>>> The changes done by gic_route_irq_to_guest are specific to one virtual
>>> irq and one physical irq. In fact, they establish the mapping between
>>> the two.
>>>
>>> Given that concurrent changes to a physical irq are protected by the
>>> desc->lock in route_irq_to_guest, why do you think that taking the new
>>> pending_irq lock in gic_route_irq_to_guest would not be sufficient?
>>> (gic_route_irq_to_guest is always called by route_irq_to_guest.)
>>
>> Because if we just replace the rank lock by the pending lock there deadlock
>> would still be there. I guess we can take the pending_irq lock in
>> route_irq_to_guest before the desc lock.
>
> I think that's right, it would work.
>
>
>> But I like the idea of hiding the vgic locking in gic_route_irq_to_guest to
>> keep irq.c fairly Interrupt Controller agnostic.
>
> Did you suggest to take the vgic domain lock in gic_route_irq_to_guest,
> instead of the rank lock?
Yes.
>
> It would work, but it would establish a new locking order constraint:
> desc->lock first, then d->arch.vgic.lock, which is a bit unnatural. For
> example, the d->arch.vgic.lock is taken by vgic-v2 and vgic-v3 in
> response to guest MMIO traps. Today, they don't do much, but it is easy
> to imagine that in the future you might want to take the desc->lock
> after the d->arch.vgic.lock, the same way we take the desc->lock after
> the rank lock today.
Well, IHMO the domain vgic lock should only be taken when you modify a
global configuration (such as enabling/disabling the distributor,
routing new IRQs...). This is what is already done today and we should
avoid to use for more than that as it will not scale.
So suggesting this lock ordering sounds good to me as you will unlikely
have to take the desc->lock when you get the domain vgic lock.
>
> On the other end, if we take the pending_irq lock before desc->lock we
> don't add any new order constraints. That's my preference
But this would require to take pending_irq in route_irq_to_guest where I
think we are making the line more blur between irq.c and gic.c
If possible I'd like to keep the irq.c agnostic to what we do in the vGIC.
>
>
> But maybe I misunderstood and you meant to suggest taking the
> d->arch.vgic.lock before the desc->lock in route_irq_to_guest? I guess
> that would work too.
See my answer just above.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2017-05-09 15:24 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-04 15:31 [RFC PATCH 00/10] ARM VGIC rework: remove rank, introduce per-IRQ lock Andre Przywara
2017-05-04 15:31 ` [RFC PATCH 01/10] ARM: vGIC: remove rank lock from IRQ routing functions Andre Przywara
2017-05-04 15:53 ` Julien Grall
2017-05-08 9:15 ` Andre Przywara
2017-05-08 14:26 ` Julien Grall
2017-05-08 21:53 ` Stefano Stabellini
2017-05-08 22:13 ` Julien Grall
2017-05-09 0:47 ` Stefano Stabellini
2017-05-09 15:24 ` Julien Grall
2017-05-04 15:31 ` [RFC PATCH 02/10] ARM: vGIC: rework gic_raise_*_irq() functions Andre Przywara
2017-05-04 15:31 ` [RFC PATCH 03/10] ARM: vGIC: introduce and initialize pending_irq lock Andre Przywara
2017-05-04 15:31 ` [RFC PATCH 04/10] ARM: vGIC: add struct pending_irq locking Andre Przywara
2017-05-04 16:21 ` Julien Grall
2017-05-05 9:02 ` Andre Przywara
2017-05-05 23:29 ` Stefano Stabellini
2017-05-06 6:41 ` Julien Grall
2017-05-05 23:42 ` Stefano Stabellini
2017-05-04 16:54 ` Julien Grall
2017-05-05 23:26 ` Stefano Stabellini
2017-05-04 15:31 ` [RFC PATCH 05/10] ARM: vGIC: move priority from irq_rank to struct pending_irq Andre Przywara
2017-05-04 16:39 ` Julien Grall
2017-05-04 16:47 ` Julien Grall
2017-05-04 15:31 ` [RFC PATCH 06/10] ARM: vGIC: move config " Andre Przywara
2017-05-04 16:55 ` Julien Grall
2017-05-04 15:31 ` [RFC PATCH 07/10] ARM: vGIC: move enable status " Andre Przywara
2017-05-04 15:31 ` [RFC PATCH 08/10] ARM: vGIC: move target vcpu " Andre Przywara
2017-05-04 17:20 ` Julien Grall
2017-05-04 15:31 ` [RFC PATCH 09/10] ARM: vGIC: introduce vgic_get/put_pending_irq Andre Przywara
2017-05-04 17:36 ` Julien Grall
2017-05-04 22:42 ` Stefano Stabellini
2017-05-04 15:31 ` [RFC PATCH 10/10] ARM: vGIC: remove struct irq_rank and support functions Andre Przywara
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.