[Qemu-devel] [PATCH 0/6] Six minor M-profile bugfixes

[Qemu-devel] [PATCH 0/6] Six minor M-profile bugfixes

[Peter Maydell]

This series fixes half a dozen miscellaneous minor bugs
in our M-profile emulation. (The condbits fix also affects
A-profile cores.)

thanks
-- PMM

Peter Maydell (6):
  target/arm: NS BusFault on vector table fetch escalates to NS
    HardFault
  arm v8M: Forcibly clear negative-priority exceptions on deactivate
  target/arm: v8M: Check state of exception being returned from
  target/arm: Use _ra versions of cpu_stl_data() in v7M helpers
  hw/timer/armv7m_systick: Forbid non-privileged accesses
  target/arm: Execute Thumb instructions when their condbits are 0xf

 hw/intc/armv7m_nvic.c     | 54 ++++++++++++++++++++++++++++++++++-----
 hw/timer/armv7m_systick.c | 26 ++++++++++++++-----
 target/arm/helper.c       | 45 ++++++++++++++++++++++----------
 target/arm/translate.c    | 15 +++++++++--
 4 files changed, 112 insertions(+), 28 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/6] target/arm: NS BusFault on vector table fetch escalates to NS HardFault

[Peter Maydell]

In the M-profile architecture, when we do a vector table fetch and it
fails, we need to report a HardFault.  Whether this is a Secure HF or
a NonSecure HF depends on several things.  If AIRCR.BFHFNMINS is 0
then HF is always Secure, because there is no NonSecure HardFault.
Otherwise, the answer depends on whether the 'underlying exception'
(MemManage, BusFault, SecureFault) targets Secure or NonSecure.  (In
the pseudocode, this is handled in the Vector() function: the final
exc.isSecure is calculated by looking at the exc.isSecure from the
exception returned from the memory access, not the isSecure input
argument.)

We weren't doing this correctly, because we were looking at
the target security domain of the exception we were trying to
load the vector table entry for. This produces errors of two kinds:
 * a load from the NS vector table which hits the "NS access
   to S memory" SecureFault should end up as a Secure HardFault,
   but we were raising an NS HardFault
 * a load from the S vector table which causes a BusFault
   should raise an NS HardFault if BFHFNMINS == 1 (because
   in that case all BusFaults are NonSecure), but we were raising
   a Secure HardFault

Correct the logic.

We also fix a comment error where we claimed that we might
be escalating MemManage to HardFault, and forgot about SecureFault.
(Vector loads can never hit MPU access faults, because they're
always aligned and always use the default address map.)

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index df4276f5f6c..375249d3c72 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8225,7 +8225,11 @@ static bool arm_v7m_load_vector(ARMCPU *cpu, int exc, bool targets_secure,
         if (sattrs.ns) {
             attrs.secure = false;
         } else if (!targets_secure) {
-            /* NS access to S memory */
+            /*
+             * NS access to S memory: the underlying exception which we escalate
+             * to HardFault is SecureFault, which always targets Secure.
+             */
+            exc_secure = true;
             goto load_fail;
         }
     }
