[Qemu-devel] [PATCH 0/7] Fix some more EL3 things and enable EL3 for AArch64

[Qemu-devel] [PATCH 0/7] Fix some more EL3 things and enable EL3 for AArch64

[Peter Maydell]

This series fixes a couple more minor EL3 related missing parts,
and then turns on EL3 for AArch64 CPUs. The minor fixed things are:
 * implement MDCR_EL3 and SDCR
 * trap Secure EL1 accesses to SCR and MVBAR to EL3
 * add EL3 support to the code that decides whether to generate
   debug exceptions
 * fix corner cases in our NSACR handling

To do the NSACR fix I had to change the CPAccessFn API to take
an extra bool to tell the function if the access is a read or write.

The only major thing I know of that we're missing for 64-bit EL3
is that we need to go through the "EL3 configurable controls" section
of the ARM ARM to make sure we trap on the right things. But
(a) I expect we're missing some for 32-bit as well and (b) this
is enough to run some real-world EL3 code (ARM Trusted Firmware
and OP-TEE), so it makes sense to me to turn on EL3 now.


thanks
-- PMM

Peter Maydell (7):
  target-arm: Fix typo in comment in arm_is_secure_below_el3()
  target-arm: Implement MDCR_EL3 and SDCR
  target-arm: Use access_trap_aa32s_el1() for SCR and MVBAR
  target-arm: Update arm_generate_debug_exceptions() to handle EL2/EL3
  target-arm: Add isread parameter to CPAccessFns
  target-arm: Implement NSACR trapping behaviour
  target-arm: Enable EL3 for Cortex-A53 and Cortex-A57

 target-arm/cpu.h           |  55 +++++++++++++--
 target-arm/cpu64.c         |   2 +
 target-arm/helper.c        | 171 ++++++++++++++++++++++++++++++++++++---------
 target-arm/helper.h        |   2 +-
 target-arm/op_helper.c     |   5 +-
 target-arm/translate-a64.c |   6 +-
 target-arm/translate.c     |   7 +-
 7 files changed, 200 insertions(+), 48 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/7] target-arm: Fix typo in comment in arm_is_secure_below_el3()

[Peter Maydell]

Fix a typo where "EL2" was written but "EL3" intended.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/cpu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index b8b3364..52284e9 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -931,7 +931,7 @@ static inline bool arm_is_secure_below_el3(CPUARMState *env)
     if (arm_feature(env, ARM_FEATURE_EL3)) {
         return !(env->cp15.scr_el3 & SCR_NS);
     } else {
-        /* If EL2 is not supported then the secure state is implementation
+        /* If EL3 is not supported then the secure state is implementation
          * defined, in which case QEMU defaults to non-secure.
          */
         return false;
-- 
1.9.1

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/7] target-arm: Fix typo in comment in arm_is_secure_below_el3()

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> Fix a typo where "EL2" was written but "EL3" intended.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/cpu.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index b8b3364..52284e9 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -931,7 +931,7 @@ static inline bool arm_is_secure_below_el3(CPUARMState *env)
>      if (arm_feature(env, ARM_FEATURE_EL3)) {
>          return !(env->cp15.scr_el3 & SCR_NS);
>      } else {
> -        /* If EL2 is not supported then the secure state is implementation
> +        /* If EL3 is not supported then the secure state is implementation
>           * defined, in which case QEMU defaults to non-secure.
>           */
>          return false;

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

--
Alex Bennée

Re: [Qemu-devel] [PATCH 1/7] target-arm: Fix typo in comment in arm_is_secure_below_el3()

[Edgar E. Iglesias]

On Wed, Feb 03, 2016 at 01:38:35PM +0000, Peter Maydell wrote:
> Fix a typo where "EL2" was written but "EL3" intended.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/cpu.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index b8b3364..52284e9 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -931,7 +931,7 @@ static inline bool arm_is_secure_below_el3(CPUARMState *env)
>      if (arm_feature(env, ARM_FEATURE_EL3)) {
>          return !(env->cp15.scr_el3 & SCR_NS);
>      } else {
> -        /* If EL2 is not supported then the secure state is implementation
> +        /* If EL3 is not supported then the secure state is implementation
>           * defined, in which case QEMU defaults to non-secure.
>           */
>          return false;
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/7] target-arm: Fix typo in comment in arm_is_secure_below_el3()

[Sergey Fedorov]

On 03.02.2016 16:38, Peter Maydell wrote:
> Fix a typo where "EL2" was written but "EL3" intended.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>

> ---
>  target-arm/cpu.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index b8b3364..52284e9 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -931,7 +931,7 @@ static inline bool arm_is_secure_below_el3(CPUARMState *env)
>      if (arm_feature(env, ARM_FEATURE_EL3)) {
>          return !(env->cp15.scr_el3 & SCR_NS);
>      } else {
> -        /* If EL2 is not supported then the secure state is implementation
> +        /* If EL3 is not supported then the secure state is implementation
>           * defined, in which case QEMU defaults to non-secure.
>           */
>          return false;

[Qemu-devel] [PATCH 2/7] target-arm: Implement MDCR_EL3 and SDCR

[Peter Maydell]

Implement the MDCR_EL3 register (which is SDCR for AArch32).
For the moment we implement it as reads-as-written.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/cpu.h    |  1 +
 target-arm/helper.c | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 52284e9..cf2df50 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -382,6 +382,7 @@ typedef struct CPUARMState {
         uint64_t mdscr_el1;
         uint64_t oslsr_el1; /* OS Lock Status */
         uint64_t mdcr_el2;
+        uint64_t mdcr_el3;
         /* If the counter is enabled, this stores the last time the counter
          * was reset. Otherwise it stores the counter value
          */
diff --git a/target-arm/helper.c b/target-arm/helper.c
index b631b83..8b96b80 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -364,6 +364,23 @@ static CPAccessResult access_el3_aa32ns_aa64any(CPUARMState *env,
     return CP_ACCESS_OK;
 }
 
+/* Some secure-only AArch32 registers trap to EL3 if used from
+ * Secure EL1 (but are just ordinary UNDEF in other non-EL3 contexts).
+ * We assume that the .access field is set to PL1_RW.
+ */
+static CPAccessResult access_trap_aa32s_el1(CPUARMState *env,
+                                            const ARMCPRegInfo *ri)
+{
+    if (arm_current_el(env) == 3) {
+        return CP_ACCESS_OK;
+    }
+    if (arm_is_secure_below_el3(env)) {
+        return CP_ACCESS_TRAP_EL3;
+    }
+    /* This will be EL1 NS and EL2 NS, which just UNDEF */
+    return CP_ACCESS_TRAP_UNCATEGORIZED;
+}
+
 static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 {
     ARMCPU *cpu = arm_env_get_cpu(env);
@@ -3532,6 +3549,13 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
       .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 0,
       .access = PL3_RW, .fieldoffset = offsetoflow32(CPUARMState, cp15.scr_el3),
       .writefn = scr_write },
+    { .name = "MDCR_EL3", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 3, .opc2 = 1,
+      .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el3) },
+    { .name = "SDCR", .type = ARM_CP_ALIAS,
+      .cp = 15, .opc1 = 0, .crn = 1, .crm = 3, .opc2 = 1,
+      .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
+      .fieldoffset = offsetoflow32(CPUARMState, cp15.mdcr_el3) },
     { .name = "SDER32_EL3", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 1,
       .access = PL3_RW, .resetvalue = 0,
-- 
1.9.1

Re: [Qemu-devel] [PATCH 2/7] target-arm: Implement MDCR_EL3 and SDCR

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> Implement the MDCR_EL3 register (which is SDCR for AArch32).
> For the moment we implement it as reads-as-written.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/cpu.h    |  1 +
>  target-arm/helper.c | 24 ++++++++++++++++++++++++
>  2 files changed, 25 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 52284e9..cf2df50 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -382,6 +382,7 @@ typedef struct CPUARMState {
>          uint64_t mdscr_el1;
>          uint64_t oslsr_el1; /* OS Lock Status */
>          uint64_t mdcr_el2;
> +        uint64_t mdcr_el3;
>          /* If the counter is enabled, this stores the last time the counter
>           * was reset. Otherwise it stores the counter value
>           */
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index b631b83..8b96b80 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -364,6 +364,23 @@ static CPAccessResult access_el3_aa32ns_aa64any(CPUARMState *env,
>      return CP_ACCESS_OK;
>  }
>
> +/* Some secure-only AArch32 registers trap to EL3 if used from
> + * Secure EL1 (but are just ordinary UNDEF in other non-EL3 contexts).
> + * We assume that the .access field is set to PL1_RW.
> + */
> +static CPAccessResult access_trap_aa32s_el1(CPUARMState *env,
> +                                            const ARMCPRegInfo *ri)
> +{

I wonder if we should assert the fact we are in AArch32 here in case the
wrong access function gets added to a AArch64 register?

> +    if (arm_current_el(env) == 3) {
> +        return CP_ACCESS_OK;
> +    }
> +    if (arm_is_secure_below_el3(env)) {
> +        return CP_ACCESS_TRAP_EL3;
> +    }
> +    /* This will be EL1 NS and EL2 NS, which just UNDEF */
> +    return CP_ACCESS_TRAP_UNCATEGORIZED;
> +}
> +
>  static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>  {
>      ARMCPU *cpu = arm_env_get_cpu(env);
> @@ -3532,6 +3549,13 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>        .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 0,
>        .access = PL3_RW, .fieldoffset = offsetoflow32(CPUARMState, cp15.scr_el3),
>        .writefn = scr_write },
> +    { .name = "MDCR_EL3", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 3, .opc2 = 1,
> +      .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el3) },
> +    { .name = "SDCR", .type = ARM_CP_ALIAS,
> +      .cp = 15, .opc1 = 0, .crn = 1, .crm = 3, .opc2 = 1,
> +      .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
> +      .fieldoffset = offsetoflow32(CPUARMState, cp15.mdcr_el3) },

Does anything ensure the fields are reset to 0 on a warm reset?

>      { .name = "SDER32_EL3", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 1,
>        .access = PL3_RW, .resetvalue = 0,


--
Alex Bennée

Re: [Qemu-devel] [PATCH 2/7] target-arm: Implement MDCR_EL3 and SDCR

[Peter Maydell]

On 5 February 2016 at 11:13, Alex Bennée <alex.bennee@linaro.org> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> Implement the MDCR_EL3 register (which is SDCR for AArch32).
>> For the moment we implement it as reads-as-written.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

>> +/* Some secure-only AArch32 registers trap to EL3 if used from
>> + * Secure EL1 (but are just ordinary UNDEF in other non-EL3 contexts).
>> + * We assume that the .access field is set to PL1_RW.
>> + */
>> +static CPAccessResult access_trap_aa32s_el1(CPUARMState *env,
>> +                                            const ARMCPRegInfo *ri)
>> +{
>
> I wonder if we should assert the fact we are in AArch32 here in case the
> wrong access function gets added to a AArch64 register?

We mostly don't do that kind of checking in these access
functions. If you add the wrong access function to your regdef
then your register misbehaves anyway :-)

>> +    if (arm_current_el(env) == 3) {
>> +        return CP_ACCESS_OK;
>> +    }
>> +    if (arm_is_secure_below_el3(env)) {
>> +        return CP_ACCESS_TRAP_EL3;
>> +    }
>> +    /* This will be EL1 NS and EL2 NS, which just UNDEF */
>> +    return CP_ACCESS_TRAP_UNCATEGORIZED;
>> +}
>> +
>>  static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>>  {
>>      ARMCPU *cpu = arm_env_get_cpu(env);
>> @@ -3532,6 +3549,13 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>>        .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 0,
>>        .access = PL3_RW, .fieldoffset = offsetoflow32(CPUARMState, cp15.scr_el3),
>>        .writefn = scr_write },
>> +    { .name = "MDCR_EL3", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 3, .opc2 = 1,
>> +      .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el3) },
>> +    { .name = "SDCR", .type = ARM_CP_ALIAS,
>> +      .cp = 15, .opc1 = 0, .crn = 1, .crm = 3, .opc2 = 1,
>> +      .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
>> +      .fieldoffset = offsetoflow32(CPUARMState, cp15.mdcr_el3) },
>
> Does anything ensure the fields are reset to 0 on a warm reset?

Yes, the cpreg framework resets things. arm_cpu_reset() calls
cp_reg_reset() on every register the CPU knows about. Note that
.resetvalue is 0 as usual for unspecified fields in structure
initializers, but it would be clearer to specifically state it
(ie ".resetvalue = 0") as we do in most cases.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/7] target-arm: Implement MDCR_EL3 and SDCR

[Edgar E. Iglesias]

On Wed, Feb 03, 2016 at 01:38:36PM +0000, Peter Maydell wrote:
> Implement the MDCR_EL3 register (which is SDCR for AArch32).
> For the moment we implement it as reads-as-written.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/cpu.h    |  1 +
>  target-arm/helper.c | 24 ++++++++++++++++++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 52284e9..cf2df50 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -382,6 +382,7 @@ typedef struct CPUARMState {
>          uint64_t mdscr_el1;
>          uint64_t oslsr_el1; /* OS Lock Status */
>          uint64_t mdcr_el2;
> +        uint64_t mdcr_el3;


Should we maybe arrayify these even if we waste space?

Anyway:

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



>          /* If the counter is enabled, this stores the last time the counter
>           * was reset. Otherwise it stores the counter value
>           */
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index b631b83..8b96b80 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -364,6 +364,23 @@ static CPAccessResult access_el3_aa32ns_aa64any(CPUARMState *env,
>      return CP_ACCESS_OK;
>  }
>  
> +/* Some secure-only AArch32 registers trap to EL3 if used from
> + * Secure EL1 (but are just ordinary UNDEF in other non-EL3 contexts).
> + * We assume that the .access field is set to PL1_RW.
> + */
> +static CPAccessResult access_trap_aa32s_el1(CPUARMState *env,
> +                                            const ARMCPRegInfo *ri)
> +{
> +    if (arm_current_el(env) == 3) {
> +        return CP_ACCESS_OK;
> +    }
> +    if (arm_is_secure_below_el3(env)) {
> +        return CP_ACCESS_TRAP_EL3;
> +    }
> +    /* This will be EL1 NS and EL2 NS, which just UNDEF */
> +    return CP_ACCESS_TRAP_UNCATEGORIZED;
> +}
> +
>  static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>  {
>      ARMCPU *cpu = arm_env_get_cpu(env);
> @@ -3532,6 +3549,13 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>        .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 0,
>        .access = PL3_RW, .fieldoffset = offsetoflow32(CPUARMState, cp15.scr_el3),
>        .writefn = scr_write },
> +    { .name = "MDCR_EL3", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 3, .opc2 = 1,
> +      .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el3) },
> +    { .name = "SDCR", .type = ARM_CP_ALIAS,
> +      .cp = 15, .opc1 = 0, .crn = 1, .crm = 3, .opc2 = 1,
> +      .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
> +      .fieldoffset = offsetoflow32(CPUARMState, cp15.mdcr_el3) },
>      { .name = "SDER32_EL3", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 1,
>        .access = PL3_RW, .resetvalue = 0,
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 2/7] target-arm: Implement MDCR_EL3 and SDCR

[Sergey Fedorov]

On 03.02.2016 16:38, Peter Maydell wrote:
> Implement the MDCR_EL3 register (which is SDCR for AArch32).
> For the moment we implement it as reads-as-written.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/cpu.h    |  1 +
>  target-arm/helper.c | 24 ++++++++++++++++++++++++
>  2 files changed, 25 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 52284e9..cf2df50 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -382,6 +382,7 @@ typedef struct CPUARMState {
>          uint64_t mdscr_el1;
>          uint64_t oslsr_el1; /* OS Lock Status */
>          uint64_t mdcr_el2;
> +        uint64_t mdcr_el3;
>          /* If the counter is enabled, this stores the last time the counter
>           * was reset. Otherwise it stores the counter value
>           */
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index b631b83..8b96b80 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -364,6 +364,23 @@ static CPAccessResult access_el3_aa32ns_aa64any(CPUARMState *env,
>      return CP_ACCESS_OK;
>  }
>  
> +/* Some secure-only AArch32 registers trap to EL3 if used from
> + * Secure EL1 (but are just ordinary UNDEF in other non-EL3 contexts).
> + * We assume that the .access field is set to PL1_RW.
> + */

Maybe we should also make a note that there is no secure EL1 if EL3 is
using AArch32 as it is done for ats_access()?

Kind regards,
Sergey

> +static CPAccessResult access_trap_aa32s_el1(CPUARMState *env,
> +                                            const ARMCPRegInfo *ri)
> +{
> +    if (arm_current_el(env) == 3) {
> +        return CP_ACCESS_OK;
> +    }
> +    if (arm_is_secure_below_el3(env)) {
> +        return CP_ACCESS_TRAP_EL3;
> +    }
> +    /* This will be EL1 NS and EL2 NS, which just UNDEF */
> +    return CP_ACCESS_TRAP_UNCATEGORIZED;
> +}
> +
>  static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>  {
>      ARMCPU *cpu = arm_env_get_cpu(env);
> @@ -3532,6 +3549,13 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>        .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 0,
>        .access = PL3_RW, .fieldoffset = offsetoflow32(CPUARMState, cp15.scr_el3),
>        .writefn = scr_write },
> +    { .name = "MDCR_EL3", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 3, .opc2 = 1,
> +      .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el3) },
> +    { .name = "SDCR", .type = ARM_CP_ALIAS,
> +      .cp = 15, .opc1 = 0, .crn = 1, .crm = 3, .opc2 = 1,
> +      .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
> +      .fieldoffset = offsetoflow32(CPUARMState, cp15.mdcr_el3) },
>      { .name = "SDER32_EL3", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 1,
>        .access = PL3_RW, .resetvalue = 0,

Re: [Qemu-devel] [PATCH 2/7] target-arm: Implement MDCR_EL3 and SDCR

[Peter Maydell]

On 6 February 2016 at 18:42, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> On 03.02.2016 16:38, Peter Maydell wrote:
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -364,6 +364,23 @@ static CPAccessResult access_el3_aa32ns_aa64any(CPUARMState *env,
>>      return CP_ACCESS_OK;
>>  }
>>
>> +/* Some secure-only AArch32 registers trap to EL3 if used from
>> + * Secure EL1 (but are just ordinary UNDEF in other non-EL3 contexts).
>> + * We assume that the .access field is set to PL1_RW.
>> + */
>
> Maybe we should also make a note that there is no secure EL1 if EL3 is
> using AArch32 as it is done for ats_access()?

I added a line to the comment:
 * Note that an access from Secure EL1 can only happen if EL3 is AArch64.

thanks
-- PMM

[Qemu-devel] [PATCH 3/7] target-arm: Use access_trap_aa32s_el1() for SCR and MVBAR

[Peter Maydell]

The registers MVBAR and SCR should have the behaviour of trapping to
EL3 if accessed from Secure EL1, but we were incorrectly implementing
them to UNDEF (which would trap to EL1).  Fix this by using the new
access_trap_aa32s_el1() access function.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 8b96b80..d85b04f 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3547,7 +3547,8 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
       .resetvalue = 0, .writefn = scr_write },
     { .name = "SCR",  .type = ARM_CP_ALIAS,
       .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 0,
-      .access = PL3_RW, .fieldoffset = offsetoflow32(CPUARMState, cp15.scr_el3),
+      .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
+      .fieldoffset = offsetoflow32(CPUARMState, cp15.scr_el3),
       .writefn = scr_write },
     { .name = "MDCR_EL3", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 3, .opc2 = 1,
@@ -3569,7 +3570,8 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
       .access = PL3_W | PL1_R, .resetvalue = 0,
       .fieldoffset = offsetof(CPUARMState, cp15.nsacr) },
     { .name = "MVBAR", .cp = 15, .opc1 = 0, .crn = 12, .crm = 0, .opc2 = 1,
-      .access = PL3_RW, .writefn = vbar_write, .resetvalue = 0,
+      .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
+      .writefn = vbar_write, .resetvalue = 0,
       .fieldoffset = offsetof(CPUARMState, cp15.mvbar) },
     { .name = "SCTLR_EL3", .state = ARM_CP_STATE_AA64,
       .type = ARM_CP_ALIAS, /* reset handled by AArch32 view */
-- 
1.9.1

Re: [Qemu-devel] [Qemu-arm] [PATCH 3/7] target-arm: Use access_trap_aa32s_el1() for SCR and MVBAR

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> The registers MVBAR and SCR should have the behaviour of trapping to
> EL3 if accessed from Secure EL1, but we were incorrectly implementing
> them to UNDEF (which would trap to EL1).  Fix this by using the new
> access_trap_aa32s_el1() access function.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/helper.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 8b96b80..d85b04f 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3547,7 +3547,8 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>        .resetvalue = 0, .writefn = scr_write },
>      { .name = "SCR",  .type = ARM_CP_ALIAS,
>        .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 0,
> -      .access = PL3_RW, .fieldoffset = offsetoflow32(CPUARMState, cp15.scr_el3),
> +      .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
> +      .fieldoffset = offsetoflow32(CPUARMState, cp15.scr_el3),
>        .writefn = scr_write },
>      { .name = "MDCR_EL3", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 3, .opc2 = 1,
> @@ -3569,7 +3570,8 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>        .access = PL3_W | PL1_R, .resetvalue = 0,
>        .fieldoffset = offsetof(CPUARMState, cp15.nsacr) },
>      { .name = "MVBAR", .cp = 15, .opc1 = 0, .crn = 12, .crm = 0, .opc2 = 1,
> -      .access = PL3_RW, .writefn = vbar_write, .resetvalue = 0,
> +      .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
> +      .writefn = vbar_write, .resetvalue = 0,
>        .fieldoffset = offsetof(CPUARMState, cp15.mvbar) },
>      { .name = "SCTLR_EL3", .state = ARM_CP_STATE_AA64,
>        .type = ARM_CP_ALIAS, /* reset handled by AArch32 view */

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

--
Alex Bennée

Re: [Qemu-devel] [PATCH 3/7] target-arm: Use access_trap_aa32s_el1() for SCR and MVBAR

[Edgar E. Iglesias]

On Wed, Feb 03, 2016 at 01:38:37PM +0000, Peter Maydell wrote:
> The registers MVBAR and SCR should have the behaviour of trapping to
> EL3 if accessed from Secure EL1, but we were incorrectly implementing
> them to UNDEF (which would trap to EL1).  Fix this by using the new
> access_trap_aa32s_el1() access function.

Hi,

It seems to me like if EL3 is running in AArch32, then we shouldn't
trap accesses from Secure EL1 but I can't find that logic. Am I missing
something?

Cheers,
Edgar


> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/helper.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 8b96b80..d85b04f 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3547,7 +3547,8 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>        .resetvalue = 0, .writefn = scr_write },
>      { .name = "SCR",  .type = ARM_CP_ALIAS,
>        .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 0,
> -      .access = PL3_RW, .fieldoffset = offsetoflow32(CPUARMState, cp15.scr_el3),
> +      .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
> +      .fieldoffset = offsetoflow32(CPUARMState, cp15.scr_el3),
>        .writefn = scr_write },
>      { .name = "MDCR_EL3", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 3, .opc2 = 1,
> @@ -3569,7 +3570,8 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>        .access = PL3_W | PL1_R, .resetvalue = 0,
>        .fieldoffset = offsetof(CPUARMState, cp15.nsacr) },
>      { .name = "MVBAR", .cp = 15, .opc1 = 0, .crn = 12, .crm = 0, .opc2 = 1,
> -      .access = PL3_RW, .writefn = vbar_write, .resetvalue = 0,
> +      .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
> +      .writefn = vbar_write, .resetvalue = 0,
>        .fieldoffset = offsetof(CPUARMState, cp15.mvbar) },
>      { .name = "SCTLR_EL3", .state = ARM_CP_STATE_AA64,
>        .type = ARM_CP_ALIAS, /* reset handled by AArch32 view */
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 3/7] target-arm: Use access_trap_aa32s_el1() for SCR and MVBAR

[Peter Maydell]

On 6 February 2016 at 12:17, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> It seems to me like if EL3 is running in AArch32, then we shouldn't
> trap accesses from Secure EL1 but I can't find that logic. Am I missing
> something?

If EL3 is running in AArch32 then there is no Secure EL1 -- all
of SVC, IRQ, and the other PL1 modes run at EL3 privilege,
and arm_current_el() will return 3 in this situation.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/7] target-arm: Use access_trap_aa32s_el1() for SCR and MVBAR

