| Message ID | 1402658663-2113-2-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
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. > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > CC: Jan Beulich <jbeulich@suse.com> > --- > 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
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@eu.citrix.com> > > CC: Jan Beulich <jbeulich@suse.com> > > --- > > 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 > >
On Fri, 2014-06-13 at 12:41 +0100, Stefano Stabellini wrote: > 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 Ah! I wondered why Jan was CCd ;-)
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
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. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> CC: Jan Beulich <jbeulich@suse.com> --- xen/arch/arm/gic.c | 10 +++++----- xen/arch/arm/irq.c | 29 +++++++++++++++-------------- 2 files changed, 20 insertions(+), 19 deletions(-)