[Qemu-devel] [PATCH 00/15] v7M: cleanups and bugfixes prior to v8M

[Qemu-devel] [PATCH 00/15] v7M: cleanups and bugfixes prior to v8M

[Peter Maydell]

(This is 2.11 material, obviously, but it's a coherent and
large enough set of patches that I figured I might as well
push it out for review now.)

This patchset is a collection of cleanups, bugfixes, etc to
the existing v7M code which are either necessary preliminary
to implementing v8M or just things I noticed along the way.
The non-trivial stuff is:
 * migration for M profile is shifted to not use read_cpsr()
   and write_cpsr() which assume A profile semantics
   (back compatibility with old migration state is maintained)
 * we implement the "user accesses should BusFault" behaviour
   for the memory mapped registers in the SCS, though this
   won't actually kick in until we turn MEMTX_ERROR into a
   BusFault (I have patches for that)

thanks
-- PMM

Peter Maydell (15):
  target/arm: Use MMUAccessType enum rather than int
  target/arm: Don't trap WFI/WFE for M profile
  target/arm: Consolidate PMSA handling in get_phys_addr()
  target/arm: Tighten up Thumb decode where new v8M insns will be
  hw/intc/armv7m_nvic.c: Remove out of date comment
  target/arm: Remove incorrect comment about MPU_CTRL
  target/arm: Fix outdated comment about exception exit
  target/arm: Define and use XPSR bit masks
  target/arm: Don't store M profile PRIMASK and FAULTMASK in daif
  target/arm: Don't use cpsr_write/cpsr_read to transfer M profile XPSR
  target/arm: Make arm_cpu_dump_state() handle the M-profile XPSR
  target/arm: Don't calculate lr in arm_v7m_cpu_do_interrupt() until
    needed
  target/arm: Create and use new function arm_v7m_is_handler_mode()
  armv7m_nvic.h: Move from include/hw/arm to include/hw/intc
  nvic: Implement "user accesses BusFault" SCS region behaviour

 hw/intc/armv7m_nvic.c                  |  68 +++++++++++-------
 include/hw/arm/armv7m.h                |   2 +-
 include/hw/{arm => intc}/armv7m_nvic.h |   0
 target/arm/cpu.c                       |   5 --
 target/arm/cpu.h                       |  54 ++++++++++----
 target/arm/helper.c                    | 124 ++++++++++++++++-----------------
 target/arm/internals.h                 |   3 +-
 target/arm/machine.c                   |  54 +++++++++++++-
 target/arm/op_helper.c                 |   5 ++
 target/arm/translate.c                 | 106 +++++++++++++++++++++-------
 10 files changed, 286 insertions(+), 135 deletions(-)
 rename include/hw/{arm => intc}/armv7m_nvic.h (100%)

-- 
2.7.4

[Qemu-devel] [PATCH 01/15] target/arm: Use MMUAccessType enum rather than int

[Peter Maydell]

In the ARM get_phys_addr() code, switch to using the MMUAccessType
enum and its MMU_* values rather than int and literal 0/1/2.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c    | 30 +++++++++++++++---------------
 target/arm/internals.h |  3 ++-
 2 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index fa60040..b78d277 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -20,13 +20,13 @@
 
 #ifndef CONFIG_USER_ONLY
 static bool get_phys_addr(CPUARMState *env, target_ulong address,
-                          int access_type, ARMMMUIdx mmu_idx,
+                          MMUAccessType access_type, ARMMMUIdx mmu_idx,
                           hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
                           target_ulong *page_size, uint32_t *fsr,
                           ARMMMUFaultInfo *fi);
 
 static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
-                               int access_type, ARMMMUIdx mmu_idx,
+                               MMUAccessType access_type, ARMMMUIdx mmu_idx,
                                hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot,
                                target_ulong *page_size_ptr, uint32_t *fsr,
                                ARMMMUFaultInfo *fi);
@@ -2135,7 +2135,7 @@ static CPAccessResult ats_access(CPUARMState *env, const ARMCPRegInfo *ri,
 }
 
 static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
-                             int access_type, ARMMMUIdx mmu_idx)
+                             MMUAccessType access_type, ARMMMUIdx mmu_idx)
 {
     hwaddr phys_addr;
     target_ulong page_size;
@@ -2194,7 +2194,7 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
 
 static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 {
-    int access_type = ri->opc2 & 1;
+    MMUAccessType access_type = ri->opc2 & 1 ? MMU_DATA_STORE : MMU_DATA_LOAD;
     uint64_t par64;
     ARMMMUIdx mmu_idx;
     int el = arm_current_el(env);
@@ -2253,7 +2253,7 @@ static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 static void ats1h_write(CPUARMState *env, const ARMCPRegInfo *ri,
                         uint64_t value)
 {
-    int access_type = ri->opc2 & 1;
+    MMUAccessType access_type = ri->opc2 & 1 ? MMU_DATA_STORE : MMU_DATA_LOAD;
     uint64_t par64;
 
     par64 = do_ats_write(env, value, access_type, ARMMMUIdx_S2NS);
@@ -2273,7 +2273,7 @@ static CPAccessResult at_s1e2_access(CPUARMState *env, const ARMCPRegInfo *ri,
 static void ats_write64(CPUARMState *env, const ARMCPRegInfo *ri,
                         uint64_t value)
 {
-    int access_type = ri->opc2 & 1;
+    MMUAccessType access_type = ri->opc2 & 1 ? MMU_DATA_STORE : MMU_DATA_LOAD;
     ARMMMUIdx mmu_idx;
     int secure = arm_is_secure_below_el3(env);
 
@@ -7510,7 +7510,7 @@ static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure,
 }
 
 static bool get_phys_addr_v5(CPUARMState *env, uint32_t address,
-                             int access_type, ARMMMUIdx mmu_idx,
+                             MMUAccessType access_type, ARMMMUIdx mmu_idx,
                              hwaddr *phys_ptr, int *prot,
                              target_ulong *page_size, uint32_t *fsr,
                              ARMMMUFaultInfo *fi)
@@ -7626,7 +7626,7 @@ do_fault:
 }
 
 static bool get_phys_addr_v6(CPUARMState *env, uint32_t address,
-                             int access_type, ARMMMUIdx mmu_idx,
+                             MMUAccessType access_type, ARMMMUIdx mmu_idx,
                              hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
                              target_ulong *page_size, uint32_t *fsr,
                              ARMMMUFaultInfo *fi)
@@ -7733,7 +7733,7 @@ static bool get_phys_addr_v6(CPUARMState *env, uint32_t address,
         if (pxn && !regime_is_user(env, mmu_idx)) {
             xn = 1;
         }
-        if (xn && access_type == 2)
+        if (xn && access_type == MMU_INST_FETCH)
             goto do_fault;
 
         if (arm_feature(env, ARM_FEATURE_V6K) &&
@@ -7848,7 +7848,7 @@ static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
 }
 
 static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
-                               int access_type, ARMMMUIdx mmu_idx,
+                               MMUAccessType access_type, ARMMMUIdx mmu_idx,
                                hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot,
                                target_ulong *page_size_ptr, uint32_t *fsr,
                                ARMMMUFaultInfo *fi)
@@ -8256,7 +8256,7 @@ static inline bool m_is_system_region(CPUARMState *env, uint32_t address)
 }
 
 static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
-                                 int access_type, ARMMMUIdx mmu_idx,
+                                 MMUAccessType access_type, ARMMMUIdx mmu_idx,
                                  hwaddr *phys_ptr, int *prot, uint32_t *fsr)
 {
     ARMCPU *cpu = arm_env_get_cpu(env);
@@ -8415,7 +8415,7 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
 }
 
 static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
-                                 int access_type, ARMMMUIdx mmu_idx,
+                                 MMUAccessType access_type, ARMMMUIdx mmu_idx,
                                  hwaddr *phys_ptr, int *prot, uint32_t *fsr)
 {
     int n;
@@ -8442,7 +8442,7 @@ static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
         return true;
     }
 
