| Message ID | 1391183143-30724-20-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Fri, Jan 31, 2014 at 03:45:27PM +0000, Peter Maydell wrote: > Make the cache ID system registers (CLIDR, CCSELR, CCSIDR, CTR) s/CCSELR/CSSELR/ > visible to AArch64. These are mostly simple 64-bit extensions of the > existing 32 bit system registers and so can share reginfo definitions. According to the document(ARM DDI 0487A.a), some AArch64 system registers are 32-bit, for example CCSIDR_EL1 is 32-bit. But System_Put() and System_Get() writes/reads 64-bit values, which makes me confused. > CTR needs to have a split definition, but we can clean up the > temporary user-mode implementation in favour of using the CPU-specified > reset value, and implement the system-mode-required semantics of > restricting its EL0 accessibility if SCTLR.UCT is not set. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > target-arm/cpu.c | 2 ++ > target-arm/cpu.h | 2 +- > target-arm/cpu64.c | 1 + > target-arm/helper.c | 31 +++++++++++++++++++++---------- > 4 files changed, 25 insertions(+), 11 deletions(-) > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > index fe18b65..8fed098 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -91,6 +91,8 @@ static void arm_cpu_reset(CPUState *s) > env->aarch64 = 1; > #if defined(CONFIG_USER_ONLY) > env->pstate = PSTATE_MODE_EL0t; > + /* Userspace expects access to CTL_EL0 */ > + env->cp15.c1_sys |= SCTLR_UCT; > #else > env->pstate = PSTATE_D | PSTATE_A | PSTATE_I | PSTATE_F > | PSTATE_MODE_EL1h; > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index d1ed423..f5b706e 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -166,7 +166,7 @@ typedef struct CPUARMState { > /* System control coprocessor (cp15) */ > struct { > uint32_t c0_cpuid; > - uint32_t c0_cssel; /* Cache size selection. */ > + uint64_t c0_cssel; /* Cache size selection. */ I see all backing fields for AArch64 system registers are converted to uint64_t, why not convert ARMCPU.ccsidr which backs CCSIDR?
On 7 February 2014 07:35, Hu Tao <hutao@cn.fujitsu.com> wrote: > On Fri, Jan 31, 2014 at 03:45:27PM +0000, Peter Maydell wrote: >> Make the cache ID system registers (CLIDR, CCSELR, CCSIDR, CTR) > > s/CCSELR/CSSELR/ > >> visible to AArch64. These are mostly simple 64-bit extensions of the >> existing 32 bit system registers and so can share reginfo definitions. > > According to the document(ARM DDI 0487A.a), some AArch64 system > registers are 32-bit, for example CCSIDR_EL1 is 32-bit. But System_Put() > and System_Get() writes/reads 64-bit values, which makes me confused. We've been round this issue before. The documentation uses "32-bit" as a shorthand for "64 bit but the top 32 bits are RES0". There is no way in AArch64 to do a 32 bit read or write of a system register -- the only instructions are MSR/MRS, which are always 64 bits. Similarly, the KVM kernel API exposes all registers as 64 bits wide. We could in theory have a mechanism that allowed a 64 bit system register access to be backed by a 32 bit field value, but it's simpler not to. >> CTR needs to have a split definition, but we can clean up the >> temporary user-mode implementation in favour of using the CPU-specified >> reset value, and implement the system-mode-required semantics of >> restricting its EL0 accessibility if SCTLR.UCT is not set. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> target-arm/cpu.c | 2 ++ >> target-arm/cpu.h | 2 +- >> target-arm/cpu64.c | 1 + >> target-arm/helper.c | 31 +++++++++++++++++++++---------- >> 4 files changed, 25 insertions(+), 11 deletions(-) >> >> diff --git a/target-arm/cpu.c b/target-arm/cpu.c >> index fe18b65..8fed098 100644 >> --- a/target-arm/cpu.c >> +++ b/target-arm/cpu.c >> @@ -91,6 +91,8 @@ static void arm_cpu_reset(CPUState *s) >> env->aarch64 = 1; >> #if defined(CONFIG_USER_ONLY) >> env->pstate = PSTATE_MODE_EL0t; >> + /* Userspace expects access to CTL_EL0 */ >> + env->cp15.c1_sys |= SCTLR_UCT; >> #else >> env->pstate = PSTATE_D | PSTATE_A | PSTATE_I | PSTATE_F >> | PSTATE_MODE_EL1h; >> diff --git a/target-arm/cpu.h b/target-arm/cpu.h >> index d1ed423..f5b706e 100644 >> --- a/target-arm/cpu.h >> +++ b/target-arm/cpu.h >> @@ -166,7 +166,7 @@ typedef struct CPUARMState { >> /* System control coprocessor (cp15) */ >> struct { >> uint32_t c0_cpuid; >> - uint32_t c0_cssel; /* Cache size selection. */ >> + uint64_t c0_cssel; /* Cache size selection. */ > > I see all backing fields for AArch64 system registers are converted to > uint64_t, why not convert ARMCPU.ccsidr which backs CCSIDR? It's not directly accessed via a .fieldoffset() entry, so there's no requirement for it to be 64 bits wide. I've also generally left the ARMCPU values which are just constants to be used in ".resetvalue =" specifications alone. It's only the cases where a struct field is the backing storage for the register that need widening. thanks -- PMM
On Sat, Feb 1, 2014 at 1:45 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > Make the cache ID system registers (CLIDR, CCSELR, CCSIDR, CTR) > visible to AArch64. These are mostly simple 64-bit extensions of the > existing 32 bit system registers and so can share reginfo definitions. > CTR needs to have a split definition, but we can clean up the > temporary user-mode implementation in favour of using the CPU-specified > reset value, and implement the system-mode-required semantics of > restricting its EL0 accessibility if SCTLR.UCT is not set. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > target-arm/cpu.c | 2 ++ > target-arm/cpu.h | 2 +- > target-arm/cpu64.c | 1 + > target-arm/helper.c | 31 +++++++++++++++++++++---------- > 4 files changed, 25 insertions(+), 11 deletions(-) > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > index fe18b65..8fed098 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -91,6 +91,8 @@ static void arm_cpu_reset(CPUState *s) > env->aarch64 = 1; > #if defined(CONFIG_USER_ONLY) > env->pstate = PSTATE_MODE_EL0t; > + /* Userspace expects access to CTL_EL0 */ > + env->cp15.c1_sys |= SCTLR_UCT; > #else > env->pstate = PSTATE_D | PSTATE_A | PSTATE_I | PSTATE_F > | PSTATE_MODE_EL1h; > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index d1ed423..f5b706e 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -166,7 +166,7 @@ typedef struct CPUARMState { > /* System control coprocessor (cp15) */ > struct { > uint32_t c0_cpuid; > - uint32_t c0_cssel; /* Cache size selection. */ > + uint64_t c0_cssel; /* Cache size selection. */ > uint32_t c1_sys; /* System control register. */ > uint32_t c1_coproc; /* Coprocessor access register. */ > uint32_t c1_xscaleauxcr; /* XScale auxiliary control register. */ > diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c > index a639c2e..8426bf1 100644 > --- a/target-arm/cpu64.c > +++ b/target-arm/cpu64.c > @@ -45,6 +45,7 @@ static void aarch64_any_initfn(Object *obj) > set_feature(&cpu->env, ARM_FEATURE_ARM_DIV); > set_feature(&cpu->env, ARM_FEATURE_V7MP); > set_feature(&cpu->env, ARM_FEATURE_AARCH64); > + cpu->ctr = 0x80030003; /* 32 byte I and D cacheline size, VIPT icache */ > } > #endif > > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 06331dd..f46dd0f 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -629,9 +629,11 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { > { .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0, > .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr), > .resetvalue = 0, }, > - { .name = "CCSIDR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0, > + { .name = "CCSIDR", .state = ARM_CP_STATE_BOTH, > + .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0, Why is the .cp field lost in the conversion to STATE_BOTH? Regards, Peter > .access = PL1_R, .readfn = ccsidr_read, .type = ARM_CP_NO_MIGRATE }, > - { .name = "CSSELR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 2, .opc2 = 0, > + { .name = "CSSELR", .state = ARM_CP_STATE_BOTH, > + .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 2, .opc2 = 0, > .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c0_cssel), > .writefn = csselr_write, .resetvalue = 0 }, > /* Auxiliary ID register: this actually has an IMPDEF value but for now > @@ -1524,13 +1526,6 @@ static const ARMCPRegInfo v8_cp_reginfo[] = { > { .name = "FPSR", .state = ARM_CP_STATE_AA64, > .opc0 = 3, .opc1 = 3, .opc2 = 1, .crn = 4, .crm = 4, > .access = PL0_RW, .readfn = aa64_fpsr_read, .writefn = aa64_fpsr_write }, > - /* This claims a 32 byte cacheline size for icache and dcache, VIPT icache. > - * It will eventually need to have a CPU-specified reset value. > - */ > - { .name = "CTR_EL0", .state = ARM_CP_STATE_AA64, > - .opc0 = 3, .opc1 = 3, .opc2 = 1, .crn = 0, .crm = 0, > - .access = PL0_R, .type = ARM_CP_CONST, > - .resetvalue = 0x80030003 }, > /* Prohibit use of DC ZVA. OPTME: implement DC ZVA and allow its use. > * For system mode the DZP bit here will need to be computed, not constant. > */ > @@ -1550,6 +1545,17 @@ static void sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri, > tlb_flush(env, 1); > } > > +static CPAccessResult ctr_el0_access(CPUARMState *env, const ARMCPRegInfo *ri) > +{ > + /* Only accessible in EL0 if SCTLR.UCT is set (and only in AArch64, > + * but the AArch32 CTR has its own reginfo struct) > + */ > + if (arm_current_pl(env) == 0 && !(env->cp15.c1_sys & SCTLR_UCT)) { > + return CP_ACCESS_TRAP; > + } > + return CP_ACCESS_OK; > +} > + > void register_cp_regs_for_features(ARMCPU *cpu) > { > /* Register all the coprocessor registers based on feature bits */ > @@ -1634,7 +1640,8 @@ void register_cp_regs_for_features(ARMCPU *cpu) > .raw_writefn = raw_write, > }; > ARMCPRegInfo clidr = { > - .name = "CLIDR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 1, > + .name = "CLIDR", .state = ARM_CP_STATE_BOTH, > + .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 1, > .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = cpu->clidr > }; > define_one_arm_cp_reg(cpu, &pmcr); > @@ -1713,6 +1720,10 @@ void register_cp_regs_for_features(ARMCPU *cpu) > { .name = "CTR", > .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 1, > .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = cpu->ctr }, > + { .name = "CTR_EL0", .state = ARM_CP_STATE_AA64, > + .opc0 = 3, .opc1 = 3, .opc2 = 1, .crn = 0, .crm = 0, > + .access = PL0_R, .accessfn = ctr_el0_access, > + .type = ARM_CP_CONST, .resetvalue = cpu->ctr }, > { .name = "TCMTR", > .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 2, > .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 0 }, > -- > 1.8.5 > >
On 9 February 2014 02:15, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Sat, Feb 1, 2014 at 1:45 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> - { .name = "CCSIDR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0, >> + { .name = "CCSIDR", .state = ARM_CP_STATE_BOTH, >> + .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0, > > Why is the .cp field lost in the conversion to STATE_BOTH? Because 64 bit sysregs don't have a concept of coprocessor. STATE_BOTH means "this is a 64 bit sysreg with a cp15 encoding in the obvious place". Anything other than cp15 needs split definitions anyway, so if we required the .cp to be specified it would always be '.cp = 15'. Essentially this is attempting to keep the length of cpreg definitions down by abbreviating parts where there isn't actually any choice. thanks -- PMM
On Sun, Feb 9, 2014 at 9:52 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 9 February 2014 02:15, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> On Sat, Feb 1, 2014 at 1:45 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> - { .name = "CCSIDR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0, >>> + { .name = "CCSIDR", .state = ARM_CP_STATE_BOTH, >>> + .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0, >> >> Why is the .cp field lost in the conversion to STATE_BOTH? > > Because 64 bit sysregs don't have a concept of coprocessor. STATE_BOTH > means "this is a 64 bit sysreg with a cp15 encoding in the obvious place". > Anything other than cp15 needs split definitions anyway, so if we required > the .cp to be specified it would always be '.cp = 15'. Essentially this is > attempting to keep the length of cpreg definitions down by abbreviating > parts where there isn't actually any choice. > Thanks, Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > thanks > -- PMM >
On Fri, Feb 07, 2014 at 10:27:35AM +0000, Peter Maydell wrote: > On 7 February 2014 07:35, Hu Tao <hutao@cn.fujitsu.com> wrote: > > On Fri, Jan 31, 2014 at 03:45:27PM +0000, Peter Maydell wrote: > >> Make the cache ID system registers (CLIDR, CCSELR, CCSIDR, CTR) > > > > s/CCSELR/CSSELR/ > > > >> visible to AArch64. These are mostly simple 64-bit extensions of the > >> existing 32 bit system registers and so can share reginfo definitions. > > > > According to the document(ARM DDI 0487A.a), some AArch64 system > > registers are 32-bit, for example CCSIDR_EL1 is 32-bit. But System_Put() > > and System_Get() writes/reads 64-bit values, which makes me confused. > > We've been round this issue before. The documentation > uses "32-bit" as a shorthand for "64 bit but the top > 32 bits are RES0". There is no way in AArch64 to do a > 32 bit read or write of a system register -- the only > instructions are MSR/MRS, which are always 64 bits. > Similarly, the KVM kernel API exposes all registers as > 64 bits wide. Thanks for the explanation!
diff --git a/target-arm/cpu.c b/target-arm/cpu.c index fe18b65..8fed098 100644 --- a/target-arm/cpu.c +++ b/target-arm/cpu.c @@ -91,6 +91,8 @@ static void arm_cpu_reset(CPUState *s) env->aarch64 = 1; #if defined(CONFIG_USER_ONLY) env->pstate = PSTATE_MODE_EL0t; + /* Userspace expects access to CTL_EL0 */ + env->cp15.c1_sys |= SCTLR_UCT; #else env->pstate = PSTATE_D | PSTATE_A | PSTATE_I | PSTATE_F | PSTATE_MODE_EL1h; diff --git a/target-arm/cpu.h b/target-arm/cpu.h index d1ed423..f5b706e 100644 --- a/target-arm/cpu.h +++ b/target-arm/cpu.h @@ -166,7 +166,7 @@ typedef struct CPUARMState { /* System control coprocessor (cp15) */ struct { uint32_t c0_cpuid; - uint32_t c0_cssel; /* Cache size selection. */ + uint64_t c0_cssel; /* Cache size selection. */ uint32_t c1_sys; /* System control register. */ uint32_t c1_coproc; /* Coprocessor access register. */ uint32_t c1_xscaleauxcr; /* XScale auxiliary control register. */ diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c index a639c2e..8426bf1 100644 --- a/target-arm/cpu64.c +++ b/target-arm/cpu64.c @@ -45,6 +45,7 @@ static void aarch64_any_initfn(Object *obj) set_feature(&cpu->env, ARM_FEATURE_ARM_DIV); set_feature(&cpu->env, ARM_FEATURE_V7MP); set_feature(&cpu->env, ARM_FEATURE_AARCH64); + cpu->ctr = 0x80030003; /* 32 byte I and D cacheline size, VIPT icache */ } #endif diff --git a/target-arm/helper.c b/target-arm/helper.c index 06331dd..f46dd0f 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -629,9 +629,11 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { { .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0, .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr), .resetvalue = 0, }, - { .name = "CCSIDR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0, + { .name = "CCSIDR", .state = ARM_CP_STATE_BOTH, + .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0, .access = PL1_R, .readfn = ccsidr_read, .type = ARM_CP_NO_MIGRATE }, - { .name = "CSSELR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 2, .opc2 = 0, + { .name = "CSSELR", .state = ARM_CP_STATE_BOTH, + .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 2, .opc2 = 0, .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c0_cssel), .writefn = csselr_write, .resetvalue = 0 }, /* Auxiliary ID register: this actually has an IMPDEF value but for now @@ -1524,13 +1526,6 @@ static const ARMCPRegInfo v8_cp_reginfo[] = { { .name = "FPSR", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 3, .opc2 = 1, .crn = 4, .crm = 4, .access = PL0_RW, .readfn = aa64_fpsr_read, .writefn = aa64_fpsr_write }, - /* This claims a 32 byte cacheline size for icache and dcache, VIPT icache. - * It will eventually need to have a CPU-specified reset value. - */ - { .name = "CTR_EL0", .state = ARM_CP_STATE_AA64, - .opc0 = 3, .opc1 = 3, .opc2 = 1, .crn = 0, .crm = 0, - .access = PL0_R, .type = ARM_CP_CONST, - .resetvalue = 0x80030003 }, /* Prohibit use of DC ZVA. OPTME: implement DC ZVA and allow its use. * For system mode the DZP bit here will need to be computed, not constant. */ @@ -1550,6 +1545,17 @@ static void sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri, tlb_flush(env, 1); } +static CPAccessResult ctr_el0_access(CPUARMState *env, const ARMCPRegInfo *ri) +{ + /* Only accessible in EL0 if SCTLR.UCT is set (and only in AArch64, + * but the AArch32 CTR has its own reginfo struct) + */ + if (arm_current_pl(env) == 0 && !(env->cp15.c1_sys & SCTLR_UCT)) { + return CP_ACCESS_TRAP; + } + return CP_ACCESS_OK; +} + void register_cp_regs_for_features(ARMCPU *cpu) { /* Register all the coprocessor registers based on feature bits */ @@ -1634,7 +1640,8 @@ void register_cp_regs_for_features(ARMCPU *cpu) .raw_writefn = raw_write, }; ARMCPRegInfo clidr = { - .name = "CLIDR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 1, + .name = "CLIDR", .state = ARM_CP_STATE_BOTH, + .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 1, .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = cpu->clidr }; define_one_arm_cp_reg(cpu, &pmcr); @@ -1713,6 +1720,10 @@ void register_cp_regs_for_features(ARMCPU *cpu) { .name = "CTR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 1, .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = cpu->ctr }, + { .name = "CTR_EL0", .state = ARM_CP_STATE_AA64, + .opc0 = 3, .opc1 = 3, .opc2 = 1, .crn = 0, .crm = 0, + .access = PL0_R, .accessfn = ctr_el0_access, + .type = ARM_CP_CONST, .resetvalue = cpu->ctr }, { .name = "TCMTR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 2, .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 0 },
Make the cache ID system registers (CLIDR, CCSELR, CCSIDR, CTR) visible to AArch64. These are mostly simple 64-bit extensions of the existing 32 bit system registers and so can share reginfo definitions. CTR needs to have a split definition, but we can clean up the temporary user-mode implementation in favour of using the CPU-specified reset value, and implement the system-mode-required semantics of restricting its EL0 accessibility if SCTLR.UCT is not set. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target-arm/cpu.c | 2 ++ target-arm/cpu.h | 2 +- target-arm/cpu64.c | 1 + target-arm/helper.c | 31 +++++++++++++++++++++---------- 4 files changed, 25 insertions(+), 11 deletions(-)