diff mbox series

[v3,23/49] semihosting: Split out semihost_sys_open

Message ID 20220521000400.454525-24-richard.henderson@linaro.org (mailing list archive)
State New, archived
Headers show
Series semihosting cleanup | expand

Commit Message

Richard Henderson May 21, 2022, 12:03 a.m. UTC 
Split out the non-ARM specific portions of SYS_OPEN to a
reusable function.  This handles gdb and host file i/o.

Add helpers to validate the length of the filename string.
Prepare for usage by other semihosting by allowing the
filename length parameter to be 0, and calling strlen.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/semihosting/syscalls.h |  25 +++++
 semihosting/arm-compat-semi.c  |  50 +--------
 semihosting/guestfd.c          |   5 +
 semihosting/syscalls.c         | 186 +++++++++++++++++++++++++++++++++
 semihosting/meson.build        |   1 +
 5 files changed, 222 insertions(+), 45 deletions(-)
 create mode 100644 include/semihosting/syscalls.h
 create mode 100644 semihosting/syscalls.c

Comments

Peter Maydell May 23, 2022, 1:30 p.m. UTC | #1 
On Sat, 21 May 2022 at 01:04, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Split out the non-ARM specific portions of SYS_OPEN to a
> reusable function.  This handles gdb and host file i/o.
>
> Add helpers to validate the length of the filename string.
> Prepare for usage by other semihosting by allowing the
> filename length parameter to be 0, and calling strlen.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>


> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
> index 7a7468799f..cc008d0338 100644
> --- a/semihosting/arm-compat-semi.c
> +++ b/semihosting/arm-compat-semi.c
> @@ -35,9 +35,10 @@
>  #include "semihosting/semihost.h"
>  #include "semihosting/console.h"
>  #include "semihosting/common-semi.h"
> -#include "semihosting/guestfd.h"
>  #include "qemu/timer.h"
>  #include "exec/gdbstub.h"
> +#include "semihosting/guestfd.h"
> +#include "semihosting/syscalls.h"

Can we keep all the semihosting/ include lines together?

> diff --git a/semihosting/guestfd.c b/semihosting/guestfd.c
> index b6405f5663..7ac2e147a8 100644
> --- a/semihosting/guestfd.c
> +++ b/semihosting/guestfd.c
> @@ -11,6 +11,11 @@
>  #include "qemu/osdep.h"
>  #include "exec/gdbstub.h"
>  #include "semihosting/guestfd.h"
> +#ifdef CONFIG_USER_ONLY
> +#include "qemu.h"
> +#else
> +#include "semihosting/softmmu-uaccess.h"
> +#endif

Does this need to be in this patch, or should it be somewhere else?

> +static void host_open(CPUState *cs, gdb_syscall_complete_cb complete,
> +                      target_ulong fname, target_ulong fname_len,
> +                      int gdb_flags, int mode)
> +{
> +    CPUArchState *env G_GNUC_UNUSED = cs->env_ptr;
> +    char *p;
> +    int ret, host_flags;
> +
> +    ret = validate_lock_user_string(&p, cs, fname, fname_len);
> +    if (ret < 0) {
> +        complete(cs, -1, -ret);
> +        return;
> +    }
> +
> +    if (gdb_flags & GDB_O_WRONLY) {
> +        host_flags = O_WRONLY;
> +    } else if (gdb_flags & GDB_O_RDWR) {
> +        host_flags = O_RDWR;
> +    } else {
> +        host_flags = O_RDONLY;
> +    }
> +    if (gdb_flags & GDB_O_CREAT) {
> +        host_flags |= O_CREAT;
> +    }
> +    if (gdb_flags & GDB_O_TRUNC) {
> +        host_flags |= O_TRUNC;
> +    }
> +    if (gdb_flags & GDB_O_EXCL) {
> +        host_flags |= O_EXCL;
> +    }
> +
> +    ret = open(p, host_flags, mode);
> +    if (ret < 0) {
> +        complete(cs, -1, errno_for_gdb());

So this changes the errno values in the not-gdb case from being
host errno values to the gdb protocol ones. Errnos in Arm semihosting
are an unspecified mess, so maybe we can get away with changing
the existing QEMU behaviour; but I'd rather we didn't do it
one syscall at a time in a big refactoring series if we can avoid it.

> +    } else {
> +        int guestfd = alloc_guestfd();
> +        associate_guestfd(guestfd, ret);
> +        complete(cs, guestfd, 0);
> +    }
> +    unlock_user(p, fname, 0);
> +}

thanks
-- PMM
Richard Henderson May 23, 2022, 3:46 p.m. UTC | #2 
On 5/23/22 06:30, Peter Maydell wrote:
> So this changes the errno values in the not-gdb case from being
> host errno values to the gdb protocol ones. Errnos in Arm semihosting
> are an unspecified mess, so maybe we can get away with changing
> the existing QEMU behaviour; but I'd rather we didn't do it
> one syscall at a time in a big refactoring series if we can avoid it.