@@ -8233,21 +8237,31 @@ static bool arm_v7m_load_vector(ARMCPU *cpu, int exc, bool targets_secure,
     vector_entry = address_space_ldl(arm_addressspace(cs, attrs), addr,
                                      attrs, &result);
     if (result != MEMTX_OK) {
+        /*
+         * Underlying exception is BusFault: its target security state
+         * depends on BFHFNMINS.
+         */
+        exc_secure = !(cpu->env.v7m.aircr & R_V7M_AIRCR_BFHFNMINS_MASK);
         goto load_fail;
     }
     *pvec = vector_entry;
     return true;
 
 load_fail:
-    /* All vector table fetch fails are reported as HardFault, with
+    /*
+     * All vector table fetch fails are reported as HardFault, with
      * HFSR.VECTTBL and .FORCED set. (FORCED is set because
-     * technically the underlying exception is a MemManage or BusFault
+     * technically the underlying exception is a SecureFault or BusFault
      * that is escalated to HardFault.) This is a terminal exception,
      * so we will either take the HardFault immediately or else enter
      * lockup (the latter case is handled in armv7m_nvic_set_pending_derived()).
+     * The HardFault is Secure if BFHFNMINS is 0 (meaning that all HFs are
+     * secure); otherwise it targets the same security state as the
+     * underlying exception.
      */
-    exc_secure = targets_secure ||
-        !(cpu->env.v7m.aircr & R_V7M_AIRCR_BFHFNMINS_MASK);
+    if (!(cpu->env.v7m.aircr & R_V7M_AIRCR_BFHFNMINS_MASK)) {
+        exc_secure = false;
+    }
     env->v7m.hfsr |= R_V7M_HFSR_VECTTBL_MASK | R_V7M_HFSR_FORCED_MASK;
     armv7m_nvic_set_pending_derived(env->nvic, ARMV7M_EXCP_HARD, exc_secure);
     return false;
-- 
2.20.1

Re: [Qemu-devel] [PATCH 1/6] target/arm: NS BusFault on vector table fetch escalates to NS HardFault

[Richard Henderson]

On 6/17/19 10:53 AM, Peter Maydell wrote:
> +     * The HardFault is Secure if BFHFNMINS is 0 (meaning that all HFs are
> +     * secure); otherwise it targets the same security state as the
> +     * underlying exception.
>       */
> -    exc_secure = targets_secure ||
> -        !(cpu->env.v7m.aircr & R_V7M_AIRCR_BFHFNMINS_MASK);
> +    if (!(cpu->env.v7m.aircr & R_V7M_AIRCR_BFHFNMINS_MASK)) {
> +        exc_secure = false;
> +    }

exc_secure = true, surely?


r~

Re: [Qemu-devel] [PATCH 1/6] target/arm: NS BusFault on vector table fetch escalates to NS HardFault

[Peter Maydell]

On Mon, 17 Jun 2019 at 20:28, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 6/17/19 10:53 AM, Peter Maydell wrote:
> > +     * The HardFault is Secure if BFHFNMINS is 0 (meaning that all HFs are
> > +     * secure); otherwise it targets the same security state as the
> > +     * underlying exception.
> >       */
> > -    exc_secure = targets_secure ||
> > -        !(cpu->env.v7m.aircr & R_V7M_AIRCR_BFHFNMINS_MASK);
> > +    if (!(cpu->env.v7m.aircr & R_V7M_AIRCR_BFHFNMINS_MASK)) {
> > +        exc_secure = false;
> > +    }
>
> exc_secure = true, surely?

Yep, good catch. I got it right in the comment text :-)

thanks
-- PMM

[Qemu-devel] [PATCH 2/6] arm v8M: Forcibly clear negative-priority exceptions on deactivate

[Peter Maydell]

