[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [RFC PATCH 09/10] ARM: vGIC: introduce vgic_get/put_pending_irq
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@xxxxxxx>
---
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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
