From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [Android-virt] [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace Date: Mon, 12 Dec 2011 16:50:29 +0200 Message-ID: <4EE614B5.3020003@redhat.com> References: <20111211102403.21693.6887.stgit@localhost> <20111211102449.21693.12265.stgit@localhost> <4EE60173.6060502@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Marc.Zyngier@arm.com, android-virt@lists.cs.columbia.edu, tech@virtualopensystems.com To: Christoffer Dall Return-path: Received: from mx1.redhat.com ([209.132.183.28]:43064 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752557Ab1LLOuo (ORCPT ); Mon, 12 Dec 2011 09:50:44 -0500 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On 12/12/2011 04:38 PM, Christoffer Dall wrote: > > > > Why don't they match? The assignment of lines to actual pins differs, > > but essentially it's the same thing (otherwise we'd use a different ioctl). > > > > because there is no notion of gsi and irq_source_id on ARM. gsi = number of irq line, just a bad name, but you do have it on ARM. irq_source_id really shouldn't have been in kvm_set_irq(), it's an implementation detail rather than an architectural feature; just ignore it. > What's the > harm of this additional tracepoint? If we get tools that use them, they have an additional difference to consider. It's a weak argument but it's there. > If I should re-use the existing one, should I simply move it outside > of __KVM_HAVE_IOAPIC? Protect it with __KVM_HAVE_IRQ_LINE so we don't leak unused tracepoints for other archs. > >> + trace_kvm_irq_line(irq_level->irq % 2, irq_level->level, vcpu_idx); > >> + > >> + if (irq_level->level) { > >> + vcpu->arch.virt_irq |= mask; > > > > racy - this is a vm ioctl, and so can (and usually is) invoked from > > outside the vcpu thread. > > > > this is taken care of in SMP host patch, but will be moved down the > patches for next revision. Yes please. It's hard to review this way. Fold all the smp stuff into the patches which introduce the functionality. > > >> + vcpu->arch.wait_for_interrupts = 0; > > > > Need an actual wakeup here (see x86's kvm_vcpu_kick() - should really be > > common code; it takes care of both the 'vcpu sleeping and needs a > > wakeup' and 'vcpu is in guest mode and needs to go to the host to > > evaluate interrupt state'). > > > > the wakeup - same as above. Good point that we need to signal the > other CPU. Will come, maybe not next revision, but the one after that. Ok. I think you can reuse x86 concepts and even code. I'll accept patches to make code arch independent ahead of the merge if it helps. -- error compiling committee.c: too many arguments to function