From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [Android-virt] [PATCH v9 11/16] ARM: KVM: Inject IRQs and FIQs from userspace Date: Tue, 07 Aug 2012 17:28:36 +0300 Message-ID: <50212614.7090409@redhat.com> References: <20120703085841.27746.82730.stgit@ubuntu> <20120703090115.27746.55688.stgit@ubuntu> <50211F58.6080602@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: Christoffer Dall , kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, tech@virtualopensystems.com, Marc Zyngier To: Peter Maydell Return-path: Received: from mx1.redhat.com ([209.132.183.28]:3573 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754881Ab2HGO25 (ORCPT ); Tue, 7 Aug 2012 10:28:57 -0400 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On 08/07/2012 05:12 PM, Peter Maydell wrote: > On 7 August 2012 14:59, Avi Kivity wrote: >> On 08/06/2012 08:20 PM, Peter Maydell wrote: >>> On 3 July 2012 10:01, Christoffer Dall wrote: >>>> From: Christoffer Dall >>>> >>>> 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