Re: [Qemu-devel] [PATCH v5 34/37] target-arm: Implement CBAR for Cortex-A57

2014-04-04 Thread Peter Maydell 
On 4 April 2014 06:32, Peter Crosthwaite peter.crosthwa...@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

Re: [Qemu-devel] [PATCH v5 34/37] target-arm: Implement CBAR for Cortex-A57

2014-04-04 Thread Peter Crosthwaite 
On Fri, Apr 4, 2014 at 6:25 PM, Peter Maydell peter.mayd...@linaro.org wrote:
 On 4 April 2014 06:32, Peter Crosthwaite peter.crosthwa...@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

Re: [Qemu-devel] [PATCH v5 34/37] target-arm: Implement CBAR for Cortex-A57

2014-04-04 Thread Peter Maydell 
On 4 April 2014 13:32, Peter Crosthwaite peter.crosthwa...@xilinx.com wrote:
 On Fri, Apr 4, 2014 at 6:25 PM, Peter Maydell peter.mayd...@linaro.org 
 wrote:
 On 4 April 2014 06:32, Peter Crosthwaite peter.crosthwa...@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

Re: [Qemu-devel] [PATCH v5 34/37] target-arm: Implement CBAR for Cortex-A57

2014-04-03 Thread Peter Crosthwaite 
On Sat, Mar 29, 2014 at 2:10 AM, Peter Maydell peter.mayd...@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.mayd...@linaro.org

Reviewed-by: Peter Crosthwaite peter.crosthwa...@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 },
 +

[Qemu-devel] [PATCH v5 34/37] target-arm: Implement CBAR for Cortex-A57

2014-03-28 Thread Peter Maydell 
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.mayd...@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(-)

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 =