Re: [Xen-devel] [PATCH v9 02/28] ARM: VGIC: move irq_to_pending() calls under the VGIC VCPU lock

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxx>
> ---
>  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@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel