[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On Fri, 13 Jun 2014, Ian Campbell wrote:
> On Fri, 2014-06-13 at 12:24 +0100, 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
> 
> "alignment"
> 
> > bytes.
> 
> Are the non underscore versions of the register definitions even used
> after this? If not then I'd prefer to just use the non-underscored
> names.

Yes, by x86


> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > CC: Jan Beulich <jbeulich@xxxxxxxx>
> > ---
> >  xen/arch/arm/gic.c |   10 +++++-----
> >  xen/arch/arm/irq.c |   29 +++++++++++++++--------------
> >  2 files changed, 20 insertions(+), 19 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index a4422fd..2a88998 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -137,7 +137,7 @@ static void gic_irq_enable(struct irq_desc *desc)
> >      ASSERT(spin_is_locked(&desc->lock));
> >  
> >      spin_lock_irqsave(&gic.lock, flags);
> > -    desc->status &= ~IRQ_DISABLED;
> > +    clear_bit(_IRQ_DISABLED, &desc->status);
> >      dsb(sy);
> >      /* Enable routing */
> >      GICD[GICD_ISENABLER + irq / 32] = (1u << (irq % 32));
> > @@ -154,7 +154,7 @@ static void gic_irq_disable(struct irq_desc *desc)
> >      spin_lock_irqsave(&gic.lock, flags);
> >      /* Disable routing */
> >      GICD[GICD_ICENABLER + irq / 32] = (1u << (irq % 32));
> > -    desc->status |= IRQ_DISABLED;
> > +    set_bit(_IRQ_DISABLED, &desc->status);
> >      spin_unlock_irqrestore(&gic.lock, flags);
> >  }
> >  
> > @@ -277,7 +277,7 @@ void gic_route_irq_to_xen(struct irq_desc *desc, bool_t 
> > level,
> >  {
> >      ASSERT(priority <= 0xff);     /* Only 8 bits of priority */
> >      ASSERT(desc->irq < gic.lines);/* Can't route interrupts that don't 
> > exist */
> > -    ASSERT(desc->status & IRQ_DISABLED);
> > +    ASSERT(test_bit(_IRQ_DISABLED, &desc->status));
> >      ASSERT(spin_is_locked(&desc->lock));
> >  
> >      desc->handler = &gic_host_irq_type;
> > @@ -296,7 +296,7 @@ void gic_route_irq_to_guest(struct domain *d, struct 
> > irq_desc *desc,
> >      ASSERT(spin_is_locked(&desc->lock));
> >  
> >      desc->handler = &gic_guest_irq_type;
> > -    desc->status |= IRQ_GUEST;
> > +    set_bit(_IRQ_GUEST, &desc->status);
> >  
> >      gic_set_irq_properties(desc->irq, level, 
> > cpumask_of(smp_processor_id()),
> >                             GIC_PRI_IRQ);
> > @@ -685,7 +685,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
> >          clear_bit(i, &this_cpu(lr_mask));
> >  
> >          if ( p->desc != NULL )
> > -            p->desc->status &= ~IRQ_INPROGRESS;
> > +            clear_bit(_IRQ_INPROGRESS, &p->desc->status);
> >          clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> >          clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
> >          p->lr = GIC_INVALID_LR;
> > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> > index 756250c..d0c7962 100644
> > --- a/xen/arch/arm/irq.c
> > +++ b/xen/arch/arm/irq.c
> > @@ -102,7 +102,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);
> > @@ -166,40 +166,41 @@ 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();
> >  
> >          vgic_vcpu_inject_spi(d, irq);
> >          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) )
> >          goto out;
> >  
> > -    desc->status |= IRQ_INPROGRESS;
> > +    set_bit(_IRQ_INPROGRESS, &desc->status);
> >  
> >      action = desc->action;
> > -    while ( desc->status & IRQ_PENDING )
> > +    while ( test_bit(_IRQ_PENDING, &desc->status) )
> >      {
> > -        desc->status &= ~IRQ_PENDING;
> > +        clear_bit(_IRQ_PENDING, &desc->status);
> >          spin_unlock_irq(&desc->lock);
> >          action->handler(irq, action->dev_id, regs);
> >          spin_lock_irq(&desc->lock);
> >      }
> >  
> > -    desc->status &= ~IRQ_INPROGRESS;
> > +    clear_bit(_IRQ_INPROGRESS, &desc->status);
> >  
> >  out:
> >      desc->handler->end(desc);
> > @@ -222,12 +223,12 @@ void release_irq(unsigned int irq)
> >  
> >      action = desc->action;
> >      desc->action  = NULL;
> > -    desc->status &= ~IRQ_GUEST;
> > +    clear_bit(_IRQ_GUEST, &desc->status);
> >  
> >      spin_unlock_irqrestore(&desc->lock,flags);
> >  
> >      /* Wait to make sure it's not being used on another CPU */
> > -    do { smp_mb(); } while ( desc->status & IRQ_INPROGRESS );
> > +    do { smp_mb(); } while ( test_bit(_IRQ_INPROGRESS, &desc->status) );
> >  
> >      if ( action && action->free_on_release )
> >          xfree(action);
> > @@ -255,7 +256,7 @@ int setup_dt_irq(const struct dt_irq *irq, struct 
> > irqaction *new)
> >  
> >      spin_lock_irqsave(&desc->lock, flags);
> >  
> > -    if ( desc->status & IRQ_GUEST )
> > +    if ( test_bit(_IRQ_GUEST, &desc->status) )
> >      {
> >          struct domain *d = irq_get_domain(desc);
> >  
> > @@ -323,10 +324,10 @@ int route_dt_irq_to_guest(struct domain *d, const 
> > struct dt_irq *irq,
> >      {
> >          struct domain *ad = irq_get_domain(desc);
> >  
> > -        if ( (desc->status & IRQ_GUEST) && d == ad )
> > +        if ( test_bit(_IRQ_GUEST, &desc->status) && d == ad )
> >              goto out;
> >  
> > -        if ( desc->status & IRQ_GUEST )
> > +        if ( test_bit(_IRQ_GUEST, &desc->status) )
> >              printk(XENLOG_ERR "ERROR: IRQ %u is already used by domain 
> > %u\n",
> >                     irq->irq, ad->domain_id);
> >          else
> 
> 

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.