[PATCH 0/3] Fix single step for traps

[PATCH 0/3] Fix single step for traps

[Julien Thierry]

Hi,

When single stepping a trapped/emulated instruction, the instruction not being
actually executed, the PE ends up single stepping the instruction we return to
after ERET-ing from the trap.

The issue affects traps in the kernel and emulated instructions in KVM guests.

This patch series ensures we properly single step trapped instruction.

First patch is just to avoid raw values when using single stepping
registers/bits.
Patches 2 and 3 fix the issue for kernel and kvm respectively.

Cheers,

Julien

Julien Thierry (3):
  arm64: Use existing defines for mdscr
  arm64: Fix single stepping in kernel traps
  arm64: kvm: Fix single step for guest skipped instructions

 arch/arm64/include/asm/assembler.h   |  5 +++--
 arch/arm64/include/asm/kvm_asm.h     |  2 ++
 arch/arm64/include/asm/kvm_emulate.h |  2 ++
 arch/arm64/include/asm/traps.h       |  2 ++
 arch/arm64/kernel/armv8_deprecated.c |  8 ++++----
 arch/arm64/kernel/cpufeature.c       |  2 +-
 arch/arm64/kernel/traps.c            | 21 ++++++++++++++++-----
 arch/arm64/kvm/debug.c               | 12 +++++++++++-
 arch/arm64/kvm/hyp/switch.c          | 12 ++++++++++++
 9 files changed, 53 insertions(+), 13 deletions(-)

--
1.9.1

[PATCH 1/3] arm64: Use existing defines for mdscr

[Julien Thierry]

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>

---
 arch/arm64/include/asm/assembler.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index 1b67c37..5d9e57a 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -30,6 +30,7 @@
 #include <asm/pgtable-hwdef.h>
 #include <asm/ptrace.h>
 #include <asm/thread_info.h>
+#include <asm/debug-monitors.h>

 /*
  * Enable and disable interrupts.
@@ -65,7 +66,7 @@
 	.macro	disable_step_tsk, flgs, tmp
 	tbz	\flgs, #TIF_SINGLESTEP, 9990f
 	mrs	\tmp, mdscr_el1
-	bic	\tmp, \tmp, #1
+	bic	\tmp, \tmp, #DBG_MDSCR_SS
 	msr	mdscr_el1, \tmp
 	isb	// Synchronise with enable_dbg
 9990:
@@ -75,7 +76,7 @@
 	tbz	\flgs, #TIF_SINGLESTEP, 9990f
 	disable_dbg
 	mrs	\tmp, mdscr_el1
-	orr	\tmp, \tmp, #1
+	orr	\tmp, \tmp, #DBG_MDSCR_SS
 	msr	mdscr_el1, \tmp
 9990:
 	.endm
--
1.9.1

[PATCH 2/3] arm64: Fix single stepping in kernel traps

[Julien Thierry]

Software Step exception is missing after stepping a trapped instruction.

Ensure SPSR.SS gets set to 0 after emulating/skipping a trapped instruction
before doing ERET.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>

---
 arch/arm64/include/asm/traps.h       |  2 ++
 arch/arm64/kernel/armv8_deprecated.c |  8 ++++----
 arch/arm64/kernel/cpufeature.c       |  2 +-
 arch/arm64/kernel/traps.c            | 21 ++++++++++++++++-----
 4 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/include/asm/traps.h b/arch/arm64/include/asm/traps.h
index 02e9035..dfeb489 100644
--- a/arch/arm64/include/asm/traps.h
+++ b/arch/arm64/include/asm/traps.h
@@ -37,6 +37,8 @@ struct undef_hook {

 void arm64_notify_segfault(struct pt_regs *regs, unsigned long addr);

+void arm64_skip_trapped_instr(struct pt_regs *regs, unsigned long size);
+
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 static inline int __in_irqentry_text(unsigned long ptr)
 {
diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c
index f0e6d71..1f38208 100644
--- a/arch/arm64/kernel/armv8_deprecated.c
+++ b/arch/arm64/kernel/armv8_deprecated.c
@@ -431,7 +431,7 @@ static int swp_handler(struct pt_regs *regs, u32 instr)
 	pr_warn_ratelimited("\"%s\" (%ld) uses obsolete SWP{B} instruction at 0x%llx\n",
 			current->comm, (unsigned long)current->pid, regs->pc);

-	regs->pc += 4;
+	arm64_skip_trapped_instr(regs, 4);
 	return 0;

 fault:
@@ -512,7 +512,7 @@ static int cp15barrier_handler(struct pt_regs *regs, u32 instr)
 	pr_warn_ratelimited("\"%s\" (%ld) uses deprecated CP15 Barrier instruction at 0x%llx\n",
 			current->comm, (unsigned long)current->pid, regs->pc);

-	regs->pc += 4;
+	arm64_skip_trapped_instr(regs, 4);
 	return 0;
 }

@@ -586,14 +586,14 @@ static int compat_setend_handler(struct pt_regs *regs, u32 big_endian)
 static int a32_setend_handler(struct pt_regs *regs, u32 instr)
 {
 	int rc = compat_setend_handler(regs, (instr >> 9) & 1);
-	regs->pc += 4;
+	arm64_skip_trapped_instr(regs, 4);
 	return rc;
 }

 static int t16_setend_handler(struct pt_regs *regs, u32 instr)
 {
 	int rc = compat_setend_handler(regs, (instr >> 3) & 1);
-	regs->pc += 2;
+	arm64_skip_trapped_instr(regs, 2);
 	return rc;
 }

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 9f9e0064..06c9d5f 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1274,7 +1274,7 @@ static int emulate_mrs(struct pt_regs *regs, u32 insn)
 	if (!rc) {
 		dst = aarch64_insn_decode_register(AARCH64_INSN_REGTYPE_RT, insn);
 		pt_regs_write_reg(regs, dst, val);
-		regs->pc += 4;
+		arm64_skip_trapped_instr(regs, AARCH64_INSN_SIZE);
 	}

 	return rc;
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 8a62648..1f37bc1 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -314,6 +314,17 @@ void arm64_notify_die(const char *str, struct pt_regs *regs,
 	}
 }

+void arm64_skip_trapped_instr(struct pt_regs *regs, unsigned long size)
+{
+	regs->pc += size;
+
+	/*
+	 * If we were single stepping, we want to get the step exception after
+	 * we return from the skipped exception
+	 */
+	regs->pstate &= ~DBG_SPSR_SS;
+}
+
 static LIST_HEAD(undef_hook);
 static DEFINE_RAW_SPINLOCK(undef_lock);

@@ -498,7 +509,7 @@ static void user_cache_maint_handler(unsigned int esr, struct pt_regs *regs)
 	if (ret)
 		arm64_notify_segfault(regs, address);
 	else
-		regs->pc += 4;
+		arm64_skip_trapped_instr(regs, AARCH64_INSN_SIZE);
 }

 static void ctr_read_handler(unsigned int esr, struct pt_regs *regs)
@@ -508,7 +519,7 @@ static void ctr_read_handler(unsigned int esr, struct pt_regs *regs)

 	pt_regs_write_reg(regs, rt, val);

-	regs->pc += 4;
+	arm64_skip_trapped_instr(regs, AARCH64_INSN_SIZE);
 }

 static void cntvct_read_handler(unsigned int esr, struct pt_regs *regs)
@@ -516,7 +527,7 @@ static void cntvct_read_handler(unsigned int esr, struct pt_regs *regs)
 	int rt = (esr & ESR_ELx_SYS64_ISS_RT_MASK) >> ESR_ELx_SYS64_ISS_RT_SHIFT;

 	pt_regs_write_reg(regs, rt, arch_counter_get_cntvct());
-	regs->pc += 4;
+	arm64_skip_trapped_instr(regs, AARCH64_INSN_SIZE);
 }

 static void cntfrq_read_handler(unsigned int esr, struct pt_regs *regs)
@@ -524,7 +535,7 @@ static void cntfrq_read_handler(unsigned int esr, struct pt_regs *regs)
 	int rt = (esr & ESR_ELx_SYS64_ISS_RT_MASK) >> ESR_ELx_SYS64_ISS_RT_SHIFT;

 	pt_regs_write_reg(regs, rt, arch_timer_get_rate());
-	regs->pc += 4;
+	arm64_skip_trapped_instr(regs, AARCH64_INSN_SIZE);
 }

 struct sys64_hook {
@@ -742,7 +753,7 @@ static int bug_handler(struct pt_regs *regs, unsigned int esr)
 	}

 	/* If thread survives, skip over the BUG instruction and continue: */
-	regs->pc += AARCH64_INSN_SIZE;	/* skip BRK and resume */
+	arm64_skip_trapped_instr(regs, AARCH64_INSN_SIZE);
 	return DBG_HOOK_HANDLED;
 }

--
1.9.1

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

[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 <julien.thierry@arm.com>
Cc: Christoffer Dall <christoffer.dall@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>

---
 arch/arm64/include/asm/kvm_asm.h     |  2 ++
 arch/arm64/include/asm/kvm_emulate.h |  2 ++
 arch/arm64/kvm/debug.c               | 12 +++++++++++-
 arch/arm64/kvm/hyp/switch.c          | 12 ++++++++++++
 4 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index 26a64d0..398bbaa 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -32,6 +32,8 @@

 #define KVM_ARM64_DEBUG_DIRTY_SHIFT	0
 #define KVM_ARM64_DEBUG_DIRTY		(1 << KVM_ARM64_DEBUG_DIRTY_SHIFT)
+#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT	1
+#define KVM_ARM64_DEBUG_INST_SKIP	(1 << KVM_ARM64_DEBUG_INST_SKIP_SHIFT)

 #define kvm_ksym_ref(sym)						\
 	({								\
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index fe39e68..d401c64 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu *vcpu, bool is_wide_instr)
 		kvm_skip_instr32(vcpu, is_wide_instr);
 	else
 		*vcpu_pc(vcpu) += 4;
+	/* Let debug engine know we skipped an instruction */
+	vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP;
 }

 static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu)
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index dbadfaf..4806e6b 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -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));

 		/*
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 945e79c..6030acd 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -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)
@@ -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;
+
 				exit_code = ARM_EXCEPTION_EL1_SERROR;
 			}

--
1.9.1

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

[Marc Zyngier]

Hi Julien,

On 30/08/17 10:01, Julien Thierry wrote:
> Software Step exception is missing after trapping instruction from the guest.
> 
> We need to set the PSR.SS to 0 for the guest vcpu before resuming guest
> execution.
> 
> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> 
> ---
>  arch/arm64/include/asm/kvm_asm.h     |  2 ++
>  arch/arm64/include/asm/kvm_emulate.h |  2 ++
>  arch/arm64/kvm/debug.c               | 12 +++++++++++-
>  arch/arm64/kvm/hyp/switch.c          | 12 ++++++++++++
>  4 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index 26a64d0..398bbaa 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -32,6 +32,8 @@
> 
>  #define KVM_ARM64_DEBUG_DIRTY_SHIFT	0
>  #define KVM_ARM64_DEBUG_DIRTY		(1 << KVM_ARM64_DEBUG_DIRTY_SHIFT)
> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT	1
> +#define KVM_ARM64_DEBUG_INST_SKIP	(1 << KVM_ARM64_DEBUG_INST_SKIP_SHIFT)
> 
>  #define kvm_ksym_ref(sym)						\
>  	({								\
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index fe39e68..d401c64 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu *vcpu, bool is_wide_instr)
>  		kvm_skip_instr32(vcpu, is_wide_instr);
>  	else
>  		*vcpu_pc(vcpu) += 4;
> +	/* Let debug engine know we skipped an instruction */
> +	vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP;
>  }
> 
>  static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu)
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index dbadfaf..4806e6b 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -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));
> 
>  		/*
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 945e79c..6030acd 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -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)
> @@ -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?

>  				exit_code = ARM_EXCEPTION_EL1_SERROR;
>  			}
> 
> --
> 1.9.1
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

[Julien Thierry]

Hi Marc,

On 30/08/17 10:19, Marc Zyngier wrote:
> Hi Julien,
> 
> On 30/08/17 10:01, Julien Thierry wrote:
>> Software Step exception is missing after trapping instruction from the guest.
>>
>> We need to set the PSR.SS to 0 for the guest vcpu before resuming guest
>> execution.
>>
>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>>
>> ---
>>   arch/arm64/include/asm/kvm_asm.h     |  2 ++
>>   arch/arm64/include/asm/kvm_emulate.h |  2 ++
>>   arch/arm64/kvm/debug.c               | 12 +++++++++++-
>>   arch/arm64/kvm/hyp/switch.c          | 12 ++++++++++++
>>   4 files changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
>> index 26a64d0..398bbaa 100644
>> --- a/arch/arm64/include/asm/kvm_asm.h
>> +++ b/arch/arm64/include/asm/kvm_asm.h
>> @@ -32,6 +32,8 @@
>>
>>   #define KVM_ARM64_DEBUG_DIRTY_SHIFT	0
>>   #define KVM_ARM64_DEBUG_DIRTY		(1 << KVM_ARM64_DEBUG_DIRTY_SHIFT)
>> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT	1
>> +#define KVM_ARM64_DEBUG_INST_SKIP	(1 << KVM_ARM64_DEBUG_INST_SKIP_SHIFT)
>>
>>   #define kvm_ksym_ref(sym)						\
>>   	({								\
>> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
>> index fe39e68..d401c64 100644
>> --- a/arch/arm64/include/asm/kvm_emulate.h
>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu *vcpu, bool is_wide_instr)
>>   		kvm_skip_instr32(vcpu, is_wide_instr);
>>   	else
>>   		*vcpu_pc(vcpu) += 4;
>> +	/* Let debug engine know we skipped an instruction */
>> +	vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP;
>>   }
>>
>>   static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu)
>> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
>> index dbadfaf..4806e6b 100644
>> --- a/arch/arm64/kvm/debug.c
>> +++ b/arch/arm64/kvm/debug.c
>> @@ -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));
>>
>>   		/*
>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
>> index 945e79c..6030acd 100644
>> --- a/arch/arm64/kvm/hyp/switch.c
>> +++ b/arch/arm64/kvm/hyp/switch.c
>> @@ -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)
>> @@ -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?

The thing is that whenever you get out of __kvm_vcpu_run, 
kvm_arm_setup_debug will get called before running a guest again. So my 
idea is that kvm_arm_setup_debug needs to be aware whether SPSR.SS must 
be set or not. Otherwise kvm_arm_setup_debug will always set the SPSR.SS 
flag. So KVM_ARM64_DEBUG_INST_SKIP is to tell the debug setup that we 
skipped an instruction (here as well as in kvm_skip_instr).

The fact that SPSR.SS is clear in the illegal access case is because it 
uses __skip_instr instead of kvm_skip_instr because we have not saved 
the guest state at that point. But the clearing of SPSR.SS in 
__skip_instr is more targeted at cases that directly jump back into the 
guest (trapped GICv2/3 CPUIF accesses).

Let me know if you feel there is a cleaner way to do this or something 
that would make this clearer.

Thanks,

-- 
Julien Thierry

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

[Christoffer Dall]

Hi Julien,

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

On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry <julien.thierry@arm.com> wrote:
> Software Step exception is missing after trapping instruction from the guest.
>
> We need to set the PSR.SS to 0 for the guest vcpu before resuming guest
> execution.
>
> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
>
> ---
>  arch/arm64/include/asm/kvm_asm.h     |  2 ++
>  arch/arm64/include/asm/kvm_emulate.h |  2 ++
>  arch/arm64/kvm/debug.c               | 12 +++++++++++-
>  arch/arm64/kvm/hyp/switch.c          | 12 ++++++++++++
>  4 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index 26a64d0..398bbaa 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -32,6 +32,8 @@
>
>  #define KVM_ARM64_DEBUG_DIRTY_SHIFT    0
>  #define KVM_ARM64_DEBUG_DIRTY          (1 << KVM_ARM64_DEBUG_DIRTY_SHIFT)
> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT        1
> +#define KVM_ARM64_DEBUG_INST_SKIP      (1 << KVM_ARM64_DEBUG_INST_SKIP_SHIFT)
>
>  #define kvm_ksym_ref(sym)                                              \
>         ({                                                              \
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index fe39e68..d401c64 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu *vcpu, bool is_wide_instr)
>                 kvm_skip_instr32(vcpu, is_wide_instr);
>         else
>                 *vcpu_pc(vcpu) += 4;
> +       /* Let debug engine know we skipped an instruction */
> +       vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP;

