diff mbox series

[v4,04/54] target/arm: remove run time semihosting checks

Message ID 20190731160719.11396-5-alex.bennee@linaro.org
State Superseded
Headers show
Series plugins for TCG | expand

Commit Message

Alex Bennée July 31, 2019, 4:06 p.m. UTC
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>


---
v2
  - fix re-base conflicts
  - hoist EXCP_SEMIHOST check
  - comment cleanups
---
 target/arm/helper.c | 90 +++++++++------------------------------------
 1 file changed, 18 insertions(+), 72 deletions(-)

-- 
2.20.1

Comments

Xingtao Yao (Fujitsu)" via Aug. 1, 2019, 1:27 p.m. UTC | #1
On Jul 31 17:06, Alex Bennée wrote:
> 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>

> 

> ---

> v2

>   - fix re-base conflicts

>   - hoist EXCP_SEMIHOST check

>   - comment cleanups

> ---

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

>  1 file changed, 18 insertions(+), 72 deletions(-)

> 

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

> index b74c23a9bc0..c5b90a83d36 100644

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

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

> @@ -8259,86 +8259,30 @@ 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.

> + */

> +static void handle_semihosting(CPUState *cs)

>  {

>  #ifdef CONFIG_TCG

> -    /* Check whether this exception is a semihosting call; if so

> -     * then handle it and return true; otherwise return false.

> -     */

>      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

>  }

>  

> @@ -8371,11 +8315,13 @@ 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)) {

> +    if (cs->exception_index == EXCP_SEMIHOST) {

> +        handle_semihosting(cs);

>          return;

>      }


Previously, this code would never return here if CONFIG_TCG was not
defined because check_for_semihosting() always returned false in that
case. Is it now true that `cs->exception_index` will never hold a value
of EXCP_SEMIHOST if CONFIG_TCG is not defined (or that it is otherwise
correct to return here in that case where it wasn't previously)?

-Aaron
Peter Maydell Aug. 1, 2019, 1:36 p.m. UTC | #2
On Thu, 1 Aug 2019 at 14:27, Aaron Lindsay OS
<aaron@os.amperecomputing.com> wrote:
>

> On Jul 31 17:06, Alex Bennée wrote:


> > @@ -8371,11 +8315,13 @@ 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)) {

> > +    if (cs->exception_index == EXCP_SEMIHOST) {

> > +        handle_semihosting(cs);

> >          return;

> >      }

>

> Previously, this code would never return here if CONFIG_TCG was not

> defined because check_for_semihosting() always returned false in that

> case. Is it now true that `cs->exception_index` will never hold a value

> of EXCP_SEMIHOST if CONFIG_TCG is not defined (or that it is otherwise

> correct to return here in that case where it wasn't previously)?


It's always true that cs->exception_index can't be EXCP_SEMIHOST
here if we're not using TCG, because the only way you can get into
this function other than by using TCG is the call in
kvm_arm_handle_debug(), which sets exception_index beforehand.
More generally, we only call this function in non-TCG if we're
trying to manually configure the CPU state to simulate a guest
exception entry, and that will always be done by setting up the
exception_index to the kind of exception we're simulating before
the call. EXCP_SEMIHOST isn't a real exception so that should never
happen (you could argue that we should assert, I suppose).

thanks
-- PMM
Richard Henderson Aug. 1, 2019, 2:53 p.m. UTC | #3
On 7/31/19 9:06 AM, Alex Bennée wrote:
> -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.

> + */

> +static void handle_semihosting(CPUState *cs)

>  {

>  #ifdef CONFIG_TCG


Let's move the ifdef outside the function...

> -    if (check_for_semihosting(cs)) {

> +    if (cs->exception_index == EXCP_SEMIHOST) {

> +        handle_semihosting(cs);

>          return;

>      }


... and put another one here.

Peter described how we can't get EXCP_SEMIHOST here from kvm, and suggested an
assert.  Well, the assert is already present just below:

    assert(!excp_is_internal(cs->exception_index));

All we need to do is not return early beforehand.


r~
diff mbox series

Patch

diff --git a/target/arm/helper.c b/target/arm/helper.c
index b74c23a9bc0..c5b90a83d36 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8259,86 +8259,30 @@  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.
+ */
+static void handle_semihosting(CPUState *cs)
 {
 #ifdef CONFIG_TCG
-    /* Check whether this exception is a semihosting call; if so
-     * then handle it and return true; otherwise return false.
-     */
     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
 }
 
@@ -8371,11 +8315,13 @@  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)) {
+    if (cs->exception_index == EXCP_SEMIHOST) {
+        handle_semihosting(cs);
         return;
     }