To prevent execution priority remaining negative if the guest
returns from an NMI or HardFault with a corrupted IPSR, the
v8M interrupt deactivation process forces the HardFault and NMI
to inactive based on the current raw execution priority,
even if the interrupt the guest is trying to deactivate
is something else. In the pseudocode this is done in the
Deactivate() function.

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

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index b8ede30b3cb..330eb728dd5 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -812,15 +812,45 @@ void armv7m_nvic_get_pending_irq_info(void *opaque,
 int armv7m_nvic_complete_irq(void *opaque, int irq, bool secure)
 {
     NVICState *s = (NVICState *)opaque;
-    VecInfo *vec;
+    VecInfo *vec = NULL;
     int ret;
 
     assert(irq > ARMV7M_EXCP_RESET && irq < s->num_irq);
 
-    if (secure && exc_is_banked(irq)) {
-        vec = &s->sec_vectors[irq];
-    } else {
-        vec = &s->vectors[irq];
+    /*
+     * For negative priorities, v8M will forcibly deactivate the appropriate
+     * NMI or HardFault regardless of what interrupt we're being asked to
+     * deactivate (compare the DeActivate() pseudocode). This is a guard
+     * against software returning from NMI or HardFault with a corrupted
+     * IPSR and leaving the CPU in a negative-priority state.
+     * v7M does not do this, but simply deactivates the requested interrupt.
+     */
+    if (arm_feature(&s->cpu->env, ARM_FEATURE_V8)) {
+        switch (armv7m_nvic_raw_execution_priority(s)) {
+        case -1:
+            if (s->cpu->env.v7m.aircr & R_V7M_AIRCR_BFHFNMINS_MASK) {
+                vec = &s->vectors[ARMV7M_EXCP_HARD];
+            } else {
+                vec = &s->sec_vectors[ARMV7M_EXCP_HARD];
+            }
+            break;
+        case -2:
+            vec = &s->vectors[ARMV7M_EXCP_NMI];
+            break;
+        case -3:
+            vec = &s->sec_vectors[ARMV7M_EXCP_HARD];
+            break;
+        default:
+            break;
+        }
+    }
+
+    if (!vec) {
+        if (secure && exc_is_banked(irq)) {
+            vec = &s->sec_vectors[irq];
+        } else {
+            vec = &s->vectors[irq];
+        }
     }
 
     trace_nvic_complete_irq(irq, secure);
-- 
2.20.1

Re: [Qemu-devel] [PATCH 2/6] arm v8M: Forcibly clear negative-priority exceptions on deactivate

[Richard Henderson]

On 6/17/19 10:53 AM, Peter Maydell wrote:
> To prevent execution priority remaining negative if the guest
> returns from an NMI or HardFault with a corrupted IPSR, the
> v8M interrupt deactivation process forces the HardFault and NMI
> to inactive based on the current raw execution priority,
> even if the interrupt the guest is trying to deactivate
> is something else. In the pseudocode this is done in the
> Deactivate() function.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/armv7m_nvic.c | 40 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 35 insertions(+), 5 deletions(-)

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


r~

[Qemu-devel] [PATCH 3/6] target/arm: v8M: Check state of exception being returned from

[Peter Maydell]

In v8M, an attempt to return from an exception which is not
active is an illegal exception return. For this purpose,
exceptions which can configurably target either Secure or
NonSecure are not considered to be active if they are
configured for the opposite security state for the one
we're trying to return from (eg attempt to return from
an NS NMI but NMI targets Secure). In the pseudocode this
is handled by IsActiveForState().

Detect this case rather than counting an active exception
possibly of the wrong security state as being sufficient.

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

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 330eb728dd5..9f8f0d3ff55 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -860,7 +860,19 @@ int armv7m_nvic_complete_irq(void *opaque, int irq, bool secure)
         return -1;
     }
 
-    ret = nvic_rettobase(s);
+    /*
+     * If this is a configurable exception and it is currently
+     * targeting the opposite security state from the one we're trying
+     * to complete it for, this counts as an illegal exception return.
+     * We still need to deactivate whatever vector the logic above has
+     * selected, though, as it might not be the same as the one for the
+     * requested exception number.
+     */
+    if (!exc_is_banked(irq) && exc_targets_secure(s, irq) != secure) {
+        ret = -1;
+    } else {
+        ret = nvic_rettobase(s);
+    }
 
     vec->active = 0;
     if (vec->level) {
-- 
2.20.1

Re: [Qemu-devel] [PATCH 3/6] target/arm: v8M: Check state of exception being returned from

[Richard Henderson]

On 6/17/19 10:53 AM, Peter Maydell wrote:
> In v8M, an attempt to return from an exception which is not
> active is an illegal exception return. For this purpose,
> exceptions which can configurably target either Secure or
> NonSecure are not considered to be active if they are
> configured for the opposite security state for the one
> we're trying to return from (eg attempt to return from
> an NS NMI but NMI targets Secure). In the pseudocode this
> is handled by IsActiveForState().
> 
> Detect this case rather than counting an active exception
> possibly of the wrong security state as being sufficient.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/armv7m_nvic.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)

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


r~

[Qemu-devel] [PATCH 4/6] target/arm: Use _ra versions of cpu_stl_data() in v7M helpers

[Peter Maydell]

In the various helper functions for v7M/v8M instructions, use
the _ra versions of cpu_stl_data() and friends. Otherwise we
may get wrong behaviour or an assert() due to not being able
to locate the TB if there is an exception on the memory access
or if it performs an IO operation when in icount mode.

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 375249d3c72..866fe54780e 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8141,8 +8141,8 @@ void HELPER(v7m_blxns)(CPUARMState *env, uint32_t dest)
     }
 
     /* Note that these stores can throw exceptions on MPU faults */
-    cpu_stl_data(env, sp, nextinst);
-    cpu_stl_data(env, sp + 4, saved_psr);
+    cpu_stl_data_ra(env, sp, nextinst, GETPC());
+    cpu_stl_data_ra(env, sp + 4, saved_psr, GETPC());
 
     env->regs[13] = sp;
     env->regs[14] = 0xfeffffff;