Why do we need to defer this action until later?

Can't we simply do clear DBG_SPSR_SS here?

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.

>  }
>
>  static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu)
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index dbadfaf..4806e6b 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -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;

Then you wouldn't need this.

>                         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));
>
>                 /*
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 945e79c..6030acd 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -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);

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.

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

>                                 exit_code = ARM_EXCEPTION_EL1_SERROR;
>                         }
>
> --
> 1.9.1

Thanks,
-Christoffer

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

[Julien Thierry]

Hi Christoffer,

On 30/08/17 19:53, Christoffer Dall wrote:
> Hi Julien,
> 
> [cc'ing Alex Benn?e here who wrote the debug code for arm64]
> 
> On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry <julien.thierry@arm.com> wrote:
>> Software Step exception is missing after trapping instruction from the guest.
>>
>> We need to set the PSR.SS to 0 for the guest vcpu before resuming guest
>> execution.
>>
>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>>
>> ---
>>   arch/arm64/include/asm/kvm_asm.h     |  2 ++
>>   arch/arm64/include/asm/kvm_emulate.h |  2 ++
>>   arch/arm64/kvm/debug.c               | 12 +++++++++++-
>>   arch/arm64/kvm/hyp/switch.c          | 12 ++++++++++++
>>   4 files changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
>> index 26a64d0..398bbaa 100644
>> --- a/arch/arm64/include/asm/kvm_asm.h
>> +++ b/arch/arm64/include/asm/kvm_asm.h
>> @@ -32,6 +32,8 @@
>>
>>   #define KVM_ARM64_DEBUG_DIRTY_SHIFT    0
>>   #define KVM_ARM64_DEBUG_DIRTY          (1 << KVM_ARM64_DEBUG_DIRTY_SHIFT)
>> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT        1
>> +#define KVM_ARM64_DEBUG_INST_SKIP      (1 << KVM_ARM64_DEBUG_INST_SKIP_SHIFT)
>>
>>   #define kvm_ksym_ref(sym)                                              \
>>          ({                                                              \
>> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
>> index fe39e68..d401c64 100644
>> --- a/arch/arm64/include/asm/kvm_emulate.h
>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu *vcpu, bool is_wide_instr)
>>                  kvm_skip_instr32(vcpu, is_wide_instr);
>>          else
>>                  *vcpu_pc(vcpu) += 4;
>> +       /* Let debug engine know we skipped an instruction */
>> +       vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP;
> 
> Why do we need to defer this action until later?
> 
> Can't we simply do clear DBG_SPSR_SS here?
> 

That was my first intention, but it turns out that the current state of 
things (without this patch) is that every time we enter a guest, 
kvm_arm_setup_debug gets called and if single step is requested for the 
guest it will set the flag in the SPSR (ignoring the fact that we 
cleared it). 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...


> 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.
> 
>>   }
>>
>>   static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu)
>> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
>> index dbadfaf..4806e6b 100644
>> --- a/arch/arm64/kvm/debug.c
>> +++ b/arch/arm64/kvm/debug.c
>> @@ -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;
> 
> Then you wouldn't need this.
> 
>>                          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));
>>
>>                  /*
>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
>> index 945e79c..6030acd 100644
>> --- a/arch/arm64/kvm/hyp/switch.c
>> +++ b/arch/arm64/kvm/hyp/switch.c
>> @@ -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);
> 
> 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.

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

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

Thanks,

-- 
Julien Thierry

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

[Christoffer Dall]

On Thu, Aug 31, 2017 at 10:45 AM, Julien Thierry <julien.thierry@arm.com> wrote:
> Hi Christoffer,
>
>
> On 30/08/17 19:53, Christoffer Dall wrote:
>>
>> Hi Julien,
>>
>> [cc'ing Alex Benn?e here who wrote the debug code for arm64]
>>
>> On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry <julien.thierry@arm.com>
>> wrote:
>>>
>>> Software Step exception is missing after trapping instruction from the
>>> guest.
>>>
>>> We need to set the PSR.SS to 0 for the guest vcpu before resuming guest
>>> execution.
>>>
>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>> Cc: Will Deacon <will.deacon@arm.com>
>>>
>>> ---
>>>   arch/arm64/include/asm/kvm_asm.h     |  2 ++
>>>   arch/arm64/include/asm/kvm_emulate.h |  2 ++
>>>   arch/arm64/kvm/debug.c               | 12 +++++++++++-
>>>   arch/arm64/kvm/hyp/switch.c          | 12 ++++++++++++
>>>   4 files changed, 27 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/include/asm/kvm_asm.h
>>> b/arch/arm64/include/asm/kvm_asm.h
>>> index 26a64d0..398bbaa 100644
>>> --- a/arch/arm64/include/asm/kvm_asm.h
>>> +++ b/arch/arm64/include/asm/kvm_asm.h
>>> @@ -32,6 +32,8 @@
>>>
>>>   #define KVM_ARM64_DEBUG_DIRTY_SHIFT    0
>>>   #define KVM_ARM64_DEBUG_DIRTY          (1 <<
>>> KVM_ARM64_DEBUG_DIRTY_SHIFT)
>>> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT        1
>>> +#define KVM_ARM64_DEBUG_INST_SKIP      (1 <<
>>> KVM_ARM64_DEBUG_INST_SKIP_SHIFT)
>>>
>>>   #define kvm_ksym_ref(sym)
>>> \
>>>          ({
>>> \
>>> diff --git a/arch/arm64/include/asm/kvm_emulate.h
>>> b/arch/arm64/include/asm/kvm_emulate.h
>>> index fe39e68..d401c64 100644
>>> --- a/arch/arm64/include/asm/kvm_emulate.h
>>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>>> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu
>>> *vcpu, bool is_wide_instr)
>>>                  kvm_skip_instr32(vcpu, is_wide_instr);
>>>          else
>>>                  *vcpu_pc(vcpu) += 4;
>>> +       /* Let debug engine know we skipped an instruction */
>>> +       vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP;
>>
>>
>> Why do we need to defer this action until later?
>>
>> Can't we simply do clear DBG_SPSR_SS here?
>>
>
> That was my first intention, but it turns out that the current state of
> things (without this patch) is that every time we enter a guest,
> kvm_arm_setup_debug gets called and if single step is requested for the
> guest it will set the flag in the SPSR (ignoring the fact that we cleared
> it).

Ah, right, duh.

> This happens even if we exit the guest because of a data abort.
>
> For normal single step execution, we do need to reset SPSR.SS to 1 before
> running the guest since completion of a step should clear that bit before
> taking a software step exception. So what kvm_arm_setup_debug does seems
> correct to me but missed the case for trapped/emulated instructions.
>
> So even if we just clear DBG_SPSR_SS here, we would still need to tell
> kvm_arm_setup_debug not to change the bit. Or resetting SPSR.SS to 1 for
> normal single stepping needs to be done before we skip instructions in KVM
> but that doesn't sound right to me...
>

So I'm wondering if we're going about this wrong.  Perhaps we need to
discover at the end of the run loop that we were asked to single step
execution and simply return to userspace, setting the debug exit
reason etc., instead of entering the guest with PSTATE.SS==0 and
relying on another trap back in to the guest just to set two fields on
the kvm_run structure and exit to user space ?

>
>
>> 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.
>>
>>>   }
>>>
>>>   static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu)
>>> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
>>> index dbadfaf..4806e6b 100644
>>> --- a/arch/arm64/kvm/debug.c
>>> +++ b/arch/arm64/kvm/debug.c
>>> @@ -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;
>>
>>
>> Then you wouldn't need this.
>>
>>>                          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));
>>>
>>>                  /*
>>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
>>> index 945e79c..6030acd 100644
>>> --- a/arch/arm64/kvm/hyp/switch.c
>>> +++ b/arch/arm64/kvm/hyp/switch.c
>>> @@ -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);
>>
>>
>> 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.
>
>>>   }
>>>
>>>   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().
>>
>
> 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

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

[Julien Thierry]

On 31/08/17 09:54, Christoffer Dall wrote:
> On Thu, Aug 31, 2017 at 10:45 AM, Julien Thierry <julien.thierry@arm.com> wrote:
>> Hi Christoffer,
>>
>>
>> On 30/08/17 19:53, Christoffer Dall wrote:
>>>
>>> Hi Julien,
>>>
>>> [cc'ing Alex Benn?e here who wrote the debug code for arm64]
>>>
>>> On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry <julien.thierry@arm.com>
>>> wrote:
>>>>
>>>> Software Step exception is missing after trapping instruction from the
>>>> guest.
>>>>
>>>> We need to set the PSR.SS to 0 for the guest vcpu before resuming guest
>>>> execution.
>>>>
>>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>>>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>>
>>>> ---
>>>>    arch/arm64/include/asm/kvm_asm.h     |  2 ++
>>>>    arch/arm64/include/asm/kvm_emulate.h |  2 ++
>>>>    arch/arm64/kvm/debug.c               | 12 +++++++++++-
>>>>    arch/arm64/kvm/hyp/switch.c          | 12 ++++++++++++
>>>>    4 files changed, 27 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm64/include/asm/kvm_asm.h
>>>> b/arch/arm64/include/asm/kvm_asm.h
>>>> index 26a64d0..398bbaa 100644
>>>> --- a/arch/arm64/include/asm/kvm_asm.h
>>>> +++ b/arch/arm64/include/asm/kvm_asm.h
>>>> @@ -32,6 +32,8 @@
>>>>
>>>>    #define KVM_ARM64_DEBUG_DIRTY_SHIFT    0
>>>>    #define KVM_ARM64_DEBUG_DIRTY          (1 <<
>>>> KVM_ARM64_DEBUG_DIRTY_SHIFT)
>>>> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT        1
>>>> +#define KVM_ARM64_DEBUG_INST_SKIP      (1 <<
>>>> KVM_ARM64_DEBUG_INST_SKIP_SHIFT)
>>>>
>>>>    #define kvm_ksym_ref(sym)
>>>> \
>>>>           ({
>>>> \
>>>> diff --git a/arch/arm64/include/asm/kvm_emulate.h
>>>> b/arch/arm64/include/asm/kvm_emulate.h
>>>> index fe39e68..d401c64 100644
>>>> --- a/arch/arm64/include/asm/kvm_emulate.h
>>>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>>>> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu
>>>> *vcpu, bool is_wide_instr)
>>>>                   kvm_skip_instr32(vcpu, is_wide_instr);
>>>>           else
>>>>                   *vcpu_pc(vcpu) += 4;
>>>> +       /* Let debug engine know we skipped an instruction */
>>>> +       vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP;
>>>
>>>
>>> Why do we need to defer this action until later?
>>>
>>> Can't we simply do clear DBG_SPSR_SS here?
>>>
>>
>> That was my first intention, but it turns out that the current state of
>> things (without this patch) is that every time we enter a guest,
>> kvm_arm_setup_debug gets called and if single step is requested for the
>> guest it will set the flag in the SPSR (ignoring the fact that we cleared
>> it).
> 
> Ah, right, duh.
> 
>> This happens even if we exit the guest because of a data abort.
>>
>> For normal single step execution, we do need to reset SPSR.SS to 1 before
>> running the guest since completion of a step should clear that bit before
>> taking a software step exception. So what kvm_arm_setup_debug does seems
>> correct to me but missed the case for trapped/emulated instructions.
>>
>> So even if we just clear DBG_SPSR_SS here, we would still need to tell
>> kvm_arm_setup_debug not to change the bit. Or resetting SPSR.SS to 1 for
>> normal single stepping needs to be done before we skip instructions in KVM
>> but that doesn't sound right to me...
>>
> 
> So I'm wondering if we're going about this wrong.  Perhaps we need to
> discover at the end of the run loop that we were asked to single step
> execution and simply return to userspace, setting the debug exit
> reason etc., instead of entering the guest with PSTATE.SS==0 and
> relying on another trap back in to the guest just to set two fields on
> the kvm_run structure and exit to user space ?
> 

So if I understand correctly, the suggestion is that when we trap an 
instruction we check whether it was supposed to be single stepped, if it 
was we set up the vcpu registers as if it had taken a software step 
exception and return from kvm_arch_vcpu_ioctl_run. Is that right?

Interesting idea, I can try to explore that possibility.

Thanks for the suggestion,

