From patchwork Wed Oct 4 10:08:56 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Alex_Benn=C3=A9e?= X-Patchwork-Id: 9984135 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 3B616602B8 for ; Wed, 4 Oct 2017 10:09:32 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 2BCCA27CAF for ; Wed, 4 Oct 2017 10:09:32 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 2040628173; Wed, 4 Oct 2017 10:09:32 +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=-4.2 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_MED autolearn=ham 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 0ABE127CAF for ; Wed, 4 Oct 2017 10:09:31 +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-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:Message-ID:Date: In-reply-to:Subject:To:From:References:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Owner; bh=XAa5zJgqScKMn7+5NXXzc4WXTNMz4sZMQDtpBhpbMKo=; b=cgvZNYrpeW8s2X3zCwC/ggFhUl v4w7MVSLacaBCbiRXg1CO0H0EFt0qBkCw4JwaFsc7Ea1h3yqVtqPvRDPHUozJRWpxdliIE48KXioA fd4RH9fmOKYSa/W6cWi0ooetNXscbonsPt5xc+LnA+QiM1g/DodWaaarJgkGbo1tELKJhCbwNko/L bQ4X1OHC5ICJnRg+zMnR1qj5/6qqB8Q9k1gPX3bhhNfmtep6uOWoyYF3DBMul0ZXB62anmSmN5UXb etKmBeNM/ffPHfSMfy5aHC9pKtLS9MFzSOJAglITxpq8QhTpBa02DZrDeKhR0UlqL0kIqYhmPBTPS IOGrsIzg==; 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 1dzgcJ-0003TC-PO; Wed, 04 Oct 2017 10:09:27 +0000 Received: from mail-wr0-x22a.google.com ([2a00:1450:400c:c0c::22a]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1dzgcC-0003Oa-6E for linux-arm-kernel@lists.infradead.org; Wed, 04 Oct 2017 10:09:26 +0000 Received: by mail-wr0-x22a.google.com with SMTP id j14so8313193wre.8 for ; Wed, 04 Oct 2017 03:08:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=references:user-agent:from:to:cc:subject:in-reply-to:date :message-id:mime-version:content-transfer-encoding; bh=nWVYu6be4Ki4pmDug5Er7INdQYdPORIcYV7EY08d3tU=; b=SdFQf9pj0Mznqt+AMkSlPic2wK578RTGSRRe+J+7p3PxL2vFhcAuVFDazvIYO7aG0r bl1cIg7w8pbT2vIxH7qfGUH6++Qzg5uoIUJReGpHSQNMliFR31DEll8jThRy7wXTaWSY hxHi/bVCigD451uhoC8qrSptfnafZro1ubzSw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:references:user-agent:from:to:cc:subject :in-reply-to:date:message-id:mime-version:content-transfer-encoding; bh=nWVYu6be4Ki4pmDug5Er7INdQYdPORIcYV7EY08d3tU=; b=DAoIDVa1PDWNJzrtCrMntuwXKh+aDBITNSwwBLPZ2swAnE4itP6+870cQpAQdmeFsM 77pWFzEBrbCgpjlScY9e5VurjbxMwo9oTKWYWw0U8oVRDI1sRI2deGeM0FrPeyG4nkgN q0vhnmwylGBpTkVyR9tdan0bLSDJgvm4cI67bxj+5McPPLF0T3EJBfkoEnSIwwscPXap 0jwJEH9hJ3BE46fBTeRXIOE/aFQOUbRGUsfXby0nVNyldPbxENFnS9kgqpVrG8wgaU3I CEcYVQ0CllL/jMIfYLuIs3/Y1yBWuTC3+Ot+BqBAYTAxfYm7AxYoGH2G8LDMFuL8YYs6 lDxA== X-Gm-Message-State: AMCzsaUOePXVTM4FhYMKa/wtyfQjxZzmLRVzsc25Pbozx5LwBiIogRIB lpyfq3m3d6qRM1A1LVY+rG78ww== X-Google-Smtp-Source: AOwi7QC5Fnah6j3R3Isr8R/ZU9clWejGJI/cuFDdJFJKMwv2keplpm/9bZ/LsuOKG/O6dMJ44v6ojg== X-Received: by 10.223.131.226 with SMTP id 89mr2836421wre.122.1507111738016; Wed, 04 Oct 2017 03:08:58 -0700 (PDT) Received: from zen.linaro.local ([81.128.185.34]) by smtp.gmail.com with ESMTPSA id z192sm16794939wmz.28.2017.10.04.03.08.57 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 04 Oct 2017 03:08:57 -0700 (PDT) Received: from zen (localhost [127.0.0.1]) by zen.linaro.local (Postfix) with ESMTPS id AC4863E0191; Wed, 4 Oct 2017 11:08:56 +0100 (BST) References: <1504083688-48334-1-git-send-email-julien.thierry@arm.com> <1504083688-48334-4-git-send-email-julien.thierry@arm.com> <3c249a68-45e3-a3a5-7d05-4cfc2d97713b@arm.com> <3d7d2b36-da2f-04dc-611e-d7aab7666c29@arm.com> <9bc5abc2-ab03-3137-82bd-e8afa62624eb@arm.com> <861b4e4f-0fbe-cbc6-39ad-4660065449de@arm.com> <877ewcz3bv.fsf@linaro.org> <4d9fc0a2-bcf9-ca26-8646-037c2dcc6545@arm.com> <873770yz1o.fsf@linaro.org> <5a4f0493-6d3d-85de-fc45-030d4b03b5a8@arm.com> <871smkywgn.fsf@linaro.org> User-agent: mu4e 0.9.19; emacs 26.0.60 From: Alex =?utf-8?Q?Benn=C3=A9e?= To: Julien Thierry Subject: Re: [PATCH 3/3] arm64: kvm: Fix single step for guest skipped instructions In-reply-to: Date: Wed, 04 Oct 2017 11:08:56 +0100 Message-ID: <87k20bxm13.fsf@linaro.org> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20171004_030920_541998_8F2DB36E X-CRM114-Status: GOOD ( 31.10 ) 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: Marc Zyngier , Catalin Marinas , Will Deacon , Christoffer Dall , Paolo Bonzini , "linux-arm-kernel@lists.infradead.org" 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 [Added Paolo, including QEMU userspace patch] Julien Thierry writes: > On 03/10/17 18:26, Alex Bennée wrote: >> >> Julien Thierry writes: >> >>> On 03/10/17 17:30, Alex Bennée wrote: >>>> >>>> Julien Thierry writes: >>>> >>>>> On 03/10/17 15:57, Alex Bennée wrote: >>>>>> >>>>>> Julien Thierry writes: >>>>>> >>>>>>> On 31/08/17 15:01, Christoffer Dall wrote: >>>> >>>>>>>>>>> >>>>>>>>>>> Does that sound like what you had in mind? Or does it seem better than >>>>>>>>>>> the >>>>>>>>>>> current patch? >>>>>>>>>>> >>>>>>>>>> I was thinking to change the skip_instruction function to return an >>>>>>>>>> int, and then call kvm_handle_debug_ss() from skip_instruction, which >>>>>>>>>> would update the kvm_run structure and exit here and then. >>>>>>>>>> >>>>>>>>> >>>>>>>>> Setting up the debug exception from within kvm_skip_instruction seem to >>>>>>>>> change a bit too much its semantic from arm to arm64. I would find this >>>>>>>>> easily confusing. >>>>>>>>> >>>>>>>>>> However, I'm now thinking that this doesn't really work either, >>>>>>>>>> because we could have to emulate a trapped MMIO instruction in user >>>>>>>>>> space, and then it's not clear how to exit with a debug exception at >>>>>>>>>> the same time. >>>>>> >>>>>> A debug exception at guest exit point is (IIRC) just having the >>>>>> appropriate status in the run->exit_reason (KVM_EXIT_DEBUG). If you need >>>>>> to exit for MMIO emulation (i.e. the instruction has not run yet) you >>>>>> shouldn't do that. Exit, emulate and return. We could handle the ioctl >>>>>> to clear SS in userspace but I guess that gets just as messy. >>>>>> >>>>>>>>>> >>>>>>>>>> So perhaps we should stick with your original approach. >>>>>>>>>> >>>>>>>>> >>>>>>>>> I had not realized that was possible. This makes things more complicated for >>>>>>>>> avoiding a back and forth with the guest for trapped exceptions. Out of >>>>>>>>> luck, having the debug flag does look like single stepping would work as >>>>>>>>> expected for userland MMIOs. >>>> >>>> >>>> >>>> This is my currently untested but otherwise simpler solution: >>>> >>>> From 46ea80d7dc9b98661fcd51c41090f8ad74a6690f Mon Sep 17 00:00:00 2001 >>>> From: =?UTF-8?q?Alex=20Benn=C3=A9e?= >>>> Date: Tue, 3 Oct 2017 17:17:15 +0100 >>>> Subject: [PATCH] KVM: arm64: handle single-stepping trapped instructions >>>> MIME-Version: 1.0 >>>> Content-Type: text/plain; charset=UTF-8 >>>> Content-Transfer-Encoding: 8bit >>>> >>>> If we are using guest debug to single-step the guest we need to ensure >>>> we exit after emulating the instruction. This only affects >>>> instructions emulated by the kernel. If we exit to userspace anyway we >>>> leave it to userspace to work out what to do. >>>> >>>> Signed-off-by: Alex Bennée >>>> --- >>>> arch/arm64/kvm/handle_exit.c | 51 ++++++++++++++++++++++++++++++++------------ >>>> 1 file changed, 37 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c >>>> index 7debb74843a0..b197ffb10e96 100644 >>>> --- a/arch/arm64/kvm/handle_exit.c >>>> +++ b/arch/arm64/kvm/handle_exit.c >>>> @@ -178,6 +178,42 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu) >>>> return arm_exit_handlers[hsr_ec]; >>>> } >>>> >>>> +/* >>>> + * When handling traps we need to ensure exit the guest if we >>>> + * successfully emulated the instruction while single-stepping. If we >>>> + * have to exit anyway for userspace emulation then it's up to >>>> + * userspace to handle the "while SSing case". >>>> + */ >>>> + >>> >>> I have not tested the code but if it work we also need to do something >>> similar for MMIOs that are handled by the kernel (without returning to >>> userland). But it should be pretty similar. >> >> >> Which path do they take to the mmio emulation? >> > > Nevermind, I was mistaken, mmio code will get called by exit_handler > and "handled" will be true if the mmio was handled by KVM. So your > patch already handles this. > > There is also the case of GIC CPU inteface accesses being trapped > (which shouldn't be the default behaviour). If the vgic access fails, > we will skip the instruction (in __kvm_vcpu_run in > arch/arm64/kvm/hyp/switch.c) and if we were single stepping we will > single step 2 instructions. But this seems to be a corner case of a > corner case (GIC CPUIF trapped + access failing + single stepping), so > I don't know if we want to take that into account right now? Yeah looking at the hyp code I did wonder if it warranted the complexity of adding handling there. > I'm still a bit concerned about the change of semantics for > KVM_EXIT_MMIO with regards to userland (cf. my previous mail). But if > this is deemed to be a reasonable change, the patch seems fine to me. Have we changed the semantics? A normal un-handled by the kernel IO/MMIO exit needs to be processed before the single step takes effect. I can't speak for other userspace but I think for QEMU it is as simple as the patch bellow. That said it wasn't quite clear where the PC gets updated in this path - I think the kernel updates the PC before the KVM_EXIT_MMIO in the same path as the internal handling. I'd like to test these patches on some real life examples. I tried tracing over the pl011_write code in a kernel boot but I ran into the problem of el1_irq's occurring as I tried to step through the guest kernel. Is this something you've come across? What MMIO accesses have you been using in your testing? QEMU Patch bellow: From 2e8fcea695a9eca67fbeb331d3104d1d9e7e337a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Benn=C3=A9e?= Date: Wed, 4 Oct 2017 09:49:41 +0000 Subject: [PATCH] kvm: exit run loop after emulating IO when single stepping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If single-stepping is enabled we should exit the run-loop after emulating the access. Otherwise single-stepping across emulated IO accesses may skip an instruction. This only addresses user-space emulation. Stuff done in kernel-mode should be handled there. Signed-off-by: Alex Bennée --- accel/kvm/kvm-all.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 2.14.1 > > Thanks, -- Alex Bennée diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index 90c88b517d..85bcb2b0d4 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -1940,7 +1940,7 @@ int kvm_cpu_exec(CPUState *cpu) run->io.direction, run->io.size, run->io.count); - ret = 0; + ret = cpu->singlestep_enabled ? EXCP_DEBUG : 0; break; case KVM_EXIT_MMIO: DPRINTF("handle_mmio\n"); @@ -1950,7 +1950,7 @@ int kvm_cpu_exec(CPUState *cpu) run->mmio.data, run->mmio.len, run->mmio.is_write); - ret = 0; + ret = cpu->singlestep_enabled ? EXCP_DEBUG : 0; break; case KVM_EXIT_IRQ_WINDOW_OPEN: DPRINTF("irq_window_open\n");