[Qemu-devel] [PATCH v3 0/4] semihosting fixes

[Qemu-devel] [PATCH v3 0/4] semihosting fixes

[Alex Bennée]

Hi Peter,

I've re-based and re-tested on the post-decodetree changes. In the end
I merged the A32/T32 patches and dropped Richard's r-b as the code has
changed a bit.

Alex Bennée (3):
  target/arm: handle M-profile semihosting at translate time
  target/arm: handle A-profile semihosting at translate time
  target/arm: remove run time semihosting checks

Emilio G. Cota (1):
  atomic_template: fix indentation in GEN_ATOMIC_HELPER

 accel/tcg/atomic_template.h |  2 +-
 target/arm/helper.c         | 96 +++++++++----------------------------
 target/arm/m_helper.c       | 18 +++----
 target/arm/translate.c      | 30 ++++++++++--
 4 files changed, 54 insertions(+), 92 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v3 1/4] target/arm: handle M-profile semihosting at translate time

[Alex Bennée]

We do this for other semihosting calls so we might as well do it for
M-profile as well.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

---
v2
  - update for change to gen_exception_internal_insn API
v3
  - update for decode tree
---
 target/arm/m_helper.c  | 18 ++++++------------
 target/arm/translate.c | 11 ++++++++++-
 2 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index 884d35d2b02..27cd2f3f964 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -2114,19 +2114,13 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
             break;
         }
         break;
+    case EXCP_SEMIHOST:
+        qemu_log_mask(CPU_LOG_INT,
+                      "...handling as semihosting call 0x%x\n",
+                      env->regs[0]);
+        env->regs[0] = do_arm_semihosting(env);
+        return;
     case EXCP_BKPT:
-        if (semihosting_enabled()) {
-            int nr;
-            nr = arm_lduw_code(env, env->regs[15], arm_sctlr_b(env)) & 0xff;
-            if (nr == 0xab) {
-                env->regs[15] += 2;
-                qemu_log_mask(CPU_LOG_INT,
-                              "...handling as semihosting call 0x%x\n",
-                              env->regs[0]);
-                env->regs[0] = do_arm_semihosting(env);
-                return;
-            }
-        }
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_DEBUG, false);
         break;
     case EXCP_IRQ:
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 34bb280e3da..4cda7812bcb 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8424,7 +8424,16 @@ static bool trans_BKPT(DisasContext *s, arg_BKPT *a)
     if (!ENABLE_ARCH_5) {
         return false;
     }
-    gen_exception_bkpt_insn(s, syn_aa32_bkpt(a->imm, false));
+    if (arm_dc_feature(s, ARM_FEATURE_M) &&
+        semihosting_enabled() &&
+#ifndef CONFIG_USER_ONLY
+        s->current_el != 0 &&
+#endif
+        (a->imm == 0xab)) {
+        gen_exception_internal_insn(s, s->base.pc_next, EXCP_SEMIHOST);
+    } else {
+        gen_exception_bkpt_insn(s, syn_aa32_bkpt(a->imm, false));
+    }
     return true;
 }
 
-- 
2.20.1

Re: [Qemu-devel] [PATCH v3 1/4] target/arm: handle M-profile semihosting at translate time

[Richard Henderson]

