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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.

Perhaps this could be made clearer by renaming vgic_irq_rank to
vgic_register_rank? Then perhaps as a helper:
#define VGIC_IRQ_RANK(v, irq) vgic_register_rank(v, 1, irq/32) for the
case of looking up a rank from an irq rather than a register offset?

Ian.



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