[Edgar E. Iglesias]

On Sat, Feb 06, 2016 at 01:48:19PM +0000, Peter Maydell wrote:
> On 6 February 2016 at 12:17, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > It seems to me like if EL3 is running in AArch32, then we shouldn't
> > trap accesses from Secure EL1 but I can't find that logic. Am I missing
> > something?
> 
> If EL3 is running in AArch32 then there is no Secure EL1 -- all
> of SVC, IRQ, and the other PL1 modes run at EL3 privilege,
> and arm_current_el() will return 3 in this situation.

Yep, I keep forgetting that for AArch32...

Cheers,
Edgar

Re: [Qemu-devel] [PATCH 3/7] target-arm: Use access_trap_aa32s_el1() for SCR and MVBAR

[Edgar E. Iglesias]

On Wed, Feb 03, 2016 at 01:38:37PM +0000, Peter Maydell wrote:
> The registers MVBAR and SCR should have the behaviour of trapping to
> EL3 if accessed from Secure EL1, but we were incorrectly implementing
> them to UNDEF (which would trap to EL1).  Fix this by using the new
> access_trap_aa32s_el1() access function.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/helper.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 8b96b80..d85b04f 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3547,7 +3547,8 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>        .resetvalue = 0, .writefn = scr_write },
>      { .name = "SCR",  .type = ARM_CP_ALIAS,
>        .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 0,
> -      .access = PL3_RW, .fieldoffset = offsetoflow32(CPUARMState, cp15.scr_el3),
> +      .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
> +      .fieldoffset = offsetoflow32(CPUARMState, cp15.scr_el3),
>        .writefn = scr_write },
>      { .name = "MDCR_EL3", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 3, .opc2 = 1,
> @@ -3569,7 +3570,8 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>        .access = PL3_W | PL1_R, .resetvalue = 0,
>        .fieldoffset = offsetof(CPUARMState, cp15.nsacr) },
>      { .name = "MVBAR", .cp = 15, .opc1 = 0, .crn = 12, .crm = 0, .opc2 = 1,
> -      .access = PL3_RW, .writefn = vbar_write, .resetvalue = 0,
> +      .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
> +      .writefn = vbar_write, .resetvalue = 0,
>        .fieldoffset = offsetof(CPUARMState, cp15.mvbar) },
>      { .name = "SCTLR_EL3", .state = ARM_CP_STATE_AA64,
>        .type = ARM_CP_ALIAS, /* reset handled by AArch32 view */
> -- 
> 1.9.1
> 

[Qemu-devel] [PATCH 4/7] target-arm: Update arm_generate_debug_exceptions() to handle EL2/EL3

[Peter Maydell]

The arm_generate_debug_exceptions() function as originally implemented
assumes no EL2 or EL3. Since we now have much more of an implementation
of those now, fix this assumption.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/cpu.h | 48 +++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 43 insertions(+), 5 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index cf2df50..0fb79d0 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -1742,9 +1742,7 @@ typedef enum ARMASIdx {
     ARMASIdx_S = 1,
 } ARMASIdx;
 
-/* Return the Exception Level targeted by debug exceptions;
- * currently always EL1 since we don't implement EL2 or EL3.
- */
+/* Return the Exception Level targeted by debug exceptions. */
 static inline int arm_debug_target_el(CPUARMState *env)
 {
     bool secure = arm_is_secure(env);
@@ -1767,6 +1765,14 @@ static inline int arm_debug_target_el(CPUARMState *env)
 
 static inline bool aa64_generate_debug_exceptions(CPUARMState *env)
 {
+    if (arm_is_secure(env)) {
+        /* MDCR_EL3.SDD disables debug events from Secure state */
+        if (extract32(env->cp15.mdcr_el3, 16, 1) != 0
+            || arm_current_el(env) == 3) {
+            return false;
+        }
+    }
+
     if (arm_current_el(env) == arm_debug_target_el(env)) {
         if ((extract32(env->cp15.mdscr_el1, 13, 1) == 0)
             || (env->daif & PSTATE_D)) {
@@ -1778,10 +1784,42 @@ static inline bool aa64_generate_debug_exceptions(CPUARMState *env)
 
 static inline bool aa32_generate_debug_exceptions(CPUARMState *env)
 {
-    if (arm_current_el(env) == 0 && arm_el_is_aa64(env, 1)) {
+    int el = arm_current_el(env);
+
+    if (el == 0 && arm_el_is_aa64(env, 1)) {
         return aa64_generate_debug_exceptions(env);
     }
-    return arm_current_el(env) != 2;
+
+    if (arm_is_secure(env)) {
+        int spd;
+
+        if (el == 0 && (env->cp15.sder & 1)) {
+            /* SDER.SUIDEN means debug exceptions from Secure EL0
+             * are always enabled. Otherwise they are controlled by
+             * SDCR.SPD like those from other Secure ELs.
+             */
+            return true;
+        }
+
+        spd = extract32(env->cp15.mdcr_el3, 14, 2);
+        switch (spd) {
+        case 1:
+            /* SPD == 0b01 is reserved, but behaves as 0b00. */
+        case 0:
+            /* For 0b00 we return true if external secure invasive debug
+             * is enabled. On real hardware this is controlled by external
+             * signals to the core. QEMU always permits debug, and behaves
+             * as if DBGEN, SPIDEN, NIDEN and SPNIDEN are all tied high.
+             */
+            return true;
+        case 2:
+            return false;
+        case 3:
+            return true;
+        }
+    }
+
+    return el != 2;
 }
 
 /* Return true if debugging exceptions are currently enabled.
-- 
1.9.1

Re: [Qemu-devel] [PATCH 4/7] target-arm: Update arm_generate_debug_exceptions() to handle EL2/EL3

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> The arm_generate_debug_exceptions() function as originally implemented
> assumes no EL2 or EL3. Since we now have much more of an implementation
> of those now, fix this assumption.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/cpu.h | 48 +++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 43 insertions(+), 5 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index cf2df50..0fb79d0 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1742,9 +1742,7 @@ typedef enum ARMASIdx {
>      ARMASIdx_S = 1,
>  } ARMASIdx;
>
> -/* Return the Exception Level targeted by debug exceptions;
> - * currently always EL1 since we don't implement EL2 or EL3.
> - */
> +/* Return the Exception Level targeted by debug exceptions. */
>  static inline int arm_debug_target_el(CPUARMState *env)
>  {
>      bool secure = arm_is_secure(env);
> @@ -1767,6 +1765,14 @@ static inline int arm_debug_target_el(CPUARMState *env)
>
>  static inline bool aa64_generate_debug_exceptions(CPUARMState *env)
>  {
> +    if (arm_is_secure(env)) {
> +        /* MDCR_EL3.SDD disables debug events from Secure state */

Is it worth commenting that BRK still works?

> +        if (extract32(env->cp15.mdcr_el3,ctct 16, 1) != 0

The != 0 is superfluous here.

> +            || arm_current_el(env) == 3) {
> +            return false;
> +        }
> +    }
> +
>      if (arm_current_el(env) == arm_debug_target_el(env)) {
>          if ((extract32(env->cp15.mdscr_el1, 13, 1) == 0)
>              || (env->daif & PSTATE_D)) {
> @@ -1778,10 +1784,42 @@ static inline bool aa64_generate_debug_exceptions(CPUARMState *env)
>
>  static inline bool aa32_generate_debug_exceptions(CPUARMState *env)
>  {
> -    if (arm_current_el(env) == 0 && arm_el_is_aa64(env, 1)) {
> +    int el = arm_current_el(env);
> +
> +    if (el == 0 && arm_el_is_aa64(env, 1)) {
>          return aa64_generate_debug_exceptions(env);
>      }
> -    return arm_current_el(env) != 2;
> +
> +    if (arm_is_secure(env)) {
> +        int spd;
> +
> +        if (el == 0 && (env->cp15.sder & 1)) {
> +            /* SDER.SUIDEN means debug exceptions from Secure EL0
> +             * are always enabled. Otherwise they are controlled by
> +             * SDCR.SPD like those from other Secure ELs.
> +             */
> +            return true;
> +        }
> +
> +        spd = extract32(env->cp15.mdcr_el3, 14, 2);
> +        switch (spd) {
> +        case 1:
> +            /* SPD == 0b01 is reserved, but behaves as 0b00. */
> +        case 0:
> +            /* For 0b00 we return true if external secure invasive debug
> +             * is enabled. On real hardware this is controlled by external
> +             * signals to the core. QEMU always permits debug, and behaves
> +             * as if DBGEN, SPIDEN, NIDEN and SPNIDEN are all tied high.
> +             */
> +            return true;
> +        case 2:
> +            return false;
> +        case 3:
> +            return true;
> +        }
> +    }
> +
> +    return el != 2;
>  }
>
>  /* Return true if debugging exceptions are currently enabled.

Otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

--
Alex Bennée

Re: [Qemu-devel] [PATCH 4/7] target-arm: Update arm_generate_debug_exceptions() to handle EL2/EL3

[Peter Maydell]

On 5 February 2016 at 14:09, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>> @@ -1767,6 +1765,14 @@ static inline int arm_debug_target_el(CPUARMState *env)
>>
>>  static inline bool aa64_generate_debug_exceptions(CPUARMState *env)
>>  {
>> +    if (arm_is_secure(env)) {
>> +        /* MDCR_EL3.SDD disables debug events from Secure state */
>
> Is it worth commenting that BRK still works?

This is true for all the checks made by this family of functions
so if we were going to say anything about BRK then this would not
be the right spot for it.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/7] target-arm: Update arm_generate_debug_exceptions() to handle EL2/EL3

[Sergey Fedorov]

On 03.02.2016 16:38, Peter Maydell wrote:
> The arm_generate_debug_exceptions() function as originally implemented
> assumes no EL2 or EL3. Since we now have much more of an implementation
> of those now, fix this assumption.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>

> ---
>  target-arm/cpu.h | 48 +++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 43 insertions(+), 5 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index cf2df50..0fb79d0 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1742,9 +1742,7 @@ typedef enum ARMASIdx {
>      ARMASIdx_S = 1,
>  } ARMASIdx;
>  
> -/* Return the Exception Level targeted by debug exceptions;
> - * currently always EL1 since we don't implement EL2 or EL3.
> - */
> +/* Return the Exception Level targeted by debug exceptions. */
>  static inline int arm_debug_target_el(CPUARMState *env)
>  {
>      bool secure = arm_is_secure(env);
> @@ -1767,6 +1765,14 @@ static inline int arm_debug_target_el(CPUARMState *env)
>  
>  static inline bool aa64_generate_debug_exceptions(CPUARMState *env)
>  {
> +    if (arm_is_secure(env)) {
> +        /* MDCR_EL3.SDD disables debug events from Secure state */
> +        if (extract32(env->cp15.mdcr_el3, 16, 1) != 0
> +            || arm_current_el(env) == 3) {
> +            return false;
> +        }
> +    }
> +
>      if (arm_current_el(env) == arm_debug_target_el(env)) {
>          if ((extract32(env->cp15.mdscr_el1, 13, 1) == 0)
>              || (env->daif & PSTATE_D)) {
> @@ -1778,10 +1784,42 @@ static inline bool aa64_generate_debug_exceptions(CPUARMState *env)
>  
>  static inline bool aa32_generate_debug_exceptions(CPUARMState *env)
>  {
> -    if (arm_current_el(env) == 0 && arm_el_is_aa64(env, 1)) {
> +    int el = arm_current_el(env);
> +
> +    if (el == 0 && arm_el_is_aa64(env, 1)) {
>          return aa64_generate_debug_exceptions(env);
>      }
> -    return arm_current_el(env) != 2;
> +
> +    if (arm_is_secure(env)) {
> +        int spd;
> +
> +        if (el == 0 && (env->cp15.sder & 1)) {
> +            /* SDER.SUIDEN means debug exceptions from Secure EL0
> +             * are always enabled. Otherwise they are controlled by
> +             * SDCR.SPD like those from other Secure ELs.
> +             */
> +            return true;
> +        }
> +
> +        spd = extract32(env->cp15.mdcr_el3, 14, 2);
> +        switch (spd) {
> +        case 1:
> +            /* SPD == 0b01 is reserved, but behaves as 0b00. */
> +        case 0:
> +            /* For 0b00 we return true if external secure invasive debug
> +             * is enabled. On real hardware this is controlled by external
> +             * signals to the core. QEMU always permits debug, and behaves
> +             * as if DBGEN, SPIDEN, NIDEN and SPNIDEN are all tied high.
> +             */
> +            return true;
> +        case 2:
> +            return false;
> +        case 3:
> +            return true;
> +        }
> +    }
> +
> +    return el != 2;
>  }
>  
>  /* Return true if debugging exceptions are currently enabled.

[Qemu-devel] [PATCH 5/7] target-arm: Add isread parameter to CPAccessFns

[Peter Maydell]

System registers might have access requirements which need to
be described via a CPAccessFn and which differ for reads and
writes. For this to be possible we need to pass the access
function a parameter to tell it whether the access being checked
is a read or a write.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/cpu.h           |  4 ++-
 target-arm/helper.c        | 81 +++++++++++++++++++++++++++++-----------------
 target-arm/helper.h        |  2 +-
 target-arm/op_helper.c     |  5 +--
 target-arm/translate-a64.c |  6 ++--
 target-arm/translate.c     |  7 ++--
 6 files changed, 68 insertions(+), 37 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 0fb79d0..5137632 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -1319,7 +1319,9 @@ typedef uint64_t CPReadFn(CPUARMState *env, const ARMCPRegInfo *opaque);
 typedef void CPWriteFn(CPUARMState *env, const ARMCPRegInfo *opaque,
                        uint64_t value);
 /* Access permission check functions for coprocessor registers. */
-typedef CPAccessResult CPAccessFn(CPUARMState *env, const ARMCPRegInfo *opaque);
+typedef CPAccessResult CPAccessFn(CPUARMState *env,
+                                  const ARMCPRegInfo *opaque,
+                                  bool isread);
 /* Hook function for register reset */
 typedef void CPResetFn(CPUARMState *env, const ARMCPRegInfo *opaque);
 
diff --git a/target-arm/helper.c b/target-arm/helper.c
index d85b04f..8bc3ea3 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -344,7 +344,8 @@ void init_cpreg_list(ARMCPU *cpu)
  * access_el3_aa32ns_aa64any: Used to check both AArch32/64 register views.
  */
 static CPAccessResult access_el3_aa32ns(CPUARMState *env,
-                                        const ARMCPRegInfo *ri)
+                                        const ARMCPRegInfo *ri,
+                                        bool isread)
 {
     bool secure = arm_is_secure_below_el3(env);
 
@@ -356,10 +357,11 @@ static CPAccessResult access_el3_aa32ns(CPUARMState *env,
 }
 
 static CPAccessResult access_el3_aa32ns_aa64any(CPUARMState *env,
-                                                const ARMCPRegInfo *ri)
+                                                const ARMCPRegInfo *ri,
+                                                bool isread)
 {
     if (!arm_el_is_aa64(env, 3)) {
-        return access_el3_aa32ns(env, ri);
+        return access_el3_aa32ns(env, ri, isread);
     }
     return CP_ACCESS_OK;
 }
@@ -369,7 +371,8 @@ static CPAccessResult access_el3_aa32ns_aa64any(CPUARMState *env,
  * We assume that the .access field is set to PL1_RW.
  */
 static CPAccessResult access_trap_aa32s_el1(CPUARMState *env,
-                                            const ARMCPRegInfo *ri)
+                                            const ARMCPRegInfo *ri,
+                                            bool isread)
 {
     if (arm_current_el(env) == 3) {
         return CP_ACCESS_OK;
@@ -651,7 +654,8 @@ static void cpacr_write(CPUARMState *env, const ARMCPRegInfo *ri,
     env->cp15.cpacr_el1 = value;
 }
 
-static CPAccessResult cpacr_access(CPUARMState *env, const ARMCPRegInfo *ri)
+static CPAccessResult cpacr_access(CPUARMState *env, const ARMCPRegInfo *ri,
+                                   bool isread)
 {
     if (arm_feature(env, ARM_FEATURE_V8)) {
         /* Check if CPACR accesses are to be trapped to EL2 */
@@ -668,7 +672,8 @@ static CPAccessResult cpacr_access(CPUARMState *env, const ARMCPRegInfo *ri)
     return CP_ACCESS_OK;
 }
 
-static CPAccessResult cptr_access(CPUARMState *env, const ARMCPRegInfo *ri)
+static CPAccessResult cptr_access(CPUARMState *env, const ARMCPRegInfo *ri,
+                                  bool isread)
 {
     /* Check if CPTR accesses are set to trap to EL3 */
     if (arm_current_el(env) == 2 && (env->cp15.cptr_el[3] & CPTR_TCPAC)) {
@@ -710,7 +715,8 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
     REGINFO_SENTINEL
 };
 
-static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri)
+static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri,
+                                   bool isread)
 {
     /* Performance monitor registers user accessibility is controlled
      * by PMUSERENR.
@@ -1154,7 +1160,8 @@ static void teecr_write(CPUARMState *env, const ARMCPRegInfo *ri,
     env->teecr = value;
 }
 
-static CPAccessResult teehbr_access(CPUARMState *env, const ARMCPRegInfo *ri)
+static CPAccessResult teehbr_access(CPUARMState *env, const ARMCPRegInfo *ri,
+                                    bool isread)
 {
     if (arm_current_el(env) == 0 && (env->teecr & 1)) {
         return CP_ACCESS_TRAP;
@@ -1207,7 +1214,8 @@ static const ARMCPRegInfo v6k_cp_reginfo[] = {
 
 #ifndef CONFIG_USER_ONLY
 
-static CPAccessResult gt_cntfrq_access(CPUARMState *env, const ARMCPRegInfo *ri)
+static CPAccessResult gt_cntfrq_access(CPUARMState *env, const ARMCPRegInfo *ri,
+                                       bool isread)
 {
     /* CNTFRQ: not visible from PL0 if both PL0PCTEN and PL0VCTEN are zero */
     if (arm_current_el(env) == 0 && !extract32(env->cp15.c14_cntkctl, 0, 2)) {
@@ -1216,7 +1224,8 @@ static CPAccessResult gt_cntfrq_access(CPUARMState *env, const ARMCPRegInfo *ri)
     return CP_ACCESS_OK;
 }
 
-static CPAccessResult gt_counter_access(CPUARMState *env, int timeridx)
+static CPAccessResult gt_counter_access(CPUARMState *env, int timeridx,
+                                        bool isread)
 {
     unsigned int cur_el = arm_current_el(env);
     bool secure = arm_is_secure(env);
@@ -1235,7 +1244,8 @@ static CPAccessResult gt_counter_access(CPUARMState *env, int timeridx)
     return CP_ACCESS_OK;
 }
 
-static CPAccessResult gt_timer_access(CPUARMState *env, int timeridx)
+static CPAccessResult gt_timer_access(CPUARMState *env, int timeridx,
+                                      bool isread)
 {
     unsigned int cur_el = arm_current_el(env);
     bool secure = arm_is_secure(env);
@@ -1257,29 +1267,34 @@ static CPAccessResult gt_timer_access(CPUARMState *env, int timeridx)
 }
 
 static CPAccessResult gt_pct_access(CPUARMState *env,
-                                         const ARMCPRegInfo *ri)
+                                    const ARMCPRegInfo *ri,
+                                    bool isread)
 {
-    return gt_counter_access(env, GTIMER_PHYS);
+    return gt_counter_access(env, GTIMER_PHYS, isread);
 }
 
 static CPAccessResult gt_vct_access(CPUARMState *env,
-                                         const ARMCPRegInfo *ri)
+                                    const ARMCPRegInfo *ri,
+                                    bool isread)
 {
-    return gt_counter_access(env, GTIMER_VIRT);
+    return gt_counter_access(env, GTIMER_VIRT, isread);
 }
 
-static CPAccessResult gt_ptimer_access(CPUARMState *env, const ARMCPRegInfo *ri)
+static CPAccessResult gt_ptimer_access(CPUARMState *env, const ARMCPRegInfo *ri,
+                                       bool isread)
 {
-    return gt_timer_access(env, GTIMER_PHYS);
+    return gt_timer_access(env, GTIMER_PHYS, isread);
 }
 
-static CPAccessResult gt_vtimer_access(CPUARMState *env, const ARMCPRegInfo *ri)
+static CPAccessResult gt_vtimer_access(CPUARMState *env, const ARMCPRegInfo *ri,
+                                       bool isread)
 {
-    return gt_timer_access(env, GTIMER_VIRT);
+    return gt_timer_access(env, GTIMER_VIRT, isread);
 }
 
 static CPAccessResult gt_stimer_access(CPUARMState *env,
-                                       const ARMCPRegInfo *ri)
+                                       const ARMCPRegInfo *ri,
+                                       bool isread)
 {
     /* The AArch64 register view of the secure physical timer is
      * always accessible from EL3, and configurably accessible from
@@ -1776,7 +1791,8 @@ static void par_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 #ifndef CONFIG_USER_ONLY
 /* get_phys_addr() isn't present for user-mode-only targets */
 
-static CPAccessResult ats_access(CPUARMState *env, const ARMCPRegInfo *ri)
+static CPAccessResult ats_access(CPUARMState *env, const ARMCPRegInfo *ri,
+                                 bool isread)
 {
     if (ri->opc2 & 4) {
         /* The ATS12NSO* operations must trap to EL3 if executed in
@@ -1921,7 +1937,8 @@ static void ats1h_write(CPUARMState *env, const ARMCPRegInfo *ri,
     A32_BANKED_CURRENT_REG_SET(env, par, par64);
 }
 
-static CPAccessResult at_s1e2_access(CPUARMState *env, const ARMCPRegInfo *ri)
+static CPAccessResult at_s1e2_access(CPUARMState *env, const ARMCPRegInfo *ri,
+                                     bool isread)
 {
     if (arm_current_el(env) == 3 && !(env->cp15.scr_el3 & SCR_NS)) {
         return CP_ACCESS_TRAP;
@@ -2575,7 +2592,8 @@ static void aa64_fpsr_write(CPUARMState *env, const ARMCPRegInfo *ri,
     vfp_set_fpsr(env, value);
 }
 
-static CPAccessResult aa64_daif_access(CPUARMState *env, const ARMCPRegInfo *ri)
+static CPAccessResult aa64_daif_access(CPUARMState *env, const ARMCPRegInfo *ri,
+                                       bool isread)
 {
     if (arm_current_el(env) == 0 && !(env->cp15.sctlr_el[1] & SCTLR_UMA)) {
         return CP_ACCESS_TRAP;
@@ -2590,7 +2608,8 @@ static void aa64_daif_write(CPUARMState *env, const ARMCPRegInfo *ri,
 }
 
 static CPAccessResult aa64_cacheop_access(CPUARMState *env,
-                                          const ARMCPRegInfo *ri)
+                                          const ARMCPRegInfo *ri,
+                                          bool isread)
 {
     /* Cache invalidate/clean: NOP, but EL0 must UNDEF unless
      * SCTLR_EL1.UCI is set.
@@ -2846,7 +2865,8 @@ static void tlbi_aa64_ipas2e1is_write(CPUARMState *env, const ARMCPRegInfo *ri,
     }
 }
 
-static CPAccessResult aa64_zva_access(CPUARMState *env, const ARMCPRegInfo *ri)
+static CPAccessResult aa64_zva_access(CPUARMState *env, const ARMCPRegInfo *ri,
+                                      bool isread)
 {
     /* We don't implement EL2, so the only control on DC ZVA is the
      * bit in the SCTLR which can prohibit access for EL0.
@@ -2863,13 +2883,14 @@ static uint64_t aa64_dczid_read(CPUARMState *env, const ARMCPRegInfo *ri)
     int dzp_bit = 1 << 4;
 
     /* DZP indicates whether DC ZVA access is allowed */
-    if (aa64_zva_access(env, NULL) == CP_ACCESS_OK) {
+    if (aa64_zva_access(env, NULL, false) == CP_ACCESS_OK) {
         dzp_bit = 0;
     }
     return cpu->dcz_blocksize | dzp_bit;
 }
 
-static CPAccessResult sp_el0_access(CPUARMState *env, const ARMCPRegInfo *ri)
+static CPAccessResult sp_el0_access(CPUARMState *env, const ARMCPRegInfo *ri,
+                                    bool isread)
 {
     if (!(env->pstate & PSTATE_SP)) {
         /* Access to SP_EL0 is undefined if it's being used as
@@ -2908,7 +2929,8 @@ static void sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri,
     tlb_flush(CPU(cpu), 1);
 }
 
-static CPAccessResult fpexc32_access(CPUARMState *env, const ARMCPRegInfo *ri)
+static CPAccessResult fpexc32_access(CPUARMState *env, const ARMCPRegInfo *ri,
+                                     bool isread)
 {
     if ((env->cp15.cptr_el[2] & CPTR_TFP) && arm_current_el(env) == 2) {
         return CP_ACCESS_TRAP_EL2;
@@ -3656,7 +3678,8 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
     REGINFO_SENTINEL
 };
 
-static CPAccessResult ctr_el0_access(CPUARMState *env, const ARMCPRegInfo *ri)
+static CPAccessResult ctr_el0_access(CPUARMState *env, const ARMCPRegInfo *ri,
+                                     bool isread)
 {
     /* Only accessible in EL0 if SCTLR.UCT is set (and only in AArch64,
      * but the AArch32 CTR has its own reginfo struct)
diff --git a/target-arm/helper.h b/target-arm/helper.h
index c2a85c7..c98e9ce 100644
--- a/target-arm/helper.h
+++ b/target-arm/helper.h
@@ -62,7 +62,7 @@ DEF_HELPER_1(cpsr_read, i32, env)
 DEF_HELPER_3(v7m_msr, void, env, i32, i32)
 DEF_HELPER_2(v7m_mrs, i32, env, i32)
 
-DEF_HELPER_3(access_check_cp_reg, void, env, ptr, i32)
+DEF_HELPER_4(access_check_cp_reg, void, env, ptr, i32, i32)
 DEF_HELPER_3(set_cp_reg, void, env, ptr, i32)
 DEF_HELPER_2(get_cp_reg, i32, env, ptr)
 DEF_HELPER_3(set_cp_reg64, void, env, ptr, i64)
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index a5ee65f..313c0f8 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -457,7 +457,8 @@ void HELPER(set_user_reg)(CPUARMState *env, uint32_t regno, uint32_t val)
     }
 }
 
-void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip, uint32_t syndrome)
+void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip, uint32_t syndrome,
+                                 uint32_t isread)
 {
     const ARMCPRegInfo *ri = rip;
     int target_el;
@@ -471,7 +472,7 @@ void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip, uint32_t syndrome)
         return;
     }
 
-    switch (ri->accessfn(env, ri)) {
+    switch (ri->accessfn(env, ri, isread)) {
     case CP_ACCESS_OK:
         return;
     case CP_ACCESS_TRAP:
diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 80f6c20..8f1eaad 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -1366,16 +1366,18 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
          * runtime; this may result in an exception.
          */
         TCGv_ptr tmpptr;
-        TCGv_i32 tcg_syn;
+        TCGv_i32 tcg_syn, tcg_isread;
         uint32_t syndrome;
 
         gen_a64_set_pc_im(s->pc - 4);
         tmpptr = tcg_const_ptr(ri);
         syndrome = syn_aa64_sysregtrap(op0, op1, op2, crn, crm, rt, isread);
         tcg_syn = tcg_const_i32(syndrome);
-        gen_helper_access_check_cp_reg(cpu_env, tmpptr, tcg_syn);
+        tcg_isread = tcg_const_i32(isread);
+        gen_helper_access_check_cp_reg(cpu_env, tmpptr, tcg_syn, tcg_isread);
         tcg_temp_free_ptr(tmpptr);
         tcg_temp_free_i32(tcg_syn);
+        tcg_temp_free_i32(tcg_isread);
     }
 
     /* Handle special cases first */
diff --git a/target-arm/translate.c b/target-arm/translate.c
index cff511b..375acf5 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -7168,7 +7168,7 @@ static int disas_coproc_insn(DisasContext *s, uint32_t insn)
              * call in order to handle c15_cpar.
              */
             TCGv_ptr tmpptr;
-            TCGv_i32 tcg_syn;
+            TCGv_i32 tcg_syn, tcg_isread;
             uint32_t syndrome;
 
             /* Note that since we are an implementation which takes an
@@ -7213,9 +7213,12 @@ static int disas_coproc_insn(DisasContext *s, uint32_t insn)
             gen_set_pc_im(s, s->pc - 4);
             tmpptr = tcg_const_ptr(ri);
             tcg_syn = tcg_const_i32(syndrome);
-            gen_helper_access_check_cp_reg(cpu_env, tmpptr, tcg_syn);
+            tcg_isread = tcg_const_i32(isread);
+            gen_helper_access_check_cp_reg(cpu_env, tmpptr, tcg_syn,
+                                           tcg_isread);
             tcg_temp_free_ptr(tmpptr);
             tcg_temp_free_i32(tcg_syn);
+            tcg_temp_free_i32(tcg_isread);
         }
 
         /* Handle special cases first */
-- 
1.9.1

Re: [Qemu-devel] [PATCH 5/7] target-arm: Add isread parameter to CPAccessFns

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> System registers might have access requirements which need to
> be described via a CPAccessFn and which differ for reads and
> writes. For this to be possible we need to pass the access
> function a parameter to tell it whether the access being checked
> is a read or a write.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/cpu.h           |  4 ++-
>  target-arm/helper.c        | 81 +++++++++++++++++++++++++++++-----------------
>  target-arm/helper.h        |  2 +-
>  target-arm/op_helper.c     |  5 +--
>  target-arm/translate-a64.c |  6 ++--
>  target-arm/translate.c     |  7 ++--
>  6 files changed, 68 insertions(+), 37 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 0fb79d0..5137632 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1319,7 +1319,9 @@ typedef uint64_t CPReadFn(CPUARMState *env, const ARMCPRegInfo *opaque);
>  typedef void CPWriteFn(CPUARMState *env, const ARMCPRegInfo *opaque,
>                         uint64_t value);
>  /* Access permission check functions for coprocessor registers. */
> -typedef CPAccessResult CPAccessFn(CPUARMState *env, const ARMCPRegInfo *opaque);
> +typedef CPAccessResult CPAccessFn(CPUARMState *env,
> +                                  const ARMCPRegInfo *opaque,
> +                                  bool isread);

I guess my only comment here is we've extended the call for every access
check with another parameter (and associated TCG activity) for something
only one handler currently cares about.

Is there an argument for an rwaccessfn() that we use for just those
registers that care about the detail? I know system registers are hardly
a fast path priority but I'm concerned about knock on effects on
performance. Have you done any measurements?

>  /* Hook function for register reset */
>  typedef void CPResetFn(CPUARMState *env, const ARMCPRegInfo *opaque);
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index d85b04f..8bc3ea3 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -344,7 +344,8 @@ void init_cpreg_list(ARMCPU *cpu)
>   * access_el3_aa32ns_aa64any: Used to check both AArch32/64 register views.
>   */
>  static CPAccessResult access_el3_aa32ns(CPUARMState *env,
> -                                        const ARMCPRegInfo *ri)
> +                                        const ARMCPRegInfo *ri,
> +                                        bool isread)
>  {
>      bool secure = arm_is_secure_below_el3(env);
>
> @@ -356,10 +357,11 @@ static CPAccessResult access_el3_aa32ns(CPUARMState *env,
>  }
>
>  static CPAccessResult access_el3_aa32ns_aa64any(CPUARMState *env,
> -                                                const ARMCPRegInfo *ri)
> +                                                const ARMCPRegInfo *ri,
> +                                                bool isread)
>  {
>      if (!arm_el_is_aa64(env, 3)) {
> -        return access_el3_aa32ns(env, ri);
> +        return access_el3_aa32ns(env, ri, isread);
>      }
>      return CP_ACCESS_OK;
>  }
> @@ -369,7 +371,8 @@ static CPAccessResult access_el3_aa32ns_aa64any(CPUARMState *env,
>   * We assume that the .access field is set to PL1_RW.
>   */
>  static CPAccessResult access_trap_aa32s_el1(CPUARMState *env,
> -                                            const ARMCPRegInfo *ri)
> +                                            const ARMCPRegInfo *ri,
> +                                            bool isread)
>  {
>      if (arm_current_el(env) == 3) {
>          return CP_ACCESS_OK;
> @@ -651,7 +654,8 @@ static void cpacr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      env->cp15.cpacr_el1 = value;
>  }
>
> -static CPAccessResult cpacr_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult cpacr_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                   bool isread)
>  {
>      if (arm_feature(env, ARM_FEATURE_V8)) {
>          /* Check if CPACR accesses are to be trapped to EL2 */
> @@ -668,7 +672,8 @@ static CPAccessResult cpacr_access(CPUARMState *env, const ARMCPRegInfo *ri)
>      return CP_ACCESS_OK;
>  }
>
> -static CPAccessResult cptr_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult cptr_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                  bool isread)
>  {
>      /* Check if CPTR accesses are set to trap to EL3 */
>      if (arm_current_el(env) == 2 && (env->cp15.cptr_el[3] & CPTR_TCPAC)) {
> @@ -710,7 +715,8 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
>      REGINFO_SENTINEL
>  };
>
> -static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                   bool isread)
>  {
>      /* Performance monitor registers user accessibility is controlled
>       * by PMUSERENR.
> @@ -1154,7 +1160,8 @@ static void teecr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      env->teecr = value;
>  }
>
> -static CPAccessResult teehbr_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult teehbr_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                    bool isread)
>  {
>      if (arm_current_el(env) == 0 && (env->teecr & 1)) {
>          return CP_ACCESS_TRAP;
> @@ -1207,7 +1214,8 @@ static const ARMCPRegInfo v6k_cp_reginfo[] = {
>
>  #ifndef CONFIG_USER_ONLY
>
> -static CPAccessResult gt_cntfrq_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult gt_cntfrq_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                       bool isread)
>  {
>      /* CNTFRQ: not visible from PL0 if both PL0PCTEN and PL0VCTEN are zero */
>      if (arm_current_el(env) == 0 && !extract32(env->cp15.c14_cntkctl, 0, 2)) {
> @@ -1216,7 +1224,8 @@ static CPAccessResult gt_cntfrq_access(CPUARMState *env, const ARMCPRegInfo *ri)
>      return CP_ACCESS_OK;
>  }
>
> -static CPAccessResult gt_counter_access(CPUARMState *env, int timeridx)
> +static CPAccessResult gt_counter_access(CPUARMState *env, int timeridx,
> +                                        bool isread)
>  {
>      unsigned int cur_el = arm_current_el(env);
>      bool secure = arm_is_secure(env);
> @@ -1235,7 +1244,8 @@ static CPAccessResult gt_counter_access(CPUARMState *env, int timeridx)
>      return CP_ACCESS_OK;
>  }
>
> -static CPAccessResult gt_timer_access(CPUARMState *env, int timeridx)
> +static CPAccessResult gt_timer_access(CPUARMState *env, int timeridx,
> +                                      bool isread)
>  {
>      unsigned int cur_el = arm_current_el(env);
>      bool secure = arm_is_secure(env);
> @@ -1257,29 +1267,34 @@ static CPAccessResult gt_timer_access(CPUARMState *env, int timeridx)
>  }
>
>  static CPAccessResult gt_pct_access(CPUARMState *env,
> -                                         const ARMCPRegInfo *ri)
> +                                    const ARMCPRegInfo *ri,
> +                                    bool isread)
>  {
> -    return gt_counter_access(env, GTIMER_PHYS);
> +    return gt_counter_access(env, GTIMER_PHYS, isread);
>  }
>
>  static CPAccessResult gt_vct_access(CPUARMState *env,
> -                                         const ARMCPRegInfo *ri)
> +                                    const ARMCPRegInfo *ri,
> +                                    bool isread)
>  {
> -    return gt_counter_access(env, GTIMER_VIRT);
> +    return gt_counter_access(env, GTIMER_VIRT, isread);
>  }
>
> -static CPAccessResult gt_ptimer_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult gt_ptimer_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                       bool isread)
>  {
> -    return gt_timer_access(env, GTIMER_PHYS);
> +    return gt_timer_access(env, GTIMER_PHYS, isread);
>  }
>
> -static CPAccessResult gt_vtimer_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult gt_vtimer_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                       bool isread)
>  {
> -    return gt_timer_access(env, GTIMER_VIRT);
> +    return gt_timer_access(env, GTIMER_VIRT, isread);
>  }
>
>  static CPAccessResult gt_stimer_access(CPUARMState *env,
> -                                       const ARMCPRegInfo *ri)
> +                                       const ARMCPRegInfo *ri,
> +                                       bool isread)
>  {
>      /* The AArch64 register view of the secure physical timer is
>       * always accessible from EL3, and configurably accessible from
> @@ -1776,7 +1791,8 @@ static void par_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>  #ifndef CONFIG_USER_ONLY
>  /* get_phys_addr() isn't present for user-mode-only targets */
>
> -static CPAccessResult ats_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult ats_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                 bool isread)
>  {
>      if (ri->opc2 & 4) {
>          /* The ATS12NSO* operations must trap to EL3 if executed in
> @@ -1921,7 +1937,8 @@ static void ats1h_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      A32_BANKED_CURRENT_REG_SET(env, par, par64);
>  }
>
> -static CPAccessResult at_s1e2_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult at_s1e2_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                     bool isread)
>  {
>      if (arm_current_el(env) == 3 && !(env->cp15.scr_el3 & SCR_NS)) {
>          return CP_ACCESS_TRAP;
> @@ -2575,7 +2592,8 @@ static void aa64_fpsr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      vfp_set_fpsr(env, value);
>  }
>
> -static CPAccessResult aa64_daif_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult aa64_daif_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                       bool isread)
>  {
>      if (arm_current_el(env) == 0 && !(env->cp15.sctlr_el[1] & SCTLR_UMA)) {
>          return CP_ACCESS_TRAP;
> @@ -2590,7 +2608,8 @@ static void aa64_daif_write(CPUARMState *env, const ARMCPRegInfo *ri,
>  }
>
>  static CPAccessResult aa64_cacheop_access(CPUARMState *env,
> -                                          const ARMCPRegInfo *ri)
> +                                          const ARMCPRegInfo *ri,
> +                                          bool isread)
>  {
>      /* Cache invalidate/clean: NOP, but EL0 must UNDEF unless
>       * SCTLR_EL1.UCI is set.
> @@ -2846,7 +2865,8 @@ static void tlbi_aa64_ipas2e1is_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      }
>  }
>
> -static CPAccessResult aa64_zva_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult aa64_zva_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                      bool isread)
>  {
>      /* We don't implement EL2, so the only control on DC ZVA is the
>       * bit in the SCTLR which can prohibit access for EL0.
> @@ -2863,13 +2883,14 @@ static uint64_t aa64_dczid_read(CPUARMState *env, const ARMCPRegInfo *ri)
>      int dzp_bit = 1 << 4;
>
>      /* DZP indicates whether DC ZVA access is allowed */
> -    if (aa64_zva_access(env, NULL) == CP_ACCESS_OK) {
> +    if (aa64_zva_access(env, NULL, false) == CP_ACCESS_OK) {
>          dzp_bit = 0;
>      }
>      return cpu->dcz_blocksize | dzp_bit;
>  }
>
> -static CPAccessResult sp_el0_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult sp_el0_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                    bool isread)
>  {
>      if (!(env->pstate & PSTATE_SP)) {
>          /* Access to SP_EL0 is undefined if it's being used as
> @@ -2908,7 +2929,8 @@ static void sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      tlb_flush(CPU(cpu), 1);
>  }
>
> -static CPAccessResult fpexc32_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult fpexc32_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                     bool isread)
>  {
>      if ((env->cp15.cptr_el[2] & CPTR_TFP) && arm_current_el(env) == 2) {
>          return CP_ACCESS_TRAP_EL2;
> @@ -3656,7 +3678,8 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>      REGINFO_SENTINEL
>  };
>
> -static CPAccessResult ctr_el0_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult ctr_el0_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                     bool isread)
>  {
>      /* Only accessible in EL0 if SCTLR.UCT is set (and only in AArch64,
>       * but the AArch32 CTR has its own reginfo struct)
> diff --git a/target-arm/helper.h b/target-arm/helper.h
> index c2a85c7..c98e9ce 100644
> --- a/target-arm/helper.h
> +++ b/target-arm/helper.h
> @@ -62,7 +62,7 @@ DEF_HELPER_1(cpsr_read, i32, env)
>  DEF_HELPER_3(v7m_msr, void, env, i32, i32)
>  DEF_HELPER_2(v7m_mrs, i32, env, i32)
>
> -DEF_HELPER_3(access_check_cp_reg, void, env, ptr, i32)
> +DEF_HELPER_4(access_check_cp_reg, void, env, ptr, i32, i32)
>  DEF_HELPER_3(set_cp_reg, void, env, ptr, i32)
>  DEF_HELPER_2(get_cp_reg, i32, env, ptr)
>  DEF_HELPER_3(set_cp_reg64, void, env, ptr, i64)
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index a5ee65f..313c0f8 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -457,7 +457,8 @@ void HELPER(set_user_reg)(CPUARMState *env, uint32_t regno, uint32_t val)
>      }
>  }
>
> -void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip, uint32_t syndrome)
> +void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip, uint32_t syndrome,
> +                                 uint32_t isread)
>  {
>      const ARMCPRegInfo *ri = rip;
>      int target_el;
> @@ -471,7 +472,7 @@ void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip, uint32_t syndrome)
>          return;
>      }
>
> -    switch (ri->accessfn(env, ri)) {
> +    switch (ri->accessfn(env, ri, isread)) {
>      case CP_ACCESS_OK:
>          return;
>      case CP_ACCESS_TRAP:
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index 80f6c20..8f1eaad 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -1366,16 +1366,18 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
>           * runtime; this may result in an exception.
>           */
>          TCGv_ptr tmpptr;
> -        TCGv_i32 tcg_syn;
> +        TCGv_i32 tcg_syn, tcg_isread;
>          uint32_t syndrome;
>
>          gen_a64_set_pc_im(s->pc - 4);
>          tmpptr = tcg_const_ptr(ri);
>          syndrome = syn_aa64_sysregtrap(op0, op1, op2, crn, crm, rt, isread);
>          tcg_syn = tcg_const_i32(syndrome);
> -        gen_helper_access_check_cp_reg(cpu_env, tmpptr, tcg_syn);
> +        tcg_isread = tcg_const_i32(isread);
> +        gen_helper_access_check_cp_reg(cpu_env, tmpptr, tcg_syn, tcg_isread);
>          tcg_temp_free_ptr(tmpptr);
>          tcg_temp_free_i32(tcg_syn);
> +        tcg_temp_free_i32(tcg_isread);
>      }
>
>      /* Handle special cases first */
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index cff511b..375acf5 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -7168,7 +7168,7 @@ static int disas_coproc_insn(DisasContext *s, uint32_t insn)
>               * call in order to handle c15_cpar.
>               */
>              TCGv_ptr tmpptr;
> -            TCGv_i32 tcg_syn;
> +            TCGv_i32 tcg_syn, tcg_isread;
>              uint32_t syndrome;
>
>              /* Note that since we are an implementation which takes an
> @@ -7213,9 +7213,12 @@ static int disas_coproc_insn(DisasContext *s, uint32_t insn)
>              gen_set_pc_im(s, s->pc - 4);
>              tmpptr = tcg_const_ptr(ri);
>              tcg_syn = tcg_const_i32(syndrome);
> -            gen_helper_access_check_cp_reg(cpu_env, tmpptr, tcg_syn);
> +            tcg_isread = tcg_const_i32(isread);
> +            gen_helper_access_check_cp_reg(cpu_env, tmpptr, tcg_syn,
> +                                           tcg_isread);
>              tcg_temp_free_ptr(tmpptr);
>              tcg_temp_free_i32(tcg_syn);
> +            tcg_temp_free_i32(tcg_isread);
>          }
>
>          /* Handle special cases first */


--
Alex Bennée

Re: [Qemu-devel] [PATCH 5/7] target-arm: Add isread parameter to CPAccessFns

[Peter Maydell]

On 5 February 2016 at 14:20, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>> -typedef CPAccessResult CPAccessFn(CPUARMState *env, const ARMCPRegInfo *opaque);
>> +typedef CPAccessResult CPAccessFn(CPUARMState *env,
>> +                                  const ARMCPRegInfo *opaque,
>> +                                  bool isread);
>
> I guess my only comment here is we've extended the call for every access
> check with another parameter (and associated TCG activity) for something
> only one handler currently cares about.
>
> Is there an argument for an rwaccessfn() that we use for just those
> registers that care about the detail? I know system registers are hardly
> a fast path priority but I'm concerned about knock on effects on
> performance. Have you done any measurements?

I haven't measured, no, but since there are only 3 arguments the
third argument is going to be in a register on any host architecture
we care about, which means the overhead is just going to be a single
"load constant 0 or 1 into register before the call". I think that's
going to be lost in the noise compared to actually having to make
the function call at all, the work the function call does, and then
the second function call later to do the read or write.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 5/7] target-arm: Add isread parameter to CPAccessFns

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 5 February 2016 at 14:20, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>> -typedef CPAccessResult CPAccessFn(CPUARMState *env, const ARMCPRegInfo *opaque);
>>> +typedef CPAccessResult CPAccessFn(CPUARMState *env,
>>> +                                  const ARMCPRegInfo *opaque,
>>> +                                  bool isread);
>>
>> I guess my only comment here is we've extended the call for every access
>> check with another parameter (and associated TCG activity) for something
>> only one handler currently cares about.
>>
>> Is there an argument for an rwaccessfn() that we use for just those
>> registers that care about the detail? I know system registers are hardly
>> a fast path priority but I'm concerned about knock on effects on
>> performance. Have you done any measurements?
>
> I haven't measured, no, but since there are only 3 arguments the
> third argument is going to be in a register on any host architecture
> we care about, which means the overhead is just going to be a single
> "load constant 0 or 1 into register before the call". I think that's
> going to be lost in the noise compared to actually having to make
> the function call at all, the work the function call does, and then
> the second function call later to do the read or write.

I was thinking of knock on effects on spilling other registers in the
TCG code. I guess this depends on how complex the code is around system
register access.

>
> thanks
> -- PMM


--
Alex Bennée

Re: [Qemu-devel] [PATCH 5/7] target-arm: Add isread parameter to CPAccessFns

[Peter Maydell]

On 5 February 2016 at 16:17, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>> I haven't measured, no, but since there are only 3 arguments the
>> third argument is going to be in a register on any host architecture
>> we care about, which means the overhead is just going to be a single
>> "load constant 0 or 1 into register before the call". I think that's
>> going to be lost in the noise compared to actually having to make
>> the function call at all, the work the function call does, and then
>> the second function call later to do the read or write.
>
> I was thinking of knock on effects on spilling other registers in the
> TCG code. I guess this depends on how complex the code is around system
> register access.

The register in question is going to be caller-saves anyway, so no
effect on spilling in the TCG code I think.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 5/7] target-arm: Add isread parameter to CPAccessFns

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 5 February 2016 at 16:17, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>> I haven't measured, no, but since there are only 3 arguments the
>>> third argument is going to be in a register on any host architecture
>>> we care about, which means the overhead is just going to be a single
>>> "load constant 0 or 1 into register before the call". I think that's
>>> going to be lost in the noise compared to actually having to make
>>> the function call at all, the work the function call does, and then
>>> the second function call later to do the read or write.
>>
>> I was thinking of knock on effects on spilling other registers in the
>> TCG code. I guess this depends on how complex the code is around system
>> register access.
>
> The register in question is going to be caller-saves anyway, so no
> effect on spilling in the TCG code I think.

Well my only remaining objection is the additional line-wrapping looks
ugly which I don't think is a valid objection ;-)

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

>
> thanks
> -- PMM


--
Alex Bennée

Re: [Qemu-devel] [PATCH 5/7] target-arm: Add isread parameter to CPAccessFns

[Edgar E. Iglesias]

On Wed, Feb 03, 2016 at 01:38:39PM +0000, Peter Maydell wrote:
> System registers might have access requirements which need to
> be described via a CPAccessFn and which differ for reads and
> writes. For this to be possible we need to pass the access
> function a parameter to tell it whether the access being checked
> is a read or a write.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  target-arm/cpu.h           |  4 ++-
>  target-arm/helper.c        | 81 +++++++++++++++++++++++++++++-----------------
>  target-arm/helper.h        |  2 +-
>  target-arm/op_helper.c     |  5 +--
>  target-arm/translate-a64.c |  6 ++--
>  target-arm/translate.c     |  7 ++--
>  6 files changed, 68 insertions(+), 37 deletions(-)
> 
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 0fb79d0..5137632 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1319,7 +1319,9 @@ typedef uint64_t CPReadFn(CPUARMState *env, const ARMCPRegInfo *opaque);
>  typedef void CPWriteFn(CPUARMState *env, const ARMCPRegInfo *opaque,
>                         uint64_t value);
>  /* Access permission check functions for coprocessor registers. */
> -typedef CPAccessResult CPAccessFn(CPUARMState *env, const ARMCPRegInfo *opaque);
> +typedef CPAccessResult CPAccessFn(CPUARMState *env,
> +                                  const ARMCPRegInfo *opaque,
> +                                  bool isread);
>  /* Hook function for register reset */
>  typedef void CPResetFn(CPUARMState *env, const ARMCPRegInfo *opaque);
>  
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index d85b04f..8bc3ea3 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -344,7 +344,8 @@ void init_cpreg_list(ARMCPU *cpu)
>   * access_el3_aa32ns_aa64any: Used to check both AArch32/64 register views.
>   */
>  static CPAccessResult access_el3_aa32ns(CPUARMState *env,
> -                                        const ARMCPRegInfo *ri)
> +                                        const ARMCPRegInfo *ri,
> +                                        bool isread)
>  {
>      bool secure = arm_is_secure_below_el3(env);
>  
> @@ -356,10 +357,11 @@ static CPAccessResult access_el3_aa32ns(CPUARMState *env,
>  }
>  
>  static CPAccessResult access_el3_aa32ns_aa64any(CPUARMState *env,
> -                                                const ARMCPRegInfo *ri)
> +                                                const ARMCPRegInfo *ri,
> +                                                bool isread)
>  {
>      if (!arm_el_is_aa64(env, 3)) {
> -        return access_el3_aa32ns(env, ri);
> +        return access_el3_aa32ns(env, ri, isread);
>      }
>      return CP_ACCESS_OK;
>  }
> @@ -369,7 +371,8 @@ static CPAccessResult access_el3_aa32ns_aa64any(CPUARMState *env,
>   * We assume that the .access field is set to PL1_RW.
>   */
>  static CPAccessResult access_trap_aa32s_el1(CPUARMState *env,
> -                                            const ARMCPRegInfo *ri)
> +                                            const ARMCPRegInfo *ri,
> +                                            bool isread)
>  {
>      if (arm_current_el(env) == 3) {
>          return CP_ACCESS_OK;
> @@ -651,7 +654,8 @@ static void cpacr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      env->cp15.cpacr_el1 = value;
>  }
>  
> -static CPAccessResult cpacr_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult cpacr_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                   bool isread)
>  {
>      if (arm_feature(env, ARM_FEATURE_V8)) {
>          /* Check if CPACR accesses are to be trapped to EL2 */
> @@ -668,7 +672,8 @@ static CPAccessResult cpacr_access(CPUARMState *env, const ARMCPRegInfo *ri)
>      return CP_ACCESS_OK;
>  }
>  
> -static CPAccessResult cptr_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult cptr_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                  bool isread)
>  {
>      /* Check if CPTR accesses are set to trap to EL3 */
>      if (arm_current_el(env) == 2 && (env->cp15.cptr_el[3] & CPTR_TCPAC)) {
> @@ -710,7 +715,8 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
>      REGINFO_SENTINEL
>  };
>  
> -static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                   bool isread)
>  {
>      /* Performance monitor registers user accessibility is controlled
>       * by PMUSERENR.
> @@ -1154,7 +1160,8 @@ static void teecr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      env->teecr = value;
>  }
>  
> -static CPAccessResult teehbr_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult teehbr_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                    bool isread)
>  {
>      if (arm_current_el(env) == 0 && (env->teecr & 1)) {
>          return CP_ACCESS_TRAP;
> @@ -1207,7 +1214,8 @@ static const ARMCPRegInfo v6k_cp_reginfo[] = {
>  
>  #ifndef CONFIG_USER_ONLY
>  
> -static CPAccessResult gt_cntfrq_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult gt_cntfrq_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                       bool isread)
>  {
>      /* CNTFRQ: not visible from PL0 if both PL0PCTEN and PL0VCTEN are zero */
>      if (arm_current_el(env) == 0 && !extract32(env->cp15.c14_cntkctl, 0, 2)) {
> @@ -1216,7 +1224,8 @@ static CPAccessResult gt_cntfrq_access(CPUARMState *env, const ARMCPRegInfo *ri)
>      return CP_ACCESS_OK;
>  }
>  
> -static CPAccessResult gt_counter_access(CPUARMState *env, int timeridx)
> +static CPAccessResult gt_counter_access(CPUARMState *env, int timeridx,
> +                                        bool isread)
>  {
>      unsigned int cur_el = arm_current_el(env);
>      bool secure = arm_is_secure(env);
> @@ -1235,7 +1244,8 @@ static CPAccessResult gt_counter_access(CPUARMState *env, int timeridx)
>      return CP_ACCESS_OK;
>  }
>  
> -static CPAccessResult gt_timer_access(CPUARMState *env, int timeridx)
> +static CPAccessResult gt_timer_access(CPUARMState *env, int timeridx,
> +                                      bool isread)
>  {
>      unsigned int cur_el = arm_current_el(env);
>      bool secure = arm_is_secure(env);
> @@ -1257,29 +1267,34 @@ static CPAccessResult gt_timer_access(CPUARMState *env, int timeridx)
>  }
>  
>  static CPAccessResult gt_pct_access(CPUARMState *env,
> -                                         const ARMCPRegInfo *ri)
> +                                    const ARMCPRegInfo *ri,
> +                                    bool isread)
>  {
> -    return gt_counter_access(env, GTIMER_PHYS);
> +    return gt_counter_access(env, GTIMER_PHYS, isread);
>  }
>  
>  static CPAccessResult gt_vct_access(CPUARMState *env,
> -                                         const ARMCPRegInfo *ri)
> +                                    const ARMCPRegInfo *ri,
> +                                    bool isread)
>  {
> -    return gt_counter_access(env, GTIMER_VIRT);
> +    return gt_counter_access(env, GTIMER_VIRT, isread);
>  }
>  
> -static CPAccessResult gt_ptimer_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult gt_ptimer_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                       bool isread)
>  {
> -    return gt_timer_access(env, GTIMER_PHYS);
> +    return gt_timer_access(env, GTIMER_PHYS, isread);
>  }
>  
> -static CPAccessResult gt_vtimer_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult gt_vtimer_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                       bool isread)
>  {
> -    return gt_timer_access(env, GTIMER_VIRT);
> +    return gt_timer_access(env, GTIMER_VIRT, isread);
>  }
>  
>  static CPAccessResult gt_stimer_access(CPUARMState *env,
> -                                       const ARMCPRegInfo *ri)
> +                                       const ARMCPRegInfo *ri,
> +                                       bool isread)
>  {
>      /* The AArch64 register view of the secure physical timer is
>       * always accessible from EL3, and configurably accessible from
> @@ -1776,7 +1791,8 @@ static void par_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>  #ifndef CONFIG_USER_ONLY
>  /* get_phys_addr() isn't present for user-mode-only targets */
>  
> -static CPAccessResult ats_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult ats_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                 bool isread)
>  {
>      if (ri->opc2 & 4) {
>          /* The ATS12NSO* operations must trap to EL3 if executed in
> @@ -1921,7 +1937,8 @@ static void ats1h_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      A32_BANKED_CURRENT_REG_SET(env, par, par64);
>  }
>  
> -static CPAccessResult at_s1e2_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult at_s1e2_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                     bool isread)
>  {
>      if (arm_current_el(env) == 3 && !(env->cp15.scr_el3 & SCR_NS)) {
>          return CP_ACCESS_TRAP;
> @@ -2575,7 +2592,8 @@ static void aa64_fpsr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      vfp_set_fpsr(env, value);
>  }
>  
> -static CPAccessResult aa64_daif_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult aa64_daif_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                       bool isread)
>  {
>      if (arm_current_el(env) == 0 && !(env->cp15.sctlr_el[1] & SCTLR_UMA)) {
>          return CP_ACCESS_TRAP;
> @@ -2590,7 +2608,8 @@ static void aa64_daif_write(CPUARMState *env, const ARMCPRegInfo *ri,
>  }
>  
>  static CPAccessResult aa64_cacheop_access(CPUARMState *env,
> -                                          const ARMCPRegInfo *ri)
> +                                          const ARMCPRegInfo *ri,
> +                                          bool isread)
>  {
>      /* Cache invalidate/clean: NOP, but EL0 must UNDEF unless
>       * SCTLR_EL1.UCI is set.
> @@ -2846,7 +2865,8 @@ static void tlbi_aa64_ipas2e1is_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      }
>  }
>  
> -static CPAccessResult aa64_zva_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult aa64_zva_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                      bool isread)
>  {
>      /* We don't implement EL2, so the only control on DC ZVA is the
>       * bit in the SCTLR which can prohibit access for EL0.
> @@ -2863,13 +2883,14 @@ static uint64_t aa64_dczid_read(CPUARMState *env, const ARMCPRegInfo *ri)
>      int dzp_bit = 1 << 4;
>  
>      /* DZP indicates whether DC ZVA access is allowed */
> -    if (aa64_zva_access(env, NULL) == CP_ACCESS_OK) {
> +    if (aa64_zva_access(env, NULL, false) == CP_ACCESS_OK) {
>          dzp_bit = 0;
>      }
>      return cpu->dcz_blocksize | dzp_bit;
>  }
>  
> -static CPAccessResult sp_el0_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult sp_el0_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                    bool isread)
>  {
>      if (!(env->pstate & PSTATE_SP)) {
>          /* Access to SP_EL0 is undefined if it's being used as
> @@ -2908,7 +2929,8 @@ static void sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      tlb_flush(CPU(cpu), 1);
>  }
>  
> -static CPAccessResult fpexc32_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult fpexc32_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                     bool isread)
>  {
>      if ((env->cp15.cptr_el[2] & CPTR_TFP) && arm_current_el(env) == 2) {
>          return CP_ACCESS_TRAP_EL2;
> @@ -3656,7 +3678,8 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>      REGINFO_SENTINEL
>  };
>  
> -static CPAccessResult ctr_el0_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult ctr_el0_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                     bool isread)
>  {
>      /* Only accessible in EL0 if SCTLR.UCT is set (and only in AArch64,
>       * but the AArch32 CTR has its own reginfo struct)
> diff --git a/target-arm/helper.h b/target-arm/helper.h
> index c2a85c7..c98e9ce 100644
> --- a/target-arm/helper.h
> +++ b/target-arm/helper.h
> @@ -62,7 +62,7 @@ DEF_HELPER_1(cpsr_read, i32, env)
>  DEF_HELPER_3(v7m_msr, void, env, i32, i32)
>  DEF_HELPER_2(v7m_mrs, i32, env, i32)
>  
> -DEF_HELPER_3(access_check_cp_reg, void, env, ptr, i32)
> +DEF_HELPER_4(access_check_cp_reg, void, env, ptr, i32, i32)
>  DEF_HELPER_3(set_cp_reg, void, env, ptr, i32)
>  DEF_HELPER_2(get_cp_reg, i32, env, ptr)
>  DEF_HELPER_3(set_cp_reg64, void, env, ptr, i64)
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index a5ee65f..313c0f8 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -457,7 +457,8 @@ void HELPER(set_user_reg)(CPUARMState *env, uint32_t regno, uint32_t val)
>      }
>  }
>  
> -void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip, uint32_t syndrome)
> +void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip, uint32_t syndrome,
> +                                 uint32_t isread)
>  {
>      const ARMCPRegInfo *ri = rip;
>      int target_el;
> @@ -471,7 +472,7 @@ void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip, uint32_t syndrome)
>          return;
>      }
>  
> -    switch (ri->accessfn(env, ri)) {
> +    switch (ri->accessfn(env, ri, isread)) {
>      case CP_ACCESS_OK:
>          return;
>      case CP_ACCESS_TRAP:
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index 80f6c20..8f1eaad 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -1366,16 +1366,18 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
>           * runtime; this may result in an exception.
>           */
>          TCGv_ptr tmpptr;
> -        TCGv_i32 tcg_syn;
> +        TCGv_i32 tcg_syn, tcg_isread;
>          uint32_t syndrome;
>  
>          gen_a64_set_pc_im(s->pc - 4);
>          tmpptr = tcg_const_ptr(ri);
>          syndrome = syn_aa64_sysregtrap(op0, op1, op2, crn, crm, rt, isread);
>          tcg_syn = tcg_const_i32(syndrome);
> -        gen_helper_access_check_cp_reg(cpu_env, tmpptr, tcg_syn);
> +        tcg_isread = tcg_const_i32(isread);
> +        gen_helper_access_check_cp_reg(cpu_env, tmpptr, tcg_syn, tcg_isread);
>          tcg_temp_free_ptr(tmpptr);
>          tcg_temp_free_i32(tcg_syn);
> +        tcg_temp_free_i32(tcg_isread);
>      }
>  
>      /* Handle special cases first */
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index cff511b..375acf5 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -7168,7 +7168,7 @@ static int disas_coproc_insn(DisasContext *s, uint32_t insn)
>               * call in order to handle c15_cpar.
>               */
>              TCGv_ptr tmpptr;
> -            TCGv_i32 tcg_syn;
> +            TCGv_i32 tcg_syn, tcg_isread;
>              uint32_t syndrome;
>  
>              /* Note that since we are an implementation which takes an
> @@ -7213,9 +7213,12 @@ static int disas_coproc_insn(DisasContext *s, uint32_t insn)
>              gen_set_pc_im(s, s->pc - 4);
>              tmpptr = tcg_const_ptr(ri);
>              tcg_syn = tcg_const_i32(syndrome);
> -            gen_helper_access_check_cp_reg(cpu_env, tmpptr, tcg_syn);
> +            tcg_isread = tcg_const_i32(isread);
> +            gen_helper_access_check_cp_reg(cpu_env, tmpptr, tcg_syn,
> +                                           tcg_isread);
>              tcg_temp_free_ptr(tmpptr);
>              tcg_temp_free_i32(tcg_syn);
> +            tcg_temp_free_i32(tcg_isread);
>          }
>  
>          /* Handle special cases first */
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 5/7] target-arm: Add isread parameter to CPAccessFns

[Sergey Fedorov]

On 03.02.2016 16:38, Peter Maydell wrote:
> System registers might have access requirements which need to
> be described via a CPAccessFn and which differ for reads and
> writes. For this to be possible we need to pass the access
> function a parameter to tell it whether the access being checked
> is a read or a write.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>

> ---
>  target-arm/cpu.h           |  4 ++-
>  target-arm/helper.c        | 81 +++++++++++++++++++++++++++++-----------------
>  target-arm/helper.h        |  2 +-
>  target-arm/op_helper.c     |  5 +--
>  target-arm/translate-a64.c |  6 ++--
>  target-arm/translate.c     |  7 ++--
>  6 files changed, 68 insertions(+), 37 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 0fb79d0..5137632 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1319,7 +1319,9 @@ typedef uint64_t CPReadFn(CPUARMState *env, const ARMCPRegInfo *opaque);
>  typedef void CPWriteFn(CPUARMState *env, const ARMCPRegInfo *opaque,
>                         uint64_t value);
>  /* Access permission check functions for coprocessor registers. */
> -typedef CPAccessResult CPAccessFn(CPUARMState *env, const ARMCPRegInfo *opaque);
> +typedef CPAccessResult CPAccessFn(CPUARMState *env,
> +                                  const ARMCPRegInfo *opaque,
> +                                  bool isread);
>  /* Hook function for register reset */
>  typedef void CPResetFn(CPUARMState *env, const ARMCPRegInfo *opaque);
>  
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index d85b04f..8bc3ea3 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -344,7 +344,8 @@ void init_cpreg_list(ARMCPU *cpu)
>   * access_el3_aa32ns_aa64any: Used to check both AArch32/64 register views.
>   */
>  static CPAccessResult access_el3_aa32ns(CPUARMState *env,
> -                                        const ARMCPRegInfo *ri)
> +                                        const ARMCPRegInfo *ri,
> +                                        bool isread)
>  {
>      bool secure = arm_is_secure_below_el3(env);
>  
> @@ -356,10 +357,11 @@ static CPAccessResult access_el3_aa32ns(CPUARMState *env,
>  }
>  
>  static CPAccessResult access_el3_aa32ns_aa64any(CPUARMState *env,
> -                                                const ARMCPRegInfo *ri)
> +                                                const ARMCPRegInfo *ri,
> +                                                bool isread)
>  {
>      if (!arm_el_is_aa64(env, 3)) {
> -        return access_el3_aa32ns(env, ri);
> +        return access_el3_aa32ns(env, ri, isread);
>      }
>      return CP_ACCESS_OK;
>  }
> @@ -369,7 +371,8 @@ static CPAccessResult access_el3_aa32ns_aa64any(CPUARMState *env,
>   * We assume that the .access field is set to PL1_RW.
>   */
>  static CPAccessResult access_trap_aa32s_el1(CPUARMState *env,
> -                                            const ARMCPRegInfo *ri)
> +                                            const ARMCPRegInfo *ri,
> +                                            bool isread)
>  {
>      if (arm_current_el(env) == 3) {
>          return CP_ACCESS_OK;
> @@ -651,7 +654,8 @@ static void cpacr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      env->cp15.cpacr_el1 = value;
>  }
>  
> -static CPAccessResult cpacr_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult cpacr_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                   bool isread)
>  {
>      if (arm_feature(env, ARM_FEATURE_V8)) {
>          /* Check if CPACR accesses are to be trapped to EL2 */
> @@ -668,7 +672,8 @@ static CPAccessResult cpacr_access(CPUARMState *env, const ARMCPRegInfo *ri)
>      return CP_ACCESS_OK;
>  }
>  
> -static CPAccessResult cptr_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult cptr_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                  bool isread)
>  {
>      /* Check if CPTR accesses are set to trap to EL3 */
>      if (arm_current_el(env) == 2 && (env->cp15.cptr_el[3] & CPTR_TCPAC)) {
> @@ -710,7 +715,8 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
>      REGINFO_SENTINEL
>  };
>  
> -static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                   bool isread)
>  {
>      /* Performance monitor registers user accessibility is controlled
>       * by PMUSERENR.
> @@ -1154,7 +1160,8 @@ static void teecr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      env->teecr = value;
>  }
>  
> -static CPAccessResult teehbr_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult teehbr_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                    bool isread)
>  {
>      if (arm_current_el(env) == 0 && (env->teecr & 1)) {
>          return CP_ACCESS_TRAP;
> @@ -1207,7 +1214,8 @@ static const ARMCPRegInfo v6k_cp_reginfo[] = {
>  
>  #ifndef CONFIG_USER_ONLY
>  
> -static CPAccessResult gt_cntfrq_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult gt_cntfrq_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                       bool isread)
>  {
>      /* CNTFRQ: not visible from PL0 if both PL0PCTEN and PL0VCTEN are zero */
>      if (arm_current_el(env) == 0 && !extract32(env->cp15.c14_cntkctl, 0, 2)) {
> @@ -1216,7 +1224,8 @@ static CPAccessResult gt_cntfrq_access(CPUARMState *env, const ARMCPRegInfo *ri)
>      return CP_ACCESS_OK;
>  }
>  
> -static CPAccessResult gt_counter_access(CPUARMState *env, int timeridx)
> +static CPAccessResult gt_counter_access(CPUARMState *env, int timeridx,
> +                                        bool isread)
>  {
>      unsigned int cur_el = arm_current_el(env);
>      bool secure = arm_is_secure(env);
> @@ -1235,7 +1244,8 @@ static CPAccessResult gt_counter_access(CPUARMState *env, int timeridx)
>      return CP_ACCESS_OK;
>  }
>  
> -static CPAccessResult gt_timer_access(CPUARMState *env, int timeridx)
> +static CPAccessResult gt_timer_access(CPUARMState *env, int timeridx,
> +                                      bool isread)
>  {
>      unsigned int cur_el = arm_current_el(env);
>      bool secure = arm_is_secure(env);
> @@ -1257,29 +1267,34 @@ static CPAccessResult gt_timer_access(CPUARMState *env, int timeridx)
>  }
>  
>  static CPAccessResult gt_pct_access(CPUARMState *env,
> -                                         const ARMCPRegInfo *ri)
> +                                    const ARMCPRegInfo *ri,
> +                                    bool isread)
>  {
> -    return gt_counter_access(env, GTIMER_PHYS);
> +    return gt_counter_access(env, GTIMER_PHYS, isread);
>  }
>  
>  static CPAccessResult gt_vct_access(CPUARMState *env,
> -                                         const ARMCPRegInfo *ri)
> +                                    const ARMCPRegInfo *ri,
> +                                    bool isread)
>  {
> -    return gt_counter_access(env, GTIMER_VIRT);
> +    return gt_counter_access(env, GTIMER_VIRT, isread);
>  }
>  
> -static CPAccessResult gt_ptimer_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult gt_ptimer_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                       bool isread)
>  {
> -    return gt_timer_access(env, GTIMER_PHYS);
> +    return gt_timer_access(env, GTIMER_PHYS, isread);
>  }
>  
> -static CPAccessResult gt_vtimer_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult gt_vtimer_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                       bool isread)
>  {
> -    return gt_timer_access(env, GTIMER_VIRT);
> +    return gt_timer_access(env, GTIMER_VIRT, isread);
>  }
>  
>  static CPAccessResult gt_stimer_access(CPUARMState *env,
> -                                       const ARMCPRegInfo *ri)
> +                                       const ARMCPRegInfo *ri,
> +                                       bool isread)
>  {
>      /* The AArch64 register view of the secure physical timer is
>       * always accessible from EL3, and configurably accessible from
> @@ -1776,7 +1791,8 @@ static void par_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>  #ifndef CONFIG_USER_ONLY
>  /* get_phys_addr() isn't present for user-mode-only targets */
>  
> -static CPAccessResult ats_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult ats_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                 bool isread)
>  {
>      if (ri->opc2 & 4) {
>          /* The ATS12NSO* operations must trap to EL3 if executed in
> @@ -1921,7 +1937,8 @@ static void ats1h_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      A32_BANKED_CURRENT_REG_SET(env, par, par64);
>  }
>  
> -static CPAccessResult at_s1e2_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult at_s1e2_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                     bool isread)
>  {
>      if (arm_current_el(env) == 3 && !(env->cp15.scr_el3 & SCR_NS)) {
>          return CP_ACCESS_TRAP;
> @@ -2575,7 +2592,8 @@ static void aa64_fpsr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      vfp_set_fpsr(env, value);
>  }
>  
> -static CPAccessResult aa64_daif_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult aa64_daif_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                       bool isread)
>  {
>      if (arm_current_el(env) == 0 && !(env->cp15.sctlr_el[1] & SCTLR_UMA)) {
>          return CP_ACCESS_TRAP;
> @@ -2590,7 +2608,8 @@ static void aa64_daif_write(CPUARMState *env, const ARMCPRegInfo *ri,
>  }
>  
>  static CPAccessResult aa64_cacheop_access(CPUARMState *env,
> -                                          const ARMCPRegInfo *ri)
> +                                          const ARMCPRegInfo *ri,
> +                                          bool isread)
>  {
>      /* Cache invalidate/clean: NOP, but EL0 must UNDEF unless
>       * SCTLR_EL1.UCI is set.
> @@ -2846,7 +2865,8 @@ static void tlbi_aa64_ipas2e1is_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      }
>  }
>  
> -static CPAccessResult aa64_zva_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult aa64_zva_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                      bool isread)
>  {
>      /* We don't implement EL2, so the only control on DC ZVA is the
>       * bit in the SCTLR which can prohibit access for EL0.
> @@ -2863,13 +2883,14 @@ static uint64_t aa64_dczid_read(CPUARMState *env, const ARMCPRegInfo *ri)
>      int dzp_bit = 1 << 4;
>  
>      /* DZP indicates whether DC ZVA access is allowed */
> -    if (aa64_zva_access(env, NULL) == CP_ACCESS_OK) {
> +    if (aa64_zva_access(env, NULL, false) == CP_ACCESS_OK) {
>          dzp_bit = 0;
>      }
>      return cpu->dcz_blocksize | dzp_bit;
>  }
>  
> -static CPAccessResult sp_el0_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult sp_el0_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                    bool isread)
>  {
>      if (!(env->pstate & PSTATE_SP)) {
>          /* Access to SP_EL0 is undefined if it's being used as
> @@ -2908,7 +2929,8 @@ static void sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      tlb_flush(CPU(cpu), 1);
>  }
>  
> -static CPAccessResult fpexc32_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult fpexc32_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                     bool isread)
>  {
>      if ((env->cp15.cptr_el[2] & CPTR_TFP) && arm_current_el(env) == 2) {
>          return CP_ACCESS_TRAP_EL2;
> @@ -3656,7 +3678,8 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>      REGINFO_SENTINEL
>  };
>  
> -static CPAccessResult ctr_el0_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +static CPAccessResult ctr_el0_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                     bool isread)
>  {
>      /* Only accessible in EL0 if SCTLR.UCT is set (and only in AArch64,
>       * but the AArch32 CTR has its own reginfo struct)
> diff --git a/target-arm/helper.h b/target-arm/helper.h
> index c2a85c7..c98e9ce 100644
> --- a/target-arm/helper.h
> +++ b/target-arm/helper.h
> @@ -62,7 +62,7 @@ DEF_HELPER_1(cpsr_read, i32, env)
>  DEF_HELPER_3(v7m_msr, void, env, i32, i32)
>  DEF_HELPER_2(v7m_mrs, i32, env, i32)
>  
> -DEF_HELPER_3(access_check_cp_reg, void, env, ptr, i32)
> +DEF_HELPER_4(access_check_cp_reg, void, env, ptr, i32, i32)
>  DEF_HELPER_3(set_cp_reg, void, env, ptr, i32)
>  DEF_HELPER_2(get_cp_reg, i32, env, ptr)
>  DEF_HELPER_3(set_cp_reg64, void, env, ptr, i64)
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index a5ee65f..313c0f8 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -457,7 +457,8 @@ void HELPER(set_user_reg)(CPUARMState *env, uint32_t regno, uint32_t val)
>      }
>  }
>  
> -void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip, uint32_t syndrome)
> +void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip, uint32_t syndrome,
> +                                 uint32_t isread)
>  {
>      const ARMCPRegInfo *ri = rip;
>      int target_el;
> @@ -471,7 +472,7 @@ void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip, uint32_t syndrome)
>          return;
>      }
>  
> -    switch (ri->accessfn(env, ri)) {
> +    switch (ri->accessfn(env, ri, isread)) {
>      case CP_ACCESS_OK:
>          return;
>      case CP_ACCESS_TRAP:
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index 80f6c20..8f1eaad 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -1366,16 +1366,18 @@ static void handle_sys(DisasContext *s, uint32_t insn, bool isread,
>           * runtime; this may result in an exception.
>           */
>          TCGv_ptr tmpptr;
> -        TCGv_i32 tcg_syn;
> +        TCGv_i32 tcg_syn, tcg_isread;
>          uint32_t syndrome;
>  
>          gen_a64_set_pc_im(s->pc - 4);
>          tmpptr = tcg_const_ptr(ri);
>          syndrome = syn_aa64_sysregtrap(op0, op1, op2, crn, crm, rt, isread);
>          tcg_syn = tcg_const_i32(syndrome);
> -        gen_helper_access_check_cp_reg(cpu_env, tmpptr, tcg_syn);
> +        tcg_isread = tcg_const_i32(isread);
> +        gen_helper_access_check_cp_reg(cpu_env, tmpptr, tcg_syn, tcg_isread);
>          tcg_temp_free_ptr(tmpptr);
>          tcg_temp_free_i32(tcg_syn);
> +        tcg_temp_free_i32(tcg_isread);
>      }
>  
>      /* Handle special cases first */
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index cff511b..375acf5 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -7168,7 +7168,7 @@ static int disas_coproc_insn(DisasContext *s, uint32_t insn)
>               * call in order to handle c15_cpar.
>               */
>              TCGv_ptr tmpptr;
> -            TCGv_i32 tcg_syn;
> +            TCGv_i32 tcg_syn, tcg_isread;
>              uint32_t syndrome;
>  
>              /* Note that since we are an implementation which takes an
> @@ -7213,9 +7213,12 @@ static int disas_coproc_insn(DisasContext *s, uint32_t insn)
>              gen_set_pc_im(s, s->pc - 4);
>              tmpptr = tcg_const_ptr(ri);
>              tcg_syn = tcg_const_i32(syndrome);
> -            gen_helper_access_check_cp_reg(cpu_env, tmpptr, tcg_syn);
> +            tcg_isread = tcg_const_i32(isread);
> +            gen_helper_access_check_cp_reg(cpu_env, tmpptr, tcg_syn,
> +                                           tcg_isread);
>              tcg_temp_free_ptr(tmpptr);
>              tcg_temp_free_i32(tcg_syn);
> +            tcg_temp_free_i32(tcg_isread);
>          }
>  
>          /* Handle special cases first */

[Qemu-devel] [PATCH 6/7] target-arm: Implement NSACR trapping behaviour

[Peter Maydell]

Implement some corner cases of the behaviour of the NSACR
register on ARMv8:
 * if EL3 is AArch64 then accessing the NSACR from Secure EL1
   with AArch32 should trap to EL3
 * if EL3 is not present or is AArch64 then reads from NS EL1 and
   NS EL2 return constant 0xc00

It would in theory be possible to implement all these with
a single reginfo definition, but for clarity we use three
separate definitions for the three cases and install the
right one based on the CPU feature flags.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 58 insertions(+), 4 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 8bc3ea3..34dc144 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3562,6 +3562,25 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
     REGINFO_SENTINEL
 };
 
+static CPAccessResult nsacr_access(CPUARMState *env, const ARMCPRegInfo *ri,
+                                   bool isread)
+{
+    /* The NSACR is RW at EL3, and RO for NS EL1 and NS EL2.
+     * At Secure EL1 it traps to EL3.
+     */
+    if (arm_current_el(env) == 3) {
+        return CP_ACCESS_OK;
+    }
+    if (arm_is_secure_below_el3(env)) {
+        return CP_ACCESS_TRAP_EL3;
+    }
+    /* Accesses from EL1 NS and EL2 NS are UNDEF for write but allow reads. */
+    if (isread) {
+        return CP_ACCESS_OK;
+    }
+    return CP_ACCESS_TRAP_UNCATEGORIZED;
+}
+
 static const ARMCPRegInfo el3_cp_reginfo[] = {
     { .name = "SCR_EL3", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 0,
@@ -3587,10 +3606,6 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
       .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 1,
       .access = PL3_RW, .resetvalue = 0,
       .fieldoffset = offsetoflow32(CPUARMState, cp15.sder) },
-      /* TODO: Implement NSACR trapping of secure EL1 accesses to EL3 */
-    { .name = "NSACR", .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 2,
-      .access = PL3_W | PL1_R, .resetvalue = 0,
-      .fieldoffset = offsetof(CPUARMState, cp15.nsacr) },
     { .name = "MVBAR", .cp = 15, .opc1 = 0, .crn = 12, .crm = 0, .opc2 = 1,
       .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
       .writefn = vbar_write, .resetvalue = 0,
@@ -4361,6 +4376,45 @@ void register_cp_regs_for_features(ARMCPU *cpu)
         };
         define_one_arm_cp_reg(cpu, &rvbar);
     }
+    /* The behaviour of NSACR is sufficiently various that we don't
+     * try to describe it in a single reginfo:
+     *  if EL3 is 64 bit, then trap to EL3 from S EL1,
+     *     reads as constant 0xc00 from NS EL1 and NS EL2
+     *  if EL3 is 32 bit, then RW at EL3, RO at NS EL1 and NS EL2
+     *  if v7 without EL3, register doesn't exist
+     *  if v8 without EL3, reads as constant 0xc00 from NS EL1 and NS EL2
+     */
+    if (arm_feature(env, ARM_FEATURE_EL3)) {
+        if (arm_feature(env, ARM_FEATURE_AARCH64)) {
+            ARMCPRegInfo nsacr = {
+                .name = "NSACR", .type = ARM_CP_CONST,
+                .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 2,
+                .access = PL1_RW, .accessfn = nsacr_access,
+                .resetvalue = 0xc00
+            };
+            define_one_arm_cp_reg(cpu, &nsacr);
+        } else {
+            ARMCPRegInfo nsacr = {
+                .name = "NSACR",
+                .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 2,
+                .access = PL3_RW | PL1_R,
+                .resetvalue = 0,
+                .fieldoffset = offsetof(CPUARMState, cp15.nsacr)
+            };
+            define_one_arm_cp_reg(cpu, &nsacr);
+        }
+    } else {
+        if (arm_feature(env, ARM_FEATURE_V8)) {
+            ARMCPRegInfo nsacr = {
+                .name = "NSACR", .type = ARM_CP_CONST,
+                .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 2,
+                .access = PL1_R,
+                .resetvalue = 0xc00
+            };
+            define_one_arm_cp_reg(cpu, &nsacr);
+        }
+    }
+
     if (arm_feature(env, ARM_FEATURE_MPU)) {
         if (arm_feature(env, ARM_FEATURE_V6)) {
             /* PMSAv6 not implemented */
-- 
1.9.1

Re: [Qemu-devel] [Qemu-arm] [PATCH 6/7] target-arm: Implement NSACR trapping behaviour

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> Implement some corner cases of the behaviour of the NSACR
> register on ARMv8:
>  * if EL3 is AArch64 then accessing the NSACR from Secure EL1
>    with AArch32 should trap to EL3
>  * if EL3 is not present or is AArch64 then reads from NS EL1 and
>    NS EL2 return constant 0xc00
>
> It would in theory be possible to implement all these with
> a single reginfo definition, but for clarity we use three
> separate definitions for the three cases and install the
> right one based on the CPU feature flags.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/helper.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 58 insertions(+), 4 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 8bc3ea3..34dc144 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3562,6 +3562,25 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>      REGINFO_SENTINEL
>  };
>
> +static CPAccessResult nsacr_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                   bool isread)
> +{
> +    /* The NSACR is RW at EL3, and RO for NS EL1 and NS EL2.
> +     * At Secure EL1 it traps to EL3.
> +     */
> +    if (arm_current_el(env) == 3) {
> +        return CP_ACCESS_OK;
> +    }
> +    if (arm_is_secure_below_el3(env)) {
> +        return CP_ACCESS_TRAP_EL3;
> +    }
> +    /* Accesses from EL1 NS and EL2 NS are UNDEF for write but allow reads. */
> +    if (isread) {
> +        return CP_ACCESS_OK;
> +    }
> +    return CP_ACCESS_TRAP_UNCATEGORIZED;
> +}
> +
>  static const ARMCPRegInfo el3_cp_reginfo[] = {
>      { .name = "SCR_EL3", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 0,
> @@ -3587,10 +3606,6 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>        .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 1,
>        .access = PL3_RW, .resetvalue = 0,
>        .fieldoffset = offsetoflow32(CPUARMState, cp15.sder) },
> -      /* TODO: Implement NSACR trapping of secure EL1 accesses to EL3 */
> -    { .name = "NSACR", .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 2,
> -      .access = PL3_W | PL1_R, .resetvalue = 0,
> -      .fieldoffset = offsetof(CPUARMState, cp15.nsacr) },
>      { .name = "MVBAR", .cp = 15, .opc1 = 0, .crn = 12, .crm = 0, .opc2 = 1,
>        .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
>        .writefn = vbar_write, .resetvalue = 0,
> @@ -4361,6 +4376,45 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>          };
>          define_one_arm_cp_reg(cpu, &rvbar);
>      }
> +    /* The behaviour of NSACR is sufficiently various that we don't
> +     * try to describe it in a single reginfo:
> +     *  if EL3 is 64 bit, then trap to EL3 from S EL1,
> +     *     reads as constant 0xc00 from NS EL1 and NS EL2
> +     *  if EL3 is 32 bit, then RW at EL3, RO at NS EL1 and NS EL2
> +     *  if v7 without EL3, register doesn't exist
> +     *  if v8 without EL3, reads as constant 0xc00 from NS EL1 and NS EL2
> +     */
> +    if (arm_feature(env, ARM_FEATURE_EL3)) {
> +        if (arm_feature(env, ARM_FEATURE_AARCH64)) {
> +            ARMCPRegInfo nsacr = {
> +                .name = "NSACR", .type = ARM_CP_CONST,

I was thrown by the ARM_CP_CONST here as there is EL dependency. If
nsacr_access says CP_ACCESS_OK where does the write go? The ARM ARM says
RO and the code in translate-a64 says:

        if (ri->type & ARM_CP_CONST) {
            /* If not forbidden by access permissions, treat as WI */
            return;
        }

So we might want to make that clear in the comment.

> +                .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 2,
> +                .access = PL1_RW, .accessfn = nsacr_access,

PL1_R?

> +                .resetvalue = 0xc00
> +            };
> +            define_one_arm_cp_reg(cpu, &nsacr);
> +        } else {
> +            ARMCPRegInfo nsacr = {
> +                .name = "NSACR",
> +                .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 2,
> +                .access = PL3_RW | PL1_R,
> +                .resetvalue = 0,
> +                .fieldoffset = offsetof(CPUARMState, cp15.nsacr)
> +            };
> +            define_one_arm_cp_reg(cpu, &nsacr);
> +        }
> +    } else {
> +        if (arm_feature(env, ARM_FEATURE_V8)) {
> +            ARMCPRegInfo nsacr = {
> +                .name = "NSACR", .type = ARM_CP_CONST,
> +                .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 2,
> +                .access = PL1_R,
> +                .resetvalue = 0xc00
> +            };
> +            define_one_arm_cp_reg(cpu, &nsacr);
> +        }
> +    }
> +
>      if (arm_feature(env, ARM_FEATURE_MPU)) {
>          if (arm_feature(env, ARM_FEATURE_V6)) {
>              /* PMSAv6 not implemented */

I don't know if a more compact definition would make this easier to
follow?

    /* The behaviour of NSACR is sufficiently various we tweak the reginfo:
     *  if EL3 is 64 bit, then trap to EL3 from S EL1,
     *     reads as constant 0xc00 from NS EL1 and NS EL2
     *  if EL3 is 32 bit, then RW at EL3, RO at NS EL1 and NS EL2
     *  if v7 without EL3, register doesn't exist
     *  if v8 without EL3, reads as constant 0xc00 from NS EL1 and NS EL2
     */
    if (arm_feature(env, ARM_FEATURE_V8) || arm_feature(env, ARM_FEATURE_EL3)) {
        ARMCPRegInfo nsacr = {
            .name = "NSACR",
            .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 2,
            .access = PL1_R,
            .resetvalue = 0xc00
        };

        if (arm_feature(env, ARM_FEATURE_EL3)) {
            if (arm_feature(env, ARM_FEATURE_AARCH64)) {
                nsacr.type = ARM_CP_CONST;      /* if no trap RO */
                nsacr.accessfn = nsacr_access;
            } else {
                nsacr.access = PL3_RW | PL1_R;
                nsacr.resetvalue = 0;
                nsacr.fieldoffset = offsetof(CPUARMState, cp15.nsacr);
            };
        } else {
            nsacr.type = ARM_CP_CONST;
        }

        define_one_arm_cp_reg(cpu, &nsacr);
    }



--
Alex Bennée

Re: [Qemu-devel] [Qemu-arm] [PATCH 6/7] target-arm: Implement NSACR trapping behaviour

[Peter Maydell]

On 5 February 2016 at 16:07, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> Implement some corner cases of the behaviour of the NSACR
>> register on ARMv8:
>>  * if EL3 is AArch64 then accessing the NSACR from Secure EL1
>>    with AArch32 should trap to EL3
>>  * if EL3 is not present or is AArch64 then reads from NS EL1 and
>>    NS EL2 return constant 0xc00
>>
>> It would in theory be possible to implement all these with
>> a single reginfo definition, but for clarity we use three
>> separate definitions for the three cases and install the
>> right one based on the CPU feature flags.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  target-arm/helper.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 58 insertions(+), 4 deletions(-)
>>
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 8bc3ea3..34dc144 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -3562,6 +3562,25 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>>      REGINFO_SENTINEL
>>  };
>>
>> +static CPAccessResult nsacr_access(CPUARMState *env, const ARMCPRegInfo *ri,
>> +                                   bool isread)
>> +{
>> +    /* The NSACR is RW at EL3, and RO for NS EL1 and NS EL2.
>> +     * At Secure EL1 it traps to EL3.
>> +     */
>> +    if (arm_current_el(env) == 3) {
>> +        return CP_ACCESS_OK;
>> +    }
>> +    if (arm_is_secure_below_el3(env)) {
>> +        return CP_ACCESS_TRAP_EL3;
>> +    }
>> +    /* Accesses from EL1 NS and EL2 NS are UNDEF for write but allow reads. */
>> +    if (isread) {
>> +        return CP_ACCESS_OK;
>> +    }
>> +    return CP_ACCESS_TRAP_UNCATEGORIZED;
>> +}
>> +
>>  static const ARMCPRegInfo el3_cp_reginfo[] = {
>>      { .name = "SCR_EL3", .state = ARM_CP_STATE_AA64,
>>        .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 0,
>> @@ -3587,10 +3606,6 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>>        .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 1,
>>        .access = PL3_RW, .resetvalue = 0,
>>        .fieldoffset = offsetoflow32(CPUARMState, cp15.sder) },
>> -      /* TODO: Implement NSACR trapping of secure EL1 accesses to EL3 */
>> -    { .name = "NSACR", .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 2,
>> -      .access = PL3_W | PL1_R, .resetvalue = 0,
>> -      .fieldoffset = offsetof(CPUARMState, cp15.nsacr) },
>>      { .name = "MVBAR", .cp = 15, .opc1 = 0, .crn = 12, .crm = 0, .opc2 = 1,
>>        .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
>>        .writefn = vbar_write, .resetvalue = 0,
>> @@ -4361,6 +4376,45 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>>          };
>>          define_one_arm_cp_reg(cpu, &rvbar);
>>      }
>> +    /* The behaviour of NSACR is sufficiently various that we don't
>> +     * try to describe it in a single reginfo:
>> +     *  if EL3 is 64 bit, then trap to EL3 from S EL1,
>> +     *     reads as constant 0xc00 from NS EL1 and NS EL2
>> +     *  if EL3 is 32 bit, then RW at EL3, RO at NS EL1 and NS EL2
>> +     *  if v7 without EL3, register doesn't exist
>> +     *  if v8 without EL3, reads as constant 0xc00 from NS EL1 and NS EL2
>> +     */
>> +    if (arm_feature(env, ARM_FEATURE_EL3)) {
>> +        if (arm_feature(env, ARM_FEATURE_AARCH64)) {
>> +            ARMCPRegInfo nsacr = {
>> +                .name = "NSACR", .type = ARM_CP_CONST,
>
> I was thrown by the ARM_CP_CONST here as there is EL dependency. If
> nsacr_access says CP_ACCESS_OK where does the write go?

nsacr_access can never say OK for a write here (because we can't
be at EL3 since it's a 32-bit register and this is in the
"EL3 is 64-bit" arm of the if). The register is genuinely
constant.

>> +                .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 2,
>> +                .access = PL1_RW, .accessfn = nsacr_access,
>
> PL1_R?

No, because the .access checks happen before the access fn. If
we forbid writes in .access then a write from Secure EL1 will
fail UNDEF rather than trap to EL3. So instead we set a wider
permission set in .access and use .accessfn to get the exact
checks we need.

>> +                .resetvalue = 0xc00
>> +            };
>> +            define_one_arm_cp_reg(cpu, &nsacr);
>> +        } else {
>> +            ARMCPRegInfo nsacr = {
>> +                .name = "NSACR",
>> +                .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 2,
>> +                .access = PL3_RW | PL1_R,
>> +                .resetvalue = 0,
>> +                .fieldoffset = offsetof(CPUARMState, cp15.nsacr)
>> +            };
>> +            define_one_arm_cp_reg(cpu, &nsacr);
>> +        }
>> +    } else {
>> +        if (arm_feature(env, ARM_FEATURE_V8)) {
>> +            ARMCPRegInfo nsacr = {
>> +                .name = "NSACR", .type = ARM_CP_CONST,
>> +                .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 2,
>> +                .access = PL1_R,
>> +                .resetvalue = 0xc00
>> +            };
>> +            define_one_arm_cp_reg(cpu, &nsacr);
>> +        }
>> +    }
>> +
>>      if (arm_feature(env, ARM_FEATURE_MPU)) {
>>          if (arm_feature(env, ARM_FEATURE_V6)) {
>>              /* PMSAv6 not implemented */
>
> I don't know if a more compact definition would make this easier to
> follow?

>     /* The behaviour of NSACR is sufficiently various we tweak the reginfo:
>      *  if EL3 is 64 bit, then trap to EL3 from S EL1,
>      *     reads as constant 0xc00 from NS EL1 and NS EL2
>      *  if EL3 is 32 bit, then RW at EL3, RO at NS EL1 and NS EL2
>      *  if v7 without EL3, register doesn't exist
>      *  if v8 without EL3, reads as constant 0xc00 from NS EL1 and NS EL2
>      */
>     if (arm_feature(env, ARM_FEATURE_V8) || arm_feature(env, ARM_FEATURE_EL3)) {
>         ARMCPRegInfo nsacr = {
>             .name = "NSACR",
>             .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 2,
>             .access = PL1_R,
>             .resetvalue = 0xc00
>         };
>
>         if (arm_feature(env, ARM_FEATURE_EL3)) {
>             if (arm_feature(env, ARM_FEATURE_AARCH64)) {
>                 nsacr.type = ARM_CP_CONST;      /* if no trap RO */
>                 nsacr.accessfn = nsacr_access;
>             } else {
>                 nsacr.access = PL3_RW | PL1_R;
>                 nsacr.resetvalue = 0;
>                 nsacr.fieldoffset = offsetof(CPUARMState, cp15.nsacr);
>             };
>         } else {
>             nsacr.type = ARM_CP_CONST;
>         }
>
>         define_one_arm_cp_reg(cpu, &nsacr);
>     }

I thought about this, but decided it was worse, because now you have
to read the whole dozen lines or so to figure out what the register
is defined as for each of the three interesting cases, and mentally
reassemble the regdef.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 6/7] target-arm: Implement NSACR trapping behaviour

[Edgar E. Iglesias]

On Wed, Feb 03, 2016 at 01:38:40PM +0000, Peter Maydell wrote:
> Implement some corner cases of the behaviour of the NSACR
> register on ARMv8:
>  * if EL3 is AArch64 then accessing the NSACR from Secure EL1
>    with AArch32 should trap to EL3
>  * if EL3 is not present or is AArch64 then reads from NS EL1 and
>    NS EL2 return constant 0xc00
> 
> It would in theory be possible to implement all these with
> a single reginfo definition, but for clarity we use three
> separate definitions for the three cases and install the
> right one based on the CPU feature flags.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

AFAICT it looks OK

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  target-arm/helper.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 58 insertions(+), 4 deletions(-)
> 
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 8bc3ea3..34dc144 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3562,6 +3562,25 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>      REGINFO_SENTINEL
>  };
>  
> +static CPAccessResult nsacr_access(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                   bool isread)
> +{
> +    /* The NSACR is RW at EL3, and RO for NS EL1 and NS EL2.
> +     * At Secure EL1 it traps to EL3.
> +     */
> +    if (arm_current_el(env) == 3) {
> +        return CP_ACCESS_OK;
> +    }
> +    if (arm_is_secure_below_el3(env)) {
> +        return CP_ACCESS_TRAP_EL3;
> +    }
> +    /* Accesses from EL1 NS and EL2 NS are UNDEF for write but allow reads. */
> +    if (isread) {
> +        return CP_ACCESS_OK;
> +    }
> +    return CP_ACCESS_TRAP_UNCATEGORIZED;
> +}
> +
>  static const ARMCPRegInfo el3_cp_reginfo[] = {
>      { .name = "SCR_EL3", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 0,
> @@ -3587,10 +3606,6 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>        .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 1,
>        .access = PL3_RW, .resetvalue = 0,
>        .fieldoffset = offsetoflow32(CPUARMState, cp15.sder) },
> -      /* TODO: Implement NSACR trapping of secure EL1 accesses to EL3 */
> -    { .name = "NSACR", .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 2,
> -      .access = PL3_W | PL1_R, .resetvalue = 0,
> -      .fieldoffset = offsetof(CPUARMState, cp15.nsacr) },
>      { .name = "MVBAR", .cp = 15, .opc1 = 0, .crn = 12, .crm = 0, .opc2 = 1,
>        .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
>        .writefn = vbar_write, .resetvalue = 0,
> @@ -4361,6 +4376,45 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>          };
>          define_one_arm_cp_reg(cpu, &rvbar);
>      }
> +    /* The behaviour of NSACR is sufficiently various that we don't
> +     * try to describe it in a single reginfo:
> +     *  if EL3 is 64 bit, then trap to EL3 from S EL1,
> +     *     reads as constant 0xc00 from NS EL1 and NS EL2
> +     *  if EL3 is 32 bit, then RW at EL3, RO at NS EL1 and NS EL2
> +     *  if v7 without EL3, register doesn't exist
> +     *  if v8 without EL3, reads as constant 0xc00 from NS EL1 and NS EL2
> +     */
> +    if (arm_feature(env, ARM_FEATURE_EL3)) {
> +        if (arm_feature(env, ARM_FEATURE_AARCH64)) {
> +            ARMCPRegInfo nsacr = {
> +                .name = "NSACR", .type = ARM_CP_CONST,
> +                .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 2,
> +                .access = PL1_RW, .accessfn = nsacr_access,
> +                .resetvalue = 0xc00
> +            };
> +            define_one_arm_cp_reg(cpu, &nsacr);
> +        } else {
> +            ARMCPRegInfo nsacr = {
> +                .name = "NSACR",
> +                .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 2,
> +                .access = PL3_RW | PL1_R,
> +                .resetvalue = 0,
> +                .fieldoffset = offsetof(CPUARMState, cp15.nsacr)
> +            };
> +            define_one_arm_cp_reg(cpu, &nsacr);
> +        }
> +    } else {
> +        if (arm_feature(env, ARM_FEATURE_V8)) {
> +            ARMCPRegInfo nsacr = {
> +                .name = "NSACR", .type = ARM_CP_CONST,
> +                .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 2,
> +                .access = PL1_R,
> +                .resetvalue = 0xc00
> +            };
> +            define_one_arm_cp_reg(cpu, &nsacr);
> +        }
> +    }
> +
>      if (arm_feature(env, ARM_FEATURE_MPU)) {
>          if (arm_feature(env, ARM_FEATURE_V6)) {
>              /* PMSAv6 not implemented */
> -- 
> 1.9.1
> 

[Qemu-devel] [PATCH 7/7] target-arm: Enable EL3 for Cortex-A53 and Cortex-A57

[Peter Maydell]

Enable EL3 support for our Cortex-A53 and Cortex-A57 CPU models.
We have enough implemented now to be able to run real world code
at least to some extent (I can boot ARM Trusted Firmware to the
point where it pulls in OP-TEE and then falls over because it
doesn't have a UEFI image it can chain to).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/cpu64.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
index cc177bb..073677b5 100644
--- a/target-arm/cpu64.c
+++ b/target-arm/cpu64.c
@@ -109,6 +109,7 @@ static void aarch64_a57_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_V8_SHA256);
     set_feature(&cpu->env, ARM_FEATURE_V8_PMULL);
     set_feature(&cpu->env, ARM_FEATURE_CRC);
+    set_feature(&cpu->env, ARM_FEATURE_EL3);
     cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A57;
     cpu->midr = 0x411fd070;
     cpu->revidr = 0x00000000;
@@ -161,6 +162,7 @@ static void aarch64_a53_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_V8_SHA256);
     set_feature(&cpu->env, ARM_FEATURE_V8_PMULL);
     set_feature(&cpu->env, ARM_FEATURE_CRC);
+    set_feature(&cpu->env, ARM_FEATURE_EL3);
     cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A53;
     cpu->midr = 0x410fd034;
     cpu->revidr = 0x00000000;
-- 
1.9.1

Re: [Qemu-devel] [PATCH 7/7] target-arm: Enable EL3 for Cortex-A53 and Cortex-A57

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> Enable EL3 support for our Cortex-A53 and Cortex-A57 CPU models.
> We have enough implemented now to be able to run real world code
> at least to some extent (I can boot ARM Trusted Firmware to the
> point where it pulls in OP-TEE and then falls over because it
> doesn't have a UEFI image it can chain to).
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  target-arm/cpu64.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
> index cc177bb..073677b5 100644
> --- a/target-arm/cpu64.c
> +++ b/target-arm/cpu64.c
> @@ -109,6 +109,7 @@ static void aarch64_a57_initfn(Object *obj)
>      set_feature(&cpu->env, ARM_FEATURE_V8_SHA256);
>      set_feature(&cpu->env, ARM_FEATURE_V8_PMULL);
>      set_feature(&cpu->env, ARM_FEATURE_CRC);
> +    set_feature(&cpu->env, ARM_FEATURE_EL3);
>      cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A57;
>      cpu->midr = 0x411fd070;
>      cpu->revidr = 0x00000000;
> @@ -161,6 +162,7 @@ static void aarch64_a53_initfn(Object *obj)
>      set_feature(&cpu->env, ARM_FEATURE_V8_SHA256);
>      set_feature(&cpu->env, ARM_FEATURE_V8_PMULL);
>      set_feature(&cpu->env, ARM_FEATURE_CRC);
> +    set_feature(&cpu->env, ARM_FEATURE_EL3);
>      cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A53;
>      cpu->midr = 0x410fd034;
>      cpu->revidr = 0x00000000;


--
Alex Bennée

Re: [Qemu-devel] [PATCH 7/7] target-arm: Enable EL3 for Cortex-A53 and Cortex-A57

[Edgar E. Iglesias]

On Wed, Feb 03, 2016 at 01:38:41PM +0000, Peter Maydell wrote:
> Enable EL3 support for our Cortex-A53 and Cortex-A57 CPU models.
> We have enough implemented now to be able to run real world code
> at least to some extent (I can boot ARM Trusted Firmware to the
> point where it pulls in OP-TEE and then falls over because it
> doesn't have a UEFI image it can chain to).

Cool! Finally! :-)

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/cpu64.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
> index cc177bb..073677b5 100644
> --- a/target-arm/cpu64.c
> +++ b/target-arm/cpu64.c
> @@ -109,6 +109,7 @@ static void aarch64_a57_initfn(Object *obj)
>      set_feature(&cpu->env, ARM_FEATURE_V8_SHA256);
>      set_feature(&cpu->env, ARM_FEATURE_V8_PMULL);
>      set_feature(&cpu->env, ARM_FEATURE_CRC);
> +    set_feature(&cpu->env, ARM_FEATURE_EL3);
>      cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A57;
>      cpu->midr = 0x411fd070;
>      cpu->revidr = 0x00000000;
> @@ -161,6 +162,7 @@ static void aarch64_a53_initfn(Object *obj)
>      set_feature(&cpu->env, ARM_FEATURE_V8_SHA256);
>      set_feature(&cpu->env, ARM_FEATURE_V8_PMULL);
>      set_feature(&cpu->env, ARM_FEATURE_CRC);
> +    set_feature(&cpu->env, ARM_FEATURE_EL3);
>      cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A53;
>      cpu->midr = 0x410fd034;
>      cpu->revidr = 0x00000000;
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 7/7] target-arm: Enable EL3 for Cortex-A53 and Cortex-A57

[Sergey Fedorov]

On 03.02.2016 16:38, Peter Maydell wrote:
> Enable EL3 support for our Cortex-A53 and Cortex-A57 CPU models.
> We have enough implemented now to be able to run real world code
> at least to some extent (I can boot ARM Trusted Firmware to the
> point where it pulls in OP-TEE and then falls over because it
> doesn't have a UEFI image it can chain to).
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>

> ---
>  target-arm/cpu64.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
> index cc177bb..073677b5 100644
> --- a/target-arm/cpu64.c
> +++ b/target-arm/cpu64.c
> @@ -109,6 +109,7 @@ static void aarch64_a57_initfn(Object *obj)
>      set_feature(&cpu->env, ARM_FEATURE_V8_SHA256);
>      set_feature(&cpu->env, ARM_FEATURE_V8_PMULL);
>      set_feature(&cpu->env, ARM_FEATURE_CRC);
> +    set_feature(&cpu->env, ARM_FEATURE_EL3);
>      cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A57;
>      cpu->midr = 0x411fd070;
>      cpu->revidr = 0x00000000;
> @@ -161,6 +162,7 @@ static void aarch64_a53_initfn(Object *obj)
>      set_feature(&cpu->env, ARM_FEATURE_V8_SHA256);
>      set_feature(&cpu->env, ARM_FEATURE_V8_PMULL);
>      set_feature(&cpu->env, ARM_FEATURE_CRC);
> +    set_feature(&cpu->env, ARM_FEATURE_EL3);
>      cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A53;
>      cpu->midr = 0x410fd034;
>      cpu->revidr = 0x00000000;

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/7] Fix some more EL3 things and enable EL3 for AArch64

[Peter Maydell]

On 3 February 2016 at 13:38, Peter Maydell <peter.maydell@linaro.org> wrote:
> This series fixes a couple more minor EL3 related missing parts,
> and then turns on EL3 for AArch64 CPUs. The minor fixed things are:
>  * implement MDCR_EL3 and SDCR
>  * trap Secure EL1 accesses to SCR and MVBAR to EL3
>  * add EL3 support to the code that decides whether to generate
>    debug exceptions
>  * fix corner cases in our NSACR handling
>
> To do the NSACR fix I had to change the CPAccessFn API to take
> an extra bool to tell the function if the access is a read or write.
>
> The only major thing I know of that we're missing for 64-bit EL3
> is that we need to go through the "EL3 configurable controls" section
> of the ARM ARM to make sure we trap on the right things. But
> (a) I expect we're missing some for 32-bit as well and (b) this
> is enough to run some real-world EL3 code (ARM Trusted Firmware
> and OP-TEE), so it makes sense to me to turn on EL3 now.

Applied to target-arm.next with some very minor tweaks to patch 2
(add .resetvalue = 0 to the MDCR_EL3 regdef, add a line to the comment.)

thanks
-- PMM