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

[Julien Thierry]

On 31/08/17 11:53, Christoffer Dall wrote:
> On Thu, Aug 31, 2017 at 11:37 AM, Julien Thierry <julien.thierry at 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 at 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 at 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 at arm.com>
>>>>>> Cc: Christoffer Dall <christoffer.dall at linaro.org>
>>>>>> Cc: Marc Zyngier <marc.zyngier at arm.com>
>>>>>> Cc: Catalin Marinas <catalin.marinas at arm.com>
>>>>>> Cc: Will Deacon <will.deacon at 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?

Thanks,

>>
>> Interesting idea, I can try to explore that possibility.
>>
>> Thanks for the suggestion,
>>
> Thanks!
> -Christoffer
> 

-- 
Julien Thierry