Re: [PATCH 2/2] arm64: Emulate SETEND for AArch32 tasks

From: Suzuki K. Poulose
Date: Fri Jan 09 2015 - 05:21:11 EST


On 08/01/15 18:43, Mark Rutland wrote:
Hi Suzuki,

On Wed, Jan 07, 2015 at 04:16:45PM +0000, Suzuki K. Poulose wrote:
From: "Suzuki K. Poulose" <suzuki.poulose@xxxxxxx>

Emulate deprecated 'setend' instruction for AArch32 bit tasks.

setend [le/be] - Sets the endianness of EL0

The hardware support for the instruction can be enabled by setting the
SCTLR_EL1.SED bit. Like the other emulated instructions it is controlled by
an entry in /proc/sys/abi/. For more information see :
Documentation/arm64/legacy_instructions.txt

The instruction is emulated by setting/clearing the SPSR_EL1.E bit, which
will be reflected in the PSTATE.E in AArch32 context.

A "fun" problem with emulating setend is that it will not always work
unless we emulate the entire instruction set when userspace wants to be
in an unsupported endianness.

For implementations which are not bi-endian at EL0 (i.e. where
ID_AA64MMFR0_EL1.BigEndEL0 == 0), SCTLR_EL1.E0E has a fixed value which
we cannot change. The field names are misleading: in a BE-only system
ID_AA64MMFR0_EL1.{BigEnd,BigEndEL0} == {0,0} and SCTLR_EL1.{EE,E0E} are
fixed to {1,1}.

I think we need to detect when EL0 has a fixed endianness such that we
can treat the setend instruction as undefined. Otherwise we will
silently fail to change EL0 endianness, advance the PC, and return to
userspace in the wrong endianness, which will be very painful to debug.
Userspace has the option of handling the resulting SIGILL in such cases.

You are right. I missed this scenario. To add to that things get complicated when there are heterogeneous CPUs on the system that might have differing bits for BigEndEL0. I will take a look at this one. Thanks for pointing this out.

That means we need to be able to fail to transition into INSN_EMULATE
mode as we currently can when transitioning to INSN_HW.

This patch also restores the native endianness for the execution of signal
handlers, since the process could have changed the endianness.

Signed-off-by: Suzuki K. Poulose <suzuki.poulose@xxxxxxx>
---
Documentation/arm64/legacy_instructions.txt | 5 ++
arch/arm64/Kconfig | 10 ++++
arch/arm64/include/asm/ptrace.h | 7 +++
arch/arm64/kernel/armv8_deprecated.c | 75 +++++++++++++++++++++++++++
arch/arm64/kernel/signal32.c | 5 +-
5 files changed, 101 insertions(+), 1 deletion(-)

diff --git a/Documentation/arm64/legacy_instructions.txt b/Documentation/arm64/legacy_instructions.txt
index a3b3da2..20e5621 100644
--- a/Documentation/arm64/legacy_instructions.txt
+++ b/Documentation/arm64/legacy_instructions.txt
@@ -43,3 +43,8 @@ Default: Undef (0)
Node: /proc/sys/abi/cp15_barrier
Status: Deprecated
Default: Emulate (1)
+
+* SETEND
+Node: /proc/sys/abi/setend
+Status: Deprecated
+Default: Emulate (1)

Given we can't always emulate SETEND, should we document "Emulate where
possible" or something to that effect?

Will fix it in the next revision.

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index b1f9a20..c6d1fd9 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -540,6 +540,16 @@ config CP15_BARRIER_EMULATION

If unsure, say Y

+config SETEND_EMULATION
+ bool "Emulate SETEND instruction"
+ help
+ The SETEND instruction alters the data-endianness of the
+ AArch32 EL0, and is deprecated in ARMv8.
+
+ Say Y here to enable software emulation of the instruction
+ for AArch32 userspace code.
+
+ If unsure, say Y
endif

endmenu
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index 41ed9e1..d6dd9fd 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -58,6 +58,13 @@
#define COMPAT_PSR_Z_BIT 0x40000000
#define COMPAT_PSR_N_BIT 0x80000000
#define COMPAT_PSR_IT_MASK 0x0600fc00 /* If-Then execution state mask */
+
+#ifdef CONFIG_CPU_BIG_ENDIAN
+#define COMPAT_PSR_ENDSTATE COMPAT_PSR_E_BIT
+#else
+#define COMPAT_PSR_ENDSTATE 0
+#endif
+
/*
* These are 'magic' values for PTRACE_PEEKUSR that return info about where a
* process is located in memory.
diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c
index 9054447..dc91bac 100644
--- a/arch/arm64/kernel/armv8_deprecated.c
+++ b/arch/arm64/kernel/armv8_deprecated.c
@@ -477,6 +477,7 @@ ret:
}

#define SCTLR_EL1_CP15BEN (1 << 5)
+#define SCTLR_EL1_SED (1 << 8)

static inline void config_sctlr_el1(u32 clear, u32 set)
{
@@ -521,6 +522,77 @@ static struct insn_emulation_ops cp15_barrier_ops = {
.set_hw_mode = cp15_barrier_set_hw_mode,
};

+static void setend_set_hw_mode(void *enable)
+{
+ if (enable)
+ config_sctlr_el1(SCTLR_EL1_SED, 0);
+ else
+ config_sctlr_el1(0, SCTLR_EL1_SED);
+}
+
+static int compat_setend_handler(struct pt_regs *regs, u32 endian)

If we s/endian/big_endian/ here we can drop the comments within the
function as the test will be easier to read. We could also s/u32/bool/.

OK
+{
+ char insn[16] = "setend _e";

Elsewhere (e.g. in cp15barrier_handler) we write these out in full
rather than modifying a string on the stack. I think we should do the
same here (we can change insn to a char * and assign the full relevant
string in either branch).

Doing so will mean grepping for '"setend be"' finds this function, which
is handy.

Makes sense. Thanks for the review.

Thanks
Suzuki


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/