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@xxxxxxx>
---
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@xxxxxxx>
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@xxxxxxx>
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.
