| Message ID | 20190731160719.11396-5-alex.bennee@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | plugins for TCG | expand |
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
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
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 --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; }
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