Re: [Qemu-devel] [PATCH v1 04/16] target-arm: Make far_el1 an array

[Greg Bellows]

On 3 June 2014 05:21, Alex Bennée <address@hidden> wrote:

Edgar E. Iglesias writes:

> From: "Edgar E. Iglesias" <address@hidden>
>
> No functional change.
> Prepares for future additions of the EL2 and 3 versions of this reg.
>
> Signed-off-by: Edgar E. Iglesias <address@hidden>
> ---
>  target-arm/cpu.c        |  2 +-
>  target-arm/cpu.h        |  2 +-
>  target-arm/helper-a64.c |  4 ++--
>  target-arm/helper.c     | 12 ++++++------
>  4 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 794dcb9..93bd6a0 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -446,7 +446,7 @@ static void arm1026_initfn(Object *obj)
>          ARMCPRegInfo ifar = {
>              .name = "IFAR", .cp = 15, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 1,
>              .access = PL1_RW,
> -            .fieldoffset = offsetofhigh32(CPUARMState, cp15.far_el1),
> +            .fieldoffset = offsetofhigh32(CPUARMState, cp15.far_el[1]),
>              .resetvalue = 0
>          };
>          define_one_arm_cp_reg(cpu, &ifar);
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 8d04385..172a631 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -187,7 +187,7 @@ typedef struct CPUARMState {
>          uint32_t ifsr_el2; /* Fault status registers.  */
>          uint64_t esr_el[2];
>          uint32_t c6_region[8]; /* MPU base/size registers.  */
> -        uint64_t far_el1; /* Fault address registers.  */
> +        uint64_t far_el[2]; /* Fault address registers.  */

If there are EL1, 2 and 3 versions shouldn't this be [3]?

Or [4]?  Even if we don't use all the EL slots, it would make for more readable and consistent code if we kept the indices consistent with the level value.  Otherwise, as we discussed previously, we end up with different numbering schemes depending on the register.

 

>          uint64_t par_el1;  /* Translation result. */
>          uint32_t c9_insn; /* Cache lockdown registers.  */
>          uint32_t c9_data;
> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> index bc153cb..d647441 100644
> --- a/target-arm/helper-a64.c
> +++ b/target-arm/helper-a64.c
> @@ -465,13 +465,13 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>      }
>
>      env->cp15.esr_el[1] = env->exception.syndrome;
> -    env->cp15.far_el1 = env->exception.vaddress;
> +    env->cp15.far_el[1] = env->exception.vaddress;
>
>      switch (cs->exception_index) {
>      case EXCP_PREFETCH_ABORT:
>      case EXCP_DATA_ABORT:
>          qemu_log_mask(CPU_LOG_INT, "...with FAR 0x%" PRIx64 "\n",
> -                      env->cp15.far_el1);
> +                      env->cp15.far_el[1]);

As there is no FAR_EL0 shouldn't this be the first in the array (ie. 0?)

See above comment.

 

>          break;
>      case EXCP_BKPT:
>      case EXCP_UDEF:
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index ec031f5..5350a99 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -521,7 +521,7 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
>        .access = PL0_W, .type = ARM_CP_NOP },
>      { .name = "IFAR", .cp = 15, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 2,
>        .access = PL1_RW,
> -      .fieldoffset = offsetofhigh32(CPUARMState, cp15.far_el1),
> +      .fieldoffset = offsetofhigh32(CPUARMState, cp15.far_el[1]),
>        .resetvalue = 0, },
>      /* Watchpoint Fault Address Register : should actually only be present
>       * for 1136, 1176, 11MPCore.
> @@ -1505,7 +1505,7 @@ static const ARMCPRegInfo vmsa_cp_reginfo[] = {
>      /* 64-bit FAR; this entry also gives us the AArch32 DFAR */
>      { .name = "FAR_EL1", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 0,
> -      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.far_el1),
> +      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.far_el[1]),
>        .resetvalue = 0, },
>      REGINFO_SENTINEL
>  };
> @@ -3414,8 +3414,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
>          /* Fall through to prefetch abort.  */
>      case EXCP_PREFETCH_ABORT:
>          env->cp15.ifsr_el2 = env->exception.fsr;
> -        env->cp15.far_el1 = deposit64(env->cp15.far_el1, 32, 32,
> -                                      env->exception.vaddress);
> +        env->cp15.far_el[1] = deposit64(env->cp15.far_el[1], 32, 32,
> +                                        env->exception.vaddress);
>          qemu_log_mask(CPU_LOG_INT, "...with IFSR 0x%x IFAR 0x%x\n",
>                        env->cp15.ifsr_el2, (uint32_t)env->exception.vaddress);
>          new_mode = ARM_CPU_MODE_ABT;
> @@ -3425,8 +3425,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
>          break;
>      case EXCP_DATA_ABORT:
>          env->cp15.esr_el[1] = env->exception.fsr;
> -        env->cp15.far_el1 = deposit64(env->cp15.far_el1, 0, 32,
> -                                      env->exception.vaddress);
> +        env->cp15.far_el[1] = deposit64(env->cp15.far_el[1], 0, 32,
> +                                        env->exception.vaddress);
>          qemu_log_mask(CPU_LOG_INT, "...with DFSR 0x%x DFAR 0x%x\n",
>                        (uint32_t)env->cp15.esr_el[1],
>                        (uint32_t)env->exception.vaddress);

--

Alex Bennée