diff mbox

[v1,13/17] arm: linux-user: don't set CPSR.E in BE32 mode

Message ID cc12cc9e952dc74183cf875c1ae44446067f3236.1453100525.git.crosthwaite.peter@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Crosthwaite Jan. 18, 2016, 7:12 a.m. UTC
Don't set CPSR.E for BE32 linux-user mode. As linux-user mode models
BE32, using normal BE (and system mode will not), a special case is
needed for user-mode where if sctlr.b is set, the CPU identifies as BE.

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---

 linux-user/main.c |  2 --
 target-arm/cpu.h  | 12 +++++++++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

Comments

Peter Maydell Jan. 19, 2016, 5:26 p.m. UTC | #1
On 18 January 2016 at 07:12, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> Don't set CPSR.E for BE32 linux-user mode. As linux-user mode models
> BE32, using normal BE (and system mode will not), a special case is
> needed for user-mode where if sctlr.b is set, the CPU identifies as BE.
>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
>
>  linux-user/main.c |  2 --
>  target-arm/cpu.h  | 12 +++++++++++-
>  2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/linux-user/main.c b/linux-user/main.c
> index d481458..60375fb 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -4496,8 +4496,6 @@ int main(int argc, char **argv, char **envp)
>              env->uncached_cpsr |= CPSR_E;
>          } else {
>              env->cp15.sctlr_el[1] |= SCTLR_B;
> -            /* We model BE32 as regular BE, so set CPSR_E */
> -            env->uncached_cpsr |= CPSR_E;

...this is undoing what we just did in the previous patch and
which I reviewed as being the wrong thing there...

>          }
>  #endif
>      }
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 3edd56b..96b1e99 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1812,7 +1812,17 @@ static bool arm_cpu_is_big_endian(CPUARMState *env)
>
>      /* In 32bit endianness is determined by looking at CPSR's E bit */
>      if (!is_a64(env)) {
> -        return (env->uncached_cpsr & CPSR_E) ? 1 : 0;
> +        return
> +#ifdef CONFIG_USER_ONLY
> +            /* In user mode, BE32 data accesses are just modelled as
> +             * regular BE access. In system mode, BE32 is modelled as
> +             * little endian, with the appropriate address translations on
> +             * non-word accesses. So sctlr.b only affects overall
> +             * endianness in user mode
> +             */
> +            arm_sctlr_b(env) ||
> +#endif
> +                ((env->uncached_cpsr & CPSR_E) ? 1 : 0);
>      }

