[PATCH 3/3] arm64: kvm: Fix single step for guest skipped instructions
Julien Thierry
julien.thierry at arm.com
Fri Sep 29 05:38:14 PDT 2017
On 31/08/17 15:01, Christoffer Dall wrote:
> On Thu, Aug 31, 2017 at 3:57 PM, Julien Thierry <julien.thierry at arm.com> wrote:
>>
>>
>> On 31/08/17 14:28, Christoffer Dall wrote:
>>>
>>> On Thu, Aug 31, 2017 at 2:56 PM, Julien Thierry <julien.thierry at 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 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?
>>>>
>>> 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.
>
> Yes, I think your approach is the best choice now, considering.
>
>>
>> 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.
>>
>
> I also think we need to improve the comment in the world-switch return
> path, and I'd like Alex to weigh in here before we merge this. He's
> back from holiday on Monday.
>
Ping Alex?
--
Julien Thierry
More information about the linux-arm-kernel
mailing list