Re: [PATCH v4 30/53] semihosting: Split out semihost_sys_write

[Luc Michel]

On 13:45 Tue 07 Jun     , Richard Henderson wrote:
> Split out the non-ARM specific portions of SYS_WRITE to a
> reusable function.  This handles all GuestFD.  This removes
> the last use of common_semi_syscall_len.
> 
> Note that gdb_do_syscall %x reads target_ulong, not int.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Luc Michel <lmichel@kalray.eu>

> ---
>  include/semihosting/syscalls.h |  6 ++++
>  semihosting/arm-compat-semi.c  | 52 +-------------------------------
>  semihosting/syscalls.c         | 54 ++++++++++++++++++++++++++++++++++
>  3 files changed, 61 insertions(+), 51 deletions(-)
> 
> diff --git a/include/semihosting/syscalls.h b/include/semihosting/syscalls.h
> index 20da8138b0..2464467579 100644
> --- a/include/semihosting/syscalls.h
> +++ b/include/semihosting/syscalls.h
> @@ -33,4 +33,10 @@ void semihost_sys_read(CPUState *cs, 
> gdb_syscall_complete_cb complete,
>  void semihost_sys_read_gf(CPUState *cs, gdb_syscall_complete_cb complete,
>                            GuestFD *gf, target_ulong buf, target_ulong len);
>  
> +void semihost_sys_write(CPUState *cs, gdb_syscall_complete_cb complete,
> +                        int fd, target_ulong buf, target_ulong len);
> +
> +void semihost_sys_write_gf(CPUState *cs, gdb_syscall_complete_cb complete,
> +                           GuestFD *gf, target_ulong buf, target_ulong len);
> +
>  #endif /* SEMIHOSTING_SYSCALLS_H */
> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
> index 5e11ec307a..d1d35e6032 100644
> --- a/semihosting/arm-compat-semi.c
> +++ b/semihosting/arm-compat-semi.c
> @@ -215,8 +215,6 @@ static inline uint32_t get_swi_errno(CPUState *cs)
>  #endif
>  }
>  
> -static target_ulong common_semi_syscall_len;
> -
>  static void common_semi_cb(CPUState *cs, target_ulong ret, target_ulong err)
>  {
>      if (err) {
> @@ -230,9 +228,6 @@ static void common_semi_cb(CPUState *cs, target_ulong 
> ret, target_ulong err)
>          /* Fixup syscalls that use nonstardard return conventions.  */
>          target_ulong reg0 = common_semi_arg(cs, 0);
>          switch (reg0) {
> -        case TARGET_SYS_WRITE:
> -            ret = common_semi_syscall_len - ret;
> -            break;
>          case TARGET_SYS_SEEK:
>              ret = 0;
>              break;
> @@ -294,30 +289,10 @@ common_semi_flen_cb(CPUState *cs, target_ulong ret, 
> target_ulong err)
>   * do the work and return the required return value to the guest
>   * via common_semi_cb.
>   */
> -typedef void sys_writefn(CPUState *cs, GuestFD *gf,
> -                         target_ulong buf, uint32_t len);
>  typedef void sys_isattyfn(CPUState *cs, GuestFD *gf);
>  typedef void sys_seekfn(CPUState *cs, GuestFD *gf, target_ulong offset);
>  typedef void sys_flenfn(CPUState *cs, GuestFD *gf);
>  
> -static void host_writefn(CPUState *cs, GuestFD *gf,
> -                         target_ulong buf, uint32_t len)
> -{
> -    CPUArchState *env = cs->env_ptr;
> -    uint32_t ret = 0;
> -    char *s = lock_user(VERIFY_READ, buf, len, 1);
> -    (void) env; /* Used in arm softmmu lock_user implicitly */
> -    if (s) {
> -        ret = write(gf->hostfd, s, len);
> -        unlock_user(s, buf, 0);
> -        if (ret == (uint32_t)-1) {
> -            ret = 0;
> -        }
> -    }
> -    /* Return bytes not written, on error as well. */
> -    common_semi_cb(cs, len - ret, 0);
> -}
> -
>  static void host_isattyfn(CPUState *cs, GuestFD *gf)
>  {
>      common_semi_cb(cs, isatty(gf->hostfd), 0);
> @@ -340,13 +315,6 @@ static void host_flenfn(CPUState *cs, GuestFD *gf)
>      }
>  }
>  
> -static void gdb_writefn(CPUState *cs, GuestFD *gf,
> -                        target_ulong buf, uint32_t len)
> -{
> -    common_semi_syscall_len = len;
> -    gdb_do_syscall(common_semi_cb, "write,%x,%x,%x", gf->hostfd, buf, len);
> -}
> -
>  static void gdb_isattyfn(CPUState *cs, GuestFD *gf)
>  {
>      gdb_do_syscall(common_semi_cb, "isatty,%x", gf->hostfd);
> @@ -380,13 +348,6 @@ static const uint8_t featurefile_data[] = {
>      SH_EXT_EXIT_EXTENDED | SH_EXT_STDOUT_STDERR, /* Feature byte 0 */
>  };
>  
> -static void staticfile_writefn(CPUState *cs, GuestFD *gf,
> -                               target_ulong buf, uint32_t len)
> -{
> -    /* This fd can never be open for writing */
> -    common_semi_cb(cs, -1, EBADF);
> -}
> -
>  static void staticfile_isattyfn(CPUState *cs, GuestFD *gf)
>  {
>      common_semi_cb(cs, 0, 0);
> @@ -404,7 +365,6 @@ static void staticfile_flenfn(CPUState *cs, GuestFD *gf)
>  }
>  
>  typedef struct GuestFDFunctions {
> -    sys_writefn *writefn;
>      sys_isattyfn *isattyfn;
>      sys_seekfn *seekfn;
>      sys_flenfn *flenfn;
> @@ -412,19 +372,16 @@ typedef struct GuestFDFunctions {
>  
>  static const GuestFDFunctions guestfd_fns[] = {
>      [GuestFDHost] = {
> -        .writefn = host_writefn,
>          .isattyfn = host_isattyfn,
>          .seekfn = host_seekfn,
>          .flenfn = host_flenfn,
>      },
>      [GuestFDGDB] = {
> -        .writefn = gdb_writefn,
>          .isattyfn = gdb_isattyfn,
>          .seekfn = gdb_seekfn,
>          .flenfn = gdb_flenfn,
>      },
>      [GuestFDStatic] = {
> -        .writefn = staticfile_writefn,
>          .isattyfn = staticfile_isattyfn,
>          .seekfn = staticfile_seekfn,
>          .flenfn = staticfile_flenfn,
> @@ -449,7 +406,6 @@ void do_common_semihosting(CPUState *cs)
>      char * s;
>      int nr;
>      uint32_t ret;
> -    uint32_t len;
>      GuestFD *gf;
>      int64_t elapsed;
>  
> @@ -530,13 +486,7 @@ void do_common_semihosting(CPUState *cs)
>          GET_ARG(0);
>          GET_ARG(1);
>          GET_ARG(2);
> -        len = arg2;
> -
> -        gf = get_guestfd(arg0);
> -        if (!gf) {
> -            goto do_badf;
> -        }
> -        guestfd_fns[gf->type].writefn(cs, gf, arg1, len);
> +        semihost_sys_write(cs, common_semi_rw_cb, arg0, arg1, arg2);
>          break;
>  
>      case TARGET_SYS_READ:
> diff --git a/semihosting/syscalls.c b/semihosting/syscalls.c
> index d42a190746..5cb12d6adc 100644
> --- a/semihosting/syscalls.c
> +++ b/semihosting/syscalls.c
> @@ -107,6 +107,13 @@ static void gdb_read(CPUState *cs, 
> gdb_syscall_complete_cb complete,
>                     (target_ulong)gf->hostfd, buf, len);
>  }
>  
> +static void gdb_write(CPUState *cs, gdb_syscall_complete_cb complete,
> +                      GuestFD *gf, target_ulong buf, target_ulong len)
> +{
> +    gdb_do_syscall(complete, "write,%x,%x,%x",
> +                   (target_ulong)gf->hostfd, buf, len);
> +}
> +
>  /*
>   * Host semihosting syscall implementations.
>   */
> @@ -193,6 +200,22 @@ static void host_read(CPUState *cs, 
> gdb_syscall_complete_cb complete,
>      }
>  }
>  
> +static void host_write(CPUState *cs, gdb_syscall_complete_cb complete,
> +                       GuestFD *gf, target_ulong buf, target_ulong len)
> +{
> +    CPUArchState *env G_GNUC_UNUSED = cs->env_ptr;
> +    void *ptr = lock_user(VERIFY_READ, buf, len, 1);
> +    ssize_t ret;
> +
> +    if (!ptr) {
> +        complete(cs, -1, EFAULT);
> +        return;
> +    }
> +    ret = write(gf->hostfd, ptr, len);
> +    complete(cs, ret, ret == -1 ? errno : 0);
> +    unlock_user(ptr, buf, 0);
> +}
> +
>  /*
>   * Static file semihosting syscall implementations.
>   */
> @@ -286,3 +309,34 @@ void semihost_sys_read(CPUState *cs, 
> gdb_syscall_complete_cb complete,
>          complete(cs, -1, EBADF);
>      }
>  }
> +
> +void semihost_sys_write_gf(CPUState *cs, gdb_syscall_complete_cb complete,
> +                           GuestFD *gf, target_ulong buf, target_ulong len)
> +{
> +    switch (gf->type) {
> +    case GuestFDGDB:
> +        gdb_write(cs, complete, gf, buf, len);
> +        break;
> +    case GuestFDHost:
> +        host_write(cs, complete, gf, buf, len);
> +        break;
> +    case GuestFDStatic:
> +        /* Static files are never open for writing: EBADF. */
> +        complete(cs, -1, EBADF);
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
> +void semihost_sys_write(CPUState *cs, gdb_syscall_complete_cb complete,
> +                        int fd, target_ulong buf, target_ulong len)
> +{
> +    GuestFD *gf = get_guestfd(fd);
> +
> +    if (gf) {
> +        semihost_sys_write_gf(cs, complete, gf, buf, len);
> +    } else {
> +        complete(cs, -1, EBADF);
> +    }
> +}
> -- 
> 2.34.1
> 
> 
> 
> 
> To declare a filtering error, please use the following link : 
> https://www.security-mail.net/reporter.php?mid=c32f.629fcee1.e7f15.0&r=lmichel%40kalrayinc.com&s=qemu-devel-bounces%2Blmichel%3Dkalrayinc.com%40nongnu.org&o=%5BPATCH+v4+30%2F53%5D+semihosting%3A+Split+out+semihost_sys_write&verdict=C&c=3bbac77ccd2af6af6edbaa9dc0a61a8e124e1221
> 

--