Fix single step for traps

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 <[email protected]>

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

---
 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(-)

arch/arm64/include/asm/kvm_asm.h +2 -0

index 26a64d0..398bbaa 100644
@@ -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)						\
 	({								\

arch/arm64/include/asm/kvm_emulate.h +2 -0

index fe39e68..d401c64 100644
@@ -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;
 }
 
 static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu)

arch/arm64/kvm/debug.c +11 -1

index dbadfaf..4806e6b 100644
@@ -151,12 +151,22 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 		 * debugging the system.
 		 */
 		if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
-			*vcpu_cpsr(vcpu) |=  DBG_SPSR_SS;
+			/*
+			 * If we skipped an instruction while single stepping,
+			 * spsr.ss needs to be 0 in order to trigger SS
+			 * exception on return.
+			 */
+			if (!(vcpu->arch.debug_flags & KVM_ARM64_DEBUG_INST_SKIP))
+				*vcpu_cpsr(vcpu) |=  DBG_SPSR_SS;
+			else
+				*vcpu_cpsr(vcpu) &=  ~DBG_SPSR_SS;
 			vcpu_sys_reg(vcpu, MDSCR_EL1) |= DBG_MDSCR_SS;
 		} else {
 			vcpu_sys_reg(vcpu, MDSCR_EL1) &= ~DBG_MDSCR_SS;
 		}
 
+		vcpu->arch.debug_flags &= ~KVM_ARM64_DEBUG_INST_SKIP;
+
 		trace_kvm_arm_set_dreg32("SPSR_EL2", *vcpu_cpsr(vcpu));
 
 		/*

arch/arm64/kvm/hyp/switch.c +5 -0

index 945e79c..6030acd 100644
@@ -22,6 +22,7 @@
 #include <asm/kvm_emulate.h>
 #include <asm/kvm_hyp.h>
 #include <asm/fpsimd.h>
+#include <asm/debug-monitors.h>
 
 static bool __hyp_text __fpsimd_enabled_nvhe(void)
 {

@@ -276,6 +277,10 @@ static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
 	}
 
 	write_sysreg_el2(*vcpu_pc(vcpu), elr);
+
+	*vcpu_cpsr(vcpu) = read_sysreg_el2(spsr);
+	*vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS;
+	write_sysreg_el2(*vcpu_cpsr(vcpu), spsr);
 }
 
 int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)

arch/arm64/kvm/hyp/switch.c +0 -1

 				exit_code = ARM_EXCEPTION_EL1_SERROR;
 			}
 
--

1.9.1

Hi Julien,

Quoted 94 lines · Julien Thierry

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 <[email protected]>

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

---
  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(-)

arch/arm64/include/asm/kvm_asm.h +2 -0

index 26a64d0..398bbaa 100644
@@ -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)						\
  	({								\

arch/arm64/include/asm/kvm_emulate.h +2 -0

index fe39e68..d401c64 100644
@@ -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;
  }
 
  static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu)

arch/arm64/kvm/debug.c +11 -1

index dbadfaf..4806e6b 100644
@@ -151,12 +151,22 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
  		 * debugging the system.
  		 */
  		if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
-			*vcpu_cpsr(vcpu) |=  DBG_SPSR_SS;
+			/*
+			 * If we skipped an instruction while single stepping,
+			 * spsr.ss needs to be 0 in order to trigger SS
+			 * exception on return.
+			 */
+			if (!(vcpu->arch.debug_flags & KVM_ARM64_DEBUG_INST_SKIP))
+				*vcpu_cpsr(vcpu) |=  DBG_SPSR_SS;
+			else
+				*vcpu_cpsr(vcpu) &=  ~DBG_SPSR_SS;
  			vcpu_sys_reg(vcpu, MDSCR_EL1) |= DBG_MDSCR_SS;
  		} else {
  			vcpu_sys_reg(vcpu, MDSCR_EL1) &= ~DBG_MDSCR_SS;
  		}
 
+		vcpu->arch.debug_flags &= ~KVM_ARM64_DEBUG_INST_SKIP;
+
  		trace_kvm_arm_set_dreg32("SPSR_EL2", *vcpu_cpsr(vcpu));
 
  		/*

arch/arm64/kvm/hyp/switch.c +5 -0

index 945e79c..6030acd 100644
@@ -22,6 +22,7 @@
  #include <asm/kvm_emulate.h>
  #include <asm/kvm_hyp.h>
  #include <asm/fpsimd.h>
+#include <asm/debug-monitors.h>
 
  static bool __hyp_text __fpsimd_enabled_nvhe(void)
  {

@@ -276,6 +277,10 @@ static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
  	}
 
  	write_sysreg_el2(*vcpu_pc(vcpu), elr);
+
+	*vcpu_cpsr(vcpu) = read_sysreg_el2(spsr);
+	*vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS;
+	write_sysreg_el2(*vcpu_cpsr(vcpu), spsr);
  }
 

arch/arm64/kvm/hyp/switch.c int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu) · line 357

Quoted 12 lines

arch/arm64/kvm/hyp/switch.c +7 -0

  int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)

@@ -343,6 +348,13 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
  			if (ret == -1) {
  				/* Promote an illegal access to an SError */
  				__skip_instr(vcpu);
+
+				/*
+				 * We're not jumping back, let debug setup know
+				 * we skipped an instruction.
+				 */
+				vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP;
+

This feels odd. You've already cleared the SS bit from the SPSR, and yet you're setting that flag. What is the semantic of this?

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 <[email protected]>

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

---
 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(-)

arch/arm64/include/asm/kvm_asm.h +2 -0

index 26a64d0..398bbaa 100644
@@ -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)                                              \
        ({                                                              \

arch/arm64/include/asm/kvm_emulate.h +0 -0

index fe39e68..d401c64 100644

arch/arm64/include/asm/kvm_emulate.h +0 -0

 }
 
 static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu)

arch/arm64/kvm/debug.c +0 -0

index dbadfaf..4806e6b 100644
@@ -151,12 +151,22 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
                 * debugging the system.
                 */
                if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {

arch/arm64/kvm/debug.c +2 -0

                        vcpu_sys_reg(vcpu, MDSCR_EL1) |= DBG_MDSCR_SS;
                } else {
                        vcpu_sys_reg(vcpu, MDSCR_EL1) &= ~DBG_MDSCR_SS;
                }
 
+               vcpu->arch.debug_flags &= ~KVM_ARM64_DEBUG_INST_SKIP;
+
                trace_kvm_arm_set_dreg32("SPSR_EL2", *vcpu_cpsr(vcpu));
 
                /*

arch/arm64/kvm/hyp/switch.c +1 -0

index 945e79c..6030acd 100644
@@ -22,6 +22,7 @@
 #include <asm/kvm_emulate.h>
 #include <asm/kvm_hyp.h>
 #include <asm/fpsimd.h>
+#include <asm/debug-monitors.h>
 
 static bool __hyp_text __fpsimd_enabled_nvhe(void)
 {

arch/arm64/kvm/hyp/switch.c +0 -1

                                exit_code = ARM_EXCEPTION_EL1_SERROR;
                        }
 
--

1.9.1

Hi Julien,

[cc'ing Alex Benn?e here who wrote the debug code for arm64]

Quoted 36 lines · Julien Thierry

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 <[email protected]>

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

---
  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(-)

arch/arm64/include/asm/kvm_asm.h +2 -0

index 26a64d0..398bbaa 100644
@@ -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)                                              \
         ({                                                              \

arch/arm64/include/asm/kvm_emulate.h +0 -0

index fe39e68..d401c64 100644

arch/arm64/include/asm/kvm_emulate.h static inline void kvm_skip_instr(struct kvm_vcpu *vcpu, bool is_wide_instr) · line 99

Quoted 5 lines

arch/arm64/include/asm/kvm_emulate.h +2 -0

@@ -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?

I think even if the guest kernel is single-stepping guest userspace, you're still safe because you'll take another debug exception from having 'executed' (read: emulated) this instruction in the hypervisor.

Quoted 11 lines

arch/arm64/include/asm/kvm_emulate.h +0 -0

  }
 
  static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu)

arch/arm64/kvm/debug.c +0 -0

index dbadfaf..4806e6b 100644
@@ -151,12 +151,22 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
                  * debugging the system.
                  */
                 if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {

arch/arm64/kvm/debug.c void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) · line 162

Quoted 10 lines

