Re: [PATCH v2 09/12] arm64: traps: Fix inconsistent faulting instruction skipping

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:

quoted

On Thu, Oct 10, 2019 at 07:44:37PM +0100, Dave Martin wrote:

quoted

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 [off-list ref]

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel