On 31 October 2014 12:27, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 30 October 2014 21:28, Greg Bellows <greg.bell...@linaro.org> wrote:
> > From: Fabian Aggeler <aggel...@ethz.ch>
> >
> > 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 <aggel...@ethz.ch>
> > Signed-off-by: Greg Bellows <greg.bell...@linaro.org>
> >
> > ---
> >
> > 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
>
