* [PATCH REPOST 0/3] Fix single step for traps
@ 2017-10-03 17:05 Julien Thierry
2017-10-03 17:05 ` [PATCH REPOST 1/3] arm64: Use existing defines for mdscr Julien Thierry
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Julien Thierry @ 2017-10-03 17:05 UTC (permalink / raw)
To: linux-arm-kernel
[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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH REPOST 1/3] arm64: Use existing defines for mdscr
2017-10-03 17:05 [PATCH REPOST 0/3] Fix single step for traps Julien Thierry
@ 2017-10-03 17:05 ` Julien Thierry
2017-10-03 23:28 ` Alex Bennée
2017-10-03 23:28 ` Alex Bennée
2017-10-03 17:05 ` [PATCH REPOST 2/3] arm64: Fix single stepping in kernel traps Julien Thierry
2017-10-03 17:05 ` [PATCH REPOST 3/3] arm64: kvm: Fix single step for guest skipped instructions Julien Thierry
2 siblings, 2 replies; 10+ messages in thread
From: Julien Thierry @ 2017-10-03 17:05 UTC (permalink / raw)
To: linux-arm-kernel
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 d58a625..00a4526 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH REPOST 2/3] arm64: Fix single stepping in kernel traps
2017-10-03 17:05 [PATCH REPOST 0/3] Fix single step for traps Julien Thierry
2017-10-03 17:05 ` [PATCH REPOST 1/3] arm64: Use existing defines for mdscr Julien Thierry
@ 2017-10-03 17:05 ` Julien Thierry
2017-10-03 23:45 ` Alex Bennée
2017-10-03 17:05 ` [PATCH REPOST 3/3] arm64: kvm: Fix single step for guest skipped instructions Julien Thierry
2 siblings, 1 reply; 10+ messages in thread
From: Julien Thierry @ 2017-10-03 17:05 UTC (permalink / raw)
To: linux-arm-kernel
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 d131501..dd7affb 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);
+
static inline int __in_irqentry_text(unsigned long ptr)
{
return ptr >= (unsigned long)&__irqentry_text_start &&
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 cd52d36..2956d5a 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -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;
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 5ea4b85..ed9d856 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH REPOST 3/3] arm64: kvm: Fix single step for guest skipped instructions
2017-10-03 17:05 [PATCH REPOST 0/3] Fix single step for traps Julien Thierry
2017-10-03 17:05 ` [PATCH REPOST 1/3] arm64: Use existing defines for mdscr Julien Thierry
2017-10-03 17:05 ` [PATCH REPOST 2/3] arm64: Fix single stepping in kernel traps Julien Thierry
@ 2017-10-03 17:05 ` Julien Thierry
2017-10-04 0:16 ` Alex Bennée
2 siblings, 1 reply; 10+ messages in thread
From: Julien Thierry @ 2017-10-03 17:05 UTC (permalink / raw)
To: linux-arm-kernel
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: Alex Benn?e <alex.bennee@linaro.org>
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 | 17 ++++++++++++++++-
arch/arm64/kvm/hyp/switch.c | 10 ++++++++++
4 files changed, 30 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 e5df3fc..ee02dd2ee 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..b5fcb96 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -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));
/*
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 945e79c..34fe215 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,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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH REPOST 1/3] arm64: Use existing defines for mdscr
2017-10-03 17:05 ` [PATCH REPOST 1/3] arm64: Use existing defines for mdscr Julien Thierry
@ 2017-10-03 23:28 ` Alex Bennée
2017-10-03 23:28 ` Alex Bennée
1 sibling, 0 replies; 10+ messages in thread
From: Alex Bennée @ 2017-10-03 23:28 UTC (permalink / raw)
To: linux-arm-kernel
Julien Thierry <julien.thierry@arm.com> writes:
> 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>
Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
>
> ---
> 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 d58a625..00a4526 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
--
Alex Benn?e
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH REPOST 1/3] arm64: Use existing defines for mdscr
2017-10-03 17:05 ` [PATCH REPOST 1/3] arm64: Use existing defines for mdscr Julien Thierry
2017-10-03 23:28 ` Alex Bennée
@ 2017-10-03 23:28 ` Alex Bennée
1 sibling, 0 replies; 10+ messages in thread
From: Alex Bennée @ 2017-10-03 23:28 UTC (permalink / raw)
To: linux-arm-kernel
Julien Thierry <julien.thierry@arm.com> writes:
> 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>
Sorry I meant:
Reviewed-by: Alex Benn?e <alex.bennee@linaro.org>
>
> ---
> 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 d58a625..00a4526 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
--
Alex Benn?e
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH REPOST 2/3] arm64: Fix single stepping in kernel traps
2017-10-03 17:05 ` [PATCH REPOST 2/3] arm64: Fix single stepping in kernel traps Julien Thierry
@ 2017-10-03 23:45 ` Alex Bennée
2017-10-04 11:12 ` Julien Thierry
0 siblings, 1 reply; 10+ messages in thread
From: Alex Bennée @ 2017-10-03 23:45 UTC (permalink / raw)
To: linux-arm-kernel
Julien Thierry <julien.thierry@arm.com> writes:
> 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 d131501..dd7affb 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);
> +
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....
> static inline int __in_irqentry_text(unsigned long ptr)
> {
> return ptr >= (unsigned long)&__irqentry_text_start &&
> 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;
> }
Why didn't we use AARCH64_INSN_SIZE for these here?
>
> 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;
> }
I guess we don't have an equivalent AARCH32_T16_INSN_SIZE?
>
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index cd52d36..2956d5a 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -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;
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 5ea4b85..ed9d856 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -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;
> }
It would be nice to find a better name but otherwise:
Reviewed-by: Alex Benn?e <alex.bennee@linaro.org>
--
Alex Benn?e
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH REPOST 3/3] arm64: kvm: Fix single step for guest skipped instructions
2017-10-03 17:05 ` [PATCH REPOST 3/3] arm64: kvm: Fix single step for guest skipped instructions Julien Thierry
@ 2017-10-04 0:16 ` Alex Bennée
0 siblings, 0 replies; 10+ messages in thread
From: Alex Bennée @ 2017-10-04 0:16 UTC (permalink / raw)
To: linux-arm-kernel
Julien Thierry <julien.thierry@arm.com> writes:
> 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: Alex Benn?e <alex.bennee@linaro.org>
> 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 | 17 ++++++++++++++++-
> arch/arm64/kvm/hyp/switch.c | 10 ++++++++++
> 4 files changed, 30 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 e5df3fc..ee02dd2ee 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;
> }
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.
>
> 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..b5fcb96 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -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));
>
> /*
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 945e79c..34fe215 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,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?
>
> 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;
> +
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.
> exit_code = ARM_EXCEPTION_EL1_SERROR;
> }
--
Alex Benn?e
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH REPOST 2/3] arm64: Fix single stepping in kernel traps
2017-10-03 23:45 ` Alex Bennée
@ 2017-10-04 11:12 ` Julien Thierry
2017-10-04 12:01 ` Alex Bennée
0 siblings, 1 reply; 10+ messages in thread
From: Julien Thierry @ 2017-10-04 11:12 UTC (permalink / raw)
To: linux-arm-kernel
On 04/10/17 00:45, Alex Benn?e wrote:
>
> Julien Thierry <julien.thierry@arm.com> writes:
>
>> 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 d131501..dd7affb 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);
>> +
>
> 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....
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?
>
>> static inline int __in_irqentry_text(unsigned long ptr)
>> {
>> return ptr >= (unsigned long)&__irqentry_text_start &&
>> 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;
>> }
>
> Why didn't we use AARCH64_INSN_SIZE for these here?
Because these are AARCH32 instructions, although it is the same size, I
thought it would be misleading to use AARCH64 macro.
>
>>
>> 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;
>> }
>
> 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.
>>
>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> index cd52d36..2956d5a 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -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;
>> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
>> index 5ea4b85..ed9d856 100644
>> --- a/arch/arm64/kernel/traps.c
>> +++ b/arch/arm64/kernel/traps.c
>> @@ -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;
>> }
>
> It would be nice to find a better name but otherwise:
>
> Reviewed-by: Alex Benn?e <alex.bennee@linaro.org>
>
Thanks,
--
Julien Thierry
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH REPOST 2/3] arm64: Fix single stepping in kernel traps
2017-10-04 11:12 ` Julien Thierry
@ 2017-10-04 12:01 ` Alex Bennée
0 siblings, 0 replies; 10+ messages in thread
From: Alex Bennée @ 2017-10-04 12:01 UTC (permalink / raw)
To: linux-arm-kernel
Julien Thierry <julien.thierry@arm.com> writes:
> On 04/10/17 00:45, Alex Benn?e wrote:
>>
>> Julien Thierry <julien.thierry@arm.com> writes:
>>
>>> 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 d131501..dd7affb 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);
>>> +
>>
>> 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....
>
> 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.
>
>>
>>> static inline int __in_irqentry_text(unsigned long ptr)
>>> {
>>> return ptr >= (unsigned long)&__irqentry_text_start &&
>>> 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;
>>> }
>>
>> Why didn't we use AARCH64_INSN_SIZE for these here?
>
> 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...
>
>>
>>>
>>> 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;
>>> }
>>
>> 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.
Maybe AARCH32_T32_INSN_SIZE and AARCH32_T16_INSN_SIZE? Then it is clear
there are two encodings.
>
>>>
>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>>> index cd52d36..2956d5a 100644
>>> --- a/arch/arm64/kernel/cpufeature.c
>>> +++ b/arch/arm64/kernel/cpufeature.c
>>> @@ -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;
>>> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
>>> index 5ea4b85..ed9d856 100644
>>> --- a/arch/arm64/kernel/traps.c
>>> +++ b/arch/arm64/kernel/traps.c
>>> @@ -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;
>>> }
>>
>> It would be nice to find a better name but otherwise:
>>
>> Reviewed-by: Alex Benn?e <alex.bennee@linaro.org>
>>
>
> Thanks,
--
Alex Benn?e
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-10-04 12:01 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-03 17:05 [PATCH REPOST 0/3] Fix single step for traps Julien Thierry
2017-10-03 17:05 ` [PATCH REPOST 1/3] arm64: Use existing defines for mdscr Julien Thierry
2017-10-03 23:28 ` Alex Bennée
2017-10-03 23:28 ` Alex Bennée
2017-10-03 17:05 ` [PATCH REPOST 2/3] arm64: Fix single stepping in kernel traps Julien Thierry
2017-10-03 23:45 ` Alex Bennée
2017-10-04 11:12 ` Julien Thierry
2017-10-04 12:01 ` Alex Bennée
2017-10-03 17:05 ` [PATCH REPOST 3/3] arm64: kvm: Fix single step for guest skipped instructions Julien Thierry
2017-10-04 0:16 ` Alex Bennée
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.