[Qemu-devel] [PATCH v2 00/15] target/arm: Implement semihosting v2.0

[Qemu-devel] [PATCH v2 00/15] target/arm: Implement semihosting v2.0

[Peter Maydell]

This patchset implements support in QEMU for v2.0 of the
Arm semihosting specification:
 https://developer.arm.com/docs/100863/latest/preface

Specifically, v2.0 has:
 * a mechanism for detection of optional extra features,
   which works by allowing the guest to open a magic file
   named ":semihosting-features" and read some feature
   flags from it
 * two defined extensions:
  - STDOUT_STDERR lets the guest separately open stdout and
    stderr via the ":tt" magic filename (v1.0 only allowed
    access to stdout)
  - EXIT_EXTENDED lets A32/T32 guests exit with a specified
    exit status (otherwise only available to A64 guests).
    This is something that people have been complaining
    about for a long time.

(Technically some of the things we already support, like
having an A64 semihosting interface at all, are also part of
the v2.0 spec.)

This patchset:
 * fixes some bugs relating to errnos in some cases
 * makes semihosting hand out its own filedescriptors rather
   than just passing out host fd numbers
 * abstracts out the fd-related semihosting calls so they
   indirect via a function table based on the type of the fd
 * adds a new type of fd representing the magic file
   ":semihosting-features" which is used for feature-detection
 * implements both of the extensions defined by the v2.0 spec

I've tested this by improving my semihosting test suite:
 https://git.linaro.org/people/peter.maydell/semihosting-tests.git/
(if people have other guest binaries that make much use of
semihosting then testing would certainly be welcome.)

Changes v1->v2:
 * Added a patch which corrects misunderstanding in a FIXME
   comment about the when the callback function is called
   for arm_gdb_syscall()
 * in patch 4, if the SYS_open is going via the gdbstub, we
   must do the associate_guestfd() work in the gdbstub callback
   function. This is because in softmmu mode the callback will
   not be called until after do_arm_semihosting() returns.
   (The v1 series effectively broke SYS_open in the gdbstub
   + softmmu config)
 * Pass CPUARMState* to set_swi_errno(), rather than creating
   an odd local-to-this-file typedef of TaskState for the
   softmmu compilation
 * New patch: avoid ifdeffery in gdb callback fns by
   using set_swi_errno() rather than doing it by-hand
 * The various 'factor out SYS_foo' patches are basically
   unchanged, but all the functions no longer need to take
   a TaskState*. This seemed kind of borderline as to whether
   to retain Alex's reviewed-by tags, so I dropped them.
 * Since we need 'env' for set_swi_errno(), we don't need
   to put the variable declaration inside ifdefs any more
   in the host_readfn() etc.

I do plan to have a go at fixing the odd FIXME surrounding
arm_gdb_syscall() which patch 3 clarifies/states in a comment.
But I thought it better to not tangle that up with this
patchset, which is already pretty long.

thanks
-- PMM


Peter Maydell (15):
  target/arm/arm-semi: Capture errno in softmmu version of
    set_swi_errno()
  target/arm/arm-semi: Always set some kind of errno for failed calls
  target/arm/arm-semi: Correct comment about gdb syscall races
  target/arm/arm-semi: Make semihosting code hand out its own file
    descriptors
  target/arm/arm-semi: Restrict use of TaskState*
  target/arm/arm-semi: Use set_swi_errno() in gdbstub callback functions
  target/arm/arm-semi: Factor out implementation of SYS_CLOSE
  target/arm/arm-semi: Factor out implementation of SYS_WRITE
  target/arm/arm-semi: Factor out implementation of SYS_READ
  target/arm/arm-semi: Factor out implementation of SYS_ISTTY
  target/arm/arm-semi: Factor out implementation of SYS_SEEK
  target/arm/arm-semi: Factor out implementation of SYS_FLEN
  target/arm/arm-semi: Implement support for semihosting feature
    detection
  target/arm/arm-semi: Implement SH_EXT_EXIT_EXTENDED extension
  target/arm/arm-semi: Implement SH_EXT_STDOUT_STDERR extension

 target/arm/arm-semi.c | 707 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 577 insertions(+), 130 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v2 01/15] target/arm/arm-semi: Capture errno in softmmu version of set_swi_errno()

[Peter Maydell]

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;
+
 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);
-- 
2.20.1

[Qemu-devel] [PATCH v2 02/15] target/arm/arm-semi: Always set some kind of errno for failed calls

[Peter Maydell]

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>
---
 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);               \
         }                                               \
     } 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;
-- 
2.20.1

[Qemu-devel] [PATCH v2 03/15] target/arm/arm-semi: Correct comment about gdb syscall races

[Peter Maydell]

In arm_gdb_syscall() we have a comment suggesting a race
because the syscall completion callback might not happen
before the gdb_do_syscallv() call returns. The comment is
correct that the callback may not happen but incorrect about
the effects. Correct it and note the important caveat that
callers must never do any work of any kind after return from
arm_gdb_syscall() that depends on its return value.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I'll come back to do the cleanup later, but I preferred
not to tangle it up with the rest of the refactoring in
this series; I also think it's probably easier done
afterwards rather than before.
---
 target/arm/arm-semi.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index 51b55816faf..302529f2278 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -217,10 +217,21 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb,
     gdb_do_syscallv(cb, fmt, va);
     va_end(va);
 
-    /* FIXME: we are implicitly relying on the syscall completing
-     * before this point, which is not guaranteed. We should
-     * put in an explicit synchronization between this and
-     * the callback function.
+    /*
+     * FIXME: in softmmu mode, the gdbstub will schedule our callback
+     * to occur, but will not actually call it to complete the syscall
+     * until after this function has returned and we are back in the
+     * CPU main loop. Therefore callers to this function must not
+     * do anything with its return value, because it is not necessarily
+     * the result of the syscall, but could just be the old value of X0.
+     * The only thing safe to do with this is that the callers of
+     * do_arm_semihosting() will write it straight back into X0.
+     * (In linux-user mode, the callback will have happened before
+     * gdb_do_syscallv() returns.)
+     *
+     * We should tidy this up so neither this function nor
+     * do_arm_semihosting() return a value, so the mistake of
+     * doing something with the return value is not possible to make.
      */
 
     return is_a64(env) ? env->xregs[0] : env->regs[0];
-- 
2.20.1

[Qemu-devel] [PATCH v2 04/15] target/arm/arm-semi: Make semihosting code hand out its own file descriptors

[Peter Maydell]

Currently the Arm semihosting code returns the guest file descriptors
(handles) which are simply the fd values from the host OS or the
remote gdbstub. Part of the semihosting 2.0 specification requires
that we implement special handling of opening a ":semihosting-features"
filename. Guest fds which result from opening the special file
won't correspond to host fds, so to ensure that we don't end up
with duplicate fds we need to have QEMU code control the allocation
of the fd values we give the guest.

Add in an abstraction layer which lets us allocate new guest FD
values, and translate from a guest FD value back to the host one.
This also fixes an odd hole where a semihosting guest could
use the semihosting API to read, write or close file descriptors
that it had never allocated but which were being used by QEMU itself.
(This isn't a security hole, because enabling semihosting permits
the guest to do arbitrary file access to the whole host filesystem,
and so should only be done if the guest is completely trusted.)

Currently the only kind of guest fd is one which maps to a
host fd, but in a following commit we will add one which maps
to the :semihosting-features magic data.

If the guest is migrated with an open semihosting file descriptor
then subsequent attempts to use the fd will all fail; this is
not a change from the previous situation (where the host fd
being used on the source end would not be re-opened on the
destination end).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Change since v1: we mustn't treat the return value of
arm_gdb_syscall() as being the new fd from gdb, as in
softmmu mode it is not. So we need a custom callback for open
that can update the guestfd association.
---
 target/arm/arm-semi.c | 232 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 216 insertions(+), 16 deletions(-)

diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index 302529f2278..a4741d7e11b 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -106,6 +106,113 @@ static int open_modeflags[12] = {
     O_RDWR | O_CREAT | O_APPEND | O_BINARY
 };
 
+typedef enum GuestFDType {
+    GuestFDUnused = 0,
+    GuestFDHost = 1,
+} GuestFDType;
+
+/*
+ * Guest file descriptors are integer indexes into an array of
+ * these structures (we will dynamically resize as necessary).
+ */
+typedef struct GuestFD {
+    GuestFDType type;
+    int hostfd;
+} GuestFD;
+
+static GArray *guestfd_array;
+
+/*
+ * Allocate a new guest file descriptor and return it; if we
+ * couldn't allocate a new fd then return -1.
+ * This is a fairly simplistic implementation because we don't
+ * expect that most semihosting guest programs will make very
+ * heavy use of opening and closing fds.
+ */
+static int alloc_guestfd(void)
+{
+    guint i;
+
+    if (!guestfd_array) {
+        /* New entries zero-initialized, i.e. type GuestFDUnused */
+        guestfd_array = g_array_new(FALSE, TRUE, sizeof(GuestFD));
+    }
+
+    for (i = 0; i < guestfd_array->len; i++) {
+        GuestFD *gf = &g_array_index(guestfd_array, GuestFD, i);
+
+        if (gf->type == GuestFDUnused) {
+            return i;
+        }
+    }
+
+    /* All elements already in use: expand the array */
+    g_array_set_size(guestfd_array, i + 1);
+    return i;
+}
+
+/*
+ * Look up the guestfd in the data structure; return NULL
+ * for out of bounds, but don't check whether the slot is unused.
+ * This is used internally by the other guestfd functions.
+ */
+static GuestFD *do_get_guestfd(int guestfd)
+{
+    if (!guestfd_array) {
+        return NULL;
+    }
+
+    if (guestfd < 0 || guestfd >= guestfd_array->len) {
+        return NULL;
+    }
+
+    return &g_array_index(guestfd_array, GuestFD, guestfd);
+}
+
+/*
+ * Associate the specified guest fd (which must have been
+ * allocated via alloc_fd() and not previously used) with
+ * the specified host fd.
+ */
+static void associate_guestfd(int guestfd, int hostfd)
+{
+    GuestFD *gf = do_get_guestfd(guestfd);
+
+    assert(gf);
+    gf->type = GuestFDHost;
+    gf->hostfd = hostfd;
+}
+
+/*
+ * Deallocate the specified guest file descriptor. This doesn't
+ * close the host fd, it merely undoes the work of alloc_fd().
+ */
+static void dealloc_guestfd(int guestfd)
+{
+    GuestFD *gf = do_get_guestfd(guestfd);
+
+    assert(gf);
+    gf->type = GuestFDUnused;
+}
+
+/*
+ * Given a guest file descriptor, get the associated struct.
+ * If the fd is not valid, return NULL. This is the function
+ * used by the various semihosting calls to validate a handle
+ * from the guest.
+ * Note: calling alloc_guestfd() or dealloc_guestfd() will
+ * invalidate any GuestFD* obtained by calling this function.
+ */
+static GuestFD *get_guestfd(int guestfd)
+{
+    GuestFD *gf = do_get_guestfd(guestfd);
+
+    if (!gf || gf->type == GuestFDUnused) {
+        return NULL;
+    }
+    return gf;
+}
+
 #ifdef CONFIG_USER_ONLY
 static inline uint32_t set_swi_errno(TaskState *ts, uint32_t code)
 {
@@ -207,6 +314,34 @@ static void arm_semi_flen_cb(CPUState *cs, target_ulong ret, target_ulong err)
 #endif
 }
 
