| Message ID | 20190703155244.28166-5-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | arm semihosting cleanups | expand |
Hi Alex, Peter. On 7/3/19 5:52 PM, 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> > --- > target/arm/helper.c | 84 +++++++++------------------------------------ > 1 file changed, 17 insertions(+), 67 deletions(-) > > diff --git a/target/arm/helper.c b/target/arm/helper.c > index ad29dc4072..5c1f741380 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -10364,83 +10364,33 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs) > new_el, env->pc, pstate_read(env)); > } > > -static inline bool check_for_semihosting(CPUState *cs) > +/* > + * Check whether this exception is a semihosting call; if so > + * then handle it and return true; otherwise return false. > + * > + * All the permission and validity checks are done at translate time. > + */ > +static inline bool handle_semihosting(CPUState *cs) > { > - /* 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. > - */ > + if (cs->exception_index == EXCP_SEMIHOST) { > + if (is_a64(env)) { > 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; > - } 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; > + } else { > + qemu_log_mask(CPU_LOG_INT, > + "...handling as semihosting call 0x%x\n", > + env->regs[0]); > + env->regs[0] = do_arm_semihosting(env); > } > - > - 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; > } > + > + return false; > } > > /* Handle a CPU exception for A and R profile CPUs. > @@ -10476,7 +10426,7 @@ void arm_cpu_do_interrupt(CPUState *cs) > * code that caused the exception, not the target exception level, > * so must be handled here. > */ > - if (check_for_semihosting(cs)) { > + if (handle_semihosting(cs)) { > return; > } > > This patch clashes with this one: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00343.html that Peter already queued: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00692.html Peter, if you want this for 4.1, it might be easier to dequeue "Restrict semi-hosting to TCG", apply Alex series, and re-apply "Restrict semi-hosting to TCG", the conflicts should be trivial (function name changed check_for_semihosting -> handle_semihosting). Thanks, Phil.
On 7/3/19 5:52 PM, 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> > --- > target/arm/helper.c | 84 +++++++++------------------------------------ > 1 file changed, 17 insertions(+), 67 deletions(-) > > diff --git a/target/arm/helper.c b/target/arm/helper.c > index ad29dc4072..5c1f741380 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -10364,83 +10364,33 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs) > new_el, env->pc, pstate_read(env)); > } > > -static inline bool check_for_semihosting(CPUState *cs) > +/* > + * Check whether this exception is a semihosting call; if so > + * then handle it and return true; otherwise return false. > + * > + * All the permission and validity checks are done at translate time. > + */ > +static inline bool handle_semihosting(CPUState *cs) Drop the inline, probably. Since you are renaming away the "check", perhaps hoist > + if (cs->exception_index == EXCP_SEMIHOST) { this check to the caller, change the return to void, > @@ -10476,7 +10426,7 @@ void arm_cpu_do_interrupt(CPUState *cs) > * code that caused the exception, not the target exception level, > * so must be handled here. > */ > - if (check_for_semihosting(cs)) { > + if (handle_semihosting(cs)) { > return; > } so we get if (cs->exception_index == EXCP_SEMIHOST) { handle_semihosting(cs); return; } r~
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > Hi Alex, Peter. > > On 7/3/19 5:52 PM, 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> >> --- >> target/arm/helper.c | 84 +++++++++------------------------------------ >> 1 file changed, 17 insertions(+), 67 deletions(-) >> >> diff --git a/target/arm/helper.c b/target/arm/helper.c >> index ad29dc4072..5c1f741380 100644 >> --- a/target/arm/helper.c >> +++ b/target/arm/helper.c >> @@ -10364,83 +10364,33 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs) >> new_el, env->pc, pstate_read(env)); >> } >> >> -static inline bool check_for_semihosting(CPUState *cs) >> +/* >> + * Check whether this exception is a semihosting call; if so >> + * then handle it and return true; otherwise return false. >> + * >> + * All the permission and validity checks are done at translate time. >> + */ >> +static inline bool handle_semihosting(CPUState *cs) >> { >> - /* 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. >> - */ >> + if (cs->exception_index == EXCP_SEMIHOST) { >> + if (is_a64(env)) { >> 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; >> - } 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; >> + } else { >> + qemu_log_mask(CPU_LOG_INT, >> + "...handling as semihosting call 0x%x\n", >> + env->regs[0]); >> + env->regs[0] = do_arm_semihosting(env); >> } >> - >> - 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; >> } >> + >> + return false; >> } >> >> /* Handle a CPU exception for A and R profile CPUs. >> @@ -10476,7 +10426,7 @@ void arm_cpu_do_interrupt(CPUState *cs) >> * code that caused the exception, not the target exception level, >> * so must be handled here. >> */ >> - if (check_for_semihosting(cs)) { >> + if (handle_semihosting(cs)) { >> return; >> } >> >> > > This patch clashes with this one: > https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00343.html > that Peter already queued: > https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00692.html > > Peter, if you want this for 4.1, it might be easier to dequeue "Restrict > semi-hosting to TCG", apply Alex series, and re-apply "Restrict > semi-hosting to TCG", the conflicts should be trivial (function name > changed check_for_semihosting -> handle_semihosting). This isn't 4.1 material - unless I can figure out the weird single-step while semihosting bug. > > Thanks, > > Phil. -- Alex Bennée
On 7/3/19 6:44 PM, Alex Bennée wrote: > Philippe Mathieu-Daudé <philmd@redhat.com> writes: > >> Hi Alex, Peter. >> >> On 7/3/19 5:52 PM, 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> >>> --- >>> target/arm/helper.c | 84 +++++++++------------------------------------ >>> 1 file changed, 17 insertions(+), 67 deletions(-) >>> >>> diff --git a/target/arm/helper.c b/target/arm/helper.c >>> index ad29dc4072..5c1f741380 100644 >>> --- a/target/arm/helper.c >>> +++ b/target/arm/helper.c >>> @@ -10364,83 +10364,33 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs) >>> new_el, env->pc, pstate_read(env)); >>> } >>> >>> -static inline bool check_for_semihosting(CPUState *cs) >>> +/* >>> + * Check whether this exception is a semihosting call; if so >>> + * then handle it and return true; otherwise return false. >>> + * >>> + * All the permission and validity checks are done at translate time. >>> + */ >>> +static inline bool handle_semihosting(CPUState *cs) >>> { >>> - /* 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. >>> - */ >>> + if (cs->exception_index == EXCP_SEMIHOST) { >>> + if (is_a64(env)) { >>> 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; >>> - } 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; >>> + } else { >>> + qemu_log_mask(CPU_LOG_INT, >>> + "...handling as semihosting call 0x%x\n", >>> + env->regs[0]); >>> + env->regs[0] = do_arm_semihosting(env); >>> } >>> - >>> - 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; >>> } >>> + >>> + return false; >>> } >>> >>> /* Handle a CPU exception for A and R profile CPUs. >>> @@ -10476,7 +10426,7 @@ void arm_cpu_do_interrupt(CPUState *cs) >>> * code that caused the exception, not the target exception level, >>> * so must be handled here. >>> */ >>> - if (check_for_semihosting(cs)) { >>> + if (handle_semihosting(cs)) { >>> return; >>> } >>> >>> >> >> This patch clashes with this one: >> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00343.html >> that Peter already queued: >> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00692.html >> >> Peter, if you want this for 4.1, it might be easier to dequeue "Restrict >> semi-hosting to TCG", apply Alex series, and re-apply "Restrict >> semi-hosting to TCG", the conflicts should be trivial (function name >> changed check_for_semihosting -> handle_semihosting). > > This isn't 4.1 material - unless I can figure out the weird single-step > while semihosting bug. Well I'm not sure it is ARM-specific... I remember something similar with MIPS. >> >> Thanks, >> >> Phil.
diff --git a/target/arm/helper.c b/target/arm/helper.c index ad29dc4072..5c1f741380 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -10364,83 +10364,33 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs) new_el, env->pc, pstate_read(env)); } -static inline bool check_for_semihosting(CPUState *cs) +/* + * Check whether this exception is a semihosting call; if so + * then handle it and return true; otherwise return false. + * + * All the permission and validity checks are done at translate time. + */ +static inline bool handle_semihosting(CPUState *cs) { - /* 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. - */ + if (cs->exception_index == EXCP_SEMIHOST) { + if (is_a64(env)) { 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; - } 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; + } else { + qemu_log_mask(CPU_LOG_INT, + "...handling as semihosting call 0x%x\n", + env->regs[0]); + env->regs[0] = do_arm_semihosting(env); } - - 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; } + + return false; } /* Handle a CPU exception for A and R profile CPUs. @@ -10476,7 +10426,7 @@ void arm_cpu_do_interrupt(CPUState *cs) * code that caused the exception, not the target exception level, * so must be handled here. */ - if (check_for_semihosting(cs)) { + if (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> --- target/arm/helper.c | 84 +++++++++------------------------------------ 1 file changed, 17 insertions(+), 67 deletions(-) -- 2.20.1