Re: [PATCH v4 34/53] semihosting: Split out semihost_sys_flen

[Luc Michel]

On 13:45 Tue 07 Jun     , Richard Henderson wrote:
> The ARM-specific SYS_FLEN isn't really something that can be
> reused by other semihosting apis, but there are parts that can
> reused for the implementation of semihost_sys_fstat.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

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

> ---
>  include/semihosting/syscalls.h |  4 ++
>  semihosting/arm-compat-semi.c  | 74 ++++++----------------------------
>  semihosting/syscalls.c         | 49 ++++++++++++++++++++++
>  3 files changed, 66 insertions(+), 61 deletions(-)
> 
> diff --git a/include/semihosting/syscalls.h b/include/semihosting/syscalls.h
> index c60ebafb85..1ae5ba6716 100644
> --- a/include/semihosting/syscalls.h
> +++ b/include/semihosting/syscalls.h
> @@ -45,4 +45,8 @@ void semihost_sys_lseek(CPUState *cs, 
> gdb_syscall_complete_cb complete,
>  void semihost_sys_isatty(CPUState *cs, gdb_syscall_complete_cb complete,
>                           int fd);
>  
> +void semihost_sys_flen(CPUState *cs, gdb_syscall_complete_cb fstat_cb,
> +                       gdb_syscall_complete_cb flen_cb,
> +                       int fd, target_ulong fstat_addr);
> +
>  #endif /* SEMIHOSTING_SYSCALLS_H */
> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
> index 4529c9df06..81dd22e1c5 100644
> --- a/semihosting/arm-compat-semi.c
> +++ b/semihosting/arm-compat-semi.c
> @@ -285,44 +285,25 @@ static target_ulong common_semi_flen_buf(CPUState *cs)
>  }
>  
>  static void
> -common_semi_flen_cb(CPUState *cs, target_ulong ret, target_ulong err)
> +common_semi_flen_fstat_cb(CPUState *cs, target_ulong ret, target_ulong err)
>  {
>      if (!err) {
>          /* The size is always stored in big-endian order, extract the value. 
> */
>          uint64_t size;
> -        cpu_memory_rw_debug(cs, common_semi_flen_buf(cs) +
> -                            offsetof(struct gdb_stat, gdb_st_size),
> -                            &size, 8, 0);
> -        ret = be64_to_cpu(size);
> +        if (cpu_memory_rw_debug(cs, common_semi_flen_buf(cs) +
> +                                offsetof(struct gdb_stat, gdb_st_size),
> +                                &size, 8, 0)) {
> +            ret = -1, err = EFAULT;
> +        } else {
> +            size = be64_to_cpu(size);
> +            if (ret != size) {
> +                ret = -1, err = EOVERFLOW;
> +            }
> +        }
>      }
>      common_semi_cb(cs, ret, err);
>  }
>  
> -/*
> - * 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 to the guest
> - * via common_semi_cb.
> - */
> -typedef void sys_flenfn(CPUState *cs, GuestFD *gf);
> -
> -static void host_flenfn(CPUState *cs, GuestFD *gf)
> -{
> -    struct stat buf;
> -
> -    if (fstat(gf->hostfd, &buf)) {
> -        common_semi_cb(cs, -1, errno);
> -    } else {
> -        common_semi_cb(cs, buf.st_size, 0);
> -    }
> -}
> -
> -static void gdb_flenfn(CPUState *cs, GuestFD *gf)
> -{
> -    gdb_do_syscall(common_semi_flen_cb, "fstat,%x,%x",
> -                   gf->hostfd, common_semi_flen_buf(cs));
> -}
> -
>  #define SHFB_MAGIC_0 0x53
>  #define SHFB_MAGIC_1 0x48
>  #define SHFB_MAGIC_2 0x46
> @@ -340,27 +321,6 @@ static const uint8_t featurefile_data[] = {
>      SH_EXT_EXIT_EXTENDED | SH_EXT_STDOUT_STDERR, /* Feature byte 0 */
>  };
>  
> -static void staticfile_flenfn(CPUState *cs, GuestFD *gf)
> -{
> -    common_semi_cb(cs, gf->staticfile.len, 0);
> -}
> -
> -typedef struct GuestFDFunctions {
> -    sys_flenfn *flenfn;
> -} GuestFDFunctions;
> -
> -static const GuestFDFunctions guestfd_fns[] = {
> -    [GuestFDHost] = {
> -        .flenfn = host_flenfn,
> -    },
> -    [GuestFDGDB] = {
> -        .flenfn = gdb_flenfn,
> -    },
> -    [GuestFDStatic] = {
> -        .flenfn = staticfile_flenfn,
> -    },
> -};
> -
>  /*
>   * Do a semihosting call.
>   *
> @@ -379,7 +339,6 @@ void do_common_semihosting(CPUState *cs)
>      char * s;
>      int nr;
>      uint32_t ret;
> -    GuestFD *gf;
>      int64_t elapsed;
>  
>      nr = common_semi_arg(cs, 0) & 0xffffffffU;
> @@ -492,12 +451,8 @@ void do_common_semihosting(CPUState *cs)
>  
>      case TARGET_SYS_FLEN:
>          GET_ARG(0);
> -
> -        gf = get_guestfd(arg0);
> -        if (!gf) {
> -            goto do_badf;
> -        }
> -        guestfd_fns[gf->type].flenfn(cs, gf);
> +        semihost_sys_flen(cs, common_semi_flen_fstat_cb, common_semi_cb,
> +                          arg0, common_semi_flen_buf(cs));
>          break;
>  
>      case TARGET_SYS_TMPNAM:
> @@ -819,9 +774,6 @@ void do_common_semihosting(CPUState *cs)
>          cpu_dump_state(cs, stderr, 0);
>          abort();
>  
> -    do_badf:
> -        common_semi_cb(cs, -1, EBADF);
> -        break;
>      do_fault:
>          common_semi_cb(cs, -1, EFAULT);
>          break;
> diff --git a/semihosting/syscalls.c b/semihosting/syscalls.c
> index 1f1baf7e2d..fff9550c89 100644
> --- a/semihosting/syscalls.c
> +++ b/semihosting/syscalls.c
> @@ -127,6 +127,12 @@ static void gdb_isatty(CPUState *cs, 
> gdb_syscall_complete_cb complete,
>      gdb_do_syscall(complete, "isatty,%x", (target_ulong)gf->hostfd);
>  }
>  
> +static void gdb_fstat(CPUState *cs, gdb_syscall_complete_cb complete,
> +                      GuestFD *gf, target_ulong addr)
> +{
> +    gdb_do_syscall(complete, "fstat,%x,%x", (target_ulong)gf->hostfd, addr);
> +}
> +
>  /*
>   * Host semihosting syscall implementations.
>   */
> @@ -259,6 +265,18 @@ static void host_isatty(CPUState *cs, 
> gdb_syscall_complete_cb complete,
>      complete(cs, ret, ret ? 0 : errno);
>  }
>  
> +static void host_flen(CPUState *cs, gdb_syscall_complete_cb complete,
> +                      GuestFD *gf)
> +{
> +    struct stat buf;
> +
> +    if (fstat(gf->hostfd, &buf) < 0) {
> +        complete(cs, -1, errno);
> +    } else {
> +        complete(cs, buf.st_size, 0);
> +    }
> +}
> +
>  /*
>   * Static file semihosting syscall implementations.
>   */
> @@ -311,6 +329,12 @@ static void staticfile_lseek(CPUState *cs, 
> gdb_syscall_complete_cb complete,
>      }
>  }
>  
> +static void staticfile_flen(CPUState *cs, gdb_syscall_complete_cb complete,
> +                            GuestFD *gf)
> +{
> +    complete(cs, gf->staticfile.len, 0);
> +}
> +
>  /*
>   * Syscall entry points.
>   */
> @@ -473,3 +497,28 @@ void semihost_sys_isatty(CPUState *cs, 
> gdb_syscall_complete_cb complete, int fd)
>          g_assert_not_reached();
>      }
>  }
> +
> +void semihost_sys_flen(CPUState *cs, gdb_syscall_complete_cb fstat_cb,
> +                       gdb_syscall_complete_cb flen_cb, int fd,
> +                       target_ulong fstat_addr)
> +{
> +    GuestFD *gf = get_guestfd(fd);
> +
> +    if (!gf) {
> +        flen_cb(cs, -1, EBADF);
> +        return;
> +    }
> +    switch (gf->type) {
> +    case GuestFDGDB:
> +        gdb_fstat(cs, fstat_cb, gf, fstat_addr);
> +        break;
> +    case GuestFDHost:
> +        host_flen(cs, flen_cb, gf);
> +        break;
> +    case GuestFDStatic:
> +        staticfile_flen(cs, flen_cb, gf);
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> -- 
> 2.34.1
> 
> 
> 
> 
> To declare a filtering error, please use the following link : 
> https://www.security-mail.net/reporter.php?mid=aa67.629fd36d.4e8a3.0&r=lmichel%40kalrayinc.com&s=qemu-devel-bounces%2Blmichel%3Dkalrayinc.com%40nongnu.org&o=%5BPATCH+v4+34%2F53%5D+semihosting%3A+Split+out+semihost_sys_flen&verdict=C&c=a467e715c0918a0a23cc8411b01a8abf95ca1564
> 

--