| Message ID | 20170511175340.8448-3-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 11 May 2017, Andre Przywara wrote: > So far irq_to_pending() is just a convenience function to lookup > statically allocated arrays. This will change with LPIs, which are > more dynamic. > The proper answer to the issue of preventing stale pointers is > ref-counting, which requires more rework and will be introduced with > a later rework. > For now move the irq_to_pending() calls that are used with LPIs under the > VGIC VCPU lock, and only use the returned pointer while holding the lock. > This prevents the memory from being freed while we use it. I don't like the idea of doing this just for the functions that are used by LPIs and not the other. Specifically, we are leaving out: [a]: - vgic_migrate_irq - vgic_enable_irqs - vgic_disable_irqs [b]: - arch_move_irqs Those in group [a] are easy to fix, please do. Just introduce a spinlock in vgic_disable_irqs (it is safe because gic_remove_from_queues already takes the vgic vcpu lock). [b] is not easy to fix, just add a comment. > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/gic.c | 5 ++++- > xen/arch/arm/vgic.c | 4 +++- > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index da19130..dcb1783 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -402,10 +402,13 @@ 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; > unsigned long flags; > > 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); > spin_unlock_irqrestore(&v->arch.vgic.lock, flags); > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index 83569b0..d30f324 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -466,7 +466,7 @@ 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); > + struct pending_irq *iter, *n; > unsigned long flags; > bool running; > > @@ -474,6 +474,8 @@ 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); > + > /* vcpu offline */ > if ( test_bit(_VPF_down, &v->pause_flags) ) > { > -- > 2.9.0 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel >
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index da19130..dcb1783 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -402,10 +402,13 @@ 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; unsigned long flags; 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); spin_unlock_irqrestore(&v->arch.vgic.lock, flags); diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index 83569b0..d30f324 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -466,7 +466,7 @@ 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); + struct pending_irq *iter, *n; unsigned long flags; bool running; @@ -474,6 +474,8 @@ 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); + /* vcpu offline */ if ( test_bit(_VPF_down, &v->pause_flags) ) {
So far irq_to_pending() is just a convenience function to lookup statically allocated arrays. This will change with LPIs, which are more dynamic. The proper answer to the issue of preventing stale pointers is ref-counting, which requires more rework and will be introduced with a later rework. For now move the irq_to_pending() calls that are used with LPIs under the VGIC VCPU lock, and only use the returned pointer while holding the lock. This prevents the memory from being freed while we use it. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/arch/arm/gic.c | 5 ++++- xen/arch/arm/vgic.c | 4 +++- 2 files changed, 7 insertions(+), 2 deletions(-)