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

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

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?

--
Alex Benn?e