Fix single step for traps
cover Fix single step for traps
[Reposting series rebased on v4.14-rc3]
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.
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 | 17 ++++++++++++++++- arch/arm64/kvm/hyp/switch.c | 10 ++++++++++ 9 files changed, 56 insertions(+), 13 deletions(-)
--
1.9.1
PATCH 1/3 arm64: Use existing defines for mdscr
Signed-off-by Julien Thierry <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
--- arch/arm64/include/asm/assembler.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
arch/arm64/include/asm/assembler.h +3 -2
@@ -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, 9990fdisable_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 1/3 arm64: Use existing defines for mdscr
Quoted 4 lines · Julien Thierry
Signed-off-by Julien Thierry <[email protected]>Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>Cc Mark Rutland <[email protected]>
Signed-off-by Alex Benn?e <[email protected]>
Quoted 27 lines
--- arch/arm64/include/asm/assembler.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)arch/arm64/include/asm/assembler.h +2
-1@@ -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:
arch/arm64/include/asm/assembler.h line 82
Quoted 8 lines
arch/arm64/include/asm/assembler.h +1
-1@@ -75,7 +76,7 @@ tbz \flgs, #TIF_SINGLESTEP, 9990fdisable_dbg mrs \tmp, mdscr_el1 - orr \tmp, \tmp, #1 + orr \tmp, \tmp, #DBG_MDSCR_SS msr mdscr_el1, \tmp 9990: .endm
--
Alex Benn?e
PATCH 1/3 arm64: Use existing defines for mdscr
In reply to Julien ThierryQuoted 4 lines · Julien Thierry
Signed-off-by Julien Thierry <[email protected]>Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>Cc Mark Rutland <[email protected]>
Sorry I meant:
Reviewed-by Alex Benn?e <[email protected]>
Quoted 27 lines
--- arch/arm64/include/asm/assembler.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)arch/arm64/include/asm/assembler.h +2
-1@@ -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:
arch/arm64/include/asm/assembler.h line 82
Quoted 8 lines
arch/arm64/include/asm/assembler.h +1
-1@@ -75,7 +76,7 @@ tbz \flgs, #TIF_SINGLESTEP, 9990fdisable_dbg mrs \tmp, mdscr_el1 - orr \tmp, \tmp, #1 + orr \tmp, \tmp, #DBG_MDSCR_SS msr mdscr_el1, \tmp 9990: .endm
--
Alex Benn?e
PATCH 2/3 arm64: Fix single stepping in kernel traps
In reply to Julien ThierrySoftware 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 <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
--- 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(-)
arch/arm64/include/asm/traps.h +2 -0
@@ -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); +static inline int __in_irqentry_text (unsigned long ptr) { return ptr >= (unsigned long)&__irqentry_text_start &&
arch/arm64/kernel/armv8_deprecated.c +4 -4
@@ -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; }
arch/arm64/kernel/cpufeature.c +1 -1
@@ -1287,7 +1287,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;
arch/arm64/kernel/traps.c +16 -5
@@ -293,6 +293,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);
@@ -480,7 +491,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)
@@ -490,7 +501,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)
@@ -498,7 +509,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)
@@ -506,7 +517,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 {
@@ -761,7 +772,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 2/3 arm64: Fix single stepping in kernel traps
Quoted 22 lines · 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 <[email protected]>Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>Cc Mark Rutland <[email protected]>--- 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(-)arch/arm64/include/asm/traps.h +0
-0
arch/arm64/include/asm/traps.h struct undef_hook { · line 41
Quoted 5 lines
arch/arm64/include/asm/traps.h +2
-0@@ -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); +
Personally I think this is a poor name as it implies there is no effect from the operation. I suspect arm64_update_regs is a little too generic through. Naming things as ever is hard....
Quoted 26 lines
arch/arm64/include/asm/traps.h +0
-0static inline int __in_irqentry_text (unsigned long ptr) { return ptr >= (unsigned long)&__irqentry_text_start &&arch/arm64/kernel/armv8_deprecated.c +2
-2@@ -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 ; }
arch/arm64/kernel/armv8_deprecated.c static int compat_setend_handler(struct pt_regs *regs, u32 big_endian) · line 591
Quoted 7 lines
arch/arm64/kernel/armv8_deprecated.c +1
-1@@ -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; }
Why didn't we use AARCH64_INSN_SIZE for these here?
arch/arm64/kernel/armv8_deprecated.c static int compat_setend_handler(struct pt_regs *regs, u32 big_endian) · line 598
Quoted 8 lines
arch/arm64/kernel/armv8_deprecated.c +1
-1static 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; }
I guess we don't have an equivalent AARCH32_T16_INSN_SIZE?
Quoted 73 lines
arch/arm64/kernel/cpufeature.c +1
-1@@ -1287,7 +1287,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;arch/arm64/kernel/traps.c +15
-4@@ -293,6 +293,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);@@ -480,7 +491,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)@@ -490,7 +501,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)@@ -498,7 +509,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)@@ -506,7 +517,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 {
arch/arm64/kernel/traps.c static int bug_handler(struct pt_regs *regs, unsigned int esr) · line 777
Quoted 7 lines
arch/arm64/kernel/traps.c +1
-1@@ -761,7 +772,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 ; }
It would be nice to find a better name but otherwise:
Reviewed-by Alex Benn?e <[email protected]>
--
Alex Benn?e
PATCH 2/3 arm64: Fix single stepping in kernel traps
Quoted 26 lines · Alex Benn?e
Quoted 23 lines · 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 <[email protected]>Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>Cc Mark Rutland <[email protected]>--- 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(-)arch/arm64/include/asm/traps.h +0
-0@@ -37,6 +37,8 @@ struct undef_hook {
Quoted 8 lines
arch/arm64/include/asm/traps.h struct undef_hook { · line 41
Quoted 4 lines
arch/arm64/include/asm/traps.h +2
-0void arm64_notify_segfault (struct pt_regs *regs,unsigned long addr); +void arm64_skip_trapped_instr (struct pt_regs *regs,unsigned long size); +Personally I think this is a poor name as it implies there is no effect from the operation. I suspect
arm64_update_regsis a little too generic through. Naming things as ever is hard....
What about arm64_step_trapped_instr? Don't know if it is much better but maybe less misleading than just "skip".
Or arm64_setup_next_instr?
Quoted 26 lines
Quoted 26 lines
arch/arm64/include/asm/traps.h +0
-0static inline int __in_irqentry_text (unsigned long ptr) { return ptr >= (unsigned long)&__irqentry_text_start &&arch/arm64/kernel/armv8_deprecated.c +2
-2@@ -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 ; }
Quoted 10 lines
arch/arm64/kernel/armv8_deprecated.c static int compat_setend_handler(struct pt_regs *regs, u32 big_endian) · line 591
Quoted 8 lines
arch/arm64/kernel/armv8_deprecated.c +1
-1@@ -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; }Why didn't we use
AARCH64_INSN_SIZEfor these here?
Because these are AARCH32 instructions, although it is the same size, I thought it would be misleading to use AARCH64 macro.
Quoted 12 lines
arch/arm64/kernel/armv8_deprecated.c static int compat_setend_handler(struct pt_regs *regs, u32 big_endian) · line 598
Quoted 9 lines
arch/arm64/kernel/armv8_deprecated.c +1
-1static 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; }I guess we don't have an equivalent
AARCH32_T16_INSN_SIZE?
Yes, I was a bit hesitant because thumb2 can have 16 and 32bits instructions, but I guess having "T16" in the macro name makes it clear it should be used only on 16bits instructions.
I'll add this define, and might as well add AARCH32_INSN_SIZE.
Quoted 83 lines
Quoted 73 lines
arch/arm64/kernel/cpufeature.c +1
-1@@ -1287,7 +1287,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;arch/arm64/kernel/traps.c +15
-4@@ -293,6 +293,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);@@ -480,7 +491,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)@@ -490,7 +501,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)@@ -498,7 +509,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)@@ -506,7 +517,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 {arch/arm64/kernel/traps.c static int bug_handler(struct pt_regs *regs, unsigned int esr) · line 777
Quoted 7 lines
arch/arm64/kernel/traps.c +1
-1@@ -761,7 +772,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 ; }It would be nice to find a better name but otherwise:
Quoted 2 lines
Reviewed-by Alex Benn?e <[email protected]>
Thanks,
Signature · 1 line
Julien Thierry
PATCH 2/3 arm64: Fix single stepping in kernel traps
Quoted 36 lines · Julien Thierry
Quoted 26 lines · Alex Benn?e
Quoted 23 lines · 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 <[email protected]>Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>Cc Mark Rutland <[email protected]>--- 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(-)arch/arm64/include/asm/traps.h +0
-0@@ -37,6 +37,8 @@ struct undef_hook {Quoted 8 lines
arch/arm64/include/asm/traps.h struct undef_hook { · line 41
Quoted 4 lines
arch/arm64/include/asm/traps.h +2
-0void arm64_notify_segfault (struct pt_regs *regs,unsigned long addr); +void arm64_skip_trapped_instr (struct pt_regs *regs,unsigned long size); +Personally I think this is a poor name as it implies there is no effect from the operation. I suspect
arm64_update_regsis a little too generic through. Naming things as ever is hard....
Quoted 4 lines
What about
arm64_step_trapped_instr? Don't know if it is much better but maybe less misleading than just "skip".Or
arm64_setup_next_instr?
Yes, I like arm64_setup_next_instr() better.
Quoted 38 lines
Quoted 27 lines
Quoted 27 lines
arch/arm64/include/asm/traps.h +0
-0static inline int __in_irqentry_text (unsigned long ptr) { return ptr >= (unsigned long)&__irqentry_text_start &&arch/arm64/kernel/armv8_deprecated.c +2
-2@@ -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 ; }Quoted 10 lines
arch/arm64/kernel/armv8_deprecated.c static int compat_setend_handler(struct pt_regs *regs, u32 big_endian) · line 591
Quoted 8 lines
arch/arm64/kernel/armv8_deprecated.c +1
-1@@ -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; }Why didn't we use
AARCH64_INSN_SIZEfor these here?
Quoted 2 lines
Because these are AARCH32 instructions, although it is the same size, I thought it would be misleading to use AARCH64 macro.
Ahh I was thrown by the fact we are in armv8_deprecated.c but of course AArch32 is part of armv8...
Quoted 14 lines
Quoted 13 lines
arch/arm64/kernel/armv8_deprecated.c static int compat_setend_handler(struct pt_regs *regs, u32 big_endian) · line 598
Quoted 10 lines
arch/arm64/kernel/armv8_deprecated.c +1
-1static 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; }I guess we don't have an equivalent
AARCH32_T16_INSN_SIZE?
Quoted 5 lines
Yes, I was a bit hesitant because thumb2 can have 16 and 32bits instructions, but I guess having "T16" in the macro name makes it clear it should be used only on 16bits instructions.
I'll add this define, and might as well add
AARCH32_INSN_SIZE.
Maybe AARCH32_T32_INSN_SIZE and AARCH32_T16_INSN_SIZE? Then it is clear there are two encodings.
Quoted 84 lines
Quoted 84 lines
Quoted 74 lines
arch/arm64/kernel/cpufeature.c +1
-1@@ -1287,7 +1287,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;arch/arm64/kernel/traps.c +15
-4@@ -293,6 +293,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);@@ -480,7 +491,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)@@ -490,7 +501,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)@@ -498,7 +509,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)@@ -506,7 +517,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 {arch/arm64/kernel/traps.c static int bug_handler(struct pt_regs *regs, unsigned int esr) · line 777
Quoted 7 lines
arch/arm64/kernel/traps.c +1
-1@@ -761,7 +772,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 ; }It would be nice to find a better name but otherwise:
Quoted 4 lines
Quoted 2 lines
Reviewed-by Alex Benn?e <[email protected]>Thanks,
--
Alex Benn?e
PATCH 3/3 arm64: kvm: Fix single step for guest skipped instructions
In reply to Julien ThierrySoftware Step exception is missing after trapping instruction from the guest.
We need to set the PSR.SS to 0 for the guest vcpu before resuming guest execution.
Signed-off-by Julien Thierry <[email protected]>
Cc Christoffer Dall <[email protected]>
Cc Marc Zyngier <[email protected]>
Cc Alex Benn?e <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
--- arch/arm64/include/asm/kvm_asm.h | 2 ++ arch/arm64/include/asm/kvm_emulate.h | 2 ++ arch/arm64/kvm/debug.c | 17 ++++++++++++++++- arch/arm64/kvm/hyp/switch.c | 10 ++++++++++ 4 files changed, 30 insertions(+), 1 deletion(-)
arch/arm64/include/asm/kvm_asm.h +2 -0
@@ -32,6 +32,8 @@#define KVM_ARM64_DEBUG_DIRTY_SHIFT 0#define KVM_ARM64_DEBUG_DIRTY (1 << KVM_ARM64_DEBUG_DIRTY_SHIFT) +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT 1 +#define KVM_ARM64_DEBUG_INST_SKIP (1 << KVM_ARM64_DEBUG_INST_SKIP_SHIFT)#define kvm_ksym_ref (sym) \ ({ \
arch/arm64/include/asm/kvm_emulate.h +2 -0
@@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu *vcpu, bool is_wide_instr)kvm_skip_instr32 (vcpu, is_wide_instr); else *vcpu_pc (vcpu) +=4 ; +/* Let debug engine know we skipped an instruction */ + vcpu->arch.debug_flags |=KVM_ARM64_DEBUG_INST_SKIP ; }static inline void vcpu_set_thumb (struct kvm_vcpu *vcpu)
arch/arm64/kvm/debug.c +16 -1
@@ -151,12 +151,27 @@ 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 ; +/* +* Taking a Software step exception, context being +* stepped has PSTATE.SS == 0. In order to step the next +* instruction, we need to reset this bit. +* If we skipped an instruction while single stepping, +* we want to get a software step exception for the +* skipped instruction (i.e. as soon as we return to the +* guest). This is obtained by returning to the guest +* with PSTATE.SS cleared. +*/ +if (!(vcpu->arch.debug_flags &KVM_ARM64_DEBUG_INST_SKIP )) + *vcpu_cpsr (vcpu) |=DBG_SPSR_SS ; +else + *vcpu_cpsr (vcpu) &= ~DBG_SPSR_SS ;vcpu_sys_reg (vcpu,MDSCR_EL1 ) |=DBG_MDSCR_SS ; }else {vcpu_sys_reg (vcpu,MDSCR_EL1 ) &= ~DBG_MDSCR_SS ; } + vcpu->arch.debug_flags &= ~KVM_ARM64_DEBUG_INST_SKIP ; +trace_kvm_arm_set_dreg32 ("SPSR_EL2" , *vcpu_cpsr (vcpu));/*
arch/arm64/kvm/hyp/switch.c +10 -0
@@ -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,8 @@ static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu) }write_sysreg_el2 (*vcpu_pc (vcpu), elr); + +write_sysreg_el2 (read_sysreg_el2 (spsr) & ~DBG_SPSR_SS , spsr); } int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)
@@ -343,6 +346,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
Quoted 37 lines · Julien Thierry
Software Step exception is missing after trapping instruction from the guest.
We need to set the PSR.SS to 0 for the guest vcpu before resuming guest execution.
Signed-off-by Julien Thierry <[email protected]>Cc Christoffer Dall <[email protected]>Cc Marc Zyngier <[email protected]>Cc Alex Benn?e <[email protected]>Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>--- arch/arm64/include/asm/kvm_asm.h | 2 ++ arch/arm64/include/asm/kvm_emulate.h | 2 ++ arch/arm64/kvm/debug.c | 17 ++++++++++++++++- arch/arm64/kvm/hyp/switch.c | 10 ++++++++++ 4 files changed, 30 insertions(+), 1 deletion(-)arch/arm64/include/asm/kvm_asm.h +2
-0@@ -32,6 +32,8 @@#define KVM_ARM64_DEBUG_DIRTY_SHIFT 0#define KVM_ARM64_DEBUG_DIRTY (1 << KVM_ARM64_DEBUG_DIRTY_SHIFT) +#define KVM_ARM64_DEBUG_INST_SKIP_SHIFT 1 +#define KVM_ARM64_DEBUG_INST_SKIP (1 << KVM_ARM64_DEBUG_INST_SKIP_SHIFT)#define kvm_ksym_ref (sym) \ ({ \arch/arm64/include/asm/kvm_emulate.h +0
-0
arch/arm64/include/asm/kvm_emulate.h static inline void kvm_skip_instr(struct kvm_vcpu *vcpu, bool is_wide_instr) · line 100
Quoted 6 lines
arch/arm64/include/asm/kvm_emulate.h +2
-0@@ -95,6 +95,8 @@ static inline void kvm_skip_instr(struct kvm_vcpu *vcpu, bool is_wide_instr)kvm_skip_instr32 (vcpu, is_wide_instr); else *vcpu_pc (vcpu) +=4 ; +/* Let debug engine know we skipped an instruction */ + vcpu->arch.debug_flags |=KVM_ARM64_DEBUG_INST_SKIP ; }
I can see the appeal of handling things here but I think for both trapped system instructions and mmio emulation everything flows back to handle_exit() where we could deal with it there and then.
Quoted 48 lines
arch/arm64/include/asm/kvm_emulate.h +0
-0static inline void vcpu_set_thumb (struct kvm_vcpu *vcpu)arch/arm64/kvm/debug.c +16
-1@@ -151,12 +151,27 @@ 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 ; +/* +* Taking a Software step exception, context being +* stepped has PSTATE.SS == 0. In order to step the next +* instruction, we need to reset this bit. +* If we skipped an instruction while single stepping, +* we want to get a software step exception for the +* skipped instruction (i.e. as soon as we return to the +* guest). This is obtained by returning to the guest +* with PSTATE.SS cleared. +*/ +if (!(vcpu->arch.debug_flags &KVM_ARM64_DEBUG_INST_SKIP )) + *vcpu_cpsr (vcpu) |=DBG_SPSR_SS ; +else + *vcpu_cpsr (vcpu) &= ~DBG_SPSR_SS ;vcpu_sys_reg (vcpu,MDSCR_EL1 ) |=DBG_MDSCR_SS ; }else {vcpu_sys_reg (vcpu,MDSCR_EL1 ) &= ~DBG_MDSCR_SS ; } + vcpu->arch.debug_flags &= ~KVM_ARM64_DEBUG_INST_SKIP ; +trace_kvm_arm_set_dreg32 ("SPSR_EL2" , *vcpu_cpsr (vcpu));/* arch/arm64/kvm/hyp/switch.c +1
-0@@ -22,6 +22,7 @@#include <asm/kvm_emulate.h> #include <asm/kvm_hyp.h> #include <asm/fpsimd.h> +#include <asm/debug-monitors.h> static bool __hyp_text __fpsimd_enabled_nvhe(void) {
arch/arm64/kvm/hyp/switch.c static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu) · line 282
Quoted 6 lines
arch/arm64/kvm/hyp/switch.c +2
-0@@ -276,6 +277,8 @@ static void __hyp_text __skip_instr(struct kvm_vcpu *vcpu) }write_sysreg_el2 (*vcpu_pc (vcpu), elr); + +write_sysreg_el2 (read_sysreg_el2 (spsr) & ~DBG_SPSR_SS , spsr); }
Hmm this function seems a bit magical - given it is manipulating PC/ELR. I guess it is only called for eret and other exception returns? I wonder if a rename to __skip_eret_instr would be in order for clarity?
arch/arm64/kvm/hyp/switch.c int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu) · line 355
Quoted 13 lines
arch/arm64/kvm/hyp/switch.c +7
-0int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu)@@ -343,6 +346,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'm not so sure of this. We are exiting so it seems we are going to persist some debug state over an exit where an ioctl may change things before we re-enter.
arch/arm64/kvm/hyp/switch.c int __hyp_text __kvm_vcpu_run(struct kvm_vcpu *vcpu) · line 357
Quoted 2 lines
arch/arm64/kvm/hyp/switch.c +0
-0exit_code =ARM_EXCEPTION_EL1_SERROR ; }
--
Alex Benn?e