| Message ID | 20230320131845.3138015-5-ardb@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: vfp: Switch to C API to en/disable softirqs | expand |
On Mon, Mar 20, 2023 at 2:19 PM Ard Biesheuvel <ardb@kernel.org> wrote: > The conditional MOVS instruction that appears to have been added to test > for the TIF_USING_IWMMXT thread_info flag only sets the N and Z > condition flags and register R7, none of which are referenced in the > subsequent code. This means that the instruction does nothing, which > means that we might misidentify faulting FPE instructions as iWMMXT > instructions on kernels that were built to support both. > > This seems to have been part of the original submission of the code, and > so this has never worked as intended, and nobody ever noticed, and so we > might decide to just leave this as-is. However, with the ongoing move > towards multiplatform kernels, the issue becomes more likely to > manifest, and so it is better to fix it. > > So check whether we are dealing with an undef exception regarding > coprocessor index #0 or #1, and if so, load the thread_info flag and > only dispatch it as a iWMMXT trap if the flag is set. > > Cc: <stable@vger.kernel.org> # v2.6.9+ > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> Looks right to me. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> The code dates back before git history, but it was introduced in Linux v2.6.9 and it looks like Nicolas Pitre wrote it so let's just ping him and see what he remembers about this! Yours, Linus Walleij
On Mon, Mar 20, 2023 at 2:19 PM Ard Biesheuvel <ardb@kernel.org> wrote: > The conditional MOVS instruction that appears to have been added to test > for the TIF_USING_IWMMXT thread_info flag only sets the N and Z > condition flags and register R7, none of which are referenced in the > subsequent code. Really? As far as I know, the rsb instruction is a "reversed subtract" and that also sets the carry flag. And so does a move with a shifter argument (the last dropped bit is moved to the carry flag). What am I missing? Nicolas
On Tue, 21 Mar 2023 at 20:19, Nicolas Pitre <npitre@baylibre.com> wrote: > > On Mon, Mar 20, 2023 at 2:19 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > The conditional MOVS instruction that appears to have been added to test > > for the TIF_USING_IWMMXT thread_info flag only sets the N and Z > > condition flags and register R7, none of which are referenced in the > > subsequent code. > > Really? > > As far as I know, the rsb instruction is a "reversed subtract" and that > also sets the carry flag. > > And so does a move with a shifter argument (the last dropped bit is > moved to the carry flag). > > What am I missing? > No, you are absolutely right. I looked up the wrong encoding in the ARM ARM. MOVS without a shift preserves the carry flag, but the variant you used here behaves as you describe. So the code is correct - apologies for the noise.
diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S index c39303e5c23470e6..c5d2f07994fb0d87 100644 --- a/arch/arm/kernel/entry-armv.S +++ b/arch/arm/kernel/entry-armv.S @@ -606,10 +606,11 @@ call_fpe: strb r7, [r6, #TI_USED_CP] @ set appropriate used_cp[] #ifdef CONFIG_IWMMXT @ Test if we need to give access to iWMMXt coprocessors - ldr r5, [r10, #TI_FLAGS] - rsbs r7, r8, #(1 << 8) @ CP 0 or 1 only - movscs r7, r5, lsr #(TIF_USING_IWMMXT + 1) - bcs iwmmxt_task_enable + tst r8, #0xe << 8 @ CP 0 or 1? + ldreq r5, [r10, #TI_FLAGS] @ if so, load thread_info flags + andeq r5, r5, #1 << TIF_USING_IWMMXT @ isolate TIF_USING_IWMMXT flag + teqeq r5, #1 << TIF_USING_IWMMXT @ check whether it is set + beq iwmmxt_task_enable @ branch if set #endif ARM( add pc, pc, r8, lsr #6 ) THUMB( lsr r8, r8, #6 )
The conditional MOVS instruction that appears to have been added to test for the TIF_USING_IWMMXT thread_info flag only sets the N and Z condition flags and register R7, none of which are referenced in the subsequent code. This means that the instruction does nothing, which means that we might misidentify faulting FPE instructions as iWMMXT instructions on kernels that were built to support both. This seems to have been part of the original submission of the code, and so this has never worked as intended, and nobody ever noticed, and so we might decide to just leave this as-is. However, with the ongoing move towards multiplatform kernels, the issue becomes more likely to manifest, and so it is better to fix it. So check whether we are dealing with an undef exception regarding coprocessor index #0 or #1, and if so, load the thread_info flag and only dispatch it as a iWMMXT trap if the flag is set. Cc: <stable@vger.kernel.org> # v2.6.9+ Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- arch/arm/kernel/entry-armv.S | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)