On 11 December 2011 21:36, Christoffer Dall <c.dall@xxxxxxxxxxxxxxxxxxxxxx> wrote: > 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. It's not clear to me which part of my comment this is aimed at. Shifting by the cpuindex doesn't give the right answer whether you define irq_level by bitfields or with the current phrasing you quote below. > 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." That's exactly the same thing, though, right? It's just a matter of how you choose to phrase it (in either text or in code; the values come out identical). When I was sorting out the QEMU side, I started out by looking at the kernel source code, deduced that we were encoding CPU number and irq-vs-fiq as described above (and documenting it in a slightly confusing way as a multiplication) and then wrote the qemu code in what seemed to me the clearest way. (Actually what would be clearest would be if the ioctl took the (interrupt-target, interrupt-line-for-that-target, value-of-line) tuple as three separate values rather than encoding two of them into a single integer, but I assume there's a reason we can't have 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. To be clear, I'm not attempting to suggest a change in the semantics of this field. (The qemu patch fixes the qemu side to adhere to what the kernel requires.) -- PMM -- 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