@@ -8557,6 +8557,7 @@ void HELPER(v7m_vlstm)(CPUARMState *env, uint32_t fptr)
     /* fptr is the value of Rn, the frame pointer we store the FP regs to */
     bool s = env->v7m.fpccr[M_REG_S] & R_V7M_FPCCR_S_MASK;
     bool lspact = env->v7m.fpccr[s] & R_V7M_FPCCR_LSPACT_MASK;
+    uintptr_t ra = GETPC();
 
     assert(env->v7m.secure);
 
@@ -8582,7 +8583,7 @@ void HELPER(v7m_vlstm)(CPUARMState *env, uint32_t fptr)
      * Note that we do not use v7m_stack_write() here, because the
      * accesses should not set the FSR bits for stacking errors if they
      * fail. (In pseudocode terms, they are AccType_NORMAL, not AccType_STACK
-     * or AccType_LAZYFP). Faults in cpu_stl_data() will throw exceptions
+     * or AccType_LAZYFP). Faults in cpu_stl_data_ra() will throw exceptions
      * and longjmp out.
      */
     if (!(env->v7m.fpccr[M_REG_S] & R_V7M_FPCCR_LSPEN_MASK)) {
@@ -8598,10 +8599,10 @@ void HELPER(v7m_vlstm)(CPUARMState *env, uint32_t fptr)
             if (i >= 16) {
                 faddr += 8; /* skip the slot for the FPSCR */
             }
-            cpu_stl_data(env, faddr, slo);
-            cpu_stl_data(env, faddr + 4, shi);
+            cpu_stl_data_ra(env, faddr, slo, ra);
+            cpu_stl_data_ra(env, faddr + 4, shi, ra);
         }
-        cpu_stl_data(env, fptr + 0x40, vfp_get_fpscr(env));
+        cpu_stl_data_ra(env, fptr + 0x40, vfp_get_fpscr(env), ra);
 
         /*
          * If TS is 0 then s0 to s15 and FPSCR are UNKNOWN; we choose to
@@ -8622,6 +8623,8 @@ void HELPER(v7m_vlstm)(CPUARMState *env, uint32_t fptr)
 
 void HELPER(v7m_vlldm)(CPUARMState *env, uint32_t fptr)
 {
+    uintptr_t ra = GETPC();
+
     /* fptr is the value of Rn, the frame pointer we load the FP regs from */
     assert(env->v7m.secure);
 
@@ -8655,13 +8658,13 @@ void HELPER(v7m_vlldm)(CPUARMState *env, uint32_t fptr)
                 faddr += 8; /* skip the slot for the FPSCR */
             }
 
-            slo = cpu_ldl_data(env, faddr);
-            shi = cpu_ldl_data(env, faddr + 4);
+            slo = cpu_ldl_data_ra(env, faddr, ra);
+            shi = cpu_ldl_data_ra(env, faddr + 4, ra);
 
             dn = (uint64_t) shi << 32 | slo;
             *aa32_vfp_dreg(env, i / 2) = dn;
         }
-        fpscr = cpu_ldl_data(env, fptr + 0x40);
+        fpscr = cpu_ldl_data_ra(env, fptr + 0x40, ra);
         vfp_set_fpscr(env, fpscr);
     }
 
-- 
2.20.1

Re: [Qemu-devel] [PATCH 4/6] target/arm: Use _ra versions of cpu_stl_data() in v7M helpers

[Richard Henderson]

On 6/17/19 10:53 AM, Peter Maydell wrote:
> In the various helper functions for v7M/v8M instructions, use
> the _ra versions of cpu_stl_data() and friends. Otherwise we
> may get wrong behaviour or an assert() due to not being able
> to locate the TB if there is an exception on the memory access
> or if it performs an IO operation when in icount mode.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)

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


r~

[Qemu-devel] [PATCH 5/6] hw/timer/armv7m_systick: Forbid non-privileged accesses

[Peter Maydell]

Like most of the v7M memory mapped system registers, the systick
registers are accessible to privileged code only and user accesses
must generate a BusFault. We implement that for registers in
the NVIC proper already, but missed it for systick since we
implement it as a separate device. Correct the omission.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/timer/armv7m_systick.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/hw/timer/armv7m_systick.c b/hw/timer/armv7m_systick.c
index a17317ce2fe..94640743b5d 100644
--- a/hw/timer/armv7m_systick.c
+++ b/hw/timer/armv7m_systick.c
@@ -75,11 +75,17 @@ static void systick_timer_tick(void *opaque)
     }
 }
 