+static int arm_semi_open_guestfd;
+
+static void arm_semi_open_cb(CPUState *cs, target_ulong ret, target_ulong err)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+#ifdef CONFIG_USER_ONLY
+    TaskState *ts = cs->opaque;
+#endif
+    if (ret == (target_ulong)-1) {
+#ifdef CONFIG_USER_ONLY
+        ts->swi_errno = err;
+#else
+        syscall_err = err;
+#endif
+        dealloc_guestfd(arm_semi_open_guestfd);
+    } else {
+        associate_guestfd(arm_semi_open_guestfd, ret);
+        ret = arm_semi_open_guestfd;
+    }
+
+    if (is_a64(env)) {
+        env->xregs[0] = ret;
+    } else {
+        env->regs[0] = ret;
+    }
+}
+
 static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb,
                                     const char *fmt, ...)
 {
@@ -283,6 +418,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
 #else
     CPUARMState *ts = env;
 #endif
+    GuestFD *gf;
 
     if (is_a64(env)) {
         /* Note that the syscall number is in W0, not X0 */
@@ -295,6 +431,9 @@ target_ulong do_arm_semihosting(CPUARMState *env)
 
     switch (nr) {
     case TARGET_SYS_OPEN:
+    {
+        int guestfd;
+
         GET_ARG(0);
         GET_ARG(1);
         GET_ARG(2);
@@ -308,26 +447,52 @@ target_ulong do_arm_semihosting(CPUARMState *env)
             errno = EINVAL;
             return set_swi_errno(ts, -1);
         }
+
+        guestfd = alloc_guestfd();
+        if (guestfd < 0) {
+            unlock_user(s, arg0, 0);
+            errno = EMFILE;
+            return set_swi_errno(ts, -1);
+        }
+
         if (strcmp(s, ":tt") == 0) {
             int result_fileno = arg1 < 4 ? STDIN_FILENO : STDOUT_FILENO;
+            associate_guestfd(guestfd, result_fileno);
             unlock_user(s, arg0, 0);
-            return result_fileno;
+            return guestfd;
         }
         if (use_gdb_syscalls()) {
-            ret = arm_gdb_syscall(cpu, arm_semi_cb, "open,%s,%x,1a4", arg0,
+            arm_semi_open_guestfd = guestfd;
+            ret = arm_gdb_syscall(cpu, arm_semi_open_cb, "open,%s,%x,1a4", arg0,
                                   (int)arg2+1, gdb_open_modeflags[arg1]);
         } else {
             ret = set_swi_errno(ts, open(s, open_modeflags[arg1], 0644));
+            if (ret == (uint32_t)-1) {
+                dealloc_guestfd(guestfd);
+            } else {
+                associate_guestfd(guestfd, ret);
+                ret = guestfd;
+            }
         }
         unlock_user(s, arg0, 0);
         return ret;
+    }
     case TARGET_SYS_CLOSE:
         GET_ARG(0);
-        if (use_gdb_syscalls()) {
-            return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", arg0);
-        } else {
-            return set_swi_errno(ts, close(arg0));
+
+        gf = get_guestfd(arg0);
+        if (!gf) {
+            errno = EBADF;
+            return set_swi_errno(ts, -1);
         }
+
+        if (use_gdb_syscalls()) {
+            ret = arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
+        } else {
+            ret = set_swi_errno(ts, close(gf->hostfd));
+        }
+        dealloc_guestfd(arg0);
+        return ret;
     case TARGET_SYS_WRITEC:
         qemu_semihosting_console_outc(env, args);
         return 0xdeadbeef;
@@ -338,17 +503,24 @@ target_ulong do_arm_semihosting(CPUARMState *env)
         GET_ARG(1);
         GET_ARG(2);
         len = arg2;
+
+        gf = get_guestfd(arg0);
+        if (!gf) {
+            errno = EBADF;
+            return set_swi_errno(ts, -1);
+        }
+
         if (use_gdb_syscalls()) {
             arm_semi_syscall_len = len;
             return arm_gdb_syscall(cpu, arm_semi_cb, "write,%x,%x,%x",
-                                   arg0, arg1, len);
+                                   gf->hostfd, arg1, len);
         } else {
             s = lock_user(VERIFY_READ, arg1, len, 1);
             if (!s) {
                 /* Return bytes not written on error */
                 return len;
             }
-            ret = set_swi_errno(ts, write(arg0, s, len));
+            ret = set_swi_errno(ts, write(gf->hostfd, s, len));
             unlock_user(s, arg1, 0);
             if (ret == (uint32_t)-1) {
                 ret = 0;
@@ -361,10 +533,17 @@ target_ulong do_arm_semihosting(CPUARMState *env)
         GET_ARG(1);
         GET_ARG(2);
         len = arg2;
+
+        gf = get_guestfd(arg0);
+        if (!gf) {
+            errno = EBADF;
+            return set_swi_errno(ts, -1);
+        }
+
         if (use_gdb_syscalls()) {
             arm_semi_syscall_len = len;
             return arm_gdb_syscall(cpu, arm_semi_cb, "read,%x,%x,%x",
-                                   arg0, arg1, len);
+                                   gf->hostfd, arg1, len);
         } else {
             s = lock_user(VERIFY_WRITE, arg1, len, 0);
             if (!s) {
@@ -372,7 +551,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
                 return len;
             }
             do {
-                ret = set_swi_errno(ts, read(arg0, s, len));
+                ret = set_swi_errno(ts, read(gf->hostfd, s, len));
             } while (ret == -1 && errno == EINTR);
             unlock_user(s, arg1, len);
             if (ret == (uint32_t)-1) {
@@ -386,31 +565,52 @@ target_ulong do_arm_semihosting(CPUARMState *env)
         return 0;
     case TARGET_SYS_ISTTY:
         GET_ARG(0);
+
+        gf = get_guestfd(arg0);
+        if (!gf) {
+            errno = EBADF;
+            return set_swi_errno(ts, -1);
+        }
+
         if (use_gdb_syscalls()) {
-            return arm_gdb_syscall(cpu, arm_semi_cb, "isatty,%x", arg0);
+            return arm_gdb_syscall(cpu, arm_semi_cb, "isatty,%x", gf->hostfd);
         } else {
-            return isatty(arg0);
+            return isatty(gf->hostfd);
         }
     case TARGET_SYS_SEEK:
         GET_ARG(0);
         GET_ARG(1);
+
+        gf = get_guestfd(arg0);
+        if (!gf) {
+            errno = EBADF;
+            return set_swi_errno(ts, -1);
+        }
+
         if (use_gdb_syscalls()) {
             return arm_gdb_syscall(cpu, arm_semi_cb, "lseek,%x,%x,0",
-                                   arg0, arg1);
+                                   gf->hostfd, arg1);
         } else {
-            ret = set_swi_errno(ts, lseek(arg0, arg1, SEEK_SET));
+            ret = set_swi_errno(ts, lseek(gf->hostfd, arg1, SEEK_SET));
             if (ret == (uint32_t)-1)
               return -1;
             return 0;
         }
     case TARGET_SYS_FLEN:
         GET_ARG(0);
+
+        gf = get_guestfd(arg0);
+        if (!gf) {
+            errno = EBADF;
+            return set_swi_errno(ts, -1);
+        }
+
         if (use_gdb_syscalls()) {
             return arm_gdb_syscall(cpu, arm_semi_flen_cb, "fstat,%x,%x",
-                                   arg0, arm_flen_buf(cpu));
+                                   gf->hostfd, arm_flen_buf(cpu));
         } else {
             struct stat buf;
-            ret = set_swi_errno(ts, fstat(arg0, &buf));
+            ret = set_swi_errno(ts, fstat(gf->hostfd, &buf));
             if (ret == (uint32_t)-1)
                 return -1;
             return buf.st_size;
-- 
2.20.1

[Qemu-devel] [PATCH v2 05/15] target/arm/arm-semi: Restrict use of TaskState*

[Peter Maydell]

The semihosting code needs accuss to the linux-user only
TaskState pointer so it can set the semihosting errno per-thread
for linux-user mode. At the moment we do this by having some
ifdefs so that we define a 'ts' local in do_arm_semihosting()
which is either a real TaskState * or just a CPUARMState *,
depending on which mode we're compiling for.

This is awkward if we want to refactor do_arm_semihosting()
into other functions which might need to be passed the TaskState.
Restrict usage of the TaskState local by:
 * making set_swi_errno() always take the CPUARMState pointer
   and (for the linux-user version) get TaskState from that
 * creating a new get_swi_errno() which reads the errno
 * having the two semihosting calls which need the TaskState
   for other purposes (SYS_GET_CMDLINE and SYS_HEAPINFO)
   define a variable with scope restricted to just that code

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
We use 'CPUARMState *', aka 'env', rather than the other
options of passing the ARMCPU* or the CPUState *, purely
because it means that the later refactoring of each SYS_*
can pass just the CPUARMState * and incidentally avoid
an ugly ifdef caused by the implicit use of env in the
softmmu lock_user().
---
 target/arm/arm-semi.c | 111 ++++++++++++++++++++++++------------------
 1 file changed, 63 insertions(+), 48 deletions(-)

diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index a4741d7e11b..2618588076f 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -213,26 +213,45 @@ static GuestFD *get_guestfd(int guestfd)
     return gf;
 }
 
-#ifdef CONFIG_USER_ONLY
-static inline uint32_t set_swi_errno(TaskState *ts, uint32_t code)
-{
-    if (code == (uint32_t)-1)
-        ts->swi_errno = errno;
-    return code;
-}
-#else
+/*
+ * The semihosting API has no concept of its errno being thread-safe,
+ * as the API design predates SMP CPUs and was intended as a simple
+ * real-hardware set of debug functionality. For QEMU, we make the
+ * errno be per-thread in linux-user mode; in softmmu it is a simple
+ * global, and we assume that the guest takes care of avoiding any races.
+ */
+#ifndef CONFIG_USER_ONLY
 static target_ulong syscall_err;
 
+#include "exec/softmmu-semi.h"
+#endif
+
 static inline uint32_t set_swi_errno(CPUARMState *env, uint32_t code)
 {
     if (code == (uint32_t)-1) {
+#ifdef CONFIG_USER_ONLY
+        CPUState *cs = env_cpu(env);
+        TaskState *ts = cs->opaque;
+
+        ts->swi_errno = errno;
+#else
         syscall_err = errno;
+#endif
     }
     return code;
 }
 
-#include "exec/softmmu-semi.h"
+static inline uint32_t get_swi_errno(CPUARMState *env)
+{
+#ifdef CONFIG_USER_ONLY
+    CPUState *cs = env_cpu(env);
+    TaskState *ts = cs->opaque;
+
+    return ts->swi_errno;
+#else
+    return syscall_err;
 #endif
+}
 
 static target_ulong arm_semi_syscall_len;
 
@@ -379,12 +398,12 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb,
     if (is_a64(env)) {                                  \
         if (get_user_u64(arg ## n, args + (n) * 8)) {   \
             errno = EFAULT;                             \
-            return set_swi_errno(ts, -1);               \
+            return set_swi_errno(env, -1);              \
         }                                               \
     } else {                                            \
         if (get_user_u32(arg ## n, args + (n) * 4)) {   \
             errno = EFAULT;                             \
-            return set_swi_errno(ts, -1);               \
+            return set_swi_errno(env, -1);              \
         }                                               \
     }                                                   \
 } while (0)
@@ -413,11 +432,6 @@ target_ulong do_arm_semihosting(CPUARMState *env)
     int nr;
     uint32_t ret;
     uint32_t len;
-#ifdef CONFIG_USER_ONLY
-    TaskState *ts = cs->opaque;
-#else
-    CPUARMState *ts = env;
-#endif
     GuestFD *gf;
 
     if (is_a64(env)) {
@@ -440,19 +454,19 @@ target_ulong do_arm_semihosting(CPUARMState *env)
         s = lock_user_string(arg0);
         if (!s) {
             errno = EFAULT;
-            return set_swi_errno(ts, -1);
+            return set_swi_errno(env, -1);
         }
         if (arg1 >= 12) {
             unlock_user(s, arg0, 0);
             errno = EINVAL;
-            return set_swi_errno(ts, -1);
+            return set_swi_errno(env, -1);
         }
 
         guestfd = alloc_guestfd();
         if (guestfd < 0) {
             unlock_user(s, arg0, 0);
             errno = EMFILE;
-            return set_swi_errno(ts, -1);
+            return set_swi_errno(env, -1);
         }
 
         if (strcmp(s, ":tt") == 0) {
@@ -466,7 +480,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
             ret = arm_gdb_syscall(cpu, arm_semi_open_cb, "open,%s,%x,1a4", arg0,
                                   (int)arg2+1, gdb_open_modeflags[arg1]);
         } else {
-            ret = set_swi_errno(ts, open(s, open_modeflags[arg1], 0644));
+            ret = set_swi_errno(env, open(s, open_modeflags[arg1], 0644));
             if (ret == (uint32_t)-1) {
                 dealloc_guestfd(guestfd);
             } else {
@@ -483,13 +497,13 @@ target_ulong do_arm_semihosting(CPUARMState *env)
         gf = get_guestfd(arg0);
         if (!gf) {
             errno = EBADF;
-            return set_swi_errno(ts, -1);
+            return set_swi_errno(env, -1);
         }
 
         if (use_gdb_syscalls()) {
             ret = arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
         } else {
-            ret = set_swi_errno(ts, close(gf->hostfd));
+            ret = set_swi_errno(env, close(gf->hostfd));
         }
         dealloc_guestfd(arg0);
         return ret;
@@ -507,7 +521,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
         gf = get_guestfd(arg0);
         if (!gf) {
             errno = EBADF;
-            return set_swi_errno(ts, -1);
+            return set_swi_errno(env, -1);
         }
 
         if (use_gdb_syscalls()) {
@@ -520,7 +534,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
                 /* Return bytes not written on error */
                 return len;
             }
-            ret = set_swi_errno(ts, write(gf->hostfd, s, len));
+            ret = set_swi_errno(env, write(gf->hostfd, s, len));
             unlock_user(s, arg1, 0);
             if (ret == (uint32_t)-1) {
                 ret = 0;
@@ -537,7 +551,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
         gf = get_guestfd(arg0);
         if (!gf) {
             errno = EBADF;
-            return set_swi_errno(ts, -1);
+            return set_swi_errno(env, -1);
         }
 
         if (use_gdb_syscalls()) {
@@ -551,7 +565,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
                 return len;
             }
             do {
-                ret = set_swi_errno(ts, read(gf->hostfd, s, len));
+                ret = set_swi_errno(env, read(gf->hostfd, s, len));
             } while (ret == -1 && errno == EINTR);
             unlock_user(s, arg1, len);
             if (ret == (uint32_t)-1) {
@@ -569,7 +583,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
         gf = get_guestfd(arg0);
         if (!gf) {
             errno = EBADF;
-            return set_swi_errno(ts, -1);
+            return set_swi_errno(env, -1);
         }
 
         if (use_gdb_syscalls()) {
@@ -584,14 +598,14 @@ target_ulong do_arm_semihosting(CPUARMState *env)
         gf = get_guestfd(arg0);
         if (!gf) {
             errno = EBADF;
-            return set_swi_errno(ts, -1);
+            return set_swi_errno(env, -1);
         }
 
         if (use_gdb_syscalls()) {
             return arm_gdb_syscall(cpu, arm_semi_cb, "lseek,%x,%x,0",
                                    gf->hostfd, arg1);
         } else {
-            ret = set_swi_errno(ts, lseek(gf->hostfd, arg1, SEEK_SET));
+            ret = set_swi_errno(env, lseek(gf->hostfd, arg1, SEEK_SET));
             if (ret == (uint32_t)-1)
               return -1;
             return 0;
@@ -602,7 +616,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
         gf = get_guestfd(arg0);
         if (!gf) {
             errno = EBADF;
-            return set_swi_errno(ts, -1);
+            return set_swi_errno(env, -1);
         }
 
         if (use_gdb_syscalls()) {
@@ -610,7 +624,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
                                    gf->hostfd, arm_flen_buf(cpu));
         } else {
             struct stat buf;
-            ret = set_swi_errno(ts, fstat(gf->hostfd, &buf));
+            ret = set_swi_errno(env, fstat(gf->hostfd, &buf));
             if (ret == (uint32_t)-1)
                 return -1;
             return buf.st_size;
@@ -628,9 +642,9 @@ target_ulong do_arm_semihosting(CPUARMState *env)
             s = lock_user_string(arg0);
             if (!s) {
                 errno = EFAULT;
-                return set_swi_errno(ts, -1);
+                return set_swi_errno(env, -1);
             }
-            ret =  set_swi_errno(ts, remove(s));
+            ret =  set_swi_errno(env, remove(s));
             unlock_user(s, arg0, 0);
         }
         return ret;
@@ -648,9 +662,9 @@ target_ulong do_arm_semihosting(CPUARMState *env)
             s2 = lock_user_string(arg2);
             if (!s || !s2) {
                 errno = EFAULT;
-                ret = set_swi_errno(ts, -1);
+                ret = set_swi_errno(env, -1);
             } else {
-                ret = set_swi_errno(ts, rename(s, s2));
+                ret = set_swi_errno(env, rename(s, s2));
             }
             if (s2)
                 unlock_user(s2, arg2, 0);
@@ -661,7 +675,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
     case TARGET_SYS_CLOCK:
         return clock() / (CLOCKS_PER_SEC / 100);
     case TARGET_SYS_TIME:
-        return set_swi_errno(ts, time(NULL));
+        return set_swi_errno(env, time(NULL));
     case TARGET_SYS_SYSTEM:
         GET_ARG(0);
         GET_ARG(1);
@@ -672,18 +686,14 @@ target_ulong do_arm_semihosting(CPUARMState *env)
             s = lock_user_string(arg0);
             if (!s) {
                 errno = EFAULT;
-                return set_swi_errno(ts, -1);
+                return set_swi_errno(env, -1);
             }
-            ret = set_swi_errno(ts, system(s));
+            ret = set_swi_errno(env, system(s));
             unlock_user(s, arg0, 0);
             return ret;
         }
     case TARGET_SYS_ERRNO:
-#ifdef CONFIG_USER_ONLY
-        return ts->swi_errno;
-#else
-        return syscall_err;
-#endif
+        return get_swi_errno(env);
     case TARGET_SYS_GET_CMDLINE:
         {
             /* Build a command-line from the original argv.
@@ -706,6 +716,8 @@ target_ulong do_arm_semihosting(CPUARMState *env)
             int status = 0;
 #if !defined(CONFIG_USER_ONLY)
             const char *cmdline;
+#else
+            TaskState *ts = cs->opaque;
 #endif
             GET_ARG(0);
             GET_ARG(1);
@@ -733,21 +745,21 @@ target_ulong do_arm_semihosting(CPUARMState *env)
             if (output_size > input_size) {
                 /* Not enough space to store command-line arguments.  */
                 errno = E2BIG;
-                return set_swi_errno(ts, -1);
+                return set_swi_errno(env, -1);
             }
 
             /* Adjust the command-line length.  */
             if (SET_ARG(1, output_size - 1)) {
                 /* Couldn't write back to argument block */
                 errno = EFAULT;
-                return set_swi_errno(ts, -1);
+                return set_swi_errno(env, -1);
             }
 
             /* Lock the buffer on the ARM side.  */
             output_buffer = lock_user(VERIFY_WRITE, arg0, output_size, 0);
             if (!output_buffer) {
                 errno = EFAULT;
-                return set_swi_errno(ts, -1);
+                return set_swi_errno(env, -1);
             }
 
             /* Copy the command-line arguments.  */
@@ -763,7 +775,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
             if (copy_from_user(output_buffer, ts->info->arg_start,
                                output_size)) {
                 errno = EFAULT;
-                status = set_swi_errno(ts, -1);
+                status = set_swi_errno(env, -1);
                 goto out;
             }
 
@@ -785,6 +797,9 @@ target_ulong do_arm_semihosting(CPUARMState *env)
             target_ulong retvals[4];
             target_ulong limit;
             int i;
+#ifdef CONFIG_USER_ONLY
+            TaskState *ts = cs->opaque;
+#endif
 
             GET_ARG(0);
 
@@ -834,7 +849,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
                 if (fail) {
                     /* Couldn't write back to argument block */
                     errno = EFAULT;
-                    return set_swi_errno(ts, -1);
+                    return set_swi_errno(env, -1);
                 }
             }
             return 0;
-- 
2.20.1

[Qemu-devel] [PATCH v2 06/15] target/arm/arm-semi: Use set_swi_errno() in gdbstub callback functions

[Peter Maydell]

When we are routing semihosting operations through the gdbstub, the
work of sorting out the return value and setting errno if necessary
is done by callback functions which are invoked by the gdbstub code.
Clean up some ifdeffery in those functions by having them call
set_swi_errno() to set the semihosting errno.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/arm-semi.c | 27 ++++++---------------------
 1 file changed, 6 insertions(+), 21 deletions(-)

diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index 2618588076f..02cd673d47d 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -259,17 +259,11 @@ static void arm_semi_cb(CPUState *cs, target_ulong ret, target_ulong err)
 {
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
-#ifdef CONFIG_USER_ONLY
-    TaskState *ts = cs->opaque;
-#endif
     target_ulong reg0 = is_a64(env) ? env->xregs[0] : env->regs[0];
 
     if (ret == (target_ulong)-1) {
-#ifdef CONFIG_USER_ONLY
-        ts->swi_errno = err;
-#else
-        syscall_err = err;
-#endif
+        errno = err;
+        set_swi_errno(env, -1);
         reg0 = ret;
     } else {
         /* Fixup syscalls that use nonstardard return conventions.  */
@@ -326,11 +320,8 @@ static void arm_semi_flen_cb(CPUState *cs, target_ulong ret, target_ulong err)
     } else {
         env->regs[0] = size;
     }
-#ifdef CONFIG_USER_ONLY
-    ((TaskState *)cs->opaque)->swi_errno = err;
-#else
-    syscall_err = err;
-#endif
+    errno = err;
+    set_swi_errno(env, -1);
 }
 
 static int arm_semi_open_guestfd;
@@ -339,15 +330,9 @@ static void arm_semi_open_cb(CPUState *cs, target_ulong ret, target_ulong err)
 {
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
-#ifdef CONFIG_USER_ONLY
-    TaskState *ts = cs->opaque;
-#endif
     if (ret == (target_ulong)-1) {
-#ifdef CONFIG_USER_ONLY
-        ts->swi_errno = err;
-#else
-        syscall_err = err;
-#endif
+        errno = err;
+        set_swi_errno(env, -1);
         dealloc_guestfd(arm_semi_open_guestfd);
     } else {
         associate_guestfd(arm_semi_open_guestfd, ret);
-- 
2.20.1

[Qemu-devel] [PATCH v2 07/15] target/arm/arm-semi: Factor out implementation of SYS_CLOSE

[Peter Maydell]

Currently for the semihosting calls which take a file descriptor
(SYS_CLOSE, SYS_WRITE, SYS_READ, SYS_ISTTY, SYS_SEEK, SYS_FLEN)
we have effectively two implementations, one for real host files
and one for when we indirect via the gdbstub. We want to add a
third one to deal with the magic :semihosting-features file.

Instead of having a three-way if statement in each of these
cases, factor out the implementation of the calls to separate
functions which we dispatch to via function pointers selected
via the GuestFDType for the guest fd.

In this commit, we set up the framework for the dispatch,
and convert the SYS_CLOSE call to use it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/arm-semi.c | 44 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 7 deletions(-)

diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index 02cd673d47d..e5f1e2aaaf2 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -109,6 +109,7 @@ static int open_modeflags[12] = {
 typedef enum GuestFDType {
     GuestFDUnused = 0,
     GuestFDHost = 1,
+    GuestFDGDB = 2,
 } GuestFDType;
 
 /*
@@ -172,14 +173,14 @@ static GuestFD *do_get_guestfd(int guestfd)
 /*
  * Associate the specified guest fd (which must have been
  * allocated via alloc_fd() and not previously used) with
- * the specified host fd.
+ * the specified host/gdb fd.
  */
 static void associate_guestfd(int guestfd, int hostfd)
 {
     GuestFD *gf = do_get_guestfd(guestfd);
 
     assert(gf);
-    gf->type = GuestFDHost;
+    gf->type = use_gdb_syscalls() ? GuestFDGDB : GuestFDHost;
     gf->hostfd = hostfd;
 }
 
@@ -376,6 +377,39 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb,
     return is_a64(env) ? env->xregs[0] : env->regs[0];
 }
 
+/*
+ * Types for functions implementing various semihosting calls
+ * for specific types of guest file descriptor. These must all
+ * do the work and return the required return value for the guest,
+ * setting the guest errno if appropriate.
+ */
+typedef uint32_t sys_closefn(ARMCPU *cpu, GuestFD *gf);
+
+static uint32_t host_closefn(ARMCPU *cpu, GuestFD *gf)
+{
+    CPUARMState *env = &cpu->env;
+
+    return set_swi_errno(env, close(gf->hostfd));
+}
+
+static uint32_t gdb_closefn(ARMCPU *cpu, GuestFD *gf)
+{
+    return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
+}
+
+typedef struct GuestFDFunctions {
+    sys_closefn *closefn;
+} GuestFDFunctions;
+
+static const GuestFDFunctions guestfd_fns[] = {
+    [GuestFDHost] = {
+        .closefn = host_closefn,
+    },
+    [GuestFDGDB] = {
+        .closefn = gdb_closefn,
+    },
+};
+
 /* Read the input value from the argument block; fail the semihosting
  * call if the memory read fails.
  */
@@ -485,11 +519,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
             return set_swi_errno(env, -1);
         }
 
-        if (use_gdb_syscalls()) {
-            ret = arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
-        } else {
-            ret = set_swi_errno(env, close(gf->hostfd));
-        }
+        ret = guestfd_fns[gf->type].closefn(cpu, gf);
         dealloc_guestfd(arg0);
         return ret;
     case TARGET_SYS_WRITEC:
-- 
2.20.1

[Qemu-devel] [PATCH v2 08/15] target/arm/arm-semi: Factor out implementation of SYS_WRITE

[Peter Maydell]

Factor out the implementation of SYS_WRITE via the
new function tables.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/arm-semi.c | 51 ++++++++++++++++++++++++++++---------------
 1 file changed, 33 insertions(+), 18 deletions(-)

diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index e5f1e2aaaf2..c21cbb97bc1 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -384,6 +384,8 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb,
  * setting the guest errno if appropriate.
  */
 typedef uint32_t sys_closefn(ARMCPU *cpu, GuestFD *gf);
+typedef uint32_t sys_writefn(ARMCPU *cpu, GuestFD *gf,
+                             target_ulong buf, uint32_t len);
 
 static uint32_t host_closefn(ARMCPU *cpu, GuestFD *gf)
 {
@@ -392,21 +394,51 @@ static uint32_t host_closefn(ARMCPU *cpu, GuestFD *gf)
     return set_swi_errno(env, close(gf->hostfd));
 }
 
+static uint32_t host_writefn(ARMCPU *cpu, GuestFD *gf,
+                             target_ulong buf, uint32_t len)
+{
+    uint32_t ret;
+    CPUARMState *env = &cpu->env;
+    char *s = lock_user(VERIFY_READ, buf, len, 1);
+    if (!s) {
+        /* Return bytes not written on error */
+        return len;
+    }
+    ret = set_swi_errno(env, write(gf->hostfd, s, len));
+    unlock_user(s, buf, 0);
+    if (ret == (uint32_t)-1) {
+        ret = 0;
+    }
+    /* Return bytes not written */
+    return len - ret;
+}
+
 static uint32_t gdb_closefn(ARMCPU *cpu, GuestFD *gf)
 {
     return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
 }
 
+static uint32_t gdb_writefn(ARMCPU *cpu, GuestFD *gf,
+                            target_ulong buf, uint32_t len)
+{
+    arm_semi_syscall_len = len;
+    return arm_gdb_syscall(cpu, arm_semi_cb, "write,%x,%x,%x",
+                           gf->hostfd, buf, len);
+}
+
 typedef struct GuestFDFunctions {
     sys_closefn *closefn;
+    sys_writefn *writefn;
 } GuestFDFunctions;
 
 static const GuestFDFunctions guestfd_fns[] = {
     [GuestFDHost] = {
         .closefn = host_closefn,
+        .writefn = host_writefn,
     },
     [GuestFDGDB] = {
         .closefn = gdb_closefn,
+        .writefn = gdb_writefn,
     },
 };
 
@@ -539,24 +571,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
             return set_swi_errno(env, -1);
         }
 
-        if (use_gdb_syscalls()) {
-            arm_semi_syscall_len = len;
-            return arm_gdb_syscall(cpu, arm_semi_cb, "write,%x,%x,%x",
-                                   gf->hostfd, arg1, len);
-        } else {
-            s = lock_user(VERIFY_READ, arg1, len, 1);
-            if (!s) {
-                /* Return bytes not written on error */
-                return len;
-            }
-            ret = set_swi_errno(env, write(gf->hostfd, s, len));
-            unlock_user(s, arg1, 0);
-            if (ret == (uint32_t)-1) {
-                ret = 0;
-            }
-            /* Return bytes not written */
-            return len - ret;
-        }
+        return guestfd_fns[gf->type].writefn(cpu, gf, arg1, len);
     case TARGET_SYS_READ:
         GET_ARG(0);
         GET_ARG(1);
-- 
2.20.1

[Qemu-devel] [PATCH v2 09/15] target/arm/arm-semi: Factor out implementation of SYS_READ

[Peter Maydell]

Factor out the implementation of SYS_READ via the
new function tables.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/arm-semi.c | 55 +++++++++++++++++++++++++++----------------
 1 file changed, 35 insertions(+), 20 deletions(-)

diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index c21cbb97bc1..958083a105c 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -386,6 +386,8 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb,
 typedef uint32_t sys_closefn(ARMCPU *cpu, GuestFD *gf);
 typedef uint32_t sys_writefn(ARMCPU *cpu, GuestFD *gf,
                              target_ulong buf, uint32_t len);
+typedef uint32_t sys_readfn(ARMCPU *cpu, GuestFD *gf,
+                            target_ulong buf, uint32_t len);
 
 static uint32_t host_closefn(ARMCPU *cpu, GuestFD *gf)
 {
@@ -413,6 +415,27 @@ static uint32_t host_writefn(ARMCPU *cpu, GuestFD *gf,
     return len - ret;
 }
 
+static uint32_t host_readfn(ARMCPU *cpu, GuestFD *gf,
+                            target_ulong buf, uint32_t len)
+{
+    uint32_t ret;
+    CPUARMState *env = &cpu->env;
+    char *s = lock_user(VERIFY_WRITE, buf, len, 0);
+    if (!s) {
+        /* return bytes not read */
+        return len;
+    }
+    do {
+        ret = set_swi_errno(env, read(gf->hostfd, s, len));
+    } while (ret == -1 && errno == EINTR);
+    unlock_user(s, buf, len);
+    if (ret == (uint32_t)-1) {
+        ret = 0;
+    }
+    /* Return bytes not read */
+    return len - ret;
+}
+
 static uint32_t gdb_closefn(ARMCPU *cpu, GuestFD *gf)
 {
     return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
@@ -426,19 +449,30 @@ static uint32_t gdb_writefn(ARMCPU *cpu, GuestFD *gf,
                            gf->hostfd, buf, len);
 }
 
+static uint32_t gdb_readfn(ARMCPU *cpu, GuestFD *gf,
+                           target_ulong buf, uint32_t len)
+{
+    arm_semi_syscall_len = len;
+    return arm_gdb_syscall(cpu, arm_semi_cb, "read,%x,%x,%x",
+                           gf->hostfd, buf, len);
+}
+
 typedef struct GuestFDFunctions {
     sys_closefn *closefn;
     sys_writefn *writefn;
+    sys_readfn *readfn;
 } GuestFDFunctions;
 
 static const GuestFDFunctions guestfd_fns[] = {
     [GuestFDHost] = {
         .closefn = host_closefn,
         .writefn = host_writefn,
+        .readfn = host_readfn,
     },
     [GuestFDGDB] = {
         .closefn = gdb_closefn,
         .writefn = gdb_writefn,
+        .readfn = gdb_readfn,
     },
 };
 
@@ -584,26 +618,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
             return set_swi_errno(env, -1);
         }
 
-        if (use_gdb_syscalls()) {
-            arm_semi_syscall_len = len;
-            return arm_gdb_syscall(cpu, arm_semi_cb, "read,%x,%x,%x",
-                                   gf->hostfd, arg1, len);
-        } else {
-            s = lock_user(VERIFY_WRITE, arg1, len, 0);
-            if (!s) {
-                /* return bytes not read */
-                return len;
-            }
-            do {
-                ret = set_swi_errno(env, read(gf->hostfd, s, len));
-            } while (ret == -1 && errno == EINTR);
-            unlock_user(s, arg1, len);
-            if (ret == (uint32_t)-1) {
-                ret = 0;
-            }
-            /* Return bytes not read */
-            return len - ret;
-        }
+        return guestfd_fns[gf->type].readfn(cpu, gf, arg1, len);
     case TARGET_SYS_READC:
         qemu_log_mask(LOG_UNIMP, "%s: SYS_READC not implemented", __func__);
         return 0;
-- 
2.20.1

[Qemu-devel] [PATCH v2 10/15] target/arm/arm-semi: Factor out implementation of SYS_ISTTY

[Peter Maydell]

Factor out the implementation of SYS_ISTTY via the new function
tables.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/arm-semi.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index 958083a105c..ecd51338fd3 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -388,6 +388,7 @@ typedef uint32_t sys_writefn(ARMCPU *cpu, GuestFD *gf,
                              target_ulong buf, uint32_t len);
 typedef uint32_t sys_readfn(ARMCPU *cpu, GuestFD *gf,
                             target_ulong buf, uint32_t len);
+typedef uint32_t sys_isattyfn(ARMCPU *cpu, GuestFD *gf);
 
 static uint32_t host_closefn(ARMCPU *cpu, GuestFD *gf)
 {
@@ -436,6 +437,11 @@ static uint32_t host_readfn(ARMCPU *cpu, GuestFD *gf,
     return len - ret;
 }
 
+static uint32_t host_isattyfn(ARMCPU *cpu, GuestFD *gf)
+{
+    return isatty(gf->hostfd);
+}
+
 static uint32_t gdb_closefn(ARMCPU *cpu, GuestFD *gf)
 {
     return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
@@ -457,10 +463,16 @@ static uint32_t gdb_readfn(ARMCPU *cpu, GuestFD *gf,
                            gf->hostfd, buf, len);
 }
 
+static uint32_t gdb_isattyfn(ARMCPU *cpu, GuestFD *gf)
+{
+    return arm_gdb_syscall(cpu, arm_semi_cb, "isatty,%x", gf->hostfd);
+}
+
 typedef struct GuestFDFunctions {
     sys_closefn *closefn;
     sys_writefn *writefn;
     sys_readfn *readfn;
+    sys_isattyfn *isattyfn;
 } GuestFDFunctions;
 
 static const GuestFDFunctions guestfd_fns[] = {
@@ -468,11 +480,13 @@ static const GuestFDFunctions guestfd_fns[] = {
         .closefn = host_closefn,
         .writefn = host_writefn,
         .readfn = host_readfn,
+        .isattyfn = host_isattyfn,
     },
     [GuestFDGDB] = {
         .closefn = gdb_closefn,
         .writefn = gdb_writefn,
         .readfn = gdb_readfn,
+        .isattyfn = gdb_isattyfn,
     },
 };
 
@@ -631,11 +645,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
             return set_swi_errno(env, -1);
         }
 
-        if (use_gdb_syscalls()) {
-            return arm_gdb_syscall(cpu, arm_semi_cb, "isatty,%x", gf->hostfd);
-        } else {
-            return isatty(gf->hostfd);
-        }
+        return guestfd_fns[gf->type].isattyfn(cpu, gf);
     case TARGET_SYS_SEEK:
         GET_ARG(0);
         GET_ARG(1);
-- 
2.20.1

[Qemu-devel] [PATCH v2 11/15] target/arm/arm-semi: Factor out implementation of SYS_SEEK

[Peter Maydell]

Factor out the implementation of SYS_SEEK via the new function
tables.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/arm-semi.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index ecd51338fd3..b5e1d73eb80 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -389,6 +389,8 @@ typedef uint32_t sys_writefn(ARMCPU *cpu, GuestFD *gf,
 typedef uint32_t sys_readfn(ARMCPU *cpu, GuestFD *gf,
                             target_ulong buf, uint32_t len);
 typedef uint32_t sys_isattyfn(ARMCPU *cpu, GuestFD *gf);
+typedef uint32_t sys_seekfn(ARMCPU *cpu, GuestFD *gf,
+                            target_ulong offset);
 
 static uint32_t host_closefn(ARMCPU *cpu, GuestFD *gf)
 {
@@ -442,6 +444,16 @@ static uint32_t host_isattyfn(ARMCPU *cpu, GuestFD *gf)
     return isatty(gf->hostfd);
 }
 
+static uint32_t host_seekfn(ARMCPU *cpu, GuestFD *gf, target_ulong offset)
+{
+    CPUARMState *env = &cpu->env;
+    uint32_t ret = set_swi_errno(env, lseek(gf->hostfd, offset, SEEK_SET));
+    if (ret == (uint32_t)-1) {
+        return -1;
+    }
+    return 0;
+}
+
 static uint32_t gdb_closefn(ARMCPU *cpu, GuestFD *gf)
 {
     return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
@@ -468,11 +480,18 @@ static uint32_t gdb_isattyfn(ARMCPU *cpu, GuestFD *gf)
     return arm_gdb_syscall(cpu, arm_semi_cb, "isatty,%x", gf->hostfd);
 }
 
+static uint32_t gdb_seekfn(ARMCPU *cpu, GuestFD *gf, target_ulong offset)
+{
+    return arm_gdb_syscall(cpu, arm_semi_cb, "lseek,%x,%x,0",
+                           gf->hostfd, offset);
+}
+
 typedef struct GuestFDFunctions {
     sys_closefn *closefn;
     sys_writefn *writefn;
     sys_readfn *readfn;
     sys_isattyfn *isattyfn;
+    sys_seekfn *seekfn;
 } GuestFDFunctions;
 
 static const GuestFDFunctions guestfd_fns[] = {
@@ -481,12 +500,14 @@ static const GuestFDFunctions guestfd_fns[] = {
         .writefn = host_writefn,
         .readfn = host_readfn,
         .isattyfn = host_isattyfn,
+        .seekfn = host_seekfn,
     },
     [GuestFDGDB] = {
         .closefn = gdb_closefn,
         .writefn = gdb_writefn,
         .readfn = gdb_readfn,
         .isattyfn = gdb_isattyfn,
+        .seekfn = gdb_seekfn,
     },
 };
 
@@ -656,15 +677,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
             return set_swi_errno(env, -1);
         }
 
-        if (use_gdb_syscalls()) {
-            return arm_gdb_syscall(cpu, arm_semi_cb, "lseek,%x,%x,0",
-                                   gf->hostfd, arg1);
-        } else {
-            ret = set_swi_errno(env, lseek(gf->hostfd, arg1, SEEK_SET));
-            if (ret == (uint32_t)-1)
-              return -1;
-            return 0;
-        }
+        return guestfd_fns[gf->type].seekfn(cpu, gf, arg1);
     case TARGET_SYS_FLEN:
         GET_ARG(0);
 
-- 
2.20.1

[Qemu-devel] [PATCH v2 12/15] target/arm/arm-semi: Factor out implementation of SYS_FLEN

[Peter Maydell]

Factor out the implementation of SYS_FLEN via the new
function tables.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/arm-semi.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index b5e1d73eb80..87c911f0187 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -391,6 +391,7 @@ typedef uint32_t sys_readfn(ARMCPU *cpu, GuestFD *gf,
 typedef uint32_t sys_isattyfn(ARMCPU *cpu, GuestFD *gf);
 typedef uint32_t sys_seekfn(ARMCPU *cpu, GuestFD *gf,
                             target_ulong offset);
+typedef uint32_t sys_flenfn(ARMCPU *cpu, GuestFD *gf);
 
 static uint32_t host_closefn(ARMCPU *cpu, GuestFD *gf)
 {
@@ -454,6 +455,17 @@ static uint32_t host_seekfn(ARMCPU *cpu, GuestFD *gf, target_ulong offset)
     return 0;
 }
 
+static uint32_t host_flenfn(ARMCPU *cpu, GuestFD *gf)
+{
+    CPUARMState *env = &cpu->env;
+    struct stat buf;
+    uint32_t ret = set_swi_errno(env, fstat(gf->hostfd, &buf));
+    if (ret == (uint32_t)-1) {
+        return -1;
+    }
+    return buf.st_size;
+}
+
 static uint32_t gdb_closefn(ARMCPU *cpu, GuestFD *gf)
 {
     return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
@@ -486,12 +498,19 @@ static uint32_t gdb_seekfn(ARMCPU *cpu, GuestFD *gf, target_ulong offset)
                            gf->hostfd, offset);
 }
 
+static uint32_t gdb_flenfn(ARMCPU *cpu, GuestFD *gf)
+{
+    return arm_gdb_syscall(cpu, arm_semi_flen_cb, "fstat,%x,%x",
+                           gf->hostfd, arm_flen_buf(cpu));
+}
+
 typedef struct GuestFDFunctions {
     sys_closefn *closefn;
     sys_writefn *writefn;
     sys_readfn *readfn;
     sys_isattyfn *isattyfn;
     sys_seekfn *seekfn;
+    sys_flenfn *flenfn;
 } GuestFDFunctions;
 
 static const GuestFDFunctions guestfd_fns[] = {
@@ -501,6 +520,7 @@ static const GuestFDFunctions guestfd_fns[] = {
         .readfn = host_readfn,
         .isattyfn = host_isattyfn,
         .seekfn = host_seekfn,
+        .flenfn = host_flenfn,
     },
     [GuestFDGDB] = {
         .closefn = gdb_closefn,
@@ -508,6 +528,7 @@ static const GuestFDFunctions guestfd_fns[] = {
         .readfn = gdb_readfn,
         .isattyfn = gdb_isattyfn,
         .seekfn = gdb_seekfn,
+        .flenfn = gdb_flenfn,
     },
 };
 
@@ -687,16 +708,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
             return set_swi_errno(env, -1);
         }
 
-        if (use_gdb_syscalls()) {
-            return arm_gdb_syscall(cpu, arm_semi_flen_cb, "fstat,%x,%x",
-                                   gf->hostfd, arm_flen_buf(cpu));
-        } else {
-            struct stat buf;
-            ret = set_swi_errno(env, fstat(gf->hostfd, &buf));
-            if (ret == (uint32_t)-1)
-                return -1;
-            return buf.st_size;
-        }
+        return guestfd_fns[gf->type].flenfn(cpu, gf);
     case TARGET_SYS_TMPNAM:
         qemu_log_mask(LOG_UNIMP, "%s: SYS_TMPNAM not implemented", __func__);
         return -1;
-- 
2.20.1

[Qemu-devel] [PATCH v2 13/15] target/arm/arm-semi: Implement support for semihosting feature detection

[Peter Maydell]

Version 2.0 of the semihosting specification added support for
allowing a guest to detect whether the implementation supported
particular features. This works by the guest opening a magic
file ":semihosting-features", which contains a fixed set of
data with some magic numbers followed by a sequence of bytes
with feature flags. The file is expected to behave sensibly
for the various semihosting calls which operate on files
(SYS_FLEN, SYS_SEEK, etc).

Implement this as another kind of guest FD using our function
table dispatch mechanism. Initially we report no extended
features, so we have just one feature flag byte which is zero.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/arm-semi.c | 109 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 108 insertions(+), 1 deletion(-)

diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index 87c911f0187..57491740d73 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -110,6 +110,7 @@ typedef enum GuestFDType {
     GuestFDUnused = 0,
     GuestFDHost = 1,
     GuestFDGDB = 2,
+    GuestFDFeatureFile = 3,
 } GuestFDType;
 
 /*
@@ -118,7 +119,10 @@ typedef enum GuestFDType {
  */
 typedef struct GuestFD {
     GuestFDType type;
-    int hostfd;
+    union {
+        int hostfd;
+        target_ulong featurefile_offset;
+    };
 } GuestFD;
 
 static GArray *guestfd_array;
@@ -504,6 +508,89 @@ static uint32_t gdb_flenfn(ARMCPU *cpu, GuestFD *gf)
                            gf->hostfd, arm_flen_buf(cpu));
 }
 
+#define SHFB_MAGIC_0 0x53
+#define SHFB_MAGIC_1 0x48
+#define SHFB_MAGIC_2 0x46
+#define SHFB_MAGIC_3 0x42
+
+static const uint8_t featurefile_data[] = {
+    SHFB_MAGIC_0,
+    SHFB_MAGIC_1,
+    SHFB_MAGIC_2,
+    SHFB_MAGIC_3,
+    0, /* Feature byte 0 */
+};
+
+static void init_featurefile_guestfd(int guestfd)
+{
+    GuestFD *gf = do_get_guestfd(guestfd);
+
+    assert(gf);
+    gf->type = GuestFDFeatureFile;
+    gf->featurefile_offset = 0;
+}
+
+static uint32_t featurefile_closefn(ARMCPU *cpu, GuestFD *gf)
+{
+    /* Nothing to do */
+    return 0;
+}
+
+static uint32_t featurefile_writefn(ARMCPU *cpu, GuestFD *gf,
+                                    target_ulong buf, uint32_t len)
+{
+    /* This fd can never be open for writing */
+    CPUARMState *env = &cpu->env;
+
+    errno = EBADF;
+    return set_swi_errno(env, -1);
+}
+
+static uint32_t featurefile_readfn(ARMCPU *cpu, GuestFD *gf,
+                                   target_ulong buf, uint32_t len)
+{
+    uint32_t i;
+#ifndef CONFIG_USER_ONLY
+    CPUARMState *env = &cpu->env;
+#endif
+    char *s;
+
+    s = lock_user(VERIFY_WRITE, buf, len, 0);
+    if (!s) {
+        return len;
+    }
+
+    for (i = 0; i < len; i++) {
+        if (gf->featurefile_offset >= sizeof(featurefile_data)) {
+            break;
+        }
+        s[i] = featurefile_data[gf->featurefile_offset];
+        gf->featurefile_offset++;
+    }
+
+    unlock_user(s, buf, len);
+
+    /* Return number of bytes not read */
+    return len - i;
+}
+
+static uint32_t featurefile_isattyfn(ARMCPU *cpu, GuestFD *gf)
+{
+    return 0;
+}
+
+static uint32_t featurefile_seekfn(ARMCPU *cpu, GuestFD *gf,
+                                   target_ulong offset)
+{
+    gf->featurefile_offset = offset;
+    return 0;
+}
+
+static uint32_t featurefile_flenfn(ARMCPU *cpu, GuestFD *gf)
+{
+    return sizeof(featurefile_data);
+}
+
 typedef struct GuestFDFunctions {
     sys_closefn *closefn;
     sys_writefn *writefn;
@@ -530,6 +617,14 @@ static const GuestFDFunctions guestfd_fns[] = {
         .seekfn = gdb_seekfn,
         .flenfn = gdb_flenfn,
     },
+    [GuestFDFeatureFile] = {
+        .closefn = featurefile_closefn,
+        .writefn = featurefile_writefn,
+        .readfn = featurefile_readfn,
+        .isattyfn = featurefile_isattyfn,
+        .seekfn = featurefile_seekfn,
+        .flenfn = featurefile_flenfn,
+    },
 };
 
 /* Read the input value from the argument block; fail the semihosting
@@ -616,6 +711,18 @@ target_ulong do_arm_semihosting(CPUARMState *env)
             unlock_user(s, arg0, 0);
             return guestfd;
         }
+        if (strcmp(s, ":semihosting-features") == 0) {
+            unlock_user(s, arg0, 0);
+            /* We must fail opens for modes other than 0 ('r') or 1 ('rb') */
+            if (arg1 != 0 && arg1 != 1) {
+                dealloc_guestfd(guestfd);
+                errno = EACCES;
+                return set_swi_errno(env, -1);
+            }
+            init_featurefile_guestfd(guestfd);
+            return guestfd;
+        }
+
         if (use_gdb_syscalls()) {
             arm_semi_open_guestfd = guestfd;
             ret = arm_gdb_syscall(cpu, arm_semi_open_cb, "open,%s,%x,1a4", arg0,
-- 
2.20.1

[Qemu-devel] [PATCH v2 14/15] target/arm/arm-semi: Implement SH_EXT_EXIT_EXTENDED extension

[Peter Maydell]

SH_EXT_EXIT_EXTENDED is a v2.0 semihosting extension: it
indicates that the implementation supports the SYS_EXIT_EXTENDED
function. This function allows both A64 and A32/T32 guests to
exit with a specified exit status, unlike the older SYS_EXIT
function which only allowed this for A64 guests. Implement
this extension.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/arm-semi.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index 57491740d73..f65d8c907e8 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -59,6 +59,7 @@
 #define TARGET_SYS_HEAPINFO    0x16
 #define TARGET_SYS_EXIT        0x18
 #define TARGET_SYS_SYNCCACHE   0x19
+#define TARGET_SYS_EXIT_EXTENDED 0x20
 
 /* ADP_Stopped_ApplicationExit is used for exit(0),
  * anything else is implemented as exit(1) */
@@ -513,12 +514,15 @@ static uint32_t gdb_flenfn(ARMCPU *cpu, GuestFD *gf)
 #define SHFB_MAGIC_2 0x46
 #define SHFB_MAGIC_3 0x42
 
+/* Feature bits reportable in feature byte 0 */
+#define SH_EXT_EXIT_EXTENDED (1 << 0)
+
 static const uint8_t featurefile_data[] = {
     SHFB_MAGIC_0,
     SHFB_MAGIC_1,
     SHFB_MAGIC_2,
     SHFB_MAGIC_3,
-    0, /* Feature byte 0 */
+    SH_EXT_EXIT_EXTENDED, /* Feature byte 0 */
 };
 
 static void init_featurefile_guestfd(int guestfd)
@@ -1042,11 +1046,14 @@ target_ulong do_arm_semihosting(CPUARMState *env)
             return 0;
         }
     case TARGET_SYS_EXIT:
-        if (is_a64(env)) {
+    case TARGET_SYS_EXIT_EXTENDED:
+        if (nr == TARGET_SYS_EXIT_EXTENDED || is_a64(env)) {
             /*
-             * The A64 version of this call takes a parameter block,
+             * The A64 version of SYS_EXIT takes a parameter block,
              * so the application-exit type can return a subcode which
              * is the exit status code from the application.
+             * SYS_EXIT_EXTENDED is an a new-in-v2.0 optional function
+             * which allows A32/T32 guests to also provide a status code.
              */
             GET_ARG(0);
             GET_ARG(1);
@@ -1058,8 +1065,10 @@ target_ulong do_arm_semihosting(CPUARMState *env)
             }
         } else {
             /*
-             * ARM specifies only Stopped_ApplicationExit as normal
-             * exit, everything else is considered an error
+             * The A32/T32 version of SYS_EXIT specifies only
+             * Stopped_ApplicationExit as normal exit, but does not
+             * allow the guest to specify the exit status code.
+             * Everything else is considered an error.
              */
             ret = (args == ADP_Stopped_ApplicationExit) ? 0 : 1;
         }
-- 
2.20.1

[Qemu-devel] [PATCH v2 15/15] target/arm/arm-semi: Implement SH_EXT_STDOUT_STDERR extension

[Peter Maydell]

SH_EXT_STDOUT_STDERR is a v2.0 semihosting extension: the guest
can open ":tt" with a file mode requesting append access in
order to open stderr, in addition to the existing "open for
read for stdin or write for stdout". Implement this and
report it via the :semihosting-features data.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/arm-semi.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index f65d8c907e8..6f7b6d801bf 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -516,13 +516,14 @@ static uint32_t gdb_flenfn(ARMCPU *cpu, GuestFD *gf)
 
 /* Feature bits reportable in feature byte 0 */
 #define SH_EXT_EXIT_EXTENDED (1 << 0)
+#define SH_EXT_STDOUT_STDERR (1 << 1)
 
 static const uint8_t featurefile_data[] = {
     SHFB_MAGIC_0,
     SHFB_MAGIC_1,
     SHFB_MAGIC_2,
     SHFB_MAGIC_3,
-    SH_EXT_EXIT_EXTENDED, /* Feature byte 0 */
+    SH_EXT_EXIT_EXTENDED | SH_EXT_STDOUT_STDERR, /* Feature byte 0 */
 };
 
 static void init_featurefile_guestfd(int guestfd)
@@ -710,7 +711,21 @@ target_ulong do_arm_semihosting(CPUARMState *env)
         }
 
         if (strcmp(s, ":tt") == 0) {
-            int result_fileno = arg1 < 4 ? STDIN_FILENO : STDOUT_FILENO;
+            int result_fileno;
+
+            /*
+             * We implement SH_EXT_STDOUT_STDERR, so:
+             *  open for read == stdin
+             *  open for write == stdout
+             *  open for append == stderr
+             */
+            if (arg1 < 4) {
+                result_fileno = STDIN_FILENO;
+            } else if (arg1 < 8) {
+                result_fileno = STDOUT_FILENO;
+            } else {
+                result_fileno = STDERR_FILENO;
+            }
             associate_guestfd(guestfd, result_fileno);
             unlock_user(s, arg0, 0);
             return guestfd;
-- 
2.20.1

Re: [PATCH v2 00/15] target/arm: Implement semihosting v2.0

[Peter Maydell]

Ping for code review, please?

thanks
-- PMM

On Mon, 16 Sep 2019 at 15:15, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This patchset implements support in QEMU for v2.0 of the
> Arm semihosting specification:
>  https://developer.arm.com/docs/100863/latest/preface
>
> Specifically, v2.0 has:
>  * a mechanism for detection of optional extra features,
>    which works by allowing the guest to open a magic file
>    named ":semihosting-features" and read some feature
>    flags from it
>  * two defined extensions:
>   - STDOUT_STDERR lets the guest separately open stdout and
>     stderr via the ":tt" magic filename (v1.0 only allowed
>     access to stdout)
>   - EXIT_EXTENDED lets A32/T32 guests exit with a specified
>     exit status (otherwise only available to A64 guests).
>     This is something that people have been complaining
>     about for a long time.
>
> (Technically some of the things we already support, like
> having an A64 semihosting interface at all, are also part of
> the v2.0 spec.)
>
> This patchset:
>  * fixes some bugs relating to errnos in some cases
>  * makes semihosting hand out its own filedescriptors rather
>    than just passing out host fd numbers
>  * abstracts out the fd-related semihosting calls so they
>    indirect via a function table based on the type of the fd
>  * adds a new type of fd representing the magic file
>    ":semihosting-features" which is used for feature-detection
>  * implements both of the extensions defined by the v2.0 spec
>
> I've tested this by improving my semihosting test suite:
>  https://git.linaro.org/people/peter.maydell/semihosting-tests.git/
> (if people have other guest binaries that make much use of
> semihosting then testing would certainly be welcome.)
>
> Changes v1->v2:
>  * Added a patch which corrects misunderstanding in a FIXME
>    comment about the when the callback function is called
>    for arm_gdb_syscall()
>  * in patch 4, if the SYS_open is going via the gdbstub, we
>    must do the associate_guestfd() work in the gdbstub callback
>    function. This is because in softmmu mode the callback will
>    not be called until after do_arm_semihosting() returns.
>    (The v1 series effectively broke SYS_open in the gdbstub
>    + softmmu config)
>  * Pass CPUARMState* to set_swi_errno(), rather than creating
>    an odd local-to-this-file typedef of TaskState for the
>    softmmu compilation
>  * New patch: avoid ifdeffery in gdb callback fns by
>    using set_swi_errno() rather than doing it by-hand
>  * The various 'factor out SYS_foo' patches are basically
>    unchanged, but all the functions no longer need to take
>    a TaskState*. This seemed kind of borderline as to whether
>    to retain Alex's reviewed-by tags, so I dropped them.
>  * Since we need 'env' for set_swi_errno(), we don't need
>    to put the variable declaration inside ifdefs any more
>    in the host_readfn() etc.
>
> I do plan to have a go at fixing the odd FIXME surrounding
> arm_gdb_syscall() which patch 3 clarifies/states in a comment.
> But I thought it better to not tangle that up with this
> patchset, which is already pretty long.
>
> thanks
> -- PMM
>
>
> Peter Maydell (15):
>   target/arm/arm-semi: Capture errno in softmmu version of
>     set_swi_errno()
>   target/arm/arm-semi: Always set some kind of errno for failed calls
>   target/arm/arm-semi: Correct comment about gdb syscall races
>   target/arm/arm-semi: Make semihosting code hand out its own file
>     descriptors
>   target/arm/arm-semi: Restrict use of TaskState*
>   target/arm/arm-semi: Use set_swi_errno() in gdbstub callback functions
>   target/arm/arm-semi: Factor out implementation of SYS_CLOSE
>   target/arm/arm-semi: Factor out implementation of SYS_WRITE
>   target/arm/arm-semi: Factor out implementation of SYS_READ
>   target/arm/arm-semi: Factor out implementation of SYS_ISTTY
>   target/arm/arm-semi: Factor out implementation of SYS_SEEK
>   target/arm/arm-semi: Factor out implementation of SYS_FLEN
>   target/arm/arm-semi: Implement support for semihosting feature
>     detection
>   target/arm/arm-semi: Implement SH_EXT_EXIT_EXTENDED extension
>   target/arm/arm-semi: Implement SH_EXT_STDOUT_STDERR extension
>
>  target/arm/arm-semi.c | 707 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 577 insertions(+), 130 deletions(-)
>
> --
> 2.20.1

Re: [Qemu-devel] [PATCH v2 01/15] target/arm/arm-semi: Capture errno in softmmu version of set_swi_errno()

[Philippe Mathieu-Daudé]

On 9/16/19 4:15 PM, Peter Maydell wrote:
> 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().)
> 

Fixes: 8e71621f784
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 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;
> +
>   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);
> 

Re: [Qemu-devel] [PATCH v2 02/15] target/arm/arm-semi: Always set some kind of errno for failed calls

[Philippe Mathieu-Daudé]

On 9/16/19 4:15 PM, Peter Maydell wrote:
> 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>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   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);               \
>           }                                               \
>       } 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;
> 

Re: [Qemu-devel] [PATCH v2 06/15] target/arm/arm-semi: Use set_swi_errno() in gdbstub callback functions

[Philippe Mathieu-Daudé]

On 9/16/19 4:15 PM, Peter Maydell wrote:
> When we are routing semihosting operations through the gdbstub, the
> work of sorting out the return value and setting errno if necessary
> is done by callback functions which are invoked by the gdbstub code.
> Clean up some ifdeffery in those functions by having them call
> set_swi_errno() to set the semihosting errno.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   target/arm/arm-semi.c | 27 ++++++---------------------
>   1 file changed, 6 insertions(+), 21 deletions(-)
> 
> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index 2618588076f..02cd673d47d 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -259,17 +259,11 @@ static void arm_semi_cb(CPUState *cs, target_ulong ret, target_ulong err)
>   {
>       ARMCPU *cpu = ARM_CPU(cs);
>       CPUARMState *env = &cpu->env;
> -#ifdef CONFIG_USER_ONLY
> -    TaskState *ts = cs->opaque;
> -#endif
>       target_ulong reg0 = is_a64(env) ? env->xregs[0] : env->regs[0];
>   
>       if (ret == (target_ulong)-1) {
> -#ifdef CONFIG_USER_ONLY
> -        ts->swi_errno = err;
> -#else
> -        syscall_err = err;
> -#endif
> +        errno = err;
> +        set_swi_errno(env, -1);
>           reg0 = ret;
>       } else {
>           /* Fixup syscalls that use nonstardard return conventions.  */
> @@ -326,11 +320,8 @@ static void arm_semi_flen_cb(CPUState *cs, target_ulong ret, target_ulong err)
>       } else {
>           env->regs[0] = size;
>       }
> -#ifdef CONFIG_USER_ONLY
> -    ((TaskState *)cs->opaque)->swi_errno = err;
> -#else
> -    syscall_err = err;
> -#endif
> +    errno = err;
> +    set_swi_errno(env, -1);
>   }
>   
>   static int arm_semi_open_guestfd;
> @@ -339,15 +330,9 @@ static void arm_semi_open_cb(CPUState *cs, target_ulong ret, target_ulong err)
>   {
>       ARMCPU *cpu = ARM_CPU(cs);
>       CPUARMState *env = &cpu->env;
> -#ifdef CONFIG_USER_ONLY
> -    TaskState *ts = cs->opaque;
> -#endif
>       if (ret == (target_ulong)-1) {
> -#ifdef CONFIG_USER_ONLY
> -        ts->swi_errno = err;
> -#else
> -        syscall_err = err;
> -#endif
> +        errno = err;
> +        set_swi_errno(env, -1);
>           dealloc_guestfd(arm_semi_open_guestfd);
>       } else {
>           associate_guestfd(arm_semi_open_guestfd, ret);
> 

Re: [Qemu-devel] [PATCH v2 07/15] target/arm/arm-semi: Factor out implementation of SYS_CLOSE

[Philippe Mathieu-Daudé]

On 9/16/19 4:15 PM, Peter Maydell wrote:
> Currently for the semihosting calls which take a file descriptor
> (SYS_CLOSE, SYS_WRITE, SYS_READ, SYS_ISTTY, SYS_SEEK, SYS_FLEN)
> we have effectively two implementations, one for real host files
> and one for when we indirect via the gdbstub. We want to add a
> third one to deal with the magic :semihosting-features file.
> 
> Instead of having a three-way if statement in each of these
> cases, factor out the implementation of the calls to separate
> functions which we dispatch to via function pointers selected
> via the GuestFDType for the guest fd.
> 
> In this commit, we set up the framework for the dispatch,
> and convert the SYS_CLOSE call to use it.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   target/arm/arm-semi.c | 44 ++++++++++++++++++++++++++++++++++++-------
>   1 file changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index 02cd673d47d..e5f1e2aaaf2 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -109,6 +109,7 @@ static int open_modeflags[12] = {
>   typedef enum GuestFDType {
>       GuestFDUnused = 0,
>       GuestFDHost = 1,
> +    GuestFDGDB = 2,
>   } GuestFDType;
>   
>   /*
> @@ -172,14 +173,14 @@ static GuestFD *do_get_guestfd(int guestfd)
>   /*
>    * Associate the specified guest fd (which must have been
>    * allocated via alloc_fd() and not previously used) with
> - * the specified host fd.
> + * the specified host/gdb fd.
>    */
>   static void associate_guestfd(int guestfd, int hostfd)
>   {
>       GuestFD *gf = do_get_guestfd(guestfd);
>   
>       assert(gf);
> -    gf->type = GuestFDHost;
> +    gf->type = use_gdb_syscalls() ? GuestFDGDB : GuestFDHost;
>       gf->hostfd = hostfd;
>   }
>   
> @@ -376,6 +377,39 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb,
>       return is_a64(env) ? env->xregs[0] : env->regs[0];
>   }
>   
> +/*
> + * Types for functions implementing various semihosting calls
> + * for specific types of guest file descriptor. These must all
> + * do the work and return the required return value for the guest,
> + * setting the guest errno if appropriate.
> + */
> +typedef uint32_t sys_closefn(ARMCPU *cpu, GuestFD *gf);
> +
> +static uint32_t host_closefn(ARMCPU *cpu, GuestFD *gf)
> +{
> +    CPUARMState *env = &cpu->env;
> +
> +    return set_swi_errno(env, close(gf->hostfd));
> +}
> +
> +static uint32_t gdb_closefn(ARMCPU *cpu, GuestFD *gf)
> +{
> +    return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
> +}
> +
> +typedef struct GuestFDFunctions {
> +    sys_closefn *closefn;
> +} GuestFDFunctions;
> +
> +static const GuestFDFunctions guestfd_fns[] = {
> +    [GuestFDHost] = {
> +        .closefn = host_closefn,
> +    },
> +    [GuestFDGDB] = {
> +        .closefn = gdb_closefn,
> +    },
> +};
> +
>   /* Read the input value from the argument block; fail the semihosting
>    * call if the memory read fails.
>    */
> @@ -485,11 +519,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
>               return set_swi_errno(env, -1);
>           }
>   
> -        if (use_gdb_syscalls()) {
> -            ret = arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
> -        } else {
> -            ret = set_swi_errno(env, close(gf->hostfd));
> -        }
> +        ret = guestfd_fns[gf->type].closefn(cpu, gf);
>           dealloc_guestfd(arg0);
>           return ret;
>       case TARGET_SYS_WRITEC:
> 

Re: [Qemu-devel] [PATCH v2 08/15] target/arm/arm-semi: Factor out implementation of SYS_WRITE

[Philippe Mathieu-Daudé]

On 9/16/19 4:15 PM, Peter Maydell wrote:
> Factor out the implementation of SYS_WRITE via the
> new function tables.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   target/arm/arm-semi.c | 51 ++++++++++++++++++++++++++++---------------
>   1 file changed, 33 insertions(+), 18 deletions(-)
> 
> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index e5f1e2aaaf2..c21cbb97bc1 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -384,6 +384,8 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb,
>    * setting the guest errno if appropriate.
>    */
>   typedef uint32_t sys_closefn(ARMCPU *cpu, GuestFD *gf);
> +typedef uint32_t sys_writefn(ARMCPU *cpu, GuestFD *gf,
> +                             target_ulong buf, uint32_t len);
>   
>   static uint32_t host_closefn(ARMCPU *cpu, GuestFD *gf)
>   {
> @@ -392,21 +394,51 @@ static uint32_t host_closefn(ARMCPU *cpu, GuestFD *gf)
>       return set_swi_errno(env, close(gf->hostfd));
>   }
>   
> +static uint32_t host_writefn(ARMCPU *cpu, GuestFD *gf,
> +                             target_ulong buf, uint32_t len)
> +{
> +    uint32_t ret;
> +    CPUARMState *env = &cpu->env;
> +    char *s = lock_user(VERIFY_READ, buf, len, 1);
> +    if (!s) {
> +        /* Return bytes not written on error */
> +        return len;
> +    }
> +    ret = set_swi_errno(env, write(gf->hostfd, s, len));
> +    unlock_user(s, buf, 0);
> +    if (ret == (uint32_t)-1) {
> +        ret = 0;
> +    }
> +    /* Return bytes not written */
> +    return len - ret;
> +}
> +
>   static uint32_t gdb_closefn(ARMCPU *cpu, GuestFD *gf)
>   {
>       return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
>   }
>   
> +static uint32_t gdb_writefn(ARMCPU *cpu, GuestFD *gf,
> +                            target_ulong buf, uint32_t len)
> +{
> +    arm_semi_syscall_len = len;
> +    return arm_gdb_syscall(cpu, arm_semi_cb, "write,%x,%x,%x",
> +                           gf->hostfd, buf, len);
> +}
> +
>   typedef struct GuestFDFunctions {
>       sys_closefn *closefn;
> +    sys_writefn *writefn;
>   } GuestFDFunctions;
>   
>   static const GuestFDFunctions guestfd_fns[] = {
>       [GuestFDHost] = {
>           .closefn = host_closefn,
> +        .writefn = host_writefn,
>       },
>       [GuestFDGDB] = {
>           .closefn = gdb_closefn,
> +        .writefn = gdb_writefn,
>       },
>   };
>   
> @@ -539,24 +571,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
>               return set_swi_errno(env, -1);
>           }
>   
> -        if (use_gdb_syscalls()) {
> -            arm_semi_syscall_len = len;
> -            return arm_gdb_syscall(cpu, arm_semi_cb, "write,%x,%x,%x",
> -                                   gf->hostfd, arg1, len);
> -        } else {
> -            s = lock_user(VERIFY_READ, arg1, len, 1);
> -            if (!s) {
> -                /* Return bytes not written on error */
> -                return len;
> -            }
> -            ret = set_swi_errno(env, write(gf->hostfd, s, len));
> -            unlock_user(s, arg1, 0);
> -            if (ret == (uint32_t)-1) {
> -                ret = 0;
> -            }
> -            /* Return bytes not written */
> -            return len - ret;
> -        }
> +        return guestfd_fns[gf->type].writefn(cpu, gf, arg1, len);
>       case TARGET_SYS_READ:
>           GET_ARG(0);
>           GET_ARG(1);
> 

Re: [Qemu-devel] [PATCH v2 09/15] target/arm/arm-semi: Factor out implementation of SYS_READ

[Philippe Mathieu-Daudé]

On 9/16/19 4:15 PM, Peter Maydell wrote:
> Factor out the implementation of SYS_READ via the
> new function tables.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   target/arm/arm-semi.c | 55 +++++++++++++++++++++++++++----------------
>   1 file changed, 35 insertions(+), 20 deletions(-)
> 
> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index c21cbb97bc1..958083a105c 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -386,6 +386,8 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb,
>   typedef uint32_t sys_closefn(ARMCPU *cpu, GuestFD *gf);
>   typedef uint32_t sys_writefn(ARMCPU *cpu, GuestFD *gf,
>                                target_ulong buf, uint32_t len);
> +typedef uint32_t sys_readfn(ARMCPU *cpu, GuestFD *gf,
> +                            target_ulong buf, uint32_t len);
>   
>   static uint32_t host_closefn(ARMCPU *cpu, GuestFD *gf)
>   {
> @@ -413,6 +415,27 @@ static uint32_t host_writefn(ARMCPU *cpu, GuestFD *gf,
>       return len - ret;
>   }
>   
> +static uint32_t host_readfn(ARMCPU *cpu, GuestFD *gf,
> +                            target_ulong buf, uint32_t len)
> +{
> +    uint32_t ret;
> +    CPUARMState *env = &cpu->env;
> +    char *s = lock_user(VERIFY_WRITE, buf, len, 0);
> +    if (!s) {
> +        /* return bytes not read */
> +        return len;
> +    }
> +    do {
> +        ret = set_swi_errno(env, read(gf->hostfd, s, len));
> +    } while (ret == -1 && errno == EINTR);
> +    unlock_user(s, buf, len);
> +    if (ret == (uint32_t)-1) {
> +        ret = 0;
> +    }
> +    /* Return bytes not read */
> +    return len - ret;
> +}
> +
>   static uint32_t gdb_closefn(ARMCPU *cpu, GuestFD *gf)
>   {
>       return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
> @@ -426,19 +449,30 @@ static uint32_t gdb_writefn(ARMCPU *cpu, GuestFD *gf,
>                              gf->hostfd, buf, len);
>   }
>   
> +static uint32_t gdb_readfn(ARMCPU *cpu, GuestFD *gf,
> +                           target_ulong buf, uint32_t len)
> +{
> +    arm_semi_syscall_len = len;
> +    return arm_gdb_syscall(cpu, arm_semi_cb, "read,%x,%x,%x",
> +                           gf->hostfd, buf, len);
> +}
> +
>   typedef struct GuestFDFunctions {
>       sys_closefn *closefn;
>       sys_writefn *writefn;
> +    sys_readfn *readfn;
>   } GuestFDFunctions;
>   
>   static const GuestFDFunctions guestfd_fns[] = {
>       [GuestFDHost] = {
>           .closefn = host_closefn,
>           .writefn = host_writefn,
> +        .readfn = host_readfn,
>       },
>       [GuestFDGDB] = {
>           .closefn = gdb_closefn,
>           .writefn = gdb_writefn,
> +        .readfn = gdb_readfn,
>       },
>   };
>   
> @@ -584,26 +618,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
>               return set_swi_errno(env, -1);
>           }
>   
> -        if (use_gdb_syscalls()) {
> -            arm_semi_syscall_len = len;
> -            return arm_gdb_syscall(cpu, arm_semi_cb, "read,%x,%x,%x",
> -                                   gf->hostfd, arg1, len);
> -        } else {
> -            s = lock_user(VERIFY_WRITE, arg1, len, 0);
> -            if (!s) {
> -                /* return bytes not read */
> -                return len;
> -            }
> -            do {
> -                ret = set_swi_errno(env, read(gf->hostfd, s, len));
> -            } while (ret == -1 && errno == EINTR);
> -            unlock_user(s, arg1, len);
> -            if (ret == (uint32_t)-1) {
> -                ret = 0;
> -            }
> -            /* Return bytes not read */
> -            return len - ret;
> -        }
> +        return guestfd_fns[gf->type].readfn(cpu, gf, arg1, len);
>       case TARGET_SYS_READC:
>           qemu_log_mask(LOG_UNIMP, "%s: SYS_READC not implemented", __func__);
>           return 0;
> 

Re: [Qemu-devel] [PATCH v2 10/15] target/arm/arm-semi: Factor out implementation of SYS_ISTTY

[Philippe Mathieu-Daudé]

On 9/16/19 4:15 PM, Peter Maydell wrote:
> Factor out the implementation of SYS_ISTTY via the new function
> tables.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   target/arm/arm-semi.c | 20 +++++++++++++++-----
>   1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index 958083a105c..ecd51338fd3 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -388,6 +388,7 @@ typedef uint32_t sys_writefn(ARMCPU *cpu, GuestFD *gf,
>                                target_ulong buf, uint32_t len);
>   typedef uint32_t sys_readfn(ARMCPU *cpu, GuestFD *gf,
>                               target_ulong buf, uint32_t len);
> +typedef uint32_t sys_isattyfn(ARMCPU *cpu, GuestFD *gf);
>   
>   static uint32_t host_closefn(ARMCPU *cpu, GuestFD *gf)
>   {
> @@ -436,6 +437,11 @@ static uint32_t host_readfn(ARMCPU *cpu, GuestFD *gf,
>       return len - ret;
>   }
>   
> +static uint32_t host_isattyfn(ARMCPU *cpu, GuestFD *gf)
> +{
> +    return isatty(gf->hostfd);
> +}
> +
>   static uint32_t gdb_closefn(ARMCPU *cpu, GuestFD *gf)
>   {
>       return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
> @@ -457,10 +463,16 @@ static uint32_t gdb_readfn(ARMCPU *cpu, GuestFD *gf,
>                              gf->hostfd, buf, len);
>   }
>   
> +static uint32_t gdb_isattyfn(ARMCPU *cpu, GuestFD *gf)
> +{
> +    return arm_gdb_syscall(cpu, arm_semi_cb, "isatty,%x", gf->hostfd);
> +}
> +
>   typedef struct GuestFDFunctions {
>       sys_closefn *closefn;
>       sys_writefn *writefn;
>       sys_readfn *readfn;
> +    sys_isattyfn *isattyfn;
>   } GuestFDFunctions;
>   
>   static const GuestFDFunctions guestfd_fns[] = {
> @@ -468,11 +480,13 @@ static const GuestFDFunctions guestfd_fns[] = {
>           .closefn = host_closefn,
>           .writefn = host_writefn,
>           .readfn = host_readfn,
> +        .isattyfn = host_isattyfn,
>       },
>       [GuestFDGDB] = {
>           .closefn = gdb_closefn,
>           .writefn = gdb_writefn,
>           .readfn = gdb_readfn,
> +        .isattyfn = gdb_isattyfn,
>       },
>   };
>   
> @@ -631,11 +645,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
>               return set_swi_errno(env, -1);
>           }
>   
> -        if (use_gdb_syscalls()) {
> -            return arm_gdb_syscall(cpu, arm_semi_cb, "isatty,%x", gf->hostfd);
> -        } else {
> -            return isatty(gf->hostfd);
> -        }
> +        return guestfd_fns[gf->type].isattyfn(cpu, gf);
>       case TARGET_SYS_SEEK:
>           GET_ARG(0);
>           GET_ARG(1);
> 

Re: [Qemu-devel] [PATCH v2 11/15] target/arm/arm-semi: Factor out implementation of SYS_SEEK

[Philippe Mathieu-Daudé]

On 9/16/19 4:15 PM, Peter Maydell wrote:
> Factor out the implementation of SYS_SEEK via the new function
> tables.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   target/arm/arm-semi.c | 31 ++++++++++++++++++++++---------
>   1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index ecd51338fd3..b5e1d73eb80 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -389,6 +389,8 @@ typedef uint32_t sys_writefn(ARMCPU *cpu, GuestFD *gf,
>   typedef uint32_t sys_readfn(ARMCPU *cpu, GuestFD *gf,
>                               target_ulong buf, uint32_t len);
>   typedef uint32_t sys_isattyfn(ARMCPU *cpu, GuestFD *gf);
> +typedef uint32_t sys_seekfn(ARMCPU *cpu, GuestFD *gf,
> +                            target_ulong offset);
>   
>   static uint32_t host_closefn(ARMCPU *cpu, GuestFD *gf)
>   {
> @@ -442,6 +444,16 @@ static uint32_t host_isattyfn(ARMCPU *cpu, GuestFD *gf)
>       return isatty(gf->hostfd);
>   }
>   
> +static uint32_t host_seekfn(ARMCPU *cpu, GuestFD *gf, target_ulong offset)
> +{
> +    CPUARMState *env = &cpu->env;
> +    uint32_t ret = set_swi_errno(env, lseek(gf->hostfd, offset, SEEK_SET));
> +    if (ret == (uint32_t)-1) {
> +        return -1;
> +    }
> +    return 0;
> +}
> +
>   static uint32_t gdb_closefn(ARMCPU *cpu, GuestFD *gf)
>   {
>       return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
> @@ -468,11 +480,18 @@ static uint32_t gdb_isattyfn(ARMCPU *cpu, GuestFD *gf)
>       return arm_gdb_syscall(cpu, arm_semi_cb, "isatty,%x", gf->hostfd);
>   }
>   
> +static uint32_t gdb_seekfn(ARMCPU *cpu, GuestFD *gf, target_ulong offset)
> +{
> +    return arm_gdb_syscall(cpu, arm_semi_cb, "lseek,%x,%x,0",
> +                           gf->hostfd, offset);
> +}
> +
>   typedef struct GuestFDFunctions {
>       sys_closefn *closefn;
>       sys_writefn *writefn;
>       sys_readfn *readfn;
>       sys_isattyfn *isattyfn;
> +    sys_seekfn *seekfn;
>   } GuestFDFunctions;
>   
>   static const GuestFDFunctions guestfd_fns[] = {
> @@ -481,12 +500,14 @@ static const GuestFDFunctions guestfd_fns[] = {
>           .writefn = host_writefn,
>           .readfn = host_readfn,
>           .isattyfn = host_isattyfn,
> +        .seekfn = host_seekfn,
>       },
>       [GuestFDGDB] = {
>           .closefn = gdb_closefn,
>           .writefn = gdb_writefn,
>           .readfn = gdb_readfn,
>           .isattyfn = gdb_isattyfn,
> +        .seekfn = gdb_seekfn,
>       },
>   };
>   
> @@ -656,15 +677,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
>               return set_swi_errno(env, -1);
>           }
>   
> -        if (use_gdb_syscalls()) {
> -            return arm_gdb_syscall(cpu, arm_semi_cb, "lseek,%x,%x,0",
> -                                   gf->hostfd, arg1);
> -        } else {
> -            ret = set_swi_errno(env, lseek(gf->hostfd, arg1, SEEK_SET));
> -            if (ret == (uint32_t)-1)
> -              return -1;
> -            return 0;
> -        }
> +        return guestfd_fns[gf->type].seekfn(cpu, gf, arg1);
>       case TARGET_SYS_FLEN:
>           GET_ARG(0);
>   
> 

Re: [Qemu-devel] [PATCH v2 12/15] target/arm/arm-semi: Factor out implementation of SYS_FLEN

[Philippe Mathieu-Daudé]

On 9/16/19 4:15 PM, Peter Maydell wrote:
> Factor out the implementation of SYS_FLEN via the new
> function tables.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   target/arm/arm-semi.c | 32 ++++++++++++++++++++++----------
>   1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index b5e1d73eb80..87c911f0187 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -391,6 +391,7 @@ typedef uint32_t sys_readfn(ARMCPU *cpu, GuestFD *gf,
>   typedef uint32_t sys_isattyfn(ARMCPU *cpu, GuestFD *gf);
>   typedef uint32_t sys_seekfn(ARMCPU *cpu, GuestFD *gf,
>                               target_ulong offset);
> +typedef uint32_t sys_flenfn(ARMCPU *cpu, GuestFD *gf);
>   
>   static uint32_t host_closefn(ARMCPU *cpu, GuestFD *gf)
>   {
> @@ -454,6 +455,17 @@ static uint32_t host_seekfn(ARMCPU *cpu, GuestFD *gf, target_ulong offset)
>       return 0;
>   }
>   
> +static uint32_t host_flenfn(ARMCPU *cpu, GuestFD *gf)
> +{
> +    CPUARMState *env = &cpu->env;
> +    struct stat buf;
> +    uint32_t ret = set_swi_errno(env, fstat(gf->hostfd, &buf));
> +    if (ret == (uint32_t)-1) {
> +        return -1;
> +    }
> +    return buf.st_size;
> +}
> +
>   static uint32_t gdb_closefn(ARMCPU *cpu, GuestFD *gf)
>   {
>       return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", gf->hostfd);
> @@ -486,12 +498,19 @@ static uint32_t gdb_seekfn(ARMCPU *cpu, GuestFD *gf, target_ulong offset)
>                              gf->hostfd, offset);
>   }
>   
> +static uint32_t gdb_flenfn(ARMCPU *cpu, GuestFD *gf)
> +{
> +    return arm_gdb_syscall(cpu, arm_semi_flen_cb, "fstat,%x,%x",
> +                           gf->hostfd, arm_flen_buf(cpu));
> +}
> +
>   typedef struct GuestFDFunctions {
>       sys_closefn *closefn;
>       sys_writefn *writefn;
>       sys_readfn *readfn;
>       sys_isattyfn *isattyfn;
>       sys_seekfn *seekfn;
> +    sys_flenfn *flenfn;
>   } GuestFDFunctions;
>   
>   static const GuestFDFunctions guestfd_fns[] = {
> @@ -501,6 +520,7 @@ static const GuestFDFunctions guestfd_fns[] = {
>           .readfn = host_readfn,
>           .isattyfn = host_isattyfn,
>           .seekfn = host_seekfn,
> +        .flenfn = host_flenfn,
>       },
>       [GuestFDGDB] = {
>           .closefn = gdb_closefn,
> @@ -508,6 +528,7 @@ static const GuestFDFunctions guestfd_fns[] = {
>           .readfn = gdb_readfn,
>           .isattyfn = gdb_isattyfn,
>           .seekfn = gdb_seekfn,
> +        .flenfn = gdb_flenfn,
>       },
>   };
>   
> @@ -687,16 +708,7 @@ target_ulong do_arm_semihosting(CPUARMState *env)
>               return set_swi_errno(env, -1);
>           }
>   
> -        if (use_gdb_syscalls()) {
> -            return arm_gdb_syscall(cpu, arm_semi_flen_cb, "fstat,%x,%x",
> -                                   gf->hostfd, arm_flen_buf(cpu));
> -        } else {
> -            struct stat buf;
> -            ret = set_swi_errno(env, fstat(gf->hostfd, &buf));
> -            if (ret == (uint32_t)-1)
> -                return -1;
> -            return buf.st_size;
> -        }
> +        return guestfd_fns[gf->type].flenfn(cpu, gf);
>       case TARGET_SYS_TMPNAM:
>           qemu_log_mask(LOG_UNIMP, "%s: SYS_TMPNAM not implemented", __func__);
>           return -1;
> 

Re: [Qemu-devel] [PATCH v2 01/15] target/arm/arm-semi: Capture errno in softmmu version of set_swi_errno()

[Peter Maydell]

On Fri, 4 Oct 2019 at 00:24, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 9/16/19 4:15 PM, Peter Maydell wrote:
> > 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().)
> >
>
> Fixes: 8e71621f784

That was the commit that added support for semihosting
to softmmu in 2007, so I don't think suggesting that this
is a 'fix' to that makes much sense. This has just been
broken forever. (It's also pretty unimportant as a bug because
the semihosting 'errno' is so ill-defined as to be unused,
I think.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 01/15] target/arm/arm-semi: Capture errno in softmmu version of set_swi_errno()

[Richard Henderson]

On 9/16/19 7:15 AM, Peter Maydell wrote:
> 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.
> ---

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v2 02/15] target/arm/arm-semi: Always set some kind of errno for failed calls

