diff mbox series

[17/20] target/arm: Implement SG instruction

Message ID 1506092407-26985-18-git-send-email-peter.maydell@linaro.org
State Superseded
Headers show
Series ARM v8M: exception entry, exit and security | expand

Commit Message

Peter Maydell Sept. 22, 2017, 3 p.m. UTC
Implement the SG instruction, which we emulate 'by hand' in the
exception handling code path.

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

---
 target/arm/helper.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 124 insertions(+), 5 deletions(-)

-- 
2.7.4

Comments

Peter Maydell Sept. 22, 2017, 5:18 p.m. UTC | #1
On 22 September 2017 at 16:00, Peter Maydell <peter.maydell@linaro.org> wrote:
> Implement the SG instruction, which we emulate 'by hand' in the

> exception handling code path.


I've just realised that this patch is correct as far as it goes
but it only implements the common path case for SG (where it is
in S&NSC memory and executed by a CPU in NS state). There is
also defined behaviour for:
 * SG in NS memory (behaves as a NOP)
 * SG in S memory and CPU already secure (clears IT bits and
   does nothing else)

Those can be implemented in translate.c in the usual way;
I'll put a patch for that in the next set (or in a respin
of this set).

thanks
-- PMM
Richard Henderson Oct. 5, 2017, 6:50 p.m. UTC | #2
On 09/22/2017 11:00 AM, Peter Maydell wrote:
> Implement the SG instruction, which we emulate 'by hand' in the

> exception handling code path.

> 

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

> ---

>  target/arm/helper.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++++++--

>  1 file changed, 124 insertions(+), 5 deletions(-)

> 

> diff --git a/target/arm/helper.c b/target/arm/helper.c

> index b1ecb66..8df819d 100644

> --- a/target/arm/helper.c

> +++ b/target/arm/helper.c

> @@ -41,6 +41,10 @@ typedef struct V8M_SAttributes {

>      bool irvalid;

>  } V8M_SAttributes;

>  

> +static void v8m_security_lookup(CPUARMState *env, uint32_t address,

> +                                MMUAccessType access_type, ARMMMUIdx mmu_idx,

> +                                V8M_SAttributes *sattrs);

> +

>  /* Definitions for the PMCCNTR and PMCR registers */

>  #define PMCRD   0x8

>  #define PMCRC   0x4

> @@ -6724,6 +6728,123 @@ static void arm_log_exception(int idx)

>      }

>  }

>  

> +static bool v7m_read_half_insn(ARMCPU *cpu, ARMMMUIdx mmu_idx, uint16_t *insn)

> +{


This function doesn't take an address ...

> +    if (get_phys_addr(env, env->regs[15], MMU_INST_FETCH, mmu_idx,

> +                      &physaddr, &attrs, &prot, &page_size, &fsr, &fi)) {


... reading it directly from r15 ...

> +    if (insn != 0xe97f) {

> +        /* Not an SG instruction first half (we choose the IMPDEF

> +         * early-SG-check option).

> +         */

> +        goto gen_invep;

> +    }

> +

> +    if (!v7m_read_half_insn(cpu, mmu_idx, &insn)) {

> +        return false;

> +    }

> +

> +    if (insn != 0xe97f) {

> +        /* Not an SG instruction second half */

> +        goto gen_invep;

> +    }


... but somehow expects to get two different values read from the same address?

Certainly you'd get the wrong exception frame if you incremented r15 in between.

> +    env->regs[15] += 4;


... that make this right and the implicit address to the readers wrong.

I don't see anything else amiss in the patch.


r~
Peter Maydell Oct. 5, 2017, 6:55 p.m. UTC | #3
On 5 October 2017 at 19:50, Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 09/22/2017 11:00 AM, Peter Maydell wrote:

>> Implement the SG instruction, which we emulate 'by hand' in the

>> exception handling code path.

>>

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

>> ---

>>  target/arm/helper.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++++++--

>>  1 file changed, 124 insertions(+), 5 deletions(-)

>>

>> diff --git a/target/arm/helper.c b/target/arm/helper.c

>> index b1ecb66..8df819d 100644

>> --- a/target/arm/helper.c

>> +++ b/target/arm/helper.c

>> @@ -41,6 +41,10 @@ typedef struct V8M_SAttributes {

>>      bool irvalid;

>>  } V8M_SAttributes;

>>

>> +static void v8m_security_lookup(CPUARMState *env, uint32_t address,

>> +                                MMUAccessType access_type, ARMMMUIdx mmu_idx,

>> +                                V8M_SAttributes *sattrs);

>> +

>>  /* Definitions for the PMCCNTR and PMCR registers */

>>  #define PMCRD   0x8

>>  #define PMCRC   0x4

>> @@ -6724,6 +6728,123 @@ static void arm_log_exception(int idx)

>>      }

>>  }

