diff mbox series

[02/13] target/arm/arm-semi: Always set some kind of errno for failed calls

Message ID 20190910144428.32597-3-peter.maydell@linaro.org (mailing list archive)
State New, archived
Headers show
Series target/arm: Implement semihosting v2.0 | expand

Commit Message

Peter Maydell Sept. 10, 2019, 2:44 p.m. UTC
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(-)

Comments

Alex Bennée Sept. 12, 2019, 10:42 a.m. UTC | #1
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
Peter Maydell Sept. 12, 2019, 10:50 a.m. UTC | #2
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
Alex Bennée Sept. 12, 2019, 11:09 a.m. UTC | #3
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 mbox series

Patch

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;