Re: [Qemu-devel] [PATCH v1 04/17] target-arm: implement SCTLR.EE
On Tue, Jan 19, 2016 at 7:58 AM, Peter Maydell wrote:
> On 18 January 2016 at 07:12, Peter Crosthwaite
> wrote:
>> From: Peter Crosthwaite
>>
>> Implement SCTLR.EE bit which controls data endianess for exceptions
>> and page table translations. SCTLR.EE is mirrored to the CPSR.E bit
>> on exception entry.
>>
>> Signed-off-by: Peter Crosthwaite
>> ---
>>
>> target-arm/helper.c | 42 --
>> 1 file changed, 32 insertions(+), 10 deletions(-)
>>
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 59d5a41..afac1b2 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -5889,7 +5889,10 @@ void arm_cpu_do_interrupt(CPUState *cs)
>> /* Clear IT bits. */
>> env->condexec_bits = 0;
>> /* Switch to the new mode, and to the correct instruction set. */
>> -env->uncached_cpsr = (env->uncached_cpsr & ~CPSR_M) | new_mode;
>> +env->uncached_cpsr = (env->uncached_cpsr & ~(CPSR_M)) | new_mode;
>
> Why change this line?
>
Reverted
>> +/* Set new mode endianess */
>
> "endianness"
>
Fixed
>> +env->uncached_cpsr = (env->uncached_cpsr & ~(CPSR_E)) |
>> +(env->cp15.sctlr_el[arm_current_el(env)] & SCTLR_EE ? CPSR_E : 0);
>
> This is a bit confusing. I think just splitting it into
> multiple statements would help:
>env->uncached_cpsr &= ~CPSR_E;
>if (env->cp15.sctlr_el[arm_current_el(env)] & SCTLR_EE) {
>env->uncached_cpsr |= CPSR_E;
>}
>
Fixed.
>> env->daif |= mask;
>> /* this is a lie, as the was no c1_sys on V4T/V5, but who cares
>> * and we should just guard the thumb mode on V4 */
>> @@ -5958,6 +5961,12 @@ static inline bool
>> regime_translation_disabled(CPUARMState *env,
>> return (regime_sctlr(env, mmu_idx) & SCTLR_M) == 0;
>> }
>>
>> +static inline bool regime_translation_big_endian(CPUARMState *env,
>> + ARMMMUIdx mmu_idx)
>> +{
>> +return (regime_sctlr(env, mmu_idx) & SCTLR_EE) != 0;
>> +}
>> +
>> /* Return the TCR controlling this translation regime */
>> static inline TCR *regime_tcr(CPUARMState *env, ARMMMUIdx mmu_idx)
>> {
>> @@ -6263,7 +6272,7 @@ static hwaddr S1_ptw_translate(CPUARMState *env,
>> ARMMMUIdx mmu_idx,
>> */
>> static uint32_t arm_ldl_ptw(CPUState *cs, hwaddr addr, bool is_secure,
>> ARMMMUIdx mmu_idx, uint32_t *fsr,
>> -ARMMMUFaultInfo *fi)
>> +ARMMMUFaultInfo *fi, bool be)
>> {
>> ARMCPU *cpu = ARM_CPU(cs);
>> CPUARMState *env = &cpu->env;
>> @@ -6274,12 +6283,16 @@ static uint32_t arm_ldl_ptw(CPUState *cs, hwaddr
>> addr, bool is_secure,
>> if (fi->s1ptw) {
>> return 0;
>> }
>> -return address_space_ldl(cs->as, addr, attrs, NULL);
>> +if (be) {
>> +return address_space_ldl_be(cs->as, addr, attrs, NULL);
>> +} else {
>> +return address_space_ldl_le(cs->as, addr, attrs, NULL);
>> +}
>> }
>
> Why not just call regime_translation_big_endian() inside arm_ldl_ptw()
> and arm_ldq_ptw(), rather than having every call site making the call
> and passing in the result?
>
Fixed.
> PS: this patch will conflict with the multi-ases series but only
> fairly trivially.
>
Resolved.
Regards,
Peter
> thanks
> -- PMM
Re: [Qemu-devel] [PATCH v1 04/17] target-arm: implement SCTLR.EE
On 18 January 2016 at 07:12, Peter Crosthwaite
wrote:
> From: Peter Crosthwaite
>
> Implement SCTLR.EE bit which controls data endianess for exceptions
> and page table translations. SCTLR.EE is mirrored to the CPSR.E bit
> on exception entry.
>
> Signed-off-by: Peter Crosthwaite
> ---
>
> target-arm/helper.c | 42 --
> 1 file changed, 32 insertions(+), 10 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 59d5a41..afac1b2 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -5889,7 +5889,10 @@ void arm_cpu_do_interrupt(CPUState *cs)
> /* Clear IT bits. */
> env->condexec_bits = 0;
> /* Switch to the new mode, and to the correct instruction set. */
> -env->uncached_cpsr = (env->uncached_cpsr & ~CPSR_M) | new_mode;
> +env->uncached_cpsr = (env->uncached_cpsr & ~(CPSR_M)) | new_mode;
Why change this line?
> +/* Set new mode endianess */
"endianness"
> +env->uncached_cpsr = (env->uncached_cpsr & ~(CPSR_E)) |
> +(env->cp15.sctlr_el[arm_current_el(env)] & SCTLR_EE ? CPSR_E : 0);
This is a bit confusing. I think just splitting it into
multiple statements would help:
env->uncached_cpsr &= ~CPSR_E;
if (env->cp15.sctlr_el[arm_current_el(env)] & SCTLR_EE) {
env->uncached_cpsr |= CPSR_E;
}
> env->daif |= mask;
> /* this is a lie, as the was no c1_sys on V4T/V5, but who cares
> * and we should just guard the thumb mode on V4 */
> @@ -5958,6 +5961,12 @@ static inline bool
> regime_translation_disabled(CPUARMState *env,
> return (regime_sctlr(env, mmu_idx) & SCTLR_M) == 0;
> }
>
> +static inline bool regime_translation_big_endian(CPUARMState *env,
> + ARMMMUIdx mmu_idx)
> +{
> +return (regime_sctlr(env, mmu_idx) & SCTLR_EE) != 0;
> +}
> +
> /* Return the TCR controlling this translation regime */
> static inline TCR *regime_tcr(CPUARMState *env, ARMMMUIdx mmu_idx)
> {
> @@ -6263,7 +6272,7 @@ static hwaddr S1_ptw_translate(CPUARMState *env,
> ARMMMUIdx mmu_idx,
> */
> static uint32_t arm_ldl_ptw(CPUState *cs, hwaddr addr, bool is_secure,
> ARMMMUIdx mmu_idx, uint32_t *fsr,
> -ARMMMUFaultInfo *fi)
> +ARMMMUFaultInfo *fi, bool be)
> {
> ARMCPU *cpu = ARM_CPU(cs);
> CPUARMState *env = &cpu->env;
> @@ -6274,12 +6283,16 @@ static uint32_t arm_ldl_ptw(CPUState *cs, hwaddr
> addr, bool is_secure,
> if (fi->s1ptw) {
> return 0;
> }
> -return address_space_ldl(cs->as, addr, attrs, NULL);
> +if (be) {
> +return address_space_ldl_be(cs->as, addr, attrs, NULL);
> +} else {
> +return address_space_ldl_le(cs->as, addr, attrs, NULL);
> +}
> }
Why not just call regime_translation_big_endian() inside arm_ldl_ptw()
and arm_ldq_ptw(), rather than having every call site making the call
and passing in the result?
PS: this patch will conflict with the multi-ases series but only
fairly trivially.
thanks
-- PMM
[Qemu-devel] [PATCH v1 04/17] target-arm: implement SCTLR.EE
From: Peter Crosthwaite
Implement SCTLR.EE bit which controls data endianess for exceptions
and page table translations. SCTLR.EE is mirrored to the CPSR.E bit
on exception entry.
Signed-off-by: Peter Crosthwaite
---
target-arm/helper.c | 42 --
1 file changed, 32 insertions(+), 10 deletions(-)
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 59d5a41..afac1b2 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5889,7 +5889,10 @@ void arm_cpu_do_interrupt(CPUState *cs)
/* Clear IT bits. */
env->condexec_bits = 0;
/* Switch to the new mode, and to the correct instruction set. */
-env->uncached_cpsr = (env->uncached_cpsr & ~CPSR_M) | new_mode;
+env->uncached_cpsr = (env->uncached_cpsr & ~(CPSR_M)) | new_mode;
+/* Set new mode endianess */
+env->uncached_cpsr = (env->uncached_cpsr & ~(CPSR_E)) |
+(env->cp15.sctlr_el[arm_current_el(env)] & SCTLR_EE ? CPSR_E : 0);
env->daif |= mask;
/* this is a lie, as the was no c1_sys on V4T/V5, but who cares
* and we should just guard the thumb mode on V4 */
@@ -5958,6 +5961,12 @@ static inline bool
regime_translation_disabled(CPUARMState *env,
return (regime_sctlr(env, mmu_idx) & SCTLR_M) == 0;
}
+static inline bool regime_translation_big_endian(CPUARMState *env,
+ ARMMMUIdx mmu_idx)
+{
+return (regime_sctlr(env, mmu_idx) & SCTLR_EE) != 0;
+}
+
/* Return the TCR controlling this translation regime */
static inline TCR *regime_tcr(CPUARMState *env, ARMMMUIdx mmu_idx)
{
@@ -6263,7 +6272,7 @@ static hwaddr S1_ptw_translate(CPUARMState *env,
ARMMMUIdx mmu_idx,
*/
static uint32_t arm_ldl_ptw(CPUState *cs, hwaddr addr, bool is_secure,
ARMMMUIdx mmu_idx, uint32_t *fsr,
-ARMMMUFaultInfo *fi)
+ARMMMUFaultInfo *fi, bool be)
{
ARMCPU *cpu = ARM_CPU(cs);
CPUARMState *env = &cpu->env;
@@ -6274,12 +6283,16 @@ static uint32_t arm_ldl_ptw(CPUState *cs, hwaddr addr,
bool is_secure,
if (fi->s1ptw) {
return 0;
}
-return address_space_ldl(cs->as, addr, attrs, NULL);
+if (be) {
+return address_space_ldl_be(cs->as, addr, attrs, NULL);
+} else {
+return address_space_ldl_le(cs->as, addr, attrs, NULL);
+}
}
static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure,
ARMMMUIdx mmu_idx, uint32_t *fsr,
-ARMMMUFaultInfo *fi)
+ARMMMUFaultInfo *fi, bool be)
{
ARMCPU *cpu = ARM_CPU(cs);
CPUARMState *env = &cpu->env;
@@ -6290,7 +6303,11 @@ static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr,
bool is_secure,
if (fi->s1ptw) {
return 0;
}
-return address_space_ldq(cs->as, addr, attrs, NULL);
+if (be) {
+return address_space_ldq_be(cs->as, addr, attrs, NULL);
+} else {
+return address_space_ldq_le(cs->as, addr, attrs, NULL);
+}
}
static bool get_phys_addr_v5(CPUARMState *env, uint32_t address,
@@ -6318,7 +6335,8 @@ static bool get_phys_addr_v5(CPUARMState *env, uint32_t
address,
goto do_fault;
}
desc = arm_ldl_ptw(cs, table, regime_is_secure(env, mmu_idx),
- mmu_idx, fsr, fi);
+ mmu_idx, fsr, fi,
+ regime_translation_big_endian(env, mmu_idx));
type = (desc & 3);
domain = (desc >> 5) & 0x0f;
if (regime_el(env, mmu_idx) == 1) {
@@ -6355,7 +6373,8 @@ static bool get_phys_addr_v5(CPUARMState *env, uint32_t
address,
table = (desc & 0xf000) | ((address >> 8) & 0xffc);
}
desc = arm_ldl_ptw(cs, table, regime_is_secure(env, mmu_idx),
- mmu_idx, fsr, fi);
+ mmu_idx, fsr, fi,
+ regime_translation_big_endian(env, mmu_idx));
switch (desc & 3) {
case 0: /* Page translation fault. */
code = 7;
@@ -6437,7 +6456,8 @@ static bool get_phys_addr_v6(CPUARMState *env, uint32_t
address,
goto do_fault;
}
desc = arm_ldl_ptw(cs, table, regime_is_secure(env, mmu_idx),
- mmu_idx, fsr, fi);
+ mmu_idx, fsr, fi,
+ regime_translation_big_endian(env, mmu_idx));
type = (desc & 3);
if (type == 0 || (type == 3 && !arm_feature(env, ARM_FEATURE_PXN))) {
/* Section translation fault, or attempt to use the encoding
@@ -6489,7 +6509,8 @@ static bool get_phys_addr_v6(CPUARMState *env, uint32_t
address,
/* Lookup l2 entry. */
table = (desc & 0xfc00) | ((address >> 10) & 0x3fc);
desc = arm_ldl_ptw(cs, table, regime_is_secure(env, mmu_idx),
- mmu_idx, fsr, fi);
+ mmu_idx, fsr, fi,
+ regime_transla
