[PATCH 3/3] arm64: kvm: Fix single step for guest skipped instructions
Alex Bennée
alex.bennee at linaro.org
Tue Oct 3 07:57:40 PDT 2017
Julien Thierry <julien.thierry at arm.com> writes:
> 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).
So to be clear kvm_arm_setup_debug only sets SS if we are about to
single-step. This is controlled by the debug ioctl which is done outside
of the main KVM run loop.
>>>>>>>>
>>>>>>>> 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.
This is because the ARMv7 support isn't complete (and also ARMv7
hardware is slightly more complex in its corner cases if we were to do it).
>>>>>
>>>>> 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.
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.
>>
>> 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?
Sorry for the delay getting back to you. I had flagged the email but
with holidays and conferences in the way it fell off my queue.
So to summarise as I understand things:
Host User Space | Host KVM | Host Hyp | Guest VM |
Enable Debug(SS)
KVM_RUN ----------->
Guest SPSR.SS set
--> World Switch ->
Insn Trap to Hyp
World Switch <-
(SS not cleared)
<--
Insn Emulated
pc += 4
-->
World Switch
(SS still set)
->
Insn +4 SS
<-
World Switch
(SS cleared)
<--
Guest exit (debug)
<--
See SS did 2 insns?
Do I understand the problem you are trying to fix correctly?
--
Alex Bennée
More information about the linux-arm-kernel
mailing list