Re: [Android-virt] [PATCH v9 11/16] ARM: KVM: Inject IRQs and FIQs from userspace

[Avi Kivity <avi@redhat.com>]

On 08/07/2012 05:12 PM, Peter Maydell wrote:
> On 7 August 2012 14:59, Avi Kivity <avi@redhat.com> wrote:
>> On 08/06/2012 08:20 PM, Peter Maydell wrote:
>>> On 3 July 2012 10:01, Christoffer Dall <c.dall@virtualopensystems.com> wrote:
>>>> From: Christoffer Dall <cdall@cs.columbia.edu>
>>>>
>>>> Userspace can inject IRQs and FIQs through the KVM_IRQ_LINE VM ioctl.
>>>> This ioctl is used since the sematics are in fact two lines that can be
>>>> either raised or lowered on the VCPU - the IRQ and FIQ lines.
>>>>
>>>> KVM needs to know which VCPU it must operate on and whether the FIQ or
>>>> IRQ line is raised/lowered. Hence both pieces of information is packed
>>>> in the kvm_irq_level->irq field. The irq fild value will be:
>>>>   IRQ: vcpu_index << 1
>>>>   FIQ: (vcpu_index << 1) | 1
>>>>
>>>> This is documented in Documentation/kvm/api.txt.
>>>
>>> It occurred to me that rather than encoding the CPU index in the IRQ
>>> field value, maybe we should just use the per-vcpu version of
>>> KVM_IRQ_LINE the same way we do for injecting the per-CPU lines
>>> of the in-kernel (V)GIC ?
>>
>> What do you mean by "per-vcpu version of KVM_IRQ_LINE"?
> 
> The ARM VGIC implementation implements "I need to raise per-CPU
> interrupt X" by providing a vcpu ioctl KVM_IRQ_LINE (this is
> in addition to the vm ioctl KVM_IRQ_LINE which it uses for
> "I need to raise the external interrupt X").
> The patch updating the API documentation is this one:
> https://lists.cs.columbia.edu/pipermail/kvmarm/2012-July/001206.html

Yikes.  "vm ioctl (and vcpu_ioctl on ARM)" is just horrible.  First, use
a different ioctl.  Second, vcpu ioctls are synchronous, you must call
them from a vcpu thread (think syscall operating on current).  You want
something asynchronous.

How about using the vm ioctl, with a range of numbers allocated for
per-processor interrupts?  In fact the documentation appears to say you
already did this.

> 
>>> (The subtext here is that it would be cool to have QEMU's
>>> generic interrupt handling code for KVM be able to say "if
>>> you do a cpu_interrupt()/cpu_reset_interrupt() and async
>>> interrupt delivery is enabled then just do the per-vcpu ioctl".
>>> Then there wouldn't need to be any kvm-specific code in
>>> hw/arm_pic.c at all...)
>>
>> If you mean "vcpu ioctl", then no, vcpu ioctls must be called from the
>> vcpu thread.
> 
> That's a shame, because it's the obvious interface for "do something
> to this specific CPU".

We can do something new if needed.  So far all vcpu operations were
synchronous (mostly, get/set state).

-- 
error compiling committee.c: too many arguments to function