Ok.

Also, I think I mentioned this in the v2 cover but not here, that having done the errno 
conversion here for arm semihosting, it worked less well for mips and xtensa, which have a 
rather better defined set of errnos.

My question from v2 was: should we in fact convert back from gdb's errno to host errno in 
gdbstub.c handle_file_io(), and then let each semihosting backend convert from host to guest?


r~
Peter Maydell May 23, 2022, 4:54 p.m. UTC | #3 
On Mon, 23 May 2022 at 16:46, Richard Henderson
<richard.henderson@linaro.org> wrote:
> Also, I think I mentioned this in the v2 cover but not here, that having done the errno
> conversion here for arm semihosting, it worked less well for mips and xtensa, which have a
> rather better defined set of errnos.
>
> My question from v2 was: should we in fact convert back from gdb's errno to host errno in
> gdbstub.c handle_file_io(), and then let each semihosting backend convert from host to guest?

That sounds like it's probably a better idea (though I'm not sure
what host errno we use for the gdb "unknown errno" case)...

-- PMM
Richard Henderson May 23, 2022, 6:21 p.m. UTC | #4 
On 5/23/22 09:54, Peter Maydell wrote:
> On Mon, 23 May 2022 at 16:46, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>> Also, I think I mentioned this in the v2 cover but not here, that having done the errno
>> conversion here for arm semihosting, it worked less well for mips and xtensa, which have a
>> rather better defined set of errnos.
>>
>> My question from v2 was: should we in fact convert back from gdb's errno to host errno in
>> gdbstub.c handle_file_io(), and then let each semihosting backend convert from host to guest?
> 
> That sounds like it's probably a better idea (though I'm not sure
> what host errno we use for the gdb "unknown errno" case)...

An excellent question.  I note that both mips and xtensa use EINVAL when there is no exact 
match for the guest.  It does seem to be the least bad option.


r~
Richard Henderson June 7, 2022, 6:23 p.m. UTC | #5 
On 5/23/22 06:30, Peter Maydell wrote:
>> diff --git a/semihosting/guestfd.c b/semihosting/guestfd.c
>> index b6405f5663..7ac2e147a8 100644
>> --- a/semihosting/guestfd.c
>> +++ b/semihosting/guestfd.c
>> @@ -11,6 +11,11 @@
>>   #include "qemu/osdep.h"
>>   #include "exec/gdbstub.h"
>>   #include "semihosting/guestfd.h"
>> +#ifdef CONFIG_USER_ONLY
>> +#include "qemu.h"
>> +#else
>> +#include "semihosting/softmmu-uaccess.h"
>> +#endif
> 
> Does this need to be in this patch, or should it be somewhere else?

The first use of target_strlen is added in this patch, so yes, belongs here.


r~
diff mbox series

Patch

diff --git a/include/semihosting/syscalls.h b/include/semihosting/syscalls.h
new file mode 100644
index 0000000000..991658bf79
--- /dev/null
+++ b/include/semihosting/syscalls.h
@@ -0,0 +1,25 @@ 
+/*
+ * Syscall implementations for semihosting.
+ *
+ * Copyright (c) 2022 Linaro
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef SEMIHOSTING_SYSCALLS_H
+#define SEMIHOSTING_SYSCALLS_H
+
+/*
+ * Argument loading from the guest is performed by the caller;
+ * results are returned via the 'complete' callback.
+ *
+ * String operands are in address/len pairs.  The len argument may be 0
+ * (when the semihosting abi does not already provide the length),
+ * or non-zero (where it should include the terminating zero).
+ */
+
+void semihost_sys_open(CPUState *cs, gdb_syscall_complete_cb complete,
+                       target_ulong fname, target_ulong fname_len,
+                       int gdb_flags, int mode);
+
+#endif /* SEMIHOSTING_SYSCALLS_H */
diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
index 7a7468799f..cc008d0338 100644
--- a/semihosting/arm-compat-semi.c
+++ b/semihosting/arm-compat-semi.c
@@ -35,9 +35,10 @@ 
 #include "semihosting/semihost.h"
 #include "semihosting/console.h"
 #include "semihosting/common-semi.h"
-#include "semihosting/guestfd.h"
 #include "qemu/timer.h"
 #include "exec/gdbstub.h"
+#include "semihosting/guestfd.h"
+#include "semihosting/syscalls.h"
 
 #ifdef CONFIG_USER_ONLY
 #include "qemu.h"
@@ -98,21 +99,6 @@  static int gdb_open_modeflags[12] = {
     GDB_O_RDWR | GDB_O_CREAT | GDB_O_APPEND | GDB_O_BINARY
 };
 
