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: 9662753 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 9AA1D60352 for ; Wed, 5 Apr 2017 00:14:22 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 8AC10205AF for ; Wed, 5 Apr 2017 00:14:22 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 7B3E02793B; Wed, 5 Apr 2017 00:14:22 +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=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id CAD21205AF for ; Wed, 5 Apr 2017 00:14:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753227AbdDEAOQ (ORCPT ); Tue, 4 Apr 2017 20:14:16 -0400 Received: from mx2.suse.de ([195.135.220.15]:38989 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752657AbdDEAOQ (ORCPT ); Tue, 4 Apr 2017 20:14:16 -0400 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> Cc: "kvmarm@lists.cs.columbia.edu" , arm-mail-list , kvm-devel , Marc Zyngier , Pekka Enberg , Peter Maydell 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> Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.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;