Xen project Mailing List

Re: [Xen-devel] [PATCH 1/7] xen/arm: fix rank calculation in vgic_vcpu_inject_irq

To: Ian Campbell <Ian.Campbell@xxxxxxxxxx>

From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>

Date: Fri, 26 Oct 2012 19:42:26 +0100

Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>, "Tim \(Xen.org\)" <tim@xxxxxxx>, Stefano Stabellini <Stefano.Stabellini@xxxxxxxxxxxxx>

Delivery-date: Fri, 26 Oct 2012 18:43:03 +0000

List-id: Xen developer discussion <xen-devel.lists.xen.org>

On Fri, 26 Oct 2012, Ian Campbell wrote: > > It means that this patch hides the bug rather than fixing it... > > What are the symptoms? > > BTW I noticed that "byte = irq & 0x3;" is redundant since byte_read also > has the & in it. But that won't cause problems. The ienable bit is wrong for irq > 32. I think that the problem is the usage of vgic_irq_rank with registers that have 1 bit per interrupt. In fact the following patch fixes the issue. --- xen/arm: pass the correct bit-per-interrupt argument to vgic_irq_rank Use 1 for registers that have 1 bit per irq. Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx> 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; } /* @@ -199,7 +193,7 @@ static int vgic_distr_mmio_read(struct vcpu *v, mmio_info_t *info) case GICD_ISENABLER ... GICD_ISENABLERN: if ( dabt.size != 2 ) goto bad_width; - rank = vgic_irq_rank(v, 8, gicd_reg - GICD_ISENABLER); + rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ISENABLER); if ( rank == NULL) goto read_as_zero; vgic_lock_rank(v, rank); *r = rank->ienable; @@ -208,7 +202,7 @@ static int vgic_distr_mmio_read(struct vcpu *v, mmio_info_t *info) case GICD_ICENABLER ... GICD_ICENABLERN: if ( dabt.size != 2 ) goto bad_width; - rank = vgic_irq_rank(v, 8, gicd_reg - GICD_ICENABLER); + rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ICENABLER); if ( rank == NULL) goto read_as_zero; vgic_lock_rank(v, rank); *r = rank->ienable; @@ -217,7 +211,7 @@ static int vgic_distr_mmio_read(struct vcpu *v, mmio_info_t *info) case GICD_ISPENDR ... GICD_ISPENDRN: if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width; - rank = vgic_irq_rank(v, 8, gicd_reg - GICD_ISPENDR); + rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ISPENDR); if ( rank == NULL) goto read_as_zero; vgic_lock_rank(v, rank); *r = byte_read(rank->ipend, dabt.sign, offset); @@ -226,7 +220,7 @@ static int vgic_distr_mmio_read(struct vcpu *v, mmio_info_t *info) case GICD_ICPENDR ... GICD_ICPENDRN: if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width; - rank = vgic_irq_rank(v, 8, gicd_reg - GICD_ICPENDR); + rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ICPENDR); if ( rank == NULL) goto read_as_zero; vgic_lock_rank(v, rank); *r = byte_read(rank->ipend, dabt.sign, offset); @@ -235,7 +229,7 @@ static int vgic_distr_mmio_read(struct vcpu *v, mmio_info_t *info) case GICD_ISACTIVER ... GICD_ISACTIVERN: if ( dabt.size != 2 ) goto bad_width; - rank = vgic_irq_rank(v, 8, gicd_reg - GICD_ISACTIVER); + rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ISACTIVER); if ( rank == NULL) goto read_as_zero; vgic_lock_rank(v, rank); *r = rank->iactive; @@ -244,7 +238,7 @@ static int vgic_distr_mmio_read(struct vcpu *v, mmio_info_t *info) case GICD_ICACTIVER ... GICD_ICACTIVERN: if ( dabt.size != 2 ) goto bad_width; - rank = vgic_irq_rank(v, 8, gicd_reg - GICD_ICACTIVER); + rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ICACTIVER); if ( rank == NULL) goto read_as_zero; vgic_lock_rank(v, rank); *r = rank->iactive; @@ -296,7 +290,7 @@ static int vgic_distr_mmio_read(struct vcpu *v, mmio_info_t *info) case GICD_CPENDSGIR ... GICD_CPENDSGIRN: if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width; - rank = vgic_irq_rank(v, 8, gicd_reg - GICD_CPENDSGIR); + rank = vgic_irq_rank(v, 1, gicd_reg - GICD_CPENDSGIR); if ( rank == NULL) goto read_as_zero; vgic_lock_rank(v, rank); *r = byte_read(rank->pendsgi, dabt.sign, offset); @@ -305,7 +299,7 @@ static int vgic_distr_mmio_read(struct vcpu *v, mmio_info_t *info) case GICD_SPENDSGIR ... GICD_SPENDSGIRN: if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width; - rank = vgic_irq_rank(v, 8, gicd_reg - GICD_SPENDSGIR); + rank = vgic_irq_rank(v, 1, gicd_reg - GICD_SPENDSGIR); if ( rank == NULL) goto read_as_zero; vgic_lock_rank(v, rank); *r = byte_read(rank->pendsgi, dabt.sign, offset); @@ -400,7 +394,7 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info) case GICD_ISENABLER ... GICD_ISENABLERN: if ( dabt.size != 2 ) goto bad_width; - rank = vgic_irq_rank(v, 8, gicd_reg - GICD_ISENABLER); + rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ISENABLER); if ( rank == NULL) goto write_ignore; vgic_lock_rank(v, rank); tr = rank->ienable; @@ -411,7 +405,7 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info) case GICD_ICENABLER ... GICD_ICENABLERN: if ( dabt.size != 2 ) goto bad_width; - rank = vgic_irq_rank(v, 8, gicd_reg - GICD_ICENABLER); + rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ICENABLER); if ( rank == NULL) goto write_ignore; vgic_lock_rank(v, rank); rank->ienable &= ~*r; @@ -432,7 +426,7 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info) case GICD_ISACTIVER ... GICD_ISACTIVERN: if ( dabt.size != 2 ) goto bad_width; - rank = vgic_irq_rank(v, 8, gicd_reg - GICD_ISACTIVER); + rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ISACTIVER); if ( rank == NULL) goto write_ignore; vgic_lock_rank(v, rank); rank->iactive &= ~*r; @@ -441,7 +435,7 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info) case GICD_ICACTIVER ... GICD_ICACTIVERN: if ( dabt.size != 2 ) goto bad_width; - rank = vgic_irq_rank(v, 8, gicd_reg - GICD_ICACTIVER); + rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ICACTIVER); if ( rank == NULL) goto write_ignore; vgic_lock_rank(v, rank); rank->iactive &= ~*r; @@ -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) { - int idx = irq >> 2, byte = irq & 0x3; + int byte = irq & 0x3; 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; _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.