>>

>> +static bool v7m_read_half_insn(ARMCPU *cpu, ARMMMUIdx mmu_idx, uint16_t *insn)

>> +{

>

> This function doesn't take an address ...

>

>> +    if (get_phys_addr(env, env->regs[15], MMU_INST_FETCH, mmu_idx,

>> +                      &physaddr, &attrs, &prot, &page_size, &fsr, &fi)) {

>

> ... reading it directly from r15 ...

>

>> +    if (insn != 0xe97f) {

>> +        /* Not an SG instruction first half (we choose the IMPDEF

>> +         * early-SG-check option).

>> +         */

>> +        goto gen_invep;

>> +    }

>> +

>> +    if (!v7m_read_half_insn(cpu, mmu_idx, &insn)) {

>> +        return false;

>> +    }

>> +

>> +    if (insn != 0xe97f) {

>> +        /* Not an SG instruction second half */

>> +        goto gen_invep;

>> +    }

>

> ... but somehow expects to get two different values read from the same address?

>

> Certainly you'd get the wrong exception frame if you incremented r15 in between.


Oops. I missed this in my testing because it happens that the
two halves of an SG instruction are the same value :-)

thanks
-- PMM
Richard Henderson Oct. 5, 2017, 6:57 p.m. UTC | #4
On 10/05/2017 02:55 PM, Peter Maydell wrote:
> Oops. I missed this in my testing because it happens that the

> two halves of an SG instruction are the same value :-)


Hah.  I didn't notice that either.


r~
diff mbox series

Patch

diff --git a/target/arm/helper.c b/target/arm/helper.c
index b1ecb66..8df819d 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -41,6 +41,10 @@  typedef struct V8M_SAttributes {
     bool irvalid;
 } V8M_SAttributes;
 
+static void v8m_security_lookup(CPUARMState *env, uint32_t address,
+                                MMUAccessType access_type, ARMMMUIdx mmu_idx,
+                                V8M_SAttributes *sattrs);
+
 /* Definitions for the PMCCNTR and PMCR registers */
 #define PMCRD   0x8
 #define PMCRC   0x4
@@ -6724,6 +6728,123 @@  static void arm_log_exception(int idx)
     }
 }
 
+static bool v7m_read_half_insn(ARMCPU *cpu, ARMMMUIdx mmu_idx, uint16_t *insn)
+{
+    /* Load a 16-bit portion of a v7M instruction, returning true on success,
+     * or false on failure (in which case we will have pended the appropriate
+     * exception).
+     * We need to do the instruction fetch's MPU and SAU checks
+     * like this because there is no MMU index that would allow
+     * doing the load with a single function call. Instead we must
+     * first check that the security attributes permit the load
+     * and that they don't mismatch on the two halves of the instruction,
+     * and then we do the load as a secure load (ie using the security
+     * attributes of the address, not the CPU, as architecturally required).
+     */
+    CPUState *cs = CPU(cpu);
+    CPUARMState *env = &cpu->env;
+    V8M_SAttributes sattrs = {};
+    MemTxAttrs attrs = {};
+    ARMMMUFaultInfo fi = {};
+    MemTxResult txres;
+    target_ulong page_size;
+    hwaddr physaddr;
+    int prot;
+    uint32_t fsr;
+
+    v8m_security_lookup(env, env->regs[15], MMU_INST_FETCH, mmu_idx, &sattrs);
+    if (!sattrs.nsc || sattrs.ns) {
+        /* This must be the second half of the insn, and it straddles a
+         * region boundary with the second half not being S&NSC.
+         */
+        env->v7m.sfsr |= R_V7M_SFSR_INVEP_MASK;
+        armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_SECURE, false);
+        qemu_log_mask(CPU_LOG_INT,
+                      "...really SecureFault with SFSR.INVEP\n");
+        return false;
+    }
+    if (get_phys_addr(env, env->regs[15], MMU_INST_FETCH, mmu_idx,
+                      &physaddr, &attrs, &prot, &page_size, &fsr, &fi)) {
+        /* the MPU lookup failed */
+        env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_IACCVIOL_MASK;
+        armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_MEM, env->v7m.secure);
+        qemu_log_mask(CPU_LOG_INT, "...really MemManage with CFSR.IACCVIOL\n");
+        return false;
+    }
+    *insn = address_space_lduw_le(arm_addressspace(cs, attrs), physaddr,
+                                 attrs, &txres);
+    if (txres != MEMTX_OK) {
+        env->v7m.cfsr[M_REG_NS] |= R_V7M_CFSR_IBUSERR_MASK;
+        armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_BUS, false);
+        qemu_log_mask(CPU_LOG_INT, "...really BusFault with CFSR.IBUSERR\n");
+        return false;
+    }
+    return true;
+}
+
+static bool v7m_handle_execute_nsc(ARMCPU *cpu)
+{
+    /* Check whether this attempt to execute code in a Secure & NS-Callable
+     * memory region is for an SG instruction; if so, then emulate the
+     * effect of the SG instruction and return true. Otherwise pend
+     * the correct kind of exception and return false.
+     */
+    CPUARMState *env = &cpu->env;
+    ARMMMUIdx mmu_idx;
+    uint16_t insn;
+
+    /* We should never get here unless get_phys_addr_pmsav8() caused
+     * an exception for NS executing in S&NSC memory.
+     */
+    assert(!env->v7m.secure);
+    assert(arm_feature(env, ARM_FEATURE_M_SECURITY));
+
+    /* We want to do the MPU lookup as secure; work out what mmu_idx that is */
+    mmu_idx = arm_v7m_mmu_idx_for_secstate(env, true);
+
+    if (!v7m_read_half_insn(cpu, mmu_idx, &insn)) {
+        return false;
+    }
+
+    if (!env->thumb) {
+        goto gen_invep;
+    }
+
+    if (insn != 0xe97f) {
+        /* Not an SG instruction first half (we choose the IMPDEF
+         * early-SG-check option).
+         */
+        goto gen_invep;
+    }
+
+    if (!v7m_read_half_insn(cpu, mmu_idx, &insn)) {
+        return false;
+    }
+
+    if (insn != 0xe97f) {
+        /* Not an SG instruction second half */
+        goto gen_invep;
+    }
+
+    /* OK, we have confirmed that we really have an SG instruction.
+     * We know we're NS in S memory so don't need to repeat those checks.
+     */
+    qemu_log_mask(CPU_LOG_INT, "...really an SG instruction at 0x%08" PRIx32
+                  ", executing it\n", env->regs[15]);
+    env->regs[14] &= ~1;
+    switch_v7m_security_state(env, true);
+    xpsr_write(env, 0, XPSR_IT);
+    env->regs[15] += 4;
+    return true;
+
+gen_invep:
+    env->v7m.sfsr |= R_V7M_SFSR_INVEP_MASK;
+    armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_SECURE, false);
+    qemu_log_mask(CPU_LOG_INT,
+                  "...really SecureFault with SFSR.INVEP\n");
+    return false;
+}
+
 void arm_v7m_cpu_do_interrupt(CPUState *cs)
 {
     ARMCPU *cpu = ARM_CPU(cs);
@@ -6766,12 +6887,10 @@  void arm_v7m_cpu_do_interrupt(CPUState *cs)
              * the SG instruction have the same security attributes.)
              * Everything else must generate an INVEP SecureFault, so we
              * emulate the SG instruction here.
-             * TODO: actually emulate SG.
              */
-            env->v7m.sfsr |= R_V7M_SFSR_INVEP_MASK;
-            armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_SECURE, false);
-            qemu_log_mask(CPU_LOG_INT,
-                          "...really SecureFault with SFSR.INVEP\n");
+            if (v7m_handle_execute_nsc(cpu)) {
+                return;
+            }
             break;
         case M_FAKE_FSR_SFAULT:
             /* Various flavours of SecureFault for attempts to execute or