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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 25 Jul 2014, Julien Grall wrote:
> 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.

I can do that


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

OK


> [..]
> 
> > 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).

It wasn't guaranteed to be atomic before either.


> 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.

I think so too.

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