Re: [PATCH v9 10/10] xen/arm: make accesses to desc->status flags atomic

[Julien Grall <julien.grall@linaro.org>]

Hi Stefano,

On 07/24/2014 06:33 PM, Stefano Stabellini wrote:
> Using *_bit manipulation functions on desc->status is safe on arm64:
> status is an unsigned int but is the first field of a struct that
> contains pointers, therefore the alignement of the struct is at least 8
> bytes.

I would add a comment in the structure irq_desc in include/common/irq.h.
To explain this assumption.

It would avoid people breaking the structure by mistake in the future.

Also I would explain a bit more why we need to use atomic while most of
the access are protected by desc->lock.

[..]

> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 7150c7a..0097349 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -126,7 +126,7 @@ static inline struct domain *irq_get_domain(struct irq_desc *desc)
>  {
>      ASSERT(spin_is_locked(&desc->lock));
>  
> -    if ( !(desc->status & IRQ_GUEST) )
> +    if ( !test_bit(_IRQ_GUEST, &desc->status) )
>          return dom_xen;
>  
>      ASSERT(desc->action != NULL);
> @@ -195,13 +195,13 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
>          goto out;
>      }
>  
> -    if ( desc->status & IRQ_GUEST )
> +    if ( test_bit(_IRQ_GUEST, &desc->status) )
>      {
>          struct domain *d = irq_get_domain(desc);
>  
>          desc->handler->end(desc);
>  
> -        desc->status |= IRQ_INPROGRESS;
> +        set_bit(_IRQ_INPROGRESS, &desc->status);
>          desc->arch.eoi_cpu = smp_processor_id();
>  
>          /* the irq cannot be a PPI, we only support delivery of SPIs to
> @@ -210,22 +210,23 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
>          goto out_no_end;
>      }
>  
> -    desc->status |= IRQ_PENDING;
> +    set_bit(_IRQ_PENDING, &desc->status);
>  
>      /*
>       * Since we set PENDING, if another processor is handling a different
>       * instance of this same irq, the other processor will take care of it.
>       */
> -    if ( desc->status & (IRQ_DISABLED | IRQ_INPROGRESS) )
> +    if ( test_bit(_IRQ_DISABLED, &desc->status) ||
> +         test_bit(_IRQ_INPROGRESS, &desc->status) )

You replaced 1 access to desc->status by 2 accesses. I think it may have
a race condition when the desc->lock it's not taken (such as in
gic_update_one_lr).

At the same time, this will never happen as a guest IRQ will never
execute this code.

If it happens for IRQ routed to Xen, the interrupt will be EOIed and
fire again as soon as we leave the IRQ exception mode.

So I think we are fine for now.

Regards,

-- 
Julien Grall