From patchwork Fri Dec 12 20:24:59 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christoffer Dall X-Patchwork-Id: 42198 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-la0-f71.google.com (mail-la0-f71.google.com [209.85.215.71]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id E866D26C77 for ; Fri, 12 Dec 2014 20:26:08 +0000 (UTC) Received: by mail-la0-f71.google.com with SMTP id q1sf5425862lam.6 for ; Fri, 12 Dec 2014 12:26:07 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:date:from:to:subject:message-id :references:mime-version:content-disposition:in-reply-to:user-agent :cc:precedence:list-id:list-unsubscribe:list-archive:list-post :list-help:list-subscribe:content-type:content-transfer-encoding :sender:errors-to:x-original-sender :x-original-authentication-results:mailing-list; bh=ZstVehqoJFHaHto61Yo17LnTUosEGWpQmgZVZ566wVI=; b=ZeFupIe3N8jmTV14c1zMdP+ANNLN09hN8DIS07WBR4RasDlCaptJcLxQdNJB85LDOv RXFv06yevkT99dI8quF1FyMX+1C8awgcy6JVlfE8IT/pewtrKsbs9fWkvVhMiwaFogmW KRE4KoC4zgJUnfIS5O6NyZSnxFfzYJra96BdA3v198cjEarf6FY0gbyqt+vf0IU4QfUH 9Ca+17kjnKsfoxRqJCEYA/hufJgXaTufXY4dblgjHvkeXi0UZSIfMxWX1xJhv6a8dN+s mNlKNyL3VQRLOUtmwq4/wWrxt5OwFxI8MSytZVndx0rYkQH+gA2d9F5h7j9v5D7fxLzt XoSA== X-Gm-Message-State: ALoCoQllceRLFV8Y2X0lUd8DhfaF3fK59m+zkXDHdqhUhwHnOItCboQz1APA8tBTiXHuW5VXhdF/ X-Received: by 10.152.10.233 with SMTP id l9mr2880878lab.0.1418415967760; Fri, 12 Dec 2014 12:26:07 -0800 (PST) X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.8.170 with SMTP id s10ls496660laa.72.gmail; Fri, 12 Dec 2014 12:26:07 -0800 (PST) X-Received: by 10.152.203.201 with SMTP id ks9mr3756151lac.57.1418415967567; Fri, 12 Dec 2014 12:26:07 -0800 (PST) Received: from mail-lb0-f180.google.com (mail-lb0-f180.google.com. [209.85.217.180]) by mx.google.com with ESMTPS id c4si2507569lbv.53.2014.12.12.12.26.07 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Fri, 12 Dec 2014 12:26:07 -0800 (PST) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.217.180 as permitted sender) client-ip=209.85.217.180; Received: by mail-lb0-f180.google.com with SMTP id l4so6598460lbv.11 for ; Fri, 12 Dec 2014 12:26:07 -0800 (PST) X-Received: by 10.152.87.12 with SMTP id t12mr5089935laz.31.1418415967394; Fri, 12 Dec 2014 12:26:07 -0800 (PST) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.112.142.69 with SMTP id ru5csp86152lbb; Fri, 12 Dec 2014 12:26:06 -0800 (PST) X-Received: by 10.70.134.229 with SMTP id pn5mr29991646pdb.164.1418415965378; Fri, 12 Dec 2014 12:26:05 -0800 (PST) Received: from bombadil.infradead.org (bombadil.infradead.org. [2001:1868:205::9]) by mx.google.com with ESMTPS id fk4si3599358pab.38.2014.12.12.12.26.04 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 12 Dec 2014 12:26:05 -0800 (PST) Received-SPF: none (google.com: linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org does not designate permitted sender hosts) client-ip=2001:1868:205::9; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1XzWlJ-0002dG-1j; Fri, 12 Dec 2014 20:24:29 +0000 Received: from mail-lb0-f170.google.com ([209.85.217.170]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1XzWlF-0002HI-A3 for linux-arm-kernel@lists.infradead.org; Fri, 12 Dec 2014 20:24:26 +0000 Received: by mail-lb0-f170.google.com with SMTP id 10so6579747lbg.1 for ; Fri, 12 Dec 2014 12:23:59 -0800 (PST) X-Received: by 10.152.120.42 with SMTP id kz10mr17470525lab.92.1418415839844; Fri, 12 Dec 2014 12:23:59 -0800 (PST) Received: from localhost (188-178-240-98-static.dk.customer.tdc.net. [188.178.240.98]) by mx.google.com with ESMTPSA id r6sm660630laa.31.2014.12.12.12.23.59 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Fri, 12 Dec 2014 12:23:59 -0800 (PST) Date: Fri, 12 Dec 2014 21:24:59 +0100 From: Christoffer Dall To: Marc Zyngier Subject: Re: [PATCH 5/5] arm/arm64: KVM: Initialize the vgic on-demand when injecting IRQs Message-ID: <20141212202459.GB16855@cbox> References: <1418139844-27892-1-git-send-email-christoffer.dall@linaro.org> <1418139844-27892-6-git-send-email-christoffer.dall@linaro.org> <5489E3FC.2000304@arm.com> <20141212111411.GJ28388@cbox> <548AD037.4060000@arm.com> <20141212113752.GK28388@cbox> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20141212113752.GK28388@cbox> User-Agent: Mutt/1.5.21 (2010-09-15) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20141212_122425_538236_9AF23796 X-CRM114-Status: GOOD ( 32.13 ) X-Spam-Score: -0.7 (/) X-Spam-Report: SpamAssassin version 3.4.0 on bombadil.infradead.org summary: Content analysis details: (-0.7 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [209.85.217.170 listed in list.dnswl.org] -0.0 RCVD_IN_MSPIKE_H3 RBL: Good reputation (+3) [209.85.217.170 listed in wl.mailspike.net] -0.0 SPF_PASS SPF: sender matches SPF record -0.0 RCVD_IN_MSPIKE_WL Mailspike good senders Cc: "kvmarm@lists.cs.columbia.edu" , "linux-arm-kernel@lists.infradead.org" , "kvm@vger.kernel.org" X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: , List-Help: , List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patch=linaro.org@lists.infradead.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: christoffer.dall@linaro.org X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.217.180 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 On Fri, Dec 12, 2014 at 12:37:52PM +0100, Christoffer Dall wrote: > On Fri, Dec 12, 2014 at 11:23:35AM +0000, Marc Zyngier wrote: > > On 12/12/14 11:14, Christoffer Dall wrote: > > > On Thu, Dec 11, 2014 at 06:35:40PM +0000, Marc Zyngier wrote: > > >> On 09/12/14 15:44, Christoffer Dall wrote: > > >>> Userspace assumes that it can wire up IRQ injections after having > > >>> created all VCPUs and after having created the VGIC, but potentially > > >>> before starting the first VCPU. This can currently lead to lost IRQs > > >>> because the state of that IRQ injection is not stored anywhere and we > > >>> don't return an error to userspace. > > >>> > > >>> We haven't seen this problem manifest itself yet, presumably because > > >>> guests reset the devices on boot, but this could cause issues with > > >>> migration and other non-standard startup configurations. > > >>> > > >>> Signed-off-by: Christoffer Dall > > >>> --- > > >>> virt/kvm/arm/vgic.c | 9 +++++++-- > > >>> 1 file changed, 7 insertions(+), 2 deletions(-) > > >>> > > >>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c > > >>> index c98cc6b..feef015 100644 > > >>> --- a/virt/kvm/arm/vgic.c > > >>> +++ b/virt/kvm/arm/vgic.c > > >>> @@ -1693,8 +1693,13 @@ out: > > >>> int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num, > > >>> bool level) > > >>> { > > >>> - if (likely(vgic_ready(kvm)) && > > >>> - vgic_update_irq_pending(kvm, cpuid, irq_num, level)) > > >>> + if (unlikely(!vgic_initialized(kvm))) { > > >>> + mutex_lock(&kvm->lock); > > >>> + vgic_init(kvm); > > >> > > >> What if this fails? > > >> > > > yeah, not good. The thing is that we also don't check the return value > > > from kvm_vgic_inject_irq(), so we can do two things: > > > > > > (1) change this function to a void, carry out the check for > > > vgic_initialized in kvm_vm_ioctl_irq_line() in arm.c and export > > > vgic_init() outside of vgic.c. > > > > > > (2) just error out if vgic_init() fails and print a kernel error (or > > > even a BUG_ON?) in kvm_timer_inject_irq() in arch_timer.c. > > > > > > In both cases we need to make sure that we never configure the timer to > > > begin injecting IRQs before the vgic is initialized, as Eric pointed out > > > before. > > > > > > What do you think? > > > > I'd favour option two. > > > > My reasoning is that the timer interrupt is triggered by the HW. If it > > has fired, that's because we've programmed it to trigger, with means a > > vcpu has run. At that point, the vgic would better be initialized, or we > > have something much nastier on our hands. > > > Sounds reasonable to me, I'll do a quick respin with the check for the > timer (to ensure the user even created a vgic). > Just to double-check, it is going to look something like this for the arch-timer path: diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index d4da244..c61d51d 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -441,6 +441,16 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu) return ret; } +#ifdef CONFIG_KVM_ARM_TIMER + /* + * If the Architected Timers are supported, userspace must have + * created an in-kernel irqchip, since otherwise we will receive + * virtual timer interrupt and have nowhere to route them to. + */ + if (!irqchip_in_kernel(kvm)) + return -ENODEV; +#endif + return 0; } diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c index 22fa819..b10e495 100644 --- a/virt/kvm/arm/arch_timer.c +++ b/virt/kvm/arm/arch_timer.c @@ -61,12 +61,14 @@ static void timer_disarm(struct arch_timer_cpu *timer) static void kvm_timer_inject_irq(struct kvm_vcpu *vcpu) { + int ret; struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu; timer->cntv_ctl |= ARCH_TIMER_CTRL_IT_MASK; - kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id, - timer->irq->irq, - timer->irq->level); + ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id, + timer->irq->irq, + timer->irq->level); + BUG_ON(ret); } static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id)