Re: [Android-virt] [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux