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

[julien.thierry@arm.com (Julien Thierry)]

On 31/08/17 14:28, Christoffer Dall wrote:
> On Thu, Aug 31, 2017 at 2:56 PM, Julien Thierry <julien.thierry@arm.com> wrote:
>>
>>
>> On 31/08/17 11:53, Christoffer Dall wrote:
>>>
>>> On Thu, Aug 31, 2017 at 11:37 AM, Julien Thierry <julien.thierry@arm.com>
>>> wrote:
>>>>
>>>>
>>>>
>>>> On 31/08/17 09:54, Christoffer Dall wrote:
>>>>>
>>>>>
>>>>> On Thu, Aug 31, 2017 at 10:45 AM, Julien Thierry
>>>>> <julien.thierry@arm.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> Hi Christoffer,
>>>>>>
>>>>>>
>>>>>> On 30/08/17 19:53, Christoffer Dall wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Hi Julien,
>>>>>>>
>>>>>>> [cc'ing Alex Benn?e here who wrote the debug code for arm64]
>>>>>>>
>>>>>>> On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry
>>>>>>> <julien.thierry@arm.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Software Step exception is missing after trapping instruction from
>>>>>>>> the
>>>>>>>> guest.
>>>>>>>>
>>>>>>>> We need to set the PSR.SS to 0 for the guest vcpu before resuming
>>>>>>>> guest
>>>>>>>> execution.
>>>>>>>>
>>>>>>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>>>>>>>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>>>>>>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>>>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>>>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>>>>>>
>>>>>>>> ---
>>>>>>>>      arch/arm64/include/asm/kvm_asm.h     |  2 ++
>>>>>>>>      arch/arm64/include/asm/kvm_emulate.h |  2 ++
>>>>>>>>      arch/arm64/kvm/debug.c               | 12 +++++++++++-
>>>>>>>>      arch/arm64/kvm/hyp/switch.c          | 12 ++++++++++++
>>>>>>>>      4 files changed, 27 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/arch/arm64/include/asm/kvm_asm.h
>>>>>>>> b/arch/arm64/include/asm/kvm_asm.h
>>>>>>>> index 26a64d0..398bbaa 100644
>>>>>>>> --- a/arch/arm64/include/asm/kvm_asm.h
>>>>>>>> +++ b/arch/arm64/include/asm/kvm_asm.h
>>>>>>>> @@ -32,6 +32,8 @@
>>>>>>>>
>>>>>>>>      #define KVM_ARM64_DEBUG_DIRTY_SHIFT    0
>>>>>>>>      #define KVM_ARM64_DEBUG_DIRTY          (1 <<
>>>>>>>> KVM_ARM64_DEBUG_DIRTY_SHIFT)
>>>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT        1
>>>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP      (1 <<
>>>>>>>> KVM_ARM64_DEBUG_INST_SKIP_SHIFT)
>>>>>>>>
>>>>>>>>      #define kvm_ksym_ref(sym)
>>>>>>>> \
>>>>>>>>             ({
>>>>>>>> \
>>>>>>>> diff --git a/arch/arm64/include/asm/kvm_emulate.h
>>>>>>>> b/arch/arm64/include/asm/kvm_emulate.h
>>>>>>>> index fe39e68..d401c64 100644
>>>>>>>> --- a/arch/arm64/include/asm/kvm_emulate.h
>>>>>>>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>>>>>>>> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu
>>>>>>>> *vcpu, bool is_wide_instr)
>>>>>>>>                     kvm_skip_instr32(vcpu, is_wide_instr);
>>>>>>>>             else
>>>>>>>>                     *vcpu_pc(vcpu) += 4;
>>>>>>>> +       /* Let debug engine know we skipped an instruction */
>>>>>>>> +       vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP;
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Why do we need to defer this action until later?
>>>>>>>
>>>>>>> Can't we simply do clear DBG_SPSR_SS here?
>>>>>>>
>>>>>>
>>>>>> That was my first intention, but it turns out that the current state of
>>>>>> things (without this patch) is that every time we enter a guest,
>>>>>> kvm_arm_setup_debug gets called and if single step is requested for the
>>>>>> guest it will set the flag in the SPSR (ignoring the fact that we
>>>>>> cleared
>>>>>> it).
>>>>>
>>>>>
>>>>>
>>>>> Ah, right, duh.
>>>>>
>>>>>> This happens even if we exit the guest because of a data abort.
>>>>>>
>>>>>> For normal single step execution, we do need to reset SPSR.SS to 1
>>>>>> before
>>>>>> running the guest since completion of a step should clear that bit
>>>>>> before
>>>>>> taking a software step exception. So what kvm_arm_setup_debug does
>>>>>> seems
>>>>>> correct to me but missed the case for trapped/emulated instructions.
>>>>>>
>>>>>> So even if we just clear DBG_SPSR_SS here, we would still need to tell
>>>>>> kvm_arm_setup_debug not to change the bit. Or resetting SPSR.SS to 1
>>>>>> for
>>>>>> normal single stepping needs to be done before we skip instructions in
>>>>>> KVM
>>>>>> but that doesn't sound right to me...
>>>>>>
>>>>>
>>>>> So I'm wondering if we're going about this wrong.  Perhaps we need to
>>>>> discover at the end of the run loop that we were asked to single step
>>>>> execution and simply return to userspace, setting the debug exit
>>>>> reason etc., instead of entering the guest with PSTATE.SS==0 and
>>>>> relying on another trap back in to the guest just to set two fields on
>>>>> the kvm_run structure and exit to user space ?
>>>>>
>>>>
>>>> So if I understand correctly, the suggestion is that when we trap an
>>>> instruction we check whether it was supposed to be single stepped, if it
>>>> was
>>>> we set up the vcpu registers as if it had taken a software step exception
>>>> and return from kvm_arch_vcpu_ioctl_run. Is that right?
>>>
>>>
>>> yes, that's the idea.  If there's a lot of complexity in setting up
>>> CPU register state, then it may not be a good idea, but if it's
>>> relatively clean, I think it can be preferred over the "let's keep a
>>> flag aroudn for later" approach.
>>>
>>
>> So I looked a bit into it.
>>
>> One annoying thing is that the single step mechanic is specific to arm64.
>> MMU and MMIO code is shared between arm and arm64 and do some handling of
>> traps.
>>
>> So cleanest way I can think of doing this would be to clear SPSR.SS in
>> arm64::kvm_skip_instr, then have some function (e.g.
>> kvm_handle/manage_debug_state) at the end of the run loop. For arm, the
>> function is empty. For arm64, the function,  if we are in an active pending
>> state (SPSR.D == 0 && SPSR.SS == 0 && MDSCR_EL1.SS == 1) and not about to
>> return to userland, returns with a "fake debug exception".
>>
>> So instead of a flag, we would just use SPSR.SS (or more generally the vcpu
>> state) to know if we need to exit with a debug exception or not. And the
>> kvm_arm_setup_debug would be left untouched (always setting SPSR.SS when
>> requested by userland).
>>
>> 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.
> 
> 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.

I can try to detail the comment in kvm_arm_setup_debug when we set SPSR, 
hopefully making things clearer when seeing that part of the code.

Thanks,

-- 
Julien Thierry