[Richard Henderson]

On 9/16/19 7:15 AM, Peter Maydell wrote:
> 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>
> ---
>  target/arm/arm-semi.c | 45 ++++++++++++++++++++++++++-----------------
>  1 file changed, 27 insertions(+), 18 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v2 03/15] target/arm/arm-semi: Correct comment about gdb syscall races

[Richard Henderson]

On 9/16/19 7:15 AM, Peter Maydell wrote:
> In arm_gdb_syscall() we have a comment suggesting a race
> because the syscall completion callback might not happen
> before the gdb_do_syscallv() call returns. The comment is
> correct that the callback may not happen but incorrect about
> the effects. Correct it and note the important caveat that
> callers must never do any work of any kind after return from
> arm_gdb_syscall() that depends on its return value.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I'll come back to do the cleanup later, but I preferred
> not to tangle it up with the rest of the refactoring in
> this series; I also think it's probably easier done
> afterwards rather than before.
> ---
>  target/arm/arm-semi.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v2 04/15] target/arm/arm-semi: Make semihosting code hand out its own file descriptors

[Richard Henderson]

On 9/16/19 7:15 AM, Peter Maydell wrote:
> Currently the Arm semihosting code returns the guest file descriptors
> (handles) which are simply the fd values from the host OS or the
> remote gdbstub. Part of the semihosting 2.0 specification requires
> that we implement special handling of opening a ":semihosting-features"
> filename. Guest fds which result from opening the special file
> won't correspond to host fds, so to ensure that we don't end up
> with duplicate fds we need to have QEMU code control the allocation
> of the fd values we give the guest.
> 
> Add in an abstraction layer which lets us allocate new guest FD
> values, and translate from a guest FD value back to the host one.
> This also fixes an odd hole where a semihosting guest could
> use the semihosting API to read, write or close file descriptors
> that it had never allocated but which were being used by QEMU itself.
> (This isn't a security hole, because enabling semihosting permits
> the guest to do arbitrary file access to the whole host filesystem,
> and so should only be done if the guest is completely trusted.)
> 
> Currently the only kind of guest fd is one which maps to a
> host fd, but in a following commit we will add one which maps
> to the :semihosting-features magic data.
> 
> If the guest is migrated with an open semihosting file descriptor
> then subsequent attempts to use the fd will all fail; this is
> not a change from the previous situation (where the host fd
> being used on the source end would not be re-opened on the
> destination end).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Change since v1: we mustn't treat the return value of
> arm_gdb_syscall() as being the new fd from gdb, as in
> softmmu mode it is not. So we need a custom callback for open
> that can update the guestfd association.
> ---
>  target/arm/arm-semi.c | 232 +++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 216 insertions(+), 16 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v2 05/15] target/arm/arm-semi: Restrict use of TaskState*

