| Message ID | 20190910144428.32597-3-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: Implement semihosting v2.0 | expand |
Peter Maydell <peter.maydell@linaro.org> writes: > If we fail a semihosting call we should always set the > semihosting errno to something; we were failing to do > this for some of the "check inputs for sanity" cases. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> although: > --- > target/arm/arm-semi.c | 45 ++++++++++++++++++++++++++----------------- > 1 file changed, 27 insertions(+), 18 deletions(-) > > diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c > index 03e60105c05..51b55816faf 100644 > --- a/target/arm/arm-semi.c > +++ b/target/arm/arm-semi.c > @@ -232,11 +232,13 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb, > #define GET_ARG(n) do { \ > if (is_a64(env)) { \ > if (get_user_u64(arg ## n, args + (n) * 8)) { \ > - return -1; \ > + errno = EFAULT; \ > + return set_swi_errno(ts, -1); \ This looks a little queasy given ts is NULL for the softmmu version. I wonder (depending on your approach to -smp for 1/13) if we should just pass the ARMCPU down to the helper? -- Alex Bennée
On Thu, 12 Sep 2019 at 11:42, Alex Bennée <alex.bennee@linaro.org> wrote: > > > Peter Maydell <peter.maydell@linaro.org> writes: > > > If we fail a semihosting call we should always set the > > semihosting errno to something; we were failing to do > > this for some of the "check inputs for sanity" cases. > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > > although: > > > --- > > target/arm/arm-semi.c | 45 ++++++++++++++++++++++++++----------------- > > 1 file changed, 27 insertions(+), 18 deletions(-) > > > > diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c > > index 03e60105c05..51b55816faf 100644 > > --- a/target/arm/arm-semi.c > > +++ b/target/arm/arm-semi.c > > @@ -232,11 +232,13 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb, > > #define GET_ARG(n) do { \ > > if (is_a64(env)) { \ > > if (get_user_u64(arg ## n, args + (n) * 8)) { \ > > - return -1; \ > > + errno = EFAULT; \ > > + return set_swi_errno(ts, -1); \ > > This looks a little queasy given ts is NULL for the softmmu version. I > wonder (depending on your approach to -smp for 1/13) if we should just > pass the ARMCPU down to the helper? NULL is fine because the softmmu version of set_swi_errno() doesn't do anything with it anyway, right? thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Thu, 12 Sep 2019 at 11:42, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> >> Peter Maydell <peter.maydell@linaro.org> writes: >> >> > If we fail a semihosting call we should always set the >> > semihosting errno to something; we were failing to do >> > this for some of the "check inputs for sanity" cases. >> > >> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> >> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> >> >> although: >> >> > --- >> > target/arm/arm-semi.c | 45 ++++++++++++++++++++++++++----------------- >> > 1 file changed, 27 insertions(+), 18 deletions(-) >> > >> > diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c >> > index 03e60105c05..51b55816faf 100644 >> > --- a/target/arm/arm-semi.c >> > +++ b/target/arm/arm-semi.c >> > @@ -232,11 +232,13 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb, >> > #define GET_ARG(n) do { \ >> > if (is_a64(env)) { \ >> > if (get_user_u64(arg ## n, args + (n) * 8)) { \ >> > - return -1; \ >> > + errno = EFAULT; \ >> > + return set_swi_errno(ts, -1); \ >> >> This looks a little queasy given ts is NULL for the softmmu version. I >> wonder (depending on your approach to -smp for 1/13) if we should just >> pass the ARMCPU down to the helper? > > NULL is fine because the softmmu version of set_swi_errno() doesn't > do anything with it anyway, right? Yes it's fine - it just looks a little odd when you are reading it. Given TaskState is derived from CPUState which you always have you could just pass cs to set_swi_errno and hide the final implementation detail there depending on if you are softmmu or linux-user. But it's purely a subjective style thing, not a bug hence the r-b ;-) -- Alex Bennée
diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c index 03e60105c05..51b55816faf 100644 --- a/target/arm/arm-semi.c +++ b/target/arm/arm-semi.c @@ -232,11 +232,13 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb, #define GET_ARG(n) do { \ if (is_a64(env)) { \ if (get_user_u64(arg ## n, args + (n) * 8)) { \ - return -1; \ + errno = EFAULT; \ + return set_swi_errno(ts, -1); \ } \ } else { \ if (get_user_u32(arg ## n, args + (n) * 4)) { \ - return -1; \ + errno = EFAULT; \ + return set_swi_errno(ts, -1); \ } \ } \ } while (0) @@ -287,12 +289,13 @@ 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; + errno = EFAULT; + return set_swi_errno(ts, -1); } if (arg1 >= 12) { unlock_user(s, arg0, 0); - return (uint32_t)-1; + errno = EINVAL; + return set_swi_errno(ts, -1); } if (strcmp(s, ":tt") == 0) { int result_fileno = arg1 < 4 ? STDIN_FILENO : STDOUT_FILENO; @@ -413,8 +416,8 @@ target_ulong do_arm_semihosting(CPUARMState *env) } else { s = lock_user_string(arg0); if (!s) { - /* FIXME - should this error code be -TARGET_EFAULT ? */ - return (uint32_t)-1; + errno = EFAULT; + return set_swi_errno(ts, -1); } ret = set_swi_errno(ts, remove(s)); unlock_user(s, arg0, 0); @@ -432,11 +435,12 @@ target_ulong do_arm_semihosting(CPUARMState *env) 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) { + errno = EFAULT; + ret = set_swi_errno(ts, -1); + } else { ret = set_swi_errno(ts, rename(s, s2)); + } if (s2) unlock_user(s2, arg2, 0); if (s) @@ -456,8 +460,8 @@ target_ulong do_arm_semihosting(CPUARMState *env) } else { s = lock_user_string(arg0); if (!s) { - /* FIXME - should this error code be -TARGET_EFAULT ? */ - return (uint32_t)-1; + errno = EFAULT; + return set_swi_errno(ts, -1); } ret = set_swi_errno(ts, system(s)); unlock_user(s, arg0, 0); @@ -517,19 +521,22 @@ target_ulong do_arm_semihosting(CPUARMState *env) if (output_size > input_size) { /* Not enough space to store command-line arguments. */ - return -1; + errno = E2BIG; + return set_swi_errno(ts, -1); } /* Adjust the command-line length. */ if (SET_ARG(1, output_size - 1)) { /* Couldn't write back to argument block */ - return -1; + errno = EFAULT; + return set_swi_errno(ts, -1); } /* Lock the buffer on the ARM side. */ output_buffer = lock_user(VERIFY_WRITE, arg0, output_size, 0); if (!output_buffer) { - return -1; + errno = EFAULT; + return set_swi_errno(ts, -1); } /* Copy the command-line arguments. */ @@ -544,7 +551,8 @@ target_ulong do_arm_semihosting(CPUARMState *env) if (copy_from_user(output_buffer, ts->info->arg_start, output_size)) { - status = -1; + errno = EFAULT; + status = set_swi_errno(ts, -1); goto out; } @@ -614,7 +622,8 @@ target_ulong do_arm_semihosting(CPUARMState *env) if (fail) { /* Couldn't write back to argument block */ - return -1; + errno = EFAULT; + return set_swi_errno(ts, -1); } } return 0;
If we fail a semihosting call we should always set the semihosting errno to something; we were failing to do this for some of the "check inputs for sanity" cases. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target/arm/arm-semi.c | 45 ++++++++++++++++++++++++++----------------- 1 file changed, 27 insertions(+), 18 deletions(-) -- 2.20.1