[Qemu-devel] [PATCH 05/20] target/arm: Fix arm_cpu_data_is_big_endian for aa64 user-only

Richard Henderson posted 20 patches 7 years, 2 months ago
There is a newer version of this series
[Qemu-devel] [PATCH 05/20] target/arm: Fix arm_cpu_data_is_big_endian for aa64 user-only
Posted by Richard Henderson 7 years, 2 months ago
Unlike aa32, endianness cannot be adjusted by userland in aa64.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.h | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 9526ed27cb..2d6d7d03aa 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2709,8 +2709,6 @@ static inline bool arm_sctlr_b(CPUARMState *env)
 /* Return true if the processor is in big-endian mode. */
 static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
 {
-    int cur_el;
-
     /* In 32bit endianness is determined by looking at CPSR's E bit */
     if (!is_a64(env)) {
         return
@@ -2729,15 +2727,24 @@ static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
             arm_sctlr_b(env) ||
 #endif
                 ((env->uncached_cpsr & CPSR_E) ? 1 : 0);
+    } else {
+#ifdef CONFIG_USER_ONLY
+        /* AArch64 does not have a SETEND instruction; endianness
+         * for usermode is fixed at compile-time.
+         */
+# ifdef TARGET_WORDS_BIGENDIAN
+        return true;
+# else
+        return false;
+# endif
+#else
+        int cur_el = arm_current_el(env);
+        if (cur_el == 0) {
+            return (env->cp15.sctlr_el[1] & SCTLR_E0E) != 0;
+        }
+        return (env->cp15.sctlr_el[cur_el] & SCTLR_EE) != 0;
+#endif
     }
-
-    cur_el = arm_current_el(env);
-
-    if (cur_el == 0) {
-        return (env->cp15.sctlr_el[1] & SCTLR_E0E) != 0;
-    }
-
-    return (env->cp15.sctlr_el[cur_el] & SCTLR_EE) != 0;
 }
 
 #include "exec/cpu-all.h"
-- 
2.17.1


Re: [Qemu-devel] [PATCH 05/20] target/arm: Fix arm_cpu_data_is_big_endian for aa64 user-only
Posted by Peter Maydell 7 years, 2 months ago
On 9 August 2018 at 05:21, Richard Henderson
<richard.henderson@linaro.org> wrote:
> Unlike aa32, endianness cannot be adjusted by userland in aa64.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.h | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 9526ed27cb..2d6d7d03aa 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2709,8 +2709,6 @@ static inline bool arm_sctlr_b(CPUARMState *env)
>  /* Return true if the processor is in big-endian mode. */
>  static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
>  {
> -    int cur_el;
> -
>      /* In 32bit endianness is determined by looking at CPSR's E bit */
>      if (!is_a64(env)) {
>          return
> @@ -2729,15 +2727,24 @@ static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
>              arm_sctlr_b(env) ||
>  #endif
>                  ((env->uncached_cpsr & CPSR_E) ? 1 : 0);
> +    } else {
> +#ifdef CONFIG_USER_ONLY
> +        /* AArch64 does not have a SETEND instruction; endianness
> +         * for usermode is fixed at compile-time.
> +         */
> +# ifdef TARGET_WORDS_BIGENDIAN
> +        return true;
> +# else
> +        return false;
> +# endif
> +#else
> +        int cur_el = arm_current_el(env);
> +        if (cur_el == 0) {
> +            return (env->cp15.sctlr_el[1] & SCTLR_E0E) != 0;
> +        }
> +        return (env->cp15.sctlr_el[cur_el] & SCTLR_EE) != 0;
> +#endif
>      }
> -
> -    cur_el = arm_current_el(env);
> -
> -    if (cur_el == 0) {
> -        return (env->cp15.sctlr_el[1] & SCTLR_E0E) != 0;
> -    }
> -
> -    return (env->cp15.sctlr_el[cur_el] & SCTLR_EE) != 0;
>  }
>

When does this make a difference? For user-mode, we've already
dealt with the "aa32" case, so the code here is aa64-only.
In linux-user/aarch64/cpu_loop.c we set sctlr_el[1]'s E0E bit
if TARGET_WORDS_BIGENDIAN is defined, and cur_el is definitely
zero, so we should already be returning true from this function
if TARGET_WORDS_BIGENDIAN and false otherwise.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 05/20] target/arm: Fix arm_cpu_data_is_big_endian for aa64 user-only
Posted by Richard Henderson 7 years, 2 months ago
On 08/17/2018 09:02 AM, Peter Maydell wrote:
> On 9 August 2018 at 05:21, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>> Unlike aa32, endianness cannot be adjusted by userland in aa64.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  target/arm/cpu.h | 27 +++++++++++++++++----------
>>  1 file changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index 9526ed27cb..2d6d7d03aa 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -2709,8 +2709,6 @@ static inline bool arm_sctlr_b(CPUARMState *env)
>>  /* Return true if the processor is in big-endian mode. */
>>  static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
>>  {
>> -    int cur_el;
>> -
>>      /* In 32bit endianness is determined by looking at CPSR's E bit */
>>      if (!is_a64(env)) {
>>          return
>> @@ -2729,15 +2727,24 @@ static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
>>              arm_sctlr_b(env) ||
>>  #endif
>>                  ((env->uncached_cpsr & CPSR_E) ? 1 : 0);
>> +    } else {
>> +#ifdef CONFIG_USER_ONLY
>> +        /* AArch64 does not have a SETEND instruction; endianness
>> +         * for usermode is fixed at compile-time.
>> +         */
>> +# ifdef TARGET_WORDS_BIGENDIAN
>> +        return true;
>> +# else
>> +        return false;
>> +# endif
>> +#else
>> +        int cur_el = arm_current_el(env);
>> +        if (cur_el == 0) {
>> +            return (env->cp15.sctlr_el[1] & SCTLR_E0E) != 0;
>> +        }
>> +        return (env->cp15.sctlr_el[cur_el] & SCTLR_EE) != 0;
>> +#endif
>>      }
>> -
>> -    cur_el = arm_current_el(env);
>> -
>> -    if (cur_el == 0) {
>> -        return (env->cp15.sctlr_el[1] & SCTLR_E0E) != 0;
>> -    }
>> -
>> -    return (env->cp15.sctlr_el[cur_el] & SCTLR_EE) != 0;
>>  }
>>
> 
> When does this make a difference? For user-mode, we've already
> dealt with the "aa32" case, so the code here is aa64-only.
> In linux-user/aarch64/cpu_loop.c we set sctlr_el[1]'s E0E bit
> if TARGET_WORDS_BIGENDIAN is defined, and cur_el is definitely
> zero, so we should already be returning true from this function
> if TARGET_WORDS_BIGENDIAN and false otherwise.

I should have re-ordered this after the other following
simplifications to see if it still matters.  But I was
after a code-size reduction.


r~