* [PATCH v3 0/2] Fix single step for traps
@ 2017-10-12 13:43 Julien Thierry
2017-10-12 13:43 ` [PATCH v3 1/2] arm64: Use existing defines for mdscr Julien Thierry
2017-10-12 13:43 ` [PATCH v3 2/2] arm64: Fix single stepping in kernel traps Julien Thierry
0 siblings, 2 replies; 8+ messages in thread
From: Julien Thierry @ 2017-10-12 13:43 UTC (permalink / raw)
To: linux-arm-kernel
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.
First patch is just to avoid raw values when using single stepping
registers/bits.
Patches 2 fixes the issue for the kernel.
Changes since v1:
* Rename arm64_skip_trapped_instr to arm64_setup_next_instr
* Add defines for AARCH32 and Thumb instruction sizes
* Drop previous KVM patch, Alex Benn?e has a better approach
Changes since v2:
* Move include in assembler.h to the right place
* Use user_fastforward_single_step to update single step state
Cheers,
Julien Thierry
Julien Thierry (2):
arm64: Use existing defines for mdscr
arm64: Fix single stepping in kernel traps
arch/arm64/include/asm/assembler.h | 5 +++--
arch/arm64/include/asm/insn.h | 5 +++++
arch/arm64/include/asm/traps.h | 6 ++++++
arch/arm64/kernel/armv8_deprecated.c | 8 ++++----
arch/arm64/kernel/cpufeature.c | 2 +-
arch/arm64/kernel/traps.c | 21 ++++++++++++++++-----
6 files changed, 35 insertions(+), 12 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 1/2] arm64: Use existing defines for mdscr
2017-10-12 13:43 [PATCH v3 0/2] Fix single step for traps Julien Thierry
@ 2017-10-12 13:43 ` Julien Thierry
2017-10-24 14:28 ` Will Deacon
2017-10-12 13:43 ` [PATCH v3 2/2] arm64: Fix single stepping in kernel traps Julien Thierry
1 sibling, 1 reply; 8+ messages in thread
From: Julien Thierry @ 2017-10-12 13:43 UTC (permalink / raw)
To: linux-arm-kernel
Signed-off-by: Julien Thierry <julien.thierry@arm.com>
Reviewed-by: Alex Benn?e <alex.bennee@linaro.org>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@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..3128a9c 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -25,6 +25,7 @@
#include <asm/asm-offsets.h>
#include <asm/cpufeature.h>
+#include <asm/debug-monitors.h>
#include <asm/mmu_context.h>
#include <asm/page.h>
#include <asm/pgtable-hwdef.h>
@@ -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] 8+ messages in thread
* [PATCH v3 2/2] arm64: Fix single stepping in kernel traps
2017-10-12 13:43 [PATCH v3 0/2] Fix single step for traps Julien Thierry
2017-10-12 13:43 ` [PATCH v3 1/2] arm64: Use existing defines for mdscr Julien Thierry
@ 2017-10-12 13:43 ` Julien Thierry
2017-10-24 14:27 ` Will Deacon
1 sibling, 1 reply; 8+ messages in thread
From: Julien Thierry @ 2017-10-12 13:43 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>
Reviewed-by: Alex Benn?e <alex.bennee@linaro.org>
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/insn.h | 5 +++++
arch/arm64/include/asm/traps.h | 6 ++++++
arch/arm64/kernel/armv8_deprecated.c | 8 ++++----
arch/arm64/kernel/cpufeature.c | 2 +-
arch/arm64/kernel/traps.c | 21 ++++++++++++++++-----
5 files changed, 32 insertions(+), 10 deletions(-)
diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index 4214c38..de5e31a 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -22,6 +22,11 @@
/* A64 instructions are always 32 bits. */
#define AARCH64_INSN_SIZE 4
+#define AARCH32_INSN_SIZE 4
+
+/* Thumb/Thumb2 instruction sizes */
+#define AARCH32_T32_INSN_SIZE 4
+#define AARCH32_T16_INSN_SIZE 2
#ifndef __ASSEMBLY__
/*
diff --git a/arch/arm64/include/asm/traps.h b/arch/arm64/include/asm/traps.h
index d131501..67b971e 100644
--- a/arch/arm64/include/asm/traps.h
+++ b/arch/arm64/include/asm/traps.h
@@ -37,6 +37,12 @@ struct undef_hook {
void arm64_notify_segfault(struct pt_regs *regs, unsigned long addr);
+/*
+ * Move regs->pc to next instruction and do necessary setup before it
+ * is executed.
+ */
+void arm64_setup_next_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 d06fbe4..2808e56 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_setup_next_instr(regs, AARCH32_INSN_SIZE);
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_setup_next_instr(regs, AARCH32_INSN_SIZE);
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_setup_next_instr(regs, AARCH32_INSN_SIZE);
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_setup_next_instr(regs, AARCH32_T16_INSN_SIZE);
return rc;
}
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 21e2c95..235834e 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_setup_next_instr(regs, AARCH64_INSN_SIZE);
}
return rc;
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 5ea4b85..f93b33f 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_setup_next_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 trap.
+ */
+ user_fastforward_single_step(current);
+}
+
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_setup_next_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_setup_next_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_setup_next_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_setup_next_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_setup_next_instr(regs, AARCH64_INSN_SIZE);
return DBG_HOOK_HANDLED;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v3 2/2] arm64: Fix single stepping in kernel traps
2017-10-12 13:43 ` [PATCH v3 2/2] arm64: Fix single stepping in kernel traps Julien Thierry
@ 2017-10-24 14:27 ` Will Deacon
2017-10-24 14:38 ` Julien Thierry
0 siblings, 1 reply; 8+ messages in thread
From: Will Deacon @ 2017-10-24 14:27 UTC (permalink / raw)
To: linux-arm-kernel
Hi Julien,
On Thu, Oct 12, 2017 at 02:43:03PM +0100, Julien Thierry wrote:
> 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.
Curious, but how did you spot this?
> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
> Reviewed-by: Alex Benn?e <alex.bennee@linaro.org>
> 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/insn.h | 5 +++++
> arch/arm64/include/asm/traps.h | 6 ++++++
> arch/arm64/kernel/armv8_deprecated.c | 8 ++++----
> arch/arm64/kernel/cpufeature.c | 2 +-
> arch/arm64/kernel/traps.c | 21 ++++++++++++++++-----
> 5 files changed, 32 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
> index 4214c38..de5e31a 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -22,6 +22,11 @@
>
> /* A64 instructions are always 32 bits. */
> #define AARCH64_INSN_SIZE 4
> +#define AARCH32_INSN_SIZE 4
> +
> +/* Thumb/Thumb2 instruction sizes */
> +#define AARCH32_T32_INSN_SIZE 4
> +#define AARCH32_T16_INSN_SIZE 2
The naming here is a bit misleading, since T32 is an instruction set
consisting of both 32-bit and 16-bit instructions. For now, you might just
be better off having the caller pass in an immediate directly rather than
add these confusing #defines.
> #ifndef __ASSEMBLY__
> /*
> diff --git a/arch/arm64/include/asm/traps.h b/arch/arm64/include/asm/traps.h
> index d131501..67b971e 100644
> --- a/arch/arm64/include/asm/traps.h
> +++ b/arch/arm64/include/asm/traps.h
> @@ -37,6 +37,12 @@ struct undef_hook {
>
> void arm64_notify_segfault(struct pt_regs *regs, unsigned long addr);
>
> +/*
> + * Move regs->pc to next instruction and do necessary setup before it
> + * is executed.
> + */
> +void arm64_setup_next_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 d06fbe4..2808e56 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_setup_next_instr(regs, AARCH32_INSN_SIZE);
> 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_setup_next_instr(regs, AARCH32_INSN_SIZE);
> 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_setup_next_instr(regs, AARCH32_INSN_SIZE);
> 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_setup_next_instr(regs, AARCH32_T16_INSN_SIZE);
> return rc;
> }
>
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 21e2c95..235834e 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_setup_next_instr(regs, AARCH64_INSN_SIZE);
> }
>
> return rc;
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 5ea4b85..f93b33f 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_setup_next_instr(struct pt_regs *regs, unsigned long size)
> +{
This strikes me as a pretty broadly named function to be exposing like this.
How about: arm64_skip_faulting_instruction instead?
Will
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 1/2] arm64: Use existing defines for mdscr
2017-10-12 13:43 ` [PATCH v3 1/2] arm64: Use existing defines for mdscr Julien Thierry
@ 2017-10-24 14:28 ` Will Deacon
2017-10-24 14:38 ` Julien Thierry
0 siblings, 1 reply; 8+ messages in thread
From: Will Deacon @ 2017-10-24 14:28 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Oct 12, 2017 at 02:43:02PM +0100, Julien Thierry wrote:
> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
> Reviewed-by: Alex Benn?e <alex.bennee@linaro.org>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
> arch/arm64/include/asm/assembler.h | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
If you write a commit message, then I'll merge this.
Will
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 2/2] arm64: Fix single stepping in kernel traps
2017-10-24 14:27 ` Will Deacon
@ 2017-10-24 14:38 ` Julien Thierry
2017-10-24 15:29 ` Alex Bennée
0 siblings, 1 reply; 8+ messages in thread
From: Julien Thierry @ 2017-10-24 14:38 UTC (permalink / raw)
To: linux-arm-kernel
Hi Will,
On 24/10/17 15:27, Will Deacon wrote:
> Hi Julien,
>
> On Thu, Oct 12, 2017 at 02:43:03PM +0100, Julien Thierry wrote:
>> 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.
>
> Curious, but how did you spot this?
>
It was discovered after investigating the issue of single stepping on
GIC distributor registers accesses from KVM guests. Once the cause of
the issue was identified as being traps/faults from KVM, it was
speculated we might have similar issues on EL1 traps which turned out to
be the case.
>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>> Reviewed-by: Alex Benn?e <alex.bennee@linaro.org>
>> 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/insn.h | 5 +++++
>> arch/arm64/include/asm/traps.h | 6 ++++++
>> arch/arm64/kernel/armv8_deprecated.c | 8 ++++----
>> arch/arm64/kernel/cpufeature.c | 2 +-
>> arch/arm64/kernel/traps.c | 21 ++++++++++++++++-----
>> 5 files changed, 32 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
>> index 4214c38..de5e31a 100644
>> --- a/arch/arm64/include/asm/insn.h
>> +++ b/arch/arm64/include/asm/insn.h
>> @@ -22,6 +22,11 @@
>>
>> /* A64 instructions are always 32 bits. */
>> #define AARCH64_INSN_SIZE 4
>> +#define AARCH32_INSN_SIZE 4
>> +
>> +/* Thumb/Thumb2 instruction sizes */
>> +#define AARCH32_T32_INSN_SIZE 4
>> +#define AARCH32_T16_INSN_SIZE 2
>
> The naming here is a bit misleading, since T32 is an instruction set
> consisting of both 32-bit and 16-bit instructions. For now, you might just
> be better off having the caller pass in an immediate directly rather than
> add these confusing #defines.
>
I see. I guess I'll remove them.
>> #ifndef __ASSEMBLY__
>> /*
>> diff --git a/arch/arm64/include/asm/traps.h b/arch/arm64/include/asm/traps.h
>> index d131501..67b971e 100644
>> --- a/arch/arm64/include/asm/traps.h
>> +++ b/arch/arm64/include/asm/traps.h
>> @@ -37,6 +37,12 @@ struct undef_hook {
>>
>> void arm64_notify_segfault(struct pt_regs *regs, unsigned long addr);
>>
>> +/*
>> + * Move regs->pc to next instruction and do necessary setup before it
>> + * is executed.
>> + */
>> +void arm64_setup_next_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 d06fbe4..2808e56 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_setup_next_instr(regs, AARCH32_INSN_SIZE);
>> 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_setup_next_instr(regs, AARCH32_INSN_SIZE);
>> 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_setup_next_instr(regs, AARCH32_INSN_SIZE);
>> 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_setup_next_instr(regs, AARCH32_T16_INSN_SIZE);
>> return rc;
>> }
>>
>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> index 21e2c95..235834e 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_setup_next_instr(regs, AARCH64_INSN_SIZE);
>> }
>>
>> return rc;
>> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
>> index 5ea4b85..f93b33f 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_setup_next_instr(struct pt_regs *regs, unsigned long size)
>> +{
>
> This strikes me as a pretty broadly named function to be exposing like this.
> How about: arm64_skip_faulting_instruction instead?
>
Hmmm, I think Alex was not fond of that kind of naming because we are
not just skipping. Although here we specify it is for faulting instruction.
Your suggestion suits, so if everyone agrees on it I'll change it.
Thanks,
--
Julien Thierry
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 1/2] arm64: Use existing defines for mdscr
2017-10-24 14:28 ` Will Deacon
@ 2017-10-24 14:38 ` Julien Thierry
0 siblings, 0 replies; 8+ messages in thread
From: Julien Thierry @ 2017-10-24 14:38 UTC (permalink / raw)
To: linux-arm-kernel
On 24/10/17 15:28, Will Deacon wrote:
> On Thu, Oct 12, 2017 at 02:43:02PM +0100, Julien Thierry wrote:
>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>> Reviewed-by: Alex Benn?e <alex.bennee@linaro.org>
>> Acked-by: Mark Rutland <mark.rutland@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> ---
>> arch/arm64/include/asm/assembler.h | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> If you write a commit message, then I'll merge this.
Ok, I'll do that once I have a definitive version for the second patch.
Thanks,
--
Julien Thierry
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 2/2] arm64: Fix single stepping in kernel traps
2017-10-24 14:38 ` Julien Thierry
@ 2017-10-24 15:29 ` Alex Bennée
0 siblings, 0 replies; 8+ messages in thread
From: Alex Bennée @ 2017-10-24 15:29 UTC (permalink / raw)
To: linux-arm-kernel
Julien Thierry <julien.thierry@arm.com> writes:
> Hi Will,
>
> On 24/10/17 15:27, Will Deacon wrote:
>> Hi Julien,
>>
>> On Thu, Oct 12, 2017 at 02:43:03PM +0100, Julien Thierry wrote:
>>> 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.
>>
>> Curious, but how did you spot this?
>>
>
> It was discovered after investigating the issue of single stepping on
> GIC distributor registers accesses from KVM guests. Once the cause of
> the issue was identified as being traps/faults from KVM, it was
> speculated we might have similar issues on EL1 traps which turned out
> to be the case.
>
>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>>> Reviewed-by: Alex Benn?e <alex.bennee@linaro.org>
>>> 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/insn.h | 5 +++++
>>> arch/arm64/include/asm/traps.h | 6 ++++++
>>> arch/arm64/kernel/armv8_deprecated.c | 8 ++++----
>>> arch/arm64/kernel/cpufeature.c | 2 +-
>>> arch/arm64/kernel/traps.c | 21 ++++++++++++++++-----
>>> 5 files changed, 32 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
>>> index 4214c38..de5e31a 100644
>>> --- a/arch/arm64/include/asm/insn.h
>>> +++ b/arch/arm64/include/asm/insn.h
>>> @@ -22,6 +22,11 @@
>>>
>>> /* A64 instructions are always 32 bits. */
>>> #define AARCH64_INSN_SIZE 4
>>> +#define AARCH32_INSN_SIZE 4
>>> +
>>> +/* Thumb/Thumb2 instruction sizes */
>>> +#define AARCH32_T32_INSN_SIZE 4
>>> +#define AARCH32_T16_INSN_SIZE 2
>>
>> The naming here is a bit misleading, since T32 is an instruction set
>> consisting of both 32-bit and 16-bit instructions. For now, you might just
>> be better off having the caller pass in an immediate directly rather than
>> add these confusing #defines.
>>
>
> I see. I guess I'll remove them.
>
>>> #ifndef __ASSEMBLY__
>>> /*
>>> diff --git a/arch/arm64/include/asm/traps.h b/arch/arm64/include/asm/traps.h
>>> index d131501..67b971e 100644
>>> --- a/arch/arm64/include/asm/traps.h
>>> +++ b/arch/arm64/include/asm/traps.h
>>> @@ -37,6 +37,12 @@ struct undef_hook {
>>>
>>> void arm64_notify_segfault(struct pt_regs *regs, unsigned long addr);
>>>
>>> +/*
>>> + * Move regs->pc to next instruction and do necessary setup before it
>>> + * is executed.
>>> + */
>>> +void arm64_setup_next_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 d06fbe4..2808e56 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_setup_next_instr(regs, AARCH32_INSN_SIZE);
>>> 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_setup_next_instr(regs, AARCH32_INSN_SIZE);
>>> 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_setup_next_instr(regs, AARCH32_INSN_SIZE);
>>> 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_setup_next_instr(regs, AARCH32_T16_INSN_SIZE);
>>> return rc;
>>> }
>>>
>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>>> index 21e2c95..235834e 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_setup_next_instr(regs, AARCH64_INSN_SIZE);
>>> }
>>>
>>> return rc;
>>> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
>>> index 5ea4b85..f93b33f 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_setup_next_instr(struct pt_regs *regs, unsigned long size)
>>> +{
>>
>> This strikes me as a pretty broadly named function to be exposing like this.
>> How about: arm64_skip_faulting_instruction instead?
>>
>
> Hmmm, I think Alex was not fond of that kind of naming because we are
> not just skipping. Although here we specify it is for faulting
> instruction.
>
> Your suggestion suits, so if everyone agrees on it I'll change it.
I'm happy to defer to Will on this one ;-)
--
Alex Benn?e
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-10-24 15:29 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-12 13:43 [PATCH v3 0/2] Fix single step for traps Julien Thierry
2017-10-12 13:43 ` [PATCH v3 1/2] arm64: Use existing defines for mdscr Julien Thierry
2017-10-24 14:28 ` Will Deacon
2017-10-24 14:38 ` Julien Thierry
2017-10-12 13:43 ` [PATCH v3 2/2] arm64: Fix single stepping in kernel traps Julien Thierry
2017-10-24 14:27 ` Will Deacon
2017-10-24 14:38 ` Julien Thierry
2017-10-24 15:29 ` 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.