Re: [Qemu-devel] [PATCH v8 13/27] target-arm: add SCTLR_EL3 and make SCTLR banked

[Greg Bellows]

Not sure of the exact reasoning as I inherited the change.  However, when I went through this code before I took it that the change was needed to filter out the case where SCTLR may be either of the ARMv8 variants (SCTLR_EL1 or SCTLR_EL3) as neither of them have a SCTLR_V bits.  In fact, looking quickly through the ARMv8 ARM, I don't see any mention of hivec support for AArch64.

I think the more appropriate check in this case is to check whether the current EL is 32-bit instead of ARMv8.   Made this change in v9 along with a comment.

I'll leave your review-by off until you okay this.

Greg

On 31 October 2014 09:07, Peter Maydell <address@hidden> wrote:

On 30 October 2014 21:28, Greg Bellows <address@hidden> wrote:
> From: Fabian Aggeler <address@hidden>
>
> Implements SCTLR_EL3 and uses secure/non-secure instance when
> needed.
>
> Signed-off-by: Fabian Aggeler <address@hidden>
> Signed-off-by: Greg Bellows <address@hidden>

> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index e0b82a6..18f4726 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -109,7 +109,7 @@ static void arm_cpu_reset(CPUState *s)
>  #if defined(CONFIG_USER_ONLY)
>          env->pstate = PSTATE_MODE_EL0t;
>          /* Userspace expects access to DC ZVA, CTL_EL0 and the cache ops */
> -        env->cp15.c1_sys |= SCTLR_UCT | SCTLR_UCI | SCTLR_DZE;
> +        env->cp15.sctlr_el[1] |= SCTLR_UCT | SCTLR_UCI | SCTLR_DZE;
>          /* and to the FP/Neon instructions */
>          env->cp15.c1_coproc = deposit64(env->cp15.c1_coproc, 20, 2, 3);
>  #else
> @@ -167,7 +167,8 @@ static void arm_cpu_reset(CPUState *s)
>          env->thumb = initial_pc & 1;
>      }
>
> -    if (env->cp15.c1_sys & SCTLR_V) {
> +    if (!arm_feature(env, ARM_FEATURE_V8)
> +            && (A32_BANKED_CURRENT_REG_GET(env, sctlr) & SCTLR_V)) {
>          env->regs[15] = 0xFFFF0000;

Why has this condition had an "if not v8" added to it? The v8
spec doesn't drop support for hivecs as far as I can tell...

Patch looks good otherwise.

-- PMM