arch/arm64/kvm/debug.c +9 -1

-                       *vcpu_cpsr(vcpu) |=  DBG_SPSR_SS;
+                       /*
+                        * If we skipped an instruction while single stepping,
+                        * spsr.ss needs to be 0 in order to trigger SS
+                        * exception on return.
+                        */
+                       if (!(vcpu->arch.debug_flags & KVM_ARM64_DEBUG_INST_SKIP))
+                               *vcpu_cpsr(vcpu) |=  DBG_SPSR_SS;
+                       else
+                               *vcpu_cpsr(vcpu) &=  ~DBG_SPSR_SS;

Then you wouldn't need this.

Quoted 25 lines

arch/arm64/kvm/debug.c +2 -0

                         vcpu_sys_reg(vcpu, MDSCR_EL1) |= DBG_MDSCR_SS;
                 } else {
                         vcpu_sys_reg(vcpu, MDSCR_EL1) &= ~DBG_MDSCR_SS;
                 }
 
+               vcpu->arch.debug_flags &= ~KVM_ARM64_DEBUG_INST_SKIP;
+
                 trace_kvm_arm_set_dreg32("SPSR_EL2", *vcpu_cpsr(vcpu));
 
                 /*

arch/arm64/kvm/hyp/switch.c +1 -0

index 945e79c..6030acd 100644
@@ -22,6 +22,7 @@
  #include <asm/kvm_emulate.h>
  #include <asm/kvm_hyp.h>
  #include <asm/fpsimd.h>
+#include <asm/debug-monitors.h>
 
  static bool __hyp_text __fpsimd_enabled_nvhe(void)
  {

@@ -276,6 +277,10 @@ static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu)
         }
 

arch/arm64/kvm/hyp/switch.c static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu) · line 283

Quoted 5 lines

arch/arm64/kvm/hyp/switch.c +4 -0

         write_sysreg_el2(*vcpu_pc(vcpu), elr);
+
+       *vcpu_cpsr(vcpu) = read_sysreg_el2(spsr);
+       *vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS;
+       write_sysreg_el2(*vcpu_cpsr(vcpu), spsr);

I don't think you really need to use *vcpu_cpsr(vcpu) here, I think you can just use a temporary variable, which may make the flow a bit easier to read actually.

arch/arm64/kvm/hyp/switch.c int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu) · line 356

Quoted 13 lines

arch/arm64/kvm/hyp/switch.c +6 -0

  }
 
  int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)

@@ -343,6 +348,13 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
                         if (ret == -1) {
                                 /* Promote an illegal access to an SError */
                                 __skip_instr(vcpu);
+
+                               /*
+                                * We're not jumping back, let debug setup know
+                                * we skipped an instruction.
+                                */
+                               vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP;

I agree with Marc, I don't think you need this, because you have modified the hardware spsr_el2, which you're going to read back in __sysreg_save_guest_state().

Hi Christoffer,

Quoted 61 lines · Christoffer Dall

Hi Julien,

[cc'ing Alex Benn?e here who wrote the debug code for arm64]

Quoted 44 lines · Julien Thierry

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 <[email protected]>

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

---
  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(-)

arch/arm64/include/asm/kvm_asm.h +2 -0

index 26a64d0..398bbaa 100644
@@ -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)

\
         ({

\

arch/arm64/include/asm/kvm_emulate.h +0 -0

index fe39e68..d401c64 100644

arch/arm64/include/asm/kvm_emulate.h static inline void kvm_skip_instr(struct kvm_vcpu · line 100

Quoted 6 lines

arch/arm64/include/asm/kvm_emulate.h +2 -0

@@ -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?

Quoted 2 lines

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

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

Quoted 59 lines

I think even if the guest kernel is single-stepping guest userspace, you're still safe because you'll take another debug exception from having 'executed' (read: emulated) this instruction in the hypervisor.

Quoted 13 lines

arch/arm64/include/asm/kvm_emulate.h +0 -0

  }
 

static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu)

arch/arm64/kvm/debug.c +1 -1

index dbadfaf..4806e6b 100644
@@ -151,12 +151,22 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
                  * debugging the system.
                  */
                 if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
-                       *vcpu_cpsr(vcpu) |=  DBG_SPSR_SS;
+                       /*

arch/arm64/kvm/debug.c void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) · line 164

Quoted 10 lines

arch/arm64/kvm/debug.c +8 -0

+                        * If we skipped an instruction while single
 stepping,
+                        * spsr.ss needs to be 0 in order to trigger SS
+                        * exception on return.
+                        */
+                       if (!(vcpu->arch.debug_flags &
 KVM_ARM64_DEBUG_INST_SKIP))
+                               *vcpu_cpsr(vcpu) |=  DBG_SPSR_SS;
+                       else
+                               *vcpu_cpsr(vcpu) &=  ~DBG_SPSR_SS;

Then you wouldn't need this.

Quoted 26 lines

arch/arm64/kvm/debug.c +2 -0

                         vcpu_sys_reg(vcpu, MDSCR_EL1) |= DBG_MDSCR_SS;
                 } else {
                         vcpu_sys_reg(vcpu, MDSCR_EL1) &= ~DBG_MDSCR_SS;
                 }
 
+               vcpu->arch.debug_flags &= ~KVM_ARM64_DEBUG_INST_SKIP;
+
                 trace_kvm_arm_set_dreg32("SPSR_EL2", *vcpu_cpsr(vcpu));

                 /*

arch/arm64/kvm/hyp/switch.c +1 -0

index 945e79c..6030acd 100644
@@ -22,6 +22,7 @@
  #include <asm/kvm_emulate.h>
  #include <asm/kvm_hyp.h>
  #include <asm/fpsimd.h>
+#include <asm/debug-monitors.h>
 
  static bool __hyp_text __fpsimd_enabled_nvhe(void)
  {

@@ -276,6 +277,10 @@ static void __hyp_text __skip_instr(struct kvm_vcpu
 *vcpu)
         }
 

Quoted 11 lines

arch/arm64/kvm/hyp/switch.c static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu) · line 283

Quoted 5 lines

arch/arm64/kvm/hyp/switch.c +4 -0

         write_sysreg_el2(*vcpu_pc(vcpu), elr);
+
+       *vcpu_cpsr(vcpu) = read_sysreg_el2(spsr);
+       *vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS;
+       write_sysreg_el2(*vcpu_cpsr(vcpu), spsr);

I don't think you really need to use *vcpu_cpsr(vcpu) here, I think you can just use a temporary variable, which may make the flow a bit easier to read actually.

Good point, I'll do that.

Quoted 13 lines

Quoted 13 lines

arch/arm64/kvm/hyp/switch.c +0 -0

  }
 

int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)

arch/arm64/kvm/hyp/switch.c +0 -0

@@ -343,6 +348,13 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
                         if (ret == -1) {
                                 /* Promote an illegal access to an

SError */

                                 __skip_instr(vcpu);

arch/arm64/kvm/hyp/switch.c +3 -0

+
+                               /*
+                                * We're not jumping back, let debug

setup know
+ * we skipped an instruction.

Quoted 9 lines

Quoted 3 lines

arch/arm64/kvm/hyp/switch.c +2 -0

+                                */
+                               vcpu->arch.debug_flags |=

KVM_ARM64_DEBUG_INST_SKIP;

I agree with Marc, I don't think you need this, because you have modified the hardware spsr_el2, which you're going to read back in __sysreg_save_guest_state().

We are exiting the guest, so it is the same case as kvm_skip_instr.

Quoted 65 lines · Julien Thierry

Hi Christoffer,

Quoted 61 lines · Christoffer Dall

Hi Julien,