On 9/6/19 8:47 AM, Alex Bennée wrote:
> +    if (arm_dc_feature(s, ARM_FEATURE_M) &&
> +        semihosting_enabled() &&
> +#ifndef CONFIG_USER_ONLY
> +        s->current_el != 0 &&
> +#endif

This last should be !IS_USER(s), no ifdef.


r~

[Qemu-devel] [PATCH v3 2/4] target/arm: handle A-profile semihosting at translate time

[Alex Bennée]

As for the other semihosting calls we can resolve this at translate
time.

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

---
v2
  - update for change to gen_exception_internal_insn API
v3
  - update for decode tree, merge T32 & A32 commits
  - dropped r-b due to changes
---
 target/arm/translate.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 4cda7812bcb..ed4a97cfb44 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -10222,14 +10222,25 @@ static bool trans_CBZ(DisasContext *s, arg_CBZ *a)
 }
 
 /*
- * Supervisor call
+ * Supervisor call - both T32 & A32 come here so we need to check
+ * which mode we are in when checking for semihosting.
  */
 
 static bool trans_SVC(DisasContext *s, arg_SVC *a)
 {
-    gen_set_pc_im(s, s->base.pc_next);
-    s->svc_imm = a->imm;
-    s->base.is_jmp = DISAS_SWI;
+    const uint32_t semihost_imm = s->thumb ? 0xab : 0x123456;
+
+    if (semihosting_enabled() &&
+#ifndef CONFIG_USER_ONLY
+        s->current_el != 0 &&
+#endif
+        (a->imm == semihost_imm)) {
+        gen_exception_internal_insn(s, s->base.pc_next, EXCP_SEMIHOST);
+    } else {
+        gen_set_pc_im(s, s->base.pc_next);
+        s->svc_imm = a->imm;
+        s->base.is_jmp = DISAS_SWI;
+    }
     return true;
 }
 
-- 
2.20.1

Re: [Qemu-devel] [PATCH v3 2/4] target/arm: handle A-profile semihosting at translate time

[Peter Maydell]

On Fri, 6 Sep 2019 at 13:47, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> As for the other semihosting calls we can resolve this at translate
> time.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v2
>   - update for change to gen_exception_internal_insn API
> v3
>   - update for decode tree, merge T32 & A32 commits
>   - dropped r-b due to changes
> ---
>  target/arm/translate.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 4cda7812bcb..ed4a97cfb44 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -10222,14 +10222,25 @@ static bool trans_CBZ(DisasContext *s, arg_CBZ *a)
>  }
>
>  /*
> - * Supervisor call
> + * Supervisor call - both T32 & A32 come here so we need to check
> + * which mode we are in when checking for semihosting.
>   */
>
>  static bool trans_SVC(DisasContext *s, arg_SVC *a)
>  {
> -    gen_set_pc_im(s, s->base.pc_next);
> -    s->svc_imm = a->imm;
> -    s->base.is_jmp = DISAS_SWI;
> +    const uint32_t semihost_imm = s->thumb ? 0xab : 0x123456;
> +
> +    if (semihosting_enabled() &&
> +#ifndef CONFIG_USER_ONLY
> +        s->current_el != 0 &&
> +#endif
> +        (a->imm == semihost_imm)) {
> +        gen_exception_internal_insn(s, s->base.pc_next, EXCP_SEMIHOST);
> +    } else {
> +        gen_set_pc_im(s, s->base.pc_next);
> +        s->svc_imm = a->imm;
> +        s->base.is_jmp = DISAS_SWI;
> +    }
>      return true;
>  }

Doesn't this accidentally enable semihosting via SVC for
M-profile ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 2/4] target/arm: handle A-profile semihosting at translate time

[Alex Bennée]

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

