[Qemu-devel] [RFC PATCH 00/21] target-arm: add CPU core TrustZone support

[Qemu-devel] [RFC PATCH 00/21] target-arm: add CPU core TrustZone support

[Sergey Fedorov]

This patch set implements a basic support of CPU core TrustZone feature. The
following major functionalities are implemented:
  * CPU monitor mode
  * Separate code translation for each secure state
  * CPACR & NSACR co-processor access control
  * Separate TLB for each secure state
  * Co-processor register banking
  * SMC instruction
  * FIQ/IRQ routing to monitor mode

There is no support for banked co-processor register migration, save/load its
VM state yet. That is an open question how to implement this functionality. Any
suggestions is greatly appreciated.

This patch set is a request for comments for the proof of concept.

Sergey Fedorov (18):
  target-arm: move SCR & VBAR into TrustZone register list
  target-arm: adjust TTBCR for TrustZone feature
  target-arm: add arm_is_secure() helper
  target-arm: reject switching to monitor mode from non-secure state
  target-arm: adjust arm_current_pl() for TrustZone
  target-arm: adjust SCR CP15 register access rights
  target-arm: add non-secure Translation Block flag
  target-arm: implement CPACR register logic
  target-arm: add NSACR support
  target-arm: add SDER definition
  target-arm: split TLB for secure state
  target-arm: add banked coprocessor register type
  target-arm: convert appropriate coprocessor registers to banked type
  target-arm: use c13_context field for CONTEXTIDR
  target-arm: switch banked CP registers
  target-arm: add MVBAR support
  target-arm: implement SMC instruction
  target-arm: implement IRQ/FIQ routing to Monitor mode

Svetlana Fedoseeva (3):
  target-arm: add TrustZone CPU feature
  target-arm: preserve RAO/WI bits of ARMv7 SCTLR
  target-arm: add CPU Monitor mode

 target-arm/cpu.c       |    6 +-
 target-arm/cpu.h       |  126 ++++++++++++----
 target-arm/helper.c    |  308 +++++++++++++++++++++++++++++---------
 target-arm/machine.c   |   12 +-
 target-arm/translate.c |  388 ++++++++++++++++++++++++++++++------------------
 target-arm/translate.h |    2 +
 6 files changed, 585 insertions(+), 257 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [RFC PATCH 01/21] target-arm: add TrustZone CPU feature

[Sergey Fedorov]

From: Svetlana Fedoseeva <s.fedoseeva@samsung.com>

Define TrustZone CPU feature. Set that feature for relevant CPUs.

Signed-off-by: Svetlana Fedoseeva <s.fedoseeva@samsung.com>
Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
---
 target-arm/cpu.c |    4 ++++
 target-arm/cpu.h |    1 +
 2 files changed, 5 insertions(+)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index d40f2a7..4607ca8 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -439,6 +439,7 @@ static void arm1176_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);
     set_feature(&cpu->env, ARM_FEATURE_CACHE_DIRTY_REG);
     set_feature(&cpu->env, ARM_FEATURE_CACHE_BLOCK_OPS);
+    set_feature(&cpu->env, ARM_FEATURE_TRUSTZONE);
     cpu->midr = 0x410fb767;
     cpu->reset_fpsid = 0x410120b5;
     cpu->mvfr0 = 0x11111111;
@@ -521,6 +522,7 @@ static void cortex_a8_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
     set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);
+    set_feature(&cpu->env, ARM_FEATURE_TRUSTZONE);
     cpu->midr = 0x410fc080;
     cpu->reset_fpsid = 0x410330c0;
     cpu->mvfr0 = 0x11110222;
@@ -585,6 +587,7 @@ static void cortex_a9_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_VFP_FP16);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
+    set_feature(&cpu->env, ARM_FEATURE_TRUSTZONE);
     /* Note that A9 supports the MP extensions even for
      * A9UP and single-core A9MP (which are both different
      * and valid configurations; we don't model A9UP).
@@ -658,6 +661,7 @@ static void cortex_a15_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
     set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);
     set_feature(&cpu->env, ARM_FEATURE_LPAE);
+    set_feature(&cpu->env, ARM_FEATURE_TRUSTZONE);
     cpu->midr = 0x412fc0f1;
     cpu->reset_fpsid = 0x410430f0;
     cpu->mvfr0 = 0x10110222;
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 9f110f1..0b93e39 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -465,6 +465,7 @@ enum arm_features {
     ARM_FEATURE_LPAE, /* has Large Physical Address Extension */
     ARM_FEATURE_V8,
     ARM_FEATURE_AARCH64, /* supports 64 bit mode */
+    ARM_FEATURE_TRUSTZONE, /* has TrustZone Security Extensions */
 };
 
 static inline int arm_feature(CPUARMState *env, int feature)
-- 
1.7.9.5

[Qemu-devel] [RFC PATCH 02/21] target-arm: move SCR & VBAR into TrustZone register list

[Sergey Fedorov]

Define a new ARM CP register info list for TrustZone Security Extension
feature. Register that list only for ARM cores with TrustZone support.
SCR and VBAR are security extension registers. So move them into
TrustZone feature register list.

Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
---
 target-arm/helper.c |   39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 3445813..a247ca0 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -543,13 +543,6 @@ static int pmintenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
     return 0;
 }
 
-static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
-                      uint64_t value)
-{
-    env->cp15.c12_vbar = value & ~0x1Ful;
-    return 0;
-}
-
 static int ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri,
                        uint64_t *value)
 {
@@ -635,13 +628,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .access = PL1_RW, .type = ARM_CP_NO_MIGRATE,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
       .resetvalue = 0, .writefn = pmintenclr_write, },
-    { .name = "VBAR", .cp = 15, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
-      .access = PL1_RW, .writefn = vbar_write,
-      .fieldoffset = offsetof(CPUARMState, cp15.c12_vbar),
-      .resetvalue = 0 },
-    { .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0,
-      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr),
-      .resetvalue = 0, },
     { .name = "CCSIDR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0,
       .access = PL1_R, .readfn = ccsidr_read, .type = ARM_CP_NO_MIGRATE },
     { .name = "CSSELR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 2, .opc2 = 0,
@@ -1526,6 +1512,28 @@ static int sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
     return 0;
 }
 
+#ifndef CONFIG_USER_ONLY
+static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                      uint64_t value)
+{
+    env->cp15.c12_vbar = value & ~0x1Ful;
+    return 0;
+}
+#endif
+
+static const ARMCPRegInfo tz_cp_reginfo[] = {
+#ifndef CONFIG_USER_ONLY
+    { .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0,
+      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr),
+      .resetvalue = 0 },
+    { .name = "VBAR", .cp = 15, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
+      .access = PL1_RW, .writefn = vbar_write,
+      .fieldoffset = offsetof(CPUARMState, cp15.c12_vbar),
+      .resetvalue = 0 },
+#endif
+    REGINFO_SENTINEL
+};
+
 void register_cp_regs_for_features(ARMCPU *cpu)
 {
     /* Register all the coprocessor registers based on feature bits */
@@ -1663,6 +1671,9 @@ void register_cp_regs_for_features(ARMCPU *cpu)
     if (arm_feature(env, ARM_FEATURE_LPAE)) {
         define_arm_cp_regs(cpu, lpae_cp_reginfo);
     }
+    if (arm_feature(env, ARM_FEATURE_TRUSTZONE)) {
+        define_arm_cp_regs(cpu, tz_cp_reginfo);
+    }
     /* Slightly awkwardly, the OMAP and StrongARM cores need all of
      * cp15 crn=0 to be writes-ignored, whereas for other cores they should
      * be read-only (ie write causes UNDEF exception).
-- 
1.7.9.5

Re: [Qemu-devel] [RFC PATCH 02/21] target-arm: move SCR & VBAR into TrustZone register list

[Peter Crosthwaite]

On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com> wrote:
> Define a new ARM CP register info list for TrustZone Security Extension
> feature. Register that list only for ARM cores with TrustZone support.
> SCR and VBAR are security extension registers. So move them into
> TrustZone feature register list.
>
> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
> ---
>  target-arm/helper.c |   39 +++++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 3445813..a247ca0 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -543,13 +543,6 @@ static int pmintenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      return 0;
>  }
>
> -static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
> -                      uint64_t value)
> -{
> -    env->cp15.c12_vbar = value & ~0x1Ful;
> -    return 0;
> -}
> -
>  static int ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri,
>                         uint64_t *value)
>  {
> @@ -635,13 +628,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>        .access = PL1_RW, .type = ARM_CP_NO_MIGRATE,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
>        .resetvalue = 0, .writefn = pmintenclr_write, },
> -    { .name = "VBAR", .cp = 15, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
> -      .access = PL1_RW, .writefn = vbar_write,
> -      .fieldoffset = offsetof(CPUARMState, cp15.c12_vbar),
> -      .resetvalue = 0 },
> -    { .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0,
> -      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr),
> -      .resetvalue = 0, },
>      { .name = "CCSIDR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0,
>        .access = PL1_R, .readfn = ccsidr_read, .type = ARM_CP_NO_MIGRATE },
>      { .name = "CSSELR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 2, .opc2 = 0,
> @@ -1526,6 +1512,28 @@ static int sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>      return 0;
>  }
>
> +#ifndef CONFIG_USER_ONLY
> +static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                      uint64_t value)
> +{
> +    env->cp15.c12_vbar = value & ~0x1Ful;
> +    return 0;
> +}
> +#endif
> +
> +static const ARMCPRegInfo tz_cp_reginfo[] = {
> +#ifndef CONFIG_USER_ONLY
> +    { .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0,
> +      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr),
> +      .resetvalue = 0 },
> +    { .name = "VBAR", .cp = 15, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
> +      .access = PL1_RW, .writefn = vbar_write,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c12_vbar),
> +      .resetvalue = 0 },
> +#endif
> +    REGINFO_SENTINEL
> +};
> +
>  void register_cp_regs_for_features(ARMCPU *cpu)
>  {
>      /* Register all the coprocessor registers based on feature bits */
> @@ -1663,6 +1671,9 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>      if (arm_feature(env, ARM_FEATURE_LPAE)) {
>          define_arm_cp_regs(cpu, lpae_cp_reginfo);
>      }
> +    if (arm_feature(env, ARM_FEATURE_TRUSTZONE)) {
> +        define_arm_cp_regs(cpu, tz_cp_reginfo);

So ARM docmentation refers to these features as being conditional on
the "security extensions" option, not "trustzone". To match
documentation i think it may actually be
ARM_FEATURE_SECURITY_EXTENSIONS (or some truntaction thereof for
brevity). On what level of ARM documentation is the "trustzone" term
defined?

Regards,
Peter

> +    }
>      /* Slightly awkwardly, the OMAP and StrongARM cores need all of
>       * cp15 crn=0 to be writes-ignored, whereas for other cores they should
>       * be read-only (ie write causes UNDEF exception).
> --
> 1.7.9.5
>
>

Re: [Qemu-devel] [RFC PATCH 02/21] target-arm: move SCR & VBAR into TrustZone register list

[Fedorov Sergey]

On 12/19/2013 07:12 AM, Peter Crosthwaite wrote:
> On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com> wrote:
>> Define a new ARM CP register info list for TrustZone Security Extension
>> feature. Register that list only for ARM cores with TrustZone support.
>> SCR and VBAR are security extension registers. So move them into
>> TrustZone feature register list.
>>
>> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
>> ---
>>   target-arm/helper.c |   39 +++++++++++++++++++++++++--------------
>>   1 file changed, 25 insertions(+), 14 deletions(-)
>>
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 3445813..a247ca0 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -543,13 +543,6 @@ static int pmintenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>       return 0;
>>   }
>>
>> -static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
>> -                      uint64_t value)
>> -{
>> -    env->cp15.c12_vbar = value & ~0x1Ful;
>> -    return 0;
>> -}
>> -
>>   static int ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri,
>>                          uint64_t *value)
>>   {
>> @@ -635,13 +628,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>>         .access = PL1_RW, .type = ARM_CP_NO_MIGRATE,
>>         .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
>>         .resetvalue = 0, .writefn = pmintenclr_write, },
>> -    { .name = "VBAR", .cp = 15, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
>> -      .access = PL1_RW, .writefn = vbar_write,
>> -      .fieldoffset = offsetof(CPUARMState, cp15.c12_vbar),
>> -      .resetvalue = 0 },
>> -    { .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0,
>> -      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr),
>> -      .resetvalue = 0, },
>>       { .name = "CCSIDR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0,
>>         .access = PL1_R, .readfn = ccsidr_read, .type = ARM_CP_NO_MIGRATE },
>>       { .name = "CSSELR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 2, .opc2 = 0,
>> @@ -1526,6 +1512,28 @@ static int sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>>       return 0;
>>   }
>>
>> +#ifndef CONFIG_USER_ONLY
>> +static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
>> +                      uint64_t value)
>> +{
>> +    env->cp15.c12_vbar = value & ~0x1Ful;
>> +    return 0;
>> +}
>> +#endif
>> +
>> +static const ARMCPRegInfo tz_cp_reginfo[] = {
>> +#ifndef CONFIG_USER_ONLY
>> +    { .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0,
>> +      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr),
>> +      .resetvalue = 0 },
>> +    { .name = "VBAR", .cp = 15, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
>> +      .access = PL1_RW, .writefn = vbar_write,
>> +      .fieldoffset = offsetof(CPUARMState, cp15.c12_vbar),
>> +      .resetvalue = 0 },
>> +#endif
>> +    REGINFO_SENTINEL
>> +};
>> +
>>   void register_cp_regs_for_features(ARMCPU *cpu)
>>   {
>>       /* Register all the coprocessor registers based on feature bits */
>> @@ -1663,6 +1671,9 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>>       if (arm_feature(env, ARM_FEATURE_LPAE)) {
>>           define_arm_cp_regs(cpu, lpae_cp_reginfo);
>>       }
>> +    if (arm_feature(env, ARM_FEATURE_TRUSTZONE)) {
>> +        define_arm_cp_regs(cpu, tz_cp_reginfo);
> So ARM docmentation refers to these features as being conditional on
> the "security extensions" option, not "trustzone". To match
> documentation i think it may actually be
> ARM_FEATURE_SECURITY_EXTENSIONS (or some truntaction thereof for
> brevity). On what level of ARM documentation is the "trustzone" term
> defined?
>
> Regards,
> Peter

The "TrustZone" term is not mentioned in ARM architecture manual. That 
is a name for technology of system-wide approach to security. So 
strictly speaking there should be used term "Security Extensions". I 
cannot find any official truncation of this term.

Best regards,
Sergey Fedorov

>> +    }
>>       /* Slightly awkwardly, the OMAP and StrongARM cores need all of
>>        * cp15 crn=0 to be writes-ignored, whereas for other cores they should
>>        * be read-only (ie write causes UNDEF exception).
>> --
>> 1.7.9.5
>>
>>

[Qemu-devel] [RFC PATCH 03/21] target-arm: adjust TTBCR for TrustZone feature

[Sergey Fedorov]

TTBCR has additional fields PD0 and PD1 when using Short-descriptor
translation table format on a CPU with TrustZone feature support.

Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
---
 target-arm/helper.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index a247ca0..6642e53 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1159,8 +1159,10 @@ static int vmsa_ttbcr_raw_write(CPUARMState *env, const ARMCPRegInfo *ri,
 {
     int maskshift = extract32(value, 0, 3);
 
-    if (arm_feature(env, ARM_FEATURE_LPAE)) {
+    if (arm_feature(env, ARM_FEATURE_LPAE) && (value & (1 << 31))) {
         value &= ~((7 << 19) | (3 << 14) | (0xf << 3));
+    } else if (arm_feature(env, ARM_FEATURE_TRUSTZONE)) {
+        value &= 0x37;
     } else {
         value &= 7;
     }
-- 
1.7.9.5

Re: [Qemu-devel] [RFC PATCH 03/21] target-arm: adjust TTBCR for TrustZone feature

[Peter Crosthwaite]

On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com> wrote:
> TTBCR has additional fields PD0 and PD1 when using Short-descriptor
> translation table format on a CPU with TrustZone feature support.
>
> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
> ---
>  target-arm/helper.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index a247ca0..6642e53 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1159,8 +1159,10 @@ static int vmsa_ttbcr_raw_write(CPUARMState *env, const ARMCPRegInfo *ri,
>  {
>      int maskshift = extract32(value, 0, 3);
>
> -    if (arm_feature(env, ARM_FEATURE_LPAE)) {
> +    if (arm_feature(env, ARM_FEATURE_LPAE) && (value & (1 << 31))) {

This appears to be changing more than just trustzone dependent
behavior. That is, if we take just this hunk and ignore the one below
you see a change in the non-tz behaviour. Is the hunk legitimate
irrespective of trustzone support?

>          value &= ~((7 << 19) | (3 << 14) | (0xf << 3));
> +    } else if (arm_feature(env, ARM_FEATURE_TRUSTZONE)) {
> +        value &= 0x37;
>      } else {
>          value &= 7;
>      }

There are a few magic numbers in the patch probably worth macrofiying.

Regards,
Peter

> --
> 1.7.9.5
>
>

Re: [Qemu-devel] [RFC PATCH 03/21] target-arm: adjust TTBCR for TrustZone feature

[Fedorov Sergey]

On 12/03/2013 04:15 PM, Peter Crosthwaite wrote:
> On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com> wrote:
>> TTBCR has additional fields PD0 and PD1 when using Short-descriptor
>> translation table format on a CPU with TrustZone feature support.
>>
>> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
>> ---
>>   target-arm/helper.c |    4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index a247ca0..6642e53 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -1159,8 +1159,10 @@ static int vmsa_ttbcr_raw_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>   {
>>       int maskshift = extract32(value, 0, 3);
>>
>> -    if (arm_feature(env, ARM_FEATURE_LPAE)) {
>> +    if (arm_feature(env, ARM_FEATURE_LPAE) && (value & (1 << 31))) {
> This appears to be changing more than just trustzone dependent
> behavior. That is, if we take just this hunk and ignore the one below
> you see a change in the non-tz behaviour. Is the hunk legitimate
> irrespective of trustzone support?

Yes, current implementation is not accurate according to ARMv7-AR 
reference manual. See "B4.1.153 TTBCR, Translation Table Base Control 
Register, VMSA | TTBCR format when using the Long-descriptor translation 
table format". When LPAE feature is supported, EAE, bit[31] selects 
translation descriptor format and, therefore, TTBCR format.

>
>>           value &= ~((7 << 19) | (3 << 14) | (0xf << 3));
>> +    } else if (arm_feature(env, ARM_FEATURE_TRUSTZONE)) {
>> +        value &= 0x37;
>>       } else {
>>           value &= 7;
>>       }
> There are a few magic numbers in the patch probably worth macrofiying.

As I can see, magic numbers are widely used through all of this file to 
represent CP register fields and other things. Maybe the macrofying 
should be done separately from this patch series?

>
> Regards,
> Peter
>
>> --
>> 1.7.9.5
>>
>>
>

Best regards,
Sergey Fedorov

Re: [Qemu-devel] [RFC PATCH 03/21] target-arm: adjust TTBCR for TrustZone feature

[Peter Crosthwaite]

On Wed, Dec 4, 2013 at 7:50 PM, Fedorov Sergey <s.fedorov@samsung.com> wrote:
>
> On 12/03/2013 04:15 PM, Peter Crosthwaite wrote:
>>
>> On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com>
>> wrote:
>>>
>>> TTBCR has additional fields PD0 and PD1 when using Short-descriptor
>>> translation table format on a CPU with TrustZone feature support.
>>>
>>> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
>>> ---
>>>   target-arm/helper.c |    4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>>> index a247ca0..6642e53 100644
>>> --- a/target-arm/helper.c
>>> +++ b/target-arm/helper.c
>>> @@ -1159,8 +1159,10 @@ static int vmsa_ttbcr_raw_write(CPUARMState *env,
>>> const ARMCPRegInfo *ri,
>>>   {
>>>       int maskshift = extract32(value, 0, 3);
>>>
>>> -    if (arm_feature(env, ARM_FEATURE_LPAE)) {
>>> +    if (arm_feature(env, ARM_FEATURE_LPAE) && (value & (1 << 31))) {
>>
>> This appears to be changing more than just trustzone dependent
>> behavior. That is, if we take just this hunk and ignore the one below
>> you see a change in the non-tz behaviour. Is the hunk legitimate
>> irrespective of trustzone support?
>
>
> Yes, current implementation is not accurate according to ARMv7-AR reference
> manual. See "B4.1.153 TTBCR, Translation Table Base Control Register, VMSA |
> TTBCR format when using the Long-descriptor translation table format". When
> LPAE feature is supported, EAE, bit[31] selects translation descriptor
> format and, therefore, TTBCR format.
>

Ok,

You should probably split it off as a separate patch. And you could
send probably send it immediately. What you just wrote is a nice
commit message.

>
>>
>>>           value &= ~((7 << 19) | (3 << 14) | (0xf << 3));
>>> +    } else if (arm_feature(env, ARM_FEATURE_TRUSTZONE)) {
>>> +        value &= 0x37;
>>>       } else {
>>>           value &= 7;
>>>       }
>>
>> There are a few magic numbers in the patch probably worth macrofiying.
>
>
> As I can see, magic numbers are widely used through all of this file to
> represent CP register fields and other things. Maybe the macrofying should
> be done separately from this patch series?
>

So target-arm is riddled with legacy style. Converting all of
target-arm to self-doccing macros is a big job, but if we keep
expanding without doing it that job only gets bigger. I'll defer to
PMM on what we want to policy to be going forward. - any thoughts?

Regards,
Peter

>>
>> Regards,
>> Peter
>>
>>> --
>>> 1.7.9.5
>>>
>>>
>>
>
> Best regards,
> Sergey Fedorov
>

Re: [Qemu-devel] [RFC PATCH 03/21] target-arm: adjust TTBCR for TrustZone feature

[Peter Crosthwaite]

On Wed, Dec 4, 2013 at 8:52 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Wed, Dec 4, 2013 at 7:50 PM, Fedorov Sergey <s.fedorov@samsung.com> wrote:
>>
>> On 12/03/2013 04:15 PM, Peter Crosthwaite wrote:
>>>
>>> On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com>
>>> wrote:
>>>>
>>>> TTBCR has additional fields PD0 and PD1 when using Short-descriptor
>>>> translation table format on a CPU with TrustZone feature support.
>>>>
>>>> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
>>>> ---
>>>>   target-arm/helper.c |    4 +++-
>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>>>> index a247ca0..6642e53 100644
>>>> --- a/target-arm/helper.c
>>>> +++ b/target-arm/helper.c
>>>> @@ -1159,8 +1159,10 @@ static int vmsa_ttbcr_raw_write(CPUARMState *env,
>>>> const ARMCPRegInfo *ri,
>>>>   {
>>>>       int maskshift = extract32(value, 0, 3);
>>>>
>>>> -    if (arm_feature(env, ARM_FEATURE_LPAE)) {
>>>> +    if (arm_feature(env, ARM_FEATURE_LPAE) && (value & (1 << 31))) {
>>>
>>> This appears to be changing more than just trustzone dependent
>>> behavior. That is, if we take just this hunk and ignore the one below
>>> you see a change in the non-tz behaviour. Is the hunk legitimate
>>> irrespective of trustzone support?
>>
>>
>> Yes, current implementation is not accurate according to ARMv7-AR reference
>> manual. See "B4.1.153 TTBCR, Translation Table Base Control Register, VMSA |
>> TTBCR format when using the Long-descriptor translation table format". When
>> LPAE feature is supported, EAE, bit[31] selects translation descriptor
>> format and, therefore, TTBCR format.
>>
>
> Ok,
>
> You should probably split it off as a separate patch. And you could
> send probably send it immediately. What you just wrote is a nice
> commit message.
>
>>
>>>
>>>>           value &= ~((7 << 19) | (3 << 14) | (0xf << 3));
>>>> +    } else if (arm_feature(env, ARM_FEATURE_TRUSTZONE)) {
>>>> +        value &= 0x37;

So if we let the magic numbers slide, can we at least add a comment
about what fields these are?

/* PD0, PD1, N */

You would also be a little more self documenting with:

1 << 5 | 1 << 4 | 0x7

Regards,
Peter

>>>>       } else {
>>>>           value &= 7;
>>>>       }
>>>
>>> There are a few magic numbers in the patch probably worth macrofiying.
>>
>>
>> As I can see, magic numbers are widely used through all of this file to
>> represent CP register fields and other things. Maybe the macrofying should
>> be done separately from this patch series?
>>
>
> So target-arm is riddled with legacy style. Converting all of
> target-arm to self-doccing macros is a big job, but if we keep
> expanding without doing it that job only gets bigger. I'll defer to
> PMM on what we want to policy to be going forward. - any thoughts?
>
> Regards,
> Peter
>
>>>
>>> Regards,
>>> Peter
>>>
>>>> --
>>>> 1.7.9.5
>>>>
>>>>
>>>
>>
>> Best regards,
>> Sergey Fedorov
>>

[Qemu-devel] [RFC PATCH 04/21] target-arm: preserve RAO/WI bits of ARMv7 SCTLR

[Sergey Fedorov]

From: Svetlana Fedoseeva <s.fedoseeva@samsung.com>

