[PATCH 3/3] arm64: kvm: Fix single step for guest skipped instructions

[alex.bennee@linaro.org (Alex Bennée)]

[Added Paolo, including QEMU userspace patch]

Julien Thierry <julien.thierry@arm.com> writes:

> On 03/10/17 18:26, Alex Benn?e wrote:
>>
>> Julien Thierry <julien.thierry@arm.com> writes:
>>
>>> On 03/10/17 17:30, Alex Benn?e wrote:
>>>>
>>>> Julien Thierry <julien.thierry@arm.com> writes:
>>>>
>>>>> On 03/10/17 15:57, Alex Benn?e wrote:
>>>>>>
>>>>>> Julien Thierry <julien.thierry@arm.com> writes:
>>>>>>
>>>>>>> On 31/08/17 15:01, Christoffer Dall wrote:
>>>> <snip>
>>>>>>>>>>>
>>>>>>>>>>> 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.
>>>>
>>>> <snip>
>>>>
>>>> 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?= <alex.bennee@linaro.org>
>>>> 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 <alex.bennee@linaro.org>
>>>> ---
>>>>    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.
>> <snip>
>>
>> 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?= <alex.bennee@linaro.org>
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 <alex.bennee@linaro.org>
---
 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