From patchwork Wed Apr 5 00:14:11 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alexander Graf X-Patchwork-Id: 9662755 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id DDC5360352 for ; Wed, 5 Apr 2017 00:15:29 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id CF96320564 for ; Wed, 5 Apr 2017 00:15:29 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id C2FE6275A2; Wed, 5 Apr 2017 00:15:29 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID autolearn=unavailable version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 7DDA120564 for ; Wed, 5 Apr 2017 00:15:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=qdBAS9/XX1iX0jvtYAiDTzV3cuMez3k7vZrdQJcuHlo=; b=PPGQIJ/+M7ADuAsjiNHngu7VO hDl2UvTuxhjD9hw7nnPawJF19iGe67lHQI7GmQltlz9tRxQRGzad3NHKv4jQNvKxQsfbJcBe2qwKX Z3Izbk+aSCBx1zDnLBukqI8Rt4d7RNJp5yK7IhT5mmfdPrO0uZSG5tk+RMPF/hyXTE1fxsxaoITfk NvOgOwvOhf9Jwj3Rju6WSzlyjMqtvT8N0fVG/o31OIxgCyXyEbVlOmUyA+p6WjVCEdZsnFCN984Av DHHYO2y8y4oUo73fazWVNpsKMr/4qFs3D2RfoawIbanCngUD7RS/3zY2nFVSLOkGb9PRtmlFmPAuB OchaL/GXw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1cvYbX-0001yC-V9; Wed, 05 Apr 2017 00:15:19 +0000 Received: from mx2.suse.de ([195.135.220.15]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1cvYax-0007ok-Dv for linux-arm-kernel@lists.infradead.org; Wed, 05 Apr 2017 00:15:00 +0000 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 435D4AAB4; Wed, 5 Apr 2017 00:14:14 +0000 (UTC) Subject: Re: [PATCH v2 2/5] KVM: arm/arm64: Add ARM user space interrupt signaling ABI To: Christoffer Dall References: <20170203145655.15007-1-cdall@linaro.org> <20170203145655.15007-3-cdall@linaro.org> <20170221114153.GA26976@cbox> From: Alexander Graf Message-ID: <5fe89e77-4fcc-f2e5-a796-a1e1f38eef9f@suse.de> Date: Wed, 5 Apr 2017 02:14:11 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170221114153.GA26976@cbox> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20170404_171445_379495_A0B4EC14 X-CRM114-Status: GOOD ( 19.61 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Peter Maydell , kvm-devel , Marc Zyngier , "kvmarm@lists.cs.columbia.edu" , Pekka Enberg , arm-mail-list Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP On 21.02.17 12:41, Christoffer Dall wrote: > Hi Alex, > > On Fri, Feb 03, 2017 at 05:51:18PM +0000, Peter Maydell wrote: >> On 3 February 2017 at 14:56, Christoffer Dall wrote: >>> From: Christoffer Dall >>> >>> We have 2 modes for dealing with interrupts in the ARM world. We can >>> either handle them all using hardware acceleration through the vgic or >>> we can emulate a gic in user space and only drive CPU IRQ pins from >>> there. >>> >>> Unfortunately, when driving IRQs from user space, we never tell user >>> space about events from devices emulated inside the kernel, which may >>> result in interrupt line state changes, so we lose out on for example >>> timer and PMU events if we run with user space gic emulation. >>> >>> Define an ABI to publish such device output levels to userspace. >>> >>> Signed-off-by: Alexander Graf >>> Signed-off-by: Christoffer Dall >> >> >> Acked-by: Peter Maydell >> >> for the userspace ABI/docs part. >> > > Given Peter's ack on this ABI, any chance you could take a look at > fixing the userspace SMP issue and respinning the userspace patches for > this series? The problem with user space SMP was simply a missing lock: ---- Once we solved that piece, I'll happily respin my user space patch. Alex diff --git a/target/arm/kvm.c b/target/arm/kvm.c index a66845f..1b33895 100644 --- a/target/arm/kvm.c +++ b/target/arm/kvm.c @@ -548,10 +548,14 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run) /* Synchronize our internal vtimer irq line with the kvm one */ if (run->s.regs.device_irq_level != cpu->device_irq_level) { - qemu_set_irq(ARM_CPU(cs)->gt_timer_outputs[GTIMER_VIRT], + qemu_mutex_lock_iothread(); + qemu_set_irq(cpu->gt_timer_outputs[GTIMER_VIRT], run->s.regs.device_irq_level & KVM_ARM_DEV_EL1_VTIMER); + qemu_set_irq(cpu->gt_timer_outputs[GTIMER_PHYS], + run->s.regs.device_irq_level & KVM_ARM_DEV_EL1_PTIMER); /* TODO: Handle changes in PTIMER and PMU as well */ cpu->device_irq_level = run->s.regs.device_irq_level; + qemu_mutex_unlock_iothread(); } return MEMTXATTRS_UNSPECIFIED; ---- I also wasn't able to use your series out of the box. There are several places in the code that check whether a timer is enabled (for register save/restore for example) and without vgic we never ended up setting that to true. I don't know how safe it is to set it to true regardless. It feels to me like someone has to put more thought into how to switch from an initialized user space timer state to a vgic one. For reference, my test patch is below: diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c index 891a725..56a5d51 100644 --- a/virt/kvm/arm/arch_timer.c +++ b/virt/kvm/arm/arch_timer.c @@ -629,8 +629,11 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu) return 0; /* Without a VGIC we do not map virtual IRQs to physical IRQs */ - if (!irqchip_in_kernel(vcpu->kvm)) + if (!irqchip_in_kernel(vcpu->kvm)) { + /* Enable timer for now, may be racy? */ + timer->enabled = 1; return 0; + } if (!vgic_initialized(vcpu->kvm)) return -ENODEV;