This doesn't seem quite right -- for system emulation we currently
pick MO_BE or MO_LE based on the TB flag which is set according
to (arm_cpu_is_big_endian(env). So if we ignore SCTLR.B in
system mode then we'll still try to do LE accesses.

>      cur_el = arm_current_el(env);

thanks
-- PMM
Peter Maydell Jan. 19, 2016, 5:35 p.m. UTC | #2
On 19 January 2016 at 17:26, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 January 2016 at 07:12, Peter Crosthwaite
> <crosthwaitepeter@gmail.com> wrote:
>> Don't set CPSR.E for BE32 linux-user mode. As linux-user mode models
>> BE32, using normal BE (and system mode will not), a special case is
>> needed for user-mode where if sctlr.b is set, the CPU identifies as BE.
>>
>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>> ---
>>
>>  linux-user/main.c |  2 --
>>  target-arm/cpu.h  | 12 +++++++++++-
>>  2 files changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/linux-user/main.c b/linux-user/main.c
>> index d481458..60375fb 100644
>> --- a/linux-user/main.c
>> +++ b/linux-user/main.c
>> @@ -4496,8 +4496,6 @@ int main(int argc, char **argv, char **envp)
>>              env->uncached_cpsr |= CPSR_E;
>>          } else {
>>              env->cp15.sctlr_el[1] |= SCTLR_B;
>> -            /* We model BE32 as regular BE, so set CPSR_E */
>> -            env->uncached_cpsr |= CPSR_E;
>
> ...this is undoing what we just did in the previous patch and
> which I reviewed as being the wrong thing there...
>
>>          }
>>  #endif
>>      }
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index 3edd56b..96b1e99 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -1812,7 +1812,17 @@ static bool arm_cpu_is_big_endian(CPUARMState *env)
>>
>>      /* In 32bit endianness is determined by looking at CPSR's E bit */
>>      if (!is_a64(env)) {
>> -        return (env->uncached_cpsr & CPSR_E) ? 1 : 0;
>> +        return
>> +#ifdef CONFIG_USER_ONLY
>> +            /* In user mode, BE32 data accesses are just modelled as
>> +             * regular BE access. In system mode, BE32 is modelled as
>> +             * little endian, with the appropriate address translations on
>> +             * non-word accesses. So sctlr.b only affects overall
>> +             * endianness in user mode
>> +             */
>> +            arm_sctlr_b(env) ||
>> +#endif
>> +                ((env->uncached_cpsr & CPSR_E) ? 1 : 0);
>>      }
>
> This doesn't seem quite right -- for system emulation we currently
> pick MO_BE or MO_LE based on the TB flag which is set according
> to (arm_cpu_is_big_endian(env). So if we ignore SCTLR.B in
> system mode then we'll still try to do LE accesses.

Ah, no, looking at the next patch this is correct, it's just the
comment is a touch confusing. I suggest

 /* In system mode, BE32 is modelled in line with the architecture
  * (as word-invariant big-endianness), where loads and stores are done
  * little endian but from addresses which are adjusted by XORing
  * with the appropriate constant. So the endianness to use for the
  * raw data access is not affected by SCTLR.B.
  * In user mode, however, we model BE32 as byte-invariant big-endianness
  * (because user-only code cannot tell the difference), and so we
  * need to use a data access endianness that depends on SCTLR.B.
  */

thanks
-- PMM
Peter Crosthwaite Feb. 27, 2016, 10:22 p.m. UTC | #3
On Tue, Jan 19, 2016 at 9:35 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 19 January 2016 at 17:26, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 18 January 2016 at 07:12, Peter Crosthwaite
>> <crosthwaitepeter@gmail.com> wrote:
>>> Don't set CPSR.E for BE32 linux-user mode. As linux-user mode models
>>> BE32, using normal BE (and system mode will not), a special case is
>>> needed for user-mode where if sctlr.b is set, the CPU identifies as BE.
>>>
>>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>>> ---
>>>
>>>  linux-user/main.c |  2 --
>>>  target-arm/cpu.h  | 12 +++++++++++-
>>>  2 files changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/linux-user/main.c b/linux-user/main.c
>>> index d481458..60375fb 100644
>>> --- a/linux-user/main.c
>>> +++ b/linux-user/main.c
>>> @@ -4496,8 +4496,6 @@ int main(int argc, char **argv, char **envp)
>>>              env->uncached_cpsr |= CPSR_E;
>>>          } else {
>>>              env->cp15.sctlr_el[1] |= SCTLR_B;
>>> -            /* We model BE32 as regular BE, so set CPSR_E */
>>> -            env->uncached_cpsr |= CPSR_E;
>>
>> ...this is undoing what we just did in the previous patch and
>> which I reviewed as being the wrong thing there...
>>
>>>          }
>>>  #endif
>>>      }
>>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>>> index 3edd56b..96b1e99 100644
>>> --- a/target-arm/cpu.h
>>> +++ b/target-arm/cpu.h
>>> @@ -1812,7 +1812,17 @@ static bool arm_cpu_is_big_endian(CPUARMState *env)
>>>
>>>      /* In 32bit endianness is determined by looking at CPSR's E bit */
>>>      if (!is_a64(env)) {
>>> -        return (env->uncached_cpsr & CPSR_E) ? 1 : 0;
>>> +        return
>>> +#ifdef CONFIG_USER_ONLY
>>> +            /* In user mode, BE32 data accesses are just modelled as
>>> +             * regular BE access. In system mode, BE32 is modelled as
>>> +             * little endian, with the appropriate address translations on
>>> +             * non-word accesses. So sctlr.b only affects overall
>>> +             * endianness in user mode
>>> +             */
>>> +            arm_sctlr_b(env) ||
>>> +#endif
>>> +                ((env->uncached_cpsr & CPSR_E) ? 1 : 0);
>>>      }
>>
>> This doesn't seem quite right -- for system emulation we currently
>> pick MO_BE or MO_LE based on the TB flag which is set according
>> to (arm_cpu_is_big_endian(env). So if we ignore SCTLR.B in
>> system mode then we'll still try to do LE accesses.
>
> Ah, no, looking at the next patch this is correct, it's just the
> comment is a touch confusing. I suggest
>
>  /* In system mode, BE32 is modelled in line with the architecture
>   * (as word-invariant big-endianness), where loads and stores are done
>   * little endian but from addresses which are adjusted by XORing
>   * with the appropriate constant. So the endianness to use for the
>   * raw data access is not affected by SCTLR.B.
>   * In user mode, however, we model BE32 as byte-invariant big-endianness
>   * (because user-only code cannot tell the difference), and so we
>   * need to use a data access endianness that depends on SCTLR.B.
>   */
>

Comment updated.

Regards,
Peter

> thanks
> -- PMM
diff mbox

Patch

diff --git a/linux-user/main.c b/linux-user/main.c
index d481458..60375fb 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -4496,8 +4496,6 @@  int main(int argc, char **argv, char **envp)
             env->uncached_cpsr |= CPSR_E;
         } else {
             env->cp15.sctlr_el[1] |= SCTLR_B;
-            /* We model BE32 as regular BE, so set CPSR_E */
-            env->uncached_cpsr |= CPSR_E;
         }
 #endif
     }
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 3edd56b..96b1e99 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -1812,7 +1812,17 @@  static bool arm_cpu_is_big_endian(CPUARMState *env)
 
     /* In 32bit endianness is determined by looking at CPSR's E bit */
     if (!is_a64(env)) {
-        return (env->uncached_cpsr & CPSR_E) ? 1 : 0;
+        return
+#ifdef CONFIG_USER_ONLY
+            /* In user mode, BE32 data accesses are just modelled as
+             * regular BE access. In system mode, BE32 is modelled as
+             * little endian, with the appropriate address translations on
+             * non-word accesses. So sctlr.b only affects overall
+             * endianness in user mode
+             */
+            arm_sctlr_b(env) ||
+#endif
+                ((env->uncached_cpsr & CPSR_E) ? 1 : 0);
     }
 
     cur_el = arm_current_el(env);