>>
>>
>>> 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.
>>>
>>>>    }
>>>>
>>>>    static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu)
>>>> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
>>>> index dbadfaf..4806e6b 100644
>>>> --- a/arch/arm64/kvm/debug.c
>>>> +++ b/arch/arm64/kvm/debug.c
>>>> @@ -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;
>>>
>>>
>>> Then you wouldn't need this.
>>>
>>>>                           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));
>>>>
>>>>                   /*
>>>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
>>>> index 945e79c..6030acd 100644
>>>> --- a/arch/arm64/kvm/hyp/switch.c
>>>> +++ b/arch/arm64/kvm/hyp/switch.c
>>>> @@ -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);
>>>
>>>
>>> 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.
>>
>>>>    }
>>>>
>>>>    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().
>>>
>>
>> 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
> 

-- 
Julien Thierry

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

[Christoffer Dall]

On Thu, Aug 31, 2017 at 11:37 AM, Julien Thierry <julien.thierry@arm.com> wrote:
>
>
> On 31/08/17 09:54, Christoffer Dall wrote:
>>
>> On Thu, Aug 31, 2017 at 10:45 AM, Julien Thierry <julien.thierry@arm.com>
>> wrote:
>>>
>>> Hi Christoffer,
>>>
>>>
>>> On 30/08/17 19:53, Christoffer Dall wrote:
>>>>
>>>>
>>>> Hi Julien,
>>>>
>>>> [cc'ing Alex Benn?e here who wrote the debug code for arm64]
>>>>
>>>> On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry
>>>> <julien.thierry@arm.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> Software Step exception is missing after trapping instruction from the
>>>>> guest.
>>>>>
>>>>> We need to set the PSR.SS to 0 for the guest vcpu before resuming guest
>>>>> execution.
>>>>>
>>>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>>>>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>>>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>>>
>>>>> ---
>>>>>    arch/arm64/include/asm/kvm_asm.h     |  2 ++
>>>>>    arch/arm64/include/asm/kvm_emulate.h |  2 ++
>>>>>    arch/arm64/kvm/debug.c               | 12 +++++++++++-
>>>>>    arch/arm64/kvm/hyp/switch.c          | 12 ++++++++++++
>>>>>    4 files changed, 27 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/arm64/include/asm/kvm_asm.h
>>>>> b/arch/arm64/include/asm/kvm_asm.h
>>>>> index 26a64d0..398bbaa 100644
>>>>> --- a/arch/arm64/include/asm/kvm_asm.h
>>>>> +++ b/arch/arm64/include/asm/kvm_asm.h
>>>>> @@ -32,6 +32,8 @@
>>>>>
>>>>>    #define KVM_ARM64_DEBUG_DIRTY_SHIFT    0
>>>>>    #define KVM_ARM64_DEBUG_DIRTY          (1 <<
>>>>> KVM_ARM64_DEBUG_DIRTY_SHIFT)
>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT        1
>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP      (1 <<
>>>>> KVM_ARM64_DEBUG_INST_SKIP_SHIFT)
>>>>>
>>>>>    #define kvm_ksym_ref(sym)
>>>>> \
>>>>>           ({
>>>>> \
>>>>> diff --git a/arch/arm64/include/asm/kvm_emulate.h
>>>>> b/arch/arm64/include/asm/kvm_emulate.h
>>>>> index fe39e68..d401c64 100644
>>>>> --- a/arch/arm64/include/asm/kvm_emulate.h
>>>>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>>>>> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu
>>>>> *vcpu, bool is_wide_instr)
>>>>>                   kvm_skip_instr32(vcpu, is_wide_instr);
>>>>>           else
>>>>>                   *vcpu_pc(vcpu) += 4;
>>>>> +       /* Let debug engine know we skipped an instruction */
>>>>> +       vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP;
>>>>
>>>>
>>>>
>>>> Why do we need to defer this action until later?
>>>>
>>>> Can't we simply do clear DBG_SPSR_SS here?
>>>>
>>>
>>> That was my first intention, but it turns out that the current state of
>>> things (without this patch) is that every time we enter a guest,
>>> kvm_arm_setup_debug gets called and if single step is requested for the
>>> guest it will set the flag in the SPSR (ignoring the fact that we cleared
>>> it).
>>
>>
>> Ah, right, duh.
>>
>>> This happens even if we exit the guest because of a data abort.
>>>
>>> For normal single step execution, we do need to reset SPSR.SS to 1 before
>>> running the guest since completion of a step should clear that bit before
>>> taking a software step exception. So what kvm_arm_setup_debug does seems
>>> correct to me but missed the case for trapped/emulated instructions.
>>>
>>> So even if we just clear DBG_SPSR_SS here, we would still need to tell
>>> kvm_arm_setup_debug not to change the bit. Or resetting SPSR.SS to 1 for
>>> normal single stepping needs to be done before we skip instructions in
>>> KVM
>>> but that doesn't sound right to me...
>>>
>>
>> So I'm wondering if we're going about this wrong.  Perhaps we need to
>> discover at the end of the run loop that we were asked to single step
>> execution and simply return to userspace, setting the debug exit
>> reason etc., instead of entering the guest with PSTATE.SS==0 and
>> relying on another trap back in to the guest just to set two fields on
>> the kvm_run structure and exit to user space ?
>>
>
> So if I understand correctly, the suggestion is that when we trap an
> instruction we check whether it was supposed to be single stepped, if it was
> we set up the vcpu registers as if it had taken a software step exception
> and return from kvm_arch_vcpu_ioctl_run. Is that right?

yes, that's the idea.  If there's a lot of complexity in setting up
CPU register state, then it may not be a good idea, but if it's
relatively clean, I think it can be preferred over the "let's keep a
flag aroudn for later" approach.

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

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

[Julien Thierry]