> On Fri, 6 Sep 2019 at 13:47, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> As for the other semihosting calls we can resolve this at translate
>> time.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> ---
>> v2
>>   - update for change to gen_exception_internal_insn API
>> v3
>>   - update for decode tree, merge T32 & A32 commits
>>   - dropped r-b due to changes
>> ---
>>  target/arm/translate.c | 19 +++++++++++++++----
>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/arm/translate.c b/target/arm/translate.c
>> index 4cda7812bcb..ed4a97cfb44 100644
>> --- a/target/arm/translate.c
>> +++ b/target/arm/translate.c
>> @@ -10222,14 +10222,25 @@ static bool trans_CBZ(DisasContext *s, arg_CBZ *a)
>>  }
>>
>>  /*
>> - * Supervisor call
>> + * Supervisor call - both T32 & A32 come here so we need to check
>> + * which mode we are in when checking for semihosting.
>>   */
>>
>>  static bool trans_SVC(DisasContext *s, arg_SVC *a)
>>  {
>> -    gen_set_pc_im(s, s->base.pc_next);
>> -    s->svc_imm = a->imm;
>> -    s->base.is_jmp = DISAS_SWI;
>> +    const uint32_t semihost_imm = s->thumb ? 0xab : 0x123456;
>> +
>> +    if (semihosting_enabled() &&
>> +#ifndef CONFIG_USER_ONLY
>> +        s->current_el != 0 &&
>> +#endif
>> +        (a->imm == semihost_imm)) {
>> +        gen_exception_internal_insn(s, s->base.pc_next, EXCP_SEMIHOST);
>> +    } else {
>> +        gen_set_pc_im(s, s->base.pc_next);
>> +        s->svc_imm = a->imm;
>> +        s->base.is_jmp = DISAS_SWI;
>> +    }
>>      return true;
>>  }
>
> Doesn't this accidentally enable semihosting via SVC for
> M-profile ?

We must have done that before then. Just gate it with &&
!arm_dc_feature(s, ARM_FEATURE_M) then?

>
> thanks
> -- PMM


--
Alex Bennée

Re: [Qemu-devel] [PATCH v3 2/4] target/arm: handle A-profile semihosting at translate time

[Peter Maydell]

On Fri, 6 Sep 2019 at 14:16, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Fri, 6 Sep 2019 at 13:47, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> >
> > Doesn't this accidentally enable semihosting via SVC for
> > M-profile ?
>
> We must have done that before then.

No, we didn't do it before, because we were handling it in
arm_cpu_do_interrupt(), which is A/R-profile only. The
M-profile code goes via arm_v7m_cpu_do_interrupt() which doesn't
check for semihosting when it sees an EXCP_SWI.

> Just gate it with && !arm_dc_feature(s, ARM_FEATURE_M) then?

Yes, I think that's the right fix.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 2/4] target/arm: handle A-profile semihosting at translate time

[Richard Henderson]

On 9/6/19 8:47 AM, Alex Bennée wrote:
> +    if (semihosting_enabled() &&
> +#ifndef CONFIG_USER_ONLY
> +        s->current_el != 0 &&
> +#endif

!IS_USER(s).


r~

[Qemu-devel] [PATCH v3 3/4] target/arm: remove run time semihosting checks

[Alex Bennée]

Now we do all our checking and use a common EXCP_SEMIHOST for
semihosting operations we can make helper code a lot simpler.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
v2
  - fix re-base conflicts
  - hoist EXCP_SEMIHOST check
  - comment cleanups
v5
  - move CONFIG_TCG ifdefs
---
 target/arm/helper.c | 96 +++++++++++----------------------------------
 1 file changed, 22 insertions(+), 74 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 507026c9154..a87ae6d46a1 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8339,88 +8339,32 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
                   new_el, env->pc, pstate_read(env));
 }
 
