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

[Alex Bennée]

Greg Bellows writes:

> 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.

Makes sense although it would be nice to mark the unused ones somehow
for debugging.

>
>
>> >          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
>>

-- 
Alex Bennée