| Message ID | 20190510191051.5973-1-alex.bennee@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] target/arm: semihosting docs, formatting and return clean-ups | expand |
On Fri, 10 May 2019 at 20:10, Alex Bennée <alex.bennee@linaro.org> wrote: > > This is a clean-up of the semihosting calls after reading ver 2.0 of > the specification. There are a number of small fixes that seemed too > insignificant to split into smaller patches: > > - add reference to the ARM semihosting spec > - add some additional commentary on return values > - fixup block comments as per standard > - audit return values, return 0xdeadbeef for corrupted values > - fix up leaks from early returns with lock_user_string > - return bytes not written/read instead of -1 > - add LOG_UNIMP for missing functionality > > This is very much a Friday patch. It might be worth splitting up if > coming back for a more concerted clean-up series for semihosting as > the asynchronous gdb calls probably need more attention. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > target/arm/arm-semi.c | 180 +++++++++++++++++++++++++----------------- > 1 file changed, 109 insertions(+), 71 deletions(-) > > diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c > index 4c326fdc2fb..8deaed2807c 100644 > --- a/target/arm/arm-semi.c > +++ b/target/arm/arm-semi.c > @@ -2,6 +2,7 @@ > * Arm "Angel" semihosting syscalls > * > * Copyright (c) 2005, 2007 CodeSourcery. > + * Copyright (c) 2019 Linaro > * Written by Paul Brook. > * > * This program is free software; you can redistribute it and/or modify > @@ -15,13 +16,19 @@ > * GNU General Public License for more details. > * > * You should have received a copy of the GNU General Public License > - * along with this program; if not, see <http://www.gnu.org/licenses/>. > + * along with this program; if not, see > + * <http://www.gnu.org/licenses/>. > + * > + * ARM Semihosting is documented in: > + * Semihosting for AArch32 and AArch64 Release 2.0 > + * https://static.docs.arm.com/100863/0200/semihosting.pdf > */ > > #include "qemu/osdep.h" > > #include "cpu.h" > #include "exec/semihost.h" > +#include "exec/log.h" > #ifdef CONFIG_USER_ONLY > #include "qemu.h" > > @@ -241,13 +248,18 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb, > put_user_u64(val, args + (n) * 8) : \ > put_user_u32(val, args + (n) * 4)) > > +/* > + * Do a semihosting call. Returns the "RETURN REGISTER" which is > + * documented as corrupted for some calls. In this case we use the > + * venerable 0xdeadbeef. > + */ I think what you mean here is something like "the specification always says that the "return register" either returns a specific value or is corrupted, so we don't need to report to our caller whether we are returning a value or trying to leave the register unchanged. We use 0xdeadbeef as the return value when there isn't a defined return value for the call." > target_ulong do_arm_semihosting(CPUARMState *env) > { > ARMCPU *cpu = arm_env_get_cpu(env); > CPUState *cs = CPU(cpu); > target_ulong args; > target_ulong arg0, arg1, arg2, arg3; > - char * s; > + char *s; > int nr; > uint32_t ret; > uint32_t len; > @@ -273,9 +285,9 @@ target_ulong do_arm_semihosting(CPUARMState *env) > GET_ARG(2); > s = lock_user_string(arg0); > if (!s) { > - /* FIXME - should this error code be -TARGET_EFAULT ? */ We definitely don't want to return -TARGET_EFAULT, but we could in theory call set_swi_errno(ts, EFAULT). The spec says that it's up to the implementation, effectively, so I think I agree we don't need to bother. Any caller which has messed things up sufficiently to be passing us bogus memory is unlikely to be in a state to successfully call SYS_ERRNO anyway. > return (uint32_t)-1; > } > + /* check for invalid open mode */ > if (arg1 >= 12) { > unlock_user(s, arg0, 0); > return (uint32_t)-1; > @@ -287,7 +299,7 @@ target_ulong do_arm_semihosting(CPUARMState *env) > } > if (use_gdb_syscalls()) { > ret = arm_gdb_syscall(cpu, arm_semi_cb, "open,%s,%x,1a4", arg0, > - (int)arg2+1, gdb_open_modeflags[arg1]); > + (int) arg2 + 1, gdb_open_modeflags[arg1]); The space after the cast looks a bit odd here. > } else { > ret = set_swi_errno(ts, open(s, open_modeflags[arg1], 0644)); > } > @@ -301,48 +313,51 @@ target_ulong do_arm_semihosting(CPUARMState *env) > return set_swi_errno(ts, close(arg0)); > } > case TARGET_SYS_WRITEC: > - { > - char c; > - > - if (get_user_u8(c, args)) > - /* FIXME - should this error code be -TARGET_EFAULT ? */ > - return (uint32_t)-1; > - /* Write to debug console. stderr is near enough. */ > - if (use_gdb_syscalls()) { > + { > + char c; > + if (!get_user_u8(c, args)) { > + /* Write to debug console. stderr is near enough. */ > + if (use_gdb_syscalls()) { > return arm_gdb_syscall(cpu, arm_semi_cb, "write,2,%x,1", args); > - } else { > + } else { > #ifdef CONFIG_SOFTMMU > - Chardev *chardev = semihosting_get_chardev(); > - if (chardev) { > - return qemu_chr_write_all(chardev, (uint8_t *) &c, 1); > - } else > + Chardev *chardev = semihosting_get_chardev(); > + if (chardev) { > + return qemu_chr_write_all(chardev, (uint8_t *) &c, 1); > + } > #endif Looks like this patch is dependent on some other patchset? > @@ -447,20 +474,23 @@ target_ulong do_arm_semihosting(CPUARMState *env) > GET_ARG(3); > if (use_gdb_syscalls()) { > return arm_gdb_syscall(cpu, arm_semi_cb, "rename,%s,%s", > - arg0, (int)arg1+1, arg2, (int)arg3+1); > + arg0, (int)arg1 + 1, arg2, (int)arg3 + 1); > } else { > char *s2; > s = lock_user_string(arg0); > s2 = lock_user_string(arg2); > - if (!s || !s2) > - /* FIXME - should this error code be -TARGET_EFAULT ? */ > - ret = (uint32_t)-1; > - else > + if (s && s2) { > ret = set_swi_errno(ts, rename(s, s2)); > - if (s2) > + } else { > + ret = -EIO; I would stick with -1 personally. The spec says "host-specific error code", but since there's not a lot the guest can do with the result anyway we can just use -1 in our implementation. Also, EIO is an odd choice for "you passed us non-readable memory", which is more usually EFAULT. More generally, can you keep changes in behaviour in a separate patch from code reformatting, please ? > @@ -472,7 +502,7 @@ target_ulong do_arm_semihosting(CPUARMState *env) > GET_ARG(1); > if (use_gdb_syscalls()) { > return arm_gdb_syscall(cpu, arm_semi_cb, "system,%s", > - arg0, (int)arg1+1); > + arg0, (int)arg1 + 1); ...here you don't have the space after the cast. I prefer this way. thanks -- PMM
Hi Alex, On 5/10/19 9:10 PM, Alex Bennée wrote: > This is a clean-up of the semihosting calls after reading ver 2.0 of > the specification. There are a number of small fixes that seemed too > insignificant to split into smaller patches: > Can you split at least this one of: > - fixup block comments as per standard The rest is probably acceptable as an unique patch: > - add reference to the ARM semihosting spec > - add some additional commentary on return values > - audit return values, return 0xdeadbeef for corrupted values > - fix up leaks from early returns with lock_user_string > - return bytes not written/read instead of -1 > - add LOG_UNIMP for missing functionality Thanks! Phil. > > This is very much a Friday patch. It might be worth splitting up if > coming back for a more concerted clean-up series for semihosting as > the asynchronous gdb calls probably need more attention. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > target/arm/arm-semi.c | 180 +++++++++++++++++++++++++----------------- > 1 file changed, 109 insertions(+), 71 deletions(-) > > diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c > index 4c326fdc2fb..8deaed2807c 100644 > --- a/target/arm/arm-semi.c > +++ b/target/arm/arm-semi.c > @@ -2,6 +2,7 @@ > * Arm "Angel" semihosting syscalls > * > * Copyright (c) 2005, 2007 CodeSourcery. > + * Copyright (c) 2019 Linaro > * Written by Paul Brook. > * > * This program is free software; you can redistribute it and/or modify > @@ -15,13 +16,19 @@ > * GNU General Public License for more details. > * > * You should have received a copy of the GNU General Public License > - * along with this program; if not, see <http://www.gnu.org/licenses/>. > + * along with this program; if not, see > + * <http://www.gnu.org/licenses/>. > + * > + * ARM Semihosting is documented in: > + * Semihosting for AArch32 and AArch64 Release 2.0 > + * https://static.docs.arm.com/100863/0200/semihosting.pdf > */ > > #include "qemu/osdep.h" > > #include "cpu.h" > #include "exec/semihost.h" > +#include "exec/log.h" > #ifdef CONFIG_USER_ONLY > #include "qemu.h" > > @@ -241,13 +248,18 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb, > put_user_u64(val, args + (n) * 8) : \ > put_user_u32(val, args + (n) * 4)) > > +/* > + * Do a semihosting call. Returns the "RETURN REGISTER" which is > + * documented as corrupted for some calls. In this case we use the > + * venerable 0xdeadbeef. > + */ > target_ulong do_arm_semihosting(CPUARMState *env) > { > ARMCPU *cpu = arm_env_get_cpu(env); > CPUState *cs = CPU(cpu); > target_ulong args; > target_ulong arg0, arg1, arg2, arg3; > - char * s; > + char *s; > int nr; > uint32_t ret; > uint32_t len; > @@ -273,9 +285,9 @@ target_ulong do_arm_semihosting(CPUARMState *env) > GET_ARG(2); > s = lock_user_string(arg0); > if (!s) { > - /* FIXME - should this error code be -TARGET_EFAULT ? */ > return (uint32_t)-1; > } > + /* check for invalid open mode */ > if (arg1 >= 12) { > unlock_user(s, arg0, 0); > return (uint32_t)-1; > @@ -287,7 +299,7 @@ target_ulong do_arm_semihosting(CPUARMState *env) > } > if (use_gdb_syscalls()) { > ret = arm_gdb_syscall(cpu, arm_semi_cb, "open,%s,%x,1a4", arg0, > - (int)arg2+1, gdb_open_modeflags[arg1]); > + (int) arg2 + 1, gdb_open_modeflags[arg1]); > } else { > ret = set_swi_errno(ts, open(s, open_modeflags[arg1], 0644)); > } > @@ -301,48 +313,51 @@ target_ulong do_arm_semihosting(CPUARMState *env) > return set_swi_errno(ts, close(arg0)); > } > case TARGET_SYS_WRITEC: > - { > - char c; > - > - if (get_user_u8(c, args)) > - /* FIXME - should this error code be -TARGET_EFAULT ? */ > - return (uint32_t)-1; > - /* Write to debug console. stderr is near enough. */ > - if (use_gdb_syscalls()) { > + { > + char c; > + if (!get_user_u8(c, args)) { > + /* Write to debug console. stderr is near enough. */ > + if (use_gdb_syscalls()) { > return arm_gdb_syscall(cpu, arm_semi_cb, "write,2,%x,1", args); > - } else { > + } else { > #ifdef CONFIG_SOFTMMU > - Chardev *chardev = semihosting_get_chardev(); > - if (chardev) { > - return qemu_chr_write_all(chardev, (uint8_t *) &c, 1); > - } else > + Chardev *chardev = semihosting_get_chardev(); > + if (chardev) { > + return qemu_chr_write_all(chardev, (uint8_t *) &c, 1); > + } > #endif > - { > - return write(STDERR_FILENO, &c, 1); > - } > - } > + return write(STDERR_FILENO, &c, 1); > + } > + } else { > + /* return register is corrupted */ > + return 0xdeadbeef; > } > + } > case TARGET_SYS_WRITE0: > - if (!(s = lock_user_string(args))) > - /* FIXME - should this error code be -TARGET_EFAULT ? */ > - return (uint32_t)-1; > - len = strlen(s); > - if (use_gdb_syscalls()) { > - return arm_gdb_syscall(cpu, arm_semi_cb, "write,2,%x,%x", > - args, len); > - } else { > + { > + s = lock_user_string(args); > + if (s) { > + len = strlen(s); > + if (use_gdb_syscalls()) { > + arm_gdb_syscall(cpu, arm_semi_cb, "write,2,%x,%x", > + args, len); > + } else { > #ifdef CONFIG_SOFTMMU > - Chardev *chardev = semihosting_get_chardev(); > - if (chardev) { > - ret = qemu_chr_write_all(chardev, (uint8_t *) s, len); > - } else > + Chardev *chardev = semihosting_get_chardev(); > + if (chardev) { > + qemu_chr_write_all(chardev, (uint8_t *) s, len); > + } else > #endif > - { > - ret = write(STDERR_FILENO, s, len); > + { > + /* result ignored */ > + ret = write(STDERR_FILENO, s, len); > + } > } > } > unlock_user(s, args, 0); > - return ret; > + /* return register is corrupted */ > + return 0xdeadbeef; > + } > case TARGET_SYS_WRITE: > GET_ARG(0); > GET_ARG(1); > @@ -355,13 +370,15 @@ target_ulong do_arm_semihosting(CPUARMState *env) > } else { > s = lock_user(VERIFY_READ, arg1, len, 1); > if (!s) { > - /* FIXME - should this error code be -TARGET_EFAULT ? */ > - return (uint32_t)-1; > + /* Return bytes not written on error */ > + return len; > } > ret = set_swi_errno(ts, write(arg0, s, len)); > unlock_user(s, arg1, 0); > - if (ret == (uint32_t)-1) > - return -1; > + if (ret == (uint32_t)-1) { > + ret = 0; > + } > + /* Return bytes not written */ > return len - ret; > } > case TARGET_SYS_READ: > @@ -376,26 +393,33 @@ target_ulong do_arm_semihosting(CPUARMState *env) > } else { > s = lock_user(VERIFY_WRITE, arg1, len, 0); > if (!s) { > - /* FIXME - should this error code be -TARGET_EFAULT ? */ > - return (uint32_t)-1; > + /* return bytes not read */ > + return len; > } > do { > ret = set_swi_errno(ts, read(arg0, s, len)); > } while (ret == -1 && errno == EINTR); > unlock_user(s, arg1, len); > - if (ret == (uint32_t)-1) > - return -1; > + if (ret == (uint32_t)-1) { > + ret = 0; > + } > + /* Return bytes not read */ > return len - ret; > } > case TARGET_SYS_READC: > - /* XXX: Read from debug console. Not implemented. */ > + qemu_log_mask(LOG_UNIMP, "%s: TARGET_SYS_READC not implemented", > + __func__); > return 0; > case TARGET_SYS_ISTTY: > GET_ARG(0); > if (use_gdb_syscalls()) { > return arm_gdb_syscall(cpu, arm_semi_cb, "isatty,%x", arg0); > } else { > - return isatty(arg0); > + if (isatty(arg0)) { > + return 1; > + } else { > + return (errno == EBADF ? -1 : 0); > + } > } > case TARGET_SYS_SEEK: > GET_ARG(0); > @@ -405,8 +429,9 @@ target_ulong do_arm_semihosting(CPUARMState *env) > arg0, arg1); > } else { > ret = set_swi_errno(ts, lseek(arg0, arg1, SEEK_SET)); > - if (ret == (uint32_t)-1) > - return -1; > + if (ret == (uint32_t)-1) { > + return -1; > + } > return 0; > } > case TARGET_SYS_FLEN: > @@ -417,28 +442,30 @@ target_ulong do_arm_semihosting(CPUARMState *env) > } else { > struct stat buf; > ret = set_swi_errno(ts, fstat(arg0, &buf)); > - if (ret == (uint32_t)-1) > + if (ret == (uint32_t)-1) { > return -1; > + } > return buf.st_size; > } > case TARGET_SYS_TMPNAM: > - /* XXX: Not implemented. */ > + qemu_log_mask(LOG_UNIMP, "%s: TARGET_SYS_TMPNAM not implemented", > + __func__); > return -1; > case TARGET_SYS_REMOVE: > GET_ARG(0); > GET_ARG(1); > if (use_gdb_syscalls()) { > ret = arm_gdb_syscall(cpu, arm_semi_cb, "unlink,%s", > - arg0, (int)arg1+1); > + arg0, (int)arg1 + 1); > } else { > s = lock_user_string(arg0); > if (!s) { > - /* FIXME - should this error code be -TARGET_EFAULT ? */ > - return (uint32_t)-1; > + return -EIO; > } > - ret = set_swi_errno(ts, remove(s)); > + ret = set_swi_errno(ts, remove(s)); > unlock_user(s, arg0, 0); > } > + /* 0 on success or host-specific error code */ > return ret; > case TARGET_SYS_RENAME: > GET_ARG(0); > @@ -447,20 +474,23 @@ target_ulong do_arm_semihosting(CPUARMState *env) > GET_ARG(3); > if (use_gdb_syscalls()) { > return arm_gdb_syscall(cpu, arm_semi_cb, "rename,%s,%s", > - arg0, (int)arg1+1, arg2, (int)arg3+1); > + arg0, (int)arg1 + 1, arg2, (int)arg3 + 1); > } else { > char *s2; > s = lock_user_string(arg0); > s2 = lock_user_string(arg2); > - if (!s || !s2) > - /* FIXME - should this error code be -TARGET_EFAULT ? */ > - ret = (uint32_t)-1; > - else > + if (s && s2) { > ret = set_swi_errno(ts, rename(s, s2)); > - if (s2) > + } else { > + ret = -EIO; > + } > + if (s2) { > unlock_user(s2, arg2, 0); > - if (s) > + } > + if (s) { > unlock_user(s, arg0, 0); > + } > + /* 0 on success or host-specific error code */ > return ret; > } > case TARGET_SYS_CLOCK: > @@ -472,7 +502,7 @@ target_ulong do_arm_semihosting(CPUARMState *env) > GET_ARG(1); > if (use_gdb_syscalls()) { > return arm_gdb_syscall(cpu, arm_semi_cb, "system,%s", > - arg0, (int)arg1+1); > + arg0, (int)arg1 + 1); > } else { > s = lock_user_string(arg0); > if (!s) { > @@ -527,14 +557,16 @@ target_ulong do_arm_semihosting(CPUARMState *env) > > output_size = ts->info->arg_end - ts->info->arg_start; > if (!output_size) { > - /* We special-case the "empty command line" case (argc==0). > - Just provide the terminating 0. */ > + /* > + * We special-case the "empty command line" case (argc==0). > + * Just provide the terminating 0. > + */ > output_size = 1; > } > #endif > > if (output_size > input_size) { > - /* Not enough space to store command-line arguments. */ > + /* Not enough space to store command-line arguments. */ > return -1; > } > > @@ -588,8 +620,10 @@ target_ulong do_arm_semihosting(CPUARMState *env) > GET_ARG(0); > > #ifdef CONFIG_USER_ONLY > - /* Some C libraries assume the heap immediately follows .bss, so > - allocate it using sbrk. */ > + /* > + * Some C libraries assume the heap immediately follows .bss, so > + * allocate it using sbrk. > + */ > if (!ts->heap_limit) { > abi_ulong ret; > > @@ -637,7 +671,8 @@ target_ulong do_arm_semihosting(CPUARMState *env) > } > case TARGET_SYS_EXIT: > if (is_a64(env)) { > - /* The A64 version of this call takes a parameter block, > + /* > + * The A64 version of this call takes a parameter block, > * so the application-exit type can return a subcode which > * is the exit status code from the application. > */ > @@ -650,14 +685,17 @@ target_ulong do_arm_semihosting(CPUARMState *env) > ret = 1; > } > } else { > - /* ARM specifies only Stopped_ApplicationExit as normal > - * exit, everything else is considered an error */ > + /* > + * ARM specifies only Stopped_ApplicationExit as normal > + * exit, everything else is considered an error > + */ > ret = (args == ADP_Stopped_ApplicationExit) ? 0 : 1; > } > gdb_exit(env, ret); > exit(ret); > case TARGET_SYS_SYNCCACHE: > - /* Clean the D-cache and invalidate the I-cache for the specified > + /* > + * Clean the D-cache and invalidate the I-cache for the specified > * virtual address range. This is a nop for us since we don't > * implement caches. This is only present on A64. > */ >
diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c index 4c326fdc2fb..8deaed2807c 100644 --- a/target/arm/arm-semi.c +++ b/target/arm/arm-semi.c @@ -2,6 +2,7 @@ * Arm "Angel" semihosting syscalls * * Copyright (c) 2005, 2007 CodeSourcery. + * Copyright (c) 2019 Linaro * Written by Paul Brook. * * This program is free software; you can redistribute it and/or modify @@ -15,13 +16,19 @@ * GNU General Public License for more details. * * You should have received a copy of the GNU General Public License - * along with this program; if not, see <http://www.gnu.org/licenses/>. + * along with this program; if not, see + * <http://www.gnu.org/licenses/>. + * + * ARM Semihosting is documented in: + * Semihosting for AArch32 and AArch64 Release 2.0 + * https://static.docs.arm.com/100863/0200/semihosting.pdf */ #include "qemu/osdep.h" #include "cpu.h" #include "exec/semihost.h" +#include "exec/log.h" #ifdef CONFIG_USER_ONLY #include "qemu.h" @@ -241,13 +248,18 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb, put_user_u64(val, args + (n) * 8) : \ put_user_u32(val, args + (n) * 4)) +/* + * Do a semihosting call. Returns the "RETURN REGISTER" which is + * documented as corrupted for some calls. In this case we use the + * venerable 0xdeadbeef. + */ target_ulong do_arm_semihosting(CPUARMState *env) { ARMCPU *cpu = arm_env_get_cpu(env); CPUState *cs = CPU(cpu); target_ulong args; target_ulong arg0, arg1, arg2, arg3; - char * s; + char *s; int nr; uint32_t ret; uint32_t len; @@ -273,9 +285,9 @@ target_ulong do_arm_semihosting(CPUARMState *env) GET_ARG(2); s = lock_user_string(arg0); if (!s) { - /* FIXME - should this error code be -TARGET_EFAULT ? */ return (uint32_t)-1; } + /* check for invalid open mode */ if (arg1 >= 12) { unlock_user(s, arg0, 0); return (uint32_t)-1; @@ -287,7 +299,7 @@ target_ulong do_arm_semihosting(CPUARMState *env) } if (use_gdb_syscalls()) { ret = arm_gdb_syscall(cpu, arm_semi_cb, "open,%s,%x,1a4", arg0, - (int)arg2+1, gdb_open_modeflags[arg1]); + (int) arg2 + 1, gdb_open_modeflags[arg1]); } else { ret = set_swi_errno(ts, open(s, open_modeflags[arg1], 0644)); } @@ -301,48 +313,51 @@ target_ulong do_arm_semihosting(CPUARMState *env) return set_swi_errno(ts, close(arg0)); } case TARGET_SYS_WRITEC: - { - char c; - - if (get_user_u8(c, args)) - /* FIXME - should this error code be -TARGET_EFAULT ? */ - return (uint32_t)-1; - /* Write to debug console. stderr is near enough. */ - if (use_gdb_syscalls()) { + { + char c; + if (!get_user_u8(c, args)) { + /* Write to debug console. stderr is near enough. */ + if (use_gdb_syscalls()) { return arm_gdb_syscall(cpu, arm_semi_cb, "write,2,%x,1", args); - } else { + } else { #ifdef CONFIG_SOFTMMU - Chardev *chardev = semihosting_get_chardev(); - if (chardev) { - return qemu_chr_write_all(chardev, (uint8_t *) &c, 1); - } else + Chardev *chardev = semihosting_get_chardev(); + if (chardev) { + return qemu_chr_write_all(chardev, (uint8_t *) &c, 1); + } #endif - { - return write(STDERR_FILENO, &c, 1); - } - } + return write(STDERR_FILENO, &c, 1); + } + } else { + /* return register is corrupted */ + return 0xdeadbeef; } + } case TARGET_SYS_WRITE0: - if (!(s = lock_user_string(args))) - /* FIXME - should this error code be -TARGET_EFAULT ? */ - return (uint32_t)-1; - len = strlen(s); - if (use_gdb_syscalls()) { - return arm_gdb_syscall(cpu, arm_semi_cb, "write,2,%x,%x", - args, len); - } else { + { + s = lock_user_string(args); + if (s) { + len = strlen(s); + if (use_gdb_syscalls()) { + arm_gdb_syscall(cpu, arm_semi_cb, "write,2,%x,%x", + args, len); + } else { #ifdef CONFIG_SOFTMMU - Chardev *chardev = semihosting_get_chardev(); - if (chardev) { - ret = qemu_chr_write_all(chardev, (uint8_t *) s, len); - } else + Chardev *chardev = semihosting_get_chardev(); + if (chardev) { + qemu_chr_write_all(chardev, (uint8_t *) s, len); + } else #endif - { - ret = write(STDERR_FILENO, s, len); + { + /* result ignored */ + ret = write(STDERR_FILENO, s, len); + } } } unlock_user(s, args, 0); - return ret; + /* return register is corrupted */ + return 0xdeadbeef; + } case TARGET_SYS_WRITE: GET_ARG(0); GET_ARG(1); @@ -355,13 +370,15 @@ target_ulong do_arm_semihosting(CPUARMState *env) } else { s = lock_user(VERIFY_READ, arg1, len, 1); if (!s) { - /* FIXME - should this error code be -TARGET_EFAULT ? */ - return (uint32_t)-1; + /* Return bytes not written on error */ + return len; } ret = set_swi_errno(ts, write(arg0, s, len)); unlock_user(s, arg1, 0); - if (ret == (uint32_t)-1) - return -1; + if (ret == (uint32_t)-1) { + ret = 0; + } + /* Return bytes not written */ return len - ret; } case TARGET_SYS_READ: @@ -376,26 +393,33 @@ target_ulong do_arm_semihosting(CPUARMState *env) } else { s = lock_user(VERIFY_WRITE, arg1, len, 0); if (!s) { - /* FIXME - should this error code be -TARGET_EFAULT ? */ - return (uint32_t)-1; + /* return bytes not read */ + return len; } do { ret = set_swi_errno(ts, read(arg0, s, len)); } while (ret == -1 && errno == EINTR); unlock_user(s, arg1, len); - if (ret == (uint32_t)-1) - return -1; + if (ret == (uint32_t)-1) { + ret = 0; + } + /* Return bytes not read */ return len - ret; } case TARGET_SYS_READC: - /* XXX: Read from debug console. Not implemented. */ + qemu_log_mask(LOG_UNIMP, "%s: TARGET_SYS_READC not implemented", + __func__); return 0; case TARGET_SYS_ISTTY: GET_ARG(0); if (use_gdb_syscalls()) { return arm_gdb_syscall(cpu, arm_semi_cb, "isatty,%x", arg0); } else { - return isatty(arg0); + if (isatty(arg0)) { + return 1; + } else { + return (errno == EBADF ? -1 : 0); + } } case TARGET_SYS_SEEK: GET_ARG(0); @@ -405,8 +429,9 @@ target_ulong do_arm_semihosting(CPUARMState *env) arg0, arg1); } else { ret = set_swi_errno(ts, lseek(arg0, arg1, SEEK_SET)); - if (ret == (uint32_t)-1) - return -1; + if (ret == (uint32_t)-1) { + return -1; + } return 0; } case TARGET_SYS_FLEN: @@ -417,28 +442,30 @@ target_ulong do_arm_semihosting(CPUARMState *env) } else { struct stat buf; ret = set_swi_errno(ts, fstat(arg0, &buf)); - if (ret == (uint32_t)-1) + if (ret == (uint32_t)-1) { return -1; + } return buf.st_size; } case TARGET_SYS_TMPNAM: - /* XXX: Not implemented. */ + qemu_log_mask(LOG_UNIMP, "%s: TARGET_SYS_TMPNAM not implemented", + __func__); return -1; case TARGET_SYS_REMOVE: GET_ARG(0); GET_ARG(1); if (use_gdb_syscalls()) { ret = arm_gdb_syscall(cpu, arm_semi_cb, "unlink,%s", - arg0, (int)arg1+1); + arg0, (int)arg1 + 1); } else { s = lock_user_string(arg0); if (!s) { - /* FIXME - should this error code be -TARGET_EFAULT ? */ - return (uint32_t)-1; + return -EIO; } - ret = set_swi_errno(ts, remove(s)); + ret = set_swi_errno(ts, remove(s)); unlock_user(s, arg0, 0); } + /* 0 on success or host-specific error code */ return ret; case TARGET_SYS_RENAME: GET_ARG(0); @@ -447,20 +474,23 @@ target_ulong do_arm_semihosting(CPUARMState *env) GET_ARG(3); if (use_gdb_syscalls()) { return arm_gdb_syscall(cpu, arm_semi_cb, "rename,%s,%s", - arg0, (int)arg1+1, arg2, (int)arg3+1); + arg0, (int)arg1 + 1, arg2, (int)arg3 + 1); } else { char *s2; s = lock_user_string(arg0); s2 = lock_user_string(arg2); - if (!s || !s2) - /* FIXME - should this error code be -TARGET_EFAULT ? */ - ret = (uint32_t)-1; - else + if (s && s2) { ret = set_swi_errno(ts, rename(s, s2)); - if (s2) + } else { + ret = -EIO; + } + if (s2) { unlock_user(s2, arg2, 0); - if (s) + } + if (s) { unlock_user(s, arg0, 0); + } + /* 0 on success or host-specific error code */ return ret; } case TARGET_SYS_CLOCK: @@ -472,7 +502,7 @@ target_ulong do_arm_semihosting(CPUARMState *env) GET_ARG(1); if (use_gdb_syscalls()) { return arm_gdb_syscall(cpu, arm_semi_cb, "system,%s", - arg0, (int)arg1+1); + arg0, (int)arg1 + 1); } else { s = lock_user_string(arg0); if (!s) { @@ -527,14 +557,16 @@ target_ulong do_arm_semihosting(CPUARMState *env) output_size = ts->info->arg_end - ts->info->arg_start; if (!output_size) { - /* We special-case the "empty command line" case (argc==0). - Just provide the terminating 0. */ + /* + * We special-case the "empty command line" case (argc==0). + * Just provide the terminating 0. + */ output_size = 1; } #endif if (output_size > input_size) { - /* Not enough space to store command-line arguments. */ + /* Not enough space to store command-line arguments. */ return -1; } @@ -588,8 +620,10 @@ target_ulong do_arm_semihosting(CPUARMState *env) GET_ARG(0); #ifdef CONFIG_USER_ONLY - /* Some C libraries assume the heap immediately follows .bss, so - allocate it using sbrk. */ + /* + * Some C libraries assume the heap immediately follows .bss, so + * allocate it using sbrk. + */ if (!ts->heap_limit) { abi_ulong ret; @@ -637,7 +671,8 @@ target_ulong do_arm_semihosting(CPUARMState *env) } case TARGET_SYS_EXIT: if (is_a64(env)) { - /* The A64 version of this call takes a parameter block, + /* + * The A64 version of this call takes a parameter block, * so the application-exit type can return a subcode which * is the exit status code from the application. */ @@ -650,14 +685,17 @@ target_ulong do_arm_semihosting(CPUARMState *env) ret = 1; } } else { - /* ARM specifies only Stopped_ApplicationExit as normal - * exit, everything else is considered an error */ + /* + * ARM specifies only Stopped_ApplicationExit as normal + * exit, everything else is considered an error + */ ret = (args == ADP_Stopped_ApplicationExit) ? 0 : 1; } gdb_exit(env, ret); exit(ret); case TARGET_SYS_SYNCCACHE: - /* Clean the D-cache and invalidate the I-cache for the specified + /* + * Clean the D-cache and invalidate the I-cache for the specified * virtual address range. This is a nop for us since we don't * implement caches. This is only present on A64. */
This is a clean-up of the semihosting calls after reading ver 2.0 of the specification. There are a number of small fixes that seemed too insignificant to split into smaller patches: - add reference to the ARM semihosting spec - add some additional commentary on return values - fixup block comments as per standard - audit return values, return 0xdeadbeef for corrupted values - fix up leaks from early returns with lock_user_string - return bytes not written/read instead of -1 - add LOG_UNIMP for missing functionality This is very much a Friday patch. It might be worth splitting up if coming back for a more concerted clean-up series for semihosting as the asynchronous gdb calls probably need more attention. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- target/arm/arm-semi.c | 180 +++++++++++++++++++++++++----------------- 1 file changed, 109 insertions(+), 71 deletions(-)