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

[Robin Murphy]

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.