-static uint64_t systick_read(void *opaque, hwaddr addr, unsigned size)
+static MemTxResult systick_read(void *opaque, hwaddr addr, uint64_t *data,
+                                unsigned size, MemTxAttrs attrs)
 {
     SysTickState *s = opaque;
     uint32_t val;
 
+    if (attrs.user) {
+        /* Generate BusFault for unprivileged accesses */
+        return MEMTX_ERROR;
+    }
+
     switch (addr) {
     case 0x0: /* SysTick Control and Status.  */
         val = s->control;
@@ -121,14 +127,21 @@ static uint64_t systick_read(void *opaque, hwaddr addr, unsigned size)
     }
 
     trace_systick_read(addr, val, size);
-    return val;
+    *data = val;
+    return MEMTX_OK;
 }
 
-static void systick_write(void *opaque, hwaddr addr,
-                          uint64_t value, unsigned size)
+static MemTxResult systick_write(void *opaque, hwaddr addr,
+                                 uint64_t value, unsigned size,
+                                 MemTxAttrs attrs)
 {
     SysTickState *s = opaque;
 
+    if (attrs.user) {
+        /* Generate BusFault for unprivileged accesses */
+        return MEMTX_ERROR;
+    }
+
     trace_systick_write(addr, value, size);
 
     switch (addr) {
@@ -172,11 +185,12 @@ static void systick_write(void *opaque, hwaddr addr,
         qemu_log_mask(LOG_GUEST_ERROR,
                       "SysTick: Bad write offset 0x%" HWADDR_PRIx "\n", addr);
     }
+    return MEMTX_OK;
 }
 
 static const MemoryRegionOps systick_ops = {
-    .read = systick_read,
-    .write = systick_write,
+    .read_with_attrs = systick_read,
+    .write_with_attrs = systick_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
     .valid.min_access_size = 4,
     .valid.max_access_size = 4,
-- 
2.20.1

Re: [Qemu-devel] [PATCH 5/6] hw/timer/armv7m_systick: Forbid non-privileged accesses

[Richard Henderson]

On 6/17/19 10:53 AM, Peter Maydell wrote:
> Like most of the v7M memory mapped system registers, the systick
> registers are accessible to privileged code only and user accesses
> must generate a BusFault. We implement that for registers in
> the NVIC proper already, but missed it for systick since we
> implement it as a separate device. Correct the omission.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/timer/armv7m_systick.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)

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


r~

Re: [Qemu-devel] [PATCH 5/6] hw/timer/armv7m_systick: Forbid non-privileged accesses

[Philippe Mathieu-Daudé]

On 6/17/19 7:53 PM, Peter Maydell wrote:
> Like most of the v7M memory mapped system registers, the systick
> registers are accessible to privileged code only and user accesses
> must generate a BusFault. We implement that for registers in
> the NVIC proper already, but missed it for systick since we
> implement it as a separate device. Correct the omission.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/timer/armv7m_systick.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/timer/armv7m_systick.c b/hw/timer/armv7m_systick.c
> index a17317ce2fe..94640743b5d 100644
> --- a/hw/timer/armv7m_systick.c
> +++ b/hw/timer/armv7m_systick.c
> @@ -75,11 +75,17 @@ static void systick_timer_tick(void *opaque)
>      }
>  }
>  
> -static uint64_t systick_read(void *opaque, hwaddr addr, unsigned size)
> +static MemTxResult systick_read(void *opaque, hwaddr addr, uint64_t *data,
> +                                unsigned size, MemTxAttrs attrs)
>  {
>      SysTickState *s = opaque;
>      uint32_t val;
>  
> +    if (attrs.user) {
> +        /* Generate BusFault for unprivileged accesses */
> +        return MEMTX_ERROR;
> +    }
> +
>      switch (addr) {
>      case 0x0: /* SysTick Control and Status.  */
>          val = s->control;
> @@ -121,14 +127,21 @@ static uint64_t systick_read(void *opaque, hwaddr addr, unsigned size)
>      }
>  
>      trace_systick_read(addr, val, size);
> -    return val;
> +    *data = val;
> +    return MEMTX_OK;
>  }
>  
> -static void systick_write(void *opaque, hwaddr addr,
> -                          uint64_t value, unsigned size)
> +static MemTxResult systick_write(void *opaque, hwaddr addr,
> +                                 uint64_t value, unsigned size,
> +                                 MemTxAttrs attrs)
>  {
>      SysTickState *s = opaque;
>  
> +    if (attrs.user) {
> +        /* Generate BusFault for unprivileged accesses */
> +        return MEMTX_ERROR;
> +    }
> +
>      trace_systick_write(addr, value, size);
>  
>      switch (addr) {
> @@ -172,11 +185,12 @@ static void systick_write(void *opaque, hwaddr addr,
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "SysTick: Bad write offset 0x%" HWADDR_PRIx "\n", addr);
>      }
> +    return MEMTX_OK;
>  }
>  
>  static const MemoryRegionOps systick_ops = {
> -    .read = systick_read,
> -    .write = systick_write,
> +    .read_with_attrs = systick_read,
> +    .write_with_attrs = systick_write,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>      .valid.min_access_size = 4,
>      .valid.max_access_size = 4,
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[Qemu-devel] [PATCH 6/6] target/arm: Execute Thumb instructions when their condbits are 0xf

[Peter Maydell]

Thumb instructions in an IT block are set up to be conditionally
executed depending on a set of condition bits encoded into the IT
bits of the CPSR/XPSR.  The architecture specifies that if the
condition bits are 0b1111 this means "always execute" (like 0b1110),
not "never execute"; we were treating it as "never execute".  (See
the ConditionHolds() pseudocode in both the A-profile and M-profile
Arm ARM.)

This is a bit of an obscure corner case, because the only legal
way to get to an 0b1111 set of condbits is to do an exception
return which sets the XPSR/CPSR up that way. An IT instruction
which encodes a condition sequence that would include an 0b1111 is
UNPREDICTABLE, and for v8A the CONSTRAINED UNPREDICTABLE choices
for such an IT insn are to NOP, UNDEF, or treat 0b1111 like 0b1110.
Add a comment noting that we take the latter option.

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

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 4750b9fa1bb..45ea0a11c7c 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -11595,7 +11595,14 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
                 gen_nop_hint(s, (insn >> 4) & 0xf);
                 break;
             }
