From mboxrd@z Thu Jan 1 00:00:00 1970 From: alex.bennee@linaro.org (Alex =?utf-8?Q?Benn=C3=A9e?=) Date: Wed, 04 Oct 2017 11:08:56 +0100 Subject: [PATCH 3/3] arm64: kvm: Fix single step for guest skipped instructions In-Reply-To: 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> Message-ID: <87k20bxm13.fsf@linaro.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org [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(-) 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"); -- 2.14.1 > > Thanks, -- Alex Benn?e