| Message ID | cc12cc9e952dc74183cf875c1ae44446067f3236.1453100525.git.crosthwaite.peter@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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 --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);
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(-)