diff mbox series

[01/13] target/arm/arm-semi: Capture errno in softmmu version of set_swi_errno()

Message ID 20190910144428.32597-2-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
The set_swi_errno() function is called to capture the errno
from a host system call, so that we can return -1 from the
semihosting function and later allow the guest to get a more
specific error code with the SYS_ERRNO function. It comes in
two versions, one for user-only and one for softmmu. We forgot
to capture the errno in the softmmu version; fix the error.

(Semihosting calls directed to gdb are unaffected because
they go through a different code path that captures the
error return from the gdbstub call in arm_semi_cb() or
arm_semi_flen_cb().)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
NB that a later commit will put in some cleanup of TaskState
that will let us reduce the duplication between the two
implementations of this function.
---
 target/arm/arm-semi.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Alex Bennée Sept. 12, 2019, 10:39 a.m. UTC | #1
Peter Maydell <peter.maydell@linaro.org> writes:

> The set_swi_errno() function is called to capture the errno
> from a host system call, so that we can return -1 from the
> semihosting function and later allow the guest to get a more
> specific error code with the SYS_ERRNO function. It comes in
> two versions, one for user-only and one for softmmu. We forgot
> to capture the errno in the softmmu version; fix the error.
>
> (Semihosting calls directed to gdb are unaffected because
> they go through a different code path that captures the
> error return from the gdbstub call in arm_semi_cb() or
> arm_semi_flen_cb().)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> NB that a later commit will put in some cleanup of TaskState
> that will let us reduce the duplication between the two
> implementations of this function.
> ---
>  target/arm/arm-semi.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index 90423a35deb..03e60105c05 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -114,8 +114,13 @@ static inline uint32_t set_swi_errno(TaskState *ts, uint32_t code)
>      return code;
>  }
>  #else
> +static target_ulong syscall_err;
> +

I appreciate that this is just moving things around but this will be
broken for -smp > 1 if two vCPUs make a syscall at the same time. For
linux-user this information is kept in ts->swi_errno which is
per-thread. Either we need a __thread version or find somewhere in
CPUARMState to store it.

>  static inline uint32_t set_swi_errno(CPUARMState *env, uint32_t code)
>  {
> +    if (code == (uint32_t)-1) {
> +        syscall_err = errno;
> +    }
>      return code;
>  }
>
> @@ -124,10 +129,6 @@ static inline uint32_t set_swi_errno(CPUARMState *env, uint32_t code)
>
>  static target_ulong arm_semi_syscall_len;
>
> -#if !defined(CONFIG_USER_ONLY)
> -static target_ulong syscall_err;
> -#endif
> -
>  static void arm_semi_cb(CPUState *cs, target_ulong ret, target_ulong err)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);


--
Alex Bennée
Peter Maydell Sept. 12, 2019, 10:49 a.m. UTC | #2
On Thu, 12 Sep 2019 at 11:40, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > The set_swi_errno() function is called to capture the errno
> > from a host system call, so that we can return -1 from the
> > semihosting function and later allow the guest to get a more
> > specific error code with the SYS_ERRNO function. It comes in
> > two versions, one for user-only and one for softmmu. We forgot
> > to capture the errno in the softmmu version; fix the error.
> >
> > (Semihosting calls directed to gdb are unaffected because
> > they go through a different code path that captures the
> > error return from the gdbstub call in arm_semi_cb() or
> > arm_semi_flen_cb().)
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > NB that a later commit will put in some cleanup of TaskState
> > that will let us reduce the duplication between the two
> > implementations of this function.
> > ---
> >  target/arm/arm-semi.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> > index 90423a35deb..03e60105c05 100644
> > --- a/target/arm/arm-semi.c
> > +++ b/target/arm/arm-semi.c
> > @@ -114,8 +114,13 @@ static inline uint32_t set_swi_errno(TaskState *ts, uint32_t code)
> >      return code;
> >  }
> >  #else
> > +static target_ulong syscall_err;
> > +
>
> I appreciate that this is just moving things around but this will be
> broken for -smp > 1 if two vCPUs make a syscall at the same time. For
> linux-user this information is kept in ts->swi_errno which is
> per-thread. Either we need a __thread version or find somewhere in
> CPUARMState to store it.

The semihosting API has no concept of errno being per-CPU at all.
(It really predates SMP entirely, so nobody was thinking about
that, and in any case it's a debug API, not a serious-work one).
The assumption is that the guest is handling this somehow.

The errno interface is not very useful anyway since it doesn't
actually define any errno values, and it's even worse in our
implementation because we use the host errno values rather
than defining some clean target-specific set of values...

thanks
-- PMM
diff mbox series

Patch

diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index 90423a35deb..03e60105c05 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -114,8 +114,13 @@  static inline uint32_t set_swi_errno(TaskState *ts, uint32_t code)
     return code;
 }
 #else
+static target_ulong syscall_err;
+
 static inline uint32_t set_swi_errno(CPUARMState *env, uint32_t code)
 {
+    if (code == (uint32_t)-1) {
+        syscall_err = errno;
+    }
     return code;
 }
 
@@ -124,10 +129,6 @@  static inline uint32_t set_swi_errno(CPUARMState *env, uint32_t code)
 
 static target_ulong arm_semi_syscall_len;
 
-#if !defined(CONFIG_USER_ONLY)
-static target_ulong syscall_err;
-#endif
-
 static void arm_semi_cb(CPUState *cs, target_ulong ret, target_ulong err)
 {
     ARMCPU *cpu = ARM_CPU(cs);