Re: [PATCH v6 3/4] linux-user: Support futex_time64

[Laurent Vivier]

Le 06/03/2020 à 19:24, Alistair Francis a écrit :
> Add support for host and target futex_time64. If futex_time64 exists on
> the host we try that first before falling back to the standard futux
> syscall.

In fact there are two definitions for futex: one to use as system call
in TARGET_NR_exit (sys_futex) and one to use to translate
TARGET_NR_futex (see d509eeb13c9c ("linux-user: Use safe_syscall for
futex syscall")).

We also rely on the system timespec definition, so I'm not sure anymore
we can use both 32bit and 64bit host syscalls (it's more complicated
than I expected...)


> Signed-off-by: Alistair Francis <address@hidden>
> ---
>  linux-user/syscall.c | 98 ++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 80 insertions(+), 18 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 0f219b26c1..8a50e2d3dc 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -245,7 +245,12 @@ static type name (type1 arg1,type2 arg2,type3 arg3,type4 
> arg4,type5 arg5,        \
>  #define __NR_sys_rt_sigqueueinfo __NR_rt_sigqueueinfo
>  #define __NR_sys_rt_tgsigqueueinfo __NR_rt_tgsigqueueinfo
>  #define __NR_sys_syslog __NR_syslog
> -#define __NR_sys_futex __NR_futex
> +#if defined(__NR_futex)
> +# define __NR_sys_futex __NR_futex
> +#endif
> +#if defined(__NR_futex_time64)
> +# define __NR_sys_futex_time64 __NR_futex_time64
> +#endif

As you don't use futex_time64() in TARGET_NR_exit, you don't need to
define it.

>  #define __NR_sys_inotify_init __NR_inotify_init
>  #define __NR_sys_inotify_add_watch __NR_inotify_add_watch
>  #define __NR_sys_inotify_rm_watch __NR_inotify_rm_watch
> @@ -295,10 +300,15 @@ _syscall1(int,exit_group,int,error_code)
>  #if defined(TARGET_NR_set_tid_address) && defined(__NR_set_tid_address)
>  _syscall1(int,set_tid_address,int *,tidptr)
>  #endif
> -#if defined(TARGET_NR_futex) && defined(__NR_futex)
> +#if (defined(TARGET_NR_futex) || defined(TARGET_NR_futex_time64)) && \
> +    defined(__NR_futex)
>  _syscall6(int,sys_futex,int *,uaddr,int,op,int,val,
>            const struct timespec *,timeout,int *,uaddr2,int,val3)
>  #endif
> +#if defined(TARGET_NR_futex_time64) && defined(__NR_futex_time64)
> +_syscall6(int,sys_futex_time64,int *,uaddr,int,op,int,val,
> +          const struct timespec *,timeout,int *,uaddr2,int,val3)
> +#endif

We don't need them as the sys_ version are not used in TARGET_NR_futex
and TARGET_NR_futex_time64...

>  #define __NR_sys_sched_getaffinity __NR_sched_getaffinity
>  _syscall3(int, sys_sched_getaffinity, pid_t, pid, unsigned int, len,
>            unsigned long *, user_mask_ptr);
> @@ -762,10 +772,14 @@ safe_syscall5(int, ppoll, struct pollfd *, ufds, 
> unsigned int, nfds,
>  safe_syscall6(int, epoll_pwait, int, epfd, struct epoll_event *, events,
>                int, maxevents, int, timeout, const sigset_t *, sigmask,
>                size_t, sigsetsize)
> -#ifdef TARGET_NR_futex
> +#if defined(__NR_futex)
>  safe_syscall6(int,futex,int *,uaddr,int,op,int,val, \
>                const struct timespec *,timeout,int *,uaddr2,int,val3)
>  #endif
> +#if defined(__NR_futex_time64)
> +safe_syscall6(int,futex_time64,int *,uaddr,int,op,int,val, \
> +              const struct timespec *,timeout,int *,uaddr2,int,val3)
> +#endif

... we use these ones, the safe_ version.

>  safe_syscall2(int, rt_sigsuspend, sigset_t *, newset, size_t, sigsetsize)
>  safe_syscall2(int, kill, pid_t, pid, int, sig)
>  safe_syscall2(int, tkill, int, tid, int, sig)
> @@ -1210,7 +1224,7 @@ static inline abi_long copy_to_user_timeval64(abi_ulong 
> target_tv_addr,
>      return 0;
>  }
>  
> -#if defined(TARGET_NR_futex) || \
> +#if defined(TARGET_NR_futex) || defined(TARGET_NR_futex_time64) || \
>      defined(TARGET_NR_rt_sigtimedwait) || \
>      defined(TARGET_NR_pselect6) || defined(TARGET_NR_pselect6) || \
>      defined(TARGET_NR_nanosleep) || defined(TARGET_NR_clock_settime) || \
> @@ -6898,12 +6912,12 @@ static inline abi_long host_to_target_statx(struct 
> target_statx *host_stx,
>     futexes locally would make futexes shared between multiple processes
>     tricky.  However they're probably useless because guest atomic
>     operations won't work either.  */
> -#if defined(TARGET_NR_futex)
> +#if defined(TARGET_NR_futex) || defined(TARGET_NR_futex_time64)
>  static int do_futex(target_ulong uaddr, int op, int val, target_ulong 
> timeout,
>                      target_ulong uaddr2, int val3)
>  {
>      struct timespec ts, *pts;
> -    int base_op;
> +    int base_op, err = -ENOSYS;
>  
>      /* ??? We assume FUTEX_* constants are the same on both host
>         and target.  */
> @@ -6915,18 +6929,49 @@ static int do_futex(target_ulong uaddr, int op, int 
> val, target_ulong timeout,
>      switch (base_op) {
>      case FUTEX_WAIT:
>      case FUTEX_WAIT_BITSET:
> +#ifdef __NR_futex_time64
> +        struct __kernel_timespec ts64, *pts64;
> +
>          if (timeout) {
> -            pts = &ts;
> -            target_to_host_timespec(pts, timeout);
> +            pts64 = &ts64;
> +            target_to_host_timespec64(pts64, timeout);

We use __kernel_timespec with a function that takes timespec.
If system defines __NR_futex_time64 we can guess it uses the 64bit
version of timespec but I'm not sure compiler is happy with that.

Moreover you guess timeout is the target 64bit version and you don't
know that.

>          } else {
> -            pts = NULL;
> +            pts64 = NULL;
> +        }
> +
> +        err = get_errno(safe_futex_time64(g2h(uaddr), op, tswap32(val),
> +                         pts64, NULL, val3));
> +#endif
> +#ifdef __NR_futex
> +        if (err == -ENOSYS) {
> +            if (timeout) {
> +                pts = &ts;
> +                target_to_host_timespec(pts, timeout);

But if __NR_futex_time64 is defined, we guessed timespec is 64bit, so it
cannot be used with target_to_host_timespec().

You guess timeout is the target 32bit version and you don't know that:
you must pass the syscall number (TARGET_NR_futex and
TARGET_NR_futex_time64) to use the good version of the target structure.

target_to_host_timespec64() is to use with the 64bit _target_ syscall,
target_to_host_timespec() with the 32bit _target_ syscall.

To ease the translation you should decorelate target and host syscalls.
To ease the definition of this function you should rely on the system
timespec definition (it appears to be complicated to support both
syscalls because of the timespec definition).

Something like:

static int do_safe_futex(int *uaddr, int op, int val,
                         const struct timespec *timeout, int *uaddr2,
                         int val3)
{
#if HOST_LONG_BITS == 64
#if defined(__NR_futex)
    /* always a 64-bit time_t, it doesn't define _time64 version  */
    return get_errno(safe_futex(uaddr, op, val, timeout, uaddr2, val3));
#endif
#else
#if defined(__NR_futex_time64)
    if (sizeof(timeout->tv_sec) == 8) {
        /* _time64 function on 32bit arch */
        return get_errno(safe_futex_time64(uaddr, op, val, timeout,
uaddr2, val3));
    }
#endif
#if defined(__NR_futex)
    /* old function on 32bit arch */
    return get_errno(safe_futex(uaddr, op, val, timeout, uaddr2, val3));
#endif
#endif
    return -TARGET_ENOSYS;
}

and use it as-is in do_futex() and define a do_futex_time64() that use
it with target_to_host_timespec64() (or add a parameter to do_futex() to
select timespec type).

You should also define a do_sys_futex() to use with TARGET_NR_exit.

> +            } else {
> +                pts = NULL;
> +            }
> +            return get_errno(safe_futex(g2h(uaddr), op, tswap32(val),
> +                             pts, NULL, val3));
>          }
> -        return get_errno(safe_futex(g2h(uaddr), op, tswap32(val),
> -                         pts, NULL, val3));
> +#endif
>      case FUTEX_WAKE:
> -        return get_errno(safe_futex(g2h(uaddr), op, val, NULL, NULL, 0));
> +#ifdef __NR_futex_time64
> +        err = get_errno(safe_futex_time64(g2h(uaddr), op, val, NULL, NULL, 
> 0));
> +#endif
> +#ifdef __NR_futex
> +        if (err == -ENOSYS) {
> +            return get_errno(safe_futex(g2h(uaddr), op, val, NULL, NULL, 0));
> +        }
> +#endif
>      case FUTEX_FD:
> -        return get_errno(safe_futex(g2h(uaddr), op, val, NULL, NULL, 0));
> +#ifdef __NR_futex_time64
> +        err = get_errno(safe_futex_time64(g2h(uaddr), op, val, NULL, NULL, 
> 0));
> +#endif
> +#ifdef __NR_futex
> +        if (err == -ENOSYS) {
> +            return get_errno(safe_futex(g2h(uaddr), op, val, NULL, NULL, 0));
> +        }
> +#endif
>      case FUTEX_REQUEUE:
>      case FUTEX_CMP_REQUEUE:
>      case FUTEX_WAKE_OP:
> @@ -6935,12 +6980,25 @@ static int do_futex(target_ulong uaddr, int op, int 
> val, target_ulong timeout,
>             But the prototype takes a `struct timespec *'; insert casts
>             to satisfy the compiler.  We do not need to tswap TIMEOUT
>             since it's not compared to guest memory.  */
> +#ifdef __NR_futex_time64
> +        struct __kernel_timespec *pts64;
> +        pts64 = (struct __kernel_timespec *)(uintptr_t) timeout;
> +        ret = get_errno(safe_futex_time64(g2h(uaddr), op, val, pts64,
> +                                   g2h(uaddr2),
> +                                   (base_op == FUTEX_CMP_REQUEUE
> +                                    ? tswap32(val3)
> +                                    : val3)));
> +#endif
> +#ifdef __NR_futex
>          pts = (struct timespec *)(uintptr_t) timeout;
> -        return get_errno(safe_futex(g2h(uaddr), op, val, pts,
> -                                    g2h(uaddr2),
> -                                    (base_op == FUTEX_CMP_REQUEUE
> -                                     ? tswap32(val3)
> -                                     : val3)));
> +        if (err == -ENOSYS) {
> +            return get_errno(safe_futex(g2h(uaddr), op, val, pts,
> +                                      g2h(uaddr2),
> +                                      (base_op == FUTEX_CMP_REQUEUE
> +                                       ? tswap32(val3)
> +                                       : val3)));
> +        }
> +#endif
>      default:
>          return -TARGET_ENOSYS;
>      }
> @@ -11599,6 +11657,10 @@ static abi_long do_syscall1(void *cpu_env, int num, 
> abi_long arg1,
>      case TARGET_NR_futex:
>          return do_futex(arg1, arg2, arg3, arg4, arg5, arg6);
>  #endif
> +#ifdef TARGET_NR_futex_time64
> +    case TARGET_NR_futex_time64:
> +        return do_futex(arg1, arg2, arg3, arg4, arg5, arg6);

How do you know if arg4 is target_kernel_timespec (64bit) or
target_timespec (32bit) in the function?

So you should define a do_futex64() function or pass the the syscall
number to the function.

> +#endif
>  #if defined(TARGET_NR_inotify_init) && defined(__NR_inotify_init)
>      case TARGET_NR_inotify_init:
>          ret = get_errno(sys_inotify_init());
> 

Thanks,
Laurent