-static int open_modeflags[12] = {
-    O_RDONLY,
-    O_RDONLY | O_BINARY,
-    O_RDWR,
-    O_RDWR | O_BINARY,
-    O_WRONLY | O_CREAT | O_TRUNC,
-    O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
-    O_RDWR | O_CREAT | O_TRUNC,
-    O_RDWR | O_CREAT | O_TRUNC | O_BINARY,
-    O_WRONLY | O_CREAT | O_APPEND,
-    O_WRONLY | O_CREAT | O_APPEND | O_BINARY,
-    O_RDWR | O_CREAT | O_APPEND,
-    O_RDWR | O_CREAT | O_APPEND | O_BINARY
-};
-
 #ifndef CONFIG_USER_ONLY
 
 /**
@@ -284,20 +270,6 @@  common_semi_flen_cb(CPUState *cs, target_ulong ret, target_ulong err)
     common_semi_cb(cs, ret, err);
 }
 
-static int common_semi_open_guestfd;
-
-static void
-common_semi_open_cb(CPUState *cs, target_ulong ret, target_ulong err)
-{
-    if (err) {
-        dealloc_guestfd(common_semi_open_guestfd);
-    } else {
-        associate_guestfd(common_semi_open_guestfd, ret);
-        ret = common_semi_open_guestfd;
-    }
-    common_semi_cb(cs, ret, err);
-}
-
 /*
  * Types for functions implementing various semihosting calls
  * for specific types of guest file descriptor. These must all
@@ -601,22 +573,10 @@  void do_common_semihosting(CPUState *cs)
                 staticfile_guestfd(ret, featurefile_data,
                                    sizeof(featurefile_data));
             }
-        } else if (use_gdb_syscalls()) {
-            unlock_user(s, arg0, 0);
-            common_semi_open_guestfd = alloc_guestfd();
-            gdb_do_syscall(common_semi_open_cb,
-                           "open,%s,%x,1a4", arg0, (int)arg2 + 1,
-                           gdb_open_modeflags[arg1]);
-            break;
         } else {
-            hostfd = open(s, open_modeflags[arg1], 0644);
-            if (hostfd < 0) {
-                ret = -1;
-                err = errno;
-            } else {
-                ret = alloc_guestfd();
-                associate_guestfd(ret, hostfd);
-            }
+            semihost_sys_open(cs, common_semi_cb, arg0, arg2 + 1,
+                              gdb_open_modeflags[arg1], 0644);
+            break;
         }
         unlock_user(s, arg0, 0);
         common_semi_cb(cs, ret, err);
diff --git a/semihosting/guestfd.c b/semihosting/guestfd.c
index b6405f5663..7ac2e147a8 100644
--- a/semihosting/guestfd.c
+++ b/semihosting/guestfd.c
@@ -11,6 +11,11 @@ 
 #include "qemu/osdep.h"
 #include "exec/gdbstub.h"
 #include "semihosting/guestfd.h"
+#ifdef CONFIG_USER_ONLY
+#include "qemu.h"
+#else
+#include "semihosting/softmmu-uaccess.h"
+#endif
 
 static GArray *guestfd_array;
 
diff --git a/semihosting/syscalls.c b/semihosting/syscalls.c
new file mode 100644
index 0000000000..a44d5cbae2
--- /dev/null
+++ b/semihosting/syscalls.c
@@ -0,0 +1,186 @@ 
+/*
+ * Syscall implementations for semihosting.
+ *
+ * Copyright (c) 2022 Linaro
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "exec/gdbstub.h"
+#include "semihosting/guestfd.h"
+#include "semihosting/syscalls.h"
+#ifdef CONFIG_USER_ONLY
+#include "qemu.h"
+#else
+#include "semihosting/softmmu-uaccess.h"
+#endif
+
+
+/*
+ * Validate or compute the length of the string (including terminator).
+ */
+static int validate_strlen(CPUState *cs, target_ulong str, target_ulong tlen)
+{
+    CPUArchState *env G_GNUC_UNUSED = cs->env_ptr;
+    char c;
+
+    if (tlen == 0) {
+        ssize_t slen = target_strlen(str);
+
+        if (slen < 0) {
+            return -GDB_EFAULT;
+        }
+        if (slen >= INT32_MAX) {
+            return -GDB_ENAMETOOLONG;
+        }
+        return slen + 1;
+    }
+    if (tlen > INT32_MAX) {
+        return -GDB_ENAMETOOLONG;
+    }
+    if (get_user_u8(c, str + tlen - 1)) {
+        return -GDB_EFAULT;
+    }
+    if (c != 0) {
+        return -GDB_EINVAL;
+    }
+    return tlen;
+}
+
+static int validate_lock_user_string(char **pstr, CPUState *cs,
+                                     target_ulong tstr, target_ulong tlen)
+{
+    int ret = validate_strlen(cs, tstr, tlen);
+    CPUArchState *env G_GNUC_UNUSED = cs->env_ptr;
+    char *str = NULL;
+
+    if (ret > 0) {
+        str = lock_user(VERIFY_READ, tstr, ret, true);
+        ret = str ? 0 : -GDB_EFAULT;
+    }
+    *pstr = str;
+    return ret;
+}
+
+static int errno_for_gdb(void)
+{
+#define E(N)  case E##N: return GDB_E##N
+
+    switch (errno) {
+    E(PERM);
+    E(NOENT);
+    E(INTR);
+    E(BADF);
+    E(ACCES);
+    E(FAULT);
+    E(BUSY);
+    E(EXIST);
+    E(NODEV);
+    E(NOTDIR);
+    E(ISDIR);
+    E(INVAL);
+    E(NFILE);
+    E(MFILE);
+    E(FBIG);
+    E(NOSPC);
+    E(SPIPE);
+    E(ROFS);
+    E(NAMETOOLONG);
+    }
+    return GDB_EUNKNOWN;
+
+#undef E
+}
+
+/*
+ * GDB semihosting syscall implementations.
+ */
+
+static gdb_syscall_complete_cb gdb_open_complete;
+
+static void gdb_open_cb(CPUState *cs, target_ulong ret, target_ulong err)
+{
+    if (!err) {
+        int guestfd = alloc_guestfd();
+        associate_guestfd(guestfd, ret);
+        ret = guestfd;
+    }
+    gdb_open_complete(cs, ret, err);
+}
+
+static void gdb_open(CPUState *cs, gdb_syscall_complete_cb complete,
+                     target_ulong fname, target_ulong fname_len,
+                     int gdb_flags, int mode)
+{
+    int len = validate_strlen(cs, fname, fname_len);
+    if (len < 0) {
+        complete(cs, -1, -len);
+        return;
+    }
+
+    gdb_open_complete = complete;
+    gdb_do_syscall(gdb_open_cb, "open,%s,%x,%x",
+                   fname, len, (target_ulong)gdb_flags, (target_ulong)mode);
+}
+
+/*
+ * Host semihosting syscall implementations.
+ */
+
+static void host_open(CPUState *cs, gdb_syscall_complete_cb complete,
+                      target_ulong fname, target_ulong fname_len,
+                      int gdb_flags, int mode)
+{
+    CPUArchState *env G_GNUC_UNUSED = cs->env_ptr;
+    char *p;
+    int ret, host_flags;
+
+    ret = validate_lock_user_string(&p, cs, fname, fname_len);
+    if (ret < 0) {
+        complete(cs, -1, -ret);
+        return;
+    }
+
+    if (gdb_flags & GDB_O_WRONLY) {
+        host_flags = O_WRONLY;
+    } else if (gdb_flags & GDB_O_RDWR) {
+        host_flags = O_RDWR;
+    } else {
+        host_flags = O_RDONLY;
+    }
+    if (gdb_flags & GDB_O_CREAT) {
+        host_flags |= O_CREAT;
+    }
+    if (gdb_flags & GDB_O_TRUNC) {
+        host_flags |= O_TRUNC;
+    }
+    if (gdb_flags & GDB_O_EXCL) {
+        host_flags |= O_EXCL;
+    }
+
+    ret = open(p, host_flags, mode);
+    if (ret < 0) {
+        complete(cs, -1, errno_for_gdb());
+    } else {
+        int guestfd = alloc_guestfd();
+        associate_guestfd(guestfd, ret);
+        complete(cs, guestfd, 0);
+    }
+    unlock_user(p, fname, 0);
+}
+
+/*
+ * Syscall entry points.
+ */
+
+void semihost_sys_open(CPUState *cs, gdb_syscall_complete_cb complete,
+                       target_ulong fname, target_ulong fname_len,
+                       int gdb_flags, int mode)
+{
+    if (use_gdb_syscalls()) {
+        gdb_open(cs, complete, fname, fname_len, gdb_flags, mode);
+    } else {
+        host_open(cs, complete, fname, fname_len, gdb_flags, mode);
+    }
+}
diff --git a/semihosting/meson.build b/semihosting/meson.build
index d2c1c37bfd..8057db5494 100644
--- a/semihosting/meson.build
+++ b/semihosting/meson.build
@@ -1,5 +1,6 @@ 
 specific_ss.add(when: 'CONFIG_SEMIHOSTING', if_true: files(
   'guestfd.c',
+  'syscalls.c',
 ))
 
 specific_ss.add(when: ['CONFIG_SEMIHOSTING', 'CONFIG_SOFTMMU'], if_true: files(