[cc'ing Alex Benn?e here who wrote the debug code for arm64]

Quoted 44 lines · Julien Thierry

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 <[email protected]>

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

---
   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(-)

arch/arm64/include/asm/kvm_asm.h +2 -0

index 26a64d0..398bbaa 100644
@@ -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)
\
          ({

\

arch/arm64/include/asm/kvm_emulate.h +0 -0

index fe39e68..d401c64 100644

arch/arm64/include/asm/kvm_emulate.h static inline void kvm_skip_instr(struct kvm_vcpu · line 100

Quoted 6 lines

arch/arm64/include/asm/kvm_emulate.h +2 -0

@@ -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?

Quoted 8 lines

Quoted 2 lines

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

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.

Quoted 5 lines

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 ?

Quoted 96 lines

Quoted 59 lines

I think even if the guest kernel is single-stepping guest userspace, you're still safe because you'll take another debug exception from having 'executed' (read: emulated) this instruction in the hypervisor.

Quoted 13 lines

arch/arm64/include/asm/kvm_emulate.h +0 -0

   }
 

static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu)

arch/arm64/kvm/debug.c +1 -1

index dbadfaf..4806e6b 100644
@@ -151,12 +151,22 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
                   * debugging the system.
                   */
                  if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
-                       *vcpu_cpsr(vcpu) |=  DBG_SPSR_SS;
+                       /*

arch/arm64/kvm/debug.c void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) · line 164

Quoted 10 lines

arch/arm64/kvm/debug.c +8 -0

+                        * If we skipped an instruction while single
 stepping,
+                        * spsr.ss needs to be 0 in order to trigger SS
+                        * exception on return.
+                        */
+                       if (!(vcpu->arch.debug_flags &
 KVM_ARM64_DEBUG_INST_SKIP))
+                               *vcpu_cpsr(vcpu) |=  DBG_SPSR_SS;
+                       else
+                               *vcpu_cpsr(vcpu) &=  ~DBG_SPSR_SS;

Then you wouldn't need this.

Quoted 26 lines

arch/arm64/kvm/debug.c +2 -0

                          vcpu_sys_reg(vcpu, MDSCR_EL1) |= DBG_MDSCR_SS;
                  } else {
                          vcpu_sys_reg(vcpu, MDSCR_EL1) &= ~DBG_MDSCR_SS;
                  }
 
+               vcpu->arch.debug_flags &= ~KVM_ARM64_DEBUG_INST_SKIP;
+
                  trace_kvm_arm_set_dreg32("SPSR_EL2", *vcpu_cpsr(vcpu));

                  /*

arch/arm64/kvm/hyp/switch.c +1 -0

index 945e79c..6030acd 100644
@@ -22,6 +22,7 @@
   #include <asm/kvm_emulate.h>
   #include <asm/kvm_hyp.h>
   #include <asm/fpsimd.h>
+#include <asm/debug-monitors.h>
 
   static bool __hyp_text __fpsimd_enabled_nvhe(void)
   {

@@ -276,6 +277,10 @@ static void __hyp_text __skip_instr(struct kvm_vcpu
 *vcpu)
          }
 

Quoted 11 lines

arch/arm64/kvm/hyp/switch.c static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu) · line 283

Quoted 5 lines

arch/arm64/kvm/hyp/switch.c +4 -0

          write_sysreg_el2(*vcpu_pc(vcpu), elr);
+
+       *vcpu_cpsr(vcpu) = read_sysreg_el2(spsr);
+       *vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS;
+       write_sysreg_el2(*vcpu_cpsr(vcpu), spsr);

I don't think you really need to use *vcpu_cpsr(vcpu) here, I think you can just use a temporary variable, which may make the flow a bit easier to read actually.

Good point, I'll do that.

Quoted 13 lines

Quoted 13 lines

arch/arm64/kvm/hyp/switch.c +0 -0

   }
 

int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)

arch/arm64/kvm/hyp/switch.c +0 -0

@@ -343,6 +348,13 @@ int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
                          if (ret == -1) {
                                  /* Promote an illegal access to an

SError */

                                  __skip_instr(vcpu);

arch/arm64/kvm/hyp/switch.c +3 -0

+
+                               /*
+                                * We're not jumping back, let debug

setup know
+ * we skipped an instruction.

Quoted 9 lines

Quoted 3 lines

arch/arm64/kvm/hyp/switch.c +2 -0

+                                */
+                               vcpu->arch.debug_flags |=

KVM_ARM64_DEBUG_INST_SKIP;

I agree with Marc, I don't think you need this, because you have modified the hardware spsr_el2, which you're going to read back in __sysreg_save_guest_state().

Quoted 2 lines

We are exiting the guest, so it is the same case as kvm_skip_instr.

ok, I see this now, you're right. Thanks for explaining it. For some reason, this code is just really hard to understand, so I wonder if the suggested approach above is better.

Thanks,
-Christoffer

Quoted 99 lines · Christoffer Dall

Quoted 70 lines · Julien Thierry

Hi Christoffer,

Quoted 65 lines · Christoffer Dall

Hi Julien,