-static inline bool check_for_semihosting(CPUState *cs)
-{
+/*
+ * Do semihosting call and set the appropriate return value. All the
+ * permission and validity checks have been done at translate time.
+ *
+ * We only see semihosting exceptions in TCG only as they are not
+ * trapped to the hypervisor in KVM.
+ */
 #ifdef CONFIG_TCG
-    /* Check whether this exception is a semihosting call; if so
-     * then handle it and return true; otherwise return false.
-     */
+static void handle_semihosting(CPUState *cs)
+{
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
 
     if (is_a64(env)) {
-        if (cs->exception_index == EXCP_SEMIHOST) {
-            /* This is always the 64-bit semihosting exception.
-             * The "is this usermode" and "is semihosting enabled"
-             * checks have been done at translate time.
-             */
-            qemu_log_mask(CPU_LOG_INT,
-                          "...handling as semihosting call 0x%" PRIx64 "\n",
-                          env->xregs[0]);
-            env->xregs[0] = do_arm_semihosting(env);
-            return true;
-        }
-        return false;
+        qemu_log_mask(CPU_LOG_INT,
+                      "...handling as semihosting call 0x%" PRIx64 "\n",
+                      env->xregs[0]);
+        env->xregs[0] = do_arm_semihosting(env);
     } else {
-        uint32_t imm;
-
-        /* Only intercept calls from privileged modes, to provide some
-         * semblance of security.
-         */
-        if (cs->exception_index != EXCP_SEMIHOST &&
-            (!semihosting_enabled() ||
-             ((env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_USR))) {
-            return false;
-        }
-
-        switch (cs->exception_index) {
-        case EXCP_SEMIHOST:
-            /* This is always a semihosting call; the "is this usermode"
-             * and "is semihosting enabled" checks have been done at
-             * translate time.
-             */
-            break;
-        case EXCP_SWI:
-            /* Check for semihosting interrupt.  */
-            if (env->thumb) {
-                imm = arm_lduw_code(env, env->regs[15] - 2, arm_sctlr_b(env))
-                    & 0xff;
-                if (imm == 0xab) {
-                    break;
-                }
-            } else {
-                imm = arm_ldl_code(env, env->regs[15] - 4, arm_sctlr_b(env))
-                    & 0xffffff;
-                if (imm == 0x123456) {
-                    break;
-                }
-            }
-            return false;
-        case EXCP_BKPT:
-            /* See if this is a semihosting syscall.  */
-            if (env->thumb) {
-                imm = arm_lduw_code(env, env->regs[15], arm_sctlr_b(env))
-                    & 0xff;
-                if (imm == 0xab) {
-                    env->regs[15] += 2;
-                    break;
-                }
-            }
-            return false;
-        default:
-            return false;
-        }
-
         qemu_log_mask(CPU_LOG_INT,
                       "...handling as semihosting call 0x%x\n",
                       env->regs[0]);
         env->regs[0] = do_arm_semihosting(env);
-        return true;
     }
-#else
-    return false;
-#endif
 }
+#endif
 
 /* Handle a CPU exception for A and R profile CPUs.
  * Do any appropriate logging, handle PSCI calls, and then hand off
@@ -8451,13 +8395,17 @@ void arm_cpu_do_interrupt(CPUState *cs)
         return;
     }
 
-    /* Semihosting semantics depend on the register width of the
-     * code that caused the exception, not the target exception level,
-     * so must be handled here.
+    /*
+     * Semihosting semantics depend on the register width of the code
+     * that caused the exception, not the target exception level, so
+     * must be handled here.
      */
-    if (check_for_semihosting(cs)) {
+#ifdef CONFIG_TCG
+    if (cs->exception_index == EXCP_SEMIHOST) {
+        handle_semihosting(cs);
         return;
     }
+#endif
 
     /* Hooks may change global state so BQL should be held, also the
      * BQL needs to be held for any modification of
-- 
2.20.1

[Qemu-devel] [PATCH v3 4/4] atomic_template: fix indentation in GEN_ATOMIC_HELPER

[Alex Bennée]

From: "Emilio G. Cota" <cota@braap.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Emilio G. Cota <cota@braap.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 accel/tcg/atomic_template.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/accel/tcg/atomic_template.h b/accel/tcg/atomic_template.h
index df9c8388178..287433d809b 100644
--- a/accel/tcg/atomic_template.h
+++ b/accel/tcg/atomic_template.h
@@ -149,7 +149,7 @@ ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, target_ulong addr,
 
 #define GEN_ATOMIC_HELPER(X)                                        \
 ABI_TYPE ATOMIC_NAME(X)(CPUArchState *env, target_ulong addr,       \
-                 ABI_TYPE val EXTRA_ARGS)                           \
+                        ABI_TYPE val EXTRA_ARGS)                    \
 {                                                                   \
     ATOMIC_MMU_DECLS;                                               \
     DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;                           \
-- 
2.20.1