[Qemu-devel] [PATCH v4 0/4] semihosting at translate time fixes

[Qemu-devel] [PATCH v4 0/4] semihosting at translate time fixes

[Alex Bennée]

Hi Peter,

Hopefully this is the final version of the semihosting at translate
time patches. I've applied Richard's IS_USER changes and gated the SVN
for !M profile.

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      | 24 ++++++++--
 4 files changed, 48 insertions(+), 92 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v4 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
v4
  - use !IS_USER
---
 target/arm/m_helper.c  | 18 ++++++------------
 target/arm/translate.c |  8 +++++++-
 2 files changed, 13 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..6689acc911e 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8424,7 +8424,13 @@ 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() && !IS_USER(s) &&
+        (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

[Qemu-devel] [PATCH v4 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
v4
  - !IS_USER and !arm_dc_feature(s, ARM_FEATURE_M)
---
 target/arm/translate.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 6689acc911e..d7ee4f88ace 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -10219,14 +10219,22 @@ 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 (!arm_dc_feature(s, ARM_FEATURE_M) && semihosting_enabled()
+        && !IS_USER(s) && (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

[Qemu-devel] [PATCH v4 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 v4 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

Re: [Qemu-devel] [PATCH v4 0/4] semihosting at translate time fixes

[Peter Maydell]

On Fri, 6 Sep 2019 at 21:26, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Hi Peter,
>
> Hopefully this is the final version of the semihosting at translate
> time patches. I've applied Richard's IS_USER changes and gated the SVN
> for !M profile.
>
> 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

Hi. I've just been looking at these, and I noticed that
they seem to accidentally extend the "no semihosting
in user mode" check that is currently for softmmu only
to also cover linux-user mode (where it would amount
to "never provide semihosting"). This is because we used
to do the check in the helper.c code which is only used
by softmmu, and not in the linux-user/arm/cpu_loop.c
equivalent that linux-user uses. But now we do the check
in translate.c, which is common to both.

There's also some missed cleanup in that the linux-user
code can also have the "maybe EXCP_BKPT/EXCP_SWI is a semihosting
call" checks deleted.

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

I've taken the atomic_template fix into target-arm.next,
since it's unrelated.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 0/4] semihosting at translate time fixes

[Alex Bennée]

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

> On Fri, 6 Sep 2019 at 21:26, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Hi Peter,
>>
>> Hopefully this is the final version of the semihosting at translate
>> time patches. I've applied Richard's IS_USER changes and gated the SVN
>> for !M profile.
>>
>> 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
>
> Hi. I've just been looking at these, and I noticed that
> they seem to accidentally extend the "no semihosting
> in user mode" check that is currently for softmmu only
> to also cover linux-user mode (where it would amount
> to "never provide semihosting").

I misread Richard's comments - he only actually said to drop the #ifndef
CONFIG_USER while using !IS_USER for M profile. I'll return the #ifndef
CONFIG_USER for A-profile.

It does seem a bit weird that userspace linux-user does do semihosting
whereas EL0 in softmmu doesn't. Is that because we are effectively
short-circuiting what a real ARM kernel would be doing for EL0?

> This is because we used
> to do the check in the helper.c code which is only used
> by softmmu, and not in the linux-user/arm/cpu_loop.c
> equivalent that linux-user uses. But now we do the check
> in translate.c, which is common to both.
>
> There's also some missed cleanup in that the linux-user
> code can also have the "maybe EXCP_BKPT/EXCP_SWI is a semihosting
> call" checks deleted.

I'll have a look at that.

>
>> Emilio G. Cota (1):
>>   atomic_template: fix indentation in GEN_ATOMIC_HELPER
>
> I've taken the atomic_template fix into target-arm.next,
> since it's unrelated.
>
> thanks
> -- PMM


--
Alex Bennée

Re: [Qemu-devel] [PATCH v4 0/4] semihosting at translate time fixes

[Peter Maydell]

On Wed, 11 Sep 2019 at 14:14, Alex Bennée <alex.bennee@linaro.org> wrote:
> It does seem a bit weird that userspace linux-user does do semihosting
> whereas EL0 in softmmu doesn't. Is that because we are effectively
> short-circuiting what a real ARM kernel would be doing for EL0?

It's because the "not for EL0" is a rather bogus attempt
at 'security' by not allowing userspace in a system emulator
to access the semihosting API, reserving it instead for
the guest OS (its EL1). This concept doesn't apply for
linux-user mode, where there is no guest EL1, and where in any
case the semihosting API doesn't allow the guest code to do
anything it couldn't do by directly making host OS syscalls.

I suspect this "not for EL0" thing is not something anybody
else's semihosting implementation does (though I haven't checked).

One idea I've vaguely thought about is an idea of a more
'safe' semihosting mode, where we only provide the calls
which we think are reasonable for a not-really-trusted
guest: so you could write to stdout but not read/write
arbitrary files on the filesystem, for instance.

thanks
-- PMM