-    if (access_type == 2) {
+    if (access_type == MMU_INST_FETCH) {
         mask = env->cp15.pmsav5_insn_ap;
     } else {
         mask = env->cp15.pmsav5_data_ap;
@@ -8513,7 +8513,7 @@ static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
  * @fsr: set to the DFSR/IFSR value on failure
  */
 static bool get_phys_addr(CPUARMState *env, target_ulong address,
-                          int access_type, ARMMMUIdx mmu_idx,
+                          MMUAccessType access_type, ARMMMUIdx mmu_idx,
                           hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
                           target_ulong *page_size, uint32_t *fsr,
                           ARMMMUFaultInfo *fi)
@@ -8626,7 +8626,7 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
  * fsr with ARM DFSR/IFSR fault register format value on failure.
  */
 bool arm_tlb_fill(CPUState *cs, vaddr address,
-                  int access_type, int mmu_idx, uint32_t *fsr,
+                  MMUAccessType access_type, int mmu_idx, uint32_t *fsr,
                   ARMMMUFaultInfo *fi)
 {
     ARMCPU *cpu = ARM_CPU(cs);
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 1f6efef..bb06946 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -457,7 +457,8 @@ struct ARMMMUFaultInfo {
 };
 
 /* Do a page table walk and add page to TLB if possible */
-bool arm_tlb_fill(CPUState *cpu, vaddr address, int rw, int mmu_idx,
+bool arm_tlb_fill(CPUState *cpu, vaddr address,
+                  MMUAccessType access_type, int mmu_idx,
                   uint32_t *fsr, ARMMMUFaultInfo *fi);
 
 /* Return true if the stage 1 translation regime is using LPAE format page
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH 01/15] target/arm: Use MMUAccessType enum rather than int

[Edgar E. Iglesias]

On Wed, Aug 02, 2017 at 05:43:47PM +0100, Peter Maydell wrote:
> In the ARM get_phys_addr() code, switch to using the MMUAccessType
> enum and its MMU_* values rather than int and literal 0/1/2.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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


> ---
>  target/arm/helper.c    | 30 +++++++++++++++---------------
>  target/arm/internals.h |  3 ++-
>  2 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index fa60040..b78d277 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -20,13 +20,13 @@
>  
>  #ifndef CONFIG_USER_ONLY
>  static bool get_phys_addr(CPUARMState *env, target_ulong address,
> -                          int access_type, ARMMMUIdx mmu_idx,
> +                          MMUAccessType access_type, ARMMMUIdx mmu_idx,
>                            hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
>                            target_ulong *page_size, uint32_t *fsr,
>                            ARMMMUFaultInfo *fi);
>  
>  static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> -                               int access_type, ARMMMUIdx mmu_idx,
> +                               MMUAccessType access_type, ARMMMUIdx mmu_idx,
>                                 hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot,
>                                 target_ulong *page_size_ptr, uint32_t *fsr,
>                                 ARMMMUFaultInfo *fi);
> @@ -2135,7 +2135,7 @@ static CPAccessResult ats_access(CPUARMState *env, const ARMCPRegInfo *ri,
>  }
>  
>  static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
> -                             int access_type, ARMMMUIdx mmu_idx)
> +                             MMUAccessType access_type, ARMMMUIdx mmu_idx)
>  {
>      hwaddr phys_addr;
>      target_ulong page_size;
> @@ -2194,7 +2194,7 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
>  
>  static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>  {
> -    int access_type = ri->opc2 & 1;
> +    MMUAccessType access_type = ri->opc2 & 1 ? MMU_DATA_STORE : MMU_DATA_LOAD;
>      uint64_t par64;
>      ARMMMUIdx mmu_idx;
>      int el = arm_current_el(env);
> @@ -2253,7 +2253,7 @@ static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>  static void ats1h_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                          uint64_t value)
>  {
> -    int access_type = ri->opc2 & 1;
> +    MMUAccessType access_type = ri->opc2 & 1 ? MMU_DATA_STORE : MMU_DATA_LOAD;
>      uint64_t par64;
>  
>      par64 = do_ats_write(env, value, access_type, ARMMMUIdx_S2NS);
> @@ -2273,7 +2273,7 @@ static CPAccessResult at_s1e2_access(CPUARMState *env, const ARMCPRegInfo *ri,
>  static void ats_write64(CPUARMState *env, const ARMCPRegInfo *ri,
>                          uint64_t value)
>  {
> -    int access_type = ri->opc2 & 1;
> +    MMUAccessType access_type = ri->opc2 & 1 ? MMU_DATA_STORE : MMU_DATA_LOAD;
>      ARMMMUIdx mmu_idx;
>      int secure = arm_is_secure_below_el3(env);
>  
> @@ -7510,7 +7510,7 @@ static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure,
>  }
>  
>  static bool get_phys_addr_v5(CPUARMState *env, uint32_t address,
> -                             int access_type, ARMMMUIdx mmu_idx,
> +                             MMUAccessType access_type, ARMMMUIdx mmu_idx,
>                               hwaddr *phys_ptr, int *prot,
>                               target_ulong *page_size, uint32_t *fsr,
>                               ARMMMUFaultInfo *fi)
> @@ -7626,7 +7626,7 @@ do_fault:
>  }
>  
>  static bool get_phys_addr_v6(CPUARMState *env, uint32_t address,
> -                             int access_type, ARMMMUIdx mmu_idx,
> +                             MMUAccessType access_type, ARMMMUIdx mmu_idx,
>                               hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
>                               target_ulong *page_size, uint32_t *fsr,
>                               ARMMMUFaultInfo *fi)
> @@ -7733,7 +7733,7 @@ static bool get_phys_addr_v6(CPUARMState *env, uint32_t address,
>          if (pxn && !regime_is_user(env, mmu_idx)) {
>              xn = 1;
>          }
> -        if (xn && access_type == 2)
> +        if (xn && access_type == MMU_INST_FETCH)
>              goto do_fault;
>  
>          if (arm_feature(env, ARM_FEATURE_V6K) &&
> @@ -7848,7 +7848,7 @@ static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
>  }
>  
>  static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> -                               int access_type, ARMMMUIdx mmu_idx,
> +                               MMUAccessType access_type, ARMMMUIdx mmu_idx,
>                                 hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot,
>                                 target_ulong *page_size_ptr, uint32_t *fsr,
>                                 ARMMMUFaultInfo *fi)
> @@ -8256,7 +8256,7 @@ static inline bool m_is_system_region(CPUARMState *env, uint32_t address)
>  }
>  
>  static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
> -                                 int access_type, ARMMMUIdx mmu_idx,
> +                                 MMUAccessType access_type, ARMMMUIdx mmu_idx,
>                                   hwaddr *phys_ptr, int *prot, uint32_t *fsr)
>  {
>      ARMCPU *cpu = arm_env_get_cpu(env);
> @@ -8415,7 +8415,7 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>  }
>  
>  static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
> -                                 int access_type, ARMMMUIdx mmu_idx,
> +                                 MMUAccessType access_type, ARMMMUIdx mmu_idx,
>                                   hwaddr *phys_ptr, int *prot, uint32_t *fsr)
>  {
>      int n;
> @@ -8442,7 +8442,7 @@ static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
>          return true;
>      }
>  
> -    if (access_type == 2) {
> +    if (access_type == MMU_INST_FETCH) {
>          mask = env->cp15.pmsav5_insn_ap;
>      } else {
>          mask = env->cp15.pmsav5_data_ap;
> @@ -8513,7 +8513,7 @@ static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
>   * @fsr: set to the DFSR/IFSR value on failure
>   */
>  static bool get_phys_addr(CPUARMState *env, target_ulong address,
> -                          int access_type, ARMMMUIdx mmu_idx,
> +                          MMUAccessType access_type, ARMMMUIdx mmu_idx,
>                            hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
>                            target_ulong *page_size, uint32_t *fsr,
>                            ARMMMUFaultInfo *fi)
> @@ -8626,7 +8626,7 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
>   * fsr with ARM DFSR/IFSR fault register format value on failure.
>   */
>  bool arm_tlb_fill(CPUState *cs, vaddr address,
> -                  int access_type, int mmu_idx, uint32_t *fsr,
> +                  MMUAccessType access_type, int mmu_idx, uint32_t *fsr,
>                    ARMMMUFaultInfo *fi)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 1f6efef..bb06946 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -457,7 +457,8 @@ struct ARMMMUFaultInfo {
>  };
>  
>  /* Do a page table walk and add page to TLB if possible */
> -bool arm_tlb_fill(CPUState *cpu, vaddr address, int rw, int mmu_idx,
> +bool arm_tlb_fill(CPUState *cpu, vaddr address,
> +                  MMUAccessType access_type, int mmu_idx,
>                    uint32_t *fsr, ARMMMUFaultInfo *fi);
>  
>  /* Return true if the stage 1 translation regime is using LPAE format page
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 01/15] target/arm: Use MMUAccessType enum rather than int

[Philippe Mathieu-Daudé]

On 08/02/2017 01:43 PM, Peter Maydell wrote:
> In the ARM get_phys_addr() code, switch to using the MMUAccessType
> enum and its MMU_* values rather than int and literal 0/1/2.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>   target/arm/helper.c    | 30 +++++++++++++++---------------
>   target/arm/internals.h |  3 ++-
>   2 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index fa60040..b78d277 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -20,13 +20,13 @@
>   
>   #ifndef CONFIG_USER_ONLY
>   static bool get_phys_addr(CPUARMState *env, target_ulong address,
> -                          int access_type, ARMMMUIdx mmu_idx,
> +                          MMUAccessType access_type, ARMMMUIdx mmu_idx,
>                             hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
>                             target_ulong *page_size, uint32_t *fsr,
>                             ARMMMUFaultInfo *fi);
>   
>   static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> -                               int access_type, ARMMMUIdx mmu_idx,
> +                               MMUAccessType access_type, ARMMMUIdx mmu_idx,
>                                  hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot,
>                                  target_ulong *page_size_ptr, uint32_t *fsr,
>                                  ARMMMUFaultInfo *fi);
> @@ -2135,7 +2135,7 @@ static CPAccessResult ats_access(CPUARMState *env, const ARMCPRegInfo *ri,
>   }
>   
>   static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
> -                             int access_type, ARMMMUIdx mmu_idx)
> +                             MMUAccessType access_type, ARMMMUIdx mmu_idx)
>   {
>       hwaddr phys_addr;
>       target_ulong page_size;
> @@ -2194,7 +2194,7 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
>   
>   static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>   {
> -    int access_type = ri->opc2 & 1;
> +    MMUAccessType access_type = ri->opc2 & 1 ? MMU_DATA_STORE : MMU_DATA_LOAD;
>       uint64_t par64;
>       ARMMMUIdx mmu_idx;
>       int el = arm_current_el(env);
> @@ -2253,7 +2253,7 @@ static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>   static void ats1h_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                           uint64_t value)
>   {
> -    int access_type = ri->opc2 & 1;
> +    MMUAccessType access_type = ri->opc2 & 1 ? MMU_DATA_STORE : MMU_DATA_LOAD;
>       uint64_t par64;
>   
>       par64 = do_ats_write(env, value, access_type, ARMMMUIdx_S2NS);
> @@ -2273,7 +2273,7 @@ static CPAccessResult at_s1e2_access(CPUARMState *env, const ARMCPRegInfo *ri,
>   static void ats_write64(CPUARMState *env, const ARMCPRegInfo *ri,
>                           uint64_t value)
>   {
> -    int access_type = ri->opc2 & 1;
> +    MMUAccessType access_type = ri->opc2 & 1 ? MMU_DATA_STORE : MMU_DATA_LOAD;
>       ARMMMUIdx mmu_idx;
>       int secure = arm_is_secure_below_el3(env);
>   
> @@ -7510,7 +7510,7 @@ static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure,
>   }
>   
>   static bool get_phys_addr_v5(CPUARMState *env, uint32_t address,
> -                             int access_type, ARMMMUIdx mmu_idx,
> +                             MMUAccessType access_type, ARMMMUIdx mmu_idx,
>                                hwaddr *phys_ptr, int *prot,
>                                target_ulong *page_size, uint32_t *fsr,
>                                ARMMMUFaultInfo *fi)
> @@ -7626,7 +7626,7 @@ do_fault:
>   }
>   
>   static bool get_phys_addr_v6(CPUARMState *env, uint32_t address,
> -                             int access_type, ARMMMUIdx mmu_idx,
> +                             MMUAccessType access_type, ARMMMUIdx mmu_idx,
>                                hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
>                                target_ulong *page_size, uint32_t *fsr,
>                                ARMMMUFaultInfo *fi)
> @@ -7733,7 +7733,7 @@ static bool get_phys_addr_v6(CPUARMState *env, uint32_t address,
>           if (pxn && !regime_is_user(env, mmu_idx)) {
>               xn = 1;
>           }
> -        if (xn && access_type == 2)
> +        if (xn && access_type == MMU_INST_FETCH)
>               goto do_fault;
>   
>           if (arm_feature(env, ARM_FEATURE_V6K) &&
> @@ -7848,7 +7848,7 @@ static bool check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64, int level,
>   }
>   
>   static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> -                               int access_type, ARMMMUIdx mmu_idx,
> +                               MMUAccessType access_type, ARMMMUIdx mmu_idx,
>                                  hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot,
>                                  target_ulong *page_size_ptr, uint32_t *fsr,
>                                  ARMMMUFaultInfo *fi)
> @@ -8256,7 +8256,7 @@ static inline bool m_is_system_region(CPUARMState *env, uint32_t address)
>   }
>   
>   static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
> -                                 int access_type, ARMMMUIdx mmu_idx,
> +                                 MMUAccessType access_type, ARMMMUIdx mmu_idx,
>                                    hwaddr *phys_ptr, int *prot, uint32_t *fsr)
>   {
>       ARMCPU *cpu = arm_env_get_cpu(env);
> @@ -8415,7 +8415,7 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>   }
>   
>   static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
> -                                 int access_type, ARMMMUIdx mmu_idx,
> +                                 MMUAccessType access_type, ARMMMUIdx mmu_idx,
>                                    hwaddr *phys_ptr, int *prot, uint32_t *fsr)
>   {
>       int n;
> @@ -8442,7 +8442,7 @@ static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
>           return true;
>       }
>   
> -    if (access_type == 2) {
> +    if (access_type == MMU_INST_FETCH) {
>           mask = env->cp15.pmsav5_insn_ap;
>       } else {
>           mask = env->cp15.pmsav5_data_ap;
> @@ -8513,7 +8513,7 @@ static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
>    * @fsr: set to the DFSR/IFSR value on failure
>    */
>   static bool get_phys_addr(CPUARMState *env, target_ulong address,
> -                          int access_type, ARMMMUIdx mmu_idx,
> +                          MMUAccessType access_type, ARMMMUIdx mmu_idx,
>                             hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
>                             target_ulong *page_size, uint32_t *fsr,
>                             ARMMMUFaultInfo *fi)
> @@ -8626,7 +8626,7 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
>    * fsr with ARM DFSR/IFSR fault register format value on failure.
>    */
>   bool arm_tlb_fill(CPUState *cs, vaddr address,
> -                  int access_type, int mmu_idx, uint32_t *fsr,
> +                  MMUAccessType access_type, int mmu_idx, uint32_t *fsr,
>                     ARMMMUFaultInfo *fi)
>   {
>       ARMCPU *cpu = ARM_CPU(cs);
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 1f6efef..bb06946 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -457,7 +457,8 @@ struct ARMMMUFaultInfo {
>   };
>   
>   /* Do a page table walk and add page to TLB if possible */
> -bool arm_tlb_fill(CPUState *cpu, vaddr address, int rw, int mmu_idx,
> +bool arm_tlb_fill(CPUState *cpu, vaddr address,
> +                  MMUAccessType access_type, int mmu_idx,
>                     uint32_t *fsr, ARMMMUFaultInfo *fi);
>   
>   /* Return true if the stage 1 translation regime is using LPAE format page
> 

Re: [Qemu-devel] [PATCH 01/15] target/arm: Use MMUAccessType enum rather than int

[Richard Henderson]

On 08/02/2017 09:43 AM, Peter Maydell wrote:
> In the ARM get_phys_addr() code, switch to using the MMUAccessType
> enum and its MMU_* values rather than int and literal 0/1/2.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c    | 30 +++++++++++++++---------------
>  target/arm/internals.h |  3 ++-
>  2 files changed, 17 insertions(+), 16 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 02/15] target/arm: Don't trap WFI/WFE for M profile

[Peter Maydell]

M profile cores can never trap on WFI or WFE instructions. Check for
M profile in check_wfx_trap() to ensure this.

The existing code will do the right thing for v7M cores because
the hcr_el2 and scr_el3 registers will be all-zeroes and so we
won't attempt to trap, but when we start setting ARM_FEATURE_V8
for v8M cores the v8A handling of SCTLR.nTWE and .nTWI will not
give the right results.

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

diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 2a85666..5a94a5f 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -370,6 +370,11 @@ static inline int check_wfx_trap(CPUARMState *env, bool is_wfe)
     int cur_el = arm_current_el(env);
     uint64_t mask;
 
+    if (arm_feature(env, ARM_FEATURE_M)) {
+        /* M profile cores can never trap WFI/WFE. */
+        return 0;
+    }
+
     /* If we are currently in EL0 then we need to check if SCTLR is set up for
      * WFx instructions being trapped to EL1. These trap bits don't exist in v7.
      */
-- 
2.7.4

Re: [Qemu-devel] [PATCH 02/15] target/arm: Don't trap WFI/WFE for M profile

[Edgar E. Iglesias]

On Wed, Aug 02, 2017 at 05:43:48PM +0100, Peter Maydell wrote:
> M profile cores can never trap on WFI or WFE instructions. Check for
> M profile in check_wfx_trap() to ensure this.
> 
> The existing code will do the right thing for v7M cores because
> the hcr_el2 and scr_el3 registers will be all-zeroes and so we
> won't attempt to trap, but when we start setting ARM_FEATURE_V8
> for v8M cores the v8A handling of SCTLR.nTWE and .nTWI will not
> give the right results.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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


> ---
>  target/arm/op_helper.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index 2a85666..5a94a5f 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -370,6 +370,11 @@ static inline int check_wfx_trap(CPUARMState *env, bool is_wfe)
>      int cur_el = arm_current_el(env);
>      uint64_t mask;
>  
> +    if (arm_feature(env, ARM_FEATURE_M)) {
> +        /* M profile cores can never trap WFI/WFE. */
> +        return 0;
> +    }
> +
>      /* If we are currently in EL0 then we need to check if SCTLR is set up for
>       * WFx instructions being trapped to EL1. These trap bits don't exist in v7.
>       */
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH 02/15] target/arm: Don't trap WFI/WFE for M profile

[Richard Henderson]

On 08/02/2017 09:43 AM, Peter Maydell wrote:
> M profile cores can never trap on WFI or WFE instructions. Check for
> M profile in check_wfx_trap() to ensure this.
> 
> The existing code will do the right thing for v7M cores because
> the hcr_el2 and scr_el3 registers will be all-zeroes and so we
> won't attempt to trap, but when we start setting ARM_FEATURE_V8
> for v8M cores the v8A handling of SCTLR.nTWE and .nTWI will not
> give the right results.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/op_helper.c | 5 +++++
>  1 file changed, 5 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

While looking at this, I think there's an error in helper_wfi.  The early exit
for cpu_has_work should happen after the exception check.


r~

Re: [Qemu-devel] [Qemu-arm] [PATCH 02/15] target/arm: Don't trap WFI/WFE for M profile

[Edgar E. Iglesias]

On Thu, Aug 03, 2017 at 01:28:28PM -0700, Richard Henderson wrote:
> On 08/02/2017 09:43 AM, Peter Maydell wrote:
> > M profile cores can never trap on WFI or WFE instructions. Check for
> > M profile in check_wfx_trap() to ensure this.
> > 
> > The existing code will do the right thing for v7M cores because
> > the hcr_el2 and scr_el3 registers will be all-zeroes and so we
> > won't attempt to trap, but when we start setting ARM_FEATURE_V8
> > for v8M cores the v8A handling of SCTLR.nTWE and .nTWI will not
> > give the right results.
> > 
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  target/arm/op_helper.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> While looking at this, I think there's an error in helper_wfi.  The early exit
> for cpu_has_work should happen after the exception check.

I don't have the spec at hand but IIRC the trap should only happen
if the processor would have entered the low-power state (i.e if
there's no work).

A comment in the code would probably be good...

Cheers,
Edgar 

Re: [Qemu-devel] [Qemu-arm] [PATCH 02/15] target/arm: Don't trap WFI/WFE for M profile

[Richard Henderson]

On 08/03/2017 01:40 PM, Edgar E. Iglesias wrote:
> I don't have the spec at hand but IIRC the trap should only happen
> if the processor would have entered the low-power state (i.e if
> there's no work).

  when SystemHintOp_WFE
    if IsEventRegisterSet() then
      ClearEventRegister();
    else
      if PSTATE.EL == EL0 then
        AArch64.CheckForWFxTrap(EL1, TRUE);
      if HaveEL(EL2) && !IsSecure()
         && PSTATE.EL IN {EL0, EL1} && !IsInHost() then
        AArch64.CheckForWFxTrap(EL2, TRUE);
      if HaveEL(EL3) && PSTATE.EL != EL3 then
        AArch64.CheckForWFxTrap(EL3, TRUE);
      WaitForEvent();

Ah, I see what you mean, since WaitForEvent is also
described as checking EventRegister.

Thanks.


r~

Re: [Qemu-devel] [PATCH 02/15] target/arm: Don't trap WFI/WFE for M profile

[Peter Maydell]

On 3 August 2017 at 21:28, Richard Henderson <rth@twiddle.net> wrote:
> While looking at this, I think there's an error in helper_wfi.  The early exit
> for cpu_has_work should happen after the exception check.

No, that's deliberate; as Edgar says, the trap only
happens "if the instruction would otherwise have caused the
PE to enter a low-power state".
The rationale AIUI is that the traps to EL2 are there so
that when an EL guest does a WFI in its idle loop the EL2
hypervisor can gain control and give the CPU to
something else. This obviously imposes overhead, so if
the WFI wouldn't actually halt (because there's already
a condition that will cause it to wake up) it's more
efficient just to let the guest continue to execute.
(It also means that NOP is a valid WFI implementation,
though I think that's just a coincidental bonus.)

In fact the architecture gives even more flexibility
in that it only requires the trap to be taken "if the
instruction does not complete in finite time in the
absence of a Wakeup event", so you can do more complicated
things like "just pause for a short period of time to
see if an interrupt might come in and wake us up,
before giving up and taking the trap to EL2".

thanks
-- PMM

[Qemu-devel] [PATCH 03/15] target/arm: Consolidate PMSA handling in get_phys_addr()

[Peter Maydell]

Currently get_phys_addr() has PMSAv7 handling before the
"is translation disabled?" check, and then PMSAv5 after it.
Tidy this up by making the PMSAv5 code handle the "MPU disabled"
case itself, so that we have all the PMSA code in one place.
This will make adding the PMSAv8 code slightly cleaner, and
also means that pre-v7 PMSA cores benefit from the MPU lookup
logging that the PMSAv7 codepath had.

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index b78d277..fd83a21 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8423,6 +8423,13 @@ static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
     uint32_t base;
     bool is_user = regime_is_user(env, mmu_idx);
 
+    if (regime_translation_disabled(env, mmu_idx)) {
+        /* MPU disabled.  */
+        *phys_ptr = address;
+        *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
+        return false;
+    }
+
     *phys_ptr = address;
     for (n = 7; n >= 0; n--) {
         base = env->cp15.c6_region[n];
@@ -8572,16 +8579,20 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
         }
     }
 
-    /* pmsav7 has special handling for when MPU is disabled so call it before
-     * the common MMU/MPU disabled check below.
-     */
-    if (arm_feature(env, ARM_FEATURE_PMSA) &&
-        arm_feature(env, ARM_FEATURE_V7)) {
+    if (arm_feature(env, ARM_FEATURE_PMSA)) {
         bool ret;
         *page_size = TARGET_PAGE_SIZE;
-        ret = get_phys_addr_pmsav7(env, address, access_type, mmu_idx,
-                                   phys_ptr, prot, fsr);
-        qemu_log_mask(CPU_LOG_MMU, "PMSAv7 MPU lookup for %s at 0x%08" PRIx32
+
+        if (arm_feature(env, ARM_FEATURE_V7)) {
+            /* PMSAv7 */
+            ret = get_phys_addr_pmsav7(env, address, access_type, mmu_idx,
+                                       phys_ptr, prot, fsr);
+        } else {
+            /* Pre-v7 MPU */
+            ret = get_phys_addr_pmsav5(env, address, access_type, mmu_idx,
+                                       phys_ptr, prot, fsr);
+        }
+        qemu_log_mask(CPU_LOG_MMU, "PMSA MPU lookup for %s at 0x%08" PRIx32
                       " mmu_idx %u -> %s (prot %c%c%c)\n",
                       access_type == MMU_DATA_LOAD ? "reading" :
                       (access_type == MMU_DATA_STORE ? "writing" : "execute"),
@@ -8594,21 +8605,16 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
         return ret;
     }
 
+    /* Definitely a real MMU, not an MPU */
+
     if (regime_translation_disabled(env, mmu_idx)) {
-        /* MMU/MPU disabled.  */
+        /* MMU disabled. */
         *phys_ptr = address;
         *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
         *page_size = TARGET_PAGE_SIZE;
         return 0;
     }
 
-    if (arm_feature(env, ARM_FEATURE_PMSA)) {
-        /* Pre-v7 MPU */
-        *page_size = TARGET_PAGE_SIZE;
-        return get_phys_addr_pmsav5(env, address, access_type, mmu_idx,
-                                    phys_ptr, prot, fsr);
-    }
-
     if (regime_using_lpae_format(env, mmu_idx)) {
         return get_phys_addr_lpae(env, address, access_type, mmu_idx, phys_ptr,
                                   attrs, prot, page_size, fsr, fi);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 03/15] target/arm: Consolidate PMSA handling in get_phys_addr()

[Edgar E. Iglesias]

On Wed, Aug 02, 2017 at 05:43:49PM +0100, Peter Maydell wrote:
> Currently get_phys_addr() has PMSAv7 handling before the
> "is translation disabled?" check, and then PMSAv5 after it.
> Tidy this up by making the PMSAv5 code handle the "MPU disabled"
> case itself, so that we have all the PMSA code in one place.
> This will make adding the PMSAv8 code slightly cleaner, and
> also means that pre-v7 PMSA cores benefit from the MPU lookup
> logging that the PMSAv7 codepath had.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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


> ---
>  target/arm/helper.c | 38 ++++++++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index b78d277..fd83a21 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -8423,6 +8423,13 @@ static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
>      uint32_t base;
>      bool is_user = regime_is_user(env, mmu_idx);
>  
> +    if (regime_translation_disabled(env, mmu_idx)) {
> +        /* MPU disabled.  */
> +        *phys_ptr = address;
> +        *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> +        return false;
> +    }
> +
>      *phys_ptr = address;
>      for (n = 7; n >= 0; n--) {
>          base = env->cp15.c6_region[n];
> @@ -8572,16 +8579,20 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
>          }
>      }
>  
> -    /* pmsav7 has special handling for when MPU is disabled so call it before
> -     * the common MMU/MPU disabled check below.
> -     */
> -    if (arm_feature(env, ARM_FEATURE_PMSA) &&
> -        arm_feature(env, ARM_FEATURE_V7)) {
> +    if (arm_feature(env, ARM_FEATURE_PMSA)) {
>          bool ret;
>          *page_size = TARGET_PAGE_SIZE;
> -        ret = get_phys_addr_pmsav7(env, address, access_type, mmu_idx,
> -                                   phys_ptr, prot, fsr);
> -        qemu_log_mask(CPU_LOG_MMU, "PMSAv7 MPU lookup for %s at 0x%08" PRIx32
> +
> +        if (arm_feature(env, ARM_FEATURE_V7)) {
> +            /* PMSAv7 */
> +            ret = get_phys_addr_pmsav7(env, address, access_type, mmu_idx,
> +                                       phys_ptr, prot, fsr);
> +        } else {
> +            /* Pre-v7 MPU */
> +            ret = get_phys_addr_pmsav5(env, address, access_type, mmu_idx,
> +                                       phys_ptr, prot, fsr);
> +        }
> +        qemu_log_mask(CPU_LOG_MMU, "PMSA MPU lookup for %s at 0x%08" PRIx32
>                        " mmu_idx %u -> %s (prot %c%c%c)\n",
>                        access_type == MMU_DATA_LOAD ? "reading" :
>                        (access_type == MMU_DATA_STORE ? "writing" : "execute"),
> @@ -8594,21 +8605,16 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
>          return ret;
>      }
>  
> +    /* Definitely a real MMU, not an MPU */
> +
>      if (regime_translation_disabled(env, mmu_idx)) {
> -        /* MMU/MPU disabled.  */
> +        /* MMU disabled. */
>          *phys_ptr = address;
>          *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
>          *page_size = TARGET_PAGE_SIZE;
>          return 0;
>      }
>  
> -    if (arm_feature(env, ARM_FEATURE_PMSA)) {
> -        /* Pre-v7 MPU */
> -        *page_size = TARGET_PAGE_SIZE;
> -        return get_phys_addr_pmsav5(env, address, access_type, mmu_idx,
> -                                    phys_ptr, prot, fsr);
> -    }
> -
>      if (regime_using_lpae_format(env, mmu_idx)) {
>          return get_phys_addr_lpae(env, address, access_type, mmu_idx, phys_ptr,
>                                    attrs, prot, page_size, fsr, fi);
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH 03/15] target/arm: Consolidate PMSA handling in get_phys_addr()

[Philippe Mathieu-Daudé]

On 08/02/2017 01:43 PM, Peter Maydell wrote:
> Currently get_phys_addr() has PMSAv7 handling before the
> "is translation disabled?" check, and then PMSAv5 after it.
> Tidy this up by making the PMSAv5 code handle the "MPU disabled"
> case itself, so that we have all the PMSA code in one place.
> This will make adding the PMSAv8 code slightly cleaner, and
> also means that pre-v7 PMSA cores benefit from the MPU lookup
> logging that the PMSAv7 codepath had.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>   target/arm/helper.c | 38 ++++++++++++++++++++++----------------
>   1 file changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index b78d277..fd83a21 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -8423,6 +8423,13 @@ static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
>       uint32_t base;
>       bool is_user = regime_is_user(env, mmu_idx);
>   
> +    if (regime_translation_disabled(env, mmu_idx)) {
> +        /* MPU disabled.  */
> +        *phys_ptr = address;
> +        *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> +        return false;
> +    }
> +
>       *phys_ptr = address;
>       for (n = 7; n >= 0; n--) {
>           base = env->cp15.c6_region[n];
> @@ -8572,16 +8579,20 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
>           }
>       }
>   
> -    /* pmsav7 has special handling for when MPU is disabled so call it before
> -     * the common MMU/MPU disabled check below.
> -     */
> -    if (arm_feature(env, ARM_FEATURE_PMSA) &&
> -        arm_feature(env, ARM_FEATURE_V7)) {
> +    if (arm_feature(env, ARM_FEATURE_PMSA)) {
>           bool ret;
>           *page_size = TARGET_PAGE_SIZE;
> -        ret = get_phys_addr_pmsav7(env, address, access_type, mmu_idx,
> -                                   phys_ptr, prot, fsr);
> -        qemu_log_mask(CPU_LOG_MMU, "PMSAv7 MPU lookup for %s at 0x%08" PRIx32
> +
> +        if (arm_feature(env, ARM_FEATURE_V7)) {
> +            /* PMSAv7 */
> +            ret = get_phys_addr_pmsav7(env, address, access_type, mmu_idx,
> +                                       phys_ptr, prot, fsr);
> +        } else {
> +            /* Pre-v7 MPU */
> +            ret = get_phys_addr_pmsav5(env, address, access_type, mmu_idx,
> +                                       phys_ptr, prot, fsr);
> +        }
> +        qemu_log_mask(CPU_LOG_MMU, "PMSA MPU lookup for %s at 0x%08" PRIx32
>                         " mmu_idx %u -> %s (prot %c%c%c)\n",
>                         access_type == MMU_DATA_LOAD ? "reading" :
>                         (access_type == MMU_DATA_STORE ? "writing" : "execute"),
> @@ -8594,21 +8605,16 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,
>           return ret;
>       }
>   
> +    /* Definitely a real MMU, not an MPU */
> +
>       if (regime_translation_disabled(env, mmu_idx)) {
> -        /* MMU/MPU disabled.  */
> +        /* MMU disabled. */
>           *phys_ptr = address;
>           *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
>           *page_size = TARGET_PAGE_SIZE;
>           return 0;
>       }
>   
> -    if (arm_feature(env, ARM_FEATURE_PMSA)) {
> -        /* Pre-v7 MPU */
> -        *page_size = TARGET_PAGE_SIZE;
> -        return get_phys_addr_pmsav5(env, address, access_type, mmu_idx,
> -                                    phys_ptr, prot, fsr);
> -    }
> -
>       if (regime_using_lpae_format(env, mmu_idx)) {
>           return get_phys_addr_lpae(env, address, access_type, mmu_idx, phys_ptr,
>                                     attrs, prot, page_size, fsr, fi);
> 

Re: [Qemu-devel] [PATCH 03/15] target/arm: Consolidate PMSA handling in get_phys_addr()

[Richard Henderson]

On 08/02/2017 09:43 AM, Peter Maydell wrote:
> Currently get_phys_addr() has PMSAv7 handling before the
> "is translation disabled?" check, and then PMSAv5 after it.
> Tidy this up by making the PMSAv5 code handle the "MPU disabled"
> case itself, so that we have all the PMSA code in one place.
> This will make adding the PMSAv8 code slightly cleaner, and
> also means that pre-v7 PMSA cores benefit from the MPU lookup
> logging that the PMSAv7 codepath had.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 38 ++++++++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 16 deletions(-)


Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 04/15] target/arm: Tighten up Thumb decode where new v8M insns will be

[Peter Maydell]

Tighten up the T32 decoder in the places where new v8M instructions
will be:
 * TT/TTT/TTA/TTAT are in what was nominally LDREX/STREX r15, ...
   which is UNPREDICTABLE:
   make the UNPREDICTABLE behaviour be to UNDEF
 * BXNS/BLXNS are distinguished from BX/BLX via the low 3 bits,
   which in previous architectural versions are SBZ:
   enforce the SBZ via UNDEF rather than ignoring it, and move
   the "ARCH(5)" UNDEF case up so we don't leak a TCG temporary
 * SG is in the encoding which would be LDRD/STRD with rn = r15;
   this is UNPREDICTABLE and we currently UNDEF:
   move this check further up the code so that we don't leak
   TCG temporaries in the UNDEF case and have a better place
   to put the SG decode.

This means that if a v8M binary is accidentally run on v7M
or if a test case hits something that we haven't implemented
yet the behaviour will be obvious (UNDEF) rather than obscure
(plough on treating it as a different instruction).

In the process, add some comments about the instruction patterns
at these points in the decode. Our Thumb and ARM decoders are
very difficult to understand currently, but gradually adding
comments like this should help to clarify what exactly has
been decoded when.

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

diff --git a/target/arm/translate.c b/target/arm/translate.c
index d1a5f56..3c14cb0 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -9735,10 +9735,23 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
         abort();
     case 4:
         if (insn & (1 << 22)) {
-            /* Other load/store, table branch.  */
+            /* 0b1110_100x_x1xx_xxxx_xxxx_xxxx_xxxx_xxxx
+             * - load/store doubleword, load/store exclusive, ldacq/strel,
+             *   table branch.
+             */
             if (insn & 0x01200000) {
-                /* Load/store doubleword.  */
+                /* 0b1110_1000_x11x_xxxx_xxxx_xxxx_xxxx_xxxx
+                 *  - load/store dual (post-indexed)
+                 * 0b1111_1001_x10x_xxxx_xxxx_xxxx_xxxx_xxxx
+                 *  - load/store dual (literal and immediate)
+                 * 0b1111_1001_x11x_xxxx_xxxx_xxxx_xxxx_xxxx
+                 *  - load/store dual (pre-indexed)
+                 */
                 if (rn == 15) {
+                    if (insn & (1 << 21)) {
+                        /* UNPREDICTABLE */
+                        goto illegal_op;
+                    }
                     addr = tcg_temp_new_i32();
                     tcg_gen_movi_i32(addr, s->pc & ~3);
                 } else {
@@ -9772,15 +9785,18 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
                 }
                 if (insn & (1 << 21)) {
                     /* Base writeback.  */
-                    if (rn == 15)
-                        goto illegal_op;
                     tcg_gen_addi_i32(addr, addr, offset - 4);
                     store_reg(s, rn, addr);
                 } else {
                     tcg_temp_free_i32(addr);
                 }
             } else if ((insn & (1 << 23)) == 0) {
-                /* Load/store exclusive word.  */
+                /* 0b1110_1000_010x_xxxx_xxxx_xxxx_xxxx_xxxx
+                 * - load/store exclusive word
+                 */
+                if (rs == 15) {
+                    goto illegal_op;
+                }
                 addr = tcg_temp_local_new_i32();
                 load_reg_var(s, addr, rn);
                 tcg_gen_addi_i32(addr, addr, (insn & 0xff) << 2);
@@ -11137,7 +11153,9 @@ static void disas_thumb_insn(CPUARMState *env, DisasContext *s)
             break;
         }
         if (insn & (1 << 10)) {
-            /* data processing extended or blx */
+            /* 0b0100_01xx_xxxx_xxxx
+             * - data processing extended, branch and exchange
+             */
             rd = (insn & 7) | ((insn >> 4) & 8);
             rm = (insn >> 3) & 0xf;
             op = (insn >> 8) & 3;
@@ -11160,10 +11178,21 @@ static void disas_thumb_insn(CPUARMState *env, DisasContext *s)
                 tmp = load_reg(s, rm);
                 store_reg(s, rd, tmp);
                 break;
-            case 3:/* branch [and link] exchange thumb register */
-                tmp = load_reg(s, rm);
-                if (insn & (1 << 7)) {
+            case 3:
+            {
+                /* 0b0100_0111_xxxx_xxxx
+                 * - branch [and link] exchange thumb register
+                 */
+                bool link = insn & (1 << 7);
+
+                if (insn & 7) {
+                    goto undef;
+                }
+                if (link) {
                     ARCH(5);
+                }
+                tmp = load_reg(s, rm);
+                if (link) {
                     val = (uint32_t)s->pc | 1;
                     tmp2 = tcg_temp_new_i32();
                     tcg_gen_movi_i32(tmp2, val);
@@ -11175,6 +11204,7 @@ static void disas_thumb_insn(CPUARMState *env, DisasContext *s)
                 }
                 break;
             }
+            }
             break;
         }
 
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH 04/15] target/arm: Tighten up Thumb decode where new v8M insns will be

[Edgar E. Iglesias]

On Wed, Aug 02, 2017 at 05:43:50PM +0100, Peter Maydell wrote:
> Tighten up the T32 decoder in the places where new v8M instructions
> will be:
>  * TT/TTT/TTA/TTAT are in what was nominally LDREX/STREX r15, ...
>    which is UNPREDICTABLE:
>    make the UNPREDICTABLE behaviour be to UNDEF
>  * BXNS/BLXNS are distinguished from BX/BLX via the low 3 bits,
>    which in previous architectural versions are SBZ:
>    enforce the SBZ via UNDEF rather than ignoring it, and move
>    the "ARCH(5)" UNDEF case up so we don't leak a TCG temporary
>  * SG is in the encoding which would be LDRD/STRD with rn = r15;
>    this is UNPREDICTABLE and we currently UNDEF:
>    move this check further up the code so that we don't leak
>    TCG temporaries in the UNDEF case and have a better place
>    to put the SG decode.
> 
> This means that if a v8M binary is accidentally run on v7M
> or if a test case hits something that we haven't implemented
> yet the behaviour will be obvious (UNDEF) rather than obscure
> (plough on treating it as a different instruction).
> 
> In the process, add some comments about the instruction patterns
> at these points in the decode. Our Thumb and ARM decoders are
> very difficult to understand currently, but gradually adding
> comments like this should help to clarify what exactly has
> been decoded when.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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


> ---
>  target/arm/translate.c | 48 +++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 39 insertions(+), 9 deletions(-)
> 
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index d1a5f56..3c14cb0 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -9735,10 +9735,23 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>          abort();
>      case 4:
>          if (insn & (1 << 22)) {
> -            /* Other load/store, table branch.  */
> +            /* 0b1110_100x_x1xx_xxxx_xxxx_xxxx_xxxx_xxxx
> +             * - load/store doubleword, load/store exclusive, ldacq/strel,
> +             *   table branch.
> +             */
>              if (insn & 0x01200000) {
> -                /* Load/store doubleword.  */
> +                /* 0b1110_1000_x11x_xxxx_xxxx_xxxx_xxxx_xxxx
> +                 *  - load/store dual (post-indexed)
> +                 * 0b1111_1001_x10x_xxxx_xxxx_xxxx_xxxx_xxxx
> +                 *  - load/store dual (literal and immediate)
> +                 * 0b1111_1001_x11x_xxxx_xxxx_xxxx_xxxx_xxxx
> +                 *  - load/store dual (pre-indexed)
> +                 */
>                  if (rn == 15) {
> +                    if (insn & (1 << 21)) {
> +                        /* UNPREDICTABLE */
> +                        goto illegal_op;
> +                    }
>                      addr = tcg_temp_new_i32();
>                      tcg_gen_movi_i32(addr, s->pc & ~3);
>                  } else {
> @@ -9772,15 +9785,18 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>                  }
>                  if (insn & (1 << 21)) {
>                      /* Base writeback.  */
> -                    if (rn == 15)
> -                        goto illegal_op;
>                      tcg_gen_addi_i32(addr, addr, offset - 4);
>                      store_reg(s, rn, addr);
>                  } else {
>                      tcg_temp_free_i32(addr);
>                  }
>              } else if ((insn & (1 << 23)) == 0) {
> -                /* Load/store exclusive word.  */
> +                /* 0b1110_1000_010x_xxxx_xxxx_xxxx_xxxx_xxxx
> +                 * - load/store exclusive word
> +                 */
> +                if (rs == 15) {
> +                    goto illegal_op;
> +                }
>                  addr = tcg_temp_local_new_i32();
>                  load_reg_var(s, addr, rn);
>                  tcg_gen_addi_i32(addr, addr, (insn & 0xff) << 2);
> @@ -11137,7 +11153,9 @@ static void disas_thumb_insn(CPUARMState *env, DisasContext *s)
>              break;
>          }
>          if (insn & (1 << 10)) {
> -            /* data processing extended or blx */
> +            /* 0b0100_01xx_xxxx_xxxx
> +             * - data processing extended, branch and exchange
> +             */
>              rd = (insn & 7) | ((insn >> 4) & 8);
>              rm = (insn >> 3) & 0xf;
>              op = (insn >> 8) & 3;
> @@ -11160,10 +11178,21 @@ static void disas_thumb_insn(CPUARMState *env, DisasContext *s)
>                  tmp = load_reg(s, rm);
>                  store_reg(s, rd, tmp);
>                  break;
> -            case 3:/* branch [and link] exchange thumb register */
> -                tmp = load_reg(s, rm);
> -                if (insn & (1 << 7)) {
> +            case 3:
> +            {
> +                /* 0b0100_0111_xxxx_xxxx
> +                 * - branch [and link] exchange thumb register
> +                 */
> +                bool link = insn & (1 << 7);
> +
> +                if (insn & 7) {
> +                    goto undef;
> +                }
> +                if (link) {
>                      ARCH(5);
> +                }
> +                tmp = load_reg(s, rm);
> +                if (link) {
>                      val = (uint32_t)s->pc | 1;
>                      tmp2 = tcg_temp_new_i32();
>                      tcg_gen_movi_i32(tmp2, val);
> @@ -11175,6 +11204,7 @@ static void disas_thumb_insn(CPUARMState *env, DisasContext *s)
>                  }
>                  break;
>              }
> +            }
>              break;
>          }
>  
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH 04/15] target/arm: Tighten up Thumb decode where new v8M insns will be

[Richard Henderson]

On 08/02/2017 09:43 AM, Peter Maydell wrote:
> Tighten up the T32 decoder in the places where new v8M instructions
> will be:
>  * TT/TTT/TTA/TTAT are in what was nominally LDREX/STREX r15, ...
>    which is UNPREDICTABLE:
>    make the UNPREDICTABLE behaviour be to UNDEF
>  * BXNS/BLXNS are distinguished from BX/BLX via the low 3 bits,
>    which in previous architectural versions are SBZ:
>    enforce the SBZ via UNDEF rather than ignoring it, and move
>    the "ARCH(5)" UNDEF case up so we don't leak a TCG temporary
>  * SG is in the encoding which would be LDRD/STRD with rn = r15;
>    this is UNPREDICTABLE and we currently UNDEF:
>    move this check further up the code so that we don't leak
>    TCG temporaries in the UNDEF case and have a better place
>    to put the SG decode.
> 
> This means that if a v8M binary is accidentally run on v7M
> or if a test case hits something that we haven't implemented
> yet the behaviour will be obvious (UNDEF) rather than obscure
> (plough on treating it as a different instruction).
> 
> In the process, add some comments about the instruction patterns
> at these points in the decode. Our Thumb and ARM decoders are
> very difficult to understand currently, but gradually adding
> comments like this should help to clarify what exactly has
> been decoded when.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/translate.c | 48 +++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 39 insertions(+), 9 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 05/15] hw/intc/armv7m_nvic.c: Remove out of date comment

[Peter Maydell]

Remove an out of date comment which says there's only one
item in the NVIC container region -- we put systick into its
own device object a while back and so now there are two
things in the container.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/armv7m_nvic.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 323e2d4..2e8166a 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -1036,10 +1036,6 @@ static void armv7m_nvic_realize(DeviceState *dev, Error **errp)
      *  0xd00..0xd3c - SCS registers
      *  0xd40..0xeff - Reserved or Not implemented
      *  0xf00 - STIR
-     *
-     * At the moment there is only one thing in the container region,
-     * but we leave it in place to allow us to pull systick out into
-     * its own device object later.
      */
     memory_region_init(&s->container, OBJECT(s), "nvic", 0x1000);
     /* The system register region goes at the bottom of the priority
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH 05/15] hw/intc/armv7m_nvic.c: Remove out of date comment

[Edgar E. Iglesias]

On Wed, Aug 02, 2017 at 05:43:51PM +0100, Peter Maydell wrote:
> Remove an out of date comment which says there's only one
> item in the NVIC container region -- we put systick into its
> own device object a while back and so now there are two
> things in the container.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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


> ---
>  hw/intc/armv7m_nvic.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 323e2d4..2e8166a 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -1036,10 +1036,6 @@ static void armv7m_nvic_realize(DeviceState *dev, Error **errp)
>       *  0xd00..0xd3c - SCS registers
>       *  0xd40..0xeff - Reserved or Not implemented
>       *  0xf00 - STIR
> -     *
> -     * At the moment there is only one thing in the container region,
> -     * but we leave it in place to allow us to pull systick out into
> -     * its own device object later.
>       */
>      memory_region_init(&s->container, OBJECT(s), "nvic", 0x1000);
>      /* The system register region goes at the bottom of the priority
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH 05/15] hw/intc/armv7m_nvic.c: Remove out of date comment

[Richard Henderson]

On 08/02/2017 09:43 AM, Peter Maydell wrote:
> Remove an out of date comment which says there's only one
> item in the NVIC container region -- we put systick into its
> own device object a while back and so now there are two
> things in the container.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/armv7m_nvic.c | 4 ----
>  1 file changed, 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 06/15] target/arm: Remove incorrect comment about MPU_CTRL

[Peter Maydell]

Remove the comment that claims that some MPU_CTRL bits are stored
in sctlr_el[1]. This has never been true since MPU_CTRL was added
in commit 29c483a50607 -- the comment is a leftover from
Michael Davidsaver's original implementation, which I modified
not to use sctlr_el[1]; I forgot to delete the comment then.

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

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index b39d64a..b64474c 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -416,7 +416,7 @@ typedef struct CPUARMState {
         uint32_t dfsr; /* Debug Fault Status Register */
         uint32_t mmfar; /* MemManage Fault Address */
         uint32_t bfar; /* BusFault Address */
-        unsigned mpu_ctrl; /* MPU_CTRL (some bits kept in sctlr_el[1]) */
+        unsigned mpu_ctrl; /* MPU_CTRL */
         int exception;
     } v7m;
 
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH 06/15] target/arm: Remove incorrect comment about MPU_CTRL

[Edgar E. Iglesias]

On Wed, Aug 02, 2017 at 05:43:52PM +0100, Peter Maydell wrote:
> Remove the comment that claims that some MPU_CTRL bits are stored
> in sctlr_el[1]. This has never been true since MPU_CTRL was added
> in commit 29c483a50607 -- the comment is a leftover from
> Michael Davidsaver's original implementation, which I modified
> not to use sctlr_el[1]; I forgot to delete the comment then.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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


> ---
>  target/arm/cpu.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index b39d64a..b64474c 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -416,7 +416,7 @@ typedef struct CPUARMState {
>          uint32_t dfsr; /* Debug Fault Status Register */
>          uint32_t mmfar; /* MemManage Fault Address */
>          uint32_t bfar; /* BusFault Address */
> -        unsigned mpu_ctrl; /* MPU_CTRL (some bits kept in sctlr_el[1]) */
> +        unsigned mpu_ctrl; /* MPU_CTRL */
>          int exception;
>      } v7m;
>  
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH 06/15] target/arm: Remove incorrect comment about MPU_CTRL

[Richard Henderson]

On 08/02/2017 09:43 AM, Peter Maydell wrote:
> Remove the comment that claims that some MPU_CTRL bits are stored
> in sctlr_el[1]. This has never been true since MPU_CTRL was added
> in commit 29c483a50607 -- the comment is a leftover from
> Michael Davidsaver's original implementation, which I modified
> not to use sctlr_el[1]; I forgot to delete the comment then.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/cpu.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 07/15] target/arm: Fix outdated comment about exception exit

[Peter Maydell]

When we switched our handling of exception exit to detect
the magic addresses at translate time rather than via
a do_unassigned_access hook, we forgot to update a
comment; correct the omission.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 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 fd83a21..cb88c66 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6143,7 +6143,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
     bool rettobase = false;
 
     /* We can only get here from an EXCP_EXCEPTION_EXIT, and
-     * arm_v7m_do_unassigned_access() enforces the architectural rule
+     * gen_bx_excret() enforces the architectural rule
      * that jumps to magic addresses don't have magic behaviour unless
      * we're in Handler mode (compare pseudocode BXWritePC()).
      */
-- 
2.7.4

Re: [Qemu-devel] [PATCH 07/15] target/arm: Fix outdated comment about exception exit

[Edgar E. Iglesias]

On Wed, Aug 02, 2017 at 05:43:53PM +0100, Peter Maydell wrote:
> When we switched our handling of exception exit to detect
> the magic addresses at translate time rather than via
> a do_unassigned_access hook, we forgot to update a
> comment; correct the omission.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.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 fd83a21..cb88c66 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6143,7 +6143,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>      bool rettobase = false;
>  
>      /* We can only get here from an EXCP_EXCEPTION_EXIT, and
> -     * arm_v7m_do_unassigned_access() enforces the architectural rule
> +     * gen_bx_excret() enforces the architectural rule
>       * that jumps to magic addresses don't have magic behaviour unless
>       * we're in Handler mode (compare pseudocode BXWritePC()).
>       */
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH 07/15] target/arm: Fix outdated comment about exception exit

[Richard Henderson]

On 08/02/2017 09:43 AM, Peter Maydell wrote:
> When we switched our handling of exception exit to detect
> the magic addresses at translate time rather than via
> a do_unassigned_access hook, we forgot to update a
> comment; correct the omission.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 08/15] target/arm: Define and use XPSR bit masks

[Peter Maydell]

The M profile XPSR is almost the same format as the A profile CPSR,
but not quite. Define some XPSR_* macros and use them where we
definitely dealing with an XPSR rather than reusing the CPSR ones.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h    | 38 ++++++++++++++++++++++++++++----------
 target/arm/helper.c | 15 ++++++++-------
 2 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index b64474c..1f06de0 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -883,6 +883,22 @@ void pmccntr_sync(CPUARMState *env);
 /* Mask of bits which may be set by exception return copying them from SPSR */
 #define CPSR_ERET_MASK (~CPSR_RESERVED)
 
+/* Bit definitions for M profile XPSR. Most are the same as CPSR. */
+#define XPSR_EXCP 0x1ffU
+#define XPSR_SPREALIGN (1U << 9) /* Only set in exception stack frames */
+#define XPSR_IT_2_7 CPSR_IT_2_7
+#define XPSR_GE CPSR_GE
+#define XPSR_SFPA (1U << 20) /* Only set in exception stack frames */
+#define XPSR_T (1U << 24) /* Not the same as CPSR_T ! */
+#define XPSR_IT_0_1 CPSR_IT_0_1
+#define XPSR_Q CPSR_Q
+#define XPSR_V CPSR_V
+#define XPSR_C CPSR_C
+#define XPSR_Z CPSR_Z
+#define XPSR_N CPSR_N
+#define XPSR_NZCV CPSR_NZCV
+#define XPSR_IT CPSR_IT
+
 #define TTBCR_N      (7U << 0) /* TTBCR.EAE==0 */
 #define TTBCR_T0SZ   (7U << 0) /* TTBCR.EAE==1 */
 #define TTBCR_PD0    (1U << 4)
@@ -987,26 +1003,28 @@ static inline uint32_t xpsr_read(CPUARMState *env)
 /* Set the xPSR.  Note that some bits of mask must be all-set or all-clear.  */
 static inline void xpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
 {
-    if (mask & CPSR_NZCV) {
-        env->ZF = (~val) & CPSR_Z;
+    if (mask & XPSR_NZCV) {
+        env->ZF = (~val) & XPSR_Z;
         env->NF = val;
         env->CF = (val >> 29) & 1;
         env->VF = (val << 3) & 0x80000000;
     }
-    if (mask & CPSR_Q)
-        env->QF = ((val & CPSR_Q) != 0);
-    if (mask & (1 << 24))
-        env->thumb = ((val & (1 << 24)) != 0);
-    if (mask & CPSR_IT_0_1) {
+    if (mask & XPSR_Q) {
+        env->QF = ((val & XPSR_Q) != 0);
+    }
+    if (mask & XPSR_T) {
+        env->thumb = ((val & XPSR_T) != 0);
+    }
+    if (mask & XPSR_IT_0_1) {
         env->condexec_bits &= ~3;
         env->condexec_bits |= (val >> 25) & 3;
     }
-    if (mask & CPSR_IT_2_7) {
+    if (mask & XPSR_IT_2_7) {
         env->condexec_bits &= 3;
         env->condexec_bits |= (val >> 8) & 0xfc;
     }
-    if (mask & 0x1ff) {
-        env->v7m.exception = val & 0x1ff;
+    if (mask & XPSR_EXCP) {
+        env->v7m.exception = val & XPSR_EXCP;
     }
 }
 
diff --git a/target/arm/helper.c b/target/arm/helper.c
index cb88c66..f087d42 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6119,7 +6119,7 @@ static void v7m_push_stack(ARMCPU *cpu)
     /* Align stack pointer if the guest wants that */
     if ((env->regs[13] & 4) && (env->v7m.ccr & R_V7M_CCR_STKALIGN_MASK)) {
         env->regs[13] -= 4;
-        xpsr |= 0x200;
+        xpsr |= XPSR_SPREALIGN;
     }
     /* Switch to the handler mode.  */
     v7m_push(env, xpsr);
@@ -6244,10 +6244,11 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
         env->regs[15] &= ~1U;
     }
     xpsr = v7m_pop(env);
-    xpsr_write(env, xpsr, 0xfffffdff);
+    xpsr_write(env, xpsr, ~XPSR_SPREALIGN);
     /* Undo stack alignment.  */
-    if (xpsr & 0x200)
+    if (xpsr & XPSR_SPREALIGN) {
         env->regs[13] |= 4;
+    }
 
     /* The restored xPSR exception field will be zero if we're
      * resuming in Thread mode. If that doesn't match what the
@@ -8693,10 +8694,10 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)
     case 0 ... 7: /* xPSR sub-fields */
         mask = 0;
         if ((reg & 1) && el) {
-            mask |= 0x000001ff; /* IPSR (unpriv. reads as zero) */
+            mask |= XPSR_EXCP; /* IPSR (unpriv. reads as zero) */
         }
         if (!(reg & 4)) {
-            mask |= 0xf8000000; /* APSR */
+            mask |= XPSR_NZCV | XPSR_Q; /* APSR */
         }
         /* EPSR reads as zero */
         return xpsr_read(env) & mask;
@@ -8754,10 +8755,10 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
             uint32_t apsrmask = 0;
 
             if (mask & 8) {
-                apsrmask |= 0xf8000000; /* APSR NZCVQ */
+                apsrmask |= XPSR_NZCV | XPSR_Q;
             }
             if ((mask & 4) && arm_feature(env, ARM_FEATURE_THUMB_DSP)) {
-                apsrmask |= 0x000f0000; /* APSR GE[3:0] */
+                apsrmask |= XPSR_GE;
             }
             xpsr_write(env, val, apsrmask);
         }
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH 08/15] target/arm: Define and use XPSR bit masks

[Edgar E. Iglesias]

On Wed, Aug 02, 2017 at 05:43:54PM +0100, Peter Maydell wrote:
> The M profile XPSR is almost the same format as the A profile CPSR,
> but not quite. Define some XPSR_* macros and use them where we
> definitely dealing with an XPSR rather than reusing the CPSR ones.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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


> ---
>  target/arm/cpu.h    | 38 ++++++++++++++++++++++++++++----------
>  target/arm/helper.c | 15 ++++++++-------
>  2 files changed, 36 insertions(+), 17 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index b64474c..1f06de0 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -883,6 +883,22 @@ void pmccntr_sync(CPUARMState *env);
>  /* Mask of bits which may be set by exception return copying them from SPSR */
>  #define CPSR_ERET_MASK (~CPSR_RESERVED)
>  
> +/* Bit definitions for M profile XPSR. Most are the same as CPSR. */
> +#define XPSR_EXCP 0x1ffU
> +#define XPSR_SPREALIGN (1U << 9) /* Only set in exception stack frames */
> +#define XPSR_IT_2_7 CPSR_IT_2_7
> +#define XPSR_GE CPSR_GE
> +#define XPSR_SFPA (1U << 20) /* Only set in exception stack frames */
> +#define XPSR_T (1U << 24) /* Not the same as CPSR_T ! */
> +#define XPSR_IT_0_1 CPSR_IT_0_1
> +#define XPSR_Q CPSR_Q
> +#define XPSR_V CPSR_V
> +#define XPSR_C CPSR_C
> +#define XPSR_Z CPSR_Z
> +#define XPSR_N CPSR_N
> +#define XPSR_NZCV CPSR_NZCV
> +#define XPSR_IT CPSR_IT
> +
>  #define TTBCR_N      (7U << 0) /* TTBCR.EAE==0 */
>  #define TTBCR_T0SZ   (7U << 0) /* TTBCR.EAE==1 */
>  #define TTBCR_PD0    (1U << 4)
> @@ -987,26 +1003,28 @@ static inline uint32_t xpsr_read(CPUARMState *env)
>  /* Set the xPSR.  Note that some bits of mask must be all-set or all-clear.  */
>  static inline void xpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
>  {
> -    if (mask & CPSR_NZCV) {
> -        env->ZF = (~val) & CPSR_Z;
> +    if (mask & XPSR_NZCV) {
> +        env->ZF = (~val) & XPSR_Z;
>          env->NF = val;
>          env->CF = (val >> 29) & 1;
>          env->VF = (val << 3) & 0x80000000;
>      }
> -    if (mask & CPSR_Q)
> -        env->QF = ((val & CPSR_Q) != 0);
> -    if (mask & (1 << 24))
> -        env->thumb = ((val & (1 << 24)) != 0);
> -    if (mask & CPSR_IT_0_1) {
> +    if (mask & XPSR_Q) {
> +        env->QF = ((val & XPSR_Q) != 0);
> +    }
> +    if (mask & XPSR_T) {
> +        env->thumb = ((val & XPSR_T) != 0);
> +    }
> +    if (mask & XPSR_IT_0_1) {
>          env->condexec_bits &= ~3;
>          env->condexec_bits |= (val >> 25) & 3;
>      }
> -    if (mask & CPSR_IT_2_7) {
> +    if (mask & XPSR_IT_2_7) {
>          env->condexec_bits &= 3;
>          env->condexec_bits |= (val >> 8) & 0xfc;
>      }
> -    if (mask & 0x1ff) {
> -        env->v7m.exception = val & 0x1ff;
> +    if (mask & XPSR_EXCP) {
> +        env->v7m.exception = val & XPSR_EXCP;
>      }
>  }
>  
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index cb88c66..f087d42 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6119,7 +6119,7 @@ static void v7m_push_stack(ARMCPU *cpu)
>      /* Align stack pointer if the guest wants that */
>      if ((env->regs[13] & 4) && (env->v7m.ccr & R_V7M_CCR_STKALIGN_MASK)) {
>          env->regs[13] -= 4;
> -        xpsr |= 0x200;
> +        xpsr |= XPSR_SPREALIGN;
>      }
>      /* Switch to the handler mode.  */
>      v7m_push(env, xpsr);
> @@ -6244,10 +6244,11 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>          env->regs[15] &= ~1U;
>      }
>      xpsr = v7m_pop(env);
> -    xpsr_write(env, xpsr, 0xfffffdff);
> +    xpsr_write(env, xpsr, ~XPSR_SPREALIGN);
>      /* Undo stack alignment.  */
> -    if (xpsr & 0x200)
> +    if (xpsr & XPSR_SPREALIGN) {
>          env->regs[13] |= 4;
> +    }
>  
>      /* The restored xPSR exception field will be zero if we're
>       * resuming in Thread mode. If that doesn't match what the
> @@ -8693,10 +8694,10 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)
>      case 0 ... 7: /* xPSR sub-fields */
>          mask = 0;
>          if ((reg & 1) && el) {
> -            mask |= 0x000001ff; /* IPSR (unpriv. reads as zero) */
> +            mask |= XPSR_EXCP; /* IPSR (unpriv. reads as zero) */
>          }
>          if (!(reg & 4)) {
> -            mask |= 0xf8000000; /* APSR */
> +            mask |= XPSR_NZCV | XPSR_Q; /* APSR */
>          }
>          /* EPSR reads as zero */
>          return xpsr_read(env) & mask;
> @@ -8754,10 +8755,10 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
>              uint32_t apsrmask = 0;
>  
>              if (mask & 8) {
> -                apsrmask |= 0xf8000000; /* APSR NZCVQ */
> +                apsrmask |= XPSR_NZCV | XPSR_Q;
>              }
>              if ((mask & 4) && arm_feature(env, ARM_FEATURE_THUMB_DSP)) {
> -                apsrmask |= 0x000f0000; /* APSR GE[3:0] */
> +                apsrmask |= XPSR_GE;
>              }
>              xpsr_write(env, val, apsrmask);
>          }
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH 08/15] target/arm: Define and use XPSR bit masks

[Richard Henderson]

On 08/02/2017 09:43 AM, Peter Maydell wrote:
> The M profile XPSR is almost the same format as the A profile CPSR,
> but not quite. Define some XPSR_* macros and use them where we
> definitely dealing with an XPSR rather than reusing the CPSR ones.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/cpu.h    | 38 ++++++++++++++++++++++++++++----------
>  target/arm/helper.c | 15 ++++++++-------
>  2 files changed, 36 insertions(+), 17 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 09/15] target/arm: Don't store M profile PRIMASK and FAULTMASK in daif

[Peter Maydell]

We currently store the M profile CPU register state PRIMASK and
FAULTMASK in the daif field of the CPU state in its I and F
bits. This is a legacy from the original implementation, which
tried to share the cpu_exec_interrupt code between A profile
and M profile. We've since separated out the two cases because
they are significantly different, so now there is no common
code between M and A profile which looks at env->daif: all the
uses are either in A-only or M-only code paths. Sharing the state
fields now is just confusing, and will make things awkward
when we implement v8M, where the PRIMASK and FAULTMASK
registers are banked between security states.

Switch M profile over to using v7m.faultmask and v7m.primask
fields for these registers.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/armv7m_nvic.c |  4 ++--
 target/arm/cpu.c      |  5 -----
 target/arm/cpu.h      |  4 +++-
 target/arm/helper.c   | 18 +++++-------------
 target/arm/machine.c  | 33 +++++++++++++++++++++++++++++++++
 5 files changed, 43 insertions(+), 21 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 2e8166a..343bc16 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -167,9 +167,9 @@ static inline int nvic_exec_prio(NVICState *s)
     CPUARMState *env = &s->cpu->env;
     int running;
 
-    if (env->daif & PSTATE_F) { /* FAULTMASK */
+    if (env->v7m.faultmask) {
         running = -1;
-    } else if (env->daif & PSTATE_I) { /* PRIMASK */
+    } else if (env->v7m.primask) {
         running = 0;
     } else if (env->v7m.basepri > 0) {
         running = env->v7m.basepri & nvic_gprio_mask(s);
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 05c038b..b241a63 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -185,11 +185,6 @@ static void arm_cpu_reset(CPUState *s)
         uint32_t initial_pc; /* Loaded from 0x4 */
         uint8_t *rom;
 
-        /* For M profile we store FAULTMASK and PRIMASK in the
-         * PSTATE F and I bits; these are both clear at reset.
-         */
-        env->daif &= ~(PSTATE_I | PSTATE_F);
-
         /* The reset value of this bit is IMPDEF, but ARM recommends
          * that it resets to 1, so QEMU always does that rather than making
          * it dependent on CPU model.
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 1f06de0..da90b7a 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -418,6 +418,8 @@ typedef struct CPUARMState {
         uint32_t bfar; /* BusFault Address */
         unsigned mpu_ctrl; /* MPU_CTRL */
         int exception;
+        uint32_t primask;
+        uint32_t faultmask;
     } v7m;
 
     /* Information associated with an exception about to be taken:
@@ -2179,7 +2181,7 @@ static inline int cpu_mmu_index(CPUARMState *env, bool ifetch)
          * we're in a HardFault or NMI handler.
          */
         if ((env->v7m.exception > 0 && env->v7m.exception <= 3)
-            || env->daif & PSTATE_F) {
+            || env->v7m.faultmask) {
             return arm_to_core_mmu_idx(ARMMMUIdx_MNegPri);
         }
 
diff --git a/target/arm/helper.c b/target/arm/helper.c
index f087d42..b64ddb1 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6172,7 +6172,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
 
     if (env->v7m.exception != ARMV7M_EXCP_NMI) {
         /* Auto-clear FAULTMASK on return from other than NMI */
-        env->daif &= ~PSTATE_F;
+        env->v7m.faultmask = 0;
     }
 
     switch (armv7m_nvic_complete_irq(env->nvic, env->v7m.exception)) {
@@ -8718,12 +8718,12 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)
         return (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) ?
             env->regs[13] : env->v7m.other_sp;
     case 16: /* PRIMASK */
-        return (env->daif & PSTATE_I) != 0;
+        return env->v7m.primask;
     case 17: /* BASEPRI */
     case 18: /* BASEPRI_MAX */
         return env->v7m.basepri;
     case 19: /* FAULTMASK */
-        return (env->daif & PSTATE_F) != 0;
+        return env->v7m.faultmask;
     default:
         qemu_log_mask(LOG_GUEST_ERROR, "Attempt to read unknown special"
                                        " register %d\n", reg);
@@ -8778,11 +8778,7 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
         }
         break;
     case 16: /* PRIMASK */
-        if (val & 1) {
-            env->daif |= PSTATE_I;
-        } else {
-            env->daif &= ~PSTATE_I;
-        }
+        env->v7m.primask = val & 1;
         break;
     case 17: /* BASEPRI */
         env->v7m.basepri = val & 0xff;
@@ -8793,11 +8789,7 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
             env->v7m.basepri = val;
         break;
     case 19: /* FAULTMASK */
-        if (val & 1) {
-            env->daif |= PSTATE_F;
-        } else {
-            env->daif &= ~PSTATE_F;
-        }
+        env->v7m.faultmask = val & 1;
         break;
     case 20: /* CONTROL */
         /* Writing to the SPSEL bit only has an effect if we are in
diff --git a/target/arm/machine.c b/target/arm/machine.c
index 1f66da4..2fb4b76 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -97,6 +97,17 @@ static bool m_needed(void *opaque)
     return arm_feature(env, ARM_FEATURE_M);
 }
 
+static const VMStateDescription vmstate_m_faultmask_primask = {
+    .name = "cpu/m/faultmask-primask",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(env.v7m.faultmask, ARMCPU),
+        VMSTATE_UINT32(env.v7m.primask, ARMCPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_m = {
     .name = "cpu/m",
     .version_id = 4,
@@ -115,6 +126,10 @@ static const VMStateDescription vmstate_m = {
         VMSTATE_UINT32(env.v7m.mpu_ctrl, ARMCPU),
         VMSTATE_INT32(env.v7m.exception, ARMCPU),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription*[]) {
+        &vmstate_m_faultmask_primask,
+        NULL
     }
 };
 
@@ -201,6 +216,24 @@ static int get_cpsr(QEMUFile *f, void *opaque, size_t size,
     CPUARMState *env = &cpu->env;
     uint32_t val = qemu_get_be32(f);
 
+    if (arm_feature(env, ARM_FEATURE_M)) {
+        /* If the I or F bits are set then this is a migration from
+         * an old QEMU which still stored the M profile FAULTMASK
+         * and PRIMASK in env->daif. Set v7m.faultmask and v7m.primask
+         * accordingly, and then clear the bits so they don't confuse
+         * cpsr_write(). For a new QEMU, the bits here will always be
+         * clear, and the data is transferred using the
+         * vmstate_m_faultmask_primask subsection.
+         */
+        if (val & CPSR_F) {
+            env->v7m.faultmask = 1;
+        }
+        if (val & CPSR_I) {
+            env->v7m.primask = 1;
+        }
+        val &= ~(CPSR_F | CPSR_I);
+    }
+
     env->aarch64 = ((val & PSTATE_nRW) == 0);
 
     if (is_a64(env)) {
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH 09/15] target/arm: Don't store M profile PRIMASK and FAULTMASK in daif

[Edgar E. Iglesias]

On Wed, Aug 02, 2017 at 05:43:55PM +0100, Peter Maydell wrote:
> We currently store the M profile CPU register state PRIMASK and
> FAULTMASK in the daif field of the CPU state in its I and F
> bits. This is a legacy from the original implementation, which
> tried to share the cpu_exec_interrupt code between A profile
> and M profile. We've since separated out the two cases because
> they are significantly different, so now there is no common
> code between M and A profile which looks at env->daif: all the
> uses are either in A-only or M-only code paths. Sharing the state
> fields now is just confusing, and will make things awkward
> when we implement v8M, where the PRIMASK and FAULTMASK
> registers are banked between security states.
> 
> Switch M profile over to using v7m.faultmask and v7m.primask
> fields for these registers.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/armv7m_nvic.c |  4 ++--
>  target/arm/cpu.c      |  5 -----
>  target/arm/cpu.h      |  4 +++-
>  target/arm/helper.c   | 18 +++++-------------
>  target/arm/machine.c  | 33 +++++++++++++++++++++++++++++++++
>  5 files changed, 43 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 2e8166a..343bc16 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -167,9 +167,9 @@ static inline int nvic_exec_prio(NVICState *s)
>      CPUARMState *env = &s->cpu->env;
>      int running;
>  
> -    if (env->daif & PSTATE_F) { /* FAULTMASK */
> +    if (env->v7m.faultmask) {
>          running = -1;
> -    } else if (env->daif & PSTATE_I) { /* PRIMASK */
> +    } else if (env->v7m.primask) {
>          running = 0;
>      } else if (env->v7m.basepri > 0) {
>          running = env->v7m.basepri & nvic_gprio_mask(s);
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 05c038b..b241a63 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -185,11 +185,6 @@ static void arm_cpu_reset(CPUState *s)
>          uint32_t initial_pc; /* Loaded from 0x4 */
>          uint8_t *rom;
>  
> -        /* For M profile we store FAULTMASK and PRIMASK in the
> -         * PSTATE F and I bits; these are both clear at reset.
> -         */
> -        env->daif &= ~(PSTATE_I | PSTATE_F);
> -
>          /* The reset value of this bit is IMPDEF, but ARM recommends
>           * that it resets to 1, so QEMU always does that rather than making
>           * it dependent on CPU model.
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 1f06de0..da90b7a 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -418,6 +418,8 @@ typedef struct CPUARMState {
>          uint32_t bfar; /* BusFault Address */
>          unsigned mpu_ctrl; /* MPU_CTRL */
>          int exception;
> +        uint32_t primask;
> +        uint32_t faultmask;

It seems like these could be booleans?

Cheers,
Edgar


>      } v7m;
>  
>      /* Information associated with an exception about to be taken:
> @@ -2179,7 +2181,7 @@ static inline int cpu_mmu_index(CPUARMState *env, bool ifetch)
>           * we're in a HardFault or NMI handler.
>           */
>          if ((env->v7m.exception > 0 && env->v7m.exception <= 3)
> -            || env->daif & PSTATE_F) {
> +            || env->v7m.faultmask) {
>              return arm_to_core_mmu_idx(ARMMMUIdx_MNegPri);
>          }
>  
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index f087d42..b64ddb1 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6172,7 +6172,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>  
>      if (env->v7m.exception != ARMV7M_EXCP_NMI) {
>          /* Auto-clear FAULTMASK on return from other than NMI */
> -        env->daif &= ~PSTATE_F;
> +        env->v7m.faultmask = 0;
>      }
>  
>      switch (armv7m_nvic_complete_irq(env->nvic, env->v7m.exception)) {
> @@ -8718,12 +8718,12 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)
>          return (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) ?
>              env->regs[13] : env->v7m.other_sp;
>      case 16: /* PRIMASK */
> -        return (env->daif & PSTATE_I) != 0;
> +        return env->v7m.primask;
>      case 17: /* BASEPRI */
>      case 18: /* BASEPRI_MAX */
>          return env->v7m.basepri;
>      case 19: /* FAULTMASK */
> -        return (env->daif & PSTATE_F) != 0;
> +        return env->v7m.faultmask;
>      default:
>          qemu_log_mask(LOG_GUEST_ERROR, "Attempt to read unknown special"
>                                         " register %d\n", reg);
> @@ -8778,11 +8778,7 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
>          }
>          break;
>      case 16: /* PRIMASK */
> -        if (val & 1) {
> -            env->daif |= PSTATE_I;
> -        } else {
> -            env->daif &= ~PSTATE_I;
> -        }
> +        env->v7m.primask = val & 1;
>          break;
>      case 17: /* BASEPRI */
>          env->v7m.basepri = val & 0xff;
> @@ -8793,11 +8789,7 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
>              env->v7m.basepri = val;
>          break;
>      case 19: /* FAULTMASK */
> -        if (val & 1) {
> -            env->daif |= PSTATE_F;
> -        } else {
> -            env->daif &= ~PSTATE_F;
> -        }
> +        env->v7m.faultmask = val & 1;
>          break;
>      case 20: /* CONTROL */
>          /* Writing to the SPSEL bit only has an effect if we are in
> diff --git a/target/arm/machine.c b/target/arm/machine.c
> index 1f66da4..2fb4b76 100644
> --- a/target/arm/machine.c
> +++ b/target/arm/machine.c
> @@ -97,6 +97,17 @@ static bool m_needed(void *opaque)
>      return arm_feature(env, ARM_FEATURE_M);
>  }
>  
> +static const VMStateDescription vmstate_m_faultmask_primask = {
> +    .name = "cpu/m/faultmask-primask",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(env.v7m.faultmask, ARMCPU),
> +        VMSTATE_UINT32(env.v7m.primask, ARMCPU),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_m = {
>      .name = "cpu/m",
>      .version_id = 4,
> @@ -115,6 +126,10 @@ static const VMStateDescription vmstate_m = {
>          VMSTATE_UINT32(env.v7m.mpu_ctrl, ARMCPU),
>          VMSTATE_INT32(env.v7m.exception, ARMCPU),
>          VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (const VMStateDescription*[]) {
> +        &vmstate_m_faultmask_primask,
> +        NULL
>      }
>  };
>  
> @@ -201,6 +216,24 @@ static int get_cpsr(QEMUFile *f, void *opaque, size_t size,
>      CPUARMState *env = &cpu->env;
>      uint32_t val = qemu_get_be32(f);
>  
> +    if (arm_feature(env, ARM_FEATURE_M)) {
> +        /* If the I or F bits are set then this is a migration from
> +         * an old QEMU which still stored the M profile FAULTMASK
> +         * and PRIMASK in env->daif. Set v7m.faultmask and v7m.primask
> +         * accordingly, and then clear the bits so they don't confuse
> +         * cpsr_write(). For a new QEMU, the bits here will always be
> +         * clear, and the data is transferred using the
> +         * vmstate_m_faultmask_primask subsection.
> +         */
> +        if (val & CPSR_F) {
> +            env->v7m.faultmask = 1;
> +        }
> +        if (val & CPSR_I) {
> +            env->v7m.primask = 1;
> +        }
> +        val &= ~(CPSR_F | CPSR_I);
> +    }
> +
>      env->aarch64 = ((val & PSTATE_nRW) == 0);
>  
>      if (is_a64(env)) {
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 09/15] target/arm: Don't store M profile PRIMASK and FAULTMASK in daif

[Richard Henderson]

On 08/03/2017 08:38 AM, Edgar E. Iglesias wrote:
>> +        uint32_t primask;
>> +        uint32_t faultmask;
> It seems like these could be booleans?

I was thinking the same thing until I read the v8m description as a 32-bit
register.  This makes qemu match the spec, which has value.


r~

Re: [Qemu-devel] [Qemu-arm] [PATCH 09/15] target/arm: Don't store M profile PRIMASK and FAULTMASK in daif

[Edgar E. Iglesias]

On Thu, Aug 03, 2017 at 03:05:17PM -0700, Richard Henderson wrote:
> On 08/03/2017 08:38 AM, Edgar E. Iglesias wrote:
> >> +        uint32_t primask;
> >> +        uint32_t faultmask;
> > It seems like these could be booleans?
> 
> I was thinking the same thing until I read the v8m description as a 32-bit
> register.  This makes qemu match the spec, which has value.
>

Ah, I didn't check the spec :-)

Re: [Qemu-devel] [PATCH 09/15] target/arm: Don't store M profile PRIMASK and FAULTMASK in daif

[Richard Henderson]

On 08/02/2017 09:43 AM, Peter Maydell wrote:
> We currently store the M profile CPU register state PRIMASK and
> FAULTMASK in the daif field of the CPU state in its I and F
> bits. This is a legacy from the original implementation, which
> tried to share the cpu_exec_interrupt code between A profile
> and M profile. We've since separated out the two cases because
> they are significantly different, so now there is no common
> code between M and A profile which looks at env->daif: all the
> uses are either in A-only or M-only code paths. Sharing the state
> fields now is just confusing, and will make things awkward
> when we implement v8M, where the PRIMASK and FAULTMASK
> registers are banked between security states.
> 
> Switch M profile over to using v7m.faultmask and v7m.primask
> fields for these registers.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/armv7m_nvic.c |  4 ++--
>  target/arm/cpu.c      |  5 -----
>  target/arm/cpu.h      |  4 +++-
>  target/arm/helper.c   | 18 +++++-------------
>  target/arm/machine.c  | 33 +++++++++++++++++++++++++++++++++
>  5 files changed, 43 insertions(+), 21 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 10/15] target/arm: Don't use cpsr_write/cpsr_read to transfer M profile XPSR

[Peter Maydell]

For M profile the XPSR is a similar but not identical format to the
A profile CPSR/SPSR. (For instance the Thumb bit is in a different
place.) For guest accesses we make the M profile code go through
xpsr_read() and xpsr_write() which handle the different layout.
However for migration we use cpsr_read() and cpsr_write() to
marshal state into and out of the migration data stream. This
is pretty confusing and works more by luck than anything else.
Make M profile migration use xpsr_read() and xpsr_write() instead.

The most complicated part of this is handling the possibility
that the migration source is an older QEMU which hands us a
CPSR format value; helpfully we can always tell the two apart.

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

diff --git a/target/arm/machine.c b/target/arm/machine.c
index 2fb4b76..3193b00 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -217,21 +217,37 @@ static int get_cpsr(QEMUFile *f, void *opaque, size_t size,
     uint32_t val = qemu_get_be32(f);
 
     if (arm_feature(env, ARM_FEATURE_M)) {
-        /* If the I or F bits are set then this is a migration from
-         * an old QEMU which still stored the M profile FAULTMASK
-         * and PRIMASK in env->daif. Set v7m.faultmask and v7m.primask
-         * accordingly, and then clear the bits so they don't confuse
-         * cpsr_write(). For a new QEMU, the bits here will always be
-         * clear, and the data is transferred using the
-         * vmstate_m_faultmask_primask subsection.
-         */
-        if (val & CPSR_F) {
-            env->v7m.faultmask = 1;
-        }
-        if (val & CPSR_I) {
-            env->v7m.primask = 1;
+        if (val & XPSR_EXCP) {
+            /* This is a CPSR format value from an older QEMU. (We can tell
+             * because values transferred in XPSR format always have zero
+             * for the EXCP field, and CPSR format will always have bit 4
+             * set in CPSR_M.) Rearrange it into XPSR format. The significant
+             * differences are that the T bit is not in the same place, the
+             * primask/faultmask info may be in the CPSR I and F bits, and
+             * we do not want the mode bits.
+             */
+            uint32_t newval = val;
+
+            newval &= (CPSR_NZCV | CPSR_Q | CPSR_IT | CPSR_GE);
+            if (val & CPSR_T) {
+                newval |= XPSR_T;
+            }
+            /* If the I or F bits are set then this is a migration from
+             * an old QEMU which still stored the M profile FAULTMASK
+             * and PRIMASK in env->daif. For a new QEMU, the data is
+             * transferred using the vmstate_m_faultmask_primask subsection.
+             */
+            if (val & CPSR_F) {
+                env->v7m.faultmask = 1;
+            }
+            if (val & CPSR_I) {
+                env->v7m.primask = 1;
+            }
+            val = newval;
         }
-        val &= ~(CPSR_F | CPSR_I);
+        /* Ignore the low bits, they are handled by vmstate_m. */
+        xpsr_write(env, val, ~XPSR_EXCP);
+        return 0;
     }
 
     env->aarch64 = ((val & PSTATE_nRW) == 0);
@@ -252,7 +268,10 @@ static int put_cpsr(QEMUFile *f, void *opaque, size_t size,
     CPUARMState *env = &cpu->env;
     uint32_t val;
 
-    if (is_a64(env)) {
+    if (arm_feature(env, ARM_FEATURE_M)) {
+        /* The low 9 bits are v7m.exception, which is handled by vmstate_m. */
+        val = xpsr_read(env) & ~XPSR_EXCP;
+    } else if (is_a64(env)) {
         val = pstate_read(env);
     } else {
         val = cpsr_read(env);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 10/15] target/arm: Don't use cpsr_write/cpsr_read to transfer M profile XPSR

[Richard Henderson]

On 08/02/2017 09:43 AM, Peter Maydell wrote:
> +        if (val & XPSR_EXCP) {
> +            /* This is a CPSR format value from an older QEMU. (We can tell
> +             * because values transferred in XPSR format always have zero
> +             * for the EXCP field, and CPSR format will always have bit 4
> +             * set in CPSR_M.) Rearrange it into XPSR format. The significant
> +             * differences are that the T bit is not in the same place, the
> +             * primask/faultmask info may be in the CPSR I and F bits, and
> +             * we do not want the mode bits.
> +             */
> +            uint32_t newval = val;
> +
> +            newval &= (CPSR_NZCV | CPSR_Q | CPSR_IT | CPSR_GE);
> +            if (val & CPSR_T) {
> +                newval |= XPSR_T;
> +            }
> +            /* If the I or F bits are set then this is a migration from
> +             * an old QEMU which still stored the M profile FAULTMASK
> +             * and PRIMASK in env->daif. For a new QEMU, the data is
> +             * transferred using the vmstate_m_faultmask_primask subsection.
> +             */

The second comment seems sort of redundant with the first now.


r~

Re: [Qemu-devel] [PATCH 10/15] target/arm: Don't use cpsr_write/cpsr_read to transfer M profile XPSR

[Richard Henderson]

On 08/03/2017 03:13 PM, Richard Henderson wrote:
> On 08/02/2017 09:43 AM, Peter Maydell wrote:
>> +        if (val & XPSR_EXCP) {
>> +            /* This is a CPSR format value from an older QEMU. (We can tell
>> +             * because values transferred in XPSR format always have zero
>> +             * for the EXCP field, and CPSR format will always have bit 4
>> +             * set in CPSR_M.) Rearrange it into XPSR format. The significant
>> +             * differences are that the T bit is not in the same place, the
>> +             * primask/faultmask info may be in the CPSR I and F bits, and
>> +             * we do not want the mode bits.
>> +             */
>> +            uint32_t newval = val;
>> +
>> +            newval &= (CPSR_NZCV | CPSR_Q | CPSR_IT | CPSR_GE);
>> +            if (val & CPSR_T) {
>> +                newval |= XPSR_T;
>> +            }
>> +            /* If the I or F bits are set then this is a migration from
>> +             * an old QEMU which still stored the M profile FAULTMASK
>> +             * and PRIMASK in env->daif. For a new QEMU, the data is
>> +             * transferred using the vmstate_m_faultmask_primask subsection.
>> +             */
> 
> The second comment seems sort of redundant with the first now.

... and I meant to say, otherwise,


Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 10/15] target/arm: Don't use cpsr_write/cpsr_read to transfer M profile XPSR

[Peter Maydell]

On 3 August 2017 at 23:13, Richard Henderson <rth@twiddle.net> wrote:
> On 08/02/2017 09:43 AM, Peter Maydell wrote:
>> +        if (val & XPSR_EXCP) {
>> +            /* This is a CPSR format value from an older QEMU. (We can tell
>> +             * because values transferred in XPSR format always have zero
>> +             * for the EXCP field, and CPSR format will always have bit 4
>> +             * set in CPSR_M.) Rearrange it into XPSR format. The significant
>> +             * differences are that the T bit is not in the same place, the
>> +             * primask/faultmask info may be in the CPSR I and F bits, and
>> +             * we do not want the mode bits.
>> +             */
>> +            uint32_t newval = val;
>> +
>> +            newval &= (CPSR_NZCV | CPSR_Q | CPSR_IT | CPSR_GE);
>> +            if (val & CPSR_T) {
>> +                newval |= XPSR_T;
>> +            }
>> +            /* If the I or F bits are set then this is a migration from
>> +             * an old QEMU which still stored the M profile FAULTMASK
>> +             * and PRIMASK in env->daif. For a new QEMU, the data is
>> +             * transferred using the vmstate_m_faultmask_primask subsection.
>> +             */
>
> The second comment seems sort of redundant with the first now.

I felt that the migration-compat stuff was sufficiently subtle
that it was worth retaining the second detailed comment as well
as the brief summary in the new first comment.

thanks
-- PMM

[Qemu-devel] [PATCH 11/15] target/arm: Make arm_cpu_dump_state() handle the M-profile XPSR

[Peter Maydell]

Make the arm_cpu_dump_state() debug logging handle the M-profile XPSR
rather than assuming it's an A-profile CPSR.  On M profile the PSR
line of a register dump will now look like this:

XPSR=41000000 -Z-- T priv-thread

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

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 3c14cb0..e52a6d7 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -12215,8 +12215,6 @@ void arm_cpu_dump_state(CPUState *cs, FILE *f, fprintf_function cpu_fprintf,
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
     int i;
-    uint32_t psr;
-    const char *ns_status;
 
     if (is_a64(env)) {
         aarch64_cpu_dump_state(cs, f, cpu_fprintf, flags);
@@ -12230,24 +12228,48 @@ void arm_cpu_dump_state(CPUState *cs, FILE *f, fprintf_function cpu_fprintf,
         else
             cpu_fprintf(f, " ");
     }
-    psr = cpsr_read(env);
 
-    if (arm_feature(env, ARM_FEATURE_EL3) &&
-        (psr & CPSR_M) != ARM_CPU_MODE_MON) {
-        ns_status = env->cp15.scr_el3 & SCR_NS ? "NS " : "S ";
+    if (arm_feature(env, ARM_FEATURE_M)) {
+        uint32_t xpsr = xpsr_read(env);
+        const char *mode;
+
+        if (xpsr & XPSR_EXCP) {
+            mode = "handler";
+        } else {
+            if (env->v7m.control & R_V7M_CONTROL_NPRIV_MASK) {
+                mode = "unpriv-thread";
+            } else {
+                mode = "priv-thread";
+            }
+        }
+
+        cpu_fprintf(f, "XPSR=%08x %c%c%c%c %c %s\n",
+                    xpsr,
+                    xpsr & XPSR_N ? 'N' : '-',
+                    xpsr & XPSR_Z ? 'Z' : '-',
+                    xpsr & XPSR_C ? 'C' : '-',
+                    xpsr & XPSR_V ? 'V' : '-',
+                    xpsr & XPSR_T ? 'T' : 'A',
+                    mode);
     } else {
-        ns_status = "";
-    }
-
-    cpu_fprintf(f, "PSR=%08x %c%c%c%c %c %s%s%d\n",
-                psr,
-                psr & (1 << 31) ? 'N' : '-',
-                psr & (1 << 30) ? 'Z' : '-',
-                psr & (1 << 29) ? 'C' : '-',
-                psr & (1 << 28) ? 'V' : '-',
-                psr & CPSR_T ? 'T' : 'A',
-                ns_status,
-                cpu_mode_names[psr & 0xf], (psr & 0x10) ? 32 : 26);
+        uint32_t psr = cpsr_read(env);
+        const char *ns_status = "";
+
+        if (arm_feature(env, ARM_FEATURE_EL3) &&
+            (psr & CPSR_M) != ARM_CPU_MODE_MON) {
+            ns_status = env->cp15.scr_el3 & SCR_NS ? "NS " : "S ";
+        }
+
+        cpu_fprintf(f, "PSR=%08x %c%c%c%c %c %s%s%d\n",
+                    psr,
+                    psr & CPSR_N ? 'N' : '-',
+                    psr & CPSR_Z ? 'Z' : '-',
+                    psr & CPSR_C ? 'C' : '-',
+                    psr & CPSR_V ? 'V' : '-',
+                    psr & CPSR_T ? 'T' : 'A',
+                    ns_status,
+                    cpu_mode_names[psr & 0xf], (psr & 0x10) ? 32 : 26);
+    }
 
     if (flags & CPU_DUMP_FPU) {
         int numvfpregs = 0;
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH 11/15] target/arm: Make arm_cpu_dump_state() handle the M-profile XPSR

[Edgar E. Iglesias]

On Wed, Aug 02, 2017 at 05:43:57PM +0100, Peter Maydell wrote:
> Make the arm_cpu_dump_state() debug logging handle the M-profile XPSR
> rather than assuming it's an A-profile CPSR.  On M profile the PSR
> line of a register dump will now look like this:
> 
> XPSR=41000000 -Z-- T priv-thread
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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


> ---
>  target/arm/translate.c | 58 ++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 40 insertions(+), 18 deletions(-)
> 
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 3c14cb0..e52a6d7 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -12215,8 +12215,6 @@ void arm_cpu_dump_state(CPUState *cs, FILE *f, fprintf_function cpu_fprintf,
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
>      int i;
> -    uint32_t psr;
> -    const char *ns_status;
>  
>      if (is_a64(env)) {
>          aarch64_cpu_dump_state(cs, f, cpu_fprintf, flags);
> @@ -12230,24 +12228,48 @@ void arm_cpu_dump_state(CPUState *cs, FILE *f, fprintf_function cpu_fprintf,
>          else
>              cpu_fprintf(f, " ");
>      }
> -    psr = cpsr_read(env);
>  
> -    if (arm_feature(env, ARM_FEATURE_EL3) &&
> -        (psr & CPSR_M) != ARM_CPU_MODE_MON) {
> -        ns_status = env->cp15.scr_el3 & SCR_NS ? "NS " : "S ";
> +    if (arm_feature(env, ARM_FEATURE_M)) {
> +        uint32_t xpsr = xpsr_read(env);
> +        const char *mode;
> +
> +        if (xpsr & XPSR_EXCP) {
> +            mode = "handler";
> +        } else {
> +            if (env->v7m.control & R_V7M_CONTROL_NPRIV_MASK) {
> +                mode = "unpriv-thread";
> +            } else {
> +                mode = "priv-thread";
> +            }
> +        }
> +
> +        cpu_fprintf(f, "XPSR=%08x %c%c%c%c %c %s\n",
> +                    xpsr,
> +                    xpsr & XPSR_N ? 'N' : '-',
> +                    xpsr & XPSR_Z ? 'Z' : '-',
> +                    xpsr & XPSR_C ? 'C' : '-',
> +                    xpsr & XPSR_V ? 'V' : '-',
> +                    xpsr & XPSR_T ? 'T' : 'A',
> +                    mode);
>      } else {
> -        ns_status = "";
> -    }
> -
> -    cpu_fprintf(f, "PSR=%08x %c%c%c%c %c %s%s%d\n",
> -                psr,
> -                psr & (1 << 31) ? 'N' : '-',
> -                psr & (1 << 30) ? 'Z' : '-',
> -                psr & (1 << 29) ? 'C' : '-',
> -                psr & (1 << 28) ? 'V' : '-',
> -                psr & CPSR_T ? 'T' : 'A',
> -                ns_status,
> -                cpu_mode_names[psr & 0xf], (psr & 0x10) ? 32 : 26);
> +        uint32_t psr = cpsr_read(env);
> +        const char *ns_status = "";
> +
> +        if (arm_feature(env, ARM_FEATURE_EL3) &&
> +            (psr & CPSR_M) != ARM_CPU_MODE_MON) {
> +            ns_status = env->cp15.scr_el3 & SCR_NS ? "NS " : "S ";
> +        }
> +
> +        cpu_fprintf(f, "PSR=%08x %c%c%c%c %c %s%s%d\n",
> +                    psr,
> +                    psr & CPSR_N ? 'N' : '-',
> +                    psr & CPSR_Z ? 'Z' : '-',
> +                    psr & CPSR_C ? 'C' : '-',
> +                    psr & CPSR_V ? 'V' : '-',
> +                    psr & CPSR_T ? 'T' : 'A',
> +                    ns_status,
> +                    cpu_mode_names[psr & 0xf], (psr & 0x10) ? 32 : 26);
> +    }
>  
>      if (flags & CPU_DUMP_FPU) {
>          int numvfpregs = 0;
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH 11/15] target/arm: Make arm_cpu_dump_state() handle the M-profile XPSR

[Richard Henderson]

On 08/02/2017 09:43 AM, Peter Maydell wrote:
> Make the arm_cpu_dump_state() debug logging handle the M-profile XPSR
> rather than assuming it's an A-profile CPSR.  On M profile the PSR
> line of a register dump will now look like this:
> 
> XPSR=41000000 -Z-- T priv-thread
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/translate.c | 58 ++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 40 insertions(+), 18 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 12/15] target/arm: Don't calculate lr in arm_v7m_cpu_do_interrupt() until needed

[Peter Maydell]

Move the code in arm_v7m_cpu_do_interrupt() that calculates the
magic LR value down to when we're actually going to use it.
Having the calculation and use so far apart makes the code
a little harder to understand than it needs to be.

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index b64ddb1..0ecc8f1 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6311,13 +6311,6 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
 
     arm_log_exception(cs->exception_index);
 
-    lr = 0xfffffff1;
-    if (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) {
-        lr |= 4;
-    }
-    if (env->v7m.exception == 0)
-        lr |= 8;
-
     /* For exceptions we just mark as pending on the NVIC, and let that
        handle it.  */
     switch (cs->exception_index) {
@@ -6408,6 +6401,14 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
         return; /* Never happens.  Keep compiler happy.  */
     }
 
+    lr = 0xfffffff1;
+    if (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) {
+        lr |= 4;
+    }
+    if (env->v7m.exception == 0) {
+        lr |= 8;
+    }
+
     v7m_push_stack(cpu);
     v7m_exception_taken(cpu, lr);
     qemu_log_mask(CPU_LOG_INT, "... as %d\n", env->v7m.exception);
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH 12/15] target/arm: Don't calculate lr in arm_v7m_cpu_do_interrupt() until needed

[Philippe Mathieu-Daudé]

On 08/02/2017 01:43 PM, Peter Maydell wrote:
> Move the code in arm_v7m_cpu_do_interrupt() that calculates the
> magic LR value down to when we're actually going to use it.
> Having the calculation and use so far apart makes the code
> a little harder to understand than it needs to be.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>   target/arm/helper.c | 15 ++++++++-------
>   1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index b64ddb1..0ecc8f1 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6311,13 +6311,6 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>   
>       arm_log_exception(cs->exception_index);
>   
> -    lr = 0xfffffff1;
> -    if (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) {
> -        lr |= 4;
> -    }
> -    if (env->v7m.exception == 0)
> -        lr |= 8;
> -
>       /* For exceptions we just mark as pending on the NVIC, and let that
>          handle it.  */
>       switch (cs->exception_index) {
> @@ -6408,6 +6401,14 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>           return; /* Never happens.  Keep compiler happy.  */
>       }
>   
> +    lr = 0xfffffff1;
> +    if (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) {
> +        lr |= 4;
> +    }
> +    if (env->v7m.exception == 0) {
> +        lr |= 8;
> +    }
> +
>       v7m_push_stack(cpu);
>       v7m_exception_taken(cpu, lr);
>       qemu_log_mask(CPU_LOG_INT, "... as %d\n", env->v7m.exception);
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 12/15] target/arm: Don't calculate lr in arm_v7m_cpu_do_interrupt() until needed

[Edgar E. Iglesias]

On Wed, Aug 02, 2017 at 05:43:58PM +0100, Peter Maydell wrote:
> Move the code in arm_v7m_cpu_do_interrupt() that calculates the
> magic LR value down to when we're actually going to use it.
> Having the calculation and use so far apart makes the code
> a little harder to understand than it needs to be.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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


> ---
>  target/arm/helper.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index b64ddb1..0ecc8f1 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6311,13 +6311,6 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>  
>      arm_log_exception(cs->exception_index);
>  
> -    lr = 0xfffffff1;
> -    if (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) {
> -        lr |= 4;
> -    }
> -    if (env->v7m.exception == 0)
> -        lr |= 8;
> -
>      /* For exceptions we just mark as pending on the NVIC, and let that
>         handle it.  */
>      switch (cs->exception_index) {
> @@ -6408,6 +6401,14 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>          return; /* Never happens.  Keep compiler happy.  */
>      }
>  
> +    lr = 0xfffffff1;
> +    if (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) {
> +        lr |= 4;
> +    }
> +    if (env->v7m.exception == 0) {
> +        lr |= 8;
> +    }
> +
>      v7m_push_stack(cpu);
>      v7m_exception_taken(cpu, lr);
>      qemu_log_mask(CPU_LOG_INT, "... as %d\n", env->v7m.exception);
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH 12/15] target/arm: Don't calculate lr in arm_v7m_cpu_do_interrupt() until needed

[Richard Henderson]

On 08/02/2017 09:43 AM, Peter Maydell wrote:
> Move the code in arm_v7m_cpu_do_interrupt() that calculates the
> magic LR value down to when we're actually going to use it.
> Having the calculation and use so far apart makes the code
> a little harder to understand than it needs to be.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 13/15] target/arm: Create and use new function arm_v7m_is_handler_mode()

[Peter Maydell]

Add a utility function for testing whether the CPU is in Handler
mode; this is just a check whether v7m.exception is non-zero, but
we do it in several places and it makes the code a bit easier
to read to not have to mentally figure out what the test is testing.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h    | 10 ++++++++--
 target/arm/helper.c |  8 ++++----
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index da90b7a..a3b4b78 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1630,13 +1630,19 @@ static inline int arm_highest_el(CPUARMState *env)
     return 1;
 }
 
+/* Return true if a v7M CPU is in Handler mode */
+static inline bool arm_v7m_is_handler_mode(CPUARMState *env)
+{
+    return env->v7m.exception != 0;
+}
+
 /* Return the current Exception Level (as per ARMv8; note that this differs
  * from the ARMv7 Privilege Level).
  */
 static inline int arm_current_el(CPUARMState *env)
 {
     if (arm_feature(env, ARM_FEATURE_M)) {
-        return !((env->v7m.exception == 0) && (env->v7m.control & 1));
+        return arm_v7m_is_handler_mode(env) || !(env->v7m.control & 1);
     }
 
     if (is_a64(env)) {
@@ -2636,7 +2642,7 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
     }
     *flags |= fp_exception_el(env) << ARM_TBFLAG_FPEXC_EL_SHIFT;
 
-    if (env->v7m.exception != 0) {
+    if (arm_v7m_is_handler_mode(env)) {
         *flags |= ARM_TBFLAG_HANDLER_MASK;
     }
 
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 0ecc8f1..7920153 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6147,7 +6147,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
      * that jumps to magic addresses don't have magic behaviour unless
      * we're in Handler mode (compare pseudocode BXWritePC()).
      */
-    assert(env->v7m.exception != 0);
+    assert(arm_v7m_is_handler_mode(env));
 
     /* In the spec pseudocode ExceptionReturn() is called directly
      * from BXWritePC() and gets the full target PC value including
@@ -6254,7 +6254,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
      * resuming in Thread mode. If that doesn't match what the
      * exception return type specified then this is a UsageFault.
      */
-    if (return_to_handler == (env->v7m.exception == 0)) {
+    if (return_to_handler != arm_v7m_is_handler_mode(env)) {
         /* Take an INVPC UsageFault by pushing the stack again. */
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
         env->v7m.cfsr |= R_V7M_CFSR_INVPC_MASK;
@@ -6405,7 +6405,7 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
     if (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) {
         lr |= 4;
     }
-    if (env->v7m.exception == 0) {
+    if (!arm_v7m_is_handler_mode(env)) {
         lr |= 8;
     }
 
@@ -8798,7 +8798,7 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
          * switch_v7m_sp() deals with updating the SPSEL bit in
          * env->v7m.control, so we only need update the others.
          */
-        if (env->v7m.exception == 0) {
+        if (!arm_v7m_is_handler_mode(env)) {
             switch_v7m_sp(env, (val & R_V7M_CONTROL_SPSEL_MASK) != 0);
         }
         env->v7m.control &= ~R_V7M_CONTROL_NPRIV_MASK;
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH 13/15] target/arm: Create and use new function arm_v7m_is_handler_mode()

[Philippe Mathieu-Daudé]

On 08/02/2017 01:43 PM, Peter Maydell wrote:
> Add a utility function for testing whether the CPU is in Handler
> mode; this is just a check whether v7m.exception is non-zero, but
> we do it in several places and it makes the code a bit easier
> to read to not have to mentally figure out what the test is testing.

<3 <3 <3

> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>   target/arm/cpu.h    | 10 ++++++++--
>   target/arm/helper.c |  8 ++++----
>   2 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index da90b7a..a3b4b78 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1630,13 +1630,19 @@ static inline int arm_highest_el(CPUARMState *env)
>       return 1;
>   }
>   
> +/* Return true if a v7M CPU is in Handler mode */
> +static inline bool arm_v7m_is_handler_mode(CPUARMState *env)
> +{
> +    return env->v7m.exception != 0;
> +}
> +
>   /* Return the current Exception Level (as per ARMv8; note that this differs
>    * from the ARMv7 Privilege Level).
>    */
>   static inline int arm_current_el(CPUARMState *env)
>   {
>       if (arm_feature(env, ARM_FEATURE_M)) {
> -        return !((env->v7m.exception == 0) && (env->v7m.control & 1));
> +        return arm_v7m_is_handler_mode(env) || !(env->v7m.control & 1);
>       }
>   
>       if (is_a64(env)) {
> @@ -2636,7 +2642,7 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>       }
>       *flags |= fp_exception_el(env) << ARM_TBFLAG_FPEXC_EL_SHIFT;
>   
> -    if (env->v7m.exception != 0) {
> +    if (arm_v7m_is_handler_mode(env)) {
>           *flags |= ARM_TBFLAG_HANDLER_MASK;
>       }
>   
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 0ecc8f1..7920153 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6147,7 +6147,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>        * that jumps to magic addresses don't have magic behaviour unless
>        * we're in Handler mode (compare pseudocode BXWritePC()).
>        */
> -    assert(env->v7m.exception != 0);
> +    assert(arm_v7m_is_handler_mode(env));
>   
>       /* In the spec pseudocode ExceptionReturn() is called directly
>        * from BXWritePC() and gets the full target PC value including
> @@ -6254,7 +6254,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>        * resuming in Thread mode. If that doesn't match what the
>        * exception return type specified then this is a UsageFault.
>        */
> -    if (return_to_handler == (env->v7m.exception == 0)) {
> +    if (return_to_handler != arm_v7m_is_handler_mode(env)) {
>           /* Take an INVPC UsageFault by pushing the stack again. */
>           armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
>           env->v7m.cfsr |= R_V7M_CFSR_INVPC_MASK;
> @@ -6405,7 +6405,7 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>       if (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) {
>           lr |= 4;
>       }
> -    if (env->v7m.exception == 0) {
> +    if (!arm_v7m_is_handler_mode(env)) {
>           lr |= 8;
>       }
>   
> @@ -8798,7 +8798,7 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
>            * switch_v7m_sp() deals with updating the SPSEL bit in
>            * env->v7m.control, so we only need update the others.
>            */
> -        if (env->v7m.exception == 0) {
> +        if (!arm_v7m_is_handler_mode(env)) {
>               switch_v7m_sp(env, (val & R_V7M_CONTROL_SPSEL_MASK) != 0);
>           }
>           env->v7m.control &= ~R_V7M_CONTROL_NPRIV_MASK;
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 13/15] target/arm: Create and use new function arm_v7m_is_handler_mode()

[Edgar E. Iglesias]

On Wed, Aug 02, 2017 at 05:43:59PM +0100, Peter Maydell wrote:
> Add a utility function for testing whether the CPU is in Handler
> mode; this is just a check whether v7m.exception is non-zero, but
> we do it in several places and it makes the code a bit easier
> to read to not have to mentally figure out what the test is testing.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>



> ---
>  target/arm/cpu.h    | 10 ++++++++--
>  target/arm/helper.c |  8 ++++----
>  2 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index da90b7a..a3b4b78 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1630,13 +1630,19 @@ static inline int arm_highest_el(CPUARMState *env)
>      return 1;
>  }
>  
> +/* Return true if a v7M CPU is in Handler mode */
> +static inline bool arm_v7m_is_handler_mode(CPUARMState *env)
> +{
> +    return env->v7m.exception != 0;

The != 0 shouldn't be needed when you return a bool...
Either way:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> +}
> +
>  /* Return the current Exception Level (as per ARMv8; note that this differs
>   * from the ARMv7 Privilege Level).
>   */
>  static inline int arm_current_el(CPUARMState *env)
>  {
>      if (arm_feature(env, ARM_FEATURE_M)) {
> -        return !((env->v7m.exception == 0) && (env->v7m.control & 1));
> +        return arm_v7m_is_handler_mode(env) || !(env->v7m.control & 1);
>      }
>  
>      if (is_a64(env)) {
> @@ -2636,7 +2642,7 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>      }
>      *flags |= fp_exception_el(env) << ARM_TBFLAG_FPEXC_EL_SHIFT;
>  
> -    if (env->v7m.exception != 0) {
> +    if (arm_v7m_is_handler_mode(env)) {
>          *flags |= ARM_TBFLAG_HANDLER_MASK;
>      }
>  
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 0ecc8f1..7920153 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6147,7 +6147,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>       * that jumps to magic addresses don't have magic behaviour unless
>       * we're in Handler mode (compare pseudocode BXWritePC()).
>       */
> -    assert(env->v7m.exception != 0);
> +    assert(arm_v7m_is_handler_mode(env));
>  
>      /* In the spec pseudocode ExceptionReturn() is called directly
>       * from BXWritePC() and gets the full target PC value including
> @@ -6254,7 +6254,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>       * resuming in Thread mode. If that doesn't match what the
>       * exception return type specified then this is a UsageFault.
>       */
> -    if (return_to_handler == (env->v7m.exception == 0)) {
> +    if (return_to_handler != arm_v7m_is_handler_mode(env)) {
>          /* Take an INVPC UsageFault by pushing the stack again. */
>          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
>          env->v7m.cfsr |= R_V7M_CFSR_INVPC_MASK;
> @@ -6405,7 +6405,7 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>      if (env->v7m.control & R_V7M_CONTROL_SPSEL_MASK) {
>          lr |= 4;
>      }
> -    if (env->v7m.exception == 0) {
> +    if (!arm_v7m_is_handler_mode(env)) {
>          lr |= 8;
>      }
>  
> @@ -8798,7 +8798,7 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
>           * switch_v7m_sp() deals with updating the SPSEL bit in
>           * env->v7m.control, so we only need update the others.
>           */
> -        if (env->v7m.exception == 0) {
> +        if (!arm_v7m_is_handler_mode(env)) {
>              switch_v7m_sp(env, (val & R_V7M_CONTROL_SPSEL_MASK) != 0);
>          }
>          env->v7m.control &= ~R_V7M_CONTROL_NPRIV_MASK;
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH 13/15] target/arm: Create and use new function arm_v7m_is_handler_mode()

[Richard Henderson]

On 08/02/2017 09:43 AM, Peter Maydell wrote:
> Add a utility function for testing whether the CPU is in Handler
> mode; this is just a check whether v7m.exception is non-zero, but
> we do it in several places and it makes the code a bit easier
> to read to not have to mentally figure out what the test is testing.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/cpu.h    | 10 ++++++++--
>  target/arm/helper.c |  8 ++++----
>  2 files changed, 12 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 14/15] armv7m_nvic.h: Move from include/hw/arm to include/hw/intc

[Peter Maydell]

The armv7m_nvic.h header file was accidentally placed in
include/hw/arm; move it to include/hw/intc to match where
its corresponding .c file lives.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/armv7m_nvic.c                  | 2 +-
 include/hw/arm/armv7m.h                | 2 +-
 include/hw/{arm => intc}/armv7m_nvic.h | 0
 3 files changed, 2 insertions(+), 2 deletions(-)
 rename include/hw/{arm => intc}/armv7m_nvic.h (100%)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 343bc16..5a18025 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -17,7 +17,7 @@
 #include "hw/sysbus.h"
 #include "qemu/timer.h"
 #include "hw/arm/arm.h"
-#include "hw/arm/armv7m_nvic.h"
+#include "hw/intc/armv7m_nvic.h"
 #include "target/arm/cpu.h"
 #include "exec/exec-all.h"
 #include "qemu/log.h"
diff --git a/include/hw/arm/armv7m.h b/include/hw/arm/armv7m.h
index a9b3f2a..10eb058 100644
--- a/include/hw/arm/armv7m.h
+++ b/include/hw/arm/armv7m.h
@@ -11,7 +11,7 @@
 #define HW_ARM_ARMV7M_H
 
 #include "hw/sysbus.h"
-#include "hw/arm/armv7m_nvic.h"
+#include "hw/intc/armv7m_nvic.h"
 
 #define TYPE_BITBAND "ARM,bitband-memory"
 #define BITBAND(obj) OBJECT_CHECK(BitBandState, (obj), TYPE_BITBAND)
diff --git a/include/hw/arm/armv7m_nvic.h b/include/hw/intc/armv7m_nvic.h
similarity index 100%
rename from include/hw/arm/armv7m_nvic.h
rename to include/hw/intc/armv7m_nvic.h
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH 14/15] armv7m_nvic.h: Move from include/hw/arm to include/hw/intc

[Philippe Mathieu-Daudé]

On 08/02/2017 01:44 PM, Peter Maydell wrote:
> The armv7m_nvic.h header file was accidentally placed in
> include/hw/arm; move it to include/hw/intc to match where
> its corresponding .c file lives.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>   hw/intc/armv7m_nvic.c                  | 2 +-
>   include/hw/arm/armv7m.h                | 2 +-
>   include/hw/{arm => intc}/armv7m_nvic.h | 0
>   3 files changed, 2 insertions(+), 2 deletions(-)
>   rename include/hw/{arm => intc}/armv7m_nvic.h (100%)
> 
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 343bc16..5a18025 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -17,7 +17,7 @@
>   #include "hw/sysbus.h"
>   #include "qemu/timer.h"
>   #include "hw/arm/arm.h"
> -#include "hw/arm/armv7m_nvic.h"
> +#include "hw/intc/armv7m_nvic.h"
>   #include "target/arm/cpu.h"
>   #include "exec/exec-all.h"
>   #include "qemu/log.h"
> diff --git a/include/hw/arm/armv7m.h b/include/hw/arm/armv7m.h
> index a9b3f2a..10eb058 100644
> --- a/include/hw/arm/armv7m.h
> +++ b/include/hw/arm/armv7m.h
> @@ -11,7 +11,7 @@
>   #define HW_ARM_ARMV7M_H
>   
>   #include "hw/sysbus.h"
> -#include "hw/arm/armv7m_nvic.h"
> +#include "hw/intc/armv7m_nvic.h"
>   
>   #define TYPE_BITBAND "ARM,bitband-memory"
>   #define BITBAND(obj) OBJECT_CHECK(BitBandState, (obj), TYPE_BITBAND)
> diff --git a/include/hw/arm/armv7m_nvic.h b/include/hw/intc/armv7m_nvic.h
> similarity index 100%
> rename from include/hw/arm/armv7m_nvic.h
> rename to include/hw/intc/armv7m_nvic.h
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 14/15] armv7m_nvic.h: Move from include/hw/arm to include/hw/intc

[Edgar E. Iglesias]

On Wed, Aug 02, 2017 at 05:44:00PM +0100, Peter Maydell wrote:
> The armv7m_nvic.h header file was accidentally placed in
> include/hw/arm; move it to include/hw/intc to match where
> its corresponding .c file lives.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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


> ---
>  hw/intc/armv7m_nvic.c                  | 2 +-
>  include/hw/arm/armv7m.h                | 2 +-
>  include/hw/{arm => intc}/armv7m_nvic.h | 0
>  3 files changed, 2 insertions(+), 2 deletions(-)
>  rename include/hw/{arm => intc}/armv7m_nvic.h (100%)
> 
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 343bc16..5a18025 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -17,7 +17,7 @@
>  #include "hw/sysbus.h"
>  #include "qemu/timer.h"
>  #include "hw/arm/arm.h"
> -#include "hw/arm/armv7m_nvic.h"
> +#include "hw/intc/armv7m_nvic.h"
>  #include "target/arm/cpu.h"
>  #include "exec/exec-all.h"
>  #include "qemu/log.h"
> diff --git a/include/hw/arm/armv7m.h b/include/hw/arm/armv7m.h
> index a9b3f2a..10eb058 100644
> --- a/include/hw/arm/armv7m.h
> +++ b/include/hw/arm/armv7m.h
> @@ -11,7 +11,7 @@
>  #define HW_ARM_ARMV7M_H
>  
>  #include "hw/sysbus.h"
> -#include "hw/arm/armv7m_nvic.h"
> +#include "hw/intc/armv7m_nvic.h"
>  
>  #define TYPE_BITBAND "ARM,bitband-memory"
>  #define BITBAND(obj) OBJECT_CHECK(BitBandState, (obj), TYPE_BITBAND)
> diff --git a/include/hw/arm/armv7m_nvic.h b/include/hw/intc/armv7m_nvic.h
> similarity index 100%
> rename from include/hw/arm/armv7m_nvic.h
> rename to include/hw/intc/armv7m_nvic.h
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH 14/15] armv7m_nvic.h: Move from include/hw/arm to include/hw/intc

[Richard Henderson]

On 08/02/2017 09:44 AM, Peter Maydell wrote:
> The armv7m_nvic.h header file was accidentally placed in
> include/hw/arm; move it to include/hw/intc to match where
> its corresponding .c file lives.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/armv7m_nvic.c                  | 2 +-
>  include/hw/arm/armv7m.h                | 2 +-
>  include/hw/{arm => intc}/armv7m_nvic.h | 0
>  3 files changed, 2 insertions(+), 2 deletions(-)
>  rename include/hw/{arm => intc}/armv7m_nvic.h (100%)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 15/15] nvic: Implement "user accesses BusFault" SCS region behaviour

[Peter Maydell]

The ARMv7M architecture specifies that most of the addresses in the
PPB region (which includes the NVIC, systick and system registers)
are not accessible to unprivileged accesses, which should
BusFault with a few exceptions:
 * the STIR is configurably user-accessible
 * the ITM (which we don't implement at all) is always
   user-accessible

Implement this by switching the register access functions
to the _with_attrs scheme that lets us distinguish user
mode accesses.

This allows us to pull the handling of the CCR.USERSETMPEND
flag up to the level where we can make it generate a BusFault
as it should for non-permitted accesses.

Note that until the core ARM CPU code implements turning
MEMTX_ERROR into a BusFault the registers will continue to
act as RAZ/WI to user accesses.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/armv7m_nvic.c | 58 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 41 insertions(+), 17 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 5a18025..bbfe2d5 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -733,11 +733,8 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value)
     }
     case 0xf00: /* Software Triggered Interrupt Register */
     {
-        /* user mode can only write to STIR if CCR.USERSETMPEND permits it */
         int excnum = (value & 0x1ff) + NVIC_FIRST_IRQ;
-        if (excnum < s->num_irq &&
-            (arm_current_el(&cpu->env) ||
-             (cpu->env.v7m.ccr & R_V7M_CCR_USERSETMPEND_MASK))) {
+        if (excnum < s->num_irq) {
             armv7m_nvic_set_pending(s, excnum);
         }
         break;
@@ -748,14 +745,32 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value)
     }
 }
 
-static uint64_t nvic_sysreg_read(void *opaque, hwaddr addr,
-                                 unsigned size)
+static bool nvic_user_access_ok(NVICState *s, hwaddr offset)
+{
+    /* Return true if unprivileged access to this register is permitted. */
+    switch (offset) {
+    case 0xf00: /* STIR: accessible only if CCR.USERSETMPEND permits */
+        return s->cpu->env.v7m.ccr & R_V7M_CCR_USERSETMPEND_MASK;
+    default:
+        /* All other user accesses cause a BusFault unconditionally */
+        return false;
+    }
+}
+
+static MemTxResult nvic_sysreg_read(void *opaque, hwaddr addr,
+                                    uint64_t *data, unsigned size,
+                                    MemTxAttrs attrs)
 {
     NVICState *s = (NVICState *)opaque;
     uint32_t offset = addr;
     unsigned i, startvec, end;
     uint32_t val;
 
+    if (attrs.user && !nvic_user_access_ok(s, addr)) {
+        /* Generate BusFault for unprivileged accesses */
+        return MEMTX_ERROR;
+    }
+
     switch (offset) {
     /* reads of set and clear both return the status */
     case 0x100 ... 0x13f: /* NVIC Set enable */
@@ -826,11 +841,13 @@ static uint64_t nvic_sysreg_read(void *opaque, hwaddr addr,
     }
 
     trace_nvic_sysreg_read(addr, val, size);
-    return val;
+    *data = val;
+    return MEMTX_OK;
 }
 
-static void nvic_sysreg_write(void *opaque, hwaddr addr,
-                              uint64_t value, unsigned size)
+static MemTxResult nvic_sysreg_write(void *opaque, hwaddr addr,
+                                     uint64_t value, unsigned size,
+                                     MemTxAttrs attrs)
 {
     NVICState *s = (NVICState *)opaque;
     uint32_t offset = addr;
@@ -839,6 +856,11 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,
 
     trace_nvic_sysreg_write(addr, value, size);
 
+    if (attrs.user && !nvic_user_access_ok(s, addr)) {
+        /* Generate BusFault for unprivileged accesses */
+        return MEMTX_ERROR;
+    }
+
     switch (offset) {
     case 0x100 ... 0x13f: /* NVIC Set enable */
         offset += 0x80;
@@ -853,7 +875,7 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,
             }
         }
         nvic_irq_update(s);
-        return;
+        return MEMTX_OK;
     case 0x200 ... 0x23f: /* NVIC Set pend */
         /* the special logic in armv7m_nvic_set_pending()
          * is not needed since IRQs are never escalated
@@ -870,9 +892,9 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,
             }
         }
         nvic_irq_update(s);
-        return;
+        return MEMTX_OK;
     case 0x300 ... 0x33f: /* NVIC Active */
-        return; /* R/O */
+        return MEMTX_OK; /* R/O */
     case 0x400 ... 0x5ef: /* NVIC Priority */
         startvec = 8 * (offset - 0x400) + NVIC_FIRST_IRQ; /* vector # */
 
@@ -880,26 +902,28 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,
             set_prio(s, startvec + i, (value >> (i * 8)) & 0xff);
         }
         nvic_irq_update(s);
-        return;
+        return MEMTX_OK;
     case 0xd18 ... 0xd23: /* System Handler Priority.  */
         for (i = 0; i < size; i++) {
             unsigned hdlidx = (offset - 0xd14) + i;
             set_prio(s, hdlidx, (value >> (i * 8)) & 0xff);
         }
         nvic_irq_update(s);
-        return;
+        return MEMTX_OK;
     }
     if (size == 4) {
         nvic_writel(s, offset, value);
-        return;
+        return MEMTX_OK;
     }
     qemu_log_mask(LOG_GUEST_ERROR,
                   "NVIC: Bad write of size %d at offset 0x%x\n", size, offset);
+    /* This is UNPREDICTABLE; treat as RAZ/WI */
+    return MEMTX_OK;
 }
 
 static const MemoryRegionOps nvic_sysreg_ops = {
-    .read = nvic_sysreg_read,
-    .write = nvic_sysreg_write,
+    .read_with_attrs = nvic_sysreg_read,
+    .write_with_attrs = nvic_sysreg_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH 15/15] nvic: Implement "user accesses BusFault" SCS region behaviour

[Edgar E. Iglesias]

On Wed, Aug 02, 2017 at 05:44:01PM +0100, Peter Maydell wrote:
> The ARMv7M architecture specifies that most of the addresses in the
> PPB region (which includes the NVIC, systick and system registers)
> are not accessible to unprivileged accesses, which should
> BusFault with a few exceptions:
>  * the STIR is configurably user-accessible
>  * the ITM (which we don't implement at all) is always
>    user-accessible
> 
> Implement this by switching the register access functions
> to the _with_attrs scheme that lets us distinguish user
> mode accesses.
> 
> This allows us to pull the handling of the CCR.USERSETMPEND
> flag up to the level where we can make it generate a BusFault
> as it should for non-permitted accesses.
> 
> Note that until the core ARM CPU code implements turning
> MEMTX_ERROR into a BusFault the registers will continue to
> act as RAZ/WI to user accesses.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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


> ---
>  hw/intc/armv7m_nvic.c | 58 ++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 41 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 5a18025..bbfe2d5 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -733,11 +733,8 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value)
>      }
>      case 0xf00: /* Software Triggered Interrupt Register */
>      {
> -        /* user mode can only write to STIR if CCR.USERSETMPEND permits it */
>          int excnum = (value & 0x1ff) + NVIC_FIRST_IRQ;
> -        if (excnum < s->num_irq &&
> -            (arm_current_el(&cpu->env) ||
> -             (cpu->env.v7m.ccr & R_V7M_CCR_USERSETMPEND_MASK))) {
> +        if (excnum < s->num_irq) {
>              armv7m_nvic_set_pending(s, excnum);
>          }
>          break;
> @@ -748,14 +745,32 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value)
>      }
>  }
>  
> -static uint64_t nvic_sysreg_read(void *opaque, hwaddr addr,
> -                                 unsigned size)
> +static bool nvic_user_access_ok(NVICState *s, hwaddr offset)
> +{
> +    /* Return true if unprivileged access to this register is permitted. */
> +    switch (offset) {
> +    case 0xf00: /* STIR: accessible only if CCR.USERSETMPEND permits */
> +        return s->cpu->env.v7m.ccr & R_V7M_CCR_USERSETMPEND_MASK;
> +    default:
> +        /* All other user accesses cause a BusFault unconditionally */
> +        return false;
> +    }
> +}
> +
> +static MemTxResult nvic_sysreg_read(void *opaque, hwaddr addr,
> +                                    uint64_t *data, unsigned size,
> +                                    MemTxAttrs attrs)
>  {
>      NVICState *s = (NVICState *)opaque;
>      uint32_t offset = addr;
>      unsigned i, startvec, end;
>      uint32_t val;
>  
> +    if (attrs.user && !nvic_user_access_ok(s, addr)) {
> +        /* Generate BusFault for unprivileged accesses */
> +        return MEMTX_ERROR;
> +    }
> +
>      switch (offset) {
>      /* reads of set and clear both return the status */
>      case 0x100 ... 0x13f: /* NVIC Set enable */
> @@ -826,11 +841,13 @@ static uint64_t nvic_sysreg_read(void *opaque, hwaddr addr,
>      }
>  
>      trace_nvic_sysreg_read(addr, val, size);
> -    return val;
> +    *data = val;
> +    return MEMTX_OK;
>  }
>  
> -static void nvic_sysreg_write(void *opaque, hwaddr addr,
> -                              uint64_t value, unsigned size)
> +static MemTxResult nvic_sysreg_write(void *opaque, hwaddr addr,
> +                                     uint64_t value, unsigned size,
> +                                     MemTxAttrs attrs)
>  {
>      NVICState *s = (NVICState *)opaque;
>      uint32_t offset = addr;
> @@ -839,6 +856,11 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,
>  
>      trace_nvic_sysreg_write(addr, value, size);
>  
> +    if (attrs.user && !nvic_user_access_ok(s, addr)) {
> +        /* Generate BusFault for unprivileged accesses */
> +        return MEMTX_ERROR;
> +    }
> +
>      switch (offset) {
>      case 0x100 ... 0x13f: /* NVIC Set enable */
>          offset += 0x80;
> @@ -853,7 +875,7 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,
>              }
>          }
>          nvic_irq_update(s);
> -        return;
> +        return MEMTX_OK;
>      case 0x200 ... 0x23f: /* NVIC Set pend */
>          /* the special logic in armv7m_nvic_set_pending()
>           * is not needed since IRQs are never escalated
> @@ -870,9 +892,9 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,
>              }
>          }
>          nvic_irq_update(s);
> -        return;
> +        return MEMTX_OK;
>      case 0x300 ... 0x33f: /* NVIC Active */
> -        return; /* R/O */
> +        return MEMTX_OK; /* R/O */
>      case 0x400 ... 0x5ef: /* NVIC Priority */
>          startvec = 8 * (offset - 0x400) + NVIC_FIRST_IRQ; /* vector # */
>  
> @@ -880,26 +902,28 @@ static void nvic_sysreg_write(void *opaque, hwaddr addr,
>              set_prio(s, startvec + i, (value >> (i * 8)) & 0xff);
>          }
>          nvic_irq_update(s);
> -        return;
> +        return MEMTX_OK;
>      case 0xd18 ... 0xd23: /* System Handler Priority.  */
>          for (i = 0; i < size; i++) {
>              unsigned hdlidx = (offset - 0xd14) + i;
>              set_prio(s, hdlidx, (value >> (i * 8)) & 0xff);
>          }
>          nvic_irq_update(s);
> -        return;
> +        return MEMTX_OK;
>      }
>      if (size == 4) {
>          nvic_writel(s, offset, value);
> -        return;
> +        return MEMTX_OK;
>      }
>      qemu_log_mask(LOG_GUEST_ERROR,
>                    "NVIC: Bad write of size %d at offset 0x%x\n", size, offset);
> +    /* This is UNPREDICTABLE; treat as RAZ/WI */
> +    return MEMTX_OK;
>  }
>  
>  static const MemoryRegionOps nvic_sysreg_ops = {
> -    .read = nvic_sysreg_read,
> -    .write = nvic_sysreg_write,
> +    .read_with_attrs = nvic_sysreg_read,
> +    .write_with_attrs = nvic_sysreg_write,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH 15/15] nvic: Implement "user accesses BusFault" SCS region behaviour

[Richard Henderson]

On 08/02/2017 09:44 AM, Peter Maydell wrote:
> The ARMv7M architecture specifies that most of the addresses in the
> PPB region (which includes the NVIC, systick and system registers)
> are not accessible to unprivileged accesses, which should
> BusFault with a few exceptions:
>  * the STIR is configurably user-accessible
>  * the ITM (which we don't implement at all) is always
>    user-accessible
> 
> Implement this by switching the register access functions
> to the _with_attrs scheme that lets us distinguish user
> mode accesses.
> 
> This allows us to pull the handling of the CCR.USERSETMPEND
> flag up to the level where we can make it generate a BusFault
> as it should for non-permitted accesses.
> 
> Note that until the core ARM CPU code implements turning
> MEMTX_ERROR into a BusFault the registers will continue to
> act as RAZ/WI to user accesses.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/armv7m_nvic.c | 58 ++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 41 insertions(+), 17 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~