qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 23/49] semihosting: Split out semihost_sys_open


From: Peter Maydell
Subject: Re: [PATCH v3 23/49] semihosting: Split out semihost_sys_open
Date: Mon, 23 May 2022 14:30:42 +0100

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]