On Sun, Dec 11, 2011 at 3:25 PM, Peter Maydell <peter.maydell@xxxxxxxxxx> wrote: > On 11 December 2011 20:07, Christoffer Dall > <christofferdall@xxxxxxxxxxxxxxxxxx> wrote: >> Well, if it was just "irq & 1", then I hear you, but it would be "(irq >> >> cpu_idx) & 1" which I don't think is more clear. > > Er, what? The fields are [31..1] CPU index and [0] irqtype, > right? So what you have now is: > vcpu_idx = irq_level->irq / 2; > irqtype = irq_level->irq % 2; > and the bitshifting equivalent is: > vcpu_idx = irq_level->irq >> 1; > irqtype = irq_level->irq & 1; > surely? > > Shifting by the cpuindex is definitely wrong. actually, that's not how the irq_level field is defined. If you look in Documentation/virtual/kvm/api.txt: "ARM uses two types of interrupt lines per CPU, ie. IRQ and FIQ. The value of the irq field should be (VCPU_INDEX * 2) for IRQs and ((VCPU_INDEX * 2) + 1) for FIQs. Level is used to raise/lower the line. See arch/arm/include/asm/kvm.h for convenience macros." also, in the kernel code the cpu_index is achieved by a simple integer division by 2. as I said, this was the proposal from the last round of reviews after a lengthy discussion, so I sticked with that. we should definitely fix either side, and the only sane argument is that this is an irq_line field, so an index resembling an actual line seems more semantically in line with the field purpose rather than a bit encoding, but I am open to arguments and not married to the current implementation. > (Incidentally I fixed a bug in your QEMU-side code which wasn't > feeding this field to the kernel in the way it expects: > > http://git.linaro.org/gitweb?p=qemu/qemu-linaro.git;a=commitdiff;h=2502ba067e795e48d346f9816fad45177ca64bca > > Sorry, I should have posted that to the list. I'll do that now.) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
