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

[christoffer.dall@linaro.org (Christoffer Dall)]

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.

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.

Thanks,
-Christoffer