-            /* If Then.  */
+            /*
+             * IT (If-Then)
+             *
+             * Combinations of firstcond and mask which set up an 0b1111
+             * condition are UNPREDICTABLE; we take the CONSTRAINED
+             * UNPREDICTABLE choice to treat 0b1111 the same as 0b1110,
+             * i.e. both meaning "execute always".
+             */
             s->condexec_cond = (insn >> 4) & 0xe;
             s->condexec_mask = insn & 0x1f;
             /* No actual code generated for this insn, just setup state.  */
@@ -12129,7 +12136,11 @@ static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
     if (dc->condexec_mask && !thumb_insn_is_unconditional(dc, insn)) {
         uint32_t cond = dc->condexec_cond;
 
-        if (cond != 0x0e) {     /* Skip conditional when condition is AL. */
+        /*
+         * Conditionally skip the insn. Note that both 0xe and 0xf mean
+         * "always"; 0xf is not "never".
+         */
+        if (cond < 0x0e) {
             arm_skip_unless(dc, cond);
         }
     }
-- 
2.20.1

Re: [Qemu-devel] [PATCH 6/6] target/arm: Execute Thumb instructions when their condbits are 0xf

[Richard Henderson]

On 6/17/19 10:53 AM, Peter Maydell wrote:
> Thumb instructions in an IT block are set up to be conditionally
> executed depending on a set of condition bits encoded into the IT
> bits of the CPSR/XPSR.  The architecture specifies that if the
> condition bits are 0b1111 this means "always execute" (like 0b1110),
> not "never execute"; we were treating it as "never execute".  (See
> the ConditionHolds() pseudocode in both the A-profile and M-profile
> Arm ARM.)
> 
> This is a bit of an obscure corner case, because the only legal
> way to get to an 0b1111 set of condbits is to do an exception
> return which sets the XPSR/CPSR up that way. An IT instruction
> which encodes a condition sequence that would include an 0b1111 is
> UNPREDICTABLE, and for v8A the CONSTRAINED UNPREDICTABLE choices
> for such an IT insn are to NOP, UNDEF, or treat 0b1111 like 0b1110.
> Add a comment noting that we take the latter option.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/translate.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)

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


r~