[Richard Henderson]

On 9/16/19 7:15 AM, Peter Maydell wrote:
> The semihosting code needs accuss to the linux-user only
> TaskState pointer so it can set the semihosting errno per-thread
> for linux-user mode. At the moment we do this by having some
> ifdefs so that we define a 'ts' local in do_arm_semihosting()
> which is either a real TaskState * or just a CPUARMState *,
> depending on which mode we're compiling for.
> 
> This is awkward if we want to refactor do_arm_semihosting()
> into other functions which might need to be passed the TaskState.
> Restrict usage of the TaskState local by:
>  * making set_swi_errno() always take the CPUARMState pointer
>    and (for the linux-user version) get TaskState from that
>  * creating a new get_swi_errno() which reads the errno
>  * having the two semihosting calls which need the TaskState
>    for other purposes (SYS_GET_CMDLINE and SYS_HEAPINFO)
>    define a variable with scope restricted to just that code
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> We use 'CPUARMState *', aka 'env', rather than the other
> options of passing the ARMCPU* or the CPUState *, purely
> because it means that the later refactoring of each SYS_*
> can pass just the CPUARMState * and incidentally avoid
> an ugly ifdef caused by the implicit use of env in the
> softmmu lock_user().
> ---
>  target/arm/arm-semi.c | 111 ++++++++++++++++++++++++------------------
>  1 file changed, 63 insertions(+), 48 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v2 06/15] target/arm/arm-semi: Use set_swi_errno() in gdbstub callback functions