Signed-off-by: Svetlana Fedoseeva <s.fedoseeva@samsung.com>
Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
---
 target-arm/helper.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 6642e53..d7922ad 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1507,6 +1507,10 @@ static const ARMCPRegInfo lpae_cp_reginfo[] = {
 
 static int sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 {
+    if (arm_feature(env, ARM_FEATURE_V7)) {
+        value = value | 0x00c50078; /* This bits are RAO/WI */
+    }
+
     env->cp15.c1_sys = value;
     /* ??? Lots of these bits are not implemented.  */
     /* This may enable/disable the MMU, so do a TLB flush.  */
-- 
1.7.9.5

Re: [Qemu-devel] [RFC PATCH 04/21] target-arm: preserve RAO/WI bits of ARMv7 SCTLR

[Peter Crosthwaite]

On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com> wrote:
> From: Svetlana Fedoseeva <s.fedoseeva@samsung.com>
>
> Signed-off-by: Svetlana Fedoseeva <s.fedoseeva@samsung.com>
> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
> ---
>  target-arm/helper.c |    4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 6642e53..d7922ad 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1507,6 +1507,10 @@ static const ARMCPRegInfo lpae_cp_reginfo[] = {
>
>  static int sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>  {
> +    if (arm_feature(env, ARM_FEATURE_V7)) {
> +        value = value | 0x00c50078; /* This bits are RAO/WI */

Magic number. "these bits ".

> +    }
> +
>      env->cp15.c1_sys = value;
>      /* ??? Lots of these bits are not implemented.  */
>      /* This may enable/disable the MMU, so do a TLB flush.  */
> --
> 1.7.9.5
>
>

Re: [Qemu-devel] [RFC PATCH 04/21] target-arm: preserve RAO/WI bits of ARMv7 SCTLR

[Fedorov Sergey]

On 12/03/2013 04:17 PM, Peter Crosthwaite wrote:
> On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com> wrote:
>> From: Svetlana Fedoseeva <s.fedoseeva@samsung.com>
>>
>> Signed-off-by: Svetlana Fedoseeva <s.fedoseeva@samsung.com>
>> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
>> ---
>>   target-arm/helper.c |    4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 6642e53..d7922ad 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -1507,6 +1507,10 @@ static const ARMCPRegInfo lpae_cp_reginfo[] = {
>>
>>   static int sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>>   {
>> +    if (arm_feature(env, ARM_FEATURE_V7)) {
>> +        value = value | 0x00c50078; /* This bits are RAO/WI */
> Magic number. "these bits ".

Would be acceptable to substitute this magic number with "bitshifted 
constants combined with bitwise or", e.g. as in vmsa_ttbcr_raw_write()?

>
>> +    }
>> +
>>       env->cp15.c1_sys = value;
>>       /* ??? Lots of these bits are not implemented.  */
>>       /* This may enable/disable the MMU, so do a TLB flush.  */
>> --
>> 1.7.9.5
>>
>>
>

-- 
Best regards,
Sergey Fedorov, Junior Software Engineer,
Samsung R&D Institute Rus.
E-mail: s.fedorov@samsung.com

Re: [Qemu-devel] [RFC PATCH 04/21] target-arm: preserve RAO/WI bits of ARMv7 SCTLR

[Peter Crosthwaite]

On Wed, Dec 4, 2013 at 7:55 PM, Fedorov Sergey <s.fedorov@samsung.com> wrote:
>
> On 12/03/2013 04:17 PM, Peter Crosthwaite wrote:
>>
>> On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com>
>> wrote:
>>>
>>> From: Svetlana Fedoseeva <s.fedoseeva@samsung.com>
>>>
>>> Signed-off-by: Svetlana Fedoseeva <s.fedoseeva@samsung.com>
>>> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
>>> ---
>>>   target-arm/helper.c |    4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>>> index 6642e53..d7922ad 100644
>>> --- a/target-arm/helper.c
>>> +++ b/target-arm/helper.c
>>> @@ -1507,6 +1507,10 @@ static const ARMCPRegInfo lpae_cp_reginfo[] = {
>>>
>>>   static int sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>> uint64_t value)
>>>   {
>>> +    if (arm_feature(env, ARM_FEATURE_V7)) {
>>> +        value = value | 0x00c50078; /* This bits are RAO/WI */
>>
>> Magic number. "these bits ".
>
>
> Would be acceptable to substitute this magic number with "bitshifted
> constants combined with bitwise or", e.g. as in vmsa_ttbcr_raw_write()?
>

Yes I think so, that will make life easier for the big macro
conversion (one day) :)

Regards,
Peter

>
>>
>>> +    }
>>> +
>>>       env->cp15.c1_sys = value;
>>>       /* ??? Lots of these bits are not implemented.  */
>>>       /* This may enable/disable the MMU, so do a TLB flush.  */
>>> --
>>> 1.7.9.5
>>>
>>>
>>
>
> --
> Best regards,
> Sergey Fedorov, Junior Software Engineer,
> Samsung R&D Institute Rus.
> E-mail: s.fedorov@samsung.com
>
>

[Qemu-devel] [RFC PATCH 05/21] target-arm: add CPU Monitor mode

[Sergey Fedorov]

From: Svetlana Fedoseeva <s.fedoseeva@samsung.com>

Define CPU monitor mode. Adjust core registers banking. Adjust CPU VM
state info. Provide CPU mode name for monitor mode.

Signed-off-by: Svetlana Fedoseeva <s.fedoseeva@samsung.com>
Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
---
 target-arm/cpu.h       |    7 ++++---
 target-arm/helper.c    |    3 +++
 target-arm/machine.c   |   12 ++++++------
 target-arm/translate.c |    2 +-
 4 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 0b93e39..94d8bd1 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -124,9 +124,9 @@ typedef struct CPUARMState {
     uint32_t spsr;
 
     /* Banked registers.  */
-    uint32_t banked_spsr[6];
-    uint32_t banked_r13[6];
-    uint32_t banked_r14[6];
+    uint32_t banked_spsr[7];
+    uint32_t banked_r13[7];
+    uint32_t banked_r14[7];
 
     /* These hold r8-r12.  */
     uint32_t usr_regs[5];
@@ -402,6 +402,7 @@ enum arm_cpu_mode {
   ARM_CPU_MODE_FIQ = 0x11,
   ARM_CPU_MODE_IRQ = 0x12,
   ARM_CPU_MODE_SVC = 0x13,
+  ARM_CPU_MODE_MON = 0x16,
   ARM_CPU_MODE_ABT = 0x17,
   ARM_CPU_MODE_UND = 0x1b,
   ARM_CPU_MODE_SYS = 0x1f
diff --git a/target-arm/helper.c b/target-arm/helper.c
index d7922ad..d4407cf 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2018,6 +2018,7 @@ static int bad_mode_switch(CPUARMState *env, int mode)
     case ARM_CPU_MODE_USR:
     case ARM_CPU_MODE_SYS:
     case ARM_CPU_MODE_SVC:
+    case ARM_CPU_MODE_MON:
     case ARM_CPU_MODE_ABT:
     case ARM_CPU_MODE_UND:
     case ARM_CPU_MODE_IRQ:
@@ -2202,6 +2203,8 @@ int bank_number(int mode)
         return 4;
     case ARM_CPU_MODE_FIQ:
         return 5;
+    case ARM_CPU_MODE_MON:
+        return 6;
     }
     hw_error("bank number requested for bad CPSR mode value 0x%x\n", mode);
 }
diff --git a/target-arm/machine.c b/target-arm/machine.c
index 74f010f..51d0c79 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -222,9 +222,9 @@ static int cpu_post_load(void *opaque, int version_id)
 
 const VMStateDescription vmstate_arm_cpu = {
     .name = "cpu",
-    .version_id = 13,
-    .minimum_version_id = 13,
-    .minimum_version_id_old = 13,
+    .version_id = 14,
+    .minimum_version_id = 14,
+    .minimum_version_id_old = 14,
     .pre_save = cpu_pre_save,
     .post_load = cpu_post_load,
     .fields = (VMStateField[]) {
@@ -238,9 +238,9 @@ const VMStateDescription vmstate_arm_cpu = {
             .offset = 0,
         },
         VMSTATE_UINT32(env.spsr, ARMCPU),
-        VMSTATE_UINT32_ARRAY(env.banked_spsr, ARMCPU, 6),
-        VMSTATE_UINT32_ARRAY(env.banked_r13, ARMCPU, 6),
-        VMSTATE_UINT32_ARRAY(env.banked_r14, ARMCPU, 6),
+        VMSTATE_UINT32_ARRAY(env.banked_spsr, ARMCPU, 7),
+        VMSTATE_UINT32_ARRAY(env.banked_r13, ARMCPU, 7),
+        VMSTATE_UINT32_ARRAY(env.banked_r14, ARMCPU, 7),
         VMSTATE_UINT32_ARRAY(env.usr_regs, ARMCPU, 5),
         VMSTATE_UINT32_ARRAY(env.fiq_regs, ARMCPU, 5),
         /* The length-check must come before the arrays to avoid
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 5f003e7..665c8ac 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -10295,7 +10295,7 @@ void gen_intermediate_code_pc(CPUARMState *env, TranslationBlock *tb)
 }
 
 static const char *cpu_mode_names[16] = {
-  "usr", "fiq", "irq", "svc", "???", "???", "???", "abt",
+  "usr", "fiq", "irq", "svc", "???", "???", "mon", "abt",
   "???", "???", "???", "und", "???", "???", "???", "sys"
 };
 
-- 
1.7.9.5

Re: [Qemu-devel] [RFC PATCH 05/21] target-arm: add CPU Monitor mode

[Peter Crosthwaite]

On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com> wrote:
> From: Svetlana Fedoseeva <s.fedoseeva@samsung.com>
>
> Define CPU monitor mode. Adjust core registers banking. Adjust CPU VM
> state info. Provide CPU mode name for monitor mode.
>
> Signed-off-by: Svetlana Fedoseeva <s.fedoseeva@samsung.com>
> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
> ---
>  target-arm/cpu.h       |    7 ++++---
>  target-arm/helper.c    |    3 +++
>  target-arm/machine.c   |   12 ++++++------
>  target-arm/translate.c |    2 +-
>  4 files changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 0b93e39..94d8bd1 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -124,9 +124,9 @@ typedef struct CPUARMState {
>      uint32_t spsr;
>
>      /* Banked registers.  */
> -    uint32_t banked_spsr[6];
> -    uint32_t banked_r13[6];
> -    uint32_t banked_r14[6];
> +    uint32_t banked_spsr[7];
> +    uint32_t banked_r13[7];
> +    uint32_t banked_r14[7];
>

Are there any more modes yet to be implemented? It might save on
future VMSD version bumps if we just pad this out to its ultimate
value now.

>      /* These hold r8-r12.  */
>      uint32_t usr_regs[5];
> @@ -402,6 +402,7 @@ enum arm_cpu_mode {
>    ARM_CPU_MODE_FIQ = 0x11,
>    ARM_CPU_MODE_IRQ = 0x12,
>    ARM_CPU_MODE_SVC = 0x13,
> +  ARM_CPU_MODE_MON = 0x16,
>    ARM_CPU_MODE_ABT = 0x17,
>    ARM_CPU_MODE_UND = 0x1b,
>    ARM_CPU_MODE_SYS = 0x1f
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index d7922ad..d4407cf 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -2018,6 +2018,7 @@ static int bad_mode_switch(CPUARMState *env, int mode)
>      case ARM_CPU_MODE_USR:
>      case ARM_CPU_MODE_SYS:
>      case ARM_CPU_MODE_SVC:
> +    case ARM_CPU_MODE_MON:
>      case ARM_CPU_MODE_ABT:
>      case ARM_CPU_MODE_UND:
>      case ARM_CPU_MODE_IRQ:
> @@ -2202,6 +2203,8 @@ int bank_number(int mode)
>          return 4;
>      case ARM_CPU_MODE_FIQ:
>          return 5;
> +    case ARM_CPU_MODE_MON:
> +        return 6;
>      }
>      hw_error("bank number requested for bad CPSR mode value 0x%x\n", mode);
>  }
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index 74f010f..51d0c79 100644
> --- a/target-arm/machine.c
> +++ b/target-arm/machine.c
> @@ -222,9 +222,9 @@ static int cpu_post_load(void *opaque, int version_id)
>
>  const VMStateDescription vmstate_arm_cpu = {
>      .name = "cpu",
> -    .version_id = 13,
> -    .minimum_version_id = 13,
> -    .minimum_version_id_old = 13,
> +    .version_id = 14,
> +    .minimum_version_id = 14,
> +    .minimum_version_id_old = 14,
>      .pre_save = cpu_pre_save,
>      .post_load = cpu_post_load,
>      .fields = (VMStateField[]) {
> @@ -238,9 +238,9 @@ const VMStateDescription vmstate_arm_cpu = {
>              .offset = 0,
>          },
>          VMSTATE_UINT32(env.spsr, ARMCPU),
> -        VMSTATE_UINT32_ARRAY(env.banked_spsr, ARMCPU, 6),
> -        VMSTATE_UINT32_ARRAY(env.banked_r13, ARMCPU, 6),
> -        VMSTATE_UINT32_ARRAY(env.banked_r14, ARMCPU, 6),
> +        VMSTATE_UINT32_ARRAY(env.banked_spsr, ARMCPU, 7),
> +        VMSTATE_UINT32_ARRAY(env.banked_r13, ARMCPU, 7),
> +        VMSTATE_UINT32_ARRAY(env.banked_r14, ARMCPU, 7),
>          VMSTATE_UINT32_ARRAY(env.usr_regs, ARMCPU, 5),
>          VMSTATE_UINT32_ARRAY(env.fiq_regs, ARMCPU, 5),
>          /* The length-check must come before the arrays to avoid
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 5f003e7..665c8ac 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -10295,7 +10295,7 @@ void gen_intermediate_code_pc(CPUARMState *env, TranslationBlock *tb)
>  }
>
>  static const char *cpu_mode_names[16] = {
> -  "usr", "fiq", "irq", "svc", "???", "???", "???", "abt",
> +  "usr", "fiq", "irq", "svc", "???", "???", "mon", "abt",
>    "???", "???", "???", "und", "???", "???", "???", "sys"
>  };
>
> --
> 1.7.9.5
>
>

Re: [Qemu-devel] [RFC PATCH 05/21] target-arm: add CPU Monitor mode

[Peter Maydell]

On 3 December 2013 12:20, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com> wrote:
>> From: Svetlana Fedoseeva <s.fedoseeva@samsung.com>
>>
>> Define CPU monitor mode. Adjust core registers banking. Adjust CPU VM
>> state info. Provide CPU mode name for monitor mode.
>>
>> Signed-off-by: Svetlana Fedoseeva <s.fedoseeva@samsung.com>
>> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
>> ---
>>  target-arm/cpu.h       |    7 ++++---
>>  target-arm/helper.c    |    3 +++
>>  target-arm/machine.c   |   12 ++++++------
>>  target-arm/translate.c |    2 +-
>>  4 files changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index 0b93e39..94d8bd1 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -124,9 +124,9 @@ typedef struct CPUARMState {
>>      uint32_t spsr;
>>
>>      /* Banked registers.  */
>> -    uint32_t banked_spsr[6];
>> -    uint32_t banked_r13[6];
>> -    uint32_t banked_r14[6];
>> +    uint32_t banked_spsr[7];
>> +    uint32_t banked_r13[7];
>> +    uint32_t banked_r14[7];
>>
>
> Are there any more modes yet to be implemented? It might save on
> future VMSD version bumps if we just pad this out to its ultimate
> value now.

The remaining mode defined for AArch32 which we don't
implement yet is Hyp mode, which has a banked R13 and SPSR,
but not a banked LR.

-- PMM

Re: [Qemu-devel] [RFC PATCH 05/21] target-arm: add CPU Monitor mode

[Fedorov Sergey]

On 12/03/2013 04:51 PM, Peter Maydell wrote:
> On 3 December 2013 12:20, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com> wrote:
>>> From: Svetlana Fedoseeva <s.fedoseeva@samsung.com>
>>>
>>> Define CPU monitor mode. Adjust core registers banking. Adjust CPU VM
>>> state info. Provide CPU mode name for monitor mode.
>>>
>>> Signed-off-by: Svetlana Fedoseeva <s.fedoseeva@samsung.com>
>>> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
>>> ---
>>>   target-arm/cpu.h       |    7 ++++---
>>>   target-arm/helper.c    |    3 +++
>>>   target-arm/machine.c   |   12 ++++++------
>>>   target-arm/translate.c |    2 +-
>>>   4 files changed, 14 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>>> index 0b93e39..94d8bd1 100644
>>> --- a/target-arm/cpu.h
>>> +++ b/target-arm/cpu.h
>>> @@ -124,9 +124,9 @@ typedef struct CPUARMState {
>>>       uint32_t spsr;
>>>
>>>       /* Banked registers.  */
>>> -    uint32_t banked_spsr[6];
>>> -    uint32_t banked_r13[6];
>>> -    uint32_t banked_r14[6];
>>> +    uint32_t banked_spsr[7];
>>> +    uint32_t banked_r13[7];
>>> +    uint32_t banked_r14[7];
>>>
>> Are there any more modes yet to be implemented? It might save on
>> future VMSD version bumps if we just pad this out to its ultimate
>> value now.
> The remaining mode defined for AArch32 which we don't
> implement yet is Hyp mode, which has a banked R13 and SPSR,
> but not a banked LR.
>
> -- PMM
>
>

So should a number of banked core registers be increased more? 
Personally, I'd like to keep this patch only TZ-related.

Best regards,
Sergey Fedorov

Re: [Qemu-devel] [RFC PATCH 05/21] target-arm: add CPU Monitor mode

[Peter Crosthwaite]

On Wed, Dec 4, 2013 at 8:01 PM, Fedorov Sergey <s.fedorov@samsung.com> wrote:
>
> On 12/03/2013 04:51 PM, Peter Maydell wrote:
>>
>> On 3 December 2013 12:20, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> wrote:
>>>
>>> On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com>
>>> wrote:
>>>>
>>>> From: Svetlana Fedoseeva <s.fedoseeva@samsung.com>
>>>>
>>>> Define CPU monitor mode. Adjust core registers banking. Adjust CPU VM
>>>> state info. Provide CPU mode name for monitor mode.
>>>>
>>>> Signed-off-by: Svetlana Fedoseeva <s.fedoseeva@samsung.com>
>>>> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
>>>> ---
>>>>   target-arm/cpu.h       |    7 ++++---
>>>>   target-arm/helper.c    |    3 +++
>>>>   target-arm/machine.c   |   12 ++++++------
>>>>   target-arm/translate.c |    2 +-
>>>>   4 files changed, 14 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>>>> index 0b93e39..94d8bd1 100644
>>>> --- a/target-arm/cpu.h
>>>> +++ b/target-arm/cpu.h
>>>> @@ -124,9 +124,9 @@ typedef struct CPUARMState {
>>>>       uint32_t spsr;
>>>>
>>>>       /* Banked registers.  */
>>>> -    uint32_t banked_spsr[6];
>>>> -    uint32_t banked_r13[6];
>>>> -    uint32_t banked_r14[6];
>>>> +    uint32_t banked_spsr[7];
>>>> +    uint32_t banked_r13[7];
>>>> +    uint32_t banked_r14[7];
>>>>
>>> Are there any more modes yet to be implemented? It might save on
>>> future VMSD version bumps if we just pad this out to its ultimate
>>> value now.
>>
>> The remaining mode defined for AArch32 which we don't
>> implement yet is Hyp mode, which has a banked R13 and SPSR,
>> but not a banked LR.
>>
>> -- PMM
>>
>>
>
> So should a number of banked core registers be increased more? Personally,
> I'd like to keep this patch only TZ-related.
>

So what im proposing is just a slightly more general patch. Is it
really any more complicated than just applying your change pattern for
the hyp mode? Patch subject would be something like:

"target-arm: Add all remaining missing modes"

The motiviation is less VMSD version bumps in ARM CPU (a place where I
expect assume such version bumps to be considerable annoyance).

Regards,
Peter

> Best regards,
> Sergey Fedorov
>

Re: [Qemu-devel] [RFC PATCH 05/21] target-arm: add CPU Monitor mode

[Peter Maydell]

On 4 December 2013 10:58, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> So what im proposing is just a slightly more general patch. Is it
> really any more complicated than just applying your change pattern for
> the hyp mode?

I think it would be, because of the wrinkle that hyp mode doesn't
have a banked LR, so the existing "assume we can just translate
the mode into a single index good for both LR and SP" logic
would break.

The minimal change if we wanted to keep VMSD bumps to a
minimum would be to increase the size of the banked_spsr[]
and banked_r13[] arrays to allow for Hyp mode but do nothing
else (except add a comment about it I guess).

> The motiviation is less VMSD version bumps in ARM CPU (a place where I
> expect assume such version bumps to be considerable annoyance).

Well, they're only a problem at the point where we start trying
to support cross-version migration; we're not at that point yet...

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH 05/21] target-arm: add CPU Monitor mode

[Fedorov Sergey]

On 12/04/2013 03:18 PM, Peter Maydell wrote:
> On 4 December 2013 10:58, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> So what im proposing is just a slightly more general patch. Is it
>> really any more complicated than just applying your change pattern for
>> the hyp mode?
> I think it would be, because of the wrinkle that hyp mode doesn't
> have a banked LR, so the existing "assume we can just translate
> the mode into a single index good for both LR and SP" logic
> would break.
>
> The minimal change if we wanted to keep VMSD bumps to a
> minimum would be to increase the size of the banked_spsr[]
> and banked_r13[] arrays to allow for Hyp mode but do nothing
> else (except add a comment about it I guess).

If we want to bump VMSD just once for monitor + hypervisor mode then we 
need to add ELR_hyp register definition too. I think then it would be 
better to implement hypervisor mode and its special banking scheme, too. 
But I doubt it worth to combine these two things into one patch.

>
>> The motiviation is less VMSD version bumps in ARM CPU (a place where I
>> expect assume such version bumps to be considerable annoyance).
> Well, they're only a problem at the point where we start trying
> to support cross-version migration; we're not at that point yet...
>
> thanks
> -- PMM
>

Best regards,
Sergey Fedorov

Re: [Qemu-devel] [RFC PATCH 05/21] target-arm: add CPU Monitor mode

[Peter Maydell]

On 4 December 2013 12:33, Fedorov Sergey <s.fedorov@samsung.com> wrote:
>
> On 12/04/2013 03:18 PM, Peter Maydell wrote:
>>
>> On 4 December 2013 10:58, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> wrote:
>>>
>>> So what im proposing is just a slightly more general patch. Is it
>>> really any more complicated than just applying your change pattern for
>>> the hyp mode?
>>
>> I think it would be, because of the wrinkle that hyp mode doesn't
>> have a banked LR, so the existing "assume we can just translate
>> the mode into a single index good for both LR and SP" logic
>> would break.
>>
>> The minimal change if we wanted to keep VMSD bumps to a
>> minimum would be to increase the size of the banked_spsr[]
>> and banked_r13[] arrays to allow for Hyp mode but do nothing
>> else (except add a comment about it I guess).
>
>
> If we want to bump VMSD just once for monitor + hypervisor mode then we need
> to add ELR_hyp register definition too. I think then it would be better to
> implement hypervisor mode and its special banking scheme, too. But I doubt
> it worth to combine these two things into one patch.

It's possible to add single new fields to the VMState without
requiring a compatibility break, by marking the new field as
"only present in version X or greater"; new elements on the
end of arrays are a little fiddlier.

But yes, I think we should just not worry about possible future
Hyp mode now. Let's stick with your current patch.

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH 05/21] target-arm: add CPU Monitor mode

[Peter Crosthwaite]

On Wed, Dec 4, 2013 at 10:35 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 4 December 2013 12:33, Fedorov Sergey <s.fedorov@samsung.com> wrote:
>>
>> On 12/04/2013 03:18 PM, Peter Maydell wrote:
>>>
>>> On 4 December 2013 10:58, Peter Crosthwaite
>>> <peter.crosthwaite@xilinx.com> wrote:
>>>>
>>>> So what im proposing is just a slightly more general patch. Is it
>>>> really any more complicated than just applying your change pattern for
>>>> the hyp mode?
>>>
>>> I think it would be, because of the wrinkle that hyp mode doesn't
>>> have a banked LR, so the existing "assume we can just translate
>>> the mode into a single index good for both LR and SP" logic
>>> would break.
>>>
>>> The minimal change if we wanted to keep VMSD bumps to a
>>> minimum would be to increase the size of the banked_spsr[]
>>> and banked_r13[] arrays to allow for Hyp mode but do nothing
>>> else (except add a comment about it I guess).
>>
>>
>> If we want to bump VMSD just once for monitor + hypervisor mode then we need
>> to add ELR_hyp register definition too. I think then it would be better to
>> implement hypervisor mode and its special banking scheme, too. But I doubt
>> it worth to combine these two things into one patch.
>
> It's possible to add single new fields to the VMState without
> requiring a compatibility break, by marking the new field as
> "only present in version X or greater"; new elements on the
> end of arrays are a little fiddlier.
>
> But yes, I think we should just not worry about possible future
> Hyp mode now. Let's stick with your current patch.
>

+1. Patch is good for the moment.

> thanks
> -- PMM
>

[Qemu-devel] [RFC PATCH 06/21] target-arm: add arm_is_secure() helper

[Sergey Fedorov]

arm_is_secure() helper allows to determine CPU security state.

Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
---
 target-arm/cpu.h |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 94d8bd1..a00c86f 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -474,6 +474,17 @@ static inline int arm_feature(CPUARMState *env, int feature)
     return (env->features & (1ULL << feature)) != 0;
 }
 
+/* Return non-zero if the processor is in Secure state */
+static inline bool arm_is_secure(CPUARMState *env)
+{
+#if !defined(CONFIG_USER_ONLY)
+    return ((env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_MON) ||
+            !(env->cp15.c1_scr & 1);
+#else
+    return false;
+#endif
+}
+
 void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf);
 
 /* Interface between CPU and Interrupt controller.  */
-- 
1.7.9.5

Re: [Qemu-devel] [RFC PATCH 06/21] target-arm: add arm_is_secure() helper

[Peter Crosthwaite]

On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com> wrote:
> arm_is_secure() helper allows to determine CPU security state.
>

Helper in the target-foo context usually refers to a TCG->C code
helper fn, whereas you are using in a general sense. Just
s/helper/function.

> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
> ---
>  target-arm/cpu.h |   11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 94d8bd1..a00c86f 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -474,6 +474,17 @@ static inline int arm_feature(CPUARMState *env, int feature)
>      return (env->features & (1ULL << feature)) != 0;
>  }
>
> +/* Return non-zero if the processor is in Secure state */

"Return true"

Regards,
Peter

> +static inline bool arm_is_secure(CPUARMState *env)
> +{
> +#if !defined(CONFIG_USER_ONLY)
> +    return ((env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_MON) ||
> +            !(env->cp15.c1_scr & 1);
> +#else
> +    return false;
> +#endif
> +}
> +
>  void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf);
>
>  /* Interface between CPU and Interrupt controller.  */
> --
> 1.7.9.5
>
>

[Qemu-devel] [RFC PATCH 07/21] target-arm: reject switching to monitor mode from non-secure state

[Sergey Fedorov]

Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
---
 target-arm/helper.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index d4407cf..e406ec9 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2018,12 +2018,13 @@ static int bad_mode_switch(CPUARMState *env, int mode)
     case ARM_CPU_MODE_USR:
     case ARM_CPU_MODE_SYS:
     case ARM_CPU_MODE_SVC:
-    case ARM_CPU_MODE_MON:
     case ARM_CPU_MODE_ABT:
     case ARM_CPU_MODE_UND:
     case ARM_CPU_MODE_IRQ:
     case ARM_CPU_MODE_FIQ:
         return 0;
+    case ARM_CPU_MODE_MON:
+        return !arm_is_secure(env);
     default:
         return 1;
     }
-- 
1.7.9.5

Re: [Qemu-devel] [RFC PATCH 07/21] target-arm: reject switching to monitor mode from non-secure state

[Peter Crosthwaite]

On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com> wrote:
>
> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
> ---
>  target-arm/helper.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index d4407cf..e406ec9 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -2018,12 +2018,13 @@ static int bad_mode_switch(CPUARMState *env, int mode)
>      case ARM_CPU_MODE_USR:
>      case ARM_CPU_MODE_SYS:
>      case ARM_CPU_MODE_SVC:
> -    case ARM_CPU_MODE_MON:

You added this in the last patch to delete it now. I dont think you
need it in prev patch for bisectability as your TZ functionality isn't
online at this stage of the series, I would just revert both the
addition and subtraction of this line in prev and this patches
respectively.

Regards,
Peter

>      case ARM_CPU_MODE_ABT:
>      case ARM_CPU_MODE_UND:
>      case ARM_CPU_MODE_IRQ:
>      case ARM_CPU_MODE_FIQ:
>          return 0;
> +    case ARM_CPU_MODE_MON:
> +        return !arm_is_secure(env);
>      default:
>          return 1;
>      }
> --
> 1.7.9.5
>
>

[Qemu-devel] [RFC PATCH 08/21] target-arm: adjust arm_current_pl() for TrustZone

[Sergey Fedorov]

Make arm_current_pl() to return PL3 in secure privileged mode.

Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
---
 target-arm/cpu.h |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index a00c86f..1b03450 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -622,9 +622,11 @@ static inline int arm_current_pl(CPUARMState *env)
 {
     if ((env->uncached_cpsr & 0x1f) == ARM_CPU_MODE_USR) {
         return 0;
+    } else if (arm_is_secure(env)) {
+        return 3;
     }
-    /* We don't currently implement the Virtualization or TrustZone
-     * extensions, so PL2 and PL3 don't exist for us.
+    /* We don't currently implement the Virtualization extensions, so PL2 don't
+     * exist for us.
      */
     return 1;
 }
-- 
1.7.9.5

Re: [Qemu-devel] [RFC PATCH 08/21] target-arm: adjust arm_current_pl() for TrustZone

[Peter Crosthwaite]

On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com> wrote:
> Make arm_current_pl() to return PL3 in secure privileged mode.
>
> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
> ---
>  target-arm/cpu.h |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index a00c86f..1b03450 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -622,9 +622,11 @@ static inline int arm_current_pl(CPUARMState *env)
>  {
>      if ((env->uncached_cpsr & 0x1f) == ARM_CPU_MODE_USR) {
>          return 0;
> +    } else if (arm_is_secure(env)) {
> +        return 3;
>      }
> -    /* We don't currently implement the Virtualization or TrustZone
> -     * extensions, so PL2 and PL3 don't exist for us.
> +    /* We don't currently implement the Virtualization extensions, so PL2 don't

"doesn't". (second "don't" not first).

> +     * exist for us.
>       */
>      return 1;
>  }
> --
> 1.7.9.5
>
>

[Qemu-devel] [RFC PATCH 09/21] target-arm: adjust SCR CP15 register access rights

[Sergey Fedorov]

SCR register is accessible in PL3 (secure privileged) mode only. Fix SRC
access rights since arm_current_pl() can return PL3 now.

Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
---
 target-arm/helper.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index e406ec9..3bd0a64 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1530,7 +1530,7 @@ static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
 static const ARMCPRegInfo tz_cp_reginfo[] = {
 #ifndef CONFIG_USER_ONLY
     { .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0,
-      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr),
+      .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr),
       .resetvalue = 0 },
     { .name = "VBAR", .cp = 15, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
       .access = PL1_RW, .writefn = vbar_write,
-- 
1.7.9.5

[Qemu-devel] [RFC PATCH 10/21] target-arm: add non-secure Translation Block flag

[Sergey Fedorov]

This patch is based on idea found in patch at
git://github.com/jowinter/qemu-trustzone.git
f3d955c6c0ed8c46bc0eb10b634201032a651dd2 by
Johannes Winter <johannes.winter@iaik.tugraz.at>.

This flag prevents from executing TCG code generated for other CPU
secure state. It also allows to generate different TCG code depending on
CPU secure state.

Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
---
 target-arm/cpu.h       |    7 +++++++
 target-arm/translate.c |    3 +++
 target-arm/translate.h |    1 +
 3 files changed, 11 insertions(+)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 1b03450..f47dbdf 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -854,6 +854,8 @@ static inline int cpu_mmu_index (CPUARMState *env)
 #define ARM_TBFLAG_CONDEXEC_MASK    (0xff << ARM_TBFLAG_CONDEXEC_SHIFT)
 #define ARM_TBFLAG_BSWAP_CODE_SHIFT 16
 #define ARM_TBFLAG_BSWAP_CODE_MASK  (1 << ARM_TBFLAG_BSWAP_CODE_SHIFT)
+#define ARM_TBFLAG_NS_SHIFT         17
+#define ARM_TBFLAG_NS_MASK          (1 << ARM_TBFLAG_NS_SHIFT)
 
 /* Bit usage when in AArch64 state: currently no bits defined */
 
@@ -874,6 +876,8 @@ static inline int cpu_mmu_index (CPUARMState *env)
     (((F) & ARM_TBFLAG_CONDEXEC_MASK) >> ARM_TBFLAG_CONDEXEC_SHIFT)
 #define ARM_TBFLAG_BSWAP_CODE(F) \
     (((F) & ARM_TBFLAG_BSWAP_CODE_MASK) >> ARM_TBFLAG_BSWAP_CODE_SHIFT)
+#define ARM_TBFLAG_NS(F) \
+    (((F) & ARM_TBFLAG_NS_MASK) >> ARM_TBFLAG_NS_SHIFT)
 
 static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
                                         target_ulong *cs_base, int *flags)
@@ -897,6 +901,9 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
         if (privmode) {
             *flags |= ARM_TBFLAG_PRIV_MASK;
         }
+        if (!arm_is_secure(env)) {
+            *flags |= ARM_TBFLAG_NS_MASK;
+        }
         if (env->vfp.xregs[ARM_VFP_FPEXC] & (1 << 30)) {
             *flags |= ARM_TBFLAG_VFPEN_MASK;
         }
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 665c8ac..a47fcdb 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -52,8 +52,10 @@ static uint32_t gen_opc_condexec_bits[OPC_BUF_SIZE];
 
 #if defined(CONFIG_USER_ONLY)
 #define IS_USER(s) 1
+#define IS_NS(s) 1
 #else
 #define IS_USER(s) (s->user)
+#define IS_NS(s) (s->ns)
 #endif
 
 /* These instructions trap after executing, so defer them until after the
@@ -10036,6 +10038,7 @@ static inline void gen_intermediate_code_internal(ARMCPU *cpu,
         dc->condexec_cond = ARM_TBFLAG_CONDEXEC(tb->flags) >> 4;
 #if !defined(CONFIG_USER_ONLY)
         dc->user = (ARM_TBFLAG_PRIV(tb->flags) == 0);
+        dc->ns = ARM_TBFLAG_NS(tb->flags);
 #endif
         dc->vfp_enabled = ARM_TBFLAG_VFPEN(tb->flags);
         dc->vec_len = ARM_TBFLAG_VECLEN(tb->flags);
diff --git a/target-arm/translate.h b/target-arm/translate.h
index 67c7760..05b4f34 100644
--- a/target-arm/translate.h
+++ b/target-arm/translate.h
@@ -19,6 +19,7 @@ typedef struct DisasContext {
     int bswap_code;
 #if !defined(CONFIG_USER_ONLY)
     int user;
+    int ns;
 #endif
     int vfp_enabled;
     int vec_len;
-- 
1.7.9.5

[Qemu-devel] [RFC PATCH 11/21] target-arm: implement CPACR register logic

[Sergey Fedorov]

CPACR register allows to control access rights to coprocessor 0-13
interfaces. Bits corresponding to unimplemented coprocessors should be
RAZ/WI. QEMU implement only VFP coprocessor on ARMv6+ targets. So only
cp10 & cp11 bits is writable.

Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
---
 target-arm/helper.c    |    6 ++++++
 target-arm/translate.c |   36 +++++++++++++++++++++++++-----------
 2 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 3bd0a64..19f5830 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -418,6 +418,12 @@ static const ARMCPRegInfo not_v7_cp_reginfo[] = {
 
 static int cpacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 {
+    uint32_t mask = 0;
+
+    if (arm_feature(env, ARM_FEATURE_VFP)) {
+        mask |= 0x00f00000; /* VFP coprocessor: cp10 & cp11 */
+    }
+    value &= mask;
     if (env->cp15.c1_coproc != value) {
         env->cp15.c1_coproc = value;
         /* ??? Is this safe when called from within a TB?  */
diff --git a/target-arm/translate.c b/target-arm/translate.c
index a47fcdb..78612c8 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -6284,20 +6284,34 @@ static int disas_coproc_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
     ARMCPU *cpu = arm_env_get_cpu(env);
 
     cpnum = (insn >> 8) & 0xf;
-    if (arm_feature(env, ARM_FEATURE_XSCALE)
-	    && ((env->cp15.c15_cpar ^ 0x3fff) & (1 << cpnum)))
-	return 1;
+    if (cpnum < 14) {
+        if (arm_feature(env, ARM_FEATURE_XSCALE)) {
+            if (~env->cp15.c15_cpar & (1 << cpnum)) {
+                return 1;
+            }
+        } else {
+            switch ((env->cp15.c1_coproc >> (cpnum * 2)) & 3) {
+            case 0:
+            case 2:
+                return 1;
+            case 1:
+                if (IS_USER(s)) {
+                    return 1;
+                }
+            }
+        }
+    }
 
     /* First check for coprocessor space used for actual instructions */
     switch (cpnum) {
-      case 0:
-      case 1:
-	if (arm_feature(env, ARM_FEATURE_IWMMXT)) {
-	    return disas_iwmmxt_insn(env, s, insn);
-	} else if (arm_feature(env, ARM_FEATURE_XSCALE)) {
-	    return disas_dsp_insn(env, s, insn);
-	}
-	return 1;
+    case 0:
+    case 1:
+        if (arm_feature(env, ARM_FEATURE_IWMMXT)) {
+            return disas_iwmmxt_insn(env, s, insn);
+        } else if (arm_feature(env, ARM_FEATURE_XSCALE)) {
+            return disas_dsp_insn(env, s, insn);
+        }
+        return 1;
     case 10:
     case 11:
 	return disas_vfp_insn (env, s, insn);
-- 
1.7.9.5

[Qemu-devel] [RFC PATCH 12/21] target-arm: add NSACR support

[Sergey Fedorov]

NSACR allows to control non-secure access to coprocessor interfaces 0-13
and CPACR bits.

Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
---
 target-arm/cpu.h       |    1 +
 target-arm/helper.c    |   21 +++++++++++++++++++++
 target-arm/translate.c |    3 +++
 3 files changed, 25 insertions(+)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index f47dbdf..5aeb630 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -150,6 +150,7 @@ typedef struct CPUARMState {
         uint32_t c1_coproc; /* Coprocessor access register.  */
         uint32_t c1_xscaleauxcr; /* XScale auxiliary control register.  */
         uint32_t c1_scr; /* secure config register.  */
+        uint32_t c1_nsacr; /* Non-secure access control register. */
         uint32_t c2_base0; /* MMU translation table base 0.  */
         uint32_t c2_base0_hi; /* MMU translation table base 0, high 32 bits */
         uint32_t c2_base1; /* MMU translation table base 0.  */
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 19f5830..b6d8a3c 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -424,6 +424,17 @@ static int cpacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
         mask |= 0x00f00000; /* VFP coprocessor: cp10 & cp11 */
     }
     value &= mask;
+    if (!arm_is_secure(env)) {
+        int cpnum;
+        mask = 0;
+        for (cpnum = 0; cpnum < 14; ++cpnum) {
+            if (env->cp15.c1_nsacr & (1 << cpnum)) {
+                mask |= (3 << (cpnum * 2));
+            }
+        }
+        value &= mask;
+        value |= env->cp15.c1_coproc & ~mask;
+    }
     if (env->cp15.c1_coproc != value) {
         env->cp15.c1_coproc = value;
         /* ??? Is this safe when called from within a TB?  */
@@ -1531,6 +1542,13 @@ static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
     env->cp15.c12_vbar = value & ~0x1Ful;
     return 0;
 }
+
+static int nsacr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                      uint64_t value)
+{
+    env->cp15.c1_nsacr = value & 0x3fff;
+    return 0;
+}
 #endif
 
 static const ARMCPRegInfo tz_cp_reginfo[] = {
@@ -1542,6 +1560,9 @@ static const ARMCPRegInfo tz_cp_reginfo[] = {
       .access = PL1_RW, .writefn = vbar_write,
       .fieldoffset = offsetof(CPUARMState, cp15.c12_vbar),
       .resetvalue = 0 },
+    { .name = "NSACR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 2,
+      .access = PL3_RW | PL2_R, .resetvalue = 0, .writefn = nsacr_write,
+      .fieldoffset = offsetof(CPUARMState, cp15.c1_nsacr) },
 #endif
     REGINFO_SENTINEL
 };
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 78612c8..b3615fc 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -6290,6 +6290,9 @@ static int disas_coproc_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
                 return 1;
             }
         } else {
+            if (IS_NS(s) & ~env->cp15.c1_nsacr & (1 << cpnum)) {
+                return 1;
+            }
             switch ((env->cp15.c1_coproc >> (cpnum * 2)) & 3) {
             case 0:
             case 2:
-- 
1.7.9.5

[Qemu-devel] [RFC PATCH 13/21] target-arm: add SDER definition

[Sergey Fedorov]

Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
---
 target-arm/cpu.h    |    1 +
 target-arm/helper.c |    3 +++
 2 files changed, 4 insertions(+)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 5aeb630..ffc1b21 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -150,6 +150,7 @@ typedef struct CPUARMState {
         uint32_t c1_coproc; /* Coprocessor access register.  */
         uint32_t c1_xscaleauxcr; /* XScale auxiliary control register.  */
         uint32_t c1_scr; /* secure config register.  */
+        uint32_t c1_sder; /* Secure debug enable register. */
         uint32_t c1_nsacr; /* Non-secure access control register. */
         uint32_t c2_base0; /* MMU translation table base 0.  */
         uint32_t c2_base0_hi; /* MMU translation table base 0, high 32 bits */
diff --git a/target-arm/helper.c b/target-arm/helper.c
index b6d8a3c..780d0a0 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1560,6 +1560,9 @@ static const ARMCPRegInfo tz_cp_reginfo[] = {
       .access = PL1_RW, .writefn = vbar_write,
       .fieldoffset = offsetof(CPUARMState, cp15.c12_vbar),
       .resetvalue = 0 },
+    { .name = "SDER", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 1,
+      .access = PL3_RW, .resetvalue = 0,
+      .fieldoffset = offsetof(CPUARMState, cp15.c1_sder) },
     { .name = "NSACR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 2,
       .access = PL3_RW | PL2_R, .resetvalue = 0, .writefn = nsacr_write,
       .fieldoffset = offsetof(CPUARMState, cp15.c1_nsacr) },
-- 
1.7.9.5

[Qemu-devel] [RFC PATCH 14/21] target-arm: split TLB for secure state

[Sergey Fedorov]

This patch is based on idea found in patch at
git://github.com/jowinter/qemu-trustzone.git
a9ad01767c4b25e14700b5682a412f4fd8146ee8 by
Johannes Winter <johannes.winter@iaik.tugraz.at>.

Each secure state has its own MMU state. So provide a separate TLB for
each secure state to avoid flushing the whole TLB on each secure state
change. Do not use IS_USER() macro anymore as MMU index in translation
code. Use new MEM_INDEX() and MEM_INDEX_USER() macros instead.

Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
---
 target-arm/cpu.h       |   14 ++-
 target-arm/helper.c    |    2 +-
 target-arm/translate.c |  247 +++++++++++++++++++++++++-----------------------
 target-arm/translate.h |    1 +
 4 files changed, 140 insertions(+), 124 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index ffc1b21..a20f354 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -75,7 +75,7 @@ typedef uint32_t ARMReadCPFunc(void *opaque, int cp_info,
 
 struct arm_boot_info;
 
-#define NB_MMU_MODES 2
+#define NB_MMU_MODES 4
 
 /* We currently assume float and double are IEEE single and double
    precision respectively.
@@ -827,10 +827,18 @@ static inline CPUARMState *cpu_init(const char *cpu_model)
 /* MMU modes definitions */
 #define MMU_MODE0_SUFFIX _kernel
 #define MMU_MODE1_SUFFIX _user
-#define MMU_USER_IDX 1
+#define MMU_MODE2_SUFFIX _ns_kernel
+#define MMU_MODE3_SUFFIX _ns_user
+#define MMU_USER_BIT 1
+#define MMU_NS_BIT 2
+#define MMU_KERN_IDX (0)
+#define MMU_USER_IDX (MMU_USER_BIT)
+#define MMU_NS_KERN_IDX (MMU_NS_BIT | MMU_KERN_IDX)
+#define MMU_NS_USER_IDX (MMU_NS_BIT | MMU_USER_IDX)
 static inline int cpu_mmu_index (CPUARMState *env)
 {
-    return (env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_USR ? 1 : 0;
+    return (arm_is_secure(env) << 1) |
+        ((env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_USR);
 }
 
 #include "exec/cpu-all.h"
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 780d0a0..c145cfe 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3141,7 +3141,7 @@ int cpu_arm_handle_mmu_fault (CPUARMState *env, target_ulong address,
     int prot;
     int ret, is_user;
 
-    is_user = mmu_idx == MMU_USER_IDX;
+    is_user = mmu_idx & MMU_USER_BIT;
     ret = get_phys_addr(env, address, access_type, is_user, &phys_addr, &prot,
                         &page_size);
     if (ret == 0) {
diff --git a/target-arm/translate.c b/target-arm/translate.c
index b3615fc..8548a4c 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -53,9 +53,13 @@ static uint32_t gen_opc_condexec_bits[OPC_BUF_SIZE];
 #if defined(CONFIG_USER_ONLY)
 #define IS_USER(s) 1
 #define IS_NS(s) 1
+#define MEM_INDEX(s) MMU_USER_IDX
+#define MEM_INDEX_USER(S) MMU_USER_IDX
 #else
 #define IS_USER(s) (s->user)
 #define IS_NS(s) (s->ns)
+#define MEM_INDEX(s) (s->mem_idx)
+#define MEM_INDEX_USER(S) (MEM_INDEX(s) | MMU_USER_BIT)
 #endif
 
 /* These instructions trap after executing, so defer them until after the
@@ -1140,18 +1144,18 @@ VFP_GEN_FIX(ulto)
 static inline void gen_vfp_ld(DisasContext *s, int dp, TCGv_i32 addr)
 {
     if (dp) {
-        gen_aa32_ld64(cpu_F0d, addr, IS_USER(s));
+        gen_aa32_ld64(cpu_F0d, addr, MEM_INDEX(s));
     } else {
-        gen_aa32_ld32u(cpu_F0s, addr, IS_USER(s));
+        gen_aa32_ld32u(cpu_F0s, addr, MEM_INDEX(s));
     }
 }
 
 static inline void gen_vfp_st(DisasContext *s, int dp, TCGv_i32 addr)
 {
     if (dp) {
-        gen_aa32_st64(cpu_F0d, addr, IS_USER(s));
+        gen_aa32_st64(cpu_F0d, addr, MEM_INDEX(s));
     } else {
-        gen_aa32_st32(cpu_F0s, addr, IS_USER(s));
+        gen_aa32_st32(cpu_F0s, addr, MEM_INDEX(s));
     }
 }
 
@@ -1489,24 +1493,24 @@ static int disas_iwmmxt_insn(CPUARMState *env, DisasContext *s, uint32_t insn)
         if (insn & ARM_CP_RW_BIT) {
             if ((insn >> 28) == 0xf) {			/* WLDRW wCx */
                 tmp = tcg_temp_new_i32();
-                gen_aa32_ld32u(tmp, addr, IS_USER(s));
+                gen_aa32_ld32u(tmp, addr, MEM_INDEX(s));
                 iwmmxt_store_creg(wrd, tmp);
             } else {
                 i = 1;
                 if (insn & (1 << 8)) {
                     if (insn & (1 << 22)) {		/* WLDRD */
-                        gen_aa32_ld64(cpu_M0, addr, IS_USER(s));
+                        gen_aa32_ld64(cpu_M0, addr, MEM_INDEX(s));
                         i = 0;
                     } else {				/* WLDRW wRd */
                         tmp = tcg_temp_new_i32();
-                        gen_aa32_ld32u(tmp, addr, IS_USER(s));
+                        gen_aa32_ld32u(tmp, addr, MEM_INDEX(s));
                     }
                 } else {
                     tmp = tcg_temp_new_i32();
                     if (insn & (1 << 22)) {		/* WLDRH */
-                        gen_aa32_ld16u(tmp, addr, IS_USER(s));
+                        gen_aa32_ld16u(tmp, addr, MEM_INDEX(s));
                     } else {				/* WLDRB */
-                        gen_aa32_ld8u(tmp, addr, IS_USER(s));
+                        gen_aa32_ld8u(tmp, addr, MEM_INDEX(s));
                     }
                 }
                 if (i) {
@@ -1518,24 +1522,24 @@ static int disas_iwmmxt_insn(CPUARMState *env, DisasContext *s, uint32_t insn)
         } else {
             if ((insn >> 28) == 0xf) {			/* WSTRW wCx */
                 tmp = iwmmxt_load_creg(wrd);
-                gen_aa32_st32(tmp, addr, IS_USER(s));
+                gen_aa32_st32(tmp, addr, MEM_INDEX(s));
             } else {
                 gen_op_iwmmxt_movq_M0_wRn(wrd);
                 tmp = tcg_temp_new_i32();
                 if (insn & (1 << 8)) {
                     if (insn & (1 << 22)) {		/* WSTRD */
-                        gen_aa32_st64(cpu_M0, addr, IS_USER(s));
+                        gen_aa32_st64(cpu_M0, addr, MEM_INDEX(s));
                     } else {				/* WSTRW wRd */
                         tcg_gen_trunc_i64_i32(tmp, cpu_M0);
-                        gen_aa32_st32(tmp, addr, IS_USER(s));
+                        gen_aa32_st32(tmp, addr, MEM_INDEX(s));
                     }
                 } else {
                     if (insn & (1 << 22)) {		/* WSTRH */
                         tcg_gen_trunc_i64_i32(tmp, cpu_M0);
-                        gen_aa32_st16(tmp, addr, IS_USER(s));
+                        gen_aa32_st16(tmp, addr, MEM_INDEX(s));
                     } else {				/* WSTRB */
                         tcg_gen_trunc_i64_i32(tmp, cpu_M0);
-                        gen_aa32_st8(tmp, addr, IS_USER(s));
+                        gen_aa32_st8(tmp, addr, MEM_INDEX(s));
                     }
                 }
             }
@@ -2600,15 +2604,15 @@ static TCGv_i32 gen_load_and_replicate(DisasContext *s, TCGv_i32 addr, int size)
     TCGv_i32 tmp = tcg_temp_new_i32();
     switch (size) {
     case 0:
-        gen_aa32_ld8u(tmp, addr, IS_USER(s));
+        gen_aa32_ld8u(tmp, addr, MEM_INDEX(s));
         gen_neon_dup_u8(tmp, 0);
         break;
     case 1:
-        gen_aa32_ld16u(tmp, addr, IS_USER(s));
+        gen_aa32_ld16u(tmp, addr, MEM_INDEX(s));
         gen_neon_dup_low16(tmp);
         break;
     case 2:
-        gen_aa32_ld32u(tmp, addr, IS_USER(s));
+        gen_aa32_ld32u(tmp, addr, MEM_INDEX(s));
         break;
     default: /* Avoid compiler warnings.  */
         abort();
@@ -3886,11 +3890,11 @@ static int disas_neon_ls_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
             if (size == 3) {
                 tmp64 = tcg_temp_new_i64();
                 if (load) {
-                    gen_aa32_ld64(tmp64, addr, IS_USER(s));
+                    gen_aa32_ld64(tmp64, addr, MEM_INDEX(s));
                     neon_store_reg64(tmp64, rd);
                 } else {
                     neon_load_reg64(tmp64, rd);
-                    gen_aa32_st64(tmp64, addr, IS_USER(s));
+                    gen_aa32_st64(tmp64, addr, MEM_INDEX(s));
                 }
                 tcg_temp_free_i64(tmp64);
                 tcg_gen_addi_i32(addr, addr, stride);
@@ -3899,21 +3903,21 @@ static int disas_neon_ls_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
                     if (size == 2) {
                         if (load) {
                             tmp = tcg_temp_new_i32();
-                            gen_aa32_ld32u(tmp, addr, IS_USER(s));
+                            gen_aa32_ld32u(tmp, addr, MEM_INDEX(s));
                             neon_store_reg(rd, pass, tmp);
                         } else {
                             tmp = neon_load_reg(rd, pass);
-                            gen_aa32_st32(tmp, addr, IS_USER(s));
+                            gen_aa32_st32(tmp, addr, MEM_INDEX(s));
                             tcg_temp_free_i32(tmp);
                         }
                         tcg_gen_addi_i32(addr, addr, stride);
                     } else if (size == 1) {
                         if (load) {
                             tmp = tcg_temp_new_i32();
-                            gen_aa32_ld16u(tmp, addr, IS_USER(s));
+                            gen_aa32_ld16u(tmp, addr, MEM_INDEX(s));
                             tcg_gen_addi_i32(addr, addr, stride);
                             tmp2 = tcg_temp_new_i32();
-                            gen_aa32_ld16u(tmp2, addr, IS_USER(s));
+                            gen_aa32_ld16u(tmp2, addr, MEM_INDEX(s));
                             tcg_gen_addi_i32(addr, addr, stride);
                             tcg_gen_shli_i32(tmp2, tmp2, 16);
                             tcg_gen_or_i32(tmp, tmp, tmp2);
@@ -3923,10 +3927,10 @@ static int disas_neon_ls_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
                             tmp = neon_load_reg(rd, pass);
                             tmp2 = tcg_temp_new_i32();
                             tcg_gen_shri_i32(tmp2, tmp, 16);
-                            gen_aa32_st16(tmp, addr, IS_USER(s));
+                            gen_aa32_st16(tmp, addr, MEM_INDEX(s));
                             tcg_temp_free_i32(tmp);
                             tcg_gen_addi_i32(addr, addr, stride);
-                            gen_aa32_st16(tmp2, addr, IS_USER(s));
+                            gen_aa32_st16(tmp2, addr, MEM_INDEX(s));
                             tcg_temp_free_i32(tmp2);
                             tcg_gen_addi_i32(addr, addr, stride);
                         }
@@ -3935,7 +3939,7 @@ static int disas_neon_ls_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
                             TCGV_UNUSED_I32(tmp2);
                             for (n = 0; n < 4; n++) {
                                 tmp = tcg_temp_new_i32();
-                                gen_aa32_ld8u(tmp, addr, IS_USER(s));
+                                gen_aa32_ld8u(tmp, addr, MEM_INDEX(s));
                                 tcg_gen_addi_i32(addr, addr, stride);
                                 if (n == 0) {
                                     tmp2 = tmp;
@@ -3955,7 +3959,7 @@ static int disas_neon_ls_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
                                 } else {
                                     tcg_gen_shri_i32(tmp, tmp2, n * 8);
                                 }
-                                gen_aa32_st8(tmp, addr, IS_USER(s));
+                                gen_aa32_st8(tmp, addr, MEM_INDEX(s));
                                 tcg_temp_free_i32(tmp);
                                 tcg_gen_addi_i32(addr, addr, stride);
                             }
@@ -4079,13 +4083,13 @@ static int disas_neon_ls_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
                     tmp = tcg_temp_new_i32();
                     switch (size) {
                     case 0:
-                        gen_aa32_ld8u(tmp, addr, IS_USER(s));
+                        gen_aa32_ld8u(tmp, addr, MEM_INDEX(s));
                         break;
                     case 1:
-                        gen_aa32_ld16u(tmp, addr, IS_USER(s));
+                        gen_aa32_ld16u(tmp, addr, MEM_INDEX(s));
                         break;
                     case 2:
-                        gen_aa32_ld32u(tmp, addr, IS_USER(s));
+                        gen_aa32_ld32u(tmp, addr, MEM_INDEX(s));
                         break;
                     default: /* Avoid compiler warnings.  */
                         abort();
@@ -4103,13 +4107,13 @@ static int disas_neon_ls_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
                         tcg_gen_shri_i32(tmp, tmp, shift);
                     switch (size) {
                     case 0:
-                        gen_aa32_st8(tmp, addr, IS_USER(s));
+                        gen_aa32_st8(tmp, addr, MEM_INDEX(s));
                         break;
                     case 1:
-                        gen_aa32_st16(tmp, addr, IS_USER(s));
+                        gen_aa32_st16(tmp, addr, MEM_INDEX(s));
                         break;
                     case 2:
-                        gen_aa32_st32(tmp, addr, IS_USER(s));
+                        gen_aa32_st32(tmp, addr, MEM_INDEX(s));
                         break;
                     }
                     tcg_temp_free_i32(tmp);
@@ -6550,14 +6554,14 @@ static void gen_load_exclusive(DisasContext *s, int rt, int rt2,
 
     switch (size) {
     case 0:
-        gen_aa32_ld8u(tmp, addr, IS_USER(s));
+        gen_aa32_ld8u(tmp, addr, MEM_INDEX(s));
         break;
     case 1:
-        gen_aa32_ld16u(tmp, addr, IS_USER(s));
+        gen_aa32_ld16u(tmp, addr, MEM_INDEX(s));
         break;
     case 2:
     case 3:
-        gen_aa32_ld32u(tmp, addr, IS_USER(s));
+        gen_aa32_ld32u(tmp, addr, MEM_INDEX(s));
         break;
     default:
         abort();
@@ -6568,7 +6572,7 @@ static void gen_load_exclusive(DisasContext *s, int rt, int rt2,
         TCGv_i32 tmp2 = tcg_temp_new_i32();
         tcg_gen_addi_i32(tmp2, addr, 4);
         tmp = tcg_temp_new_i32();
-        gen_aa32_ld32u(tmp, tmp2, IS_USER(s));
+        gen_aa32_ld32u(tmp, tmp2, MEM_INDEX(s));
         tcg_temp_free_i32(tmp2);
         tcg_gen_mov_i32(cpu_exclusive_high, tmp);
         store_reg(s, rt2, tmp);
@@ -6610,14 +6614,14 @@ static void gen_store_exclusive(DisasContext *s, int rd, int rt, int rt2,
     tmp = tcg_temp_new_i32();
     switch (size) {
     case 0:
-        gen_aa32_ld8u(tmp, addr, IS_USER(s));
+        gen_aa32_ld8u(tmp, addr, MEM_INDEX(s));
         break;
     case 1:
-        gen_aa32_ld16u(tmp, addr, IS_USER(s));
+        gen_aa32_ld16u(tmp, addr, MEM_INDEX(s));
         break;
     case 2:
     case 3:
-        gen_aa32_ld32u(tmp, addr, IS_USER(s));
+        gen_aa32_ld32u(tmp, addr, MEM_INDEX(s));
         break;
     default:
         abort();
@@ -6628,7 +6632,7 @@ static void gen_store_exclusive(DisasContext *s, int rd, int rt, int rt2,
         TCGv_i32 tmp2 = tcg_temp_new_i32();
         tcg_gen_addi_i32(tmp2, addr, 4);
         tmp = tcg_temp_new_i32();
-        gen_aa32_ld32u(tmp, tmp2, IS_USER(s));
+        gen_aa32_ld32u(tmp, tmp2, MEM_INDEX(s));
         tcg_temp_free_i32(tmp2);
         tcg_gen_brcond_i32(TCG_COND_NE, tmp, cpu_exclusive_high, fail_label);
         tcg_temp_free_i32(tmp);
@@ -6636,14 +6640,14 @@ static void gen_store_exclusive(DisasContext *s, int rd, int rt, int rt2,
     tmp = load_reg(s, rt);
     switch (size) {
     case 0:
-        gen_aa32_st8(tmp, addr, IS_USER(s));
+        gen_aa32_st8(tmp, addr, MEM_INDEX(s));
         break;
     case 1:
-        gen_aa32_st16(tmp, addr, IS_USER(s));
+        gen_aa32_st16(tmp, addr, MEM_INDEX(s));
         break;
     case 2:
     case 3:
-        gen_aa32_st32(tmp, addr, IS_USER(s));
+        gen_aa32_st32(tmp, addr, MEM_INDEX(s));
         break;
     default:
         abort();
@@ -6652,7 +6656,7 @@ static void gen_store_exclusive(DisasContext *s, int rd, int rt, int rt2,
     if (size == 3) {
         tcg_gen_addi_i32(addr, addr, 4);
         tmp = load_reg(s, rt2);
-        gen_aa32_st32(tmp, addr, IS_USER(s));
+        gen_aa32_st32(tmp, addr, MEM_INDEX(s));
         tcg_temp_free_i32(tmp);
     }
     tcg_gen_movi_i32(cpu_R[rd], 0);
@@ -6699,11 +6703,11 @@ static void gen_srs(DisasContext *s,
     }
     tcg_gen_addi_i32(addr, addr, offset);
     tmp = load_reg(s, 14);
-    gen_aa32_st32(tmp, addr, 0);
+    gen_aa32_st32(tmp, addr, MEM_INDEX(s));
     tcg_temp_free_i32(tmp);
     tmp = load_cpu_field(spsr);
     tcg_gen_addi_i32(addr, addr, 4);
-    gen_aa32_st32(tmp, addr, 0);
+    gen_aa32_st32(tmp, addr, MEM_INDEX(s));
     tcg_temp_free_i32(tmp);
     if (writeback) {
         switch (amode) {
@@ -6849,10 +6853,10 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
                 tcg_gen_addi_i32(addr, addr, offset);
             /* Load PC into tmp and CPSR into tmp2.  */
             tmp = tcg_temp_new_i32();
-            gen_aa32_ld32u(tmp, addr, 0);
+            gen_aa32_ld32u(tmp, addr, MEM_INDEX(s));
             tcg_gen_addi_i32(addr, addr, 4);
             tmp2 = tcg_temp_new_i32();
-            gen_aa32_ld32u(tmp2, addr, 0);
+            gen_aa32_ld32u(tmp2, addr, MEM_INDEX(s));
             if (insn & (1 << 21)) {
                 /* Base writeback.  */
                 switch (i) {
@@ -7408,13 +7412,13 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
                                 tmp = tcg_temp_new_i32();
                                 switch (op1) {
                                 case 0: /* lda */
-                                    gen_aa32_ld32u(tmp, addr, IS_USER(s));
+                                    gen_aa32_ld32u(tmp, addr, MEM_INDEX(s));
                                     break;
                                 case 2: /* ldab */
-                                    gen_aa32_ld8u(tmp, addr, IS_USER(s));
+                                    gen_aa32_ld8u(tmp, addr, MEM_INDEX(s));
                                     break;
                                 case 3: /* ldah */
-                                    gen_aa32_ld16u(tmp, addr, IS_USER(s));
+                                    gen_aa32_ld16u(tmp, addr, MEM_INDEX(s));
                                     break;
                                 default:
                                     abort();
@@ -7425,13 +7429,13 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
                                 tmp = load_reg(s, rm);
                                 switch (op1) {
                                 case 0: /* stl */
-                                    gen_aa32_st32(tmp, addr, IS_USER(s));
+                                    gen_aa32_st32(tmp, addr, MEM_INDEX(s));
                                     break;
                                 case 2: /* stlb */
-                                    gen_aa32_st8(tmp, addr, IS_USER(s));
+                                    gen_aa32_st8(tmp, addr, MEM_INDEX(s));
                                     break;
                                 case 3: /* stlh */
-                                    gen_aa32_st16(tmp, addr, IS_USER(s));
+                                    gen_aa32_st16(tmp, addr, MEM_INDEX(s));
                                     break;
                                 default:
                                     abort();
@@ -7486,11 +7490,11 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
                         tmp = load_reg(s, rm);
                         tmp2 = tcg_temp_new_i32();
                         if (insn & (1 << 22)) {
-                            gen_aa32_ld8u(tmp2, addr, IS_USER(s));
-                            gen_aa32_st8(tmp, addr, IS_USER(s));
+                            gen_aa32_ld8u(tmp2, addr, MEM_INDEX(s));
+                            gen_aa32_st8(tmp, addr, MEM_INDEX(s));
                         } else {
-                            gen_aa32_ld32u(tmp2, addr, IS_USER(s));
-                            gen_aa32_st32(tmp, addr, IS_USER(s));
+                            gen_aa32_ld32u(tmp2, addr, MEM_INDEX(s));
+                            gen_aa32_st32(tmp, addr, MEM_INDEX(s));
                         }
                         tcg_temp_free_i32(tmp);
                         tcg_temp_free_i32(addr);
@@ -7512,14 +7516,14 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
                     tmp = tcg_temp_new_i32();
                     switch(sh) {
                     case 1:
-                        gen_aa32_ld16u(tmp, addr, IS_USER(s));
+                        gen_aa32_ld16u(tmp, addr, MEM_INDEX(s));
                         break;
                     case 2:
-                        gen_aa32_ld8s(tmp, addr, IS_USER(s));
+                        gen_aa32_ld8s(tmp, addr, MEM_INDEX(s));
                         break;
                     default:
                     case 3:
-                        gen_aa32_ld16s(tmp, addr, IS_USER(s));
+                        gen_aa32_ld16s(tmp, addr, MEM_INDEX(s));
                         break;
                     }
                     load = 1;
@@ -7529,21 +7533,21 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
                     if (sh & 1) {
                         /* store */
                         tmp = load_reg(s, rd);
-                        gen_aa32_st32(tmp, addr, IS_USER(s));
+                        gen_aa32_st32(tmp, addr, MEM_INDEX(s));
                         tcg_temp_free_i32(tmp);
                         tcg_gen_addi_i32(addr, addr, 4);
                         tmp = load_reg(s, rd + 1);
-                        gen_aa32_st32(tmp, addr, IS_USER(s));
+                        gen_aa32_st32(tmp, addr, MEM_INDEX(s));
                         tcg_temp_free_i32(tmp);
                         load = 0;
                     } else {
                         /* load */
                         tmp = tcg_temp_new_i32();
-                        gen_aa32_ld32u(tmp, addr, IS_USER(s));
+                        gen_aa32_ld32u(tmp, addr, MEM_INDEX(s));
                         store_reg(s, rd, tmp);
                         tcg_gen_addi_i32(addr, addr, 4);
                         tmp = tcg_temp_new_i32();
-                        gen_aa32_ld32u(tmp, addr, IS_USER(s));
+                        gen_aa32_ld32u(tmp, addr, MEM_INDEX(s));
                         rd++;
                         load = 1;
                     }
@@ -7551,7 +7555,7 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
                 } else {
                     /* store */
                     tmp = load_reg(s, rd);
-                    gen_aa32_st16(tmp, addr, IS_USER(s));
+                    gen_aa32_st16(tmp, addr, MEM_INDEX(s));
                     tcg_temp_free_i32(tmp);
                     load = 0;
                 }
@@ -7877,7 +7881,8 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
             rn = (insn >> 16) & 0xf;
             rd = (insn >> 12) & 0xf;
             tmp2 = load_reg(s, rn);
-            i = (IS_USER(s) || (insn & 0x01200000) == 0x00200000);
+            i = (insn & 0x01200000) == 0x00200000 ?
+                MEM_INDEX_USER(s) : MEM_INDEX(s);
             if (insn & (1 << 24))
                 gen_add_data_offset(s, insn, tmp2);
             if (insn & (1 << 20)) {
@@ -7961,7 +7966,7 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
                         if (insn & (1 << 20)) {
                             /* load */
                             tmp = tcg_temp_new_i32();
-                            gen_aa32_ld32u(tmp, addr, IS_USER(s));
+                            gen_aa32_ld32u(tmp, addr, MEM_INDEX(s));
                             if (user) {
                                 tmp2 = tcg_const_i32(i);
                                 gen_helper_set_user_reg(cpu_env, tmp2, tmp);
@@ -7988,7 +7993,7 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
                             } else {
                                 tmp = load_reg(s, i);
                             }
-                            gen_aa32_st32(tmp, addr, IS_USER(s));
+                            gen_aa32_st32(tmp, addr, MEM_INDEX(s));
                             tcg_temp_free_i32(tmp);
                         }
                         j++;
@@ -8247,20 +8252,20 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
                 if (insn & (1 << 20)) {
                     /* ldrd */
                     tmp = tcg_temp_new_i32();
-                    gen_aa32_ld32u(tmp, addr, IS_USER(s));
+                    gen_aa32_ld32u(tmp, addr, MEM_INDEX(s));
                     store_reg(s, rs, tmp);
                     tcg_gen_addi_i32(addr, addr, 4);
                     tmp = tcg_temp_new_i32();
-                    gen_aa32_ld32u(tmp, addr, IS_USER(s));
+                    gen_aa32_ld32u(tmp, addr, MEM_INDEX(s));
                     store_reg(s, rd, tmp);
                 } else {
                     /* strd */
                     tmp = load_reg(s, rs);
-                    gen_aa32_st32(tmp, addr, IS_USER(s));
+                    gen_aa32_st32(tmp, addr, MEM_INDEX(s));
                     tcg_temp_free_i32(tmp);
                     tcg_gen_addi_i32(addr, addr, 4);
                     tmp = load_reg(s, rd);
-                    gen_aa32_st32(tmp, addr, IS_USER(s));
+                    gen_aa32_st32(tmp, addr, MEM_INDEX(s));
                     tcg_temp_free_i32(tmp);
                 }
                 if (insn & (1 << 21)) {
@@ -8298,11 +8303,11 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
                     tcg_gen_add_i32(addr, addr, tmp);
                     tcg_temp_free_i32(tmp);
                     tmp = tcg_temp_new_i32();
-                    gen_aa32_ld16u(tmp, addr, IS_USER(s));
+                    gen_aa32_ld16u(tmp, addr, MEM_INDEX(s));
                 } else { /* tbb */
                     tcg_temp_free_i32(tmp);
                     tmp = tcg_temp_new_i32();
-                    gen_aa32_ld8u(tmp, addr, IS_USER(s));
+                    gen_aa32_ld8u(tmp, addr, MEM_INDEX(s));
                 }
                 tcg_temp_free_i32(addr);
                 tcg_gen_shli_i32(tmp, tmp, 1);
@@ -8339,13 +8344,13 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
                         tmp = tcg_temp_new_i32();
                         switch (op) {
                         case 0: /* ldab */
-                            gen_aa32_ld8u(tmp, addr, IS_USER(s));
+                            gen_aa32_ld8u(tmp, addr, MEM_INDEX(s));
                             break;
                         case 1: /* ldah */
-                            gen_aa32_ld16u(tmp, addr, IS_USER(s));
+                            gen_aa32_ld16u(tmp, addr, MEM_INDEX(s));
                             break;
                         case 2: /* lda */
-                            gen_aa32_ld32u(tmp, addr, IS_USER(s));
+                            gen_aa32_ld32u(tmp, addr, MEM_INDEX(s));
                             break;
                         default:
                             abort();
@@ -8355,13 +8360,13 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
                         tmp = load_reg(s, rs);
                         switch (op) {
                         case 0: /* stlb */
-                            gen_aa32_st8(tmp, addr, IS_USER(s));
+                            gen_aa32_st8(tmp, addr, MEM_INDEX(s));
                             break;
                         case 1: /* stlh */
-                            gen_aa32_st16(tmp, addr, IS_USER(s));
+                            gen_aa32_st16(tmp, addr, MEM_INDEX(s));
                             break;
                         case 2: /* stl */
-                            gen_aa32_st32(tmp, addr, IS_USER(s));
+                            gen_aa32_st32(tmp, addr, MEM_INDEX(s));
                             break;
                         default:
                             abort();
@@ -8389,10 +8394,10 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
                         tcg_gen_addi_i32(addr, addr, -8);
                     /* Load PC into tmp and CPSR into tmp2.  */
                     tmp = tcg_temp_new_i32();
-                    gen_aa32_ld32u(tmp, addr, 0);
+                    gen_aa32_ld32u(tmp, addr, MEM_INDEX(s));
                     tcg_gen_addi_i32(addr, addr, 4);
                     tmp2 = tcg_temp_new_i32();
-                    gen_aa32_ld32u(tmp2, addr, 0);
+                    gen_aa32_ld32u(tmp2, addr, MEM_INDEX(s));
                     if (insn & (1 << 21)) {
                         /* Base writeback.  */
                         if (insn & (1 << 24)) {
@@ -8431,7 +8436,7 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
                     if (insn & (1 << 20)) {
                         /* Load.  */
                         tmp = tcg_temp_new_i32();
-                        gen_aa32_ld32u(tmp, addr, IS_USER(s));
+                        gen_aa32_ld32u(tmp, addr, MEM_INDEX(s));
                         if (i == 15) {
                             gen_bx(s, tmp);
                         } else if (i == rn) {
@@ -8443,7 +8448,7 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
                     } else {
                         /* Store.  */
                         tmp = load_reg(s, i);
-                        gen_aa32_st32(tmp, addr, IS_USER(s));
+                        gen_aa32_st32(tmp, addr, MEM_INDEX(s));
                         tcg_temp_free_i32(tmp);
                     }
                     tcg_gen_addi_i32(addr, addr, 4);
@@ -9113,7 +9118,7 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
         {
         int postinc = 0;
         int writeback = 0;
-        int user;
+        int mem_idx;
         if ((insn & 0x01100000) == 0x01000000) {
             if (disas_neon_ls_insn(env, s, insn))
                 goto illegal_op;
@@ -9157,7 +9162,7 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
                 return 1;
             }
         }
-        user = IS_USER(s);
+        mem_idx = MEM_INDEX(s);
         if (rn == 15) {
             addr = tcg_temp_new_i32();
             /* PC relative.  */
@@ -9194,7 +9199,7 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
                     break;
                 case 0xe: /* User privilege.  */
                     tcg_gen_addi_i32(addr, addr, imm);
-                    user = 1;
+                    mem_idx = MEM_INDEX_USER(s);
                     break;
                 case 0x9: /* Post-decrement.  */
                     imm = -imm;
@@ -9221,19 +9226,19 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
             tmp = tcg_temp_new_i32();
             switch (op) {
             case 0:
-                gen_aa32_ld8u(tmp, addr, user);
+                gen_aa32_ld8u(tmp, addr, mem_idx);
                 break;
             case 4:
-                gen_aa32_ld8s(tmp, addr, user);
+                gen_aa32_ld8s(tmp, addr, mem_idx);
                 break;
             case 1:
-                gen_aa32_ld16u(tmp, addr, user);
+                gen_aa32_ld16u(tmp, addr, mem_idx);
                 break;
             case 5:
-                gen_aa32_ld16s(tmp, addr, user);
+                gen_aa32_ld16s(tmp, addr, mem_idx);
                 break;
             case 2:
-                gen_aa32_ld32u(tmp, addr, user);
+                gen_aa32_ld32u(tmp, addr, mem_idx);
                 break;
             default:
                 tcg_temp_free_i32(tmp);
@@ -9250,13 +9255,13 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
             tmp = load_reg(s, rs);
             switch (op) {
             case 0:
-                gen_aa32_st8(tmp, addr, user);
+                gen_aa32_st8(tmp, addr, mem_idx);
                 break;
             case 1:
-                gen_aa32_st16(tmp, addr, user);
+                gen_aa32_st16(tmp, addr, mem_idx);
                 break;
             case 2:
-                gen_aa32_st32(tmp, addr, user);
+                gen_aa32_st32(tmp, addr, mem_idx);
                 break;
             default:
                 tcg_temp_free_i32(tmp);
@@ -9393,7 +9398,7 @@ static void disas_thumb_insn(CPUARMState *env, DisasContext *s)
             addr = tcg_temp_new_i32();
             tcg_gen_movi_i32(addr, val);
             tmp = tcg_temp_new_i32();
-            gen_aa32_ld32u(tmp, addr, IS_USER(s));
+            gen_aa32_ld32u(tmp, addr, MEM_INDEX(s));
             tcg_temp_free_i32(addr);
             store_reg(s, rd, tmp);
             break;
@@ -9596,28 +9601,28 @@ static void disas_thumb_insn(CPUARMState *env, DisasContext *s)
 
         switch (op) {
         case 0: /* str */
-            gen_aa32_st32(tmp, addr, IS_USER(s));
+            gen_aa32_st32(tmp, addr, MEM_INDEX(s));
             break;
         case 1: /* strh */
-            gen_aa32_st16(tmp, addr, IS_USER(s));
+            gen_aa32_st16(tmp, addr, MEM_INDEX(s));
             break;
         case 2: /* strb */
-            gen_aa32_st8(tmp, addr, IS_USER(s));
+            gen_aa32_st8(tmp, addr, MEM_INDEX(s));
             break;
         case 3: /* ldrsb */
-            gen_aa32_ld8s(tmp, addr, IS_USER(s));
+            gen_aa32_ld8s(tmp, addr, MEM_INDEX(s));
             break;
         case 4: /* ldr */
-            gen_aa32_ld32u(tmp, addr, IS_USER(s));
+            gen_aa32_ld32u(tmp, addr, MEM_INDEX(s));
             break;
         case 5: /* ldrh */
-            gen_aa32_ld16u(tmp, addr, IS_USER(s));
+            gen_aa32_ld16u(tmp, addr, MEM_INDEX(s));
             break;
         case 6: /* ldrb */
-            gen_aa32_ld8u(tmp, addr, IS_USER(s));
+            gen_aa32_ld8u(tmp, addr, MEM_INDEX(s));
             break;
         case 7: /* ldrsh */
-            gen_aa32_ld16s(tmp, addr, IS_USER(s));
+            gen_aa32_ld16s(tmp, addr, MEM_INDEX(s));
             break;
         }
         if (op >= 3) { /* load */
@@ -9639,12 +9644,12 @@ static void disas_thumb_insn(CPUARMState *env, DisasContext *s)
         if (insn & (1 << 11)) {
             /* load */
             tmp = tcg_temp_new_i32();
-            gen_aa32_ld32u(tmp, addr, IS_USER(s));
+            gen_aa32_ld32u(tmp, addr, MEM_INDEX(s));
             store_reg(s, rd, tmp);
         } else {
             /* store */
             tmp = load_reg(s, rd);
-            gen_aa32_st32(tmp, addr, IS_USER(s));
+            gen_aa32_st32(tmp, addr, MEM_INDEX(s));
             tcg_temp_free_i32(tmp);
         }
         tcg_temp_free_i32(addr);
@@ -9661,12 +9666,12 @@ static void disas_thumb_insn(CPUARMState *env, DisasContext *s)
         if (insn & (1 << 11)) {
             /* load */
             tmp = tcg_temp_new_i32();
-            gen_aa32_ld8u(tmp, addr, IS_USER(s));
+            gen_aa32_ld8u(tmp, addr, MEM_INDEX(s));
             store_reg(s, rd, tmp);
         } else {
             /* store */
             tmp = load_reg(s, rd);
-            gen_aa32_st8(tmp, addr, IS_USER(s));
+            gen_aa32_st8(tmp, addr, MEM_INDEX(s));
             tcg_temp_free_i32(tmp);
         }
         tcg_temp_free_i32(addr);
@@ -9683,12 +9688,12 @@ static void disas_thumb_insn(CPUARMState *env, DisasContext *s)
         if (insn & (1 << 11)) {
             /* load */
             tmp = tcg_temp_new_i32();
-            gen_aa32_ld16u(tmp, addr, IS_USER(s));
+            gen_aa32_ld16u(tmp, addr, MEM_INDEX(s));
             store_reg(s, rd, tmp);
         } else {
             /* store */
             tmp = load_reg(s, rd);
-            gen_aa32_st16(tmp, addr, IS_USER(s));
+            gen_aa32_st16(tmp, addr, MEM_INDEX(s));
             tcg_temp_free_i32(tmp);
         }
         tcg_temp_free_i32(addr);
@@ -9704,12 +9709,12 @@ static void disas_thumb_insn(CPUARMState *env, DisasContext *s)
         if (insn & (1 << 11)) {
             /* load */
             tmp = tcg_temp_new_i32();
-            gen_aa32_ld32u(tmp, addr, IS_USER(s));
+            gen_aa32_ld32u(tmp, addr, MEM_INDEX(s));
             store_reg(s, rd, tmp);
         } else {
             /* store */
             tmp = load_reg(s, rd);
-            gen_aa32_st32(tmp, addr, IS_USER(s));
+            gen_aa32_st32(tmp, addr, MEM_INDEX(s));
             tcg_temp_free_i32(tmp);
         }
         tcg_temp_free_i32(addr);
@@ -9777,12 +9782,12 @@ static void disas_thumb_insn(CPUARMState *env, DisasContext *s)
                     if (insn & (1 << 11)) {
                         /* pop */
                         tmp = tcg_temp_new_i32();
-                        gen_aa32_ld32u(tmp, addr, IS_USER(s));
+                        gen_aa32_ld32u(tmp, addr, MEM_INDEX(s));
                         store_reg(s, i, tmp);
                     } else {
                         /* push */
                         tmp = load_reg(s, i);
-                        gen_aa32_st32(tmp, addr, IS_USER(s));
+                        gen_aa32_st32(tmp, addr, MEM_INDEX(s));
                         tcg_temp_free_i32(tmp);
                     }
                     /* advance to the next address.  */
@@ -9794,13 +9799,13 @@ static void disas_thumb_insn(CPUARMState *env, DisasContext *s)
                 if (insn & (1 << 11)) {
                     /* pop pc */
                     tmp = tcg_temp_new_i32();
-                    gen_aa32_ld32u(tmp, addr, IS_USER(s));
+                    gen_aa32_ld32u(tmp, addr, MEM_INDEX(s));
                     /* don't set the pc until the rest of the instruction
                        has completed */
                 } else {
                     /* push lr */
                     tmp = load_reg(s, 14);
-                    gen_aa32_st32(tmp, addr, IS_USER(s));
+                    gen_aa32_st32(tmp, addr, MEM_INDEX(s));
                     tcg_temp_free_i32(tmp);
                 }
                 tcg_gen_addi_i32(addr, addr, 4);
@@ -9926,7 +9931,7 @@ static void disas_thumb_insn(CPUARMState *env, DisasContext *s)
                 if (insn & (1 << 11)) {
                     /* load */
                     tmp = tcg_temp_new_i32();
-                    gen_aa32_ld32u(tmp, addr, IS_USER(s));
+                    gen_aa32_ld32u(tmp, addr, MEM_INDEX(s));
                     if (i == rn) {
                         loaded_var = tmp;
                     } else {
@@ -9935,7 +9940,7 @@ static void disas_thumb_insn(CPUARMState *env, DisasContext *s)
                 } else {
                     /* store */
                     tmp = load_reg(s, i);
-                    gen_aa32_st32(tmp, addr, IS_USER(s));
+                    gen_aa32_st32(tmp, addr, MEM_INDEX(s));
                     tcg_temp_free_i32(tmp);
                 }
                 /* advance to the next address */
@@ -10056,6 +10061,8 @@ static inline void gen_intermediate_code_internal(ARMCPU *cpu,
 #if !defined(CONFIG_USER_ONLY)
         dc->user = (ARM_TBFLAG_PRIV(tb->flags) == 0);
         dc->ns = ARM_TBFLAG_NS(tb->flags);
+        dc->mem_idx = (IS_USER(dc) ? MMU_USER_BIT : 0) |
+            (IS_NS(dc) ? MMU_NS_BIT : 0);
 #endif
         dc->vfp_enabled = ARM_TBFLAG_VFPEN(tb->flags);
         dc->vec_len = ARM_TBFLAG_VECLEN(tb->flags);
diff --git a/target-arm/translate.h b/target-arm/translate.h
index 05b4f34..be974e3 100644
--- a/target-arm/translate.h
+++ b/target-arm/translate.h
@@ -20,6 +20,7 @@ typedef struct DisasContext {
 #if !defined(CONFIG_USER_ONLY)
     int user;
     int ns;
+    int mem_idx;
 #endif
     int vfp_enabled;
     int vec_len;
-- 
1.7.9.5

[Qemu-devel] [RFC PATCH 15/21] target-arm: add banked coprocessor register type

[Sergey Fedorov]

Banked CP registers are defined with BANKED_CP_REG macro which defines
an active register state field followed by an adjacent banked register
state array field. An active CP register state is accessed as usual
through an active state field. Banked CP register state is save in
banked register state array. The array is indexed by NS bit value.

When translating a banked CP register access instruction in secure
state, SCR.NS bit determines which field should be used. If SCR.NS bit
is set then a non-secure banked value is used instead of an active one.
That is possible only in monitor CPU mode. In non-secure state an active
register state field is always used.

Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
---
 target-arm/cpu.h       |   14 ++++++++++-
 target-arm/translate.c |   60 ++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 69 insertions(+), 5 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index a20f354..fe3a646 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -73,6 +73,17 @@ typedef void ARMWriteCPFunc(void *opaque, int cp_info,
 typedef uint32_t ARMReadCPFunc(void *opaque, int cp_info,
                                int dstreg, int operand);
 
+/* Define a banked coprocessor register state field. Use %name as the active
+ * register state field name. The banked register state array field name has
+ * "banked_" prefix. The banked register state array indexes corresponds to
+ * SCR.NS bit value.
+ */
+#define BANKED_CP_REG(type, name) \
+    struct { \
+        type name; \
+        type banked_##name[2]; \
+    }
+
 struct arm_boot_info;
 
 #define NB_MMU_MODES 4
@@ -572,13 +583,14 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t cpregid)
 #define ARM_CP_OVERRIDE 16
 #define ARM_CP_NO_MIGRATE 32
 #define ARM_CP_IO 64
+#define ARM_CP_BANKED 128
 #define ARM_CP_NOP (ARM_CP_SPECIAL | (1 << 8))
 #define ARM_CP_WFI (ARM_CP_SPECIAL | (2 << 8))
 #define ARM_LAST_SPECIAL ARM_CP_WFI
 /* Used only as a terminator for ARMCPRegInfo lists */
 #define ARM_CP_SENTINEL 0xffff
 /* Mask of only the flag bits in a type field */
-#define ARM_CP_FLAG_MASK 0x7f
+#define ARM_CP_FLAG_MASK 0xff
 
 /* Return true if cptype is a valid type field. This is used to try to
  * catch errors where the sentinel has been accidentally left off the end
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 8548a4c..345866c 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -6389,9 +6389,22 @@ static int disas_coproc_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
                     tmpptr = tcg_const_ptr(ri);
                     gen_helper_get_cp_reg64(tmp64, cpu_env, tmpptr);
                     tcg_temp_free_ptr(tmpptr);
-                } else {
+                } else if (!(ri->type & ARM_CP_BANKED) || IS_NS(s)) {
                     tmp64 = tcg_temp_new_i64();
                     tcg_gen_ld_i64(tmp64, cpu_env, ri->fieldoffset);
+                } else {
+                    TCGv_ptr tmpptr;
+                    tmp = load_cpu_field(cp15.c1_scr);
+                    tcg_gen_andi_i32(tmp, tmp, 1);
+                    tcg_gen_shli_i32(tmp, tmp, 4);
+                    tmpptr = tcg_temp_new_ptr();
+                    tcg_gen_ext_i32_ptr(tmpptr, cpu_env);
+                    tcg_gen_addi_ptr(tmpptr, cpu_env, ri->fieldoffset);
+                    tcg_gen_add_ptr(tmpptr, tmpptr, tmp);
+                    tcg_temp_free_i32(tmp);
+                    tmp64 = tcg_temp_new_i64();
+                    tcg_gen_ld_i64(tmp64, tmpptr, 0);
+                    tcg_temp_free_ptr(tmpptr);
                 }
                 tmp = tcg_temp_new_i32();
                 tcg_gen_trunc_i64_i32(tmp, tmp64);
@@ -6412,8 +6425,19 @@ static int disas_coproc_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
                     tmpptr = tcg_const_ptr(ri);
                     gen_helper_get_cp_reg(tmp, cpu_env, tmpptr);
                     tcg_temp_free_ptr(tmpptr);
-                } else {
+                } else if (!(ri->type & ARM_CP_BANKED) || IS_NS(s)) {
                     tmp = load_cpu_offset(ri->fieldoffset);
+                } else {
+                    TCGv_ptr tmpptr;
+                    tmp = load_cpu_field(cp15.c1_scr);
+                    tcg_gen_andi_i32(tmp, tmp, 1);
+                    tcg_gen_shli_i32(tmp, tmp, 2);
+                    tmpptr = tcg_temp_new_ptr();
+                    tcg_gen_ext_i32_ptr(tmpptr, cpu_env);
+                    tcg_gen_addi_ptr(tmpptr, cpu_env, ri->fieldoffset);
+                    tcg_gen_add_ptr(tmpptr, tmpptr, tmp);
+                    tcg_gen_ld_i32(tmp, tmpptr, 0);
+                    tcg_temp_free_ptr(tmpptr);
                 }
                 if (rt == 15) {
                     /* Destination register of r15 for 32 bit loads sets
@@ -6445,8 +6469,22 @@ static int disas_coproc_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
                     gen_set_pc_im(s, s->pc);
                     gen_helper_set_cp_reg64(cpu_env, tmpptr, tmp64);
                     tcg_temp_free_ptr(tmpptr);
-                } else {
+                } else if (!(ri->type & ARM_CP_BANKED) || IS_NS(s)) {
                     tcg_gen_st_i64(tmp64, cpu_env, ri->fieldoffset);
+                } else {
+                    TCGv_i32 tmp;
+                    TCGv_ptr tmpptr;
+                    tmp = load_cpu_field(cp15.c1_scr);
+                    tcg_gen_andi_i32(tmp, tmp, 1);
+                    tcg_gen_shli_i32(tmp, tmp, 4);
+                    tmpptr = tcg_temp_new_ptr();
+                    tcg_gen_ext_i32_ptr(tmpptr, cpu_env);
+                    tcg_gen_addi_ptr(tmpptr, cpu_env, ri->fieldoffset);
+                    tcg_gen_add_ptr(tmpptr, tmpptr, tmp);
+                    tcg_temp_free_i32(tmp);
+                    tmp64 = tcg_temp_new_i64();
+                    tcg_gen_st_i64(tmp64, tmpptr, 0);
+                    tcg_temp_free_ptr(tmpptr);
                 }
                 tcg_temp_free_i64(tmp64);
             } else {
@@ -6459,9 +6497,23 @@ static int disas_coproc_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
                     gen_helper_set_cp_reg(cpu_env, tmpptr, tmp);
                     tcg_temp_free_ptr(tmpptr);
                     tcg_temp_free_i32(tmp);
-                } else {
+                } else if (!(ri->type & ARM_CP_BANKED) || IS_NS(s)) {
                     TCGv_i32 tmp = load_reg(s, rt);
                     store_cpu_offset(tmp, ri->fieldoffset);
+                } else {
+                    TCGv_i32 tmp;
+                    TCGv_ptr tmpptr;
+                    tmp = load_cpu_field(cp15.c1_scr);
+                    tcg_gen_andi_i32(tmp, tmp, 1);
+                    tcg_gen_shli_i32(tmp, tmp, 2);
+                    tmpptr = tcg_temp_new_ptr();
+                    tcg_gen_ext_i32_ptr(tmpptr, cpu_env);
+                    tcg_gen_addi_ptr(tmpptr, cpu_env, ri->fieldoffset);
+                    tcg_gen_add_ptr(tmpptr, tmpptr, tmp);
+                    load_reg_var(s, tmp, rt);
+                    tcg_gen_st_i32(tmp, tmpptr, 0);
+                    tcg_temp_free_ptr(tmpptr);
+                    tcg_temp_free_i32(tmp);
                 }
             }
         }
-- 
1.7.9.5

[Qemu-devel] [RFC PATCH 16/21] target-arm: convert appropriate coprocessor registers to banked type

[Sergey Fedorov]

Define appropriate coprocessor registers with banked type. The register
state fields are defined with BANKED_CP_REG() macro.

Banked coprocessor registers with a special behaviour is adjusted to
correctly use its active or banked non-secure state.

Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
---
 target-arm/cpu.c    |    2 +-
 target-arm/cpu.h    |   62 +++++++++++++++++---------
 target-arm/helper.c |  120 +++++++++++++++++++++++++++++----------------------
 3 files changed, 111 insertions(+), 73 deletions(-)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 4607ca8..3bb3deb 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -356,7 +356,7 @@ static void arm1026_initfn(Object *obj)
         /* The 1026 had an IFAR at c6,c0,0,1 rather than the ARMv6 c6,c0,0,2 */
         ARMCPRegInfo ifar = {
             .name = "IFAR", .cp = 15, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 1,
-            .access = PL1_RW,
+            .access = PL1_RW, .type = ARM_CP_BANKED,
             .fieldoffset = offsetof(CPUARMState, cp15.c6_insn),
             .resetvalue = 0
         };
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index fe3a646..b4500b4 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -84,6 +84,26 @@ typedef uint32_t ARMReadCPFunc(void *opaque, int cp_info,
         type banked_##name[2]; \
     }
 
+/* Whether a co-processor access instruction should use a banked Non-secure
+ * copy of a register instead of active one */
+#define USE_NS_REG(env) ( \
+        unlikely( \
+            ((env)->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_MON && \
+            ((env)->cp15.c1_scr & 1/*NS*/)))
+/* Get a banked CP15 register in co-processor access instruction handler */
+#define BANKED_CP15_REG_GET(env, regname) \
+    (!USE_NS_REG(env) ? (env)->cp15.regname : \
+     (env)->cp15.banked_##regname[1])
+/* Set a banked CP15 register in co-processor access instruction handler */
+#define BANKED_CP15_REG_SET(env, regname, val) \
+    do { \
+        if (!USE_NS_REG(env)) { \
+            (env)->cp15.regname = (val); \
+        } else { \
+            (env)->cp15.banked_##regname[1] = (val); \
+        } \
+    } while (0)
+
 struct arm_boot_info;
 
 #define NB_MMU_MODES 4
@@ -156,31 +176,31 @@ typedef struct CPUARMState {
     /* System control coprocessor (cp15) */
     struct {
         uint32_t c0_cpuid;
-        uint32_t c0_cssel; /* Cache size selection.  */
-        uint32_t c1_sys; /* System control register.  */
+        BANKED_CP_REG(uint32_t, c0_cssel); /* Cache size selection.  */
+        BANKED_CP_REG(uint32_t, c1_sys); /* System control register.  */
         uint32_t c1_coproc; /* Coprocessor access register.  */
         uint32_t c1_xscaleauxcr; /* XScale auxiliary control register.  */
         uint32_t c1_scr; /* secure config register.  */
         uint32_t c1_sder; /* Secure debug enable register. */
         uint32_t c1_nsacr; /* Non-secure access control register. */
-        uint32_t c2_base0; /* MMU translation table base 0.  */
-        uint32_t c2_base0_hi; /* MMU translation table base 0, high 32 bits */
-        uint32_t c2_base1; /* MMU translation table base 0.  */
-        uint32_t c2_base1_hi; /* MMU translation table base 1, high 32 bits */
-        uint32_t c2_control; /* MMU translation table base control.  */
+        BANKED_CP_REG(uint32_t, c2_base0); /* MMU translation table base 0.  */
+        BANKED_CP_REG(uint32_t, c2_base0_hi); /* MMU translation table base 0, high 32 bits */
+        BANKED_CP_REG(uint32_t, c2_base1); /* MMU translation table base 0.  */
+        BANKED_CP_REG(uint32_t, c2_base1_hi); /* MMU translation table base 1, high 32 bits */
+        BANKED_CP_REG(uint32_t, c2_control); /* MMU translation table base control.  */
         uint32_t c2_mask; /* MMU translation table base selection mask.  */
         uint32_t c2_base_mask; /* MMU translation table base 0 mask. */
         uint32_t c2_data; /* MPU data cachable bits.  */
         uint32_t c2_insn; /* MPU instruction cachable bits.  */
-        uint32_t c3; /* MMU domain access control register
-                        MPU write buffer control.  */
-        uint32_t c5_insn; /* Fault status registers.  */
-        uint32_t c5_data;
+        BANKED_CP_REG(uint32_t, c3); /* MMU domain access control register
+                                        MPU write buffer control.  */
+        BANKED_CP_REG(uint32_t, c5_insn); /* Fault status registers.  */
+        BANKED_CP_REG(uint32_t, c5_data);
         uint32_t c6_region[8]; /* MPU base/size registers.  */
-        uint32_t c6_insn; /* Fault address registers.  */
-        uint32_t c6_data;
-        uint32_t c7_par;  /* Translation result. */
-        uint32_t c7_par_hi;  /* Translation result, high 32 bits */
+        BANKED_CP_REG(uint32_t, c6_insn); /* Fault address registers.  */
+        BANKED_CP_REG(uint32_t, c6_data);
+        BANKED_CP_REG(uint32_t, c7_par);  /* Translation result. */
+        BANKED_CP_REG(uint32_t, c7_par_hi);  /* Translation result, high 32 bits */
         uint32_t c9_insn; /* Cache lockdown registers.  */
         uint32_t c9_data;
         uint32_t c9_pmcr; /* performance monitor control register */
@@ -189,12 +209,12 @@ typedef struct CPUARMState {
         uint32_t c9_pmxevtyper; /* perf monitor event type */
         uint32_t c9_pmuserenr; /* perf monitor user enable */
         uint32_t c9_pminten; /* perf monitor interrupt enables */
-        uint32_t c12_vbar; /* vector base address register */
-        uint32_t c13_fcse; /* FCSE PID.  */
-        uint32_t c13_context; /* Context ID.  */
-        uint32_t c13_tls1; /* User RW Thread register.  */
-        uint32_t c13_tls2; /* User RO Thread register.  */
-        uint32_t c13_tls3; /* Privileged Thread register.  */
+        BANKED_CP_REG(uint32_t, c12_vbar); /* vector base address register */
+        BANKED_CP_REG(uint32_t, c13_fcse); /* FCSE PID.  */
+        BANKED_CP_REG(uint32_t, c13_context); /* Context ID.  */
+        BANKED_CP_REG(uint32_t, c13_tls1); /* User RW Thread register.  */
+        BANKED_CP_REG(uint32_t, c13_tls2); /* User RO Thread register.  */
+        BANKED_CP_REG(uint32_t, c13_tls3); /* Privileged Thread register.  */
         uint32_t c14_cntfrq; /* Counter Frequency register */
         uint32_t c14_cntkctl; /* Timer Control register */
         ARMGenericTimer c14_timer[NUM_GTIMERS];
diff --git a/target-arm/helper.c b/target-arm/helper.c
index c145cfe..9442e08 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -277,33 +277,34 @@ void init_cpreg_list(ARMCPU *cpu)
 
 static int dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 {
-    env->cp15.c3 = value;
+    BANKED_CP15_REG_SET(env, c3, value);
     tlb_flush(env, 1); /* Flush TLB as domain not tracked in TLB */
     return 0;
 }
 
 static int fcse_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 {
-    if (env->cp15.c13_fcse != value) {
+    if (BANKED_CP15_REG_GET(env, c13_fcse) != value) {
         /* Unlike real hardware the qemu TLB uses virtual addresses,
          * not modified virtual addresses, so this causes a TLB flush.
          */
         tlb_flush(env, 1);
-        env->cp15.c13_fcse = value;
+        BANKED_CP15_REG_SET(env, c13_fcse, value);
     }
     return 0;
 }
 static int contextidr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                             uint64_t value)
 {
-    if (env->cp15.c13_context != value && !arm_feature(env, ARM_FEATURE_MPU)) {
+    if (BANKED_CP15_REG_GET(env, c13_context) != value &&
+            !arm_feature(env, ARM_FEATURE_MPU)) {
         /* For VMSA (when not using the LPAE long descriptor page table
          * format) this register includes the ASID, so do a TLB flush.
          * For PMSA it is purely a process ID and no action is needed.
          */
         tlb_flush(env, 1);
     }
-    env->cp15.c13_context = value;
+    BANKED_CP15_REG_SET(env, c13_context, value);
     return 0;
 }
 
@@ -349,13 +350,16 @@ static const ARMCPRegInfo cp_reginfo[] = {
     /* MMU Domain access control / MPU write buffer control */
     { .name = "DACR", .cp = 15,
       .crn = 3, .crm = CP_ANY, .opc1 = CP_ANY, .opc2 = CP_ANY,
-      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c3),
+      .access = PL1_RW, .type = ARM_CP_BANKED,
+      .fieldoffset = offsetof(CPUARMState, cp15.c3),
       .resetvalue = 0, .writefn = dacr_write, .raw_writefn = raw_write, },
     { .name = "FCSEIDR", .cp = 15, .crn = 13, .crm = 0, .opc1 = 0, .opc2 = 0,
-      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c13_fcse),
+      .access = PL1_RW, .type = ARM_CP_BANKED,
+      .fieldoffset = offsetof(CPUARMState, cp15.c13_fcse),
       .resetvalue = 0, .writefn = fcse_write, .raw_writefn = raw_write, },
     { .name = "CONTEXTIDR", .cp = 15, .crn = 13, .crm = 0, .opc1 = 0, .opc2 = 1,
-      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c13_fcse),
+      .access = PL1_RW, .type = ARM_CP_BANKED,
+      .fieldoffset = offsetof(CPUARMState, cp15.c13_fcse),
       .resetvalue = 0, .writefn = contextidr_write, .raw_writefn = raw_write, },
     /* ??? This covers not just the impdef TLB lockdown registers but also
      * some v7VMSA registers relating to TEX remap, so it is overly broad.
@@ -455,7 +459,8 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
     { .name = "DMB", .cp = 15, .crn = 7, .crm = 10, .opc1 = 0, .opc2 = 5,
       .access = PL0_W, .type = ARM_CP_NOP },
     { .name = "IFAR", .cp = 15, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 2,
-      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c6_insn),
+      .access = PL1_RW, .type = ARM_CP_BANKED,
+      .fieldoffset = offsetof(CPUARMState, cp15.c6_insn),
       .resetvalue = 0, },
     /* Watchpoint Fault Address Register : should actually only be present
      * for 1136, 1176, 11MPCore.
@@ -564,7 +569,7 @@ static int ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri,
                        uint64_t *value)
 {
     ARMCPU *cpu = arm_env_get_cpu(env);
-    *value = cpu->ccsidr[env->cp15.c0_cssel];
+    *value = cpu->ccsidr[BANKED_CP15_REG_GET(env, c0_cssel)];
     return 0;
 }
 
@@ -648,7 +653,8 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
     { .name = "CCSIDR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0,
       .access = PL1_R, .readfn = ccsidr_read, .type = ARM_CP_NO_MIGRATE },
     { .name = "CSSELR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 2, .opc2 = 0,
-      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c0_cssel),
+      .access = PL1_RW, .type = ARM_CP_BANKED,
+      .fieldoffset = offsetof(CPUARMState, cp15.c0_cssel),
       .writefn = csselr_write, .resetvalue = 0 },
     /* Auxiliary ID register: this actually has an IMPDEF value but for now
      * just RAZ for all cores:
@@ -702,15 +708,15 @@ static const ARMCPRegInfo t2ee_cp_reginfo[] = {
 
 static const ARMCPRegInfo v6k_cp_reginfo[] = {
     { .name = "TPIDRURW", .cp = 15, .crn = 13, .crm = 0, .opc1 = 0, .opc2 = 2,
-      .access = PL0_RW,
+      .access = PL0_RW, .type = ARM_CP_BANKED,
       .fieldoffset = offsetof(CPUARMState, cp15.c13_tls1),
       .resetvalue = 0 },
     { .name = "TPIDRURO", .cp = 15, .crn = 13, .crm = 0, .opc1 = 0, .opc2 = 3,
-      .access = PL0_R|PL1_W,
+      .access = PL0_R|PL1_W, .type = ARM_CP_BANKED,
       .fieldoffset = offsetof(CPUARMState, cp15.c13_tls2),
       .resetvalue = 0 },
     { .name = "TPIDRPRW", .cp = 15, .crn = 13, .crm = 0, .opc1 = 0, .opc2 = 4,
-      .access = PL1_RW,
+      .access = PL1_RW, .type = ARM_CP_BANKED,
       .fieldoffset = offsetof(CPUARMState, cp15.c13_tls3),
       .resetvalue = 0 },
     REGINFO_SENTINEL
@@ -974,11 +980,11 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
 static int par_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 {
     if (arm_feature(env, ARM_FEATURE_LPAE)) {
-        env->cp15.c7_par = value;
+        BANKED_CP15_REG_SET(env, c7_par, value);
     } else if (arm_feature(env, ARM_FEATURE_V7)) {
-        env->cp15.c7_par = value & 0xfffff6ff;
+        BANKED_CP15_REG_SET(env, c7_par, value & 0xfffff6ff);
     } else {
-        env->cp15.c7_par = value & 0xfffff1ff;
+        BANKED_CP15_REG_SET(env, c7_par, value & 0xfffff1ff);
     }
     return 0;
 }
@@ -1027,8 +1033,8 @@ static int ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
              * fault.
              */
         }
-        env->cp15.c7_par = par64;
-        env->cp15.c7_par_hi = par64 >> 32;
+        BANKED_CP15_REG_SET(env, c7_par, par64);
+        BANKED_CP15_REG_SET(env, c7_par_hi, par64 >> 32);
     } else {
         /* ret is a DFSR/IFSR value for the short descriptor
          * translation table format (with WnR always clear).
@@ -1038,16 +1044,17 @@ static int ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
             /* We do not set any attribute bits in the PAR */
             if (page_size == (1 << 24)
                 && arm_feature(env, ARM_FEATURE_V7)) {
-                env->cp15.c7_par = (phys_addr & 0xff000000) | 1 << 1;
+                BANKED_CP15_REG_SET(env, c7_par,
+                        (phys_addr & 0xff000000) | 1 << 1);
             } else {
-                env->cp15.c7_par = phys_addr & 0xfffff000;
+                BANKED_CP15_REG_SET(env, c7_par, phys_addr & 0xfffff000);
             }
         } else {
-            env->cp15.c7_par = ((ret & (10 << 1)) >> 5) |
+            BANKED_CP15_REG_SET(env, c7_par, ((ret & (10 << 1)) >> 5) |
                 ((ret & (12 << 1)) >> 6) |
-                ((ret & 0xf) << 1) | 1;
+                ((ret & 0xf) << 1) | 1);
         }
-        env->cp15.c7_par_hi = 0;
+        BANKED_CP15_REG_SET(env, c7_par_hi, 0);
     }
     return 0;
 }
@@ -1055,7 +1062,7 @@ static int ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 
 static const ARMCPRegInfo vapa_cp_reginfo[] = {
     { .name = "PAR", .cp = 15, .crn = 7, .crm = 4, .opc1 = 0, .opc2 = 0,
-      .access = PL1_RW, .resetvalue = 0,
+      .access = PL1_RW, .type = ARM_CP_BANKED, .resetvalue = 0,
       .fieldoffset = offsetof(CPUARMState, cp15.c7_par),
       .writefn = par_write },
 #ifndef CONFIG_USER_ONLY
@@ -1145,18 +1152,18 @@ static int arm946_prbs_write(CPUARMState *env, const ARMCPRegInfo *ri,
 
 static const ARMCPRegInfo pmsav5_cp_reginfo[] = {
     { .name = "DATA_AP", .cp = 15, .crn = 5, .crm = 0, .opc1 = 0, .opc2 = 0,
-      .access = PL1_RW, .type = ARM_CP_NO_MIGRATE,
+      .access = PL1_RW, .type = ARM_CP_NO_MIGRATE | ARM_CP_BANKED,
       .fieldoffset = offsetof(CPUARMState, cp15.c5_data), .resetvalue = 0,
       .readfn = pmsav5_data_ap_read, .writefn = pmsav5_data_ap_write, },
     { .name = "INSN_AP", .cp = 15, .crn = 5, .crm = 0, .opc1 = 0, .opc2 = 1,
-      .access = PL1_RW, .type = ARM_CP_NO_MIGRATE,
+      .access = PL1_RW, .type = ARM_CP_NO_MIGRATE | ARM_CP_BANKED,
       .fieldoffset = offsetof(CPUARMState, cp15.c5_insn), .resetvalue = 0,
       .readfn = pmsav5_insn_ap_read, .writefn = pmsav5_insn_ap_write, },
     { .name = "DATA_EXT_AP", .cp = 15, .crn = 5, .crm = 0, .opc1 = 0, .opc2 = 2,
-      .access = PL1_RW,
+      .access = PL1_RW, .type = ARM_CP_BANKED,
       .fieldoffset = offsetof(CPUARMState, cp15.c5_data), .resetvalue = 0, },
     { .name = "INSN_EXT_AP", .cp = 15, .crn = 5, .crm = 0, .opc1 = 0, .opc2 = 3,
-      .access = PL1_RW,
+      .access = PL1_RW, .type = ARM_CP_BANKED,
       .fieldoffset = offsetof(CPUARMState, cp15.c5_insn), .resetvalue = 0, },
     { .name = "DCACHE_CFG", .cp = 15, .crn = 2, .crm = 0, .opc1 = 0, .opc2 = 0,
       .access = PL1_RW,
@@ -1188,9 +1195,11 @@ static int vmsa_ttbcr_raw_write(CPUARMState *env, const ARMCPRegInfo *ri,
      * for long-descriptor tables the TTBCR fields are used differently
      * and the c2_mask and c2_base_mask values are meaningless.
      */
-    env->cp15.c2_control = value;
-    env->cp15.c2_mask = ~(((uint32_t)0xffffffffu) >> maskshift);
-    env->cp15.c2_base_mask = ~((uint32_t)0x3fffu >> maskshift);
+    BANKED_CP15_REG_SET(env, c2_control, value);
+    if (!USE_NS_REG(env)) {
+            env->cp15.c2_mask = ~(((uint32_t)0xffffffffu) >> maskshift);
+            env->cp15.c2_base_mask = ~((uint32_t)0x3fffu >> maskshift);
+    }
     return 0;
 }
 
@@ -1215,23 +1224,24 @@ static void vmsa_ttbcr_reset(CPUARMState *env, const ARMCPRegInfo *ri)
 
 static const ARMCPRegInfo vmsa_cp_reginfo[] = {
     { .name = "DFSR", .cp = 15, .crn = 5, .crm = 0, .opc1 = 0, .opc2 = 0,
-      .access = PL1_RW,
+      .access = PL1_RW, .type = ARM_CP_BANKED,
       .fieldoffset = offsetof(CPUARMState, cp15.c5_data), .resetvalue = 0, },
     { .name = "IFSR", .cp = 15, .crn = 5, .crm = 0, .opc1 = 0, .opc2 = 1,
-      .access = PL1_RW,
+      .access = PL1_RW, .type = ARM_CP_BANKED,
       .fieldoffset = offsetof(CPUARMState, cp15.c5_insn), .resetvalue = 0, },
     { .name = "TTBR0", .cp = 15, .crn = 2, .crm = 0, .opc1 = 0, .opc2 = 0,
-      .access = PL1_RW,
+      .access = PL1_RW, .type = ARM_CP_BANKED,
       .fieldoffset = offsetof(CPUARMState, cp15.c2_base0), .resetvalue = 0, },
     { .name = "TTBR1", .cp = 15, .crn = 2, .crm = 0, .opc1 = 0, .opc2 = 1,
-      .access = PL1_RW,
+      .access = PL1_RW, .type = ARM_CP_BANKED,
       .fieldoffset = offsetof(CPUARMState, cp15.c2_base1), .resetvalue = 0, },
     { .name = "TTBCR", .cp = 15, .crn = 2, .crm = 0, .opc1 = 0, .opc2 = 2,
-      .access = PL1_RW, .writefn = vmsa_ttbcr_write,
+      .access = PL1_RW, .type = ARM_CP_BANKED, .writefn = vmsa_ttbcr_write,
       .resetfn = vmsa_ttbcr_reset, .raw_writefn = vmsa_ttbcr_raw_write,
       .fieldoffset = offsetof(CPUARMState, cp15.c2_control) },
     { .name = "DFAR", .cp = 15, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 0,
-      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c6_data),
+      .access = PL1_RW, .type = ARM_CP_BANKED,
+      .fieldoffset = offsetof(CPUARMState, cp15.c6_data),
       .resetvalue = 0, },
     REGINFO_SENTINEL
 };
@@ -1274,7 +1284,8 @@ static int omap_cachemaint_write(CPUARMState *env, const ARMCPRegInfo *ri,
 
 static const ARMCPRegInfo omap_cp_reginfo[] = {
     { .name = "DFSR", .cp = 15, .crn = 5, .crm = CP_ANY,
-      .opc1 = CP_ANY, .opc2 = CP_ANY, .access = PL1_RW, .type = ARM_CP_OVERRIDE,
+      .opc1 = CP_ANY, .opc2 = CP_ANY,
+      .access = PL1_RW, .type = ARM_CP_OVERRIDE | ARM_CP_BANKED,
       .fieldoffset = offsetof(CPUARMState, cp15.c5_data), .resetvalue = 0, },
     { .name = "", .cp = 15, .crn = 15, .crm = 0, .opc1 = 0, .opc2 = 0,
       .access = PL1_RW, .type = ARM_CP_NOP },
@@ -1427,14 +1438,16 @@ static const ARMCPRegInfo mpidr_cp_reginfo[] = {
 
 static int par64_read(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t *value)
 {
-    *value = ((uint64_t)env->cp15.c7_par_hi << 32) | env->cp15.c7_par;
+    *value = BANKED_CP15_REG_GET(env, c7_par_hi);
+    *value <<= 32;
+    *value |= BANKED_CP15_REG_GET(env, c7_par);
     return 0;
 }
 
 static int par64_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 {
-    env->cp15.c7_par_hi = value >> 32;
-    env->cp15.c7_par = value;
+    BANKED_CP15_REG_SET(env, c7_par_hi, value >> 32);
+    BANKED_CP15_REG_SET(env, c7_par, value);
     return 0;
 }
 
@@ -1447,15 +1460,17 @@ static void par64_reset(CPUARMState *env, const ARMCPRegInfo *ri)
 static int ttbr064_read(CPUARMState *env, const ARMCPRegInfo *ri,
                         uint64_t *value)
 {
-    *value = ((uint64_t)env->cp15.c2_base0_hi << 32) | env->cp15.c2_base0;
+    *value = BANKED_CP15_REG_GET(env, c2_base0_hi);
+    *value <<= 32;
+    *value |= BANKED_CP15_REG_GET(env, c2_base0);
     return 0;
 }
 
 static int ttbr064_raw_write(CPUARMState *env, const ARMCPRegInfo *ri,
                              uint64_t value)
 {
-    env->cp15.c2_base0_hi = value >> 32;
-    env->cp15.c2_base0 = value;
+    BANKED_CP15_REG_SET(env, c2_base0_hi, value >> 32);
+    BANKED_CP15_REG_SET(env, c2_base0, value);
     return 0;
 }
 
@@ -1476,15 +1491,17 @@ static void ttbr064_reset(CPUARMState *env, const ARMCPRegInfo *ri)
 static int ttbr164_read(CPUARMState *env, const ARMCPRegInfo *ri,
                         uint64_t *value)
 {
-    *value = ((uint64_t)env->cp15.c2_base1_hi << 32) | env->cp15.c2_base1;
+    *value = BANKED_CP15_REG_GET(env, c2_base1_hi);
+    *value <<= 32;
+    *value |= BANKED_CP15_REG_GET(env, c2_base1);
     return 0;
 }
 
 static int ttbr164_write(CPUARMState *env, const ARMCPRegInfo *ri,
                          uint64_t value)
 {
-    env->cp15.c2_base1_hi = value >> 32;
-    env->cp15.c2_base1 = value;
+    BANKED_CP15_REG_SET(env, c2_base1_hi, value >> 32);
+    BANKED_CP15_REG_SET(env, c2_base1, value);
     return 0;
 }
 
@@ -1528,7 +1545,7 @@ static int sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
         value = value | 0x00c50078; /* This bits are RAO/WI */
     }
 
-    env->cp15.c1_sys = value;
+    BANKED_CP15_REG_SET(env, c1_sys, value);
     /* ??? Lots of these bits are not implemented.  */
     /* This may enable/disable the MMU, so do a TLB flush.  */
     tlb_flush(env, 1);
@@ -1557,7 +1574,7 @@ static const ARMCPRegInfo tz_cp_reginfo[] = {
       .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr),
       .resetvalue = 0 },
     { .name = "VBAR", .cp = 15, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
-      .access = PL1_RW, .writefn = vbar_write,
+      .access = PL1_RW, .type = ARM_CP_BANKED, .writefn = vbar_write,
       .fieldoffset = offsetof(CPUARMState, cp15.c12_vbar),
       .resetvalue = 0 },
     { .name = "SDER", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 1,
@@ -1795,7 +1812,8 @@ void register_cp_regs_for_features(ARMCPU *cpu)
     {
         ARMCPRegInfo sctlr = {
             .name = "SCTLR", .cp = 15, .crn = 1, .crm = 0, .opc1 = 0, .opc2 = 0,
-            .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_sys),
+            .access = PL1_RW, .type = ARM_CP_BANKED,
+            .fieldoffset = offsetof(CPUARMState, cp15.c1_sys),
             .writefn = sctlr_write, .resetvalue = cpu->reset_sctlr,
             .raw_writefn = raw_write,
         };
-- 
1.7.9.5

[Qemu-devel] [RFC PATCH 17/21] target-arm: use c13_context field for CONTEXTIDR

[Sergey Fedorov]

Use c13_context field instead of c13_fcse for CONTEXTIDR register
definition.

Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
---
 target-arm/helper.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 9442e08..e1e9762 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -359,7 +359,7 @@ static const ARMCPRegInfo cp_reginfo[] = {
       .resetvalue = 0, .writefn = fcse_write, .raw_writefn = raw_write, },
     { .name = "CONTEXTIDR", .cp = 15, .crn = 13, .crm = 0, .opc1 = 0, .opc2 = 1,
       .access = PL1_RW, .type = ARM_CP_BANKED,
-      .fieldoffset = offsetof(CPUARMState, cp15.c13_fcse),
+      .fieldoffset = offsetof(CPUARMState, cp15.c13_context),
       .resetvalue = 0, .writefn = contextidr_write, .raw_writefn = raw_write, },
     /* ??? This covers not just the impdef TLB lockdown registers but also
      * some v7VMSA registers relating to TEX remap, so it is overly broad.
-- 
1.7.9.5

Re: [Qemu-devel] [RFC PATCH 17/21] target-arm: use c13_context field for CONTEXTIDR

[Peter Crosthwaite]

On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com> wrote:
> Use c13_context field instead of c13_fcse for CONTEXTIDR register
> definition.

This a standalone (I.E. not TZ related) bug?

Regards,
peter

>
> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
> ---
>  target-arm/helper.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 9442e08..e1e9762 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -359,7 +359,7 @@ static const ARMCPRegInfo cp_reginfo[] = {
>        .resetvalue = 0, .writefn = fcse_write, .raw_writefn = raw_write, },
>      { .name = "CONTEXTIDR", .cp = 15, .crn = 13, .crm = 0, .opc1 = 0, .opc2 = 1,
>        .access = PL1_RW, .type = ARM_CP_BANKED,
> -      .fieldoffset = offsetof(CPUARMState, cp15.c13_fcse),
> +      .fieldoffset = offsetof(CPUARMState, cp15.c13_context),
>        .resetvalue = 0, .writefn = contextidr_write, .raw_writefn = raw_write, },
>      /* ??? This covers not just the impdef TLB lockdown registers but also
>       * some v7VMSA registers relating to TEX remap, so it is overly broad.
> --
> 1.7.9.5
>
>

Re: [Qemu-devel] [RFC PATCH 17/21] target-arm: use c13_context field for CONTEXTIDR

[Fedorov Sergey]

On 12/19/2013 08:31 AM, Peter Crosthwaite wrote:
> On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com> wrote:
>> Use c13_context field instead of c13_fcse for CONTEXTIDR register
>> definition.
> This a standalone (I.E. not TZ related) bug?
>
> Regards,
> peter

Yes, I think so. Then I will submit this patch separately soon.

Best regards,
Sergey Fedorov

>
>> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
>> ---
>>   target-arm/helper.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 9442e08..e1e9762 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -359,7 +359,7 @@ static const ARMCPRegInfo cp_reginfo[] = {
>>         .resetvalue = 0, .writefn = fcse_write, .raw_writefn = raw_write, },
>>       { .name = "CONTEXTIDR", .cp = 15, .crn = 13, .crm = 0, .opc1 = 0, .opc2 = 1,
>>         .access = PL1_RW, .type = ARM_CP_BANKED,
>> -      .fieldoffset = offsetof(CPUARMState, cp15.c13_fcse),
>> +      .fieldoffset = offsetof(CPUARMState, cp15.c13_context),
>>         .resetvalue = 0, .writefn = contextidr_write, .raw_writefn = raw_write, },
>>       /* ??? This covers not just the impdef TLB lockdown registers but also
>>        * some v7VMSA registers relating to TEX remap, so it is overly broad.
>> --
>> 1.7.9.5
>>
>>

Re: [Qemu-devel] [RFC PATCH 17/21] target-arm: use c13_context field for CONTEXTIDR

[Peter Crosthwaite]

On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com> wrote:
> Use c13_context field instead of c13_fcse for CONTEXTIDR register
> definition.
>
> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Add this to your commit msg on your planned single-send. Thanks for
catching this.

Regards,
Peter

> ---
>  target-arm/helper.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 9442e08..e1e9762 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -359,7 +359,7 @@ static const ARMCPRegInfo cp_reginfo[] = {
>        .resetvalue = 0, .writefn = fcse_write, .raw_writefn = raw_write, },
>      { .name = "CONTEXTIDR", .cp = 15, .crn = 13, .crm = 0, .opc1 = 0, .opc2 = 1,
>        .access = PL1_RW, .type = ARM_CP_BANKED,
> -      .fieldoffset = offsetof(CPUARMState, cp15.c13_fcse),
> +      .fieldoffset = offsetof(CPUARMState, cp15.c13_context),
>        .resetvalue = 0, .writefn = contextidr_write, .raw_writefn = raw_write, },
>      /* ??? This covers not just the impdef TLB lockdown registers but also
>       * some v7VMSA registers relating to TEX remap, so it is overly broad.
> --
> 1.7.9.5
>
>

[Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers

[Sergey Fedorov]

Banked coprocessor registers are switched on two cases:
1) Entering or leaving CPU monitor mode with SCR.NS bit set;
2) Setting SCR.NS bit not from CPU monitor mode

Coprocessor register banking is done similar to CPU core register
banking. Some of SCTRL bits are common for secure and non-secure state.
Translation table base masks are updated on register switch instead
of TTBCR write.

Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
---
 target-arm/helper.c |   77 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 76 insertions(+), 1 deletion(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index e1e9762..7bfadb0 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -11,6 +11,7 @@ static inline int get_phys_addr(CPUARMState *env, uint32_t address,
                                 int access_type, int is_user,
                                 hwaddr *phys_ptr, int *prot,
                                 target_ulong *page_size);
+static void switch_cp15_regs(CPUARMState *env, int secure);
 #endif
 
 static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
@@ -1553,6 +1554,17 @@ static int sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 }
 
 #ifndef CONFIG_USER_ONLY
+static int scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
+{
+    if ((value & 1/*NS*/) && (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_MON) {
+        /* We are going to Non-secure state; switch banked system control registers */
+        switch_cp15_regs(env, 0);
+    }
+
+    env->cp15.c1_scr = value;
+    return 0;
+}
+
 static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
                       uint64_t value)
 {
@@ -1572,7 +1584,7 @@ static const ARMCPRegInfo tz_cp_reginfo[] = {
 #ifndef CONFIG_USER_ONLY
     { .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0,
       .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr),
-      .resetvalue = 0 },
+      .writefn = scr_write, .resetvalue = 0 },
     { .name = "VBAR", .cp = 15, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
       .access = PL1_RW, .type = ARM_CP_BANKED, .writefn = vbar_write,
       .fieldoffset = offsetof(CPUARMState, cp15.c12_vbar),
@@ -2284,6 +2296,69 @@ void switch_mode(CPUARMState *env, int mode)
     env->regs[13] = env->banked_r13[i];
     env->regs[14] = env->banked_r14[i];
     env->spsr = env->banked_spsr[i];
+
+    if ((mode == ARM_CPU_MODE_MON || old_mode == ARM_CPU_MODE_MON) &&
+            (env->cp15.c1_scr & 1/*NS*/)) {
+        /* We are going to change Security state; switch banked system control registers */
+        switch_cp15_regs(env, (mode == ARM_CPU_MODE_MON));
+    }
+}
+
+void switch_cp15_regs(CPUARMState *env, int secure)
+{
+    int i;
+
+    /* Save current Security state registers */
+    i = arm_is_secure(env) ? 0 : 1;
+    env->cp15.banked_c0_cssel[i] = env->cp15.c0_cssel;
+    env->cp15.banked_c1_sys[i] = env->cp15.c1_sys;
+    env->cp15.banked_c2_base0[i] = env->cp15.c2_base0;
+    env->cp15.banked_c2_base0_hi[i] = env->cp15.c2_base0_hi;
+    env->cp15.banked_c2_base1[i] = env->cp15.c2_base1;
+    env->cp15.banked_c2_base1_hi[i] = env->cp15.c2_base1_hi;
+    env->cp15.banked_c2_control[i] = env->cp15.c2_control;
+    env->cp15.banked_c3[i] = env->cp15.c3;
+    env->cp15.banked_c5_data[i] = env->cp15.c5_data;
+    env->cp15.banked_c5_insn[i] = env->cp15.c5_insn;
+    env->cp15.banked_c6_data[i] = env->cp15.c6_data;
+    env->cp15.banked_c6_insn[i] = env->cp15.c6_insn;
+    env->cp15.banked_c7_par[i] = env->cp15.c7_par;
+    env->cp15.banked_c7_par_hi[i] = env->cp15.c7_par_hi;
+    env->cp15.banked_c13_context[i] = env->cp15.c13_context;
+    env->cp15.banked_c13_fcse[i] = env->cp15.c13_fcse;
+    env->cp15.banked_c13_tls1[i] = env->cp15.c13_tls1;
+    env->cp15.banked_c13_tls2[i] = env->cp15.c13_tls2;
+    env->cp15.banked_c13_tls3[i] = env->cp15.c13_tls3;
+
+    /* Restore new Security state registers */
+    i = secure ? 0 : 1;
+    env->cp15.c0_cssel = env->cp15.banked_c0_cssel[i];
+    /* Maintain the value of common bits */
+    env->cp15.c1_sys &= 0x8204000;
+    env->cp15.c1_sys |= env->cp15.banked_c1_sys[i] & ~0x8204000;
+    env->cp15.c2_base0 = env->cp15.banked_c2_base0[i];
+    env->cp15.c2_base0_hi = env->cp15.banked_c2_base0_hi[i];
+    env->cp15.c2_base1 = env->cp15.banked_c2_base1[i];
+    env->cp15.c2_base1_hi = env->cp15.banked_c2_base1_hi[i];
+    {
+        int maskshift;
+        env->cp15.c2_control = env->cp15.banked_c2_control[i];
+        maskshift = extract32(env->cp15.c2_control, 0, 3);
+        env->cp15.c2_mask = ~(((uint32_t)0xffffffffu) >> maskshift);
+        env->cp15.c2_base_mask = ~((uint32_t)0x3fffu >> maskshift);
+    }
+    env->cp15.c3 = env->cp15.banked_c3[i];
+    env->cp15.c5_data = env->cp15.banked_c5_data[i];
+    env->cp15.c5_insn = env->cp15.banked_c5_insn[i];
+    env->cp15.c6_data = env->cp15.banked_c6_data[i];
+    env->cp15.c6_insn = env->cp15.banked_c6_insn[i];
+    env->cp15.c7_par = env->cp15.banked_c7_par[i];
+    env->cp15.c7_par_hi = env->cp15.banked_c7_par_hi[i];
+    env->cp15.c13_context = env->cp15.banked_c13_context[i];
+    env->cp15.c13_fcse = env->cp15.banked_c13_fcse[i];
+    env->cp15.c13_tls1 = env->cp15.banked_c13_tls1[i];
+    env->cp15.c13_tls2 = env->cp15.banked_c13_tls2[i];
+    env->cp15.c13_tls3 = env->cp15.banked_c13_tls3[i];
 }
 
 static void v7m_push(CPUARMState *env, uint32_t val)
-- 
1.7.9.5

Re: [Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers

[Peter Crosthwaite]

On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com> wrote:
> Banked coprocessor registers are switched on two cases:
> 1) Entering or leaving CPU monitor mode with SCR.NS bit set;
> 2) Setting SCR.NS bit not from CPU monitor mode
>
> Coprocessor register banking is done similar to CPU core register
> banking. Some of SCTRL bits are common for secure and non-secure state.
> Translation table base masks are updated on register switch instead
> of TTBCR write.
>

So I was rather confused as to your banking scheme until I got to this
patch. Now I see the implementation. You are mass-hot-swapping in the
active state on critical CPU state changing events. ....

> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
> ---
>  target-arm/helper.c |   77 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 76 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index e1e9762..7bfadb0 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -11,6 +11,7 @@ static inline int get_phys_addr(CPUARMState *env, uint32_t address,
>                                  int access_type, int is_user,
>                                  hwaddr *phys_ptr, int *prot,
>                                  target_ulong *page_size);
> +static void switch_cp15_regs(CPUARMState *env, int secure);
>  #endif
>
>  static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
> @@ -1553,6 +1554,17 @@ static int sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>  }
>
>  #ifndef CONFIG_USER_ONLY
> +static int scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
> +{
> +    if ((value & 1/*NS*/) && (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_MON) {
> +        /* We are going to Non-secure state; switch banked system control registers */
> +        switch_cp15_regs(env, 0);
> +    }
> +
> +    env->cp15.c1_scr = value;
> +    return 0;
> +}
> +
>  static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                        uint64_t value)
>  {
> @@ -1572,7 +1584,7 @@ static const ARMCPRegInfo tz_cp_reginfo[] = {
>  #ifndef CONFIG_USER_ONLY
>      { .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0,
>        .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr),
> -      .resetvalue = 0 },
> +      .writefn = scr_write, .resetvalue = 0 },
>      { .name = "VBAR", .cp = 15, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
>        .access = PL1_RW, .type = ARM_CP_BANKED, .writefn = vbar_write,
>        .fieldoffset = offsetof(CPUARMState, cp15.c12_vbar),
> @@ -2284,6 +2296,69 @@ void switch_mode(CPUARMState *env, int mode)
>      env->regs[13] = env->banked_r13[i];
>      env->regs[14] = env->banked_r14[i];
>      env->spsr = env->banked_spsr[i];
> +
> +    if ((mode == ARM_CPU_MODE_MON || old_mode == ARM_CPU_MODE_MON) &&
> +            (env->cp15.c1_scr & 1/*NS*/)) {
> +        /* We are going to change Security state; switch banked system control registers */
> +        switch_cp15_regs(env, (mode == ARM_CPU_MODE_MON));
> +    }
> +}
> +
> +void switch_cp15_regs(CPUARMState *env, int secure)
> +{
> +    int i;
> +
> +    /* Save current Security state registers */
> +    i = arm_is_secure(env) ? 0 : 1;
> +    env->cp15.banked_c0_cssel[i] = env->cp15.c0_cssel;
> +    env->cp15.banked_c1_sys[i] = env->cp15.c1_sys;
> +    env->cp15.banked_c2_base0[i] = env->cp15.c2_base0;
> +    env->cp15.banked_c2_base0_hi[i] = env->cp15.c2_base0_hi;
> +    env->cp15.banked_c2_base1[i] = env->cp15.c2_base1;
> +    env->cp15.banked_c2_base1_hi[i] = env->cp15.c2_base1_hi;
> +    env->cp15.banked_c2_control[i] = env->cp15.c2_control;
> +    env->cp15.banked_c3[i] = env->cp15.c3;
> +    env->cp15.banked_c5_data[i] = env->cp15.c5_data;
> +    env->cp15.banked_c5_insn[i] = env->cp15.c5_insn;
> +    env->cp15.banked_c6_data[i] = env->cp15.c6_data;
> +    env->cp15.banked_c6_insn[i] = env->cp15.c6_insn;
> +    env->cp15.banked_c7_par[i] = env->cp15.c7_par;
> +    env->cp15.banked_c7_par_hi[i] = env->cp15.c7_par_hi;
> +    env->cp15.banked_c13_context[i] = env->cp15.c13_context;
> +    env->cp15.banked_c13_fcse[i] = env->cp15.c13_fcse;
> +    env->cp15.banked_c13_tls1[i] = env->cp15.c13_tls1;
> +    env->cp15.banked_c13_tls2[i] = env->cp15.c13_tls2;
> +    env->cp15.banked_c13_tls3[i] = env->cp15.c13_tls3;
> +
> +    /* Restore new Security state registers */
> +    i = secure ? 0 : 1;
> +    env->cp15.c0_cssel = env->cp15.banked_c0_cssel[i];
> +    /* Maintain the value of common bits */
> +    env->cp15.c1_sys &= 0x8204000;
> +    env->cp15.c1_sys |= env->cp15.banked_c1_sys[i] & ~0x8204000;
> +    env->cp15.c2_base0 = env->cp15.banked_c2_base0[i];
> +    env->cp15.c2_base0_hi = env->cp15.banked_c2_base0_hi[i];
> +    env->cp15.c2_base1 = env->cp15.banked_c2_base1[i];
> +    env->cp15.c2_base1_hi = env->cp15.banked_c2_base1_hi[i];
> +    {
> +        int maskshift;
> +        env->cp15.c2_control = env->cp15.banked_c2_control[i];
> +        maskshift = extract32(env->cp15.c2_control, 0, 3);
> +        env->cp15.c2_mask = ~(((uint32_t)0xffffffffu) >> maskshift);
> +        env->cp15.c2_base_mask = ~((uint32_t)0x3fffu >> maskshift);
> +    }
> +    env->cp15.c3 = env->cp15.banked_c3[i];
> +    env->cp15.c5_data = env->cp15.banked_c5_data[i];
> +    env->cp15.c5_insn = env->cp15.banked_c5_insn[i];
> +    env->cp15.c6_data = env->cp15.banked_c6_data[i];
> +    env->cp15.c6_insn = env->cp15.banked_c6_insn[i];
> +    env->cp15.c7_par = env->cp15.banked_c7_par[i];
> +    env->cp15.c7_par_hi = env->cp15.banked_c7_par_hi[i];
> +    env->cp15.c13_context = env->cp15.banked_c13_context[i];
> +    env->cp15.c13_fcse = env->cp15.banked_c13_fcse[i];
> +    env->cp15.c13_tls1 = env->cp15.banked_c13_tls1[i];
> +    env->cp15.c13_tls2 = env->cp15.banked_c13_tls2[i];
> +    env->cp15.c13_tls3 = env->cp15.banked_c13_tls3[i];
>  }

And I think this is awkward. Can't we just teach TCG to get the right
value out of the banked array and do away with these active copies
completely? This greatly complicates the change pattern for adding a
new banked CP.

Regards,
Peter

>
>  static void v7m_push(CPUARMState *env, uint32_t val)
> --
> 1.7.9.5
>
>

Re: [Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers

[Fedorov Sergey]

On 12/19/2013 08:37 AM, Peter Crosthwaite wrote:
> On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com> wrote:
>> Banked coprocessor registers are switched on two cases:
>> 1) Entering or leaving CPU monitor mode with SCR.NS bit set;
>> 2) Setting SCR.NS bit not from CPU monitor mode
>>
>> Coprocessor register banking is done similar to CPU core register
>> banking. Some of SCTRL bits are common for secure and non-secure state.
>> Translation table base masks are updated on register switch instead
>> of TTBCR write.
>>
> So I was rather confused as to your banking scheme until I got to this
> patch. Now I see the implementation. You are mass-hot-swapping in the
> active state on critical CPU state changing events. ....
>
>> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
>> ---
>>   target-arm/helper.c |   77 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 76 insertions(+), 1 deletion(-)
>>
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index e1e9762..7bfadb0 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -11,6 +11,7 @@ static inline int get_phys_addr(CPUARMState *env, uint32_t address,
>>                                   int access_type, int is_user,
>>                                   hwaddr *phys_ptr, int *prot,
>>                                   target_ulong *page_size);
>> +static void switch_cp15_regs(CPUARMState *env, int secure);
>>   #endif
>>
>>   static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
>> @@ -1553,6 +1554,17 @@ static int sctlr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>>   }
>>
>>   #ifndef CONFIG_USER_ONLY
>> +static int scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>> +{
>> +    if ((value & 1/*NS*/) && (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_MON) {
>> +        /* We are going to Non-secure state; switch banked system control registers */
>> +        switch_cp15_regs(env, 0);
>> +    }
>> +
>> +    env->cp15.c1_scr = value;
>> +    return 0;
>> +}
>> +
>>   static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>                         uint64_t value)
>>   {
>> @@ -1572,7 +1584,7 @@ static const ARMCPRegInfo tz_cp_reginfo[] = {
>>   #ifndef CONFIG_USER_ONLY
>>       { .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0,
>>         .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr),
>> -      .resetvalue = 0 },
>> +      .writefn = scr_write, .resetvalue = 0 },
>>       { .name = "VBAR", .cp = 15, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
>>         .access = PL1_RW, .type = ARM_CP_BANKED, .writefn = vbar_write,
>>         .fieldoffset = offsetof(CPUARMState, cp15.c12_vbar),
>> @@ -2284,6 +2296,69 @@ void switch_mode(CPUARMState *env, int mode)
>>       env->regs[13] = env->banked_r13[i];
>>       env->regs[14] = env->banked_r14[i];
>>       env->spsr = env->banked_spsr[i];
>> +
>> +    if ((mode == ARM_CPU_MODE_MON || old_mode == ARM_CPU_MODE_MON) &&
>> +            (env->cp15.c1_scr & 1/*NS*/)) {
>> +        /* We are going to change Security state; switch banked system control registers */
>> +        switch_cp15_regs(env, (mode == ARM_CPU_MODE_MON));
>> +    }
>> +}
>> +
>> +void switch_cp15_regs(CPUARMState *env, int secure)
>> +{
>> +    int i;
>> +
>> +    /* Save current Security state registers */
>> +    i = arm_is_secure(env) ? 0 : 1;
>> +    env->cp15.banked_c0_cssel[i] = env->cp15.c0_cssel;
>> +    env->cp15.banked_c1_sys[i] = env->cp15.c1_sys;
>> +    env->cp15.banked_c2_base0[i] = env->cp15.c2_base0;
>> +    env->cp15.banked_c2_base0_hi[i] = env->cp15.c2_base0_hi;
>> +    env->cp15.banked_c2_base1[i] = env->cp15.c2_base1;
>> +    env->cp15.banked_c2_base1_hi[i] = env->cp15.c2_base1_hi;
>> +    env->cp15.banked_c2_control[i] = env->cp15.c2_control;
>> +    env->cp15.banked_c3[i] = env->cp15.c3;
>> +    env->cp15.banked_c5_data[i] = env->cp15.c5_data;
>> +    env->cp15.banked_c5_insn[i] = env->cp15.c5_insn;
>> +    env->cp15.banked_c6_data[i] = env->cp15.c6_data;
>> +    env->cp15.banked_c6_insn[i] = env->cp15.c6_insn;
>> +    env->cp15.banked_c7_par[i] = env->cp15.c7_par;
>> +    env->cp15.banked_c7_par_hi[i] = env->cp15.c7_par_hi;
>> +    env->cp15.banked_c13_context[i] = env->cp15.c13_context;
>> +    env->cp15.banked_c13_fcse[i] = env->cp15.c13_fcse;
>> +    env->cp15.banked_c13_tls1[i] = env->cp15.c13_tls1;
>> +    env->cp15.banked_c13_tls2[i] = env->cp15.c13_tls2;
>> +    env->cp15.banked_c13_tls3[i] = env->cp15.c13_tls3;
>> +
>> +    /* Restore new Security state registers */
>> +    i = secure ? 0 : 1;
>> +    env->cp15.c0_cssel = env->cp15.banked_c0_cssel[i];
>> +    /* Maintain the value of common bits */
>> +    env->cp15.c1_sys &= 0x8204000;
>> +    env->cp15.c1_sys |= env->cp15.banked_c1_sys[i] & ~0x8204000;
>> +    env->cp15.c2_base0 = env->cp15.banked_c2_base0[i];
>> +    env->cp15.c2_base0_hi = env->cp15.banked_c2_base0_hi[i];
>> +    env->cp15.c2_base1 = env->cp15.banked_c2_base1[i];
>> +    env->cp15.c2_base1_hi = env->cp15.banked_c2_base1_hi[i];
>> +    {
>> +        int maskshift;
>> +        env->cp15.c2_control = env->cp15.banked_c2_control[i];
>> +        maskshift = extract32(env->cp15.c2_control, 0, 3);
>> +        env->cp15.c2_mask = ~(((uint32_t)0xffffffffu) >> maskshift);
>> +        env->cp15.c2_base_mask = ~((uint32_t)0x3fffu >> maskshift);
>> +    }
>> +    env->cp15.c3 = env->cp15.banked_c3[i];
>> +    env->cp15.c5_data = env->cp15.banked_c5_data[i];
>> +    env->cp15.c5_insn = env->cp15.banked_c5_insn[i];
>> +    env->cp15.c6_data = env->cp15.banked_c6_data[i];
>> +    env->cp15.c6_insn = env->cp15.banked_c6_insn[i];
>> +    env->cp15.c7_par = env->cp15.banked_c7_par[i];
>> +    env->cp15.c7_par_hi = env->cp15.banked_c7_par_hi[i];
>> +    env->cp15.c13_context = env->cp15.banked_c13_context[i];
>> +    env->cp15.c13_fcse = env->cp15.banked_c13_fcse[i];
>> +    env->cp15.c13_tls1 = env->cp15.banked_c13_tls1[i];
>> +    env->cp15.c13_tls2 = env->cp15.banked_c13_tls2[i];
>> +    env->cp15.c13_tls3 = env->cp15.banked_c13_tls3[i];
>>   }
> And I think this is awkward. Can't we just teach TCG to get the right
> value out of the banked array and do away with these active copies
> completely? This greatly complicates the change pattern for adding a
> new banked CP.
>
> Regards,
> Peter

Yes, this banking scheme makes state changing events quite heavy. But 
maintaining the active copies allows to keep translation table walking 
code untouched. I think there is a trade-off between state changing and 
translation table walking overheads.

I think the CP banking is the most fragile thing in this patch series 
and this should become much better after review :)

Thanks!

>
>>   static void v7m_push(CPUARMState *env, uint32_t val)
>> --
>> 1.7.9.5
>>
>>

-- 
Best regards,
Sergey Fedorov

Re: [Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers

[Peter Maydell]

On 19 December 2013 07:27, Fedorov Sergey <s.fedorov@samsung.com> wrote:
> Yes, this banking scheme makes state changing events quite heavy. But
> maintaining the active copies allows to keep translation table walking code
> untouched. I think there is a trade-off between state changing and
> translation table walking overheads.

We shouldn't be doing tlb walks that often that it makes a
difference whether we do env->ttbr0 or env->ttbr0[env->ns] ...

> I think the CP banking is the most fragile thing in this patch series and
> this should become much better after review :)

It would probably be a good idea to look at the v8 ARM ARM and
figure out how banked-for-NS/S registers should fit in with the
AArch64 vs AArch32 split.
[if you don't have a copy, it's on the ARM website:
http://infocenter.arm.com/help/topic/com.arm.doc.ddi0487a.a_errata2/index.html
you'll need to register an account on the website if you don't already
have one but it's a fairly simple "fill in the form" automated process]

Note in particular that:
 * many of the current uint32_t fields in our CPU state struct are
   likely to widen to uint64_t, so the AArch64 representation is
   canonical, and the AArch32 register accessors access a part
   of that state (typically the lower 32 bits)
 * registers which are banked S/NS in AArch32 are not necessarily
   banked in AArch64

AArch64 support is likely to land before your TrustZone stuff
does so we need to make the two features work together cleanly.

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers

[Peter Crosthwaite]

On Thu, Dec 19, 2013 at 9:38 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 19 December 2013 07:27, Fedorov Sergey <s.fedorov@samsung.com> wrote:
>> Yes, this banking scheme makes state changing events quite heavy. But
>> maintaining the active copies allows to keep translation table walking code
>> untouched. I think there is a trade-off between state changing and
>> translation table walking overheads.
>
> We shouldn't be doing tlb walks that often that it makes a
> difference whether we do env->ttbr0 or env->ttbr0[env->ns] ...
>
>> I think the CP banking is the most fragile thing in this patch series and
>> this should become much better after review :)
>
> It would probably be a good idea to look at the v8 ARM ARM and
> figure out how banked-for-NS/S registers should fit in with the
> AArch64 vs AArch32 split.
> [if you don't have a copy, it's on the ARM website:
> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0487a.a_errata2/index.html
> you'll need to register an account on the website if you don't already
> have one but it's a fairly simple "fill in the form" automated process]
>
> Note in particular that:
>  * many of the current uint32_t fields in our CPU state struct are
>    likely to widen to uint64_t, so the AArch64 representation is
>    canonical, and the AArch32 register accessors access a part
>    of that state (typically the lower 32 bits)
>  * registers which are banked S/NS in AArch32 are not necessarily
>    banked in AArch64
>

Adding to that, are there any other reasons to bank a register other
than sec-extensions? It seems like what you have implemented here
is too sec specific for simply calling it "banked" (without further
clarification of what you are banking for).

Regards,
Peter

> AArch64 support is likely to land before your TrustZone stuff
> does so we need to make the two features work together cleanly.
>
> thanks
> -- PMM
>

Re: [Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers

[Fedorov Sergey]

On 12/19/2013 04:44 PM, Peter Crosthwaite wrote:
> On Thu, Dec 19, 2013 at 9:38 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 19 December 2013 07:27, Fedorov Sergey <s.fedorov@samsung.com> wrote:
>>> Yes, this banking scheme makes state changing events quite heavy. But
>>> maintaining the active copies allows to keep translation table walking code
>>> untouched. I think there is a trade-off between state changing and
>>> translation table walking overheads.
>> We shouldn't be doing tlb walks that often that it makes a
>> difference whether we do env->ttbr0 or env->ttbr0[env->ns] ...
>>
>>> I think the CP banking is the most fragile thing in this patch series and
>>> this should become much better after review :)
>> It would probably be a good idea to look at the v8 ARM ARM and
>> figure out how banked-for-NS/S registers should fit in with the
>> AArch64 vs AArch32 split.
>> [if you don't have a copy, it's on the ARM website:
>> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0487a.a_errata2/index.html
>> you'll need to register an account on the website if you don't already
>> have one but it's a fairly simple "fill in the form" automated process]
>>
>> Note in particular that:
>>   * many of the current uint32_t fields in our CPU state struct are
>>     likely to widen to uint64_t, so the AArch64 representation is
>>     canonical, and the AArch32 register accessors access a part
>>     of that state (typically the lower 32 bits)
>>   * registers which are banked S/NS in AArch32 are not necessarily
>>     banked in AArch64
>>
> Adding to that, are there any other reasons to bank a register other
> than sec-extensions? It seems like what you have implemented here
> is too sec specific for simply calling it "banked" (without further
> clarification of what you are banking for).
>
> Regards,
> Peter

I'm not sure that I understand your question correctly but I try to 
answer. From ARMv7 ARM document section "B3.15.3 Classification of 
system control registers":

"Banked system control registers have two copies, one Secure and one 
Non-secure."

I don't know any use of term "CP15 banked registers" other that related 
to Security Extensions.

Best regards,
Sergey Fedorov

>
>> AArch64 support is likely to land before your TrustZone stuff
>> does so we need to make the two features work together cleanly.
>>
>> thanks
>> -- PMM
>>

Re: [Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers

[Peter Crosthwaite]

On Thu, Dec 19, 2013 at 11:39 PM, Fedorov Sergey <s.fedorov@samsung.com> wrote:
>
> On 12/19/2013 04:44 PM, Peter Crosthwaite wrote:
>>
>> On Thu, Dec 19, 2013 at 9:38 PM, Peter Maydell <peter.maydell@linaro.org>
>> wrote:
>>>
>>> On 19 December 2013 07:27, Fedorov Sergey <s.fedorov@samsung.com> wrote:
>>>>
>>>> Yes, this banking scheme makes state changing events quite heavy. But
>>>> maintaining the active copies allows to keep translation table walking
>>>> code
>>>> untouched. I think there is a trade-off between state changing and
>>>> translation table walking overheads.
>>>
>>> We shouldn't be doing tlb walks that often that it makes a
>>> difference whether we do env->ttbr0 or env->ttbr0[env->ns] ...
>>>
>>>> I think the CP banking is the most fragile thing in this patch series
>>>> and
>>>> this should become much better after review :)
>>>
>>> It would probably be a good idea to look at the v8 ARM ARM and
>>> figure out how banked-for-NS/S registers should fit in with the
>>> AArch64 vs AArch32 split.
>>> [if you don't have a copy, it's on the ARM website:
>>>
>>> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0487a.a_errata2/index.html
>>> you'll need to register an account on the website if you don't already
>>> have one but it's a fairly simple "fill in the form" automated process]
>>>
>>> Note in particular that:
>>>   * many of the current uint32_t fields in our CPU state struct are
>>>     likely to widen to uint64_t, so the AArch64 representation is
>>>     canonical, and the AArch32 register accessors access a part
>>>     of that state (typically the lower 32 bits)
>>>   * registers which are banked S/NS in AArch32 are not necessarily
>>>     banked in AArch64
>>>
>> Adding to that, are there any other reasons to bank a register other
>> than sec-extensions? It seems like what you have implemented here
>> is too sec specific for simply calling it "banked" (without further
>> clarification of what you are banking for).
>>
>> Regards,
>> Peter
>
>
> I'm not sure that I understand your question correctly but I try to answer.
> From ARMv7 ARM document section "B3.15.3 Classification of system control
> registers":
>
> "Banked system control registers have two copies, one Secure and one
> Non-secure."
>

Ok fair enough. I will wager though that sooner or later ARM will find
a reason to bank a reg other than sec-ext. But let's stick to their
terminology which by that quote its quite clear that "banked" means
"banked for security" (No change needed).

Regards,
Peter

> I don't know any use of term "CP15 banked registers" other that related to
> Security Extensions.
>
> Best regards,
> Sergey Fedorov
>
>
>>
>>> AArch64 support is likely to land before your TrustZone stuff
>>> does so we need to make the two features work together cleanly.
>>>
>>> thanks
>>> -- PMM
>>>
>
>

Re: [Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers

[Peter Maydell]

On 19 December 2013 14:01, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Thu, Dec 19, 2013 at 11:39 PM, Fedorov Sergey <s.fedorov@samsung.com> wrote:
>> "Banked system control registers have two copies, one Secure and one
>> Non-secure."
>>
>
> Ok fair enough. I will wager though that sooner or later ARM will find
> a reason to bank a reg other than sec-ext. But let's stick to their
> terminology which by that quote its quite clear that "banked" means
> "banked for security" (No change needed).

Well, the other register banking is the gp regs which get banks
for user/svc/fiq/etc in AArch32. But I think in the context of system
registers "banked register" is fairly clear.

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers

[Fedorov Sergey]

On 12/19/2013 03:38 PM, Peter Maydell wrote:
> On 19 December 2013 07:27, Fedorov Sergey <s.fedorov@samsung.com> wrote:
>> Yes, this banking scheme makes state changing events quite heavy. But
>> maintaining the active copies allows to keep translation table walking code
>> untouched. I think there is a trade-off between state changing and
>> translation table walking overheads.
> We shouldn't be doing tlb walks that often that it makes a
> difference whether we do env->ttbr0 or env->ttbr0[env->ns] ...
>
>> I think the CP banking is the most fragile thing in this patch series and
>> this should become much better after review :)
> It would probably be a good idea to look at the v8 ARM ARM and
> figure out how banked-for-NS/S registers should fit in with the
> AArch64 vs AArch32 split.
> [if you don't have a copy, it's on the ARM website:
> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0487a.a_errata2/index.html
> you'll need to register an account on the website if you don't already
> have one but it's a fairly simple "fill in the form" automated process]
>
> Note in particular that:
>   * many of the current uint32_t fields in our CPU state struct are
>     likely to widen to uint64_t, so the AArch64 representation is
>     canonical, and the AArch32 register accessors access a part
>     of that state (typically the lower 32 bits)
>   * registers which are banked S/NS in AArch32 are not necessarily
>     banked in AArch64
>
> AArch64 support is likely to land before your TrustZone stuff
> does so we need to make the two features work together cleanly.
>
> thanks
> -- PMM
>

I've briefly looked at the v8 ARM ARM. As I can see there is no banked 
system control registers in AArch64. Seems the concept is changed to 
provide separate registers for each meaningful execution level. Please, 
correct me if I am wrong.

So I think there shouldn't be "active" and "banked" fields for banked 
AArch32 CP15 registers as in my patch. Seems it is worth to use AArch64 
view of system control registers as a basis. That means there would be 
separate S and NS register fields in CPU state structure that will me 
mapped to separate AArch64 registers. ARMCPRegInfo structure would have 
additional field holding NS register state filed offset for AArch32 
banked registers.

Which branch in https://git.linaro.org/people/peter.maydell/qemu-arm.git 
repository holds the most actual A64 support?

Thanks!

Best regards,
Sergey Fedorov

Re: [Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers

[Peter Maydell]

On 20 December 2013 14:12, Fedorov Sergey <s.fedorov@samsung.com> wrote:
> I've briefly looked at the v8 ARM ARM. As I can see there is no banked
> system control registers in AArch64. Seems the concept is changed to provide
> separate registers for each meaningful execution level. Please, correct me
> if I am wrong.

Yes, I think this is generally correct.

> So I think there shouldn't be "active" and "banked" fields for banked
> AArch32 CP15 registers as in my patch. Seems it is worth to use AArch64 view
> of system control registers as a basis. That means there would be separate S
> and NS register fields in CPU state structure that will me mapped to
> separate AArch64 registers. ARMCPRegInfo structure would have additional
> field holding NS register state filed offset for AArch32 banked registers.

This sounds like it could work, though there are some wrinkles for
registers with readfns/writefns -- do we have extra s vs ns read/write
functions, or just one set of functions which has to look in env->ns to
figure out whether to use the S or NS version?

> Which branch in https://git.linaro.org/people/peter.maydell/qemu-arm.git
> repository holds the most actual A64 support?

It's still a work in progress so it depends what you want.
a64-third-fourth-set is the last set of patches that went out for
review, and should generally work for integer instructions.
a64-working is my work-in-progress branch so it will have the
most recent versions of everything, but it rebases frequently
and is liable to occasionally be broken...

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers

[Fedorov Sergey]

On 12/20/2013 06:33 PM, Peter Maydell wrote:
> On 20 December 2013 14:12, Fedorov Sergey <s.fedorov@samsung.com> wrote:
>> I've briefly looked at the v8 ARM ARM. As I can see there is no banked
>> system control registers in AArch64. Seems the concept is changed to provide
>> separate registers for each meaningful execution level. Please, correct me
>> if I am wrong.
> Yes, I think this is generally correct.
>
>> So I think there shouldn't be "active" and "banked" fields for banked
>> AArch32 CP15 registers as in my patch. Seems it is worth to use AArch64 view
>> of system control registers as a basis. That means there would be separate S
>> and NS register fields in CPU state structure that will me mapped to
>> separate AArch64 registers. ARMCPRegInfo structure would have additional
>> field holding NS register state filed offset for AArch32 banked registers.
> This sounds like it could work, though there are some wrinkles for
> registers with readfns/writefns -- do we have extra s vs ns read/write
> functions, or just one set of functions which has to look in env->ns to
> figure out whether to use the S or NS version?

I think if most read/write functions do the same work for both S/NS
versions then this code should not be duplicated.

>
>> Which branch in https://git.linaro.org/people/peter.maydell/qemu-arm.git
>> repository holds the most actual A64 support?
> It's still a work in progress so it depends what you want.
> a64-third-fourth-set is the last set of patches that went out for
> review, and should generally work for integer instructions.
> a64-working is my work-in-progress branch so it will have the
> most recent versions of everything, but it rebases frequently
> and is liable to occasionally be broken...

Thanks.

>
> thanks
> -- PMM
>

-- 
Best regards,
Sergey Fedorov

Re: [Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers

[Fedorov Sergey]

On 12/20/2013 06:38 PM, Fedorov Sergey wrote:
> On 12/20/2013 06:33 PM, Peter Maydell wrote:
>> On 20 December 2013 14:12, Fedorov Sergey <s.fedorov@samsung.com> wrote:
>>> I've briefly looked at the v8 ARM ARM. As I can see there is no banked
>>> system control registers in AArch64. Seems the concept is changed to provide
>>> separate registers for each meaningful execution level. Please, correct me
>>> if I am wrong.
>> Yes, I think this is generally correct.
>>
>>> So I think there shouldn't be "active" and "banked" fields for banked
>>> AArch32 CP15 registers as in my patch. Seems it is worth to use AArch64 view
>>> of system control registers as a basis. That means there would be separate S
>>> and NS register fields in CPU state structure that will me mapped to
>>> separate AArch64 registers. ARMCPRegInfo structure would have additional
>>> field holding NS register state filed offset for AArch32 banked registers.
>> This sounds like it could work, though there are some wrinkles for
>> registers with readfns/writefns -- do we have extra s vs ns read/write
>> functions, or just one set of functions which has to look in env->ns to
>> figure out whether to use the S or NS version?
> I think if most read/write functions do the same work for both S/NS
> versions then this code should not be duplicated.

But on the other hand, separate S/NS read/write functions could be
reused for AArch64 register descriptions that is separate for each EL...

>
>>> Which branch in https://git.linaro.org/people/peter.maydell/qemu-arm.git
>>> repository holds the most actual A64 support?
>> It's still a work in progress so it depends what you want.
>> a64-third-fourth-set is the last set of patches that went out for
>> review, and should generally work for integer instructions.
>> a64-working is my work-in-progress branch so it will have the
>> most recent versions of everything, but it rebases frequently
>> and is liable to occasionally be broken...
> Thanks.
>
>> thanks
>> -- PMM
>>

-- 
Best regards,
Sergey Fedorov

Re: [Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers

[Peter Crosthwaite]

On Sat, Dec 21, 2013 at 12:33 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 20 December 2013 14:12, Fedorov Sergey <s.fedorov@samsung.com> wrote:
>> I've briefly looked at the v8 ARM ARM. As I can see there is no banked
>> system control registers in AArch64. Seems the concept is changed to provide
>> separate registers for each meaningful execution level. Please, correct me
>> if I am wrong.

Isn't that just another definition of banking?

>
> Yes, I think this is generally correct.
>
>> So I think there shouldn't be "active" and "banked" fields for banked
>> AArch32 CP15 registers as in my patch.

I don't see how this extra scheme warrants the
active-set+mass-hotswapping impl. Why can the accessors just be aware
of the two different banking schemes at access time?

Seems it is worth to use AArch64 view
>> of system control registers as a basis. That means there would be separate S
>> and NS register fields in CPU state structure that will me mapped to
>> separate AArch64 registers. ARMCPRegInfo structure would have additional
>> field holding NS register state filed offset for AArch32 banked registers.
>
> This sounds like it could work,

I largely agree, except for the need for an active set.

> though there are some wrinkles for
> registers with readfns/writefns -- do we have extra s vs ns read/write
> functions, or just one set of functions which has to look in env->ns to
> figure out whether to use the S or NS version?

One set sounds better to me - if your resorting to open coding your
situation is probably complicated enough such that there is little you
can do in a data driven way. That said, it could be useful to define
both r.w handlers and fieldoffsets, and then the custom handlers
access do actual register read/write through a generic helper fn
(passed the CPRegInfo) that uses ->fieldoffset and is banking aware.
This handlers the common cases where helper functions are doing:

1: Normal access + some side effects
2: Manipulation of the read/written value on the way in/out.

without the need for all handlers having to open code banking functionality.

Regards,
Peter

>
>> Which branch in https://git.linaro.org/people/peter.maydell/qemu-arm.git
>> repository holds the most actual A64 support?
>
> It's still a work in progress so it depends what you want.
> a64-third-fourth-set is the last set of patches that went out for
> review, and should generally work for integer instructions.
> a64-working is my work-in-progress branch so it will have the
> most recent versions of everything, but it rebases frequently
> and is liable to occasionally be broken...
>
> thanks
> -- PMM
>

Re: [Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers

[Peter Maydell]

On 22 December 2013 01:08, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Sat, Dec 21, 2013 at 12:33 AM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> On 20 December 2013 14:12, Fedorov Sergey <s.fedorov@samsung.com> wrote:
>>> I've briefly looked at the v8 ARM ARM. As I can see there is no banked
>>> system control registers in AArch64. Seems the concept is changed to provide
>>> separate registers for each meaningful execution level. Please, correct me
>>> if I am wrong.
>
> Isn't that just another definition of banking?

No: the crn/crm/op values differ for eg ESR_EL1, ESR_EL2 and
ESR_EL3, and if you're in EL3 you can access all three of them
(the access permissions are what stop EL1 from messing with
EL3's ESR). Banked registers are where the same access
instruction reads or writes different state depending on the value
of something else (whether we're secure/nonsecure, in this case).

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers

[Fedorov Sergey]

On 12/22/2013 05:08 AM, Peter Crosthwaite wrote:
> On Sat, Dec 21, 2013 at 12:33 AM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> On 20 December 2013 14:12, Fedorov Sergey <s.fedorov@samsung.com> wrote:
>>> I've briefly looked at the v8 ARM ARM. As I can see there is no banked
>>> system control registers in AArch64. Seems the concept is changed to provide
>>> separate registers for each meaningful execution level. Please, correct me
>>> if I am wrong.
> Isn't that just another definition of banking?
>
>> Yes, I think this is generally correct.
>>
>>> So I think there shouldn't be "active" and "banked" fields for banked
>>> AArch32 CP15 registers as in my patch.
> I don't see how this extra scheme warrants the
> active-set+mass-hotswapping impl. Why can the accessors just be aware
> of the two different banking schemes at access time?
>
> Seems it is worth to use AArch64 view
>>> of system control registers as a basis. That means there would be separate S
>>> and NS register fields in CPU state structure that will me mapped to
>>> separate AArch64 registers. ARMCPRegInfo structure would have additional
>>> field holding NS register state filed offset for AArch32 banked registers.
>> This sounds like it could work,
> I largely agree, except for the need for an active set.
>
>> though there are some wrinkles for
>> registers with readfns/writefns -- do we have extra s vs ns read/write
>> functions, or just one set of functions which has to look in env->ns to
>> figure out whether to use the S or NS version?
> One set sounds better to me - if your resorting to open coding your
> situation is probably complicated enough such that there is little you
> can do in a data driven way. That said, it could be useful to define
> both r.w handlers and fieldoffsets, and then the custom handlers
> access do actual register read/write through a generic helper fn
> (passed the CPRegInfo) that uses ->fieldoffset and is banking aware.
> This handlers the common cases where helper functions are doing:
>
> 1: Normal access + some side effects
> 2: Manipulation of the read/written value on the way in/out.
>
> without the need for all handlers having to open code banking functionality.
>
> Regards,
> Peter
>
>>> Which branch in https://git.linaro.org/people/peter.maydell/qemu-arm.git
>>> repository holds the most actual A64 support?
>> It's still a work in progress so it depends what you want.
>> a64-third-fourth-set is the last set of patches that went out for
>> review, and should generally work for integer instructions.
>> a64-working is my work-in-progress branch so it will have the
>> most recent versions of everything, but it rebases frequently
>> and is liable to occasionally be broken...
>>
>> thanks
>> -- PMM
>>

Seems I agree with you.

Best regards,
Sergey Fedorov

Re: [Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers

[Fedorov Sergey]

On 12/20/2013 06:33 PM, Peter Maydell wrote:
> This sounds like it could work, though there are some wrinkles for
> registers with readfns/writefns -- do we have extra s vs ns read/write
> functions, or just one set of functions which has to look in env->ns to
> figure out whether to use the S or NS version?

What about defining a separate ARMCPRegInfo for each banked AArch32
system register? I think it would be close to AArch64 concept. It would
allow to use separate read/write handlers if necessary or reuse the same
handlers otherwise. When the handlers is not used, the translation code
would simply lookup the ARMCPRegInfo for corresponding secure state and
use the field offset.

Best regards,
Sergey Fedorov

Re: [Qemu-devel] [RFC PATCH 18/21] target-arm: switch banked CP registers

[Peter Maydell]

On 23 December 2013 07:43, Fedorov Sergey <s.fedorov@samsung.com> wrote:
>
> On 12/20/2013 06:33 PM, Peter Maydell wrote:
>> This sounds like it could work, though there are some wrinkles for
>> registers with readfns/writefns -- do we have extra s vs ns read/write
>> functions, or just one set of functions which has to look in env->ns to
>> figure out whether to use the S or NS version?
>
> What about defining a separate ARMCPRegInfo for each banked AArch32
> system register? I think it would be close to AArch64 concept. It would
> allow to use separate read/write handlers if necessary or reuse the same
> handlers otherwise. When the handlers is not used, the translation code
> would simply lookup the ARMCPRegInfo for corresponding secure state and
> use the field offset.

Yes, I was thinking about that the other day -- add the S/NS to the
set of things in the hash table key, and have the 32 bit MCR/MRC
code pass in the right value to get the correct banked register out.
(We'd have to make all the non-banked registers go into the
hashtable twice, though, but that's not a big deal I think.)

thanks
-- PMM

[Qemu-devel] [RFC PATCH 19/21] target-arm: add MVBAR support

[Sergey Fedorov]

MVBAR register provides an exception vector base address for exceptions
taking to CPU monitor mode.

Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
---
 target-arm/cpu.h    |    1 +
 target-arm/helper.c |   16 +++++++---------
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index b4500b4..3e5b860 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -210,6 +210,7 @@ typedef struct CPUARMState {
         uint32_t c9_pmuserenr; /* perf monitor user enable */
         uint32_t c9_pminten; /* perf monitor interrupt enables */
         BANKED_CP_REG(uint32_t, c12_vbar); /* vector base address register */
+        uint32_t c12_mvbar; /* monitor vector base address register */
         BANKED_CP_REG(uint32_t, c13_fcse); /* FCSE PID.  */
         BANKED_CP_REG(uint32_t, c13_context); /* Context ID.  */
         BANKED_CP_REG(uint32_t, c13_tls1); /* User RW Thread register.  */
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 7bfadb0..582de74 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1568,7 +1568,7 @@ static int scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
                       uint64_t value)
 {
-    env->cp15.c12_vbar = value & ~0x1Ful;
+    CPREG_FIELD32(env, ri) = value & ~0x1Ful;
     return 0;
 }
 
@@ -1589,6 +1589,9 @@ static const ARMCPRegInfo tz_cp_reginfo[] = {
       .access = PL1_RW, .type = ARM_CP_BANKED, .writefn = vbar_write,
       .fieldoffset = offsetof(CPUARMState, cp15.c12_vbar),
       .resetvalue = 0 },
+    { .name = "MVBAR", .cp = 15, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 1,
+      .access = PL3_RW, .resetvalue = 0, .writefn = vbar_write,
+      .fieldoffset = offsetof(CPUARMState, cp15.c12_mvbar) },
     { .name = "SDER", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 1,
       .access = PL3_RW, .resetvalue = 0,
       .fieldoffset = offsetof(CPUARMState, cp15.c1_sder) },
@@ -2630,17 +2633,12 @@ void arm_cpu_do_interrupt(CPUState *cs)
         return; /* Never happens.  Keep compiler happy.  */
     }
     /* High vectors.  */
-    if (env->cp15.c1_sys & (1 << 13)) {
+    if (new_mode == ARM_CPU_MODE_MON) {
+        addr += env->cp15.c12_mvbar;
+    } else if (env->cp15.c1_sys & (1 << 13)) {
         /* when enabled, base address cannot be remapped.  */
         addr += 0xffff0000;
     } else {
-        /* ARM v7 architectures provide a vector base address register to remap
-         * the interrupt vector table.
-         * This register is only followed in non-monitor mode, and has a secure
-         * and un-secure copy. Since the cpu is always in a un-secure operation
-         * and is never in monitor mode this feature is always active.
-         * Note: only bits 31:5 are valid.
-         */
         addr += env->cp15.c12_vbar;
     }
     switch_mode (env, new_mode);
-- 
1.7.9.5

Re: [Qemu-devel] [RFC PATCH 19/21] target-arm: add MVBAR support

[Peter Crosthwaite]

On Tue, Dec 3, 2013 at 6:48 PM, Sergey Fedorov <s.fedorov@samsung.com> wrote:
> MVBAR register provides an exception vector base address for exceptions
> taking to CPU monitor mode.
>
> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
> ---
>  target-arm/cpu.h    |    1 +
>  target-arm/helper.c |   16 +++++++---------
>  2 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index b4500b4..3e5b860 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -210,6 +210,7 @@ typedef struct CPUARMState {
>          uint32_t c9_pmuserenr; /* perf monitor user enable */
>          uint32_t c9_pminten; /* perf monitor interrupt enables */
>          BANKED_CP_REG(uint32_t, c12_vbar); /* vector base address register */
> +        uint32_t c12_mvbar; /* monitor vector base address register */
>          BANKED_CP_REG(uint32_t, c13_fcse); /* FCSE PID.  */
>          BANKED_CP_REG(uint32_t, c13_context); /* Context ID.  */
>          BANKED_CP_REG(uint32_t, c13_tls1); /* User RW Thread register.  */
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 7bfadb0..582de74 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1568,7 +1568,7 @@ static int scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>  static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                        uint64_t value)
>  {
> -    env->cp15.c12_vbar = value & ~0x1Ful;
> +    CPREG_FIELD32(env, ri) = value & ~0x1Ful;
>      return 0;
>  }
>
> @@ -1589,6 +1589,9 @@ static const ARMCPRegInfo tz_cp_reginfo[] = {
>        .access = PL1_RW, .type = ARM_CP_BANKED, .writefn = vbar_write,
>        .fieldoffset = offsetof(CPUARMState, cp15.c12_vbar),
>        .resetvalue = 0 },
> +    { .name = "MVBAR", .cp = 15, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 1,
> +      .access = PL3_RW, .resetvalue = 0, .writefn = vbar_write,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c12_mvbar) },
>      { .name = "SDER", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 1,
>        .access = PL3_RW, .resetvalue = 0,
>        .fieldoffset = offsetof(CPUARMState, cp15.c1_sder) },
> @@ -2630,17 +2633,12 @@ void arm_cpu_do_interrupt(CPUState *cs)
>          return; /* Never happens.  Keep compiler happy.  */
>      }
>      /* High vectors.  */

Cut this comment ....

> -    if (env->cp15.c1_sys & (1 << 13)) {
> +    if (new_mode == ARM_CPU_MODE_MON) {
> +        addr += env->cp15.c12_mvbar;
> +    } else if (env->cp15.c1_sys & (1 << 13)) {

and paste it back here. Your prepend of the monitor logic makes it
stale in that location.

Regards,
Peter

>          /* when enabled, base address cannot be remapped.  */
>          addr += 0xffff0000;
>      } else {
> -        /* ARM v7 architectures provide a vector base address register to remap
> -         * the interrupt vector table.
> -         * This register is only followed in non-monitor mode, and has a secure
> -         * and un-secure copy. Since the cpu is always in a un-secure operation
> -         * and is never in monitor mode this feature is always active.
> -         * Note: only bits 31:5 are valid.
> -         */
>          addr += env->cp15.c12_vbar;
>      }
>      switch_mode (env, new_mode);
> --
> 1.7.9.5
>
>

[Qemu-devel] [RFC PATCH 20/21] target-arm: implement SMC instruction

[Sergey Fedorov]

SMC instruction is implemented similar to SVC instruction. When
executing SMC instruction from monitor CPU mode SCR.NS bit is reset.

Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
---
 target-arm/cpu.h       |    1 +
 target-arm/helper.c    |   11 +++++++++++
 target-arm/translate.c |   37 +++++++++++++++++++++++++++----------
 3 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 3e5b860..556d630 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -49,6 +49,7 @@
 #define EXCP_EXCEPTION_EXIT  8   /* Return from v7M exception.  */
 #define EXCP_KERNEL_TRAP     9   /* Jumped to kernel code page.  */
 #define EXCP_STREX          10
+#define EXCP_SMC            11   /* secure monitor call */
 
 #define ARMV7M_EXCP_RESET   1
 #define ARMV7M_EXCP_NMI     2
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 582de74..ba442c0 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2628,6 +2628,12 @@ void arm_cpu_do_interrupt(CPUState *cs)
         mask = CPSR_A | CPSR_I | CPSR_F;
         offset = 4;
         break;
+    case EXCP_SMC:
+        new_mode = ARM_CPU_MODE_MON;
+        addr = 0x08;
+        mask = CPSR_A | CPSR_I | CPSR_F;
+        offset = 0;
+        break;
     default:
         cpu_abort(env, "Unhandled exception 0x%x\n", env->exception_index);
         return; /* Never happens.  Keep compiler happy.  */
@@ -2641,6 +2647,11 @@ void arm_cpu_do_interrupt(CPUState *cs)
     } else {
         addr += env->cp15.c12_vbar;
     }
+
+    if ((env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_MON) {
+        env->cp15.c1_scr &= ~1 /* NS */;
+    }
+
     switch_mode (env, new_mode);
     env->spsr = cpsr_read(env);
     /* Clear IT bits.  */
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 345866c..b81f09a 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -66,6 +66,7 @@ static uint32_t gen_opc_condexec_bits[OPC_BUF_SIZE];
    conditional execution state has been updated.  */
 #define DISAS_WFI 4
 #define DISAS_SWI 5
+#define DISAS_SMC 6
 
 TCGv_ptr cpu_env;
 /* We reuse the same 64-bit temporaries for efficiency.  */
@@ -7105,15 +7106,21 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
             store_reg(s, rd, tmp);
             break;
         case 7:
-            /* SMC instruction (op1 == 3)
-               and undefined instructions (op1 == 0 || op1 == 2)
-               will trap */
-            if (op1 != 1) {
+            if (op1 == 1) {
+                /* bkpt */
+                ARCH(5);
+                gen_exception_insn(s, 4, EXCP_BKPT);
+            } else if (op1 == 3) {
+                /* smi/smc */
+                if (!arm_feature(env, ARM_FEATURE_TRUSTZONE) || IS_USER(s)) {
+                    goto illegal_op;
+                }
+                gen_set_pc_im(s, s->pc);
+                s->is_jmp = DISAS_SMC;
+                break;
+            } else {
                 goto illegal_op;
             }
-            /* bkpt */
-            ARCH(5);
-            gen_exception_insn(s, 4, EXCP_BKPT);
             break;
         case 0x8: /* signed multiply */
         case 0xa:
@@ -8885,9 +8892,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
 
                 if (insn & (1 << 26)) {
                     /* Secure monitor call (v6Z) */
-                    qemu_log_mask(LOG_UNIMP,
-                                  "arm: unimplemented secure monitor call\n");
-                    goto illegal_op; /* not implemented.  */
+                    if (!arm_feature(env, ARM_FEATURE_TRUSTZONE) ||
+                            IS_USER(s)) {
+                        goto illegal_op;
+                    }
+                    gen_set_pc_im(s, s->pc);
+                    s->is_jmp = DISAS_SMC;
                 } else {
                     op = (insn >> 20) & 7;
                     switch (op) {
@@ -10284,6 +10294,8 @@ static inline void gen_intermediate_code_internal(ARMCPU *cpu,
             gen_set_condexec(dc);
             if (dc->is_jmp == DISAS_SWI) {
                 gen_exception(EXCP_SWI);
+            } else if (dc->is_jmp == DISAS_SMC) {
+                gen_exception(EXCP_SMC);
             } else {
                 gen_exception(EXCP_DEBUG);
             }
@@ -10296,6 +10308,8 @@ static inline void gen_intermediate_code_internal(ARMCPU *cpu,
         gen_set_condexec(dc);
         if (dc->is_jmp == DISAS_SWI && !dc->condjmp) {
             gen_exception(EXCP_SWI);
+        } else if (dc->is_jmp == DISAS_SMC && !dc->condjmp) {
+            gen_exception(EXCP_SMC);
         } else {
             /* FIXME: Single stepping a WFI insn will not halt
                the CPU.  */
@@ -10330,6 +10344,9 @@ static inline void gen_intermediate_code_internal(ARMCPU *cpu,
         case DISAS_SWI:
             gen_exception(EXCP_SWI);
             break;
+        case DISAS_SMC:
+            gen_exception(EXCP_SMC);
+            break;
         }
         if (dc->condjmp) {
             gen_set_label(dc->condlabel);
-- 
1.7.9.5

[Qemu-devel] [RFC PATCH 21/21] target-arm: implement IRQ/FIQ routing to Monitor mode

[Sergey Fedorov]

SCR.{IRQ/FIQ} bits allows to route IRQ/FIQ exceptions to monitor CPU
mode. When taking IRQ exception to monitor mode FIQ exception is
additionally masked.

Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
---
 target-arm/helper.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index ba442c0..6472c09 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2620,12 +2620,19 @@ void arm_cpu_do_interrupt(CPUState *cs)
         /* Disable IRQ and imprecise data aborts.  */
         mask = CPSR_A | CPSR_I;
         offset = 4;
+        if (env->cp15.c1_scr & (1 << 1)) {
+            new_mode = ARM_CPU_MODE_MON;
+            mask |= CPSR_F;
+        }
         break;
     case EXCP_FIQ:
         new_mode = ARM_CPU_MODE_FIQ;
         addr = 0x1c;
         /* Disable FIQ, IRQ and imprecise data aborts.  */
         mask = CPSR_A | CPSR_I | CPSR_F;
+        if (env->cp15.c1_scr & (1 << 2)) {
+            new_mode = ARM_CPU_MODE_MON;
+        }
         offset = 4;
         break;
     case EXCP_SMC:
-- 
1.7.9.5

Re: [Qemu-devel] [RFC PATCH 00/21] target-arm: add CPU core TrustZone support

[Fedorov Sergey]

On 12/03/2013 12:48 PM, Sergey Fedorov wrote:
> This patch set implements a basic support of CPU core TrustZone feature. The
> following major functionalities are implemented:
>    * CPU monitor mode
>    * Separate code translation for each secure state
>    * CPACR & NSACR co-processor access control
>    * Separate TLB for each secure state
>    * Co-processor register banking
>    * SMC instruction
>    * FIQ/IRQ routing to monitor mode
>
> There is no support for banked co-processor register migration, save/load its
> VM state yet. That is an open question how to implement this functionality. Any
> suggestions is greatly appreciated.
>
> This patch set is a request for comments for the proof of concept.
>
> Sergey Fedorov (18):
>    target-arm: move SCR & VBAR into TrustZone register list
>    target-arm: adjust TTBCR for TrustZone feature
>    target-arm: add arm_is_secure() helper
>    target-arm: reject switching to monitor mode from non-secure state
>    target-arm: adjust arm_current_pl() for TrustZone
>    target-arm: adjust SCR CP15 register access rights
>    target-arm: add non-secure Translation Block flag
>    target-arm: implement CPACR register logic
>    target-arm: add NSACR support
>    target-arm: add SDER definition
>    target-arm: split TLB for secure state
>    target-arm: add banked coprocessor register type
>    target-arm: convert appropriate coprocessor registers to banked type
>    target-arm: use c13_context field for CONTEXTIDR
>    target-arm: switch banked CP registers
>    target-arm: add MVBAR support
>    target-arm: implement SMC instruction
>    target-arm: implement IRQ/FIQ routing to Monitor mode
>
> Svetlana Fedoseeva (3):
>    target-arm: add TrustZone CPU feature
>    target-arm: preserve RAO/WI bits of ARMv7 SCTLR
>    target-arm: add CPU Monitor mode
>
>   target-arm/cpu.c       |    6 +-
>   target-arm/cpu.h       |  126 ++++++++++++----
>   target-arm/helper.c    |  308 +++++++++++++++++++++++++++++---------
>   target-arm/machine.c   |   12 +-
>   target-arm/translate.c |  388 ++++++++++++++++++++++++++++++------------------
>   target-arm/translate.h |    2 +
>   6 files changed, 585 insertions(+), 257 deletions(-)
>

We'd like this patch series finally to be merged into mainstream. What 
should be done to achieve this goal?

Best regards,
Sergey Fedorov

Re: [Qemu-devel] [RFC PATCH 00/21] target-arm: add CPU core TrustZone support

[Peter Crosthwaite]

Hi Sergey,

On Wed, Dec 4, 2013 at 8:08 PM, Fedorov Sergey <s.fedorov@samsung.com> wrote:
> On 12/03/2013 12:48 PM, Sergey Fedorov wrote:
>>
>> This patch set implements a basic support of CPU core TrustZone feature.
>> The
>> following major functionalities are implemented:
>>    * CPU monitor mode
>>    * Separate code translation for each secure state
>>    * CPACR & NSACR co-processor access control
>>    * Separate TLB for each secure state
>>    * Co-processor register banking
>>    * SMC instruction
>>    * FIQ/IRQ routing to monitor mode
>>
>> There is no support for banked co-processor register migration, save/load
>> its
>> VM state yet. That is an open question how to implement this
>> functionality. Any
>> suggestions is greatly appreciated.
>>
>> This patch set is a request for comments for the proof of concept.
>>
>> Sergey Fedorov (18):
>>    target-arm: move SCR & VBAR into TrustZone register list
>>    target-arm: adjust TTBCR for TrustZone feature
>>    target-arm: add arm_is_secure() helper
>>    target-arm: reject switching to monitor mode from non-secure state
>>    target-arm: adjust arm_current_pl() for TrustZone
>>    target-arm: adjust SCR CP15 register access rights
>>    target-arm: add non-secure Translation Block flag
>>    target-arm: implement CPACR register logic
>>    target-arm: add NSACR support
>>    target-arm: add SDER definition
>>    target-arm: split TLB for secure state
>>    target-arm: add banked coprocessor register type
>>    target-arm: convert appropriate coprocessor registers to banked type
>>    target-arm: use c13_context field for CONTEXTIDR
>>    target-arm: switch banked CP registers
>>    target-arm: add MVBAR support
>>    target-arm: implement SMC instruction
>>    target-arm: implement IRQ/FIQ routing to Monitor mode
>>
>> Svetlana Fedoseeva (3):
>>    target-arm: add TrustZone CPU feature
>>    target-arm: preserve RAO/WI bits of ARMv7 SCTLR
>>    target-arm: add CPU Monitor mode
>>
>>   target-arm/cpu.c       |    6 +-
>>   target-arm/cpu.h       |  126 ++++++++++++----
>>   target-arm/helper.c    |  308 +++++++++++++++++++++++++++++---------
>>   target-arm/machine.c   |   12 +-
>>   target-arm/translate.c |  388
>> ++++++++++++++++++++++++++++++------------------
>>   target-arm/translate.h |    2 +
>>   6 files changed, 585 insertions(+), 257 deletions(-)
>>
>
> We'd like this patch series finally to be merged into mainstream. What
> should be done to achieve this goal?
>

Send patch series' just like this and either action or challenge
reviewer comments (as you have already done). Next time, you can drop
the RFC, that means you intend the series for review+merge. If you get
no reply after a week ping the series.

I'm about half way through the series on my first pass. And I guess
PMM has had a look as he has commented on one my comments.

A 21 patch series with +500/-250 is long, and queue maintainers are
generally hesitant to take partial series (as its often contrary to
author intention). Things generally go faster with smaller series. You
have some low-complexity cleanup type patches that dont add new
features than you could potentially send as a smaller initial series
to get upstream quickly, then take more time on the harder stuff as
follow up series with reduced patch count.

Regards,
Peter

> Best regards,
> Sergey Fedorov
>

Re: [Qemu-devel] [RFC PATCH 00/21] target-arm: add CPU core TrustZone support

[Peter Maydell]

On 4 December 2013 10:08, Fedorov Sergey <s.fedorov@samsung.com> wrote:
> On 12/03/2013 12:48 PM, Sergey Fedorov wrote:
>>
>> This patch set implements a basic support of CPU core TrustZone feature.

> We'd like this patch series finally to be merged into mainstream. What
> should be done to achieve this goal?

I'd like to see TZ support in mainline too. The high level answer
is that it needs to get code reviewed, and you need to fix issues
that are raised in code review. Unfortunately my review queue is
currently pretty full (it has Allwinner board support, DIGIC board
support, a bunch of Cadence fixes, ARMv8 32 bit new instructions
and the A64 64 bit instruction support in it, all of which are fairly
big patchsets), so it may take me a little while to get to this
patchset. It is on my todo list though, so it won't get forgotten :-)

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH 00/21] target-arm: add CPU core TrustZone support

[Fedorov Sergey]

On 12/04/2013 03:13 PM, Peter Maydell wrote:
> On 4 December 2013 10:08, Fedorov Sergey <s.fedorov@samsung.com> wrote:
>> On 12/03/2013 12:48 PM, Sergey Fedorov wrote:
>>> This patch set implements a basic support of CPU core TrustZone feature.
>> We'd like this patch series finally to be merged into mainstream. What
>> should be done to achieve this goal?
> I'd like to see TZ support in mainline too.

That is the most important for me now :-) So I will try these patches to 
get shape suitable for mainline. Thanks!

> The high level answer
> is that it needs to get code reviewed, and you need to fix issues
> that are raised in code review. Unfortunately my review queue is
> currently pretty full (it has Allwinner board support, DIGIC board
> support, a bunch of Cadence fixes, ARMv8 32 bit new instructions
> and the A64 64 bit instruction support in it, all of which are fairly
> big patchsets), so it may take me a little while to get to this
> patchset. It is on my todo list though, so it won't get forgotten :-)
>
> thanks
> -- PMM
>

Best regards,
Sergey Fedorov

Re: [Qemu-devel] [RFC PATCH 00/21] target-arm: add CPU core TrustZone support

[Peter Crosthwaite]

On Wed, Dec 4, 2013 at 10:48 PM, Fedorov Sergey <s.fedorov@samsung.com> wrote:
>
> On 12/04/2013 03:13 PM, Peter Maydell wrote:
>>
>> On 4 December 2013 10:08, Fedorov Sergey <s.fedorov@samsung.com> wrote:
>>>
>>> On 12/03/2013 12:48 PM, Sergey Fedorov wrote:
>>>>
>>>> This patch set implements a basic support of CPU core TrustZone feature.
>>>
>>> We'd like this patch series finally to be merged into mainstream. What
>>> should be done to achieve this goal?
>>
>> I'd like to see TZ support in mainline too.
>
>
> That is the most important for me now :-) So I will try these patches to get
> shape suitable for mainline. Thanks!
>
>
>> The high level answer
>> is that it needs to get code reviewed, and you need to fix issues
>> that are raised in code review.

I'm done with first round review here - sorry about the delay. Mostly
trivials but two bigger points:

1: The name "Trustzone" does not match ARM doco which uses "security extensions"
2: I think that banking scheme where you maintain active copies is a
bit awkward.

Regards,
Peter

>> Unfortunately my review queue is
>> currently pretty full (it has Allwinner board support, DIGIC board
>> support, a bunch of Cadence fixes, ARMv8 32 bit new instructions
>> and the A64 64 bit instruction support in it, all of which are fairly
>> big patchsets), so it may take me a little while to get to this
>> patchset. It is on my todo list though, so it won't get forgotten :-)
>>



>> thanks
>> -- PMM
>>
>
> Best regards,
> Sergey Fedorov
>