| Message ID | 1396023024-2262-35-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Sat, Mar 29, 2014 at 2:10 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > The Cortex-A57, like most of the other ARM cores, has a CBAR > register which defines the base address of the per-CPU > peripherals. However it has a 64-bit view as well as a > 32-bit view; expand the QOM reset-cbar property from UINT32 > to UINT64 so this can be specified, and implement the > 32-bit and 64-bit views of a 64-bit CBAR. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > --- > target-arm/cpu-qom.h | 2 +- > target-arm/cpu.c | 8 ++++++-- > target-arm/cpu.h | 1 + > target-arm/cpu64.c | 1 + > target-arm/helper.c | 39 +++++++++++++++++++++++++++++++++------ > 5 files changed, 42 insertions(+), 9 deletions(-) > > diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h > index 743985e..82f1bc7 100644 > --- a/target-arm/cpu-qom.h > +++ b/target-arm/cpu-qom.h > @@ -148,7 +148,7 @@ typedef struct ARMCPU { > * in the order L1DCache, L1ICache, L2DCache, L2ICache, etc. > */ > uint32_t ccsidr[16]; > - uint32_t reset_cbar; > + uint64_t reset_cbar; > uint32_t reset_auxcr; > bool reset_hivecs; > /* DCZ blocksize, in log_2(words), ie low 4 bits of DCZID_EL0 */ > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > index 783fc73..f9f6187 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -262,7 +262,7 @@ static void arm_cpu_initfn(Object *obj) > } > > static Property arm_cpu_reset_cbar_property = > - DEFINE_PROP_UINT32("reset-cbar", ARMCPU, reset_cbar, 0); > + DEFINE_PROP_UINT64("reset-cbar", ARMCPU, reset_cbar, 0); > > static Property arm_cpu_reset_hivecs_property = > DEFINE_PROP_BOOL("reset-hivecs", ARMCPU, reset_hivecs, false); > @@ -274,7 +274,8 @@ static void arm_cpu_post_init(Object *obj) > { > ARMCPU *cpu = ARM_CPU(obj); > > - if (arm_feature(&cpu->env, ARM_FEATURE_CBAR)) { > + if (arm_feature(&cpu->env, ARM_FEATURE_CBAR) || > + arm_feature(&cpu->env, ARM_FEATURE_CBAR_RO)) { > qdev_property_add_static(DEVICE(obj), &arm_cpu_reset_cbar_property, > &error_abort); > } > @@ -349,6 +350,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) > set_feature(env, ARM_FEATURE_V7MP); > set_feature(env, ARM_FEATURE_PXN); > } > + if (arm_feature(env, ARM_FEATURE_CBAR_RO)) { > + set_feature(env, ARM_FEATURE_CBAR); > + } > > if (cpu->reset_hivecs) { > cpu->reset_sctlr |= (1 << 13); > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index bebb333..c83f249 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -630,6 +630,7 @@ enum arm_features { > ARM_FEATURE_V8_AES, /* implements AES part of v8 Crypto Extensions */ > ARM_FEATURE_CBAR, /* has cp15 CBAR */ > ARM_FEATURE_CRC, /* ARMv8 CRC instructions */ > + ARM_FEATURE_CBAR_RO, /* has cp15 CBAR and it is read-only */ > }; > > static inline int arm_feature(CPUARMState *env, int feature) > diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c > index 70a83fc..9a0c431 100644 > --- a/target-arm/cpu64.c > +++ b/target-arm/cpu64.c > @@ -97,6 +97,7 @@ static void aarch64_a57_initfn(Object *obj) > set_feature(&cpu->env, ARM_FEATURE_NEON); > set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER); > set_feature(&cpu->env, ARM_FEATURE_AARCH64); > + set_feature(&cpu->env, ARM_FEATURE_CBAR_RO); > cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A57; > cpu->midr = 0x411fd070; > cpu->reset_fpsid = 0x41034070; > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 4a74249..f169bea 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -2463,12 +2463,39 @@ void register_cp_regs_for_features(ARMCPU *cpu) > } > > if (arm_feature(env, ARM_FEATURE_CBAR)) { > - ARMCPRegInfo cbar = { > - .name = "CBAR", .cp = 15, .crn = 15, .crm = 0, .opc1 = 4, .opc2 = 0, > - .access = PL1_R|PL3_W, .resetvalue = cpu->reset_cbar, > - .fieldoffset = offsetof(CPUARMState, cp15.c15_config_base_address) > - }; > - define_one_arm_cp_reg(cpu, &cbar); > + if (arm_feature(env, ARM_FEATURE_AARCH64)) { > + /* 32 bit view is [31:18] 0...0 [43:32]. */ > + uint32_t cbar32 = cpu->reset_cbar Should you extract64 on the lower order bits as well to avoid weird | results on a misaligned reset_cbar (or perhaps its worth an assert?). > + | extract64(cpu->reset_cbar, 32, 12); > + ARMCPRegInfo cbar_reginfo[] = { > + { .name = "CBAR", > + .type = ARM_CP_CONST, > + .cp = 15, .crn = 15, .crm = 0, .opc1 = 4, .opc2 = 0, > + .access = PL1_R, .resetvalue = cpu->reset_cbar }, > + { .name = "CBAR_EL1", .state = ARM_CP_STATE_AA64, > + .type = ARM_CP_CONST, > + .opc0 = 3, .opc1 = 1, .crn = 15, .crm = 3, .opc2 = 0, > + .access = PL1_R, .resetvalue = cbar32 }, > + REGINFO_SENTINEL > + }; > + /* We don't implement a r/w 64 bit CBAR currently */ Is it even valid? Writable CBAR seems like a bug to me that's just fixed in the V8 version bump. > + assert(arm_feature(env, ARM_FEATURE_CBAR_RO)); > + define_arm_cp_regs(cpu, cbar_reginfo); > + } else { > + ARMCPRegInfo cbar = { > + .name = "CBAR", > + .cp = 15, .crn = 15, .crm = 0, .opc1 = 4, .opc2 = 0, > + .access = PL1_R|PL3_W, .resetvalue = cpu->reset_cbar, > + .fieldoffset = offsetof(CPUARMState, > + cp15.c15_config_base_address) > + }; > + if (arm_feature(env, ARM_FEATURE_CBAR_RO)) { > + cbar.access = PL1_R; > + cbar.fieldoffset = 0; > + cbar.type = ARM_CP_CONST; > + } > + define_one_arm_cp_reg(cpu, &cbar); > + } > } > > /* Generic registers whose values depend on the implementation */ > -- > 1.9.0 > >
On 4 April 2014 06:32, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: >> + if (arm_feature(env, ARM_FEATURE_AARCH64)) { >> + /* 32 bit view is [31:18] 0...0 [43:32]. */ >> + uint32_t cbar32 = cpu->reset_cbar > > Should you extract64 on the lower order bits as well to avoid weird | > results on a misaligned reset_cbar (or perhaps its worth an assert?). Can't assert, it's a QOM property; we could perhaps validate earlier on in init, but that might be a bit painful to find a suitable place to put it. extracting the low bits too seems a reasonable compromise. >> + | extract64(cpu->reset_cbar, 32, 12); >> + ARMCPRegInfo cbar_reginfo[] = { >> + { .name = "CBAR", >> + .type = ARM_CP_CONST, >> + .cp = 15, .crn = 15, .crm = 0, .opc1 = 4, .opc2 = 0, >> + .access = PL1_R, .resetvalue = cpu->reset_cbar }, >> + { .name = "CBAR_EL1", .state = ARM_CP_STATE_AA64, >> + .type = ARM_CP_CONST, >> + .opc0 = 3, .opc1 = 1, .crn = 15, .crm = 3, .opc2 = 0, >> + .access = PL1_R, .resetvalue = cbar32 }, >> + REGINFO_SENTINEL >> + }; >> + /* We don't implement a r/w 64 bit CBAR currently */ > > Is it even valid? Writable CBAR seems like a bug to me that's just > fixed in the V8 version bump. This is all IMPDEF anyway (and as you'll see from the next patch A15's CBAR is RO too). R/W CBAR isn't an inherently dumb idea: you could use it to make system designs where setting CBAR is the responsibility of board-specific bootrom or firmware before handoff to tho OS, for instance. Having the h/w hardwire it is probably more robust though. thanks -- PMM
On Fri, Apr 4, 2014 at 6:25 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 4 April 2014 06:32, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: >>> + if (arm_feature(env, ARM_FEATURE_AARCH64)) { >>> + /* 32 bit view is [31:18] 0...0 [43:32]. */ >>> + uint32_t cbar32 = cpu->reset_cbar >> >> Should you extract64 on the lower order bits as well to avoid weird | >> results on a misaligned reset_cbar (or perhaps its worth an assert?). > > Can't assert, it's a QOM property; we could perhaps validate > earlier on in init, Is realize allowed to fail due to bad property values? Thinking more about it, perhaps the ideal solution is to populate the Error ** passed to realize and bail out and let the realize() caller deal with it. but that might be a bit painful to find a suitable > place to put it. extracting the low bits too seems a reasonable > compromise. > Ok, Regards, Peter >>> + | extract64(cpu->reset_cbar, 32, 12); >>> + ARMCPRegInfo cbar_reginfo[] = { >>> + { .name = "CBAR", >>> + .type = ARM_CP_CONST, >>> + .cp = 15, .crn = 15, .crm = 0, .opc1 = 4, .opc2 = 0, >>> + .access = PL1_R, .resetvalue = cpu->reset_cbar }, >>> + { .name = "CBAR_EL1", .state = ARM_CP_STATE_AA64, >>> + .type = ARM_CP_CONST, >>> + .opc0 = 3, .opc1 = 1, .crn = 15, .crm = 3, .opc2 = 0, >>> + .access = PL1_R, .resetvalue = cbar32 }, >>> + REGINFO_SENTINEL >>> + }; >>> + /* We don't implement a r/w 64 bit CBAR currently */ >> >> Is it even valid? Writable CBAR seems like a bug to me that's just >> fixed in the V8 version bump. > > This is all IMPDEF anyway (and as you'll see from the next patch > A15's CBAR is RO too). R/W CBAR isn't an inherently dumb idea: > you could use it to make system designs where setting CBAR is > the responsibility of board-specific bootrom or firmware before > handoff to tho OS, for instance. Having the h/w hardwire it is > probably more robust though. > > thanks > -- PMM >
On 4 April 2014 13:32, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Fri, Apr 4, 2014 at 6:25 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 4 April 2014 06:32, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: >>>> + if (arm_feature(env, ARM_FEATURE_AARCH64)) { >>>> + /* 32 bit view is [31:18] 0...0 [43:32]. */ >>>> + uint32_t cbar32 = cpu->reset_cbar >>> >>> Should you extract64 on the lower order bits as well to avoid weird | >>> results on a misaligned reset_cbar (or perhaps its worth an assert?). >> >> Can't assert, it's a QOM property; we could perhaps validate >> earlier on in init, > > Is realize allowed to fail due to bad property values? Yes; see for instance hw/intc/arm_gic_common.c : realize gets an Error** so it can fail nicely in this situation. > Thinking more > about it, perhaps the ideal solution is to populate the Error ** > passed to realize and bail out and let the realize() caller deal with > it. Yep... > but that might be a bit painful to find a suitable >> place to put it. extracting the low bits too seems a reasonable >> compromise. ...but as I say it doesn't really seem worth messing about plumbing the Error** into the right places for this corner case. thanks -- PMM
diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h index 743985e..82f1bc7 100644 --- a/target-arm/cpu-qom.h +++ b/target-arm/cpu-qom.h @@ -148,7 +148,7 @@ typedef struct ARMCPU { * in the order L1DCache, L1ICache, L2DCache, L2ICache, etc. */ uint32_t ccsidr[16]; - uint32_t reset_cbar; + uint64_t reset_cbar; uint32_t reset_auxcr; bool reset_hivecs; /* DCZ blocksize, in log_2(words), ie low 4 bits of DCZID_EL0 */ diff --git a/target-arm/cpu.c b/target-arm/cpu.c index 783fc73..f9f6187 100644 --- a/target-arm/cpu.c +++ b/target-arm/cpu.c @@ -262,7 +262,7 @@ static void arm_cpu_initfn(Object *obj) } static Property arm_cpu_reset_cbar_property = - DEFINE_PROP_UINT32("reset-cbar", ARMCPU, reset_cbar, 0); + DEFINE_PROP_UINT64("reset-cbar", ARMCPU, reset_cbar, 0); static Property arm_cpu_reset_hivecs_property = DEFINE_PROP_BOOL("reset-hivecs", ARMCPU, reset_hivecs, false); @@ -274,7 +274,8 @@ static void arm_cpu_post_init(Object *obj) { ARMCPU *cpu = ARM_CPU(obj); - if (arm_feature(&cpu->env, ARM_FEATURE_CBAR)) { + if (arm_feature(&cpu->env, ARM_FEATURE_CBAR) || + arm_feature(&cpu->env, ARM_FEATURE_CBAR_RO)) { qdev_property_add_static(DEVICE(obj), &arm_cpu_reset_cbar_property, &error_abort); } @@ -349,6 +350,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) set_feature(env, ARM_FEATURE_V7MP); set_feature(env, ARM_FEATURE_PXN); } + if (arm_feature(env, ARM_FEATURE_CBAR_RO)) { + set_feature(env, ARM_FEATURE_CBAR); + } if (cpu->reset_hivecs) { cpu->reset_sctlr |= (1 << 13); diff --git a/target-arm/cpu.h b/target-arm/cpu.h index bebb333..c83f249 100644 --- a/target-arm/cpu.h +++ b/target-arm/cpu.h @@ -630,6 +630,7 @@ enum arm_features { ARM_FEATURE_V8_AES, /* implements AES part of v8 Crypto Extensions */ ARM_FEATURE_CBAR, /* has cp15 CBAR */ ARM_FEATURE_CRC, /* ARMv8 CRC instructions */ + ARM_FEATURE_CBAR_RO, /* has cp15 CBAR and it is read-only */ }; static inline int arm_feature(CPUARMState *env, int feature) diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c index 70a83fc..9a0c431 100644 --- a/target-arm/cpu64.c +++ b/target-arm/cpu64.c @@ -97,6 +97,7 @@ static void aarch64_a57_initfn(Object *obj) set_feature(&cpu->env, ARM_FEATURE_NEON); set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER); set_feature(&cpu->env, ARM_FEATURE_AARCH64); + set_feature(&cpu->env, ARM_FEATURE_CBAR_RO); cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A57; cpu->midr = 0x411fd070; cpu->reset_fpsid = 0x41034070; diff --git a/target-arm/helper.c b/target-arm/helper.c index 4a74249..f169bea 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -2463,12 +2463,39 @@ void register_cp_regs_for_features(ARMCPU *cpu) } if (arm_feature(env, ARM_FEATURE_CBAR)) { - ARMCPRegInfo cbar = { - .name = "CBAR", .cp = 15, .crn = 15, .crm = 0, .opc1 = 4, .opc2 = 0, - .access = PL1_R|PL3_W, .resetvalue = cpu->reset_cbar, - .fieldoffset = offsetof(CPUARMState, cp15.c15_config_base_address) - }; - define_one_arm_cp_reg(cpu, &cbar); + if (arm_feature(env, ARM_FEATURE_AARCH64)) { + /* 32 bit view is [31:18] 0...0 [43:32]. */ + uint32_t cbar32 = cpu->reset_cbar + | extract64(cpu->reset_cbar, 32, 12); + ARMCPRegInfo cbar_reginfo[] = { + { .name = "CBAR", + .type = ARM_CP_CONST, + .cp = 15, .crn = 15, .crm = 0, .opc1 = 4, .opc2 = 0, + .access = PL1_R, .resetvalue = cpu->reset_cbar }, + { .name = "CBAR_EL1", .state = ARM_CP_STATE_AA64, + .type = ARM_CP_CONST, + .opc0 = 3, .opc1 = 1, .crn = 15, .crm = 3, .opc2 = 0, + .access = PL1_R, .resetvalue = cbar32 }, + REGINFO_SENTINEL + }; + /* We don't implement a r/w 64 bit CBAR currently */ + assert(arm_feature(env, ARM_FEATURE_CBAR_RO)); + define_arm_cp_regs(cpu, cbar_reginfo); + } else { + ARMCPRegInfo cbar = { + .name = "CBAR", + .cp = 15, .crn = 15, .crm = 0, .opc1 = 4, .opc2 = 0, + .access = PL1_R|PL3_W, .resetvalue = cpu->reset_cbar, + .fieldoffset = offsetof(CPUARMState, + cp15.c15_config_base_address) + }; + if (arm_feature(env, ARM_FEATURE_CBAR_RO)) { + cbar.access = PL1_R; + cbar.fieldoffset = 0; + cbar.type = ARM_CP_CONST; + } + define_one_arm_cp_reg(cpu, &cbar); + } } /* Generic registers whose values depend on the implementation */
The Cortex-A57, like most of the other ARM cores, has a CBAR register which defines the base address of the per-CPU peripherals. However it has a 64-bit view as well as a 32-bit view; expand the QOM reset-cbar property from UINT32 to UINT64 so this can be specified, and implement the 32-bit and 64-bit views of a 64-bit CBAR. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target-arm/cpu-qom.h | 2 +- target-arm/cpu.c | 8 ++++++-- target-arm/cpu.h | 1 + target-arm/cpu64.c | 1 + target-arm/helper.c | 39 +++++++++++++++++++++++++++++++++------ 5 files changed, 42 insertions(+), 9 deletions(-)