[Richard Henderson]

On 9/16/19 7:15 AM, Peter Maydell wrote:
> When we are routing semihosting operations through the gdbstub, the
> work of sorting out the return value and setting errno if necessary
> is done by callback functions which are invoked by the gdbstub code.
> Clean up some ifdeffery in those functions by having them call
> set_swi_errno() to set the semihosting errno.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/arm-semi.c | 27 ++++++---------------------
>  1 file changed, 6 insertions(+), 21 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v2 07/15] target/arm/arm-semi: Factor out implementation of SYS_CLOSE

[Richard Henderson]

On 9/16/19 7:15 AM, Peter Maydell wrote:
> Currently for the semihosting calls which take a file descriptor
> (SYS_CLOSE, SYS_WRITE, SYS_READ, SYS_ISTTY, SYS_SEEK, SYS_FLEN)
> we have effectively two implementations, one for real host files
> and one for when we indirect via the gdbstub. We want to add a
> third one to deal with the magic :semihosting-features file.
> 
> Instead of having a three-way if statement in each of these
> cases, factor out the implementation of the calls to separate
> functions which we dispatch to via function pointers selected
> via the GuestFDType for the guest fd.
> 
> In this commit, we set up the framework for the dispatch,
> and convert the SYS_CLOSE call to use it.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/arm-semi.c | 44 ++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 37 insertions(+), 7 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v2 08/15] target/arm/arm-semi: Factor out implementation of SYS_WRITE

[Richard Henderson]

On 9/16/19 7:15 AM, Peter Maydell wrote:
> Factor out the implementation of SYS_WRITE via the
> new function tables.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/arm-semi.c | 51 ++++++++++++++++++++++++++++---------------
>  1 file changed, 33 insertions(+), 18 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~