Re: [Qemu-devel] [PATCH 07/24] target-arm: A64: Make cache ID registers visible to AArch64

2014-01-28 Thread Peter Maydell 
On 28 January 2014 01:46, Peter Crosthwaite
peter.crosthwa...@xilinx.com wrote:
 There seem to be multiple instances in this series where you fallback
 to open coded R/W accessor functions for the sake of access checks. Is
 it better to define a bool check_access() fn hook in ARMCPRegInfo and
 leave the actual write/read behaviour to the data driven mechanisms?
 This may also minimise the need for raw_write hook usages as it serves
 to isolate the actual state change into its own self contained
 definition (whether open coded or not).

Yes, I think it's probably going to be better to do that. We may need
to make it more than just bool, though since for AArch64 the
kind of exception can be different I think -- the specific syndrome
information can vary.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 07/24] target-arm: A64: Make cache ID registers visible to AArch64

2014-01-28 Thread Peter Crosthwaite 
On Tue, Jan 28, 2014 at 6:45 PM, Peter Maydell peter.mayd...@linaro.org wrote:
 On 28 January 2014 01:46, Peter Crosthwaite
 peter.crosthwa...@xilinx.com wrote:
 There seem to be multiple instances in this series where you fallback
 to open coded R/W accessor functions for the sake of access checks. Is
 it better to define a bool check_access() fn hook in ARMCPRegInfo and
 leave the actual write/read behaviour to the data driven mechanisms?
 This may also minimise the need for raw_write hook usages as it serves
 to isolate the actual state change into its own self contained
 definition (whether open coded or not).

 Yes, I think it's probably going to be better to do that. We may need
 to make it more than just bool, though since for AArch64 the
 kind of exception can be different I think -- the specific syndrome
 information can vary.


I guess then it's simplest to just return in same format as read/write
accessors.

Regards,
Peter

 thanks
 -- PMM

Re: [Qemu-devel] [PATCH 07/24] target-arm: A64: Make cache ID registers visible to AArch64

2014-01-28 Thread Peter Maydell 
On 28 January 2014 14:05, Peter Crosthwaite
peter.crosthwa...@xilinx.com wrote:
 On Tue, Jan 28, 2014 at 6:45 PM, Peter Maydell peter.mayd...@linaro.org 
 wrote:
 On 28 January 2014 01:46, Peter Crosthwaite
 peter.crosthwa...@xilinx.com wrote:
 There seem to be multiple instances in this series where you fallback
 to open coded R/W accessor functions for the sake of access checks. Is
 it better to define a bool check_access() fn hook in ARMCPRegInfo and
 leave the actual write/read behaviour to the data driven mechanisms?
 This may also minimise the need for raw_write hook usages as it serves
 to isolate the actual state change into its own self contained
 definition (whether open coded or not).

 Yes, I think it's probably going to be better to do that. We may need
 to make it more than just bool, though since for AArch64 the
 kind of exception can be different I think -- the specific syndrome
 information can vary.


 I guess then it's simplest to just return in same format as read/write
 accessors.

Well, AArch64 means we also can't just return EXCP_UDEF or 0,
but ideally return at least some syndrome information.
I haven't quite figured out the nicest way to do this yet.
My current work in progress has:
 * retain all the AArch32 EXCP_* classes as values for
   env-exception_index
 * whenever we generate an exception, also set up syndrome
   register information in the cpu state struct; the
   syndrome register high 6 bits happen to contain a category
   code, but we don't map this to an env-exception_index

So for AArch64, access functions need to be able to return one
of three things:
 (1) no exception, ie access OK
 (2) exception which in UNDEF if taken to AArch32, or
 a synchronous exception with ESR_ELx.EC = 0 (uncategorized)
 if taken to AArch64
 (3) exception which is UNDEF if taken to AArch32, or a
 synchronous exception with ESR_ELx.EC == 0x3/4/5/6/C/18
 if taken to AArch64 (exact category depends on the
 insn used to access the register, and ESR_ELx
 includes all the opc1/2/crn/crm fields from the insn)
 -- this generally is used for access failed because of
 a configurable trap or enable bit

so we need to at least return a three-way ok/disabled/undef
code. I think actually most if not all of our current access
checks done in read/write fns should be type (2), but I don't
want to leave us stuck if we find one that needs to be type
(1) now or in the future. Failures due to the ri-access
permissions are always type (1).

We could return a complete EXCP_* value plus syndrome,
but that seems like overkill especially since the calling
code has most of the info needed for putting the syndrome
together properly. So I think:

typedef enum CPAccessResult {
/* Access is permitted */
CP_ACCESS_OK = 0,
/* Access fails due to a configurable trap or enable which would
 * result in an exception syndrome other than 'uncategorized'
 */
CP_ACCESS_TRAP = 1,
/* Access fails and should result in an exception syndrome of
 * 'uncategorized'
 */
CP_ACCESS_TRAP = 2,
} CPAccessResult;

typedef CPAccessResult CPAccessFn(CPUARMState *env,
  const ARMCPRegInfo *opaque);

and an entry in ARMCPRegInfo:
/* Function for checking access permissions for the register
 * which should be done in addition to those specified by
 * the access field. If NULL, no extra checks required.
 */
CPAccessFn accessfn;

and CPReadFn and CPWriteFn both go from 'int' return to
'void' return.

(Anybody got a better name than CPAccessResult for the enum type?)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 07/24] target-arm: A64: Make cache ID registers visible to AArch64

2014-01-28 Thread Peter Maydell 
On 28 January 2014 18:12, Peter Maydell peter.mayd...@linaro.org wrote:
 typedef enum CPAccessResult {
 /* Access is permitted */
 CP_ACCESS_OK = 0,
 /* Access fails due to a configurable trap or enable which would
  * result in an exception syndrome other than 'uncategorized'
  */
 CP_ACCESS_TRAP = 1,
 /* Access fails and should result in an exception syndrome of
  * 'uncategorized'
  */
 CP_ACCESS_TRAP = 2,
 } CPAccessResult;

The =2 case should be some other name, of course. I'd rather
not use the obvious CP_ACCESS_UNCATEGORIZED because that
implies use this if you don't know, when actually the set of
things which cause Uncategorized exceptions is a specific list,
and it's the 'due to a configurable trap' case which is defined
as all those which aren't defined to cause an Uncategorized
exception. On the other hand I can't come up with a better
name right now...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 07/24] target-arm: A64: Make cache ID registers visible to AArch64

2014-01-27 Thread Peter Crosthwaite 
On Wed, Jan 22, 2014 at 6:12 AM, Peter Maydell peter.mayd...@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.mayd...@linaro.org
 ---
  target-arm/cpu.c|  2 ++
  target-arm/cpu.h|  2 +-
  target-arm/cpu64.c  |  1 +
  target-arm/helper.c | 33 +++--
  4 files changed, 27 insertions(+), 11 deletions(-)

 diff --git a/target-arm/cpu.c b/target-arm/cpu.c
 index c1e55c8..1e08802 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 1aa4495..1966a19 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 205e36a..204d7c3 100644
 --- a/target-arm/helper.c
 +++ b/target-arm/helper.c
 @@ -676,9 +676,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
 @@ -1601,13 +1603,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.
   */
 @@ -1627,6 +1622,20 @@ static int sctlr_write(CPUARMState *env, const 
 ARMCPRegInfo *ri, uint64_t value)
  return 0;
  }

 +static int ctr_el0_read(CPUARMState *env, const ARMCPRegInfo *ri,
 +uint64_t *value)
 +{
 +/* This register is constant, but only accessible in AArch64 EL0 if
 + * SCTLR.UCT is set.
 + */
 +ARMCPU *cpu = arm_env_get_cpu(env);
 +if (arm_current_pl(env) == 0  !(env-cp15.c1_sys  SCTLR_UCT)) {
 +return EXCP_UDEF;
 +}

There seem to be multiple instances in this series where you fallback
to open coded R/W accessor functions for the sake of access checks. Is
it better to define a bool check_access() fn hook in ARMCPRegInfo and
leave the actual write/read behaviour to the data driven mechanisms?
This may also minimise the need for raw_write

[Qemu-devel] [PATCH 07/24] target-arm: A64: Make cache ID registers visible to AArch64

2014-01-21 Thread Peter Maydell 
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.mayd...@linaro.org
---
 target-arm/cpu.c|  2 ++
 target-arm/cpu.h|  2 +-
 target-arm/cpu64.c  |  1 +
 target-arm/helper.c | 33 +++--
 4 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index c1e55c8..1e08802 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 1aa4495..1966a19 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 205e36a..204d7c3 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -676,9 +676,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
@@ -1601,13 +1603,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.
  */
@@ -1627,6 +1622,20 @@ static int sctlr_write(CPUARMState *env, const 
ARMCPRegInfo *ri, uint64_t value)
 return 0;
 }
 
+static int ctr_el0_read(CPUARMState *env, const ARMCPRegInfo *ri,
+uint64_t *value)
+{
+/* This register is constant, but only accessible in AArch64 EL0 if
+ * SCTLR.UCT is set.
+ */
+ARMCPU *cpu = arm_env_get_cpu(env);
+if (arm_current_pl(env) == 0  !(env-cp15.c1_sys  SCTLR_UCT)) {
+return EXCP_UDEF;
+}
+*value = cpu-ctr;
+return 0;
+}
+
 void register_cp_regs_for_features(ARMCPU *cpu)
 {
 /* Register all the coprocessor registers based on feature bits */
@@ -1711,7 +1720,8 @@ void register_cp_regs_for_features(ARMCPU *cpu)
 .raw_readfn = raw_read, .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,