diff mbox

[1/4] target-arm: Correctly reset SCTLR_EL3 for 64-bit CPUs

Message ID 1459435778-5526-2-git-send-email-peter.maydell@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Maydell March 31, 2016, 2:49 p.m. UTC
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(-)

Comments

Laurent Desnogues March 31, 2016, 2:59 p.m. UTC | #1
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
>
>
sergey.fedorov@linaro.org April 4, 2016, 2:13 p.m. UTC | #2
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 mbox

Patch

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: