| Message ID | 1459435778-5526-2-git-send-email-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 31, 2016 at 4:49 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > The regdef for SCTRL_EL3 was incorrectly marked as being an > ARM_CP_ALIAS, with the remark that this was because the 32-bit > definition would take care of reset and migration. However the > intention for banked registers as documented in the comment in > add_cpreg_to_hashtable() is: > > * 2) If ARMv8 is enabled then we can count on a 64-bit version > * taking care of the secure bank. This requires that separate > * 32 and 64-bit definitions are provided. > > and so it marks the 32-bit secure banked version as an alias. > This results in the sctlr_s/sctlr_el[3] field never being reset > or migrated for a 64-bit CPU with EL3 enabled. > > Fix this by removing the ARM_CP_ALIAS annotation from SCTLR_EL3. > Since this means it now needs a real reset value, move the regdef > into the same place that we define the 32-bit SCTLR. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Laurent Desnogues <laurent.desnogues@gmail.com> Thanks, Laurent > --- > target-arm/helper.c | 23 +++++++++++++---------- > 1 file changed, 13 insertions(+), 10 deletions(-) > > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 19d5d52..e583e6a 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -3744,11 +3744,6 @@ static const ARMCPRegInfo el3_cp_reginfo[] = { > .access = PL1_RW, .accessfn = access_trap_aa32s_el1, > .writefn = vbar_write, .resetvalue = 0, > .fieldoffset = offsetof(CPUARMState, cp15.mvbar) }, > - { .name = "SCTLR_EL3", .state = ARM_CP_STATE_AA64, > - .type = ARM_CP_ALIAS, /* reset handled by AArch32 view */ > - .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 0, .opc2 = 0, > - .access = PL3_RW, .raw_writefn = raw_write, .writefn = sctlr_write, > - .fieldoffset = offsetof(CPUARMState, cp15.sctlr_el[3]) }, > { .name = "TTBR0_EL3", .state = ARM_CP_STATE_AA64, > .opc0 = 3, .opc1 = 6, .crn = 2, .crm = 0, .opc2 = 0, > .access = PL3_RW, .writefn = vmsa_ttbr_write, .resetvalue = 0, > @@ -4641,12 +4636,20 @@ void register_cp_regs_for_features(ARMCPU *cpu) > } > if (arm_feature(env, ARM_FEATURE_EL3)) { > define_arm_cp_regs(cpu, el3_cp_reginfo); > - ARMCPRegInfo rvbar = { > - .name = "RVBAR_EL3", .state = ARM_CP_STATE_AA64, > - .opc0 = 3, .opc1 = 6, .crn = 12, .crm = 0, .opc2 = 1, > - .type = ARM_CP_CONST, .access = PL3_R, .resetvalue = cpu->rvbar > + ARMCPRegInfo el3_regs[] = { > + { .name = "RVBAR_EL3", .state = ARM_CP_STATE_AA64, > + .opc0 = 3, .opc1 = 6, .crn = 12, .crm = 0, .opc2 = 1, > + .type = ARM_CP_CONST, .access = PL3_R, .resetvalue = cpu->rvbar }, > + { .name = "SCTLR_EL3", .state = ARM_CP_STATE_AA64, > + .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 0, .opc2 = 0, > + .access = PL3_RW, > + .raw_writefn = raw_write, .writefn = sctlr_write, > + .fieldoffset = offsetof(CPUARMState, cp15.sctlr_el[3]), > + .resetvalue = cpu->reset_sctlr }, > + REGINFO_SENTINEL > }; > - define_one_arm_cp_reg(cpu, &rvbar); > + > + define_arm_cp_regs(cpu, el3_regs); > } > /* The behaviour of NSACR is sufficiently various that we don't > * try to describe it in a single reginfo: > -- > 1.9.1 > >
On 31/03/16 17:49, Peter Maydell wrote: > The regdef for SCTRL_EL3 was incorrectly marked as being an > ARM_CP_ALIAS, with the remark that this was because the 32-bit > definition would take care of reset and migration. However the > intention for banked registers as documented in the comment in > add_cpreg_to_hashtable() is: > > * 2) If ARMv8 is enabled then we can count on a 64-bit version > * taking care of the secure bank. This requires that separate > * 32 and 64-bit definitions are provided. > > and so it marks the 32-bit secure banked version as an alias. > This results in the sctlr_s/sctlr_el[3] field never being reset > or migrated for a 64-bit CPU with EL3 enabled. > > Fix this by removing the ARM_CP_ALIAS annotation from SCTLR_EL3. > Since this means it now needs a real reset value, move the regdef > into the same place that we define the 32-bit SCTLR. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Sergey Fedorov <sergey.fedorov@linaro.org> > --- > target-arm/helper.c | 23 +++++++++++++---------- > 1 file changed, 13 insertions(+), 10 deletions(-) > > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 19d5d52..e583e6a 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -3744,11 +3744,6 @@ static const ARMCPRegInfo el3_cp_reginfo[] = { > .access = PL1_RW, .accessfn = access_trap_aa32s_el1, > .writefn = vbar_write, .resetvalue = 0, > .fieldoffset = offsetof(CPUARMState, cp15.mvbar) }, > - { .name = "SCTLR_EL3", .state = ARM_CP_STATE_AA64, > - .type = ARM_CP_ALIAS, /* reset handled by AArch32 view */ > - .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 0, .opc2 = 0, > - .access = PL3_RW, .raw_writefn = raw_write, .writefn = sctlr_write, > - .fieldoffset = offsetof(CPUARMState, cp15.sctlr_el[3]) }, > { .name = "TTBR0_EL3", .state = ARM_CP_STATE_AA64, > .opc0 = 3, .opc1 = 6, .crn = 2, .crm = 0, .opc2 = 0, > .access = PL3_RW, .writefn = vmsa_ttbr_write, .resetvalue = 0, > @@ -4641,12 +4636,20 @@ void register_cp_regs_for_features(ARMCPU *cpu) > } > if (arm_feature(env, ARM_FEATURE_EL3)) { > define_arm_cp_regs(cpu, el3_cp_reginfo); > - ARMCPRegInfo rvbar = { > - .name = "RVBAR_EL3", .state = ARM_CP_STATE_AA64, > - .opc0 = 3, .opc1 = 6, .crn = 12, .crm = 0, .opc2 = 1, > - .type = ARM_CP_CONST, .access = PL3_R, .resetvalue = cpu->rvbar > + ARMCPRegInfo el3_regs[] = { > + { .name = "RVBAR_EL3", .state = ARM_CP_STATE_AA64, > + .opc0 = 3, .opc1 = 6, .crn = 12, .crm = 0, .opc2 = 1, > + .type = ARM_CP_CONST, .access = PL3_R, .resetvalue = cpu->rvbar }, > + { .name = "SCTLR_EL3", .state = ARM_CP_STATE_AA64, > + .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 0, .opc2 = 0, > + .access = PL3_RW, > + .raw_writefn = raw_write, .writefn = sctlr_write, > + .fieldoffset = offsetof(CPUARMState, cp15.sctlr_el[3]), > + .resetvalue = cpu->reset_sctlr }, > + REGINFO_SENTINEL > }; > - define_one_arm_cp_reg(cpu, &rvbar); > + > + define_arm_cp_regs(cpu, el3_regs); > } > /* The behaviour of NSACR is sufficiently various that we don't > * try to describe it in a single reginfo:
diff --git a/target-arm/helper.c b/target-arm/helper.c index 19d5d52..e583e6a 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -3744,11 +3744,6 @@ static const ARMCPRegInfo el3_cp_reginfo[] = { .access = PL1_RW, .accessfn = access_trap_aa32s_el1, .writefn = vbar_write, .resetvalue = 0, .fieldoffset = offsetof(CPUARMState, cp15.mvbar) }, - { .name = "SCTLR_EL3", .state = ARM_CP_STATE_AA64, - .type = ARM_CP_ALIAS, /* reset handled by AArch32 view */ - .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 0, .opc2 = 0, - .access = PL3_RW, .raw_writefn = raw_write, .writefn = sctlr_write, - .fieldoffset = offsetof(CPUARMState, cp15.sctlr_el[3]) }, { .name = "TTBR0_EL3", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 6, .crn = 2, .crm = 0, .opc2 = 0, .access = PL3_RW, .writefn = vmsa_ttbr_write, .resetvalue = 0, @@ -4641,12 +4636,20 @@ void register_cp_regs_for_features(ARMCPU *cpu) } if (arm_feature(env, ARM_FEATURE_EL3)) { define_arm_cp_regs(cpu, el3_cp_reginfo); - ARMCPRegInfo rvbar = { - .name = "RVBAR_EL3", .state = ARM_CP_STATE_AA64, - .opc0 = 3, .opc1 = 6, .crn = 12, .crm = 0, .opc2 = 1, - .type = ARM_CP_CONST, .access = PL3_R, .resetvalue = cpu->rvbar + ARMCPRegInfo el3_regs[] = { + { .name = "RVBAR_EL3", .state = ARM_CP_STATE_AA64, + .opc0 = 3, .opc1 = 6, .crn = 12, .crm = 0, .opc2 = 1, + .type = ARM_CP_CONST, .access = PL3_R, .resetvalue = cpu->rvbar }, + { .name = "SCTLR_EL3", .state = ARM_CP_STATE_AA64, + .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 0, .opc2 = 0, + .access = PL3_RW, + .raw_writefn = raw_write, .writefn = sctlr_write, + .fieldoffset = offsetof(CPUARMState, cp15.sctlr_el[3]), + .resetvalue = cpu->reset_sctlr }, + REGINFO_SENTINEL }; - define_one_arm_cp_reg(cpu, &rvbar); + + define_arm_cp_regs(cpu, el3_regs); } /* The behaviour of NSACR is sufficiently various that we don't * try to describe it in a single reginfo:
The regdef for SCTRL_EL3 was incorrectly marked as being an ARM_CP_ALIAS, with the remark that this was because the 32-bit definition would take care of reset and migration. However the intention for banked registers as documented in the comment in add_cpreg_to_hashtable() is: * 2) If ARMv8 is enabled then we can count on a 64-bit version * taking care of the secure bank. This requires that separate * 32 and 64-bit definitions are provided. and so it marks the 32-bit secure banked version as an alias. This results in the sctlr_s/sctlr_el[3] field never being reset or migrated for a 64-bit CPU with EL3 enabled. Fix this by removing the ARM_CP_ALIAS annotation from SCTLR_EL3. Since this means it now needs a real reset value, move the regdef into the same place that we define the 32-bit SCTLR. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target-arm/helper.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-)