[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v8 03/27] ARM: GIC: Add checks for NULL pointer pending_irq's
On 12/04/17 15:51, Andre Przywara wrote:
Hi,
On 12/04/17 11:25, Julien Grall wrote:
Hi Andre,
On 12/04/17 01:44, Andre Przywara wrote:
For LPIs the struct pending_irq's are dynamically allocated and the
pointers will be stored in a radix tree. Since an LPI can be "unmapped"
at any time, teach the VGIC how to handle with irq_to_pending() returning
a NULL pointer.
We just do nothing in this case or clean up the LR if the virtual LPI
number was still in an LR.
Why not all the irq_to_pending call are not protected (such as
vgic_irq_to_migrate)?
Some of them are never called by LPIs.
Is it worth the put ASSERTs in there everywhere?
Yes.
I can copy the table with all call sites and their evaluations from that
other email into this commit message, if that sounds good.
And yes. A commit message should contain all the details saving us time
to look in the e-mails...
Fixed the rest.
Cheers,
Andre.
This is a call to forget to check NULL if we
decide to use the function in the future.
Also, I would like a comment on top of irq_to_pending stating this can
return NULL as you change the semantic of the function.
Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
---
xen/arch/arm/gic.c | 37 ++++++++++++++++++++++++++++++++-----
xen/arch/arm/vgic.c | 6 ++++++
2 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index dcb1783..62ae3b8 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -408,9 +408,15 @@ void gic_remove_from_queues(struct vcpu *v,
unsigned int virtual_irq)
spin_lock_irqsave(&v->arch.vgic.lock, flags);
p = irq_to_pending(v, virtual_irq);
-
- if ( !list_empty(&p->lr_queue) )
- list_del_init(&p->lr_queue);
+ /*
+ * If an LPIs has been removed meanwhile, it has been cleaned up
+ * already, so nothing to remove here.
+ */
+ if ( p )
+ {
+ if ( !list_empty(&p->lr_queue) )
+ list_del_init(&p->lr_queue);
+ }
This could be simplified with:
if ( p && !list_empty(&p->lr_queue) )
list_del_init(...);
Also, you probably want a likely(p).
spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
}
@@ -418,6 +424,10 @@ void gic_raise_inflight_irq(struct vcpu *v,
unsigned int virtual_irq)
{
struct pending_irq *n = irq_to_pending(v, virtual_irq);
+ /* If an LPI has been removed meanwhile, there is nothing left to
raise. */
+ if ( !n )
if ( unlikely(!n) )
+ return;
+
ASSERT(spin_is_locked(&v->arch.vgic.lock));
if ( list_empty(&n->lr_queue) )
@@ -437,6 +447,11 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned
int virtual_irq,
{
int i;
unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
+ struct pending_irq *p = irq_to_pending(v, virtual_irq);
+
+ if ( !p )
if ( unlikely(!p) )
+ /* An unmapped LPI does not need to be raised. */
+ return;
Please move this check after the ASSERT to keep the check on all the paths.
ASSERT(spin_is_locked(&v->arch.vgic.lock));
@@ -445,12 +460,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)
@@ -464,7 +479,19 @@ static void gic_update_one_lr(struct vcpu *v, int i)
gic_hw_ops->read_lr(i, &lr_val);
irq = lr_val.virq;
+
4th time I am saying this.... Spurious line.
p = irq_to_pending(v, irq);
+ /* An LPI might have been unmapped, in which case we just clean
up here. */
+ if ( !p )
unlikely(!p)
+ {
+ ASSERT(is_lpi(irq));
+
+ gic_hw_ops->clear_lr(i);
+ clear_bit(i, &this_cpu(lr_mask));
+
+ return;
+ }
+
if ( lr_val.state & GICH_LR_ACTIVE )
{
set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index c953f13..b9fc837 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -473,6 +473,12 @@ void vgic_vcpu_inject_irq(struct vcpu *v,
unsigned int virq)
spin_lock_irqsave(&v->arch.vgic.lock, flags);
n = irq_to_pending(v, virq);
+ /* If an LPI has been removed, there is nothing to inject here. */
+ if ( !n )
unlikely(...)
+ {
+ spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+ return;
+ }
priority = vgic_get_virq_priority(v, virq);
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