On 31/08/17 11:53, Christoffer Dall wrote:
> On Thu, Aug 31, 2017 at 11:37 AM, Julien Thierry <julien.thierry@arm.com> wrote:
>>
>>
>> On 31/08/17 09:54, Christoffer Dall wrote:
>>>
>>> On Thu, Aug 31, 2017 at 10:45 AM, Julien Thierry <julien.thierry@arm.com>
>>> wrote:
>>>>
>>>> Hi Christoffer,
>>>>
>>>>
>>>> On 30/08/17 19:53, Christoffer Dall wrote:
>>>>>
>>>>>
>>>>> Hi Julien,
>>>>>
>>>>> [cc'ing Alex Benn?e here who wrote the debug code for arm64]
>>>>>
>>>>> On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry
>>>>> <julien.thierry@arm.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> Software Step exception is missing after trapping instruction from the
>>>>>> guest.
>>>>>>
>>>>>> We need to set the PSR.SS to 0 for the guest vcpu before resuming guest
>>>>>> execution.
>>>>>>
>>>>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>>>>>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>>>>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>>>>
>>>>>> ---
>>>>>>     arch/arm64/include/asm/kvm_asm.h     |  2 ++
>>>>>>     arch/arm64/include/asm/kvm_emulate.h |  2 ++
>>>>>>     arch/arm64/kvm/debug.c               | 12 +++++++++++-
>>>>>>     arch/arm64/kvm/hyp/switch.c          | 12 ++++++++++++
>>>>>>     4 files changed, 27 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/arch/arm64/include/asm/kvm_asm.h
>>>>>> b/arch/arm64/include/asm/kvm_asm.h
>>>>>> index 26a64d0..398bbaa 100644
>>>>>> --- a/arch/arm64/include/asm/kvm_asm.h
>>>>>> +++ b/arch/arm64/include/asm/kvm_asm.h
>>>>>> @@ -32,6 +32,8 @@
>>>>>>
>>>>>>     #define KVM_ARM64_DEBUG_DIRTY_SHIFT    0
>>>>>>     #define KVM_ARM64_DEBUG_DIRTY          (1 <<
>>>>>> KVM_ARM64_DEBUG_DIRTY_SHIFT)
>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT        1
>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP      (1 <<
>>>>>> KVM_ARM64_DEBUG_INST_SKIP_SHIFT)
>>>>>>
>>>>>>     #define kvm_ksym_ref(sym)
>>>>>> \
>>>>>>            ({
>>>>>> \
>>>>>> diff --git a/arch/arm64/include/asm/kvm_emulate.h
>>>>>> b/arch/arm64/include/asm/kvm_emulate.h
>>>>>> index fe39e68..d401c64 100644
>>>>>> --- a/arch/arm64/include/asm/kvm_emulate.h
>>>>>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>>>>>> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu
>>>>>> *vcpu, bool is_wide_instr)
>>>>>>                    kvm_skip_instr32(vcpu, is_wide_instr);
>>>>>>            else
>>>>>>                    *vcpu_pc(vcpu) += 4;
>>>>>> +       /* Let debug engine know we skipped an instruction */
>>>>>> +       vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP;
>>>>>
>>>>>
>>>>>
>>>>> Why do we need to defer this action until later?
>>>>>
>>>>> Can't we simply do clear DBG_SPSR_SS here?
>>>>>
>>>>
>>>> That was my first intention, but it turns out that the current state of
>>>> things (without this patch) is that every time we enter a guest,
>>>> kvm_arm_setup_debug gets called and if single step is requested for the
>>>> guest it will set the flag in the SPSR (ignoring the fact that we cleared
>>>> it).
>>>
>>>
>>> Ah, right, duh.
>>>
>>>> This happens even if we exit the guest because of a data abort.
>>>>
>>>> For normal single step execution, we do need to reset SPSR.SS to 1 before
>>>> running the guest since completion of a step should clear that bit before
>>>> taking a software step exception. So what kvm_arm_setup_debug does seems
>>>> correct to me but missed the case for trapped/emulated instructions.
>>>>
>>>> So even if we just clear DBG_SPSR_SS here, we would still need to tell
>>>> kvm_arm_setup_debug not to change the bit. Or resetting SPSR.SS to 1 for
>>>> normal single stepping needs to be done before we skip instructions in
>>>> KVM
>>>> but that doesn't sound right to me...
>>>>
>>>
>>> So I'm wondering if we're going about this wrong.  Perhaps we need to
>>> discover at the end of the run loop that we were asked to single step
>>> execution and simply return to userspace, setting the debug exit
>>> reason etc., instead of entering the guest with PSTATE.SS==0 and
>>> relying on another trap back in to the guest just to set two fields on
>>> the kvm_run structure and exit to user space ?
>>>
>>
>> So if I understand correctly, the suggestion is that when we trap an
>> instruction we check whether it was supposed to be single stepped, if it was
>> we set up the vcpu registers as if it had taken a software step exception
>> and return from kvm_arch_vcpu_ioctl_run. Is that right?
> 
> yes, that's the idea.  If there's a lot of complexity in setting up
> CPU register state, then it may not be a good idea, but if it's
> relatively clean, I think it can be preferred over the "let's keep a
> flag aroudn for later" approach.
> 

So I looked a bit into it.

One annoying thing is that the single step mechanic is specific to 
arm64. MMU and MMIO code is shared between arm and arm64 and do some 
handling of traps.

So cleanest way I can think of doing this would be to clear SPSR.SS in 
arm64::kvm_skip_instr, then have some function (e.g. 
kvm_handle/manage_debug_state) at the end of the run loop. For arm, the 
function is empty. For arm64, the function,  if we are in an active 
pending state (SPSR.D == 0 && SPSR.SS == 0 && MDSCR_EL1.SS == 1) and not 
about to return to userland, returns with a "fake debug exception".

So instead of a flag, we would just use SPSR.SS (or more generally the 
vcpu state) to know if we need to exit with a debug exception or not. 
And the kvm_arm_setup_debug would be left untouched (always setting 
SPSR.SS when requested by userland).

Does that sound like what you had in mind? Or does it seem better than 
the current patch?

Thanks,

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

-- 
Julien Thierry

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

[Christoffer Dall]

On Thu, Aug 31, 2017 at 2:56 PM, Julien Thierry <julien.thierry@arm.com> wrote:
>
>
> On 31/08/17 11:53, Christoffer Dall wrote:
>>
>> On Thu, Aug 31, 2017 at 11:37 AM, Julien Thierry <julien.thierry@arm.com>
>> wrote:
>>>
>>>
>>>
>>> On 31/08/17 09:54, Christoffer Dall wrote:
>>>>
>>>>
>>>> On Thu, Aug 31, 2017 at 10:45 AM, Julien Thierry
>>>> <julien.thierry@arm.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> Hi Christoffer,
>>>>>
>>>>>
>>>>> On 30/08/17 19:53, Christoffer Dall wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hi Julien,
>>>>>>
>>>>>> [cc'ing Alex Benn?e here who wrote the debug code for arm64]
>>>>>>
>>>>>> On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry
>>>>>> <julien.thierry@arm.com>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Software Step exception is missing after trapping instruction from
>>>>>>> the
>>>>>>> guest.
>>>>>>>
>>>>>>> We need to set the PSR.SS to 0 for the guest vcpu before resuming
>>>>>>> guest
>>>>>>> execution.
>>>>>>>
>>>>>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>>>>>>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>>>>>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>>>>>
>>>>>>> ---
>>>>>>>     arch/arm64/include/asm/kvm_asm.h     |  2 ++
>>>>>>>     arch/arm64/include/asm/kvm_emulate.h |  2 ++
>>>>>>>     arch/arm64/kvm/debug.c               | 12 +++++++++++-
>>>>>>>     arch/arm64/kvm/hyp/switch.c          | 12 ++++++++++++
>>>>>>>     4 files changed, 27 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/arch/arm64/include/asm/kvm_asm.h
>>>>>>> b/arch/arm64/include/asm/kvm_asm.h
>>>>>>> index 26a64d0..398bbaa 100644
>>>>>>> --- a/arch/arm64/include/asm/kvm_asm.h
>>>>>>> +++ b/arch/arm64/include/asm/kvm_asm.h
>>>>>>> @@ -32,6 +32,8 @@
>>>>>>>
>>>>>>>     #define KVM_ARM64_DEBUG_DIRTY_SHIFT    0
>>>>>>>     #define KVM_ARM64_DEBUG_DIRTY          (1 <<
>>>>>>> KVM_ARM64_DEBUG_DIRTY_SHIFT)
>>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT        1
>>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP      (1 <<
>>>>>>> KVM_ARM64_DEBUG_INST_SKIP_SHIFT)
>>>>>>>
>>>>>>>     #define kvm_ksym_ref(sym)
>>>>>>> \
>>>>>>>            ({
>>>>>>> \
>>>>>>> diff --git a/arch/arm64/include/asm/kvm_emulate.h
>>>>>>> b/arch/arm64/include/asm/kvm_emulate.h
>>>>>>> index fe39e68..d401c64 100644
>>>>>>> --- a/arch/arm64/include/asm/kvm_emulate.h
>>>>>>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>>>>>>> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu
>>>>>>> *vcpu, bool is_wide_instr)
>>>>>>>                    kvm_skip_instr32(vcpu, is_wide_instr);
>>>>>>>            else
>>>>>>>                    *vcpu_pc(vcpu) += 4;
>>>>>>> +       /* Let debug engine know we skipped an instruction */
>>>>>>> +       vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP;
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Why do we need to defer this action until later?
>>>>>>
>>>>>> Can't we simply do clear DBG_SPSR_SS here?
>>>>>>
>>>>>
>>>>> That was my first intention, but it turns out that the current state of
>>>>> things (without this patch) is that every time we enter a guest,
>>>>> kvm_arm_setup_debug gets called and if single step is requested for the
>>>>> guest it will set the flag in the SPSR (ignoring the fact that we
>>>>> cleared
>>>>> it).
>>>>
>>>>
>>>>
>>>> Ah, right, duh.
>>>>
>>>>> This happens even if we exit the guest because of a data abort.
>>>>>
>>>>> For normal single step execution, we do need to reset SPSR.SS to 1
>>>>> before
>>>>> running the guest since completion of a step should clear that bit
>>>>> before
>>>>> taking a software step exception. So what kvm_arm_setup_debug does
>>>>> seems
>>>>> correct to me but missed the case for trapped/emulated instructions.
>>>>>
>>>>> So even if we just clear DBG_SPSR_SS here, we would still need to tell
>>>>> kvm_arm_setup_debug not to change the bit. Or resetting SPSR.SS to 1
>>>>> for
>>>>> normal single stepping needs to be done before we skip instructions in
>>>>> KVM
>>>>> but that doesn't sound right to me...
>>>>>
>>>>
>>>> So I'm wondering if we're going about this wrong.  Perhaps we need to
>>>> discover at the end of the run loop that we were asked to single step
>>>> execution and simply return to userspace, setting the debug exit
>>>> reason etc., instead of entering the guest with PSTATE.SS==0 and
>>>> relying on another trap back in to the guest just to set two fields on
>>>> the kvm_run structure and exit to user space ?
>>>>
>>>
>>> So if I understand correctly, the suggestion is that when we trap an
>>> instruction we check whether it was supposed to be single stepped, if it
>>> was
>>> we set up the vcpu registers as if it had taken a software step exception
>>> and return from kvm_arch_vcpu_ioctl_run. Is that right?
>>
>>
>> yes, that's the idea.  If there's a lot of complexity in setting up
>> CPU register state, then it may not be a good idea, but if it's
>> relatively clean, I think it can be preferred over the "let's keep a
>> flag aroudn for later" approach.
>>
>
> So I looked a bit into it.
>
> One annoying thing is that the single step mechanic is specific to arm64.
> MMU and MMIO code is shared between arm and arm64 and do some handling of
> traps.
>
> So cleanest way I can think of doing this would be to clear SPSR.SS in
> arm64::kvm_skip_instr, then have some function (e.g.
> kvm_handle/manage_debug_state) at the end of the run loop. For arm, the
> function is empty. For arm64, the function,  if we are in an active pending
> state (SPSR.D == 0 && SPSR.SS == 0 && MDSCR_EL1.SS == 1) and not about to
> return to userland, returns with a "fake debug exception".
>
> So instead of a flag, we would just use SPSR.SS (or more generally the vcpu
> state) to know if we need to exit with a debug exception or not. And the
> kvm_arm_setup_debug would be left untouched (always setting SPSR.SS when
> requested by userland).
>
> Does that sound like what you had in mind? Or does it seem better than the
> current patch?
>
I was thinking to change the skip_instruction function to return an
int, and then call kvm_handle_debug_ss() from skip_instruction, which
would update the kvm_run structure and exit here and then.

However, I'm now thinking that this doesn't really work either,
because we could have to emulate a trapped MMIO instruction in user
space, and then it's not clear how to exit with a debug exception at
the same time.

So perhaps we should stick with your original approach.

Thanks,
-Christoffer

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

[Julien Thierry]

On 31/08/17 14:28, Christoffer Dall wrote:
> On Thu, Aug 31, 2017 at 2:56 PM, Julien Thierry <julien.thierry@arm.com> wrote:
>>
>>
>> On 31/08/17 11:53, Christoffer Dall wrote:
>>>
>>> On Thu, Aug 31, 2017 at 11:37 AM, Julien Thierry <julien.thierry@arm.com>
>>> wrote:
>>>>
>>>>
>>>>
>>>> On 31/08/17 09:54, Christoffer Dall wrote:
>>>>>
>>>>>
>>>>> On Thu, Aug 31, 2017 at 10:45 AM, Julien Thierry
>>>>> <julien.thierry@arm.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> Hi Christoffer,
>>>>>>
>>>>>>
>>>>>> On 30/08/17 19:53, Christoffer Dall wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Hi Julien,
>>>>>>>
>>>>>>> [cc'ing Alex Benn?e here who wrote the debug code for arm64]
>>>>>>>
>>>>>>> On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry
>>>>>>> <julien.thierry@arm.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Software Step exception is missing after trapping instruction from
>>>>>>>> the
>>>>>>>> guest.
>>>>>>>>
>>>>>>>> We need to set the PSR.SS to 0 for the guest vcpu before resuming
>>>>>>>> guest
>>>>>>>> execution.
>>>>>>>>
>>>>>>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>>>>>>>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>>>>>>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>>>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>>>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>>>>>>
>>>>>>>> ---
>>>>>>>>      arch/arm64/include/asm/kvm_asm.h     |  2 ++
>>>>>>>>      arch/arm64/include/asm/kvm_emulate.h |  2 ++
>>>>>>>>      arch/arm64/kvm/debug.c               | 12 +++++++++++-
>>>>>>>>      arch/arm64/kvm/hyp/switch.c          | 12 ++++++++++++
>>>>>>>>      4 files changed, 27 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/arch/arm64/include/asm/kvm_asm.h
>>>>>>>> b/arch/arm64/include/asm/kvm_asm.h
>>>>>>>> index 26a64d0..398bbaa 100644
>>>>>>>> --- a/arch/arm64/include/asm/kvm_asm.h
>>>>>>>> +++ b/arch/arm64/include/asm/kvm_asm.h
>>>>>>>> @@ -32,6 +32,8 @@
>>>>>>>>
>>>>>>>>      #define KVM_ARM64_DEBUG_DIRTY_SHIFT    0
>>>>>>>>      #define KVM_ARM64_DEBUG_DIRTY          (1 <<
>>>>>>>> KVM_ARM64_DEBUG_DIRTY_SHIFT)
>>>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT        1
>>>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP      (1 <<
>>>>>>>> KVM_ARM64_DEBUG_INST_SKIP_SHIFT)
>>>>>>>>
>>>>>>>>      #define kvm_ksym_ref(sym)
>>>>>>>> \
>>>>>>>>             ({
>>>>>>>> \
>>>>>>>> diff --git a/arch/arm64/include/asm/kvm_emulate.h
>>>>>>>> b/arch/arm64/include/asm/kvm_emulate.h
>>>>>>>> index fe39e68..d401c64 100644
>>>>>>>> --- a/arch/arm64/include/asm/kvm_emulate.h
>>>>>>>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>>>>>>>> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu
>>>>>>>> *vcpu, bool is_wide_instr)
>>>>>>>>                     kvm_skip_instr32(vcpu, is_wide_instr);
>>>>>>>>             else
>>>>>>>>                     *vcpu_pc(vcpu) += 4;
>>>>>>>> +       /* Let debug engine know we skipped an instruction */
>>>>>>>> +       vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP;
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Why do we need to defer this action until later?
>>>>>>>
>>>>>>> Can't we simply do clear DBG_SPSR_SS here?
>>>>>>>
>>>>>>
>>>>>> That was my first intention, but it turns out that the current state of
>>>>>> things (without this patch) is that every time we enter a guest,
>>>>>> kvm_arm_setup_debug gets called and if single step is requested for the
>>>>>> guest it will set the flag in the SPSR (ignoring the fact that we
>>>>>> cleared
>>>>>> it).
>>>>>
>>>>>
>>>>>
>>>>> Ah, right, duh.
>>>>>
>>>>>> This happens even if we exit the guest because of a data abort.
>>>>>>
>>>>>> For normal single step execution, we do need to reset SPSR.SS to 1
>>>>>> before
>>>>>> running the guest since completion of a step should clear that bit
>>>>>> before
>>>>>> taking a software step exception. So what kvm_arm_setup_debug does
>>>>>> seems
>>>>>> correct to me but missed the case for trapped/emulated instructions.
>>>>>>
>>>>>> So even if we just clear DBG_SPSR_SS here, we would still need to tell
>>>>>> kvm_arm_setup_debug not to change the bit. Or resetting SPSR.SS to 1
>>>>>> for
>>>>>> normal single stepping needs to be done before we skip instructions in
>>>>>> KVM
>>>>>> but that doesn't sound right to me...
>>>>>>
>>>>>
>>>>> So I'm wondering if we're going about this wrong.  Perhaps we need to
>>>>> discover at the end of the run loop that we were asked to single step
>>>>> execution and simply return to userspace, setting the debug exit
>>>>> reason etc., instead of entering the guest with PSTATE.SS==0 and
>>>>> relying on another trap back in to the guest just to set two fields on
>>>>> the kvm_run structure and exit to user space ?
>>>>>
>>>>
>>>> So if I understand correctly, the suggestion is that when we trap an
>>>> instruction we check whether it was supposed to be single stepped, if it
>>>> was
>>>> we set up the vcpu registers as if it had taken a software step exception
>>>> and return from kvm_arch_vcpu_ioctl_run. Is that right?
>>>
>>>
>>> yes, that's the idea.  If there's a lot of complexity in setting up
>>> CPU register state, then it may not be a good idea, but if it's
>>> relatively clean, I think it can be preferred over the "let's keep a
>>> flag aroudn for later" approach.
>>>
>>
>> So I looked a bit into it.
>>
>> One annoying thing is that the single step mechanic is specific to arm64.
>> MMU and MMIO code is shared between arm and arm64 and do some handling of
>> traps.
>>
>> So cleanest way I can think of doing this would be to clear SPSR.SS in
>> arm64::kvm_skip_instr, then have some function (e.g.
>> kvm_handle/manage_debug_state) at the end of the run loop. For arm, the
>> function is empty. For arm64, the function,  if we are in an active pending
>> state (SPSR.D == 0 && SPSR.SS == 0 && MDSCR_EL1.SS == 1) and not about to
>> return to userland, returns with a "fake debug exception".
>>
>> So instead of a flag, we would just use SPSR.SS (or more generally the vcpu
>> state) to know if we need to exit with a debug exception or not. And the
>> kvm_arm_setup_debug would be left untouched (always setting SPSR.SS when
>> requested by userland).
>>
>> Does that sound like what you had in mind? Or does it seem better than the
>> current patch?
>>
> I was thinking to change the skip_instruction function to return an
> int, and then call kvm_handle_debug_ss() from skip_instruction, which
> would update the kvm_run structure and exit here and then.
> 

Setting up the debug exception from within kvm_skip_instruction seem to 
change a bit too much its semantic from arm to arm64. I would find this 
easily confusing.

> However, I'm now thinking that this doesn't really work either,
> because we could have to emulate a trapped MMIO instruction in user
> space, and then it's not clear how to exit with a debug exception at
> the same time.
> 
> So perhaps we should stick with your original approach.
> 

I had not realized that was possible. This makes things more complicated 
for avoiding a back and forth with the guest for trapped exceptions. Out 
of luck, having the debug flag does look like single stepping would work 
as expected for userland MMIOs.

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.

Thanks,

-- 
Julien Thierry

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

[Christoffer Dall]

On Thu, Aug 31, 2017 at 3:57 PM, Julien Thierry <julien.thierry@arm.com> wrote:
>
>
> On 31/08/17 14:28, Christoffer Dall wrote:
>>
>> On Thu, Aug 31, 2017 at 2:56 PM, Julien Thierry <julien.thierry@arm.com>
>> wrote:
>>>
>>>
>>>
>>> On 31/08/17 11:53, Christoffer Dall wrote:
>>>>
>>>>
>>>> On Thu, Aug 31, 2017 at 11:37 AM, Julien Thierry
>>>> <julien.thierry@arm.com>
>>>> wrote:
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On 31/08/17 09:54, Christoffer Dall wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Thu, Aug 31, 2017 at 10:45 AM, Julien Thierry
>>>>>> <julien.thierry@arm.com>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Hi Christoffer,
>>>>>>>
>>>>>>>
>>>>>>> On 30/08/17 19:53, Christoffer Dall wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi Julien,
>>>>>>>>
>>>>>>>> [cc'ing Alex Benn?e here who wrote the debug code for arm64]
>>>>>>>>
>>>>>>>> On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry
>>>>>>>> <julien.thierry@arm.com>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Software Step exception is missing after trapping instruction from
>>>>>>>>> the
>>>>>>>>> guest.
>>>>>>>>>
>>>>>>>>> We need to set the PSR.SS to 0 for the guest vcpu before resuming
>>>>>>>>> guest
>>>>>>>>> execution.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>>>>>>>>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>>>>>>>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>>>>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>>>>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>>      arch/arm64/include/asm/kvm_asm.h     |  2 ++
>>>>>>>>>      arch/arm64/include/asm/kvm_emulate.h |  2 ++
>>>>>>>>>      arch/arm64/kvm/debug.c               | 12 +++++++++++-
>>>>>>>>>      arch/arm64/kvm/hyp/switch.c          | 12 ++++++++++++
>>>>>>>>>      4 files changed, 27 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/arm64/include/asm/kvm_asm.h
>>>>>>>>> b/arch/arm64/include/asm/kvm_asm.h
>>>>>>>>> index 26a64d0..398bbaa 100644
>>>>>>>>> --- a/arch/arm64/include/asm/kvm_asm.h
>>>>>>>>> +++ b/arch/arm64/include/asm/kvm_asm.h
>>>>>>>>> @@ -32,6 +32,8 @@
>>>>>>>>>
>>>>>>>>>      #define KVM_ARM64_DEBUG_DIRTY_SHIFT    0
>>>>>>>>>      #define KVM_ARM64_DEBUG_DIRTY          (1 <<
>>>>>>>>> KVM_ARM64_DEBUG_DIRTY_SHIFT)
>>>>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT        1
>>>>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP      (1 <<
>>>>>>>>> KVM_ARM64_DEBUG_INST_SKIP_SHIFT)
>>>>>>>>>
>>>>>>>>>      #define kvm_ksym_ref(sym)
>>>>>>>>> \
>>>>>>>>>             ({
>>>>>>>>> \
>>>>>>>>> diff --git a/arch/arm64/include/asm/kvm_emulate.h
>>>>>>>>> b/arch/arm64/include/asm/kvm_emulate.h
>>>>>>>>> index fe39e68..d401c64 100644
>>>>>>>>> --- a/arch/arm64/include/asm/kvm_emulate.h
>>>>>>>>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>>>>>>>>> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu
>>>>>>>>> *vcpu, bool is_wide_instr)
>>>>>>>>>                     kvm_skip_instr32(vcpu, is_wide_instr);
>>>>>>>>>             else
>>>>>>>>>                     *vcpu_pc(vcpu) += 4;
>>>>>>>>> +       /* Let debug engine know we skipped an instruction */
>>>>>>>>> +       vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP;
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Why do we need to defer this action until later?
>>>>>>>>
>>>>>>>> Can't we simply do clear DBG_SPSR_SS here?
>>>>>>>>
>>>>>>>
>>>>>>> That was my first intention, but it turns out that the current state
>>>>>>> of
>>>>>>> things (without this patch) is that every time we enter a guest,
>>>>>>> kvm_arm_setup_debug gets called and if single step is requested for
>>>>>>> the
>>>>>>> guest it will set the flag in the SPSR (ignoring the fact that we
>>>>>>> cleared
>>>>>>> it).
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Ah, right, duh.
>>>>>>
>>>>>>> This happens even if we exit the guest because of a data abort.
>>>>>>>
>>>>>>> For normal single step execution, we do need to reset SPSR.SS to 1
>>>>>>> before
>>>>>>> running the guest since completion of a step should clear that bit
>>>>>>> before
>>>>>>> taking a software step exception. So what kvm_arm_setup_debug does
>>>>>>> seems
>>>>>>> correct to me but missed the case for trapped/emulated instructions.
>>>>>>>
>>>>>>> So even if we just clear DBG_SPSR_SS here, we would still need to
>>>>>>> tell
>>>>>>> kvm_arm_setup_debug not to change the bit. Or resetting SPSR.SS to 1
>>>>>>> for
>>>>>>> normal single stepping needs to be done before we skip instructions
>>>>>>> in
>>>>>>> KVM
>>>>>>> but that doesn't sound right to me...
>>>>>>>
>>>>>>
>>>>>> So I'm wondering if we're going about this wrong.  Perhaps we need to
>>>>>> discover at the end of the run loop that we were asked to single step
>>>>>> execution and simply return to userspace, setting the debug exit
>>>>>> reason etc., instead of entering the guest with PSTATE.SS==0 and
>>>>>> relying on another trap back in to the guest just to set two fields on
>>>>>> the kvm_run structure and exit to user space ?
>>>>>>
>>>>>
>>>>> So if I understand correctly, the suggestion is that when we trap an
>>>>> instruction we check whether it was supposed to be single stepped, if
>>>>> it
>>>>> was
>>>>> we set up the vcpu registers as if it had taken a software step
>>>>> exception
>>>>> and return from kvm_arch_vcpu_ioctl_run. Is that right?
>>>>
>>>>
>>>>
>>>> yes, that's the idea.  If there's a lot of complexity in setting up
>>>> CPU register state, then it may not be a good idea, but if it's
>>>> relatively clean, I think it can be preferred over the "let's keep a
>>>> flag aroudn for later" approach.
>>>>
>>>
>>> So I looked a bit into it.
>>>
>>> One annoying thing is that the single step mechanic is specific to arm64.
>>> MMU and MMIO code is shared between arm and arm64 and do some handling of
>>> traps.
>>>
>>> So cleanest way I can think of doing this would be to clear SPSR.SS in
>>> arm64::kvm_skip_instr, then have some function (e.g.
>>> kvm_handle/manage_debug_state) at the end of the run loop. For arm, the
>>> function is empty. For arm64, the function,  if we are in an active
>>> pending
>>> state (SPSR.D == 0 && SPSR.SS == 0 && MDSCR_EL1.SS == 1) and not about to
>>> return to userland, returns with a "fake debug exception".
>>>
>>> So instead of a flag, we would just use SPSR.SS (or more generally the
>>> vcpu
>>> state) to know if we need to exit with a debug exception or not. And the
>>> kvm_arm_setup_debug would be left untouched (always setting SPSR.SS when
>>> requested by userland).
>>>
>>> Does that sound like what you had in mind? Or does it seem better than
>>> the
>>> current patch?
>>>
>> I was thinking to change the skip_instruction function to return an
>> int, and then call kvm_handle_debug_ss() from skip_instruction, which
>> would update the kvm_run structure and exit here and then.
>>
>
> Setting up the debug exception from within kvm_skip_instruction seem to
> change a bit too much its semantic from arm to arm64. I would find this
> easily confusing.
>
>> However, I'm now thinking that this doesn't really work either,
>> because we could have to emulate a trapped MMIO instruction in user
>> space, and then it's not clear how to exit with a debug exception at
>> the same time.
>>
>> So perhaps we should stick with your original approach.
>>
>
> I had not realized that was possible. This makes things more complicated for
> avoiding a back and forth with the guest for trapped exceptions. Out of
> luck, having the debug flag does look like single stepping would work as
> expected for userland MMIOs.

Yes, I think your approach is the best choice now, considering.

>
> I can try to detail the comment in kvm_arm_setup_debug when we set SPSR,
> hopefully making things clearer when seeing that part of the code.
>

I also think we need to improve the comment in the world-switch return
path, and I'd like Alex to weigh in here before we merge this.   He's
back from holiday on Monday.

Thanks,
-Christoffer

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

[Julien Thierry]

On 31/08/17 15:01, Christoffer Dall wrote:
> On Thu, Aug 31, 2017 at 3:57 PM, Julien Thierry <julien.thierry@arm.com> wrote:
>>
>>
>> On 31/08/17 14:28, Christoffer Dall wrote:
>>>
>>> On Thu, Aug 31, 2017 at 2:56 PM, Julien Thierry <julien.thierry@arm.com>
>>> wrote:
>>>>
>>>>
>>>>
>>>> On 31/08/17 11:53, Christoffer Dall wrote:
>>>>>
>>>>>
>>>>> On Thu, Aug 31, 2017 at 11:37 AM, Julien Thierry
>>>>> <julien.thierry@arm.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 31/08/17 09:54, Christoffer Dall wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Thu, Aug 31, 2017 at 10:45 AM, Julien Thierry
>>>>>>> <julien.thierry@arm.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi Christoffer,
>>>>>>>>
>>>>>>>>
>>>>>>>> On 30/08/17 19:53, Christoffer Dall wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hi Julien,
>>>>>>>>>
>>>>>>>>> [cc'ing Alex Benn?e here who wrote the debug code for arm64]
>>>>>>>>>
>>>>>>>>> On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry
>>>>>>>>> <julien.thierry@arm.com>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Software Step exception is missing after trapping instruction from
>>>>>>>>>> the
>>>>>>>>>> guest.
>>>>>>>>>>
>>>>>>>>>> We need to set the PSR.SS to 0 for the guest vcpu before resuming
>>>>>>>>>> guest
>>>>>>>>>> execution.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>>>>>>>>>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>>>>>>>>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>>>>>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>>>>>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>>>>>>>>
>>>>>>>>>> ---
>>>>>>>>>>       arch/arm64/include/asm/kvm_asm.h     |  2 ++
>>>>>>>>>>       arch/arm64/include/asm/kvm_emulate.h |  2 ++
>>>>>>>>>>       arch/arm64/kvm/debug.c               | 12 +++++++++++-
>>>>>>>>>>       arch/arm64/kvm/hyp/switch.c          | 12 ++++++++++++
>>>>>>>>>>       4 files changed, 27 insertions(+), 1 deletion(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/arch/arm64/include/asm/kvm_asm.h
>>>>>>>>>> b/arch/arm64/include/asm/kvm_asm.h
>>>>>>>>>> index 26a64d0..398bbaa 100644
>>>>>>>>>> --- a/arch/arm64/include/asm/kvm_asm.h
>>>>>>>>>> +++ b/arch/arm64/include/asm/kvm_asm.h
>>>>>>>>>> @@ -32,6 +32,8 @@
>>>>>>>>>>
>>>>>>>>>>       #define KVM_ARM64_DEBUG_DIRTY_SHIFT    0
>>>>>>>>>>       #define KVM_ARM64_DEBUG_DIRTY          (1 <<
>>>>>>>>>> KVM_ARM64_DEBUG_DIRTY_SHIFT)
>>>>>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT        1
>>>>>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP      (1 <<
>>>>>>>>>> KVM_ARM64_DEBUG_INST_SKIP_SHIFT)
>>>>>>>>>>
>>>>>>>>>>       #define kvm_ksym_ref(sym)
>>>>>>>>>> \
>>>>>>>>>>              ({
>>>>>>>>>> \
>>>>>>>>>> diff --git a/arch/arm64/include/asm/kvm_emulate.h
>>>>>>>>>> b/arch/arm64/include/asm/kvm_emulate.h
>>>>>>>>>> index fe39e68..d401c64 100644
>>>>>>>>>> --- a/arch/arm64/include/asm/kvm_emulate.h
>>>>>>>>>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>>>>>>>>>> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu
>>>>>>>>>> *vcpu, bool is_wide_instr)
>>>>>>>>>>                      kvm_skip_instr32(vcpu, is_wide_instr);
>>>>>>>>>>              else
>>>>>>>>>>                      *vcpu_pc(vcpu) += 4;
>>>>>>>>>> +       /* Let debug engine know we skipped an instruction */
>>>>>>>>>> +       vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP;
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Why do we need to defer this action until later?
>>>>>>>>>
>>>>>>>>> Can't we simply do clear DBG_SPSR_SS here?
>>>>>>>>>
>>>>>>>>
>>>>>>>> That was my first intention, but it turns out that the current state
>>>>>>>> of
>>>>>>>> things (without this patch) is that every time we enter a guest,
>>>>>>>> kvm_arm_setup_debug gets called and if single step is requested for
>>>>>>>> the
>>>>>>>> guest it will set the flag in the SPSR (ignoring the fact that we
>>>>>>>> cleared
>>>>>>>> it).
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Ah, right, duh.
>>>>>>>
>>>>>>>> This happens even if we exit the guest because of a data abort.
>>>>>>>>
>>>>>>>> For normal single step execution, we do need to reset SPSR.SS to 1
>>>>>>>> before
>>>>>>>> running the guest since completion of a step should clear that bit
>>>>>>>> before
>>>>>>>> taking a software step exception. So what kvm_arm_setup_debug does
>>>>>>>> seems
>>>>>>>> correct to me but missed the case for trapped/emulated instructions.
>>>>>>>>
>>>>>>>> So even if we just clear DBG_SPSR_SS here, we would still need to
>>>>>>>> tell
>>>>>>>> kvm_arm_setup_debug not to change the bit. Or resetting SPSR.SS to 1
>>>>>>>> for
>>>>>>>> normal single stepping needs to be done before we skip instructions
>>>>>>>> in
>>>>>>>> KVM
>>>>>>>> but that doesn't sound right to me...
>>>>>>>>
>>>>>>>
>>>>>>> So I'm wondering if we're going about this wrong.  Perhaps we need to
>>>>>>> discover at the end of the run loop that we were asked to single step
>>>>>>> execution and simply return to userspace, setting the debug exit
>>>>>>> reason etc., instead of entering the guest with PSTATE.SS==0 and
>>>>>>> relying on another trap back in to the guest just to set two fields on
>>>>>>> the kvm_run structure and exit to user space ?
>>>>>>>
>>>>>>
>>>>>> So if I understand correctly, the suggestion is that when we trap an
>>>>>> instruction we check whether it was supposed to be single stepped, if
>>>>>> it
>>>>>> was
>>>>>> we set up the vcpu registers as if it had taken a software step
>>>>>> exception
>>>>>> and return from kvm_arch_vcpu_ioctl_run. Is that right?
>>>>>
>>>>>
>>>>>
>>>>> yes, that's the idea.  If there's a lot of complexity in setting up
>>>>> CPU register state, then it may not be a good idea, but if it's
>>>>> relatively clean, I think it can be preferred over the "let's keep a
>>>>> flag aroudn for later" approach.
>>>>>
>>>>
>>>> So I looked a bit into it.
>>>>
>>>> One annoying thing is that the single step mechanic is specific to arm64.
>>>> MMU and MMIO code is shared between arm and arm64 and do some handling of
>>>> traps.
>>>>
>>>> So cleanest way I can think of doing this would be to clear SPSR.SS in
>>>> arm64::kvm_skip_instr, then have some function (e.g.
>>>> kvm_handle/manage_debug_state) at the end of the run loop. For arm, the
>>>> function is empty. For arm64, the function,  if we are in an active
>>>> pending
>>>> state (SPSR.D == 0 && SPSR.SS == 0 && MDSCR_EL1.SS == 1) and not about to
>>>> return to userland, returns with a "fake debug exception".
>>>>
>>>> So instead of a flag, we would just use SPSR.SS (or more generally the
>>>> vcpu
>>>> state) to know if we need to exit with a debug exception or not. And the
>>>> kvm_arm_setup_debug would be left untouched (always setting SPSR.SS when
>>>> requested by userland).
>>>>
>>>> Does that sound like what you had in mind? Or does it seem better than
>>>> the
>>>> current patch?
>>>>
>>> I was thinking to change the skip_instruction function to return an
>>> int, and then call kvm_handle_debug_ss() from skip_instruction, which
>>> would update the kvm_run structure and exit here and then.
>>>
>>
>> Setting up the debug exception from within kvm_skip_instruction seem to
>> change a bit too much its semantic from arm to arm64. I would find this
>> easily confusing.
>>
>>> However, I'm now thinking that this doesn't really work either,
>>> because we could have to emulate a trapped MMIO instruction in user
>>> space, and then it's not clear how to exit with a debug exception at
>>> the same time.
>>>
>>> So perhaps we should stick with your original approach.
>>>
>>
>> I had not realized that was possible. This makes things more complicated for
>> avoiding a back and forth with the guest for trapped exceptions. Out of
>> luck, having the debug flag does look like single stepping would work as
>> expected for userland MMIOs.
> 
> Yes, I think your approach is the best choice now, considering.
> 
>>
>> I can try to detail the comment in kvm_arm_setup_debug when we set SPSR,
>> hopefully making things clearer when seeing that part of the code.
>>
> 
> I also think we need to improve the comment in the world-switch return
> path, and I'd like Alex to weigh in here before we merge this.   He's
> back from holiday on Monday.
> 

Ping Alex?

-- 
Julien Thierry

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

[Alex Bennée]

Julien Thierry <julien.thierry@arm.com> writes:

> On 31/08/17 15:01, Christoffer Dall wrote:
>> On Thu, Aug 31, 2017 at 3:57 PM, Julien Thierry <julien.thierry@arm.com> wrote:
>>> On 31/08/17 14:28, Christoffer Dall wrote:
>>>> On Thu, Aug 31, 2017 at 2:56 PM, Julien Thierry <julien.thierry@arm.com>
>>>> wrote:
>>>>> On 31/08/17 11:53, Christoffer Dall wrote:
>>>>>> On Thu, Aug 31, 2017 at 11:37 AM, Julien Thierry
>>>>>> <julien.thierry@arm.com>
>>>>>> wrote:
>>>>>>> On 31/08/17 09:54, Christoffer Dall wrote:
>>>>>>>> On Thu, Aug 31, 2017 at 10:45 AM, Julien Thierry
>>>>>>>> <julien.thierry@arm.com>
>>>>>>>> wrote:
>>>>>>>>> Hi Christoffer,
>>>>>>>>> On 30/08/17 19:53, Christoffer Dall wrote:
>>>>>>>>>> Hi Julien,
>>>>>>>>>>
>>>>>>>>>> [cc'ing Alex Benn?e here who wrote the debug code for arm64]
>>>>>>>>>>
>>>>>>>>>> On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry
>>>>>>>>>> <julien.thierry@arm.com>
>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>> Software Step exception is missing after trapping instruction from
>>>>>>>>>>> the
>>>>>>>>>>> guest.
>>>>>>>>>>>
>>>>>>>>>>> We need to set the PSR.SS to 0 for the guest vcpu before resuming
>>>>>>>>>>> guest
>>>>>>>>>>> execution.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>>>>>>>>>>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>>>>>>>>>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>>>>>>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>>>>>>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>>>>>>>>>
>>>>>>>>>>> ---
>>>>>>>>>>>       arch/arm64/include/asm/kvm_asm.h     |  2 ++
>>>>>>>>>>>       arch/arm64/include/asm/kvm_emulate.h |  2 ++
>>>>>>>>>>>       arch/arm64/kvm/debug.c               | 12 +++++++++++-
>>>>>>>>>>>       arch/arm64/kvm/hyp/switch.c          | 12 ++++++++++++
>>>>>>>>>>>       4 files changed, 27 insertions(+), 1 deletion(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/arch/arm64/include/asm/kvm_asm.h
>>>>>>>>>>> b/arch/arm64/include/asm/kvm_asm.h
>>>>>>>>>>> index 26a64d0..398bbaa 100644
>>>>>>>>>>> --- a/arch/arm64/include/asm/kvm_asm.h
>>>>>>>>>>> +++ b/arch/arm64/include/asm/kvm_asm.h
>>>>>>>>>>> @@ -32,6 +32,8 @@
>>>>>>>>>>>
>>>>>>>>>>>       #define KVM_ARM64_DEBUG_DIRTY_SHIFT    0
>>>>>>>>>>>       #define KVM_ARM64_DEBUG_DIRTY          (1 <<
>>>>>>>>>>> KVM_ARM64_DEBUG_DIRTY_SHIFT)
>>>>>>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT        1
>>>>>>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP      (1 <<
>>>>>>>>>>> KVM_ARM64_DEBUG_INST_SKIP_SHIFT)
>>>>>>>>>>>
>>>>>>>>>>>       #define kvm_ksym_ref(sym)
>>>>>>>>>>> \
>>>>>>>>>>>              ({
>>>>>>>>>>> \
>>>>>>>>>>> diff --git a/arch/arm64/include/asm/kvm_emulate.h
>>>>>>>>>>> b/arch/arm64/include/asm/kvm_emulate.h
>>>>>>>>>>> index fe39e68..d401c64 100644
>>>>>>>>>>> --- a/arch/arm64/include/asm/kvm_emulate.h
>>>>>>>>>>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>>>>>>>>>>> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu
>>>>>>>>>>> *vcpu, bool is_wide_instr)
>>>>>>>>>>>                      kvm_skip_instr32(vcpu, is_wide_instr);
>>>>>>>>>>>              else
>>>>>>>>>>>                      *vcpu_pc(vcpu) += 4;
>>>>>>>>>>> +       /* Let debug engine know we skipped an instruction */
>>>>>>>>>>> +       vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP;
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Why do we need to defer this action until later?
>>>>>>>>>>
>>>>>>>>>> Can't we simply do clear DBG_SPSR_SS here?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> That was my first intention, but it turns out that the current state
>>>>>>>>> of
>>>>>>>>> things (without this patch) is that every time we enter a guest,
>>>>>>>>> kvm_arm_setup_debug gets called and if single step is requested for
>>>>>>>>> the
>>>>>>>>> guest it will set the flag in the SPSR (ignoring the fact that we
>>>>>>>>> cleared
>>>>>>>>> it).

So to be clear kvm_arm_setup_debug only sets SS if we are about to
single-step. This is controlled by the debug ioctl which is done outside
of the main KVM run loop.

>>>>>>>>
>>>>>>>> Ah, right, duh.
>>>>>>>>
>>>>>>>>> This happens even if we exit the guest because of a data abort.
>>>>>>>>>
>>>>>>>>> For normal single step execution, we do need to reset SPSR.SS to 1
>>>>>>>>> before
>>>>>>>>> running the guest since completion of a step should clear that bit
>>>>>>>>> before
>>>>>>>>> taking a software step exception. So what kvm_arm_setup_debug does
>>>>>>>>> seems
>>>>>>>>> correct to me but missed the case for trapped/emulated instructions.
>>>>>>>>>
>>>>>>>>> So even if we just clear DBG_SPSR_SS here, we would still need to
>>>>>>>>> tell
>>>>>>>>> kvm_arm_setup_debug not to change the bit. Or resetting SPSR.SS to 1
>>>>>>>>> for
>>>>>>>>> normal single stepping needs to be done before we skip instructions
>>>>>>>>> in
>>>>>>>>> KVM
>>>>>>>>> but that doesn't sound right to me...
>>>>>>>>>
>>>>>>>>
>>>>>>>> So I'm wondering if we're going about this wrong.  Perhaps we need to
>>>>>>>> discover at the end of the run loop that we were asked to single step
>>>>>>>> execution and simply return to userspace, setting the debug exit
>>>>>>>> reason etc., instead of entering the guest with PSTATE.SS==0 and
>>>>>>>> relying on another trap back in to the guest just to set two fields on
>>>>>>>> the kvm_run structure and exit to user space ?
>>>>>>>>
>>>>>>>
>>>>>>> So if I understand correctly, the suggestion is that when we trap an
>>>>>>> instruction we check whether it was supposed to be single stepped, if
>>>>>>> it
>>>>>>> was
>>>>>>> we set up the vcpu registers as if it had taken a software step
>>>>>>> exception
>>>>>>> and return from kvm_arch_vcpu_ioctl_run. Is that right?
>>>>>>
>>>>>>
>>>>>>
>>>>>> yes, that's the idea.  If there's a lot of complexity in setting up
>>>>>> CPU register state, then it may not be a good idea, but if it's
>>>>>> relatively clean, I think it can be preferred over the "let's keep a
>>>>>> flag aroudn for later" approach.
>>>>>>
>>>>>
>>>>> So I looked a bit into it.
>>>>>
>>>>> One annoying thing is that the single step mechanic is specific to arm64.
>>>>> MMU and MMIO code is shared between arm and arm64 and do some handling of
>>>>> traps.

This is because the ARMv7 support isn't complete (and also ARMv7
hardware is slightly more complex in its corner cases if we were to do it).

>>>>>
>>>>> So cleanest way I can think of doing this would be to clear SPSR.SS in
>>>>> arm64::kvm_skip_instr, then have some function (e.g.
>>>>> kvm_handle/manage_debug_state) at the end of the run loop. For arm, the
>>>>> function is empty. For arm64, the function,  if we are in an active
>>>>> pending
>>>>> state (SPSR.D == 0 && SPSR.SS == 0 && MDSCR_EL1.SS == 1) and not about to
>>>>> return to userland, returns with a "fake debug exception".
>>>>>
>>>>> So instead of a flag, we would just use SPSR.SS (or more generally the
>>>>> vcpu
>>>>> state) to know if we need to exit with a debug exception or not. And the
>>>>> kvm_arm_setup_debug would be left untouched (always setting SPSR.SS when
>>>>> requested by userland).
>>>>>
>>>>> Does that sound like what you had in mind? Or does it seem better than
>>>>> the
>>>>> current patch?
>>>>>
>>>> I was thinking to change the skip_instruction function to return an
>>>> int, and then call kvm_handle_debug_ss() from skip_instruction, which
>>>> would update the kvm_run structure and exit here and then.
>>>>
>>>
>>> Setting up the debug exception from within kvm_skip_instruction seem to
>>> change a bit too much its semantic from arm to arm64. I would find this
>>> easily confusing.
>>>
>>>> However, I'm now thinking that this doesn't really work either,
>>>> because we could have to emulate a trapped MMIO instruction in user
>>>> space, and then it's not clear how to exit with a debug exception at
>>>> the same time.

A debug exception at guest exit point is (IIRC) just having the
appropriate status in the run->exit_reason (KVM_EXIT_DEBUG). If you need
to exit for MMIO emulation (i.e. the instruction has not run yet) you
shouldn't do that. Exit, emulate and return. We could handle the ioctl
to clear SS in userspace but I guess that gets just as messy.

>>>>
>>>> So perhaps we should stick with your original approach.
>>>>
>>>
>>> I had not realized that was possible. This makes things more complicated for
>>> avoiding a back and forth with the guest for trapped exceptions. Out of
>>> luck, having the debug flag does look like single stepping would work as
>>> expected for userland MMIOs.
>>
>> Yes, I think your approach is the best choice now, considering.
>>
>>>
>>> I can try to detail the comment in kvm_arm_setup_debug when we set SPSR,
>>> hopefully making things clearer when seeing that part of the code.
>>>
>>
>> I also think we need to improve the comment in the world-switch return
>> path, and I'd like Alex to weigh in here before we merge this.   He's
>> back from holiday on Monday.
>>
>
> Ping Alex?

Sorry for the delay getting back to you. I had flagged the email but
with holidays and conferences in the way it fell off my queue.

So to summarise as I understand things:

 Host User Space   |      Host KVM   |   Host Hyp    |  Guest VM      |

 Enable Debug(SS)
 KVM_RUN ----------->
                     Guest SPSR.SS set
                                   --> World Switch ->
                                                      Insn Trap to Hyp
                                       World Switch <-
                                       (SS not cleared)
                                   <--
                     Insn Emulated
                     pc += 4
                                   -->
                                       World Switch
                                       (SS still set)
                                                     ->
                                                      Insn +4 SS
                                                    <-
                                       World Switch
                                       (SS cleared)

                                    <--
                     Guest exit (debug)
                  <--
  See SS did 2 insns?

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

--
Alex Benn?e

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

[Julien Thierry]

On 03/10/17 15:57, Alex Benn?e wrote:
> 
> Julien Thierry <julien.thierry@arm.com> writes:
> 
>> On 31/08/17 15:01, Christoffer Dall wrote:
>>> On Thu, Aug 31, 2017 at 3:57 PM, Julien Thierry <julien.thierry@arm.com> wrote:
>>>> On 31/08/17 14:28, Christoffer Dall wrote:
>>>>> On Thu, Aug 31, 2017 at 2:56 PM, Julien Thierry <julien.thierry@arm.com>
>>>>> wrote:
>>>>>> On 31/08/17 11:53, Christoffer Dall wrote:
>>>>>>> On Thu, Aug 31, 2017 at 11:37 AM, Julien Thierry
>>>>>>> <julien.thierry@arm.com>
>>>>>>> wrote:
>>>>>>>> On 31/08/17 09:54, Christoffer Dall wrote:
>>>>>>>>> On Thu, Aug 31, 2017 at 10:45 AM, Julien Thierry
>>>>>>>>> <julien.thierry@arm.com>
>>>>>>>>> wrote:
>>>>>>>>>> Hi Christoffer,
>>>>>>>>>> On 30/08/17 19:53, Christoffer Dall wrote:
>>>>>>>>>>> Hi Julien,
>>>>>>>>>>>
>>>>>>>>>>> [cc'ing Alex Benn?e here who wrote the debug code for arm64]
>>>>>>>>>>>
>>>>>>>>>>> On Wed, Aug 30, 2017 at 11:01 AM, Julien Thierry
>>>>>>>>>>> <julien.thierry@arm.com>
>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> Software Step exception is missing after trapping instruction from
>>>>>>>>>>>> the
>>>>>>>>>>>> guest.
>>>>>>>>>>>>
>>>>>>>>>>>> We need to set the PSR.SS to 0 for the guest vcpu before resuming
>>>>>>>>>>>> guest
>>>>>>>>>>>> execution.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>>>>>>>>>>>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>>>>>>>>>>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>>>>>>>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>>>>>>>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>>>>>>>>>>
>>>>>>>>>>>> ---
>>>>>>>>>>>>        arch/arm64/include/asm/kvm_asm.h     |  2 ++
>>>>>>>>>>>>        arch/arm64/include/asm/kvm_emulate.h |  2 ++
>>>>>>>>>>>>        arch/arm64/kvm/debug.c               | 12 +++++++++++-
>>>>>>>>>>>>        arch/arm64/kvm/hyp/switch.c          | 12 ++++++++++++
>>>>>>>>>>>>        4 files changed, 27 insertions(+), 1 deletion(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/arch/arm64/include/asm/kvm_asm.h
>>>>>>>>>>>> b/arch/arm64/include/asm/kvm_asm.h
>>>>>>>>>>>> index 26a64d0..398bbaa 100644
>>>>>>>>>>>> --- a/arch/arm64/include/asm/kvm_asm.h
>>>>>>>>>>>> +++ b/arch/arm64/include/asm/kvm_asm.h
>>>>>>>>>>>> @@ -32,6 +32,8 @@
>>>>>>>>>>>>
>>>>>>>>>>>>        #define KVM_ARM64_DEBUG_DIRTY_SHIFT    0
>>>>>>>>>>>>        #define KVM_ARM64_DEBUG_DIRTY          (1 <<
>>>>>>>>>>>> KVM_ARM64_DEBUG_DIRTY_SHIFT)
>>>>>>>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT        1
>>>>>>>>>>>> +#define KVM_ARM64_DEBUG_INST_SKIP      (1 <<
>>>>>>>>>>>> KVM_ARM64_DEBUG_INST_SKIP_SHIFT)
>>>>>>>>>>>>
>>>>>>>>>>>>        #define kvm_ksym_ref(sym)
>>>>>>>>>>>> \
>>>>>>>>>>>>               ({
>>>>>>>>>>>> \
>>>>>>>>>>>> diff --git a/arch/arm64/include/asm/kvm_emulate.h
>>>>>>>>>>>> b/arch/arm64/include/asm/kvm_emulate.h
>>>>>>>>>>>> index fe39e68..d401c64 100644
>>>>>>>>>>>> --- a/arch/arm64/include/asm/kvm_emulate.h
>>>>>>>>>>>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>>>>>>>>>>>> @@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu
>>>>>>>>>>>> *vcpu, bool is_wide_instr)
>>>>>>>>>>>>                       kvm_skip_instr32(vcpu, is_wide_instr);
>>>>>>>>>>>>               else
>>>>>>>>>>>>                       *vcpu_pc(vcpu) += 4;
>>>>>>>>>>>> +       /* Let debug engine know we skipped an instruction */
>>>>>>>>>>>> +       vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_INST_SKIP;
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Why do we need to defer this action until later?
>>>>>>>>>>>
>>>>>>>>>>> Can't we simply do clear DBG_SPSR_SS here?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> That was my first intention, but it turns out that the current state
>>>>>>>>>> of
>>>>>>>>>> things (without this patch) is that every time we enter a guest,
>>>>>>>>>> kvm_arm_setup_debug gets called and if single step is requested for
>>>>>>>>>> the
>>>>>>>>>> guest it will set the flag in the SPSR (ignoring the fact that we
>>>>>>>>>> cleared
>>>>>>>>>> it).
> 
> So to be clear kvm_arm_setup_debug only sets SS if we are about to
> single-step. This is controlled by the debug ioctl which is done outside
> of the main KVM run loop.
> 
>>>>>>>>>
>>>>>>>>> Ah, right, duh.
>>>>>>>>>
>>>>>>>>>> This happens even if we exit the guest because of a data abort.
>>>>>>>>>>
>>>>>>>>>> For normal single step execution, we do need to reset SPSR.SS to 1
>>>>>>>>>> before
>>>>>>>>>> running the guest since completion of a step should clear that bit
>>>>>>>>>> before
>>>>>>>>>> taking a software step exception. So what kvm_arm_setup_debug does
>>>>>>>>>> seems
>>>>>>>>>> correct to me but missed the case for trapped/emulated instructions.
>>>>>>>>>>
>>>>>>>>>> So even if we just clear DBG_SPSR_SS here, we would still need to
>>>>>>>>>> tell
>>>>>>>>>> kvm_arm_setup_debug not to change the bit. Or resetting SPSR.SS to 1
>>>>>>>>>> for
>>>>>>>>>> normal single stepping needs to be done before we skip instructions
>>>>>>>>>> in
>>>>>>>>>> KVM
>>>>>>>>>> but that doesn't sound right to me...
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> So I'm wondering if we're going about this wrong.  Perhaps we need to
>>>>>>>>> discover at the end of the run loop that we were asked to single step
>>>>>>>>> execution and simply return to userspace, setting the debug exit
>>>>>>>>> reason etc., instead of entering the guest with PSTATE.SS==0 and
>>>>>>>>> relying on another trap back in to the guest just to set two fields on
>>>>>>>>> the kvm_run structure and exit to user space ?
>>>>>>>>>
>>>>>>>>
>>>>>>>> So if I understand correctly, the suggestion is that when we trap an
>>>>>>>> instruction we check whether it was supposed to be single stepped, if
>>>>>>>> it
>>>>>>>> was
>>>>>>>> we set up the vcpu registers as if it had taken a software step
>>>>>>>> exception
>>>>>>>> and return from kvm_arch_vcpu_ioctl_run. Is that right?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> yes, that's the idea.  If there's a lot of complexity in setting up
>>>>>>> CPU register state, then it may not be a good idea, but if it's
>>>>>>> relatively clean, I think it can be preferred over the "let's keep a
>>>>>>> flag aroudn for later" approach.
>>>>>>>
>>>>>>
>>>>>> So I looked a bit into it.
>>>>>>
>>>>>> One annoying thing is that the single step mechanic is specific to arm64.
>>>>>> MMU and MMIO code is shared between arm and arm64 and do some handling of
>>>>>> traps.
> 
> This is because the ARMv7 support isn't complete (and also ARMv7
> hardware is slightly more complex in its corner cases if we were to do it).
> 
>>>>>>
>>>>>> So cleanest way I can think of doing this would be to clear SPSR.SS in
>>>>>> arm64::kvm_skip_instr, then have some function (e.g.
>>>>>> kvm_handle/manage_debug_state) at the end of the run loop. For arm, the
>>>>>> function is empty. For arm64, the function,  if we are in an active
>>>>>> pending
>>>>>> state (SPSR.D == 0 && SPSR.SS == 0 && MDSCR_EL1.SS == 1) and not about to
>>>>>> return to userland, returns with a "fake debug exception".
>>>>>>
>>>>>> So instead of a flag, we would just use SPSR.SS (or more generally the
>>>>>> vcpu
>>>>>> state) to know if we need to exit with a debug exception or not. And the
>>>>>> kvm_arm_setup_debug would be left untouched (always setting SPSR.SS when
>>>>>> requested by userland).
>>>>>>
>>>>>> Does that sound like what you had in mind? Or does it seem better than
>>>>>> the
>>>>>> current patch?
>>>>>>
>>>>> I was thinking to change the skip_instruction function to return an
>>>>> int, and then call kvm_handle_debug_ss() from skip_instruction, which
>>>>> would update the kvm_run structure and exit here and then.
>>>>>
>>>>
>>>> Setting up the debug exception from within kvm_skip_instruction seem to
>>>> change a bit too much its semantic from arm to arm64. I would find this
>>>> easily confusing.
>>>>
>>>>> However, I'm now thinking that this doesn't really work either,
>>>>> because we could have to emulate a trapped MMIO instruction in user
>>>>> space, and then it's not clear how to exit with a debug exception at
>>>>> the same time.
> 
> A debug exception at guest exit point is (IIRC) just having the
> appropriate status in the run->exit_reason (KVM_EXIT_DEBUG). If you need
> to exit for MMIO emulation (i.e. the instruction has not run yet) you
> shouldn't do that. Exit, emulate and return. We could handle the ioctl
> to clear SS in userspace but I guess that gets just as messy.
> 
>>>>>
>>>>> So perhaps we should stick with your original approach.
>>>>>
>>>>
>>>> I had not realized that was possible. This makes things more complicated for
>>>> avoiding a back and forth with the guest for trapped exceptions. Out of
>>>> luck, having the debug flag does look like single stepping would work as
>>>> expected for userland MMIOs.
>>>
>>> Yes, I think your approach is the best choice now, considering.
>>>
>>>>
>>>> I can try to detail the comment in kvm_arm_setup_debug when we set SPSR,
>>>> hopefully making things clearer when seeing that part of the code.
>>>>
>>>
>>> I also think we need to improve the comment in the world-switch return
>>> path, and I'd like Alex to weigh in here before we merge this.   He's
>>> back from holiday on Monday.
>>>
>>
>> Ping Alex?
> 
> Sorry for the delay getting back to you. I had flagged the email but
> with holidays and conferences in the way it fell off my queue.
> 

No problem, thanks for looking at it.

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

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

Cheers,

-- 
Julien Thierry

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

[Alex Bennée]

Julien Thierry <julien.thierry@arm.com> writes:

> On 03/10/17 15:57, Alex Benn?e wrote:
>>
>> Julien Thierry <julien.thierry@arm.com> writes:
>>
>>> On 31/08/17 15:01, Christoffer Dall wrote:
<snip>
>>>>>>>>>>> On 30/08/17 19:53, Christoffer Dall wrote:
>>>>>>>>>>>> Hi Julien,
>>>>>>>>>>>>
>>>>>>>>>>>> [cc'ing Alex Benn?e here who wrote the debug code for arm64]
>>>>>>>>>>>>
<snip>
>>>>> I can try to detail the comment in kvm_arm_setup_debug when we set SPSR,
>>>>> hopefully making things clearer when seeing that part of the code.
>>>>>
>>>>
>>>> I also think we need to improve the comment in the world-switch return
>>>> path, and I'd like Alex to weigh in here before we merge this.   He's
>>>> back from holiday on Monday.
>>>>
>>>
>>> Ping Alex?
>>
>> Sorry for the delay getting back to you. I had flagged the email but
>> with holidays and conferences in the way it fell off my queue.
>>
>
> No problem, thanks for looking at it.
>
>> 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?
>
> 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.

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.

--
Alex Benn?e

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

[Julien Thierry]

On 03/10/17 16:48, Alex Benn?e wrote:
> 
> Julien Thierry <julien.thierry@arm.com> writes:
> 
>> On 03/10/17 15:57, Alex Benn?e wrote:
>>>
>>> Julien Thierry <julien.thierry@arm.com> writes:
>>>
>>>> On 31/08/17 15:01, Christoffer Dall wrote:
> <snip>
>>>>>>>>>>>> On 30/08/17 19:53, Christoffer Dall wrote:
>>>>>>>>>>>>> Hi Julien,
>>>>>>>>>>>>>
>>>>>>>>>>>>> [cc'ing Alex Benn?e here who wrote the debug code for arm64]
>>>>>>>>>>>>>
> <snip>
>>>>>> I can try to detail the comment in kvm_arm_setup_debug when we set SPSR,
>>>>>> hopefully making things clearer when seeing that part of the code.
>>>>>>
>>>>>
>>>>> I also think we need to improve the comment in the world-switch return
>>>>> path, and I'd like Alex to weigh in here before we merge this.   He's
>>>>> back from holiday on Monday.
>>>>>
>>>>
>>>> Ping Alex?
>>>
>>> Sorry for the delay getting back to you. I had flagged the email but
>>> with holidays and conferences in the way it fell off my queue.
>>>
>>
>> No problem, thanks for looking at it.
>>
>>> 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?
>>
>> 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.
> 

I think the issue was that there was no clear way on how to let userland 
know that it has to deal with both MMIO emulation + debug exception in 
one return.
I guess currently userland relies on having KVM_EXIT_DEBUG exit code to 
inform the debugger. I'm not well aware how MMIO works, so maybe the 
userland being aware it has set single step it should also deal with the 
debug case when it perfoms MMIO.

However, KVM_EXIT_MMIO code seem to be part of the arch independent 
interface, so I guess it should have the same semantic on all platforms? 
(and avoid having one where it means "do MMIO" and another where it 
means "do MMIO + debug step if needed")

KVM doc states:

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

Since the pausing is supposed to take place after the instruction has 
completed, I'm not sure we can expect userland to MMIO + debug 
emulation. But maybe I'm not interpreting this correctly.

Otherwise I can have a look at what other arch expect from userland in 
such scenario.

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

Yes, haven't updated it since v4.13, I guess I can repost it rebased 
some 4.14-rc.

Thanks,

-- 
Julien Thierry

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

[Alex Bennée]

Julien Thierry <julien.thierry@arm.com> writes:

> On 03/10/17 15:57, Alex Benn?e wrote:
>>
>> Julien Thierry <julien.thierry@arm.com> writes:
>>
>>> On 31/08/17 15:01, Christoffer Dall wrote:
<snip>
>>>>>>>
>>>>>>> Does that sound like what you had in mind? Or does it seem better than
>>>>>>> the
>>>>>>> current patch?
>>>>>>>
>>>>>> I was thinking to change the skip_instruction function to return an
>>>>>> int, and then call kvm_handle_debug_ss() from skip_instruction, which
>>>>>> would update the kvm_run structure and exit here and then.
>>>>>>
>>>>>
>>>>> Setting up the debug exception from within kvm_skip_instruction seem to
>>>>> change a bit too much its semantic from arm to arm64. I would find this
>>>>> easily confusing.
>>>>>
>>>>>> However, I'm now thinking that this doesn't really work either,
>>>>>> because we could have to emulate a trapped MMIO instruction in user
>>>>>> space, and then it's not clear how to exit with a debug exception at
>>>>>> the same time.
>>
>> A debug exception at guest exit point is (IIRC) just having the
>> appropriate status in the run->exit_reason (KVM_EXIT_DEBUG). If you need
>> to exit for MMIO emulation (i.e. the instruction has not run yet) you
>> shouldn't do that. Exit, emulate and return. We could handle the ioctl
>> to clear SS in userspace but I guess that gets just as messy.
>>
>>>>>>
>>>>>> So perhaps we should stick with your original approach.
>>>>>>
>>>>>
>>>>> I had not realized that was possible. This makes things more complicated for
>>>>> avoiding a back and forth with the guest for trapped exceptions. Out of
>>>>> luck, having the debug flag does look like single stepping would work as
>>>>> expected for userland MMIOs.

<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?= <alex.bennee@linaro.org>
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 <alex.bennee@linaro.org>
---
 arch/arm64/kvm/handle_exit.c | 51 ++++++++++++++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 7debb74843a0..b197ffb10e96 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -178,6 +178,42 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
 	return arm_exit_handlers[hsr_ec];
 }

+/*
+ * 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".
+ */
+
+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

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

[Julien Thierry]

On 03/10/17 17:30, Alex Benn?e wrote:
> 
> Julien Thierry <julien.thierry@arm.com> writes:
> 
>> On 03/10/17 15:57, Alex Benn?e wrote:
>>>
>>> Julien Thierry <julien.thierry@arm.com> writes:
>>>
>>>> On 31/08/17 15:01, Christoffer Dall wrote:
> <snip>
>>>>>>>>
>>>>>>>> Does that sound like what you had in mind? Or does it seem better than
>>>>>>>> the
>>>>>>>> current patch?
>>>>>>>>
>>>>>>> I was thinking to change the skip_instruction function to return an
>>>>>>> int, and then call kvm_handle_debug_ss() from skip_instruction, which
>>>>>>> would update the kvm_run structure and exit here and then.
>>>>>>>
>>>>>>
>>>>>> Setting up the debug exception from within kvm_skip_instruction seem to
>>>>>> change a bit too much its semantic from arm to arm64. I would find this
>>>>>> easily confusing.
>>>>>>
>>>>>>> However, I'm now thinking that this doesn't really work either,
>>>>>>> because we could have to emulate a trapped MMIO instruction in user
>>>>>>> space, and then it's not clear how to exit with a debug exception at
>>>>>>> the same time.
>>>
>>> A debug exception at guest exit point is (IIRC) just having the
>>> appropriate status in the run->exit_reason (KVM_EXIT_DEBUG). If you need
>>> to exit for MMIO emulation (i.e. the instruction has not run yet) you
>>> shouldn't do that. Exit, emulate and return. We could handle the ioctl
>>> to clear SS in userspace but I guess that gets just as messy.
>>>
>>>>>>>
>>>>>>> So perhaps we should stick with your original approach.
>>>>>>>
>>>>>>
>>>>>> I had not realized that was possible. This makes things more complicated for
>>>>>> avoiding a back and forth with the guest for trapped exceptions. Out of
>>>>>> luck, having the debug flag does look like single stepping would work as
>>>>>> expected for userland MMIOs.
> 
> <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?= <alex.bennee@linaro.org>
> 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 <alex.bennee@linaro.org>
> ---
>   arch/arm64/kvm/handle_exit.c | 51 ++++++++++++++++++++++++++++++++------------
>   1 file changed, 37 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 7debb74843a0..b197ffb10e96 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -178,6 +178,42 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
>   	return arm_exit_handlers[hsr_ec];
>   }
> 
> +/*
> + * 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.

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

-- 
Julien Thierry

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

[Alex Bennée]

Julien Thierry <julien.thierry@arm.com> writes:

> On 03/10/17 17:30, Alex Benn?e wrote:
>>
>> Julien Thierry <julien.thierry@arm.com> writes:
>>
>>> On 03/10/17 15:57, Alex Benn?e wrote:
>>>>
>>>> Julien Thierry <julien.thierry@arm.com> writes:
>>>>
>>>>> On 31/08/17 15:01, Christoffer Dall wrote:
>> <snip>
>>>>>>>>>
>>>>>>>>> Does that sound like what you had in mind? Or does it seem better than
>>>>>>>>> the
>>>>>>>>> current patch?
>>>>>>>>>
>>>>>>>> I was thinking to change the skip_instruction function to return an
>>>>>>>> int, and then call kvm_handle_debug_ss() from skip_instruction, which
>>>>>>>> would update the kvm_run structure and exit here and then.
>>>>>>>>
>>>>>>>
>>>>>>> Setting up the debug exception from within kvm_skip_instruction seem to
>>>>>>> change a bit too much its semantic from arm to arm64. I would find this
>>>>>>> easily confusing.
>>>>>>>
>>>>>>>> However, I'm now thinking that this doesn't really work either,
>>>>>>>> because we could have to emulate a trapped MMIO instruction in user
>>>>>>>> space, and then it's not clear how to exit with a debug exception at
>>>>>>>> the same time.
>>>>
>>>> A debug exception at guest exit point is (IIRC) just having the
>>>> appropriate status in the run->exit_reason (KVM_EXIT_DEBUG). If you need
>>>> to exit for MMIO emulation (i.e. the instruction has not run yet) you
>>>> shouldn't do that. Exit, emulate and return. We could handle the ioctl
>>>> to clear SS in userspace but I guess that gets just as messy.
>>>>
>>>>>>>>
>>>>>>>> So perhaps we should stick with your original approach.
>>>>>>>>
>>>>>>>
>>>>>>> I had not realized that was possible. This makes things more complicated for
>>>>>>> avoiding a back and forth with the guest for trapped exceptions. Out of
>>>>>>> luck, having the debug flag does look like single stepping would work as
>>>>>>> expected for userland MMIOs.
>>
>> <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?= <alex.bennee@linaro.org>
>> 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 <alex.bennee@linaro.org>
>> ---
>>   arch/arm64/kvm/handle_exit.c | 51 ++++++++++++++++++++++++++++++++------------
>>   1 file changed, 37 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>> index 7debb74843a0..b197ffb10e96 100644
>> --- a/arch/arm64/kvm/handle_exit.c
>> +++ b/arch/arm64/kvm/handle_exit.c
>> @@ -178,6 +178,42 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
>>   	return arm_exit_handlers[hsr_ec];
>>   }
>>
>> +/*
>> + * 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?

--
Alex Benn?e

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

[Julien Thierry]

On 03/10/17 18:26, Alex Benn?e wrote:
> 
> Julien Thierry <julien.thierry@arm.com> writes:
> 
>> On 03/10/17 17:30, Alex Benn?e wrote:
>>>
>>> Julien Thierry <julien.thierry@arm.com> writes:
>>>
>>>> On 03/10/17 15:57, Alex Benn?e wrote:
>>>>>
>>>>> Julien Thierry <julien.thierry@arm.com> writes:
>>>>>
>>>>>> On 31/08/17 15:01, Christoffer Dall wrote:
>>> <snip>
>>>>>>>>>>
>>>>>>>>>> Does that sound like what you had in mind? Or does it seem better than
>>>>>>>>>> the
>>>>>>>>>> current patch?
>>>>>>>>>>
>>>>>>>>> I was thinking to change the skip_instruction function to return an
>>>>>>>>> int, and then call kvm_handle_debug_ss() from skip_instruction, which
>>>>>>>>> would update the kvm_run structure and exit here and then.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Setting up the debug exception from within kvm_skip_instruction seem to
>>>>>>>> change a bit too much its semantic from arm to arm64. I would find this
>>>>>>>> easily confusing.
>>>>>>>>
>>>>>>>>> However, I'm now thinking that this doesn't really work either,
>>>>>>>>> because we could have to emulate a trapped MMIO instruction in user
>>>>>>>>> space, and then it's not clear how to exit with a debug exception at
>>>>>>>>> the same time.
>>>>>
>>>>> A debug exception at guest exit point is (IIRC) just having the
>>>>> appropriate status in the run->exit_reason (KVM_EXIT_DEBUG). If you need
>>>>> to exit for MMIO emulation (i.e. the instruction has not run yet) you
>>>>> shouldn't do that. Exit, emulate and return. We could handle the ioctl
>>>>> to clear SS in userspace but I guess that gets just as messy.
>>>>>
>>>>>>>>>
>>>>>>>>> So perhaps we should stick with your original approach.
>>>>>>>>>
>>>>>>>>
>>>>>>>> I had not realized that was possible. This makes things more complicated for
>>>>>>>> avoiding a back and forth with the guest for trapped exceptions. Out of
>>>>>>>> luck, having the debug flag does look like single stepping would work as
>>>>>>>> expected for userland MMIOs.
>>>
>>> <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?= <alex.bennee@linaro.org>
>>> 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 <alex.bennee@linaro.org>
>>> ---
>>>    arch/arm64/kvm/handle_exit.c | 51 ++++++++++++++++++++++++++++++++------------
>>>    1 file changed, 37 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>>> index 7debb74843a0..b197ffb10e96 100644
>>> --- a/arch/arm64/kvm/handle_exit.c
>>> +++ b/arch/arm64/kvm/handle_exit.c
>>> @@ -178,6 +178,42 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
>>>    	return arm_exit_handlers[hsr_ec];
>>>    }
>>>
>>> +/*
>>> + * 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?
> 

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.

Thanks,

-- 
Julien Thierry

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

[Alex Bennée]

[Added Paolo, including QEMU userspace patch]

Julien Thierry <julien.thierry@arm.com> writes:

> On 03/10/17 18:26, Alex Benn?e wrote:
>>
>> Julien Thierry <julien.thierry@arm.com> writes:
>>
>>> On 03/10/17 17:30, Alex Benn?e wrote:
>>>>
>>>> Julien Thierry <julien.thierry@arm.com> writes:
>>>>
>>>>> On 03/10/17 15:57, Alex Benn?e wrote:
>>>>>>
>>>>>> Julien Thierry <julien.thierry@arm.com> writes:
>>>>>>
>>>>>>> On 31/08/17 15:01, Christoffer Dall wrote:
>>>> <snip>
>>>>>>>>>>>
>>>>>>>>>>> Does that sound like what you had in mind? Or does it seem better than
>>>>>>>>>>> the
>>>>>>>>>>> current patch?
>>>>>>>>>>>
>>>>>>>>>> I was thinking to change the skip_instruction function to return an
>>>>>>>>>> int, and then call kvm_handle_debug_ss() from skip_instruction, which
>>>>>>>>>> would update the kvm_run structure and exit here and then.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Setting up the debug exception from within kvm_skip_instruction seem to
>>>>>>>>> change a bit too much its semantic from arm to arm64. I would find this
>>>>>>>>> easily confusing.
>>>>>>>>>
>>>>>>>>>> However, I'm now thinking that this doesn't really work either,
>>>>>>>>>> because we could have to emulate a trapped MMIO instruction in user
>>>>>>>>>> space, and then it's not clear how to exit with a debug exception at
>>>>>>>>>> the same time.
>>>>>>
>>>>>> A debug exception at guest exit point is (IIRC) just having the
>>>>>> appropriate status in the run->exit_reason (KVM_EXIT_DEBUG). If you need
>>>>>> to exit for MMIO emulation (i.e. the instruction has not run yet) you
>>>>>> shouldn't do that. Exit, emulate and return. We could handle the ioctl
>>>>>> to clear SS in userspace but I guess that gets just as messy.
>>>>>>
>>>>>>>>>>
>>>>>>>>>> So perhaps we should stick with your original approach.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I had not realized that was possible. This makes things more complicated for
>>>>>>>>> avoiding a back and forth with the guest for trapped exceptions. Out of
>>>>>>>>> luck, having the debug flag does look like single stepping would work as
>>>>>>>>> expected for userland MMIOs.
>>>>
>>>> <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?= <alex.bennee@linaro.org>
>>>> 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 <alex.bennee@linaro.org>
>>>> ---
>>>>    arch/arm64/kvm/handle_exit.c | 51 ++++++++++++++++++++++++++++++++------------
>>>>    1 file changed, 37 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>>>> index 7debb74843a0..b197ffb10e96 100644
>>>> --- a/arch/arm64/kvm/handle_exit.c
>>>> +++ b/arch/arm64/kvm/handle_exit.c
>>>> @@ -178,6 +178,42 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
>>>>    	return arm_exit_handlers[hsr_ec];
>>>>    }
>>>>
>>>> +/*
>>>> + * 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?
>>
>
> 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?

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

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

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?= <alex.bennee@linaro.org>
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 <alex.bennee@linaro.org>
---
 accel/kvm/kvm-all.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 90c88b517d..85bcb2b0d4 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -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




>
> Thanks,


--
Alex Benn?e

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

[Paolo Bonzini]

On 04/10/2017 12:08, Alex Benn?e wrote:
> 
> From 2e8fcea695a9eca67fbeb331d3104d1d9e7e337a Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <alex.bennee@linaro.org>
> 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 <alex.bennee@linaro.org>
> ---
>  accel/kvm/kvm-all.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 90c88b517d..85bcb2b0d4 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -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");

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?

Paolo

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

[Julien Thierry]

On 04/10/17 11:08, Alex Benn?e wrote:
> 
> [Added Paolo, including QEMU userspace patch]
> 
> Julien Thierry <julien.thierry@arm.com> writes:
> 
>> On 03/10/17 18:26, Alex Benn?e wrote:
>>>
>>> Julien Thierry <julien.thierry@arm.com> writes:
>>>
>>>> On 03/10/17 17:30, Alex Benn?e wrote:
>>>>>
>>>>> Julien Thierry <julien.thierry@arm.com> writes:
>>>>>
>>>>>> On 03/10/17 15:57, Alex Benn?e wrote:
>>>>>>>
>>>>>>> Julien Thierry <julien.thierry@arm.com> writes:
>>>>>>>
>>>>>>>> On 31/08/17 15:01, Christoffer Dall wrote:
>>>>> <snip>
>>>>>>>>>>>>
>>>>>>>>>>>> Does that sound like what you had in mind? Or does it seem better than
>>>>>>>>>>>> the
>>>>>>>>>>>> current patch?
>>>>>>>>>>>>
>>>>>>>>>>> I was thinking to change the skip_instruction function to return an
>>>>>>>>>>> int, and then call kvm_handle_debug_ss() from skip_instruction, which
>>>>>>>>>>> would update the kvm_run structure and exit here and then.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Setting up the debug exception from within kvm_skip_instruction seem to
>>>>>>>>>> change a bit too much its semantic from arm to arm64. I would find this
>>>>>>>>>> easily confusing.
>>>>>>>>>>
>>>>>>>>>>> However, I'm now thinking that this doesn't really work either,
>>>>>>>>>>> because we could have to emulate a trapped MMIO instruction in user
>>>>>>>>>>> space, and then it's not clear how to exit with a debug exception at
>>>>>>>>>>> the same time.
>>>>>>>
>>>>>>> A debug exception at guest exit point is (IIRC) just having the
>>>>>>> appropriate status in the run->exit_reason (KVM_EXIT_DEBUG). If you need
>>>>>>> to exit for MMIO emulation (i.e. the instruction has not run yet) you
>>>>>>> shouldn't do that. Exit, emulate and return. We could handle the ioctl
>>>>>>> to clear SS in userspace but I guess that gets just as messy.
>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> So perhaps we should stick with your original approach.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I had not realized that was possible. This makes things more complicated for
>>>>>>>>>> avoiding a back and forth with the guest for trapped exceptions. Out of
>>>>>>>>>> luck, having the debug flag does look like single stepping would work as
>>>>>>>>>> expected for userland MMIOs.
>>>>>
>>>>> <snip>
>>
>> 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.
> 
>> 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.
> 

Well I'm not sure. The part I am concerned about is:
> 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.

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

> QEMU Patch bellow:
> 
>  From 2e8fcea695a9eca67fbeb331d3104d1d9e7e337a Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <alex.bennee@linaro.org>
> 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 <alex.bennee@linaro.org>
> ---
>   accel/kvm/kvm-all.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 90c88b517d..85bcb2b0d4 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -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
> 

-- 
Julien Thierry

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

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 04/10/2017 12:08, Alex Benn?e wrote:
>>
>> From 2e8fcea695a9eca67fbeb331d3104d1d9e7e337a Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <alex.bennee@linaro.org>
>> 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 <alex.bennee@linaro.org>
>> ---
>>  accel/kvm/kvm-all.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index 90c88b517d..85bcb2b0d4 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -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");
>
> 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.

--
Alex Benn?e

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

[Paolo Bonzini]

On 04/10/2017 12:50, Alex Benn?e wrote:
> 
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 04/10/2017 12:08, Alex Benn?e wrote:
>>>
>>> From 2e8fcea695a9eca67fbeb331d3104d1d9e7e337a Mon Sep 17 00:00:00 2001
>>> From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <alex.bennee@linaro.org>
>>> 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 <alex.bennee@linaro.org>
>>> ---
>>>  accel/kvm/kvm-all.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>> index 90c88b517d..85bcb2b0d4 100644
>>> --- a/accel/kvm/kvm-all.c
>>> +++ b/accel/kvm/kvm-all.c
>>> @@ -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");
>>
>> 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.

Nevermind, I was confusing cpu->singlestep_enabled with the "singlestep"
global.

Paolo

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

[Alex Bennée]

Julien Thierry <julien.thierry@arm.com> writes:

> On 04/10/17 11:08, Alex Benn?e wrote:
>>
>> [Added Paolo, including QEMU userspace patch]
>>
>> Julien Thierry <julien.thierry@arm.com> writes:
>>
>>> On 03/10/17 18:26, Alex Benn?e wrote:
>>>>
>>>> Julien Thierry <julien.thierry@arm.com> writes:
>>>>
>>>>> On 03/10/17 17:30, Alex Benn?e wrote:
>>>>>>
>>>>>> Julien Thierry <julien.thierry@arm.com> writes:
>>>>>>
>>>>>>> On 03/10/17 15:57, Alex Benn?e wrote:
>>>>>>>>
>>>>>>>> Julien Thierry <julien.thierry@arm.com> writes:
>>>>>>>>
>>>>>>>>> On 31/08/17 15:01, Christoffer Dall wrote:
>>>>>> <snip>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Does that sound like what you had in mind? Or does it seem better than
>>>>>>>>>>>>> the
>>>>>>>>>>>>> current patch?
>>>>>>>>>>>>>
>>>>>>>>>>>> I was thinking to change the skip_instruction function to return an
>>>>>>>>>>>> int, and then call kvm_handle_debug_ss() from skip_instruction, which
>>>>>>>>>>>> would update the kvm_run structure and exit here and then.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Setting up the debug exception from within kvm_skip_instruction seem to
>>>>>>>>>>> change a bit too much its semantic from arm to arm64. I would find this
>>>>>>>>>>> easily confusing.
>>>>>>>>>>>
>>>>>>>>>>>> However, I'm now thinking that this doesn't really work either,
>>>>>>>>>>>> because we could have to emulate a trapped MMIO instruction in user
>>>>>>>>>>>> space, and then it's not clear how to exit with a debug exception at
>>>>>>>>>>>> the same time.
>>>>>>>>
>>>>>>>> A debug exception at guest exit point is (IIRC) just having the
>>>>>>>> appropriate status in the run->exit_reason (KVM_EXIT_DEBUG). If you need
>>>>>>>> to exit for MMIO emulation (i.e. the instruction has not run yet) you
>>>>>>>> shouldn't do that. Exit, emulate and return. We could handle the ioctl
>>>>>>>> to clear SS in userspace but I guess that gets just as messy.
>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> So perhaps we should stick with your original approach.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I had not realized that was possible. This makes things more complicated for
>>>>>>>>>>> avoiding a back and forth with the guest for trapped exceptions. Out of
>>>>>>>>>>> luck, having the debug flag does look like single stepping would work as
>>>>>>>>>>> expected for userland MMIOs.
>>>>>>
>>>>>> <snip>
>>>
>>> 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.
>>
>>> 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.
>>
>
> Well I'm not sure. The part I am concerned about is:
>> 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.

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@least not behind the curve on this support ;-)

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

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

> 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

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.

>
> Thanks,
>
>> QEMU Patch bellow:
>>
>>  From 2e8fcea695a9eca67fbeb331d3104d1d9e7e337a Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <alex.bennee@linaro.org>
>> 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 <alex.bennee@linaro.org>
>> ---
>>   accel/kvm/kvm-all.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index 90c88b517d..85bcb2b0d4 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -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
>>


--
Alex Benn?e

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

[Julien Thierry]

On 04/10/17 16:42, Alex Benn?e wrote:
> 
> Julien Thierry <julien.thierry@arm.com> writes:
> 
>> On 04/10/17 11:08, Alex Benn?e wrote:
>>>
>>> [Added Paolo, including QEMU userspace patch]
>>>
>>> Julien Thierry <julien.thierry@arm.com> writes:
>>>
>>>> On 03/10/17 18:26, Alex Benn?e wrote:
>>>>>
>>>>> Julien Thierry <julien.thierry@arm.com> writes:
>>>>>
>>>>>> On 03/10/17 17:30, Alex Benn?e wrote:
>>>>>>>
>>>>>>> Julien Thierry <julien.thierry@arm.com> writes:
>>>>>>>
>>>>>>>> On 03/10/17 15:57, Alex Benn?e wrote:
>>>>>>>>>
>>>>>>>>> Julien Thierry <julien.thierry@arm.com> writes:
>>>>>>>>>
>>>>>>>>>> On 31/08/17 15:01, Christoffer Dall wrote:
>>>>>>> <snip>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Does that sound like what you had in mind? Or does it seem better than
>>>>>>>>>>>>>> the
>>>>>>>>>>>>>> current patch?
>>>>>>>>>>>>>>
>>>>>>>>>>>>> I was thinking to change the skip_instruction function to return an
>>>>>>>>>>>>> int, and then call kvm_handle_debug_ss() from skip_instruction, which
>>>>>>>>>>>>> would update the kvm_run structure and exit here and then.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Setting up the debug exception from within kvm_skip_instruction seem to
>>>>>>>>>>>> change a bit too much its semantic from arm to arm64. I would find this
>>>>>>>>>>>> easily confusing.
>>>>>>>>>>>>
>>>>>>>>>>>>> However, I'm now thinking that this doesn't really work either,
>>>>>>>>>>>>> because we could have to emulate a trapped MMIO instruction in user
>>>>>>>>>>>>> space, and then it's not clear how to exit with a debug exception at
>>>>>>>>>>>>> the same time.
>>>>>>>>>
>>>>>>>>> A debug exception at guest exit point is (IIRC) just having the
>>>>>>>>> appropriate status in the run->exit_reason (KVM_EXIT_DEBUG). If you need
>>>>>>>>> to exit for MMIO emulation (i.e. the instruction has not run yet) you
>>>>>>>>> shouldn't do that. Exit, emulate and return. We could handle the ioctl
>>>>>>>>> to clear SS in userspace but I guess that gets just as messy.
>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> So perhaps we should stick with your original approach.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> I had not realized that was possible. This makes things more complicated for
>>>>>>>>>>>> avoiding a back and forth with the guest for trapped exceptions. Out of
>>>>>>>>>>>> luck, having the debug flag does look like single stepping would work as
>>>>>>>>>>>> expected for userland MMIOs.
>>>>>>>
>>>>>>> <snip>
>>>>
>>>> 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.
>>>
>>>> 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.
>>>
>>
>> Well I'm not sure. The part I am concerned about is:
>>> 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.
> 
> 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
> 

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

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

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

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

Cheers,

>>
>>> QEMU Patch bellow:
>>>
>>>   From 2e8fcea695a9eca67fbeb331d3104d1d9e7e337a Mon Sep 17 00:00:00 2001
>>> From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <alex.bennee@linaro.org>
>>> 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 <alex.bennee@linaro.org>
>>> ---
>>>    accel/kvm/kvm-all.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>> index 90c88b517d..85bcb2b0d4 100644
>>> --- a/accel/kvm/kvm-all.c
>>> +++ b/accel/kvm/kvm-all.c
>>> @@ -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
>>>
> 
> 
> --
> Alex Benn?e
> 

-- 
Julien Thierry

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

[Alex Bennée]

Julien Thierry <julien.thierry@arm.com> writes:

> On 04/10/17 16:42, Alex Benn?e wrote:
>>
<snip>
>>
>> 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
>>
>
> Thanks for the explanation. This approach sounds good to me. Also, it
> means QEMU doesn't need to get patched, is that correct?

Correct.

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

One minor wrinkle in kvm_handle_mmio_return() I can't use
vcpu->debug_flags as the v7 code doesn't have it in kvm_vcpu_arch.

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

Thanks - this will be useful.

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


Nope I'm happy for you to carry it to merge. I'll see if I can send you
a v2 once I've addressed the mmio completion.

>
> Thank you for looking at this and providing your input.

No problem, sorry it took so long.

--
Alex Benn?e