| Message ID | 1570733080-21015-10-git-send-email-Dave.Martin@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: ARMv8.5-A: Branch Target Identification support | expand |
On Thu, Oct 10, 2019 at 07:44:37PM +0100, Dave Martin wrote: > Correct skipping of an instruction on AArch32 works a bit > differently from AArch64, mainly due to the different CPSR/PSTATE > semantics. > > There have been various attempts to get this right. Currenty > arm64_skip_faulting_instruction() mostly does the right thing, but > does not advance the IT state machine for the AArch32 case. > > arm64_compat_skip_faulting_instruction() handles the IT state > machine but is local to traps.c, and porting other code to use it > will make a mess since there are some call sites that apply for > both the compat and native cases. > > Since manual instruction skipping implies a trap, it's a relatively > slow path. > > So, make arm64_skip_faulting_instruction() handle both compat and > native, and get rid of the arm64_compat_skip_faulting_instruction() > special case. > > Fixes: 32a3e635fb0e ("arm64: compat: Add CNTFRQ trap handler") > Fixes: 1f1c014035a8 ("arm64: compat: Add condition code checks and IT advance") > Fixes: 6436beeee572 ("arm64: Fix single stepping in kernel traps") > Fixes: bd35a4adc413 ("arm64: Port SWP/SWPB emulation support from arm") > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > --- > arch/arm64/kernel/traps.c | 18 ++++++++---------- > 1 file changed, 8 insertions(+), 10 deletions(-) This looks good to me; it's certainly easier to reason about. I couldn't spot a place where we do the wrong thing today, given AFAICT all the instances in arch/arm64/kernel/armv8_deprecated.c would be UNPREDICTABLE within an IT block. It might be worth calling out an example in the commit message to justify the fixes tags. Thanks, Mark. > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c > index 15e3c4f..44c91d4 100644 > --- a/arch/arm64/kernel/traps.c > +++ b/arch/arm64/kernel/traps.c > @@ -268,6 +268,8 @@ void arm64_notify_die(const char *str, struct pt_regs *regs, > } > } > > +static void advance_itstate(struct pt_regs *regs); > + > void arm64_skip_faulting_instruction(struct pt_regs *regs, unsigned long size) > { > regs->pc += size; > @@ -278,6 +280,9 @@ void arm64_skip_faulting_instruction(struct pt_regs *regs, unsigned long size) > */ > if (user_mode(regs)) > user_fastforward_single_step(current); > + > + if (regs->pstate & PSR_MODE32_BIT) > + advance_itstate(regs); > } > > static LIST_HEAD(undef_hook); > @@ -629,19 +634,12 @@ static void advance_itstate(struct pt_regs *regs) > compat_set_it_state(regs, it); > } > > -static void arm64_compat_skip_faulting_instruction(struct pt_regs *regs, > - unsigned int sz) > -{ > - advance_itstate(regs); > - arm64_skip_faulting_instruction(regs, sz); > -} > - > static void compat_cntfrq_read_handler(unsigned int esr, struct pt_regs *regs) > { > int reg = (esr & ESR_ELx_CP15_32_ISS_RT_MASK) >> ESR_ELx_CP15_32_ISS_RT_SHIFT; > > pt_regs_write_reg(regs, reg, arch_timer_get_rate()); > - arm64_compat_skip_faulting_instruction(regs, 4); > + arm64_skip_faulting_instruction(regs, 4); > } > > static const struct sys64_hook cp15_32_hooks[] = { > @@ -661,7 +659,7 @@ static void compat_cntvct_read_handler(unsigned int esr, struct pt_regs *regs) > > pt_regs_write_reg(regs, rt, lower_32_bits(val)); > pt_regs_write_reg(regs, rt2, upper_32_bits(val)); > - arm64_compat_skip_faulting_instruction(regs, 4); > + arm64_skip_faulting_instruction(regs, 4); > } > > static const struct sys64_hook cp15_64_hooks[] = { > @@ -682,7 +680,7 @@ asmlinkage void __exception do_cp15instr(unsigned int esr, struct pt_regs *regs) > * There is no T16 variant of a CP access, so we > * always advance PC by 4 bytes. > */ > - arm64_compat_skip_faulting_instruction(regs, 4); > + arm64_skip_faulting_instruction(regs, 4); > return; > } > > -- > 2.1.4 >
On Fri, Oct 11, 2019 at 04:24:53PM +0100, Mark Rutland wrote: > On Thu, Oct 10, 2019 at 07:44:37PM +0100, Dave Martin wrote: > > Correct skipping of an instruction on AArch32 works a bit > > differently from AArch64, mainly due to the different CPSR/PSTATE > > semantics. > > > > There have been various attempts to get this right. Currenty > > arm64_skip_faulting_instruction() mostly does the right thing, but > > does not advance the IT state machine for the AArch32 case. > > > > arm64_compat_skip_faulting_instruction() handles the IT state > > machine but is local to traps.c, and porting other code to use it > > will make a mess since there are some call sites that apply for > > both the compat and native cases. > > > > Since manual instruction skipping implies a trap, it's a relatively > > slow path. > > > > So, make arm64_skip_faulting_instruction() handle both compat and > > native, and get rid of the arm64_compat_skip_faulting_instruction() > > special case. > > > > Fixes: 32a3e635fb0e ("arm64: compat: Add CNTFRQ trap handler") > > Fixes: 1f1c014035a8 ("arm64: compat: Add condition code checks and IT advance") > > Fixes: 6436beeee572 ("arm64: Fix single stepping in kernel traps") > > Fixes: bd35a4adc413 ("arm64: Port SWP/SWPB emulation support from arm") > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > --- > > arch/arm64/kernel/traps.c | 18 ++++++++---------- > > 1 file changed, 8 insertions(+), 10 deletions(-) > > This looks good to me; it's certainly easier to reason about. > > I couldn't spot a place where we do the wrong thing today, given AFAICT > all the instances in arch/arm64/kernel/armv8_deprecated.c would be > UNPREDICTABLE within an IT block. > > It might be worth calling out an example in the commit message to > justify the fixes tags. IIRC I found no bug here; rather we have pointlessly fragmented code, so I followed the "if fixing the same bug in multiple places, merge those places so you need only fix it in one place next time" rule. Since arm64_skip_faulting_instruction() is most of the way to being generically usable anyway, this series merges all the special-case handling into it. I could add something like --8<-- This allows this fiddly operation to be maintained in a single place rather than trying to maintain fragmented versions spread around arch/arm64. -->8-- Any good? Cheers ---Dave [...] > > > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c > > index 15e3c4f..44c91d4 100644 > > --- a/arch/arm64/kernel/traps.c > > +++ b/arch/arm64/kernel/traps.c > > @@ -268,6 +268,8 @@ void arm64_notify_die(const char *str, struct pt_regs *regs, > > } > > } > > > > +static void advance_itstate(struct pt_regs *regs); > > + > > void arm64_skip_faulting_instruction(struct pt_regs *regs, unsigned long size) > > { > > regs->pc += size; > > @@ -278,6 +280,9 @@ void arm64_skip_faulting_instruction(struct pt_regs *regs, unsigned long size) > > */ > > if (user_mode(regs)) > > user_fastforward_single_step(current); > > + > > + if (regs->pstate & PSR_MODE32_BIT) > > + advance_itstate(regs); > > } > > > > static LIST_HEAD(undef_hook); > > @@ -629,19 +634,12 @@ static void advance_itstate(struct pt_regs *regs) > > compat_set_it_state(regs, it); > > } > > > > -static void arm64_compat_skip_faulting_instruction(struct pt_regs *regs, > > - unsigned int sz) > > -{ > > - advance_itstate(regs); > > - arm64_skip_faulting_instruction(regs, sz); > > -} > > - > > static void compat_cntfrq_read_handler(unsigned int esr, struct pt_regs *regs) > > { > > int reg = (esr & ESR_ELx_CP15_32_ISS_RT_MASK) >> ESR_ELx_CP15_32_ISS_RT_SHIFT; > > > > pt_regs_write_reg(regs, reg, arch_timer_get_rate()); > > - arm64_compat_skip_faulting_instruction(regs, 4); > > + arm64_skip_faulting_instruction(regs, 4); > > } > > > > static const struct sys64_hook cp15_32_hooks[] = { > > @@ -661,7 +659,7 @@ static void compat_cntvct_read_handler(unsigned int esr, struct pt_regs *regs) > > > > pt_regs_write_reg(regs, rt, lower_32_bits(val)); > > pt_regs_write_reg(regs, rt2, upper_32_bits(val)); > > - arm64_compat_skip_faulting_instruction(regs, 4); > > + arm64_skip_faulting_instruction(regs, 4); > > } > > > > static const struct sys64_hook cp15_64_hooks[] = { > > @@ -682,7 +680,7 @@ asmlinkage void __exception do_cp15instr(unsigned int esr, struct pt_regs *regs) > > * There is no T16 variant of a CP access, so we > > * always advance PC by 4 bytes. > > */ > > - arm64_compat_skip_faulting_instruction(regs, 4); > > + arm64_skip_faulting_instruction(regs, 4); > > return; > > } > > > > -- > > 2.1.4 > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Oct 15, 2019 at 04:21:09PM +0100, Dave Martin wrote: > On Fri, Oct 11, 2019 at 04:24:53PM +0100, Mark Rutland wrote: > > On Thu, Oct 10, 2019 at 07:44:37PM +0100, Dave Martin wrote: > > > Correct skipping of an instruction on AArch32 works a bit > > > differently from AArch64, mainly due to the different CPSR/PSTATE > > > semantics. > > > > > > There have been various attempts to get this right. Currenty > > > arm64_skip_faulting_instruction() mostly does the right thing, but > > > does not advance the IT state machine for the AArch32 case. > > > > > > arm64_compat_skip_faulting_instruction() handles the IT state > > > machine but is local to traps.c, and porting other code to use it > > > will make a mess since there are some call sites that apply for > > > both the compat and native cases. > > > > > > Since manual instruction skipping implies a trap, it's a relatively > > > slow path. > > > > > > So, make arm64_skip_faulting_instruction() handle both compat and > > > native, and get rid of the arm64_compat_skip_faulting_instruction() > > > special case. > > > > > > Fixes: 32a3e635fb0e ("arm64: compat: Add CNTFRQ trap handler") > > > Fixes: 1f1c014035a8 ("arm64: compat: Add condition code checks and IT advance") > > > Fixes: 6436beeee572 ("arm64: Fix single stepping in kernel traps") > > > Fixes: bd35a4adc413 ("arm64: Port SWP/SWPB emulation support from arm") > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > > --- > > > arch/arm64/kernel/traps.c | 18 ++++++++---------- > > > 1 file changed, 8 insertions(+), 10 deletions(-) > > > > This looks good to me; it's certainly easier to reason about. > > > > I couldn't spot a place where we do the wrong thing today, given AFAICT > > all the instances in arch/arm64/kernel/armv8_deprecated.c would be > > UNPREDICTABLE within an IT block. > > > > It might be worth calling out an example in the commit message to > > justify the fixes tags. > > IIRC I found no bug here; rather we have pointlessly fragmented code, > so I followed the "if fixing the same bug in multiple places, merge > those places so you need only fix it in one place next time" rule. Sure thing, that makes sense to me. > Since arm64_skip_faulting_instruction() is most of the way to being > generically usable anyway, this series merges all the special-case > handling into it. > > I could add something like > > --8<-- > > This allows this fiddly operation to be maintained in a single > place rather than trying to maintain fragmented versions spread > around arch/arm64. > > -->8-- > > Any good? My big concern is that the commit message reads as a fix, implying that there's an existing correctness bug. I think that simplifying it to make it clearer that it's a cleanup/improvement would be preferable. How about: | arm64: unify native/compat instruction skipping | | Skipping of an instruction on AArch32 works a bit differently from | AArch64, mainly due to the different CPSR/PSTATE semantics. | | Currently arm64_skip_faulting_instruction() is only suitable for | AArch64, and arm64_compat_skip_faulting_instruction() handles the IT | state machine but is local to traps.c. | | Since manual instruction skipping implies a trap, it's a relatively | slow path. | | So, make arm64_skip_faulting_instruction() handle both compat and | native, and get rid of the arm64_compat_skip_faulting_instruction() | special case. | | Signed-off-by: Dave Martin <Dave.Martin@arm.com> With that, feel free to add: Reviewed-by: Mark Rutland <mark.rutland@arm.com> We could even point out that the armv8_deprecated cases are UNPREDICTABLE in an IT block, and correctly emulated either way. Thanks, Mark.
On Tue, Oct 15, 2019 at 05:42:04PM +0100, Mark Rutland wrote: > On Tue, Oct 15, 2019 at 04:21:09PM +0100, Dave Martin wrote: > > On Fri, Oct 11, 2019 at 04:24:53PM +0100, Mark Rutland wrote: > > > On Thu, Oct 10, 2019 at 07:44:37PM +0100, Dave Martin wrote: > > > > Correct skipping of an instruction on AArch32 works a bit > > > > differently from AArch64, mainly due to the different CPSR/PSTATE > > > > semantics. > > > > > > > > There have been various attempts to get this right. Currenty > > > > arm64_skip_faulting_instruction() mostly does the right thing, but > > > > does not advance the IT state machine for the AArch32 case. > > > > > > > > arm64_compat_skip_faulting_instruction() handles the IT state > > > > machine but is local to traps.c, and porting other code to use it > > > > will make a mess since there are some call sites that apply for > > > > both the compat and native cases. > > > > > > > > Since manual instruction skipping implies a trap, it's a relatively > > > > slow path. > > > > > > > > So, make arm64_skip_faulting_instruction() handle both compat and > > > > native, and get rid of the arm64_compat_skip_faulting_instruction() > > > > special case. > > > > > > > > Fixes: 32a3e635fb0e ("arm64: compat: Add CNTFRQ trap handler") > > > > Fixes: 1f1c014035a8 ("arm64: compat: Add condition code checks and IT advance") > > > > Fixes: 6436beeee572 ("arm64: Fix single stepping in kernel traps") > > > > Fixes: bd35a4adc413 ("arm64: Port SWP/SWPB emulation support from arm") > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > > > --- > > > > arch/arm64/kernel/traps.c | 18 ++++++++---------- > > > > 1 file changed, 8 insertions(+), 10 deletions(-) > > > > > > This looks good to me; it's certainly easier to reason about. > > > > > > I couldn't spot a place where we do the wrong thing today, given AFAICT > > > all the instances in arch/arm64/kernel/armv8_deprecated.c would be > > > UNPREDICTABLE within an IT block. > > > > > > It might be worth calling out an example in the commit message to > > > justify the fixes tags. > > > > IIRC I found no bug here; rather we have pointlessly fragmented code, > > so I followed the "if fixing the same bug in multiple places, merge > > those places so you need only fix it in one place next time" rule. > > Sure thing, that makes sense to me. > > > Since arm64_skip_faulting_instruction() is most of the way to being > > generically usable anyway, this series merges all the special-case > > handling into it. > > > > I could add something like > > > > --8<-- > > > > This allows this fiddly operation to be maintained in a single > > place rather than trying to maintain fragmented versions spread > > around arch/arm64. > > > > -->8-- > > > > Any good? > > My big concern is that the commit message reads as a fix, implying that > there's an existing correctness bug. I think that simplifying it to make > it clearer that it's a cleanup/improvement would be preferable. > > How about: > > | arm64: unify native/compat instruction skipping > | > | Skipping of an instruction on AArch32 works a bit differently from > | AArch64, mainly due to the different CPSR/PSTATE semantics. > | > | Currently arm64_skip_faulting_instruction() is only suitable for > | AArch64, and arm64_compat_skip_faulting_instruction() handles the IT > | state machine but is local to traps.c. > | > | Since manual instruction skipping implies a trap, it's a relatively > | slow path. > | > | So, make arm64_skip_faulting_instruction() handle both compat and > | native, and get rid of the arm64_compat_skip_faulting_instruction() > | special case. > | > | Signed-off-by: Dave Martin <Dave.Martin@arm.com> And drop the Fixes tags. Yes, I think that's reasonable. I think I was probably glossing over the fact that we don't need to get the ITSTATE machine advance correct for the compat insn emulation; as you say, I can't see what else this patch "fixes". > With that, feel free to add: > > Reviewed-by: Mark Rutland <mark.rutland@arm.com> Thanks! > We could even point out that the armv8_deprecated cases are > UNPREDICTABLE in an IT block, and correctly emulated either way. Yes, I'll add something along those lines. Cheers ---Dave
On Tue, Oct 15, 2019 at 05:49:05PM +0100, Dave Martin wrote: > On Tue, Oct 15, 2019 at 05:42:04PM +0100, Mark Rutland wrote: > > On Tue, Oct 15, 2019 at 04:21:09PM +0100, Dave Martin wrote: > > > On Fri, Oct 11, 2019 at 04:24:53PM +0100, Mark Rutland wrote: > > > > On Thu, Oct 10, 2019 at 07:44:37PM +0100, Dave Martin wrote: > > > > > Correct skipping of an instruction on AArch32 works a bit > > > > > differently from AArch64, mainly due to the different CPSR/PSTATE > > > > > semantics. > > > > > > > > > > There have been various attempts to get this right. Currenty > > > > > arm64_skip_faulting_instruction() mostly does the right thing, but > > > > > does not advance the IT state machine for the AArch32 case. > > > > > > > > > > arm64_compat_skip_faulting_instruction() handles the IT state > > > > > machine but is local to traps.c, and porting other code to use it > > > > > will make a mess since there are some call sites that apply for > > > > > both the compat and native cases. > > > > > > > > > > Since manual instruction skipping implies a trap, it's a relatively > > > > > slow path. > > > > > > > > > > So, make arm64_skip_faulting_instruction() handle both compat and > > > > > native, and get rid of the arm64_compat_skip_faulting_instruction() > > > > > special case. > > > > > > > > > > Fixes: 32a3e635fb0e ("arm64: compat: Add CNTFRQ trap handler") > > > > > Fixes: 1f1c014035a8 ("arm64: compat: Add condition code checks and IT advance") > > > > > Fixes: 6436beeee572 ("arm64: Fix single stepping in kernel traps") > > > > > Fixes: bd35a4adc413 ("arm64: Port SWP/SWPB emulation support from arm") > > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > > > > --- > > > > > arch/arm64/kernel/traps.c | 18 ++++++++---------- > > > > > 1 file changed, 8 insertions(+), 10 deletions(-) > > > > > > > > This looks good to me; it's certainly easier to reason about. > > > > > > > > I couldn't spot a place where we do the wrong thing today, given AFAICT > > > > all the instances in arch/arm64/kernel/armv8_deprecated.c would be > > > > UNPREDICTABLE within an IT block. > > > > > > > > It might be worth calling out an example in the commit message to > > > > justify the fixes tags. > > > > > > IIRC I found no bug here; rather we have pointlessly fragmented code, > > > so I followed the "if fixing the same bug in multiple places, merge > > > those places so you need only fix it in one place next time" rule. > > > > Sure thing, that makes sense to me. > > > > > Since arm64_skip_faulting_instruction() is most of the way to being > > > generically usable anyway, this series merges all the special-case > > > handling into it. > > > > > > I could add something like > > > > > > --8<-- > > > > > > This allows this fiddly operation to be maintained in a single > > > place rather than trying to maintain fragmented versions spread > > > around arch/arm64. > > > > > > -->8-- > > > > > > Any good? > > > > My big concern is that the commit message reads as a fix, implying that > > there's an existing correctness bug. I think that simplifying it to make > > it clearer that it's a cleanup/improvement would be preferable. > > > > How about: > > > > | arm64: unify native/compat instruction skipping > > | > > | Skipping of an instruction on AArch32 works a bit differently from > > | AArch64, mainly due to the different CPSR/PSTATE semantics. > > | > > | Currently arm64_skip_faulting_instruction() is only suitable for > > | AArch64, and arm64_compat_skip_faulting_instruction() handles the IT > > | state machine but is local to traps.c. > > | > > | Since manual instruction skipping implies a trap, it's a relatively > > | slow path. > > | > > | So, make arm64_skip_faulting_instruction() handle both compat and > > | native, and get rid of the arm64_compat_skip_faulting_instruction() > > | special case. > > | > > | Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > And drop the Fixes tags. Yes, I think that's reasonable. > > I think I was probably glossing over the fact that we don't need to get > the ITSTATE machine advance correct for the compat insn emulation; as > you say, I can't see what else this patch "fixes". > > > With that, feel free to add: > > > > Reviewed-by: Mark Rutland <mark.rutland@arm.com> > > Thanks! > > > We could even point out that the armv8_deprecated cases are > > UNPREDICTABLE in an IT block, and correctly emulated either way. > > Yes, I'll add something along those lines. Taking another look, I now can't track down where e.g., SWP in an IT block is specified to be UNPREDICTABLE. I only see e.g., ARM DDI 0487E.a Section 1.8.2 ("F1.8.2 Partial deprecation of IT"), which only deprecates the affected instructions. The legacy AArch32 SWP{B} insn is obsoleted by ARMv8, but the whole point of the armv8_deprecated stuff is to provide some backwards compatiblity with v7. So, this needs a closer look. I'll leave the Fixes tags for now, so that the archaeology doesn't need to redone if we decide that this patch does fix incorrect behaviour. Cheers ---Dave
On 18/10/2019 17:40, Dave Martin wrote: > On Tue, Oct 15, 2019 at 05:49:05PM +0100, Dave Martin wrote: >> On Tue, Oct 15, 2019 at 05:42:04PM +0100, Mark Rutland wrote: >>> On Tue, Oct 15, 2019 at 04:21:09PM +0100, Dave Martin wrote: >>>> On Fri, Oct 11, 2019 at 04:24:53PM +0100, Mark Rutland wrote: >>>>> On Thu, Oct 10, 2019 at 07:44:37PM +0100, Dave Martin wrote: >>>>>> Correct skipping of an instruction on AArch32 works a bit >>>>>> differently from AArch64, mainly due to the different CPSR/PSTATE >>>>>> semantics. >>>>>> >>>>>> There have been various attempts to get this right. Currenty >>>>>> arm64_skip_faulting_instruction() mostly does the right thing, but >>>>>> does not advance the IT state machine for the AArch32 case. >>>>>> >>>>>> arm64_compat_skip_faulting_instruction() handles the IT state >>>>>> machine but is local to traps.c, and porting other code to use it >>>>>> will make a mess since there are some call sites that apply for >>>>>> both the compat and native cases. >>>>>> >>>>>> Since manual instruction skipping implies a trap, it's a relatively >>>>>> slow path. >>>>>> >>>>>> So, make arm64_skip_faulting_instruction() handle both compat and >>>>>> native, and get rid of the arm64_compat_skip_faulting_instruction() >>>>>> special case. >>>>>> >>>>>> Fixes: 32a3e635fb0e ("arm64: compat: Add CNTFRQ trap handler") >>>>>> Fixes: 1f1c014035a8 ("arm64: compat: Add condition code checks and IT advance") >>>>>> Fixes: 6436beeee572 ("arm64: Fix single stepping in kernel traps") >>>>>> Fixes: bd35a4adc413 ("arm64: Port SWP/SWPB emulation support from arm") >>>>>> Signed-off-by: Dave Martin <Dave.Martin@arm.com> >>>>>> --- >>>>>> arch/arm64/kernel/traps.c | 18 ++++++++---------- >>>>>> 1 file changed, 8 insertions(+), 10 deletions(-) >>>>> >>>>> This looks good to me; it's certainly easier to reason about. >>>>> >>>>> I couldn't spot a place where we do the wrong thing today, given AFAICT >>>>> all the instances in arch/arm64/kernel/armv8_deprecated.c would be >>>>> UNPREDICTABLE within an IT block. >>>>> >>>>> It might be worth calling out an example in the commit message to >>>>> justify the fixes tags. >>>> >>>> IIRC I found no bug here; rather we have pointlessly fragmented code, >>>> so I followed the "if fixing the same bug in multiple places, merge >>>> those places so you need only fix it in one place next time" rule. >>> >>> Sure thing, that makes sense to me. >>> >>>> Since arm64_skip_faulting_instruction() is most of the way to being >>>> generically usable anyway, this series merges all the special-case >>>> handling into it. >>>> >>>> I could add something like >>>> >>>> --8<-- >>>> >>>> This allows this fiddly operation to be maintained in a single >>>> place rather than trying to maintain fragmented versions spread >>>> around arch/arm64. >>>> >>>> -->8-- >>>> >>>> Any good? >>> >>> My big concern is that the commit message reads as a fix, implying that >>> there's an existing correctness bug. I think that simplifying it to make >>> it clearer that it's a cleanup/improvement would be preferable. >>> >>> How about: >>> >>> | arm64: unify native/compat instruction skipping >>> | >>> | Skipping of an instruction on AArch32 works a bit differently from >>> | AArch64, mainly due to the different CPSR/PSTATE semantics. >>> | >>> | Currently arm64_skip_faulting_instruction() is only suitable for >>> | AArch64, and arm64_compat_skip_faulting_instruction() handles the IT >>> | state machine but is local to traps.c. >>> | >>> | Since manual instruction skipping implies a trap, it's a relatively >>> | slow path. >>> | >>> | So, make arm64_skip_faulting_instruction() handle both compat and >>> | native, and get rid of the arm64_compat_skip_faulting_instruction() >>> | special case. >>> | >>> | Signed-off-by: Dave Martin <Dave.Martin@arm.com> >> >> And drop the Fixes tags. Yes, I think that's reasonable. >> >> I think I was probably glossing over the fact that we don't need to get >> the ITSTATE machine advance correct for the compat insn emulation; as >> you say, I can't see what else this patch "fixes". >> >>> With that, feel free to add: >>> >>> Reviewed-by: Mark Rutland <mark.rutland@arm.com> >> >> Thanks! >> >>> We could even point out that the armv8_deprecated cases are >>> UNPREDICTABLE in an IT block, and correctly emulated either way. >> >> Yes, I'll add something along those lines. > > Taking another look, I now can't track down where e.g., SWP in an IT > block is specified to be UNPREDICTABLE. I only see e.g., > ARM DDI 0487E.a Section 1.8.2 ("F1.8.2 Partial deprecation of IT"), > which only deprecates the affected instructions. > > The legacy AArch32 SWP{B} insn is obsoleted by ARMv8, but the whole > point of the armv8_deprecated stuff is to provide some backwards > compatiblity with v7. > > > So, this needs a closer look. > > I'll leave the Fixes tags for now, so that the archaeology doesn't need > to redone if we decide that this patch does fix incorrect behaviour. The Thumb encoding of SETEND is explicitly not allowed in an IT block, while a Thumb encoding of SWB{B} has never existed, so that's moot. As far as I can see from DDI0406C.c, nothing prevents a Thumb MCR/MRC from being in an IT block (the ARM encodings are conditional, after all) so while they do fall under the performance deprecation, it would seem to be our bug if we don't already handle conditional CP15 barriers correctly. Robin.
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c index 15e3c4f..44c91d4 100644 --- a/arch/arm64/kernel/traps.c +++ b/arch/arm64/kernel/traps.c @@ -268,6 +268,8 @@ void arm64_notify_die(const char *str, struct pt_regs *regs, } } +static void advance_itstate(struct pt_regs *regs); + void arm64_skip_faulting_instruction(struct pt_regs *regs, unsigned long size) { regs->pc += size; @@ -278,6 +280,9 @@ void arm64_skip_faulting_instruction(struct pt_regs *regs, unsigned long size) */ if (user_mode(regs)) user_fastforward_single_step(current); + + if (regs->pstate & PSR_MODE32_BIT) + advance_itstate(regs); } static LIST_HEAD(undef_hook); @@ -629,19 +634,12 @@ static void advance_itstate(struct pt_regs *regs) compat_set_it_state(regs, it); } -static void arm64_compat_skip_faulting_instruction(struct pt_regs *regs, - unsigned int sz) -{ - advance_itstate(regs); - arm64_skip_faulting_instruction(regs, sz); -} - static void compat_cntfrq_read_handler(unsigned int esr, struct pt_regs *regs) { int reg = (esr & ESR_ELx_CP15_32_ISS_RT_MASK) >> ESR_ELx_CP15_32_ISS_RT_SHIFT; pt_regs_write_reg(regs, reg, arch_timer_get_rate()); - arm64_compat_skip_faulting_instruction(regs, 4); + arm64_skip_faulting_instruction(regs, 4); } static const struct sys64_hook cp15_32_hooks[] = { @@ -661,7 +659,7 @@ static void compat_cntvct_read_handler(unsigned int esr, struct pt_regs *regs) pt_regs_write_reg(regs, rt, lower_32_bits(val)); pt_regs_write_reg(regs, rt2, upper_32_bits(val)); - arm64_compat_skip_faulting_instruction(regs, 4); + arm64_skip_faulting_instruction(regs, 4); } static const struct sys64_hook cp15_64_hooks[] = { @@ -682,7 +680,7 @@ asmlinkage void __exception do_cp15instr(unsigned int esr, struct pt_regs *regs) * There is no T16 variant of a CP access, so we * always advance PC by 4 bytes. */ - arm64_compat_skip_faulting_instruction(regs, 4); + arm64_skip_faulting_instruction(regs, 4); return; }
Correct skipping of an instruction on AArch32 works a bit differently from AArch64, mainly due to the different CPSR/PSTATE semantics. There have been various attempts to get this right. Currenty arm64_skip_faulting_instruction() mostly does the right thing, but does not advance the IT state machine for the AArch32 case. arm64_compat_skip_faulting_instruction() handles the IT state machine but is local to traps.c, and porting other code to use it will make a mess since there are some call sites that apply for both the compat and native cases. Since manual instruction skipping implies a trap, it's a relatively slow path. So, make arm64_skip_faulting_instruction() handle both compat and native, and get rid of the arm64_compat_skip_faulting_instruction() special case. Fixes: 32a3e635fb0e ("arm64: compat: Add CNTFRQ trap handler") Fixes: 1f1c014035a8 ("arm64: compat: Add condition code checks and IT advance") Fixes: 6436beeee572 ("arm64: Fix single stepping in kernel traps") Fixes: bd35a4adc413 ("arm64: Port SWP/SWPB emulation support from arm") Signed-off-by: Dave Martin <Dave.Martin@arm.com> --- arch/arm64/kernel/traps.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)