From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH v5 05/13] ARM: KVM: Inject IRQs and FIQs from userspace Date: Mon, 12 Dec 2011 15:28:19 +0200 Message-ID: <4EE60173.6060502@redhat.com> References: <20111211102403.21693.6887.stgit@localhost> <20111211102449.21693.12265.stgit@localhost> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: android-virt@lists.cs.columbia.edu, kvm@vger.kernel.org, Marc.Zyngier@arm.com, catalin.marinas@arm.com, tech@virtualopensystems.com, peter.maydell@linaro.org To: Christoffer Dall Return-path: Received: from mx1.redhat.com ([209.132.183.28]:13191 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752997Ab1LLN2f (ORCPT ); Mon, 12 Dec 2011 08:28:35 -0500 In-Reply-To: <20111211102449.21693.12265.stgit@localhost> Sender: kvm-owner@vger.kernel.org List-ID: On 12/11/2011 12:24 PM, Christoffer Dall wrote: > 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 * 2 > FIQ: (vcpu_index * 2) + 1 > > This is documented in Documentation/kvm/api.txt. > > The effect of the ioctl is simply to simply raise/lower the > corresponding virt_irq field on the VCPU struct, which will cause the > world-switch code to raise/lower virtual interrupts when running the > guest on next switch. The wait_for_interrupt flag is also cleared for > raised IRQs causing an idle VCPU to become active again. > > Note: The custom trace_kvm_irq_line is used despite a generic definition of > trace_kvm_set_irq, since the trace-Kvm_set_irq depends on the x86-specific > define of __HAVE_IOAPIC. Either the trace event should be created > regardless of this define or it should depend on another ifdef clause, > common for both x86 and ARM. However, since the arguments don't really > match those used in ARM, I am yet to be convinced why this is necessary. 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). > > Capability: KVM_CAP_IRQCHIP > -Architectures: x86, ia64 > +Architectures: x86, ia64, arm > Type: vm ioctl > Parameters: struct kvm_irq_level > Returns: 0 on success, -1 on error > @@ -582,6 +582,14 @@ Requires that an interrupt controller model has been previously created with > KVM_CREATE_IRQCHIP. Note that edge-triggered interrupts require the level > to be set to 1 and then back to 0. > > +KVM_CREATE_IRQCHIP (except for ARM). Note that edge-triggered interrupts > +require the level to be set to 1 and then back to 0. Need to replace the previous line beginning with KVM_CREATE_IRQCHIP, not duplicate it. > + > +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. Userspace only includes ; also please name the macros here. > > +static int kvm_arch_vm_ioctl_irq_line(struct kvm *kvm, > + struct kvm_irq_level *irq_level) > +{ > + u32 mask; > + unsigned int vcpu_idx; > + struct kvm_vcpu *vcpu; > + > + vcpu_idx = irq_level->irq / 2; > + if (vcpu_idx >= KVM_MAX_VCPUS) > + return -EINVAL; > + > + vcpu = kvm_get_vcpu(kvm, vcpu_idx); > + if (!vcpu) > + return -EINVAL; > + > + switch (irq_level->irq % 2) { > + case KVM_ARM_IRQ_LINE: > + mask = HCR_VI; > + break; > + case KVM_ARM_FIQ_LINE: > + mask = HCR_VF; > + break; > + default: > + return -EINVAL; > + } > + > + 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. > + 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'). > + } else > + vcpu->arch.virt_irq &= ~mask; > + > + return 0; > +} > + > -- error compiling committee.c: too many arguments to function