From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH 1/7] xen/arm: fix rank calculation in vgic_vcpu_inject_irq Date: Fri, 26 Oct 2012 21:47:20 +0100 Message-ID: <1351284440.12176.9.camel@dagon.hellion.org.uk> References: <1351091027-20740-1-git-send-email-stefano.stabellini@eu.citrix.com> <1351157246.18035.129.camel@zakaz.uk.xensource.com> <1351269622.15162.105.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: "xen-devel@lists.xensource.com" , "Tim (Xen.org)" List-Id: xen-devel@lists.xenproject.org On Fri, 2012-10-26 at 19:42 +0100, Stefano Stabellini wrote: > I think that the problem is the usage of vgic_irq_rank with registers > that have 1 bit per interrupt. That's very plausible indeed. > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index 3c3983f..92731b6 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -42,13 +42,7 @@ > */ > static inline int REG_RANK_NR(int b, uint32_t n) > { > - switch ( b ) > - { > - case 8: return n >> 3; > - case 4: return n >> 2; > - case 2: return n >> 1; > - default: BUG(); > - } > + return n / b; All the infrastructure will fall apart if b isn't a power of two, that's why I used the switch. Can you just add the appropriate case 1 instead? Probably the bug should be a call to an undefined function to make this a compile time rather than runtime failure too. > @@ -577,9 +571,9 @@ struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq) > > void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual) The changes to this function are all unnecessary, since it was already consistent within itself. If you want to change it please add a helper which takes an irq and hides the shifts. I'd be inclined to rename the existing vgic_irq_rank to vgic_reg_rank and then call the new helper vgic_irq_rank since that would better reflect the purpose of both. > { > - int idx = irq >> 2, byte = irq & 0x3; > + int byte = irq & 0x3; byte isn't actually needed either. > uint8_t priority; > - struct vgic_irq_rank *rank = vgic_irq_rank(v, 8, idx); > + struct vgic_irq_rank *rank = vgic_irq_rank(v, 1, irq / 32); > struct pending_irq *iter, *n = irq_to_pending(v, irq); > unsigned long flags; > > @@ -587,7 +581,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual) > if (!list_empty(&n->inflight)) > return; > > - priority = byte_read(rank->ipriority[REG_RANK_INDEX(8, idx)], 0, byte); > + priority = byte_read(rank->ipriority[REG_RANK_INDEX(8, irq / 4)], 0, byte); > > n->irq = irq; > n->priority = priority;