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

Fri Jan 9 02:21:00 PST 2015

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 at arm.com>
>>
>> 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 at arm.com>
>> ---
>>   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