Re: [Qemu-devel] [PATCH v1 13/17] arm: linux-user: don't set CPSR.E in BE32 mode
On Tue, Jan 19, 2016 at 9:35 AM, Peter Maydell wrote:
> On 19 January 2016 at 17:26, Peter Maydell wrote:
>> On 18 January 2016 at 07:12, Peter Crosthwaite
>> 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
>>> ---
>>>
>>> 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
Re: [Qemu-devel] [PATCH v1 13/17] arm: linux-user: don't set CPSR.E in BE32 mode
On 19 January 2016 at 17:26, Peter Maydell wrote:
> On 18 January 2016 at 07:12, Peter Crosthwaite
> 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
>> ---
>>
>> 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
Re: [Qemu-devel] [PATCH v1 13/17] arm: linux-user: don't set CPSR.E in BE32 mode
On 18 January 2016 at 07:12, Peter Crosthwaite
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
> ---
>
> 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