[cc'ing Alex Benn?e here who wrote the debug code for arm64]

Quoted 45 lines · Julien Thierry

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 <[email protected]>

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

---
   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(-)

arch/arm64/include/asm/kvm_asm.h +2 -0

index 26a64d0..398bbaa 100644
@@ -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)
\
          ({

\

arch/arm64/include/asm/kvm_emulate.h +0 -0

index fe39e68..d401c64 100644

arch/arm64/include/asm/kvm_emulate.h static inline void kvm_skip_instr(struct kvm_vcpu · line 100

Quoted 6 lines

arch/arm64/include/asm/kvm_emulate.h +2 -0

@@ -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?

Quoted 8 lines

Quoted 2 lines

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

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.

Quoted 6 lines

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

Quoted 7 lines

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?

Interesting idea, I can try to explore that possibility.

Thanks for the suggestion,

Quoted 110 lines · Julien Thierry

Quoted 99 lines · Christoffer Dall

Quoted 70 lines · Julien Thierry

Hi Christoffer,

Quoted 65 lines · Christoffer Dall

Hi Julien,

[cc'ing Alex Benn?e here who wrote the debug code for arm64]

Quoted 45 lines · Julien Thierry

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 <[email protected]>

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

---
    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(-)

arch/arm64/include/asm/kvm_asm.h +2 -0

index 26a64d0..398bbaa 100644
@@ -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)
\
           ({

\

arch/arm64/include/asm/kvm_emulate.h +0 -0

index fe39e68..d401c64 100644

arch/arm64/include/asm/kvm_emulate.h static inline void kvm_skip_instr(struct kvm_vcpu · line 100

Quoted 6 lines

arch/arm64/include/asm/kvm_emulate.h +2 -0

@@ -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?

Quoted 8 lines

Quoted 2 lines

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

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.

Quoted 6 lines

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

Quoted 7 lines

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 ?

Quoted 4 lines

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.

Quoted 5 lines

Interesting idea, I can try to explore that possibility.

Thanks for the suggestion,

Thanks!
-Christoffer

Quoted 135 lines · Christoffer Dall

Quoted 125 lines · Julien Thierry

Quoted 113 lines · Christoffer Dall

Quoted 79 lines · Julien Thierry

Hi Christoffer,

Quoted 70 lines · Christoffer Dall

Hi Julien,

[cc'ing Alex Benn?e here who wrote the debug code for arm64]

Quoted 48 lines · Julien Thierry

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 <[email protected]>

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

---
    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(-)

arch/arm64/include/asm/kvm_asm.h +2 -0

index 26a64d0..398bbaa 100644
@@ -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)
\
           ({

\

arch/arm64/include/asm/kvm_emulate.h +0 -0

index fe39e68..d401c64 100644

arch/arm64/include/asm/kvm_emulate.h static inline void kvm_skip_instr(struct kvm_vcpu · line 100

Quoted 6 lines

arch/arm64/include/asm/kvm_emulate.h +2 -0

@@ -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?

Quoted 2 lines

Can't we simply do clear DBG_SPSR_SS here?

Quoted 6 lines

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 10 lines

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.

Quoted 7 lines

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

Quoted 7 lines

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 ?

Quoted 5 lines

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?

Quoted 5 lines

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?

Quoted 162 lines · Julien Thierry

Quoted 135 lines · Christoffer Dall

Quoted 125 lines · Julien Thierry

Quoted 113 lines · Christoffer Dall

Quoted 79 lines · Julien Thierry

Hi Christoffer,

Quoted 70 lines · Christoffer Dall

Hi Julien,

[cc'ing Alex Benn?e here who wrote the debug code for arm64]

Quoted 48 lines · Julien Thierry

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 <[email protected]>

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

---
     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(-)

arch/arm64/include/asm/kvm_asm.h +2 -0

index 26a64d0..398bbaa 100644
@@ -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)
\
            ({

\

arch/arm64/include/asm/kvm_emulate.h +0 -0

index fe39e68..d401c64 100644

arch/arm64/include/asm/kvm_emulate.h static inline void kvm_skip_instr(struct kvm_vcpu · line 100

Quoted 6 lines

arch/arm64/include/asm/kvm_emulate.h +2 -0

@@ -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?

Quoted 2 lines

Can't we simply do clear DBG_SPSR_SS here?

Quoted 6 lines

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 10 lines

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.

Quoted 7 lines

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

Quoted 7 lines

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 ?

Quoted 5 lines

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?

Quoted 5 lines

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

Quoted 3 lines

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.

Quoted 184 lines · Christoffer Dall

Quoted 181 lines · Julien Thierry

Quoted 151 lines · Christoffer Dall

Quoted 136 lines · Julien Thierry

Quoted 123 lines · Christoffer Dall

Quoted 83 lines · Julien Thierry

Hi Christoffer,

Quoted 73 lines · Christoffer Dall

Hi Julien,

[cc'ing Alex Benn?e here who wrote the debug code for arm64]

Quoted 49 lines · Julien Thierry

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 <[email protected]>

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

---
     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(-)

arch/arm64/include/asm/kvm_asm.h +2 -0

index 26a64d0..398bbaa 100644
@@ -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)
\
            ({

\

arch/arm64/include/asm/kvm_emulate.h +0 -0

index fe39e68..d401c64 100644

arch/arm64/include/asm/kvm_emulate.h static inline void kvm_skip_instr(struct kvm_vcpu · line 100

Quoted 6 lines

arch/arm64/include/asm/kvm_emulate.h +2 -0

@@ -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?

Quoted 2 lines

Can't we simply do clear DBG_SPSR_SS here?

Quoted 8 lines

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 10 lines

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.

Quoted 9 lines

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

Quoted 7 lines

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 ?

Quoted 7 lines

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?

Quoted 5 lines

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

Quoted 8 lines

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 2 lines

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.

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.

Quoted 205 lines · Julien Thierry

Quoted 184 lines · Christoffer Dall

Quoted 181 lines · Julien Thierry

Quoted 151 lines · Christoffer Dall

Quoted 136 lines · Julien Thierry

Quoted 123 lines · Christoffer Dall

Quoted 83 lines · Julien Thierry

Hi Christoffer,

Quoted 73 lines · Christoffer Dall

Hi Julien,

[cc'ing Alex Benn?e here who wrote the debug code for arm64]

Quoted 49 lines · Julien Thierry

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 <[email protected]>

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

---
      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(-)

arch/arm64/include/asm/kvm_asm.h +2 -0

index 26a64d0..398bbaa 100644
@@ -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)
\
             ({

\

arch/arm64/include/asm/kvm_emulate.h +0 -0

index fe39e68..d401c64 100644

arch/arm64/include/asm/kvm_emulate.h static inline void kvm_skip_instr(struct kvm_vcpu · line 100

Quoted 6 lines

arch/arm64/include/asm/kvm_emulate.h +2 -0

@@ -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?

Quoted 2 lines

Can't we simply do clear DBG_SPSR_SS here?

Quoted 8 lines

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 10 lines

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.

Quoted 9 lines

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

Quoted 7 lines

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 ?

Quoted 7 lines

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?

Quoted 5 lines

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

Quoted 8 lines

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 1 line

Quoted 3 lines

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.

Quoted 83 lines · Christoffer Dall

Quoted 82 lines · Julien Thierry

Quoted 81 lines · Christoffer Dall

Quoted 79 lines · Julien Thierry

Quoted 78 lines · Christoffer Dall

Quoted 75 lines · Julien Thierry

Quoted 74 lines · Christoffer Dall

Quoted 71 lines · Julien Thierry

Hi Christoffer,

Quoted 68 lines · Christoffer Dall

Hi Julien,

[cc'ing Alex Benn?e here who wrote the debug code for arm64]

Quoted 46 lines · Julien Thierry

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 <[email protected]>

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

---
      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(-)

arch/arm64/include/asm/kvm_asm.h +2 -0

index 26a64d0..398bbaa 100644
@@ -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)
\
             ({

\

arch/arm64/include/asm/kvm_emulate.h +0 -0

index fe39e68..d401c64 100644

arch/arm64/include/asm/kvm_emulate.h static inline void kvm_skip_instr(struct kvm_vcpu · line 100

Quoted 6 lines

arch/arm64/include/asm/kvm_emulate.h +2 -0

@@ -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?

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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

Quoted 47 lines

Quoted 47 lines

Quoted 47 lines

Quoted 47 lines

Quoted 46 lines

Quoted 31 lines

Quoted 23 lines

Ah, right, duh.

Quoted 10 lines

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.

Quoted 9 lines

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

Quoted 7 lines

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 ?

Quoted 7 lines

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.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

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.

Quoted 24 lines

Quoted 24 lines

Quoted 23 lines

Quoted 15 lines

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

Quoted 4 lines

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.

Quoted 8 lines

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 16 lines

Quoted 8 lines

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

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?

Quoted 84 lines · Julien Thierry

Quoted 83 lines · Christoffer Dall

Quoted 82 lines · Julien Thierry

Quoted 81 lines · Christoffer Dall

Quoted 79 lines · Julien Thierry

Quoted 78 lines · Christoffer Dall

Quoted 75 lines · Julien Thierry

Quoted 74 lines · Christoffer Dall

Quoted 71 lines · Julien Thierry

Hi Christoffer,

Quoted 68 lines · Christoffer Dall

Hi Julien,

[cc'ing Alex Benn?e here who wrote the debug code for arm64]

Quoted 46 lines · Julien Thierry

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 <[email protected]>

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

---
       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(-)

arch/arm64/include/asm/kvm_asm.h +2 -0

index 26a64d0..398bbaa 100644
@@ -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)
\
              ({

\

arch/arm64/include/asm/kvm_emulate.h +0 -0

index fe39e68..d401c64 100644

arch/arm64/include/asm/kvm_emulate.h static inline void kvm_skip_instr(struct kvm_vcpu · line 100

Quoted 6 lines

arch/arm64/include/asm/kvm_emulate.h +2 -0

@@ -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?

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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.

Quoted 47 lines

Quoted 47 lines

Quoted 47 lines

Quoted 47 lines

Quoted 47 lines

Quoted 46 lines

Quoted 31 lines

Quoted 23 lines

Ah, right, duh.

Quoted 10 lines

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.

Quoted 9 lines

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

Quoted 7 lines

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 ?

Quoted 7 lines

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.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

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

Quoted 24 lines

Quoted 24 lines

Quoted 24 lines

Quoted 23 lines

Quoted 15 lines

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

Quoted 4 lines

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.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 21 lines

Quoted 20 lines

Quoted 8 lines

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 1 line

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?

Quoted 4 lines · Alex Benn?e

Quoted 1 line · Julien Thierry

On 31/08/17 15:01, Christoffer Dall wrote:

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines · Christoffer Dall

Quoted 4 lines

Hi Julien,

[cc'ing Alex Benn?e here who wrote the debug code for arm64]

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

Quoted 3 lines

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.

Quoted 5 lines

Quoted 1 line

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.

No problem, thanks for looking at it.

Quoted 26 lines

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

Quoted 3 lines

See SS did 2 insns?

Do I understand the problem you are trying to fix correctly?

Yes that's the issue. The debugger is not made aware of the emulated/skipped instruction and the hypervisor jumps back into the guest.

Clearing SS before jumping back to the guest will simply trigger a debug exception as soon as we ERET from EL2 to EL1 (so we end up just getting back to EL2).

Quoted 5 lines · Julien Thierry

Quoted 4 lines · Alex Benn?e

Quoted 1 line · Julien Thierry

On 31/08/17 15:01, Christoffer Dall wrote:

<snip>

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines · Christoffer Dall

Quoted 4 lines

Hi Julien,

[cc'ing Alex Benn?e here who wrote the debug code for arm64]

<snip>

Quoted 47 lines

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

Quoted 3 lines

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.

Quoted 5 lines

Quoted 1 line

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.

No problem, thanks for looking at it.

Quoted 28 lines

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?

Quoted 1 line

Do I understand the problem you are trying to fix correctly?

Quoted 7 lines

Yes that's the issue. The debugger is not made aware of the emulated/skipped instruction and the hypervisor jumps back into the guest.

Clearing SS before jumping back to the guest will simply trigger a debug exception as soon as we ERET from EL2 to EL1 (so we end up just getting back to EL2).

Why don't we just exit KVM after we've emulated the instruction if we are under debug? After all at this point whatever needed to be done is done and the guest debug code can get on with life.

I understand there is the problem of exiting for an MMIO emulation but maybe that complexity should be handled by userspace ("render unto userspace the things that are userspaces") and it can decide to lift the step ioctl if appropriate.

NOTE: For KVM_EXIT_IO, KVM_EXIT_MMIO, KVM_EXIT_OSI, KVM_EXIT_PAPR and
KVM_EXIT_EPR the corresponding
operations are complete (and guest state is consistent) only after userspace has re-entered the kernel with KVM_RUN. The kernel side will first finish incomplete operations and then check for pending signals. Userspace can re-enter the guest with an unmasked signal pending to complete pending operations.

I guess I should have a look at the series. Are you re-basing anytime soon? It looks like it currently has a few minor merge conflicts with current master.

Quoted 4 lines · Alex Benn?e

Quoted 1 line · Julien Thierry

On 31/08/17 15:01, Christoffer Dall wrote:

Quoted 26 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

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.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

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.

From 46ea80d7dc9b98661fcd51c41090f8ad74a6690f Mon Sep 17 00:00:00 2001

Quoted 5 lines · Julien Thierry

Quoted 4 lines · Alex Benn?e

Quoted 1 line · Julien Thierry

On 31/08/17 15:01, Christoffer Dall wrote:

<snip>

Quoted 26 lines

Quoted 26 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

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.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

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.

<snip>

This is my currently untested but otherwise simpler solution:

From 46ea80d7dc9b98661fcd51c41090f8ad74a6690f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <[email protected]>
Date: Tue, 3 Oct 2017 17:17:15 +0100
Subject: [PATCH] KVM: arm64: handle single-stepping trapped instructions
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If we are using guest debug to single-step the guest we need to ensure we exit after emulating the instruction. This only affects instructions emulated by the kernel. If we exit to userspace anyway we leave it to userspace to work out what to do.

Signed-off-by Alex Benn?e <[email protected]>

---
  arch/arm64/kvm/handle_exit.c | 51 ++++++++++++++++++++++++++++++++------------
  1 file changed, 37 insertions(+), 14 deletions(-)

arch/arm64/kvm/handle_exit.c +0 -0

index 7debb74843a0..b197ffb10e96 100644

arch/arm64/kvm/handle_exit.c +30 -15

+static int handle_trap_exceptions(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	int handled;
+
+	/*
+	 * See ARM ARM B1.14.1: "Hyp traps on instructions
+	 * that fail their condition code check"
+	 */
+	if (!kvm_condition_valid(vcpu)) {
+		kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
+		handled = 1;
+	} else {
+		exit_handle_fn exit_handler;
+
+		exit_handler = kvm_get_exit_handler(vcpu);
+		handled = exit_handler(vcpu, run);
+	}
+
+	if (handled && (vcpu->debug_flags & KVM_GUESTDBG_SINGLESTEP)) {
+		u32 hsr = kvm_vcpu_get_hsr(vcpu);
+
+		handled = 0;
+		run->exit_reason = KVM_EXIT_DEBUG;
+		run->debug.arch.hsr = hsr;
+	}
+
+	return handled;
+}
+
  /*
   * Return > 0 to return to guest, < 0 on error, 0 (and set exit_reason) on
   * proper exit to userspace.

@@ -185,8 +221,6 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
  int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
  		       int exception_index)
  {
-	exit_handle_fn exit_handler;
-
  	if (ARM_SERROR_PENDING(exception_index)) {
  		u8 hsr_ec = ESR_ELx_EC(kvm_vcpu_get_hsr(vcpu));
 

@@ -214,18 +248,7 @@ int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
  		kvm_inject_vabt(vcpu);
  		return 1;
  	case ARM_EXCEPTION_TRAP:
-		/*
-		 * See ARM ARM B1.14.1: "Hyp traps on instructions
-		 * that fail their condition code check"
-		 */
-		if (!kvm_condition_valid(vcpu)) {
-			kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
-			return 1;
-		}
-
-		exit_handler = kvm_get_exit_handler(vcpu);
-
-		return exit_handler(vcpu, run);
+		return handle_trap_exceptions(vcpu, run);
  	case ARM_EXCEPTION_HYP_GONE:
  		/*
  		 * EL2 has been reset to the hyp-stub. This happens when a guest
--

2.14.1

Quoted 73 lines · Alex Benn?e

Quoted 5 lines · Julien Thierry

Quoted 4 lines · Alex Benn?e

Quoted 1 line · Julien Thierry

On 31/08/17 15:01, Christoffer Dall wrote:

<snip>

Quoted 26 lines

Quoted 26 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

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.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

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.

<snip>

This is my currently untested but otherwise simpler solution:

From 46ea80d7dc9b98661fcd51c41090f8ad74a6690f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <[email protected]>
Date: Tue, 3 Oct 2017 17:17:15 +0100
Subject: [PATCH] KVM: arm64: handle single-stepping trapped instructions
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If we are using guest debug to single-step the guest we need to ensure we exit after emulating the instruction. This only affects instructions emulated by the kernel. If we exit to userspace anyway we leave it to userspace to work out what to do.

Signed-off-by Alex Benn?e <[email protected]>

---
  arch/arm64/kvm/handle_exit.c | 51 ++++++++++++++++++++++++++++++++------------
  1 file changed, 37 insertions(+), 14 deletions(-)

arch/arm64/kvm/handle_exit.c +0 -0

index 7debb74843a0..b197ffb10e96 100644
@@ -178,6 +178,42 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
  	return arm_exit_handlers[hsr_ec];
  }
 

arch/arm64/kvm/handle_exit.c static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu) · line 187

Quoted 7 lines

arch/arm64/kvm/handle_exit.c +7 -0

+/*
+ * When handling traps we need to ensure exit the guest if we
+ * successfully emulated the instruction while single-stepping. If we
+ * have to exit anyway for userspace emulation then it's up to
+ * userspace to handle the "while SSing case".
+ */
+

I have not tested the code but if it work we also need to do something similar for MMIOs that are handled by the kernel (without returning to userland). But it should be pretty similar.

Quoted 74 lines · Julien Thierry

Quoted 73 lines · Alex Benn?e

Quoted 5 lines · Julien Thierry

Quoted 4 lines · Alex Benn?e

Quoted 1 line · Julien Thierry

On 31/08/17 15:01, Christoffer Dall wrote:

<snip>

Quoted 26 lines

Quoted 26 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

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.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

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.

<snip>

This is my currently untested but otherwise simpler solution:

From 46ea80d7dc9b98661fcd51c41090f8ad74a6690f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <[email protected]>
Date: Tue, 3 Oct 2017 17:17:15 +0100
Subject: [PATCH] KVM: arm64: handle single-stepping trapped instructions
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If we are using guest debug to single-step the guest we need to ensure we exit after emulating the instruction. This only affects instructions emulated by the kernel. If we exit to userspace anyway we leave it to userspace to work out what to do.

Signed-off-by Alex Benn?e <[email protected]>

---
   arch/arm64/kvm/handle_exit.c | 51 ++++++++++++++++++++++++++++++++------------
   1 file changed, 37 insertions(+), 14 deletions(-)

arch/arm64/kvm/handle_exit.c +0 -0

index 7debb74843a0..b197ffb10e96 100644
@@ -178,6 +178,42 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
   	return arm_exit_handlers[hsr_ec];
   }
 

Quoted 11 lines

arch/arm64/kvm/handle_exit.c static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu) · line 187

Quoted 7 lines

arch/arm64/kvm/handle_exit.c +7 -0

+/*
+ * When handling traps we need to ensure exit the guest if we
+ * successfully emulated the instruction while single-stepping. If we
+ * have to exit anyway for userspace emulation then it's up to
+ * userspace to handle the "while SSing case".
+ */
+

I have not tested the code but if it work we also need to do something similar for MMIOs that are handled by the kernel (without returning to userland). But it should be pretty similar.

<snip>

Which path do they take to the mmio emulation?

Quoted 88 lines · Alex Benn?e

Quoted 74 lines · Julien Thierry

Quoted 73 lines · Alex Benn?e

Quoted 5 lines · Julien Thierry

Quoted 4 lines · Alex Benn?e

Quoted 1 line · Julien Thierry

On 31/08/17 15:01, Christoffer Dall wrote:

<snip>

Quoted 26 lines

Quoted 26 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

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.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

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.

<snip>

This is my currently untested but otherwise simpler solution:

From 46ea80d7dc9b98661fcd51c41090f8ad74a6690f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <[email protected]>
Date: Tue, 3 Oct 2017 17:17:15 +0100
Subject: [PATCH] KVM: arm64: handle single-stepping trapped instructions
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If we are using guest debug to single-step the guest we need to ensure we exit after emulating the instruction. This only affects instructions emulated by the kernel. If we exit to userspace anyway we leave it to userspace to work out what to do.

Signed-off-by Alex Benn?e <[email protected]>

---
   arch/arm64/kvm/handle_exit.c | 51 ++++++++++++++++++++++++++++++++------------
   1 file changed, 37 insertions(+), 14 deletions(-)

arch/arm64/kvm/handle_exit.c +0 -0

index 7debb74843a0..b197ffb10e96 100644
@@ -178,6 +178,42 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
   	return arm_exit_handlers[hsr_ec];
   }
 

Quoted 11 lines

arch/arm64/kvm/handle_exit.c static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu) · line 187

Quoted 7 lines

arch/arm64/kvm/handle_exit.c +7 -0

+/*
+ * When handling traps we need to ensure exit the guest if we
+ * successfully emulated the instruction while single-stepping. If we
+ * have to exit anyway for userspace emulation then it's up to
+ * userspace to handle the "while SSing case".
+ */
+

I have not tested the code but if it work we also need to do something similar for MMIOs that are handled by the kernel (without returning to userland). But it should be pretty similar.

Quoted 4 lines

<snip>

Which path do they take to the mmio emulation?

Nevermind, I was mistaken, mmio code will get called by exit_handler and "handled" will be true if the mmio was handled by KVM. So your patch already handles this.

There is also the case of GIC CPU inteface accesses being trapped (which shouldn't be the default behaviour). If the vgic access fails, we will skip the instruction (in __kvm_vcpu_run in arch/arm64/kvm/hyp/switch.c) and if we were single stepping we will single step 2 instructions. But this seems to be a corner case of a corner case (GIC CPUIF trapped + access failing + single stepping), so I don't know if we want to take that into account right now?

I'm still a bit concerned about the change of semantics for KVM_EXIT_MMIO with regards to userland (cf. my previous mail). But if this is deemed to be a reasonable change, the patch seems fine to me.

From 2e8fcea695a9eca67fbeb331d3104d1d9e7e337a Mon Sep 17 00:00:00 2001

Thanks,

From 2e8fcea695a9eca67fbeb331d3104d1d9e7e337a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <[email protected]>
Date: Wed, 4 Oct 2017 09:49:41 +0000
Subject: [PATCH] kvm: exit run loop after emulating IO when single stepping
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If single-stepping is enabled we should exit the run-loop after emulating the access. Otherwise single-stepping across emulated IO accesses may skip an instruction.

This only addresses user-space emulation. Stuff done in kernel-mode should be handled there.

Signed-off-by Alex Benn?e <[email protected]>

---
 accel/kvm/kvm-all.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

accel/kvm/kvm-all.c +1 -1

index 90c88b517d..85bcb2b0d4 100644
@@ -1940,7 +1940,7 @@ int kvm_cpu_exec(CPUState *cpu)
                           run->io.direction,
                           run->io.size,
                           run->io.count);
-            ret = 0;
+            ret = cpu->singlestep_enabled ? EXCP_DEBUG : 0;
             break;
         case KVM_EXIT_MMIO:
             DPRINTF("handle_mmio\n");

Quoted 38 lines · Alex Benn?e

From 2e8fcea695a9eca67fbeb331d3104d1d9e7e337a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <[email protected]>
Date: Wed, 4 Oct 2017 09:49:41 +0000
Subject: [PATCH] kvm: exit run loop after emulating IO when single stepping
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If single-stepping is enabled we should exit the run-loop after emulating the access. Otherwise single-stepping across emulated IO accesses may skip an instruction.

This only addresses user-space emulation. Stuff done in kernel-mode should be handled there.

Signed-off-by Alex Benn?e <[email protected]>

---
 accel/kvm/kvm-all.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

accel/kvm/kvm-all.c +1 -1

index 90c88b517d..85bcb2b0d4 100644
@@ -1940,7 +1940,7 @@ int kvm_cpu_exec(CPUState *cpu)
                           run->io.direction,
                           run->io.size,
                           run->io.count);
-            ret = 0;
+            ret = cpu->singlestep_enabled ? EXCP_DEBUG : 0;
             break;
         case KVM_EXIT_MMIO:
             DPRINTF("handle_mmio\n");

@@ -1950,7 +1950,7 @@ int kvm_cpu_exec(CPUState *cpu)
                              run->mmio.data,
                              run->mmio.len,
                              run->mmio.is_write);

accel/kvm/kvm-all.c int kvm_cpu_exec(CPUState *cpu) · line 1956

Quoted 5 lines

accel/kvm/kvm-all.c +1 -1

-            ret = 0;
+            ret = cpu->singlestep_enabled ? EXCP_DEBUG : 0;
             break;
         case KVM_EXIT_IRQ_WINDOW_OPEN:
             DPRINTF("irq_window_open\n");

Singlestep mode doesn't make much sense for KVM. For TCG the purpose is to build one-instruction translation blocks, but what would it mean for KVM?

Quoted 39 lines · Paolo Bonzini

Quoted 38 lines · Alex Benn?e

From 2e8fcea695a9eca67fbeb331d3104d1d9e7e337a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <[email protected]>
Date: Wed, 4 Oct 2017 09:49:41 +0000
Subject: [PATCH] kvm: exit run loop after emulating IO when single stepping
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If single-stepping is enabled we should exit the run-loop after emulating the access. Otherwise single-stepping across emulated IO accesses may skip an instruction.

This only addresses user-space emulation. Stuff done in kernel-mode should be handled there.

Signed-off-by Alex Benn?e <[email protected]>

---
 accel/kvm/kvm-all.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

accel/kvm/kvm-all.c +1 -1

index 90c88b517d..85bcb2b0d4 100644
@@ -1940,7 +1940,7 @@ int kvm_cpu_exec(CPUState *cpu)
                           run->io.direction,
                           run->io.size,
                           run->io.count);
-            ret = 0;
+            ret = cpu->singlestep_enabled ? EXCP_DEBUG : 0;
             break;
         case KVM_EXIT_MMIO:
             DPRINTF("handle_mmio\n");

@@ -1950,7 +1950,7 @@ int kvm_cpu_exec(CPUState *cpu)
                              run->mmio.data,
                              run->mmio.len,
                              run->mmio.is_write);

Quoted 8 lines

accel/kvm/kvm-all.c int kvm_cpu_exec(CPUState *cpu) · line 1956

Quoted 5 lines

accel/kvm/kvm-all.c +1 -1

-            ret = 0;
+            ret = cpu->singlestep_enabled ? EXCP_DEBUG : 0;
             break;
         case KVM_EXIT_IRQ_WINDOW_OPEN:
             DPRINTF("irq_window_open\n");

Singlestep mode doesn't make much sense for KVM. For TCG the purpose is to build one-instruction translation blocks, but what would it mean for KVM?

It's used by the kvm_arch_handle_debug() code to verify single-stepping is enabled when processing debug exceptions. And also kvm_update_debug:

    if (cpu->singlestep_enabled) {
        data.dbg.control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
    }

We also have an aliased singlestep_enabled in our disas_context for the translator.

[Added Paolo, including QEMU userspace patch]

Quoted 50 lines · Julien Thierry

Quoted 42 lines · Alex Benn?e

Quoted 39 lines · Julien Thierry

Quoted 38 lines · Alex Benn?e

Quoted 5 lines · Julien Thierry

Quoted 4 lines · Alex Benn?e

Quoted 1 line · Julien Thierry

On 31/08/17 15:01, Christoffer Dall wrote:

<snip>

Quoted 29 lines

Quoted 29 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

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.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

So perhaps we should stick with your original approach.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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.

<snip>

Quoted 7 lines

There is also the case of GIC CPU inteface accesses being trapped (which shouldn't be the default behaviour). If the vgic access fails, we will skip the instruction (in __kvm_vcpu_run in arch/arm64/kvm/hyp/switch.c) and if we were single stepping we will single step 2 instructions. But this seems to be a corner case of a corner case (GIC CPUIF trapped + access failing + single stepping), so I don't know if we want to take that into account right now?

Yeah looking at the hyp code I did wonder if it warranted the complexity of adding handling there.

Quoted 3 lines

I'm still a bit concerned about the change of semantics for KVM_EXIT_MMIO with regards to userland (cf. my previous mail). But if this is deemed to be a reasonable change, the patch seems fine to me.

Have we changed the semantics? A normal un-handled by the kernel IO/MMIO exit needs to be processed before the single step takes effect. I can't speak for other userspace but I think for QEMU it is as simple as the patch bellow. That said it wasn't quite clear where the PC gets updated in this path - I think the kernel updates the PC before the KVM_EXIT_MMIO in the same path as the internal handling.

NOTE: For KVM_EXIT_IO, KVM_EXIT_MMIO, KVM_EXIT_OSI, KVM_EXIT_PAPR and
KVM_EXIT_EPR the corresponding
operations are complete (and guest state is consistent) only after userspace has re-entered the kernel with KVM_RUN. The kernel side will first finish incomplete operations and then check for pending signals. Userspace can re-enter the guest with an unmasked signal pending to complete pending operations.

I'd like to test these patches on some real life examples. I tried tracing over the pl011_write code in a kernel boot but I ran into the problem of el1_irq's occurring as I tried to step through the guest kernel. Is this something you've come across? What MMIO accesses have you been using in your testing?

QEMU Patch bellow:

From 2e8fcea695a9eca67fbeb331d3104d1d9e7e337a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <[email protected]>
Date: Wed, 4 Oct 2017 09:49:41 +0000
Subject: [PATCH] kvm: exit run loop after emulating IO when single stepping
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If single-stepping is enabled we should exit the run-loop after emulating the access. Otherwise single-stepping across emulated IO accesses may skip an instruction.

This only addresses user-space emulation. Stuff done in kernel-mode should be handled there.

Signed-off-by Alex Benn?e <[email protected]>

---
  accel/kvm/kvm-all.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

accel/kvm/kvm-all.c +2 -3

index 90c88b517d..85bcb2b0d4 100644
@@ -1940,7 +1940,7 @@ int kvm_cpu_exec(CPUState *cpu)
                            run->io.direction,
                            run->io.size,
                            run->io.count);
-            ret = 0;
+            ret = cpu->singlestep_enabled ? EXCP_DEBUG : 0;
              break;
          case KVM_EXIT_MMIO:
              DPRINTF("handle_mmio\n");

@@ -1950,7 +1950,7 @@ int kvm_cpu_exec(CPUState *cpu)
                               run->mmio.data,
                               run->mmio.len,
                               run->mmio.is_write);
-            ret = 0;
+            ret = cpu->singlestep_enabled ? EXCP_DEBUG : 0;
              break;
          case KVM_EXIT_IRQ_WINDOW_OPEN:
              DPRINTF("irq_window_open\n");
--

2.14.1

Quoted 70 lines · Alex Benn?e

[Added Paolo, including QEMU userspace patch]

Quoted 50 lines · Julien Thierry

Quoted 42 lines · Alex Benn?e

Quoted 39 lines · Julien Thierry

Quoted 38 lines · Alex Benn?e

Quoted 5 lines · Julien Thierry

Quoted 4 lines · Alex Benn?e

Quoted 1 line · Julien Thierry

On 31/08/17 15:01, Christoffer Dall wrote:

<snip>

Quoted 29 lines

Quoted 29 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

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.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

So perhaps we should stick with your original approach.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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.

<snip>

Quoted 7 lines

There is also the case of GIC CPU inteface accesses being trapped (which shouldn't be the default behaviour). If the vgic access fails, we will skip the instruction (in __kvm_vcpu_run in arch/arm64/kvm/hyp/switch.c) and if we were single stepping we will single step 2 instructions. But this seems to be a corner case of a corner case (GIC CPUIF trapped + access failing + single stepping), so I don't know if we want to take that into account right now?

Yeah looking at the hyp code I did wonder if it warranted the complexity of adding handling there.

Quoted 3 lines

I'm still a bit concerned about the change of semantics for KVM_EXIT_MMIO with regards to userland (cf. my previous mail). But if this is deemed to be a reasonable change, the patch seems fine to me.

Quoted 7 lines

Have we changed the semantics? A normal un-handled by the kernel IO/MMIO exit needs to be processed before the single step takes effect. I can't speak for other userspace but I think for QEMU it is as simple as the patch bellow. That said it wasn't quite clear where the PC gets updated in this path - I think the kernel updates the PC before the KVM_EXIT_MMIO in the same path as the internal handling.

Well I'm not sure. The part I am concerned about is:

Quoted 7 lines

NOTE: For KVM_EXIT_IO, KVM_EXIT_MMIO, KVM_EXIT_OSI, KVM_EXIT_PAPR and
KVM_EXIT_EPR the corresponding
operations are complete (and guest state is consistent) only after userspace has re-entered the kernel with KVM_RUN. The kernel side will first finish incomplete operations and then check for pending signals. Userspace can re-enter the guest with an unmasked signal pending to complete pending operations.

From Documentation/virtual/kvm/api.txt.

The way I interpret this is that userland should not consider the MMIO complete before running the vcpu again. If that the case it shouldn't trigger the single step since the instruction is not completely finished.

Maybe I don't interpret this correctly or it is not relevant here. Although I'd like to understand why.

Quoted 6 lines

I'd like to test these patches on some real life examples. I tried tracing over the pl011_write code in a kernel boot but I ran into the problem of el1_irq's occurring as I tried to step through the guest kernel. Is this something you've come across? What MMIO accesses have you been using in your testing?

I didn't know which MMIOs were handled by userland so I have only tested traps and MMIOs handled by the kernel.

This sounds like an issue when you are debugging an interruptible context, an issue Pratyush has been looking at [1]. Are you taking a guest interrupt when you try to execute the instruction to be stepped? I don't know what's the status of this patch series. Can you test the userland MMIO in a non-interruptible context?

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-July/517234.html

Thanks,

Quoted 39 lines

QEMU Patch bellow:

From 2e8fcea695a9eca67fbeb331d3104d1d9e7e337a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <[email protected]>
Date: Wed, 4 Oct 2017 09:49:41 +0000
Subject: [PATCH] kvm: exit run loop after emulating IO when single stepping
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If single-stepping is enabled we should exit the run-loop after emulating the access. Otherwise single-stepping across emulated IO accesses may skip an instruction.

This only addresses user-space emulation. Stuff done in kernel-mode should be handled there.

Signed-off-by Alex Benn?e <[email protected]>

---
  accel/kvm/kvm-all.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

accel/kvm/kvm-all.c +1 -1

index 90c88b517d..85bcb2b0d4 100644
@@ -1940,7 +1940,7 @@ int kvm_cpu_exec(CPUState *cpu)
                            run->io.direction,
                            run->io.size,
                            run->io.count);
-            ret = 0;
+            ret = cpu->singlestep_enabled ? EXCP_DEBUG : 0;
              break;
          case KVM_EXIT_MMIO:
              DPRINTF("handle_mmio\n");

@@ -1950,7 +1950,7 @@ int kvm_cpu_exec(CPUState *cpu)
                               run->mmio.data,
                               run->mmio.len,
                               run->mmio.is_write);

Quoted 8 lines

accel/kvm/kvm-all.c +1 -2

-            ret = 0;
+            ret = cpu->singlestep_enabled ? EXCP_DEBUG : 0;
              break;
          case KVM_EXIT_IRQ_WINDOW_OPEN:
              DPRINTF("irq_window_open\n");
--

2.14.1

Quoted 90 lines · Julien Thierry

Quoted 70 lines · Alex Benn?e

[Added Paolo, including QEMU userspace patch]

Quoted 50 lines · Julien Thierry

Quoted 42 lines · Alex Benn?e

Quoted 39 lines · Julien Thierry

Quoted 38 lines · Alex Benn?e

Quoted 5 lines · Julien Thierry

Quoted 4 lines · Alex Benn?e

Quoted 1 line · Julien Thierry

On 31/08/17 15:01, Christoffer Dall wrote:

<snip>

Quoted 29 lines

Quoted 29 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

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.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

So perhaps we should stick with your original approach.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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.

<snip>

Quoted 7 lines

There is also the case of GIC CPU inteface accesses being trapped (which shouldn't be the default behaviour). If the vgic access fails, we will skip the instruction (in __kvm_vcpu_run in arch/arm64/kvm/hyp/switch.c) and if we were single stepping we will single step 2 instructions. But this seems to be a corner case of a corner case (GIC CPUIF trapped + access failing + single stepping), so I don't know if we want to take that into account right now?

Yeah looking at the hyp code I did wonder if it warranted the complexity of adding handling there.

Quoted 3 lines

I'm still a bit concerned about the change of semantics for KVM_EXIT_MMIO with regards to userland (cf. my previous mail). But if this is deemed to be a reasonable change, the patch seems fine to me.

Quoted 7 lines

Have we changed the semantics? A normal un-handled by the kernel IO/MMIO exit needs to be processed before the single step takes effect. I can't speak for other userspace but I think for QEMU it is as simple as the patch bellow. That said it wasn't quite clear where the PC gets updated in this path - I think the kernel updates the PC before the KVM_EXIT_MMIO in the same path as the internal handling.

Well I'm not sure. The part I am concerned about is:

Quoted 7 lines

NOTE: For KVM_EXIT_IO, KVM_EXIT_MMIO, KVM_EXIT_OSI, KVM_EXIT_PAPR and
KVM_EXIT_EPR the corresponding
operations are complete (and guest state is consistent) only after userspace has re-entered the kernel with KVM_RUN. The kernel side will first finish incomplete operations and then check for pending signals. Userspace can re-enter the guest with an unmasked signal pending to complete pending operations.

From Documentation/virtual/kvm/api.txt.

Quoted 7 lines

The way I interpret this is that userland should not consider the MMIO complete before running the vcpu again. If that the case it shouldn't trigger the single step since the instruction is not completely finished.

Maybe I don't interpret this correctly or it is not relevant here. Although I'd like to understand why.

No it certainly needs clarifying. The comment was originally added in:

   679613442f84702c06a80f2320cb8a50089200bc

Looking more closely though it has a point. The IO/MMIO exits work purely from the address and data entries in the run structure. When we return to KVM_RUN we do:

	if (run->exit_reason == KVM_EXIT_MMIO) {
		ret = kvm_handle_mmio_return(vcpu, vcpu->run);
		if (ret)
			return ret;
	}

So you are correct the instruction emulation is not complete. Once that fixup is done however I think we are good to return. So perhaps we can avoid involving QEMU entirely in this by generating a debug exit here.

QEMU ->

         kvm_run ->
                     switch ->
                               guest
                            <-
                     trap
                  <-
         exit mmio
      <-
QEMU
      -> kvm_run
         handle_mmio_return
         exit debug
      <-

QEMU

I don't think this affects the handle_exit() case as we only force the exit for successfully emulated instructions inside kvm.

Looking at x86 for reference it does seem happy with triggering exits on single stepped emulation, see kvm_vcpu_do_singlestep(). However it also has a number of comments on IO emulation:

   /* FIXME: return into emulator if single-stepping.  */

So ARM is at least not behind the curve on this support ;-)

Quoted 7 lines

Quoted 6 lines

I'd like to test these patches on some real life examples. I tried tracing over the pl011_write code in a kernel boot but I ran into the problem of el1_irq's occurring as I tried to step through the guest kernel. Is this something you've come across? What MMIO accesses have you been using in your testing?

Quoted 2 lines

I didn't know which MMIOs were handled by userland so I have only tested traps and MMIOs handled by the kernel.

Any particular MMIOs I could also use to replicate in my tests?

Quoted 6 lines

This sounds like an issue when you are debugging an interruptible context, an issue Pratyush has been looking at [1]. Are you taking a guest interrupt when you try to execute the instruction to be stepped? I don't know what's the status of this patch series. Can you test the userland MMIO in a non-interruptible context?

Quoted 2 lines

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-July/517234.html

Again looking at x86 it looks like the approach is to suspend IRQs if you are single-stepping. I'll have a look at Pratyush's patches.

Quoted 40 lines

Quoted 40 lines

QEMU Patch bellow:

From 2e8fcea695a9eca67fbeb331d3104d1d9e7e337a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <[email protected]>
Date: Wed, 4 Oct 2017 09:49:41 +0000
Subject: [PATCH] kvm: exit run loop after emulating IO when single stepping
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If single-stepping is enabled we should exit the run-loop after emulating the access. Otherwise single-stepping across emulated IO accesses may skip an instruction.

This only addresses user-space emulation. Stuff done in kernel-mode should be handled there.

Signed-off-by Alex Benn?e <[email protected]>

---
   accel/kvm/kvm-all.c | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)

accel/kvm/kvm-all.c +1 -1

index 90c88b517d..85bcb2b0d4 100644
@@ -1940,7 +1940,7 @@ int kvm_cpu_exec(CPUState *cpu)
                             run->io.direction,
                             run->io.size,
                             run->io.count);
-            ret = 0;
+            ret = cpu->singlestep_enabled ? EXCP_DEBUG : 0;
               break;
           case KVM_EXIT_MMIO:
               DPRINTF("handle_mmio\n");

@@ -1950,7 +1950,7 @@ int kvm_cpu_exec(CPUState *cpu)
                                run->mmio.data,
                                run->mmio.len,
                                run->mmio.is_write);

Quoted 8 lines

Quoted 8 lines

accel/kvm/kvm-all.c +1 -2

-            ret = 0;
+            ret = cpu->singlestep_enabled ? EXCP_DEBUG : 0;
               break;
           case KVM_EXIT_IRQ_WINDOW_OPEN:
               DPRINTF("irq_window_open\n");
--

2.14.1

--
Alex Benn?e

Quoted 1 line · Alex Benn?e

Quoted 30 lines

Looking more closely though it has a point. The IO/MMIO exits work purely from the address and data entries in the run structure. When we return to KVM_RUN we do:

	if (run->exit_reason == KVM_EXIT_MMIO) {
		ret = kvm_handle_mmio_return(vcpu, vcpu->run);
		if (ret)
			return ret;
	}

So you are correct the instruction emulation is not complete. Once that fixup is done however I think we are good to return. So perhaps we can avoid involving QEMU entirely in this by generating a debug exit here.

QEMU ->

         kvm_run ->
                     switch ->
                               guest
                            <-
                     trap
                  <-
         exit mmio
      <-
QEMU
      -> kvm_run
         handle_mmio_return
         exit debug
      <-

Quoted 2 lines

QEMU

Thanks for the explanation. This approach sounds good to me. Also, it means QEMU doesn't need to get patched, is that correct?

Quoted 4 lines

I don't think this affects the handle_exit() case as we only force the exit for successfully emulated instructions inside kvm.

Yes, in handle_exit MMIO handled by the kernel will exit with KVM_EXIT_DEBUG and MMIO handled by the userland will exit with KVM_EXIT_MMIO. So from your patch we just need to add the exit debug after handle_mmio_return.

Quoted 19 lines

Looking at x86 for reference it does seem happy with triggering exits on single stepped emulation, see kvm_vcpu_do_singlestep(). However it also has a number of comments on IO emulation:

   /* FIXME: return into emulator if single-stepping.  */

So ARM is at least not behind the curve on this support ;-)

Quoted 9 lines

Quoted 6 lines

I'd like to test these patches on some real life examples. I tried tracing over the pl011_write code in a kernel boot but I ran into the problem of el1_irq's occurring as I tried to step through the guest kernel. Is this something you've come across? What MMIO accesses have you been using in your testing?

I didn't know which MMIOs were handled by userland so I have only tested traps and MMIOs handled by the kernel.

Quoted 2 lines

Any particular MMIOs I could also use to replicate in my tests?

For MMIOs handled by the kernel, I was testing with the GIC. You could try to break on gic_cpu_config in the guest, where it will write to distributor registers. The function should be called during initialization, before IRQs are enabled, so you shouldn't be bothered by the earlier trouble.

Quoted 9 lines

Quoted 8 lines

This sounds like an issue when you are debugging an interruptible context, an issue Pratyush has been looking at [1]. Are you taking a guest interrupt when you try to execute the instruction to be stepped? I don't know what's the status of this patch series. Can you test the userland MMIO in a non-interruptible context?

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-July/517234.html

Quoted 3 lines

Again looking at x86 it looks like the approach is to suspend IRQs if you are single-stepping. I'll have a look at Pratyush's patches.

I think that was the idea with Pratyush's patches as well.

So, I'm happy to replace my patch with yours in this series (unless you want to post it separated since it doesn't depend on my patches).

Thank you for looking at this and providing your input.