On Thu, Aug 31, 2017 at 2:56 PM, Julien Thierry [off-list ref] wrote:

On 31/08/17 11:53, Christoffer Dall wrote:

quoted

On Thu, Aug 31, 2017 at 11:37 AM, Julien Thierry [off-list ref]
wrote:

quoted

On 31/08/17 09:54, Christoffer Dall wrote:

quoted

On Thu, Aug 31, 2017 at 10:45 AM, Julien Thierry
[off-list ref]
wrote:

quoted

Hi Christoffer,


On 30/08/17 19:53, Christoffer Dall wrote:

quoted

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
[off-list ref]
wrote:

quoted

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 <redacted>
Cc: Christoffer Dall <redacted>
Cc: Marc Zyngier <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <redacted>

---
    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.

quoted

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