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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 25 Oct 2012, Ian Campbell wrote:
> On Wed, 2012-10-24 at 16:03 +0100, Stefano Stabellini wrote:
> > Each rank holds 32 irqs, so we should divide by 32 rather than by 4.
> 
> I think this is wrong, because idx isn't used in the way your reasoning
> implies, when we use it we assume 8 bits-per-interrupt in the register
> not 1.
> 
> The accessor function vgix_irq_rank is couched in terms of
> GICD_<FOO><n>, which is convenient where we are emulating register
> accesses but is very confusing in this one function where we aren't
> thinking in terms of registers!
> 
> vgic_irq_rank(v, 8, idx) is saying "find me the rank containing
> GICD_<FOO><idx> where FOO has 8 bits per interrupt". Since 4 lots of 8
> bits goes into 32 we therefore need to divide by 4. This makes sense if
> you consider that FOO here is PRIORITY and that GICD_PRIORITY0 controls
> irq 0..3, GICD_PRIORITY1 irq 4..7 etc.
> 
> With idx = irq >> 2 you get:
> 
> IRQ00: idx = 00 REG_RANK_NR(08, 00) = 0, REG_RANK_INDEX(08, 00) = 0, BYTE = 0
> IRQ01: idx = 00 REG_RANK_NR(08, 00) = 0, REG_RANK_INDEX(08, 00) = 0, BYTE = 1
> IRQ02: idx = 00 REG_RANK_NR(08, 00) = 0, REG_RANK_INDEX(08, 00) = 0, BYTE = 2
> IRQ03: idx = 00 REG_RANK_NR(08, 00) = 0, REG_RANK_INDEX(08, 00) = 0, BYTE = 3
> IRQ04: idx = 01 REG_RANK_NR(08, 01) = 0, REG_RANK_INDEX(08, 01) = 1, BYTE = 0
> IRQ05: idx = 01 REG_RANK_NR(08, 01) = 0, REG_RANK_INDEX(08, 01) = 1, BYTE = 1
> [...]
> IRQ30: idx = 07 REG_RANK_NR(08, 07) = 0, REG_RANK_INDEX(08, 07) = 7, BYTE = 2
> IRQ31: idx = 07 REG_RANK_NR(08, 07) = 0, REG_RANK_INDEX(08, 07) = 7, BYTE = 3
> IRQ32: idx = 08 REG_RANK_NR(08, 08) = 1, REG_RANK_INDEX(08, 08) = 0, BYTE = 0
> IRQ33: idx = 08 REG_RANK_NR(08, 08) = 1, REG_RANK_INDEX(08, 08) = 0, BYTE = 1
> [...]
> IRQ62: idx = 15 REG_RANK_NR(08, 15) = 1, REG_RANK_INDEX(08, 15) = 7, BYTE = 2
> IRQ63: idx = 15 REG_RANK_NR(08, 15) = 1, REG_RANK_INDEX(08, 15) = 7, BYTE = 3
> IRQ64: idx = 16 REG_RANK_NR(08, 16) = 2, REG_RANK_INDEX(08, 16) = 0, BYTE = 0
> IRQ65: idx = 16 REG_RANK_NR(08, 16) = 2, REG_RANK_INDEX(08, 16) = 0, BYTE = 1
> [...]
> 
> IOW interrupts 0..31 are in rank 0, 32..63 are rank 1 etc, which is correct
> 
> With idx = irq / 32 you instead get:
> 
> IRQ00: idx = 00 REG_RANK_NR(08, 00) = 0, REG_RANK_INDEX(08, 00) = 0, BYTE = 0
> IRQ01: idx = 00 REG_RANK_NR(08, 00) = 0, REG_RANK_INDEX(08, 00) = 0, BYTE = 1
> IRQ02: idx = 00 REG_RANK_NR(08, 00) = 0, REG_RANK_INDEX(08, 00) = 0, BYTE = 2
> IRQ03: idx = 00 REG_RANK_NR(08, 00) = 0, REG_RANK_INDEX(08, 00) = 0, BYTE = 3
> IRQ04: idx = 00 REG_RANK_NR(08, 00) = 0, REG_RANK_INDEX(08, 00) = 0, BYTE = 0
> IRQ05: idx = 00 REG_RANK_NR(08, 00) = 0, REG_RANK_INDEX(08, 00) = 0, BYTE = 1
> [...]
> IRQ30: idx = 00 REG_RANK_NR(08, 00) = 0, REG_RANK_INDEX(08, 00) = 0, BYTE = 2
> IRQ31: idx = 00 REG_RANK_NR(08, 00) = 0, REG_RANK_INDEX(08, 00) = 0, BYTE = 3
> IRQ32: idx = 01 REG_RANK_NR(08, 01) = 0, REG_RANK_INDEX(08, 01) = 1, BYTE = 0
> IRQ33: idx = 01 REG_RANK_NR(08, 01) = 0, REG_RANK_INDEX(08, 01) = 1, BYTE = 1
> [...]
> IRQ62: idx = 01 REG_RANK_NR(08, 01) = 0, REG_RANK_INDEX(08, 01) = 1, BYTE = 2
> IRQ63: idx = 01 REG_RANK_NR(08, 01) = 0, REG_RANK_INDEX(08, 01) = 1, BYTE = 3
> IRQ64: idx = 02 REG_RANK_NR(08, 02) = 0, REG_RANK_INDEX(08, 02) = 2, BYTE = 0
> IRQ65: idx = 02 REG_RANK_NR(08, 02) = 0, REG_RANK_INDEX(08, 02) = 2, BYTE = 1
> [...]
> IRQ255: idx = 07 REG_RANK_NR(08, 07) = 0, REG_RANK_INDEX(08, 07) = 7, BYTE = 3
> IRQ256: idx = 08 REG_RANK_NR(08, 08) = 1, REG_RANK_INDEX(08, 08) = 0, BYTE = 0
> 
> Here allinterrupts up to 255 are in rank 0, which is wrong.
> 
> For idx = irq / 32 you would need to use REG_RANK_NR(1, idx) not (8,
> idx). (and you'd need to trivially fix REG_RANK_NR to handle b == 1).
> 
> Perhaps the way to think of it is not as "/ 32" but rather as ">> 5". 
> Since REG_RANK_NR(8, ) is effectively >> 3 when combined with the
> existing >> 2 we get >> 5 overall.
> 
> If you change it as you have done then you are doing >> 5 *and* >> 3,
> i.e. >> 8 aka "/ 256" -- which explains why interrupts up to 255 end up
> in rank 0 with your change.

Your explanation is sounds, but the fact remains that the code doesn't
work as is :)
It means that this patch hides the bug rather than fixing it...

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