Re: [Qemu-devel] [PATCH v8 25/27] target-arm: make c13 cp regs banked (F

On 31 October 2014 12:27, Peter Maydell <address@hidden> wrote:

On 30 October 2014 21:28, Greg Bellows <address@hidden> wrote:
> From: Fabian Aggeler <address@hidden>
>
> When EL3 is running in AArch32 (or ARMv7 with Security Extensions)
> FCSEIDR, CONTEXTIDR, TPIDRURW, TPIDRURO and TPIDRPRW have a secure
> and a non-secure instance.
>
> Signed-off-by: Fabian Aggeler <address@hidden>
> Signed-off-by: Greg Bellows <address@hidden>
>
> ---
>
> v6 -> v7
> - Fix linux-user/arm/target-cpu.h to use array based tpidr_el.
> - Fix linux-user/main.c to use array based tpidrro_el.
> - Remove tab identified by checkpatch failure.
> - FIx linux-user/aarch64/target_cpu.h to use array based tpidr_el.
>
> v5 -> v6
> - Changed _el field variants to be array based
> - Rework data layout for correct aliasing
> - Merged CONTEXTIDR and CONTEXTIDR_EL1 reginfo entries
>
> v3 -> v4
> - Fix tpidrprw mapping
> ---
> linux-user/aarch64/target_cpu.h | 2 +-
> linux-user/arm/target_cpu.h | 2 +-
> linux-user/main.c | 72 ++++++++++++++++++++---------------------
> target-arm/cpu.h | 35 +++++++++++++++++---
> target-arm/helper.c | 37 ++++++++++++---------
> target-arm/op_helper.c | 2 +-
> 6 files changed, 91 insertions(+), 59 deletions(-)
>
> diff --git a/linux-user/aarch64/target_cpu.h b/linux-user/aarch64/target_cpu.h
> index 21560ef..b5593dc 100644
> --- a/linux-user/aarch64/target_cpu.h
> +++ b/linux-user/aarch64/target_cpu.h
> @@ -32,7 +32,7 @@ static inline void cpu_set_tls(CPUARMState *env, target_ulong newtls)
> /* Note that AArch64 Linux keeps the TLS pointer in TPIDR; this is
> * different from AArch32 Linux, which uses TPIDRRO.
> */
> - env->cp15.tpidr_el0 = newtls;
> + env->cp15.tpidr_el[0] = newtls;
> }
>
> #endif
> diff --git a/linux-user/arm/target_cpu.h b/linux-user/arm/target_cpu.h
> index 39d65b6..d8a534d 100644
> --- a/linux-user/arm/target_cpu.h
> +++ b/linux-user/arm/target_cpu.h
> @@ -29,7 +29,7 @@ static inline void cpu_clone_regs(CPUARMState *env, target_ulong newsp)
>
> static inline void cpu_set_tls(CPUARMState *env, target_ulong newtls)
> {
> - env->cp15.tpidrro_el0 = newtls;
> + env->cp15.tpidrro_el[0] = newtls;
> }
>
> #endif
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 483eb3f..4f2bae2 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -564,7 +564,7 @@ do_kernel_trap(CPUARMState *env)
> end_exclusive();
> break;
> case 0xffff0fe0: /* __kernel_get_tls */
> - env->regs[0] = env->cp15.tpidrro_el0;
> + env->regs[0] = env->cp15.tpidrro_el[0];
> break;
> case 0xffff0f60: /* __kernel_cmpxchg64 */
> arm_kernel_cmpxchg64_helper(env);
> @@ -2804,7 +2804,7 @@ void cpu_loop(CPUCRISState *env)
> CPUState *cs = CPU(cris_env_get_cpu(env));
> int trapnr, ret;
> target_siginfo_t info;
> -
> +
> while (1) {
> trapnr = cpu_cris_exec (env);
> switch (trapnr) {

Stray whitespace change.

> @@ -2822,13 +2822,13 @@ void cpu_loop(CPUCRISState *env)
> /* just indicate that signals should be handled asap */
> break;
> case EXCP_BREAK:
> - ret = do_syscall(env,
> - env->regs[9],
> - env->regs[10],
> - env->regs[11],
> - env->regs[12],
> - env->regs[13],
> - env->pregs[7],
> + ret = do_syscall(env,
> + env->regs[9],
> + env->regs[10],
> + env->regs[11],
> + env->regs[12],
> + env->regs[13],
> + env->pregs[7],
> env->pregs[11],
> 0, 0);
> env->regs[10] = ret;

??? whitespace changes I guess? This patch shouldn't be touching
CRIS specific code... There's a bunch more whitespace changes
below too, please get rid of them all.

Yeah, my bad. I have a filter on my editor that fixes white space anomalies such as what checkpatch catches. Undid changes in v9.

> @@ -2863,7 +2863,7 @@ void cpu_loop(CPUMBState *env)
> CPUState *cs = CPU(mb_env_get_cpu(env));
> int trapnr, ret;
> target_siginfo_t info;
> -
> +
> while (1) {
> trapnr = cpu_mb_exec (env);
> switch (trapnr) {
> @@ -2884,13 +2884,13 @@ void cpu_loop(CPUMBState *env)
> /* Return address is 4 bytes after the call. */
> env->regs[14] += 4;
> env->sregs[SR_PC] = env->regs[14];
> - ret = do_syscall(env,
> - env->regs[12],
> - env->regs[5],
> - env->regs[6],
> - env->regs[7],
> - env->regs[8],
> - env->regs[9],
> + ret = do_syscall(env,
> + env->regs[12],
> + env->regs[5],
> + env->regs[6],
> + env->regs[7],
> + env->regs[8],
> + env->regs[9],
> env->regs[10],
> 0, 0);
> env->regs[3] = ret;
> @@ -3424,7 +3424,7 @@ void stop_all_tasks(void)
> void init_task_state(TaskState *ts)
> {
> int i;
> -
> +
> ts->used = 1;
> ts->first_free = ts->sigqueue_table;
> for (i = 0; i < MAX_SIGQUEUE_SIZE - 1; i++) {
> @@ -4271,23 +4271,23 @@ int main(int argc, char **argv, char **envp)
> env->regs[12] = regs->r12;
> env->regs[13] = regs->r13;
> env->regs[14] = regs->r14;
> - env->regs[15] = regs->r15;
> - env->regs[16] = regs->r16;
> - env->regs[17] = regs->r17;
> - env->regs[18] = regs->r18;
> - env->regs[19] = regs->r19;
> - env->regs[20] = regs->r20;
> - env->regs[21] = regs->r21;
> - env->regs[22] = regs->r22;
> - env->regs[23] = regs->r23;
> - env->regs[24] = regs->r24;
> - env->regs[25] = regs->r25;
> - env->regs[26] = regs->r26;
> - env->regs[27] = regs->r27;
> - env->regs[28] = regs->r28;
> - env->regs[29] = regs->r29;
> - env->regs[30] = regs->r30;
> - env->regs[31] = regs->r31;
> + env->regs[15] = regs->r15;
> + env->regs[16] = regs->r16;
> + env->regs[17] = regs->r17;
> + env->regs[18] = regs->r18;
> + env->regs[19] = regs->r19;
> + env->regs[20] = regs->r20;
> + env->regs[21] = regs->r21;
> + env->regs[22] = regs->r22;
> + env->regs[23] = regs->r23;
> + env->regs[24] = regs->r24;
> + env->regs[25] = regs->r25;
> + env->regs[26] = regs->r26;
> + env->regs[27] = regs->r27;
> + env->regs[28] = regs->r28;
> + env->regs[29] = regs->r29;
> + env->regs[30] = regs->r30;
> + env->regs[31] = regs->r31;
> env->sregs[SR_PC] = regs->pc;
> }
> #elif defined(TARGET_MIPS)
> @@ -4349,7 +4349,7 @@ int main(int argc, char **argv, char **envp)
> env->regs[12] = regs->r12;
> env->regs[13] = regs->r13;
> env->regs[14] = info->start_stack;
> - env->regs[15] = regs->acr;
> + env->regs[15] = regs->acr;
> env->pc = regs->erp;
> }
> #elif defined(TARGET_S390X)
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index e0954c7..348ce73 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -316,11 +316,36 @@ typedef struct CPUARMState {
> uint64_t vbar_el[4];
> };
> uint32_t mvbar; /* (monitor) vector base address register */
> - uint32_t c13_fcse; /* FCSE PID. */
> - uint64_t contextidr_el1; /* Context ID. */
> - uint64_t tpidr_el0; /* User RW Thread register. */
> - uint64_t tpidrro_el0; /* User RO Thread register. */
> - uint64_t tpidr_el1; /* Privileged Thread register. */
> + struct { /* FCSE PID. */
> + uint32_t fcseidr_ns;
> + uint32_t fcseidr_s;
> + };
> + union { /* Context ID. */
> + struct {
> + uint64_t _unused_contextidr;
> + uint64_t contextidr_ns;
> + uint64_t contextidr_s;
> + };
> + uint64_t contextidr_el[2];

Union of three uint64_t fields with an array with only 2
entries? That looks fishy.

I actually did this on purpose because there is no contextidr_el[2 or 3]. This is inconsistent with how I did it elsewhere though. Fixed it in v9.

> + };
> + union { /* User RW Thread register. */
> + struct {
> + uint64_t tpidrurw_ns;
> + uint64_t tpidrprw_ns;
> + uint64_t htpidr;
> + uint64_t _tpidr_el3;
> + };
> + uint64_t tpidr_el[4];
> + };
> + /* The secure banks of these registers don't map anywhere */
> + uint64_t tpidrurw_s;
> + uint64_t tpidrprw_s;
> + uint64_t tpidruro_s;
> +
> + union { /* User RO Thread register. */
> + uint64_t tpidruro_ns;
> + uint64_t tpidrro_el[1];

A one-element array??

This was done to remain consistent with how all the other _el[n] fields worked. Not my favorite either, but it keeps with the pattern.

Change it?

> + };
> uint64_t c14_cntfrq; /* Counter Frequency register */
> uint64_t c14_cntkctl; /* Timer Control register */
> ARMGenericTimer c14_timer[NUM_GTIMERS];
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index fb040d4..d782897 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -420,12 +420,15 @@ static void tlbimvaa_is_write(CPUARMState *env, const ARMCPRegInfo *ri,
>
> static const ARMCPRegInfo cp_reginfo[] = {
> { .name = "FCSEIDR", .cp = 15, .crn = 13, .crm = 0, .opc1 = 0, .opc2 = 0,
> - .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c13_fcse),
> + .access = PL1_RW,
> + .bank_fieldoffsets = { offsetof(CPUARMState, cp15.fcseidr_s),
> + offsetof(CPUARMState, cp15.fcseidr_ns) },
> .resetvalue = 0, .writefn = fcse_write, .raw_writefn = raw_write, },
> { .name = "CONTEXTIDR", .state = ARM_CP_STATE_BOTH,
> - .opc0 = 3, .opc1 = 0, .crn = 13, .crm = 0, .opc2 = 1,
> + .cp = 15, .opc1 = 0, .crn = 13, .crm = 0, .opc2 = 1,

Lost .opc3 again, and you don't need to specify .cp = 15 for a BOTH entry.

> .access = PL1_RW,
> - .fieldoffset = offsetof(CPUARMState, cp15.contextidr_el1),
> + .bank_fieldoffsets = { offsetof(CPUARMState, cp15.contextidr_s),
> + offsetof(CPUARMState, cp15.contextidr_ns) },
> .resetvalue = 0, .writefn = contextidr_write, .raw_writefn = raw_write, },
> REGINFO_SENTINEL
> };
> @@ -1025,23 +1028,27 @@ static const ARMCPRegInfo v6k_cp_reginfo[] = {
> { .name = "TPIDR_EL0", .state = ARM_CP_STATE_AA64,
> .opc0 = 3, .opc1 = 3, .opc2 = 2, .crn = 13, .crm = 0,
> .access = PL0_RW,
> - .fieldoffset = offsetof(CPUARMState, cp15.tpidr_el0), .resetvalue = 0 },
> + .fieldoffset = offsetof(CPUARMState, cp15.tpidr_el[0]), .resetvalue = 0 },
> { .name = "TPIDRURW", .cp = 15, .crn = 13, .crm = 0, .opc1 = 0, .opc2 = 2,
> - .access = PL0_RW,
> - .fieldoffset = offsetoflow32(CPUARMState, cp15.tpidr_el0),
> - .resetfn = arm_cp_reset_ignore },
> + .access = PL0_RW, .resetvalue = 0,
> + .bank_fieldoffsets = { offsetoflow32(CPUARMState, cp15.tpidrurw_s),
> + offsetoflow32(CPUARMState, cp15.tpidrurw_ns) } },
> { .name = "TPIDRRO_EL0", .state = ARM_CP_STATE_AA64,
> .opc0 = 3, .opc1 = 3, .opc2 = 3, .crn = 13, .crm = 0,
> - .access = PL0_R|PL1_W,
> - .fieldoffset = offsetof(CPUARMState, cp15.tpidrro_el0), .resetvalue = 0 },
> + .access = PL0_R|PL1_W, .resetvalue = 0,
> + .fieldoffset = offsetof(CPUARMState, cp15.tpidrro_el[0]) },
> { .name = "TPIDRURO", .cp = 15, .crn = 13, .crm = 0, .opc1 = 0, .opc2 = 3,
> - .access = PL0_R|PL1_W,
> - .fieldoffset = offsetoflow32(CPUARMState, cp15.tpidrro_el0),
> - .resetfn = arm_cp_reset_ignore },
> - { .name = "TPIDR_EL1", .state = ARM_CP_STATE_BOTH,
> + .access = PL0_R|PL1_W, .resetvalue = 0,
> + .bank_fieldoffsets = { offsetoflow32(CPUARMState, cp15.tpidruro_s),
> + offsetoflow32(CPUARMState, cp15.tpidruro_ns) } },

This diff is hard to read because you've moved .resetvalue around in the
struct, so the changed lines are more than have actually changed semantically.

Undid authors field rearranging in v9.

> + { .name = "TPIDR_EL1", .state = ARM_CP_STATE_AA64,
> .opc0 = 3, .opc1 = 0, .opc2 = 4, .crn = 13, .crm = 0,
> .access = PL1_RW,
> - .fieldoffset = offsetof(CPUARMState, cp15.tpidr_el1), .resetvalue = 0 },
> + .fieldoffset = offsetof(CPUARMState, cp15.tpidr_el[1]), .resetvalue = 0 },
> + { .name = "TPIDRPRW", .cp = 15, .crn = 13, .crm = 0, .opc1 = 0, .opc2 = 4,
> + .access = PL1_RW, .resetvalue = 0,
> + .bank_fieldoffsets = { offsetoflow32(CPUARMState, cp15.tpidrprw_s),
> + offsetoflow32(CPUARMState, cp15.tpidrprw_ns) } },
> REGINFO_SENTINEL
> };
>
> @@ -5045,7 +5052,7 @@ static inline int get_phys_addr(CPUARMState *env, target_ulong address,
>
> /* Fast Context Switch Extension. */
> if (address < 0x02000000)
> - address += env->cp15.c13_fcse;
> + address += A32_BANKED_CURRENT_REG_GET(env, fcseidr);

Add braces while you're changing this line.

Done in v9.

> if ((sctlr & SCTLR_M) == 0) {
> /* MMU/MPU disabled. */
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index a8dea5a..2bed914 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -575,7 +575,7 @@ static bool linked_bp_matches(ARMCPU *cpu, int lbn)
> * short descriptor format (in which case it holds both PROCID and ASID),
> * since we don't implement the optional v7 context ID masking.
> */
> - contextidr = extract64(env->cp15.contextidr_el1, 0, 32);
> + contextidr = extract64(env->cp15.contextidr_el[1], 0, 32);
>
> switch (bt) {
> case 3: /* linked context ID match */
> --
> 1.8.3.2

thanks
-- PMM

From:	Greg Bellows
Subject:	Re: [Qemu-devel] [PATCH v8 25/27] target-arm: make c13 cp regs banked (FCSEIDR, ...)
Date:	Mon, 3 Nov 2014 16:57:55 -0600