diff mbox

[v5,1/8] linux-user: Add support for adjtimex() syscall

Message ID 20160914202008.14119-2-aleksandar.markovic@rt-rk.com (mailing list archive)
State New, archived
Headers show

Commit Message

Aleksandar Markovic Sept. 14, 2016, 8:19 p.m. UTC
From: Aleksandar Markovic <aleksandar.markovic@imgtec.com>

This patch implements Qemu user mode adjtimex() syscall support.

Syscall adjtimex() reads and optionally sets parameters for a clock
adjustment algorithm used in network synchonization or similar scenarios.

The implementation is based on invocation of host's adjtimex(), and
its key part is in the correspondent case segment of the main switch
statement of the function do_syscall(), in file linux-user/syscalls.c.
Also, support for related structure "timex" is added to the file
linux-user/syscall_defs.h, based on its definition in Linux kernel. All
necessary conversions of the data structures from target to host and from
host to target are covered. Two new functions, target_to_host_timex() and
host_to_target_timex(), are provided for the purpose of such conversions.
Moreover, the relevant support for "-strace" Qemu option is included in
files linux-user/strace.c and linux-user/strace.list.

This patch also fixes failures of LTP tests adjtimex01 and adjtimex02, if
executed in Qemu user mode.

Signed-off-by: Aleksandar Rikalo <aleksandar.rikalo@imgtec.com>
Signed-off-by: Aleksandar Markovic <aleksandar.markovic@imgtec.com>
---
 linux-user/strace.c       | 12 +++++++
 linux-user/strace.list    |  2 +-
 linux-user/syscall.c      | 90 ++++++++++++++++++++++++++++++++++++++++++++++-
 linux-user/syscall_defs.h | 28 +++++++++++++++
 4 files changed, 130 insertions(+), 2 deletions(-)

Comments

Laurent Vivier Sept. 17, 2016, 6:40 p.m. UTC | #1
Le 14/09/2016 à 22:19, Aleksandar Markovic a écrit :
> From: Aleksandar Markovic <aleksandar.markovic@imgtec.com>
> 
> This patch implements Qemu user mode adjtimex() syscall support.
> 
> Syscall adjtimex() reads and optionally sets parameters for a clock
> adjustment algorithm used in network synchonization or similar scenarios.
> 
> The implementation is based on invocation of host's adjtimex(), and
> its key part is in the correspondent case segment of the main switch
> statement of the function do_syscall(), in file linux-user/syscalls.c.
> Also, support for related structure "timex" is added to the file
> linux-user/syscall_defs.h, based on its definition in Linux kernel. All
> necessary conversions of the data structures from target to host and from
> host to target are covered. Two new functions, target_to_host_timex() and
> host_to_target_timex(), are provided for the purpose of such conversions.
> Moreover, the relevant support for "-strace" Qemu option is included in
> files linux-user/strace.c and linux-user/strace.list.
> 
> This patch also fixes failures of LTP tests adjtimex01 and adjtimex02, if
> executed in Qemu user mode.
> 
> Signed-off-by: Aleksandar Rikalo <aleksandar.rikalo@imgtec.com>
> Signed-off-by: Aleksandar Markovic <aleksandar.markovic@imgtec.com>
> ---
>  linux-user/strace.c       | 12 +++++++
>  linux-user/strace.list    |  2 +-
>  linux-user/syscall.c      | 90 ++++++++++++++++++++++++++++++++++++++++++++++-
>  linux-user/syscall_defs.h | 28 +++++++++++++++
>  4 files changed, 130 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/strace.c b/linux-user/strace.c
> index cc10dc4..7ddcaf8 100644
> --- a/linux-user/strace.c
> +++ b/linux-user/strace.c
> @@ -919,6 +919,18 @@ print_access(const struct syscallname *name,
>  }
>  #endif
>  
> +#ifdef TARGET_NR_adjtimex
> +static void
> +print_adjtimex(const struct syscallname *name,
> +    abi_long arg0, abi_long arg1, abi_long arg2,
> +    abi_long arg3, abi_long arg4, abi_long arg5)
> +{
> +    print_syscall_prologue(name);
> +    print_pointer(arg0, 1);
> +    print_syscall_epilogue(name);
> +}
> +#endif
> +
>  #ifdef TARGET_NR_brk
>  static void
>  print_brk(const struct syscallname *name,
> diff --git a/linux-user/strace.list b/linux-user/strace.list
> index aa967a2..9a665a8 100644
> --- a/linux-user/strace.list
> +++ b/linux-user/strace.list
> @@ -16,7 +16,7 @@
>  { TARGET_NR_add_key, "add_key" , NULL, NULL, NULL },
>  #endif
>  #ifdef TARGET_NR_adjtimex
> -{ TARGET_NR_adjtimex, "adjtimex" , NULL, NULL, NULL },
> +{ TARGET_NR_adjtimex, "adjtimex" , NULL, print_adjtimex, NULL },
>  #endif
>  #ifdef TARGET_NR_afs_syscall
>  { TARGET_NR_afs_syscall, "afs_syscall" , NULL, NULL, NULL },
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index ca06943..5643840 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -35,6 +35,7 @@
>  #include <sys/swap.h>
>  #include <linux/capability.h>
>  #include <sched.h>
> +#include <sys/timex.h>
>  #ifdef __ia64__
>  int __clone2(int (*fn)(void *), void *child_stack_base,
>               size_t stack_size, int flags, void *arg, ...);
> @@ -6578,6 +6579,78 @@ static inline abi_long target_ftruncate64(void *cpu_env, abi_long arg1,
>  }
>  #endif
>  
> +#ifdef TARGET_NR_adjtimex
> +static inline abi_long target_to_host_timex(struct timex *host_buf,
> +                                            abi_long target_addr)
> +{
> +    struct target_timex *target_buf;

target_tx instead of target_buf would be nicer(?)

> +
> +    if (!lock_user_struct(VERIFY_READ, target_buf, target_addr, 1)) {
> +        return -TARGET_EFAULT;
> +    }
> +
> +    host_buf->modes = tswap32(target_buf->modes);
> +    host_buf->offset = tswapal(target_buf->offset);
> +    host_buf->freq = tswapal(target_buf->freq);
> +    host_buf->maxerror = tswapal(target_buf->maxerror);
> +    host_buf->esterror = tswapal(target_buf->esterror);
> +    host_buf->status = tswap32(target_buf->status);
> +    host_buf->constant = tswapal(target_buf->constant);
> +    host_buf->precision = tswapal(target_buf->precision);
> +    host_buf->tolerance = tswapal(target_buf->tolerance);
> +    host_buf->time.tv_sec = tswapal(target_buf->time.tv_sec);
> +    host_buf->time.tv_usec = tswapal(target_buf->time.tv_usec);
> +    host_buf->tick = tswapal(target_buf->tick);
> +    host_buf->ppsfreq = tswapal(target_buf->ppsfreq);
> +    host_buf->jitter = tswapal(target_buf->jitter);
> +    host_buf->shift = tswap32(target_buf->shift);
> +    host_buf->stabil = tswapal(target_buf->stabil);
> +    host_buf->jitcnt = tswapal(target_buf->jitcnt);
> +    host_buf->calcnt = tswapal(target_buf->calcnt);
> +    host_buf->errcnt = tswapal(target_buf->errcnt);
> +    host_buf->stbcnt = tswapal(target_buf->stbcnt);
> +    host_buf->tai = tswap32(target_buf->tai);

You should use __get_user(), see
c7e35da linux-user: Handle negative values in timespec conversion


> +
> +    unlock_user_struct(target_buf, target_addr, 0);
> +    return 0;
> +}
> +
> +static inline abi_long host_to_target_timex(abi_long target_addr,
> +                                            struct timex *host_buf)
> +{
> +    struct target_timex *target_buf;

target_tx?

> +
> +    if (!lock_user_struct(VERIFY_WRITE, target_buf, target_addr, 0)) {
> +        return -TARGET_EFAULT;
> +    }
> +
> +    target_buf->modes = tswap32(host_buf->modes);
> +    target_buf->offset = tswapal(host_buf->offset);
> +    target_buf->freq = tswapal(host_buf->freq);
> +    target_buf->maxerror = tswapal(host_buf->maxerror);
> +    target_buf->esterror = tswapal(host_buf->esterror);
> +    target_buf->status = tswap32(host_buf->status);
> +    target_buf->constant = tswapal(host_buf->constant);
> +    target_buf->precision = tswapal(host_buf->precision);
> +    target_buf->tolerance = tswapal(host_buf->tolerance);
> +    target_buf->time.tv_sec = tswapal(host_buf->time.tv_sec);
> +    target_buf->time.tv_usec = tswapal(host_buf->time.tv_usec);
> +    target_buf->tick = tswapal(host_buf->tick);
> +    target_buf->ppsfreq = tswapal(host_buf->ppsfreq);
> +    target_buf->jitter = tswapal(host_buf->jitter);
> +    target_buf->shift = tswap32(host_buf->shift);
> +    target_buf->stabil = tswapal(host_buf->stabil);
> +    target_buf->jitcnt = tswapal(host_buf->jitcnt);
> +    target_buf->calcnt = tswapal(host_buf->calcnt);
> +    target_buf->errcnt = tswapal(host_buf->errcnt);
> +    target_buf->stbcnt = tswapal(host_buf->stbcnt);
> +    target_buf->tai = tswap32(host_buf->tai);

I think you should use __put_user()...

> +    unlock_user_struct(target_buf, target_addr, 1);
> +    return 0;
> +}
> +#endif
> +
>  static inline abi_long target_to_host_timespec(struct timespec *host_ts,
>                                                 abi_ulong target_addr)
>  {
> @@ -9419,8 +9492,23 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>          break;
>  #endif
>  #endif
> +#ifdef TARGET_NR_adjtimex
>      case TARGET_NR_adjtimex:
> -        goto unimplemented;
> +        {
> +            struct timex host_buf;
> +

check arg1 != NULL, so you manage target_to_host and host_to_target NULL
case.

> +            if (target_to_host_timex(&host_buf, arg1) != 0) {
> +                goto efault;
> +            }
> +            ret = get_errno(adjtimex(&host_buf));
> +            if (!is_error(ret) && arg1) {
> +                if (host_to_target_timex(arg1, &host_buf) != 0) {
> +                    goto efault;
> +                }
> +            }
> +        }
> +        break;
> +#endif
>  #ifdef TARGET_NR_create_module
>      case TARGET_NR_create_module:
>  #endif
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 7835654..afd9191 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -207,6 +207,34 @@ struct target_itimerspec {
>      struct target_timespec it_value;
>  };
>  
> +struct target_timex {
> +    unsigned int modes;          /* Mode selector */
> +    abi_long offset;             /* Time offset */
> +    abi_long freq;               /* Frequency offset */
> +    abi_long maxerror;           /* Maximum error (microseconds) */
> +    abi_long esterror;           /* Estimated error (microseconds) */
> +    int status;                  /* Clock command/status */
> +    abi_long constant;           /* PLL (phase-locked loop) time constant */
> +    abi_long precision;          /* Clock precision (microseconds, ro) */
> +    abi_long tolerance;          /* Clock freq. tolerance (ppm, ro) */
> +    struct target_timeval time;  /* Current time */
> +    abi_long tick;               /* Microseconds between clock ticks */
> +    abi_long ppsfreq;            /* PPS (pulse per second) frequency */
> +    abi_long jitter;             /* PPS jitter (ro); nanoseconds */
> +    int shift;                   /* PPS interval duration (seconds) */
> +    abi_long stabil;             /* PPS stability */
> +    abi_long jitcnt;             /* PPS jitter limit exceeded (ro) */
> +    abi_long calcnt;             /* PPS calibration intervals */
> +    abi_long errcnt;             /* PPS calibration errors */
> +    abi_long stbcnt;             /* PPS stability limit exceeded */
> +    int tai;                     /* TAI offset */
> +
> +    /* Further padding bytes to allow for future expansion */
> +    int:32; int:32; int:32; int:32;
> +    int:32; int:32; int:32; int:32;
> +    int:32; int:32; int:32;
> +};
> +

for the alignment purpose, use abi_uint/abi_int instead of uint/int.

>  typedef abi_long target_clock_t;
>  
>  #define TARGET_HZ 100
> 

Thanks,
Laurent
Laurent Vivier Sept. 17, 2016, 10:38 p.m. UTC | #2
Le 17/09/2016 à 20:40, Laurent Vivier a écrit :
> 
> 
> Le 14/09/2016 à 22:19, Aleksandar Markovic a écrit :

>>  static inline abi_long target_to_host_timespec(struct timespec *host_ts,
>>                                                 abi_ulong target_addr)
>>  {
>> @@ -9419,8 +9492,23 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>>          break;
>>  #endif
>>  #endif
>> +#ifdef TARGET_NR_adjtimex
>>      case TARGET_NR_adjtimex:
>> -        goto unimplemented;
>> +        {
>> +            struct timex host_buf;
>> +
> 
> check arg1 != NULL, so you manage target_to_host and host_to_target NULL
> case.

In fact, I think you should not check that, it will be managed by
lock_user_struct().

Laurent
Riku Voipio Sept. 22, 2016, 4:42 a.m. UTC | #3
On Wed, Sep 14, 2016 at 10:19:54PM +0200, Aleksandar Markovic wrote:
> From: Aleksandar Markovic <aleksandar.markovic@imgtec.com>
> 
> This patch implements Qemu user mode adjtimex() syscall support.
> 
> Syscall adjtimex() reads and optionally sets parameters for a clock
> adjustment algorithm used in network synchonization or similar scenarios.
> 
> The implementation is based on invocation of host's adjtimex(), and
> its key part is in the correspondent case segment of the main switch
> statement of the function do_syscall(), in file linux-user/syscalls.c.
> Also, support for related structure "timex" is added to the file
> linux-user/syscall_defs.h, based on its definition in Linux kernel. All
> necessary conversions of the data structures from target to host and from
> host to target are covered. Two new functions, target_to_host_timex() and
> host_to_target_timex(), are provided for the purpose of such conversions.
> Moreover, the relevant support for "-strace" Qemu option is included in
> files linux-user/strace.c and linux-user/strace.list.
> 
> This patch also fixes failures of LTP tests adjtimex01 and adjtimex02, if
> executed in Qemu user mode.
> 
> Signed-off-by: Aleksandar Rikalo <aleksandar.rikalo@imgtec.com>
> Signed-off-by: Aleksandar Markovic <aleksandar.markovic@imgtec.com>
> ---
>  linux-user/strace.c       | 12 +++++++
>  linux-user/strace.list    |  2 +-
>  linux-user/syscall.c      | 90 ++++++++++++++++++++++++++++++++++++++++++++++-
>  linux-user/syscall_defs.h | 28 +++++++++++++++
>  4 files changed, 130 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/strace.c b/linux-user/strace.c
> index cc10dc4..7ddcaf8 100644
> --- a/linux-user/strace.c
> +++ b/linux-user/strace.c
> @@ -919,6 +919,18 @@ print_access(const struct syscallname *name,
>  }
>  #endif
>  
> +#ifdef TARGET_NR_adjtimex

The ifdef isn't needed, all linux platorms support adjtimex.

> +static void
> +print_adjtimex(const struct syscallname *name,
> +    abi_long arg0, abi_long arg1, abi_long arg2,
> +    abi_long arg3, abi_long arg4, abi_long arg5)
> +{
> +    print_syscall_prologue(name);
> +    print_pointer(arg0, 1);
> +    print_syscall_epilogue(name);
> +}
> +#endif
> +
>  #ifdef TARGET_NR_brk
>  static void
>  print_brk(const struct syscallname *name,
> diff --git a/linux-user/strace.list b/linux-user/strace.list
> index aa967a2..9a665a8 100644
> --- a/linux-user/strace.list
> +++ b/linux-user/strace.list
> @@ -16,7 +16,7 @@
>  { TARGET_NR_add_key, "add_key" , NULL, NULL, NULL },
>  #endif
>  #ifdef TARGET_NR_adjtimex
> -{ TARGET_NR_adjtimex, "adjtimex" , NULL, NULL, NULL },
> +{ TARGET_NR_adjtimex, "adjtimex" , NULL, print_adjtimex, NULL },
>  #endif
>  #ifdef TARGET_NR_afs_syscall
>  { TARGET_NR_afs_syscall, "afs_syscall" , NULL, NULL, NULL },
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index ca06943..5643840 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -35,6 +35,7 @@
>  #include <sys/swap.h>
>  #include <linux/capability.h>
>  #include <sched.h>
> +#include <sys/timex.h>
>  #ifdef __ia64__
>  int __clone2(int (*fn)(void *), void *child_stack_base,
>               size_t stack_size, int flags, void *arg, ...);
> @@ -6578,6 +6579,78 @@ static inline abi_long target_ftruncate64(void *cpu_env, abi_long arg1,
>  }
>  #endif
>  
> +#ifdef TARGET_NR_adjtimex
> +static inline abi_long target_to_host_timex(struct timex *host_buf,
> +                                            abi_long target_addr)
> +{
> +    struct target_timex *target_buf;
> +
> +    if (!lock_user_struct(VERIFY_READ, target_buf, target_addr, 1)) {
> +        return -TARGET_EFAULT;
> +    }
> +
> +    host_buf->modes = tswap32(target_buf->modes);
> +    host_buf->offset = tswapal(target_buf->offset);
> +    host_buf->freq = tswapal(target_buf->freq);
> +    host_buf->maxerror = tswapal(target_buf->maxerror);
> +    host_buf->esterror = tswapal(target_buf->esterror);
> +    host_buf->status = tswap32(target_buf->status);
> +    host_buf->constant = tswapal(target_buf->constant);
> +    host_buf->precision = tswapal(target_buf->precision);
> +    host_buf->tolerance = tswapal(target_buf->tolerance);
> +    host_buf->time.tv_sec = tswapal(target_buf->time.tv_sec);
> +    host_buf->time.tv_usec = tswapal(target_buf->time.tv_usec);
> +    host_buf->tick = tswapal(target_buf->tick);
> +    host_buf->ppsfreq = tswapal(target_buf->ppsfreq);
> +    host_buf->jitter = tswapal(target_buf->jitter);
> +    host_buf->shift = tswap32(target_buf->shift);
> +    host_buf->stabil = tswapal(target_buf->stabil);
> +    host_buf->jitcnt = tswapal(target_buf->jitcnt);
> +    host_buf->calcnt = tswapal(target_buf->calcnt);
> +    host_buf->errcnt = tswapal(target_buf->errcnt);
> +    host_buf->stbcnt = tswapal(target_buf->stbcnt);
> +    host_buf->tai = tswap32(target_buf->tai);
> +
> +    unlock_user_struct(target_buf, target_addr, 0);
> +    return 0;
> +}
> +
> +static inline abi_long host_to_target_timex(abi_long target_addr,
> +                                            struct timex *host_buf)
> +{
> +    struct target_timex *target_buf;
> +
> +    if (!lock_user_struct(VERIFY_WRITE, target_buf, target_addr, 0)) {
> +        return -TARGET_EFAULT;
> +    }
> +
> +    target_buf->modes = tswap32(host_buf->modes);
> +    target_buf->offset = tswapal(host_buf->offset);
> +    target_buf->freq = tswapal(host_buf->freq);
> +    target_buf->maxerror = tswapal(host_buf->maxerror);
> +    target_buf->esterror = tswapal(host_buf->esterror);
> +    target_buf->status = tswap32(host_buf->status);
> +    target_buf->constant = tswapal(host_buf->constant);
> +    target_buf->precision = tswapal(host_buf->precision);
> +    target_buf->tolerance = tswapal(host_buf->tolerance);
> +    target_buf->time.tv_sec = tswapal(host_buf->time.tv_sec);
> +    target_buf->time.tv_usec = tswapal(host_buf->time.tv_usec);
> +    target_buf->tick = tswapal(host_buf->tick);
> +    target_buf->ppsfreq = tswapal(host_buf->ppsfreq);
> +    target_buf->jitter = tswapal(host_buf->jitter);
> +    target_buf->shift = tswap32(host_buf->shift);
> +    target_buf->stabil = tswapal(host_buf->stabil);
> +    target_buf->jitcnt = tswapal(host_buf->jitcnt);
> +    target_buf->calcnt = tswapal(host_buf->calcnt);
> +    target_buf->errcnt = tswapal(host_buf->errcnt);
> +    target_buf->stbcnt = tswapal(host_buf->stbcnt);
> +    target_buf->tai = tswap32(host_buf->tai);
> +
> +    unlock_user_struct(target_buf, target_addr, 1);
> +    return 0;
> +}
> +#endif
> +
>  static inline abi_long target_to_host_timespec(struct timespec *host_ts,
>                                                 abi_ulong target_addr)
>  {
> @@ -9419,8 +9492,23 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>          break;
>  #endif
>  #endif
> +#ifdef TARGET_NR_adjtimex
>      case TARGET_NR_adjtimex:
> -        goto unimplemented;
> +        {
> +            struct timex host_buf;
> +
> +            if (target_to_host_timex(&host_buf, arg1) != 0) {
> +                goto efault;
> +            }
> +            ret = get_errno(adjtimex(&host_buf));
> +            if (!is_error(ret) && arg1) {
> +                if (host_to_target_timex(arg1, &host_buf) != 0) {
> +                    goto efault;
> +                }
> +            }
> +        }
> +        break;
> +#endif
>  #ifdef TARGET_NR_create_module
>      case TARGET_NR_create_module:
>  #endif
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 7835654..afd9191 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -207,6 +207,34 @@ struct target_itimerspec {
>      struct target_timespec it_value;
>  };
>  
> +struct target_timex {
> +    unsigned int modes;          /* Mode selector */
> +    abi_long offset;             /* Time offset */
> +    abi_long freq;               /* Frequency offset */
> +    abi_long maxerror;           /* Maximum error (microseconds) */
> +    abi_long esterror;           /* Estimated error (microseconds) */
> +    int status;                  /* Clock command/status */
> +    abi_long constant;           /* PLL (phase-locked loop) time constant */
> +    abi_long precision;          /* Clock precision (microseconds, ro) */
> +    abi_long tolerance;          /* Clock freq. tolerance (ppm, ro) */
> +    struct target_timeval time;  /* Current time */
> +    abi_long tick;               /* Microseconds between clock ticks */
> +    abi_long ppsfreq;            /* PPS (pulse per second) frequency */
> +    abi_long jitter;             /* PPS jitter (ro); nanoseconds */
> +    int shift;                   /* PPS interval duration (seconds) */
> +    abi_long stabil;             /* PPS stability */
> +    abi_long jitcnt;             /* PPS jitter limit exceeded (ro) */
> +    abi_long calcnt;             /* PPS calibration intervals */
> +    abi_long errcnt;             /* PPS calibration errors */
> +    abi_long stbcnt;             /* PPS stability limit exceeded */
> +    int tai;                     /* TAI offset */
> +
> +    /* Further padding bytes to allow for future expansion */
> +    int:32; int:32; int:32; int:32;
> +    int:32; int:32; int:32; int:32;
> +    int:32; int:32; int:32;
> +};
> +
>  typedef abi_long target_clock_t;
>  
>  #define TARGET_HZ 100
> -- 
> 2.9.3
> 
>
Aleksandar Markovic Sept. 22, 2016, 7:05 a.m. UTC | #4
Hi, Riku,

Thanks a lots for your review.

I have no problem deleting "#ifdef TARGET_NR_adjtimex" in strace.c.

However, I want to bring to your attention the case of future target Linux platforms seizing supporting adjtimex(). Let's say this reflects in such a way that TARGET_NR_adjtimex is not defined for such platform. Then we will have a compiler error that print_adjtimex() is unused. With "ifdef", we wouldn't.

Thanks,
Aleksandar
Riku Voipio Sept. 22, 2016, 8:12 a.m. UTC | #5
On Thu, Sep 22, 2016 at 07:05:11AM +0000, Aleksandar Markovic wrote:
> Hi, Riku,
> 
> Thanks a lots for your review.
> 
> I have no problem deleting "#ifdef TARGET_NR_adjtimex" in strace.c.
> 
> However, I want to bring to your attention the case of future target Linux platforms seizing supporting adjtimex(). Let's say this reflects in such a way that TARGET_NR_adjtimex is not defined for such platform. Then we will have a compiler error that print_adjtimex() is unused. With "ifdef", we wouldn't.
> 

To add a new plaform to qemu, changes to the code are needed anyways, so
adding an ifdef then isn't a problem. The #ifdef TARGET_NR_ guards are
useful for 1) legacy syscalls that new platforms don't support, or 2) new
syscalls that old platforms don't support (yet). adjtimex may be deprecated
on day, but even the new platforms (aarch64, ppc64le) have opted to support
it.

> Thanks,
> Aleksandar
> 
> ________________________________________
> From: Riku Voipio [riku.voipio@iki.fi]
> Sent: Wednesday, September 21, 2016 9:42 PM
> To: Aleksandar Markovic
> Cc: qemu-devel@nongnu.org; peter.maydell@linaro.org; Petar Jovanovic; Miodrag Dinic; Aleksandar Rikalo; Aleksandar Markovic
> Subject: Re: [Qemu-devel] [PATCH v5 1/8] linux-user: Add support for adjtimex() syscall
> 
> On Wed, Sep 14, 2016 at 10:19:54PM +0200, Aleksandar Markovic wrote:
> > From: Aleksandar Markovic <aleksandar.markovic@imgtec.com>
> >
> > This patch implements Qemu user mode adjtimex() syscall support.
> >
> > Syscall adjtimex() reads and optionally sets parameters for a clock
> > adjustment algorithm used in network synchonization or similar scenarios.
> >
> > The implementation is based on invocation of host's adjtimex(), and
> > its key part is in the correspondent case segment of the main switch
> > statement of the function do_syscall(), in file linux-user/syscalls.c.
> > Also, support for related structure "timex" is added to the file
> > linux-user/syscall_defs.h, based on its definition in Linux kernel. All
> > necessary conversions of the data structures from target to host and from
> > host to target are covered. Two new functions, target_to_host_timex() and
> > host_to_target_timex(), are provided for the purpose of such conversions.
> > Moreover, the relevant support for "-strace" Qemu option is included in
> > files linux-user/strace.c and linux-user/strace.list.
> >
> > This patch also fixes failures of LTP tests adjtimex01 and adjtimex02, if
> > executed in Qemu user mode.
> >
> > Signed-off-by: Aleksandar Rikalo <aleksandar.rikalo@imgtec.com>
> > Signed-off-by: Aleksandar Markovic <aleksandar.markovic@imgtec.com>
> > ---
> >  linux-user/strace.c       | 12 +++++++
> >  linux-user/strace.list    |  2 +-
> >  linux-user/syscall.c      | 90 ++++++++++++++++++++++++++++++++++++++++++++++-
> >  linux-user/syscall_defs.h | 28 +++++++++++++++
> >  4 files changed, 130 insertions(+), 2 deletions(-)
> >
> > diff --git a/linux-user/strace.c b/linux-user/strace.c
> > index cc10dc4..7ddcaf8 100644
> > --- a/linux-user/strace.c
> > +++ b/linux-user/strace.c
> > @@ -919,6 +919,18 @@ print_access(const struct syscallname *name,
> >  }
> >  #endif
> >
> > +#ifdef TARGET_NR_adjtimex
> 
> The ifdef isn't needed, all linux platorms support adjtimex.
> 
> > +static void
> > +print_adjtimex(const struct syscallname *name,
> > +    abi_long arg0, abi_long arg1, abi_long arg2,
> > +    abi_long arg3, abi_long arg4, abi_long arg5)
> > +{
> > +    print_syscall_prologue(name);
> > +    print_pointer(arg0, 1);
> > +    print_syscall_epilogue(name);
> > +}
> > +#endif
> > +
> >  #ifdef TARGET_NR_brk
> >  static void
> >  print_brk(const struct syscallname *name,
> > diff --git a/linux-user/strace.list b/linux-user/strace.list
> > index aa967a2..9a665a8 100644
> > --- a/linux-user/strace.list
> > +++ b/linux-user/strace.list
> > @@ -16,7 +16,7 @@
> >  { TARGET_NR_add_key, "add_key" , NULL, NULL, NULL },
> >  #endif
> >  #ifdef TARGET_NR_adjtimex
> > -{ TARGET_NR_adjtimex, "adjtimex" , NULL, NULL, NULL },
> > +{ TARGET_NR_adjtimex, "adjtimex" , NULL, print_adjtimex, NULL },
> >  #endif
> >  #ifdef TARGET_NR_afs_syscall
> >  { TARGET_NR_afs_syscall, "afs_syscall" , NULL, NULL, NULL },
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index ca06943..5643840 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -35,6 +35,7 @@
> >  #include <sys/swap.h>
> >  #include <linux/capability.h>
> >  #include <sched.h>
> > +#include <sys/timex.h>
> >  #ifdef __ia64__
> >  int __clone2(int (*fn)(void *), void *child_stack_base,
> >               size_t stack_size, int flags, void *arg, ...);
> > @@ -6578,6 +6579,78 @@ static inline abi_long target_ftruncate64(void *cpu_env, abi_long arg1,
> >  }
> >  #endif
> >
> > +#ifdef TARGET_NR_adjtimex
> > +static inline abi_long target_to_host_timex(struct timex *host_buf,
> > +                                            abi_long target_addr)
> > +{
> > +    struct target_timex *target_buf;
> > +
> > +    if (!lock_user_struct(VERIFY_READ, target_buf, target_addr, 1)) {
> > +        return -TARGET_EFAULT;
> > +    }
> > +
> > +    host_buf->modes = tswap32(target_buf->modes);
> > +    host_buf->offset = tswapal(target_buf->offset);
> > +    host_buf->freq = tswapal(target_buf->freq);
> > +    host_buf->maxerror = tswapal(target_buf->maxerror);
> > +    host_buf->esterror = tswapal(target_buf->esterror);
> > +    host_buf->status = tswap32(target_buf->status);
> > +    host_buf->constant = tswapal(target_buf->constant);
> > +    host_buf->precision = tswapal(target_buf->precision);
> > +    host_buf->tolerance = tswapal(target_buf->tolerance);
> > +    host_buf->time.tv_sec = tswapal(target_buf->time.tv_sec);
> > +    host_buf->time.tv_usec = tswapal(target_buf->time.tv_usec);
> > +    host_buf->tick = tswapal(target_buf->tick);
> > +    host_buf->ppsfreq = tswapal(target_buf->ppsfreq);
> > +    host_buf->jitter = tswapal(target_buf->jitter);
> > +    host_buf->shift = tswap32(target_buf->shift);
> > +    host_buf->stabil = tswapal(target_buf->stabil);
> > +    host_buf->jitcnt = tswapal(target_buf->jitcnt);
> > +    host_buf->calcnt = tswapal(target_buf->calcnt);
> > +    host_buf->errcnt = tswapal(target_buf->errcnt);
> > +    host_buf->stbcnt = tswapal(target_buf->stbcnt);
> > +    host_buf->tai = tswap32(target_buf->tai);
> > +
> > +    unlock_user_struct(target_buf, target_addr, 0);
> > +    return 0;
> > +}
> > +
> > +static inline abi_long host_to_target_timex(abi_long target_addr,
> > +                                            struct timex *host_buf)
> > +{
> > +    struct target_timex *target_buf;
> > +
> > +    if (!lock_user_struct(VERIFY_WRITE, target_buf, target_addr, 0)) {
> > +        return -TARGET_EFAULT;
> > +    }
> > +
> > +    target_buf->modes = tswap32(host_buf->modes);
> > +    target_buf->offset = tswapal(host_buf->offset);
> > +    target_buf->freq = tswapal(host_buf->freq);
> > +    target_buf->maxerror = tswapal(host_buf->maxerror);
> > +    target_buf->esterror = tswapal(host_buf->esterror);
> > +    target_buf->status = tswap32(host_buf->status);
> > +    target_buf->constant = tswapal(host_buf->constant);
> > +    target_buf->precision = tswapal(host_buf->precision);
> > +    target_buf->tolerance = tswapal(host_buf->tolerance);
> > +    target_buf->time.tv_sec = tswapal(host_buf->time.tv_sec);
> > +    target_buf->time.tv_usec = tswapal(host_buf->time.tv_usec);
> > +    target_buf->tick = tswapal(host_buf->tick);
> > +    target_buf->ppsfreq = tswapal(host_buf->ppsfreq);
> > +    target_buf->jitter = tswapal(host_buf->jitter);
> > +    target_buf->shift = tswap32(host_buf->shift);
> > +    target_buf->stabil = tswapal(host_buf->stabil);
> > +    target_buf->jitcnt = tswapal(host_buf->jitcnt);
> > +    target_buf->calcnt = tswapal(host_buf->calcnt);
> > +    target_buf->errcnt = tswapal(host_buf->errcnt);
> > +    target_buf->stbcnt = tswapal(host_buf->stbcnt);
> > +    target_buf->tai = tswap32(host_buf->tai);
> > +
> > +    unlock_user_struct(target_buf, target_addr, 1);
> > +    return 0;
> > +}
> > +#endif
> > +
> >  static inline abi_long target_to_host_timespec(struct timespec *host_ts,
> >                                                 abi_ulong target_addr)
> >  {
> > @@ -9419,8 +9492,23 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
> >          break;
> >  #endif
> >  #endif
> > +#ifdef TARGET_NR_adjtimex
> >      case TARGET_NR_adjtimex:
> > -        goto unimplemented;
> > +        {
> > +            struct timex host_buf;
> > +
> > +            if (target_to_host_timex(&host_buf, arg1) != 0) {
> > +                goto efault;
> > +            }
> > +            ret = get_errno(adjtimex(&host_buf));
> > +            if (!is_error(ret) && arg1) {
> > +                if (host_to_target_timex(arg1, &host_buf) != 0) {
> > +                    goto efault;
> > +                }
> > +            }
> > +        }
> > +        break;
> > +#endif
> >  #ifdef TARGET_NR_create_module
> >      case TARGET_NR_create_module:
> >  #endif
> > diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> > index 7835654..afd9191 100644
> > --- a/linux-user/syscall_defs.h
> > +++ b/linux-user/syscall_defs.h
> > @@ -207,6 +207,34 @@ struct target_itimerspec {
> >      struct target_timespec it_value;
> >  };
> >
> > +struct target_timex {
> > +    unsigned int modes;          /* Mode selector */
> > +    abi_long offset;             /* Time offset */
> > +    abi_long freq;               /* Frequency offset */
> > +    abi_long maxerror;           /* Maximum error (microseconds) */
> > +    abi_long esterror;           /* Estimated error (microseconds) */
> > +    int status;                  /* Clock command/status */
> > +    abi_long constant;           /* PLL (phase-locked loop) time constant */
> > +    abi_long precision;          /* Clock precision (microseconds, ro) */
> > +    abi_long tolerance;          /* Clock freq. tolerance (ppm, ro) */
> > +    struct target_timeval time;  /* Current time */
> > +    abi_long tick;               /* Microseconds between clock ticks */
> > +    abi_long ppsfreq;            /* PPS (pulse per second) frequency */
> > +    abi_long jitter;             /* PPS jitter (ro); nanoseconds */
> > +    int shift;                   /* PPS interval duration (seconds) */
> > +    abi_long stabil;             /* PPS stability */
> > +    abi_long jitcnt;             /* PPS jitter limit exceeded (ro) */
> > +    abi_long calcnt;             /* PPS calibration intervals */
> > +    abi_long errcnt;             /* PPS calibration errors */
> > +    abi_long stbcnt;             /* PPS stability limit exceeded */
> > +    int tai;                     /* TAI offset */
> > +
> > +    /* Further padding bytes to allow for future expansion */
> > +    int:32; int:32; int:32; int:32;
> > +    int:32; int:32; int:32; int:32;
> > +    int:32; int:32; int:32;
> > +};
> > +
> >  typedef abi_long target_clock_t;
> >
> >  #define TARGET_HZ 100
> > --
> > 2.9.3
> >
> >
Aleksandar Markovic Sept. 22, 2016, 12:28 p.m. UTC | #6
Hi, Riku,

I really appreciate your clarification. I am going to address your concern in the next version of this series, which is planned to be posted in a short time.

Thank,
Aleksandar
diff mbox

Patch

diff --git a/linux-user/strace.c b/linux-user/strace.c
index cc10dc4..7ddcaf8 100644
--- a/linux-user/strace.c
+++ b/linux-user/strace.c
@@ -919,6 +919,18 @@  print_access(const struct syscallname *name,
 }
 #endif
 
+#ifdef TARGET_NR_adjtimex
+static void
+print_adjtimex(const struct syscallname *name,
+    abi_long arg0, abi_long arg1, abi_long arg2,
+    abi_long arg3, abi_long arg4, abi_long arg5)
+{
+    print_syscall_prologue(name);
+    print_pointer(arg0, 1);
+    print_syscall_epilogue(name);
+}
+#endif
+
 #ifdef TARGET_NR_brk
 static void
 print_brk(const struct syscallname *name,
diff --git a/linux-user/strace.list b/linux-user/strace.list
index aa967a2..9a665a8 100644
--- a/linux-user/strace.list
+++ b/linux-user/strace.list
@@ -16,7 +16,7 @@ 
 { TARGET_NR_add_key, "add_key" , NULL, NULL, NULL },
 #endif
 #ifdef TARGET_NR_adjtimex
-{ TARGET_NR_adjtimex, "adjtimex" , NULL, NULL, NULL },
+{ TARGET_NR_adjtimex, "adjtimex" , NULL, print_adjtimex, NULL },
 #endif
 #ifdef TARGET_NR_afs_syscall
 { TARGET_NR_afs_syscall, "afs_syscall" , NULL, NULL, NULL },
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index ca06943..5643840 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -35,6 +35,7 @@ 
 #include <sys/swap.h>
 #include <linux/capability.h>
 #include <sched.h>
+#include <sys/timex.h>
 #ifdef __ia64__
 int __clone2(int (*fn)(void *), void *child_stack_base,
              size_t stack_size, int flags, void *arg, ...);
@@ -6578,6 +6579,78 @@  static inline abi_long target_ftruncate64(void *cpu_env, abi_long arg1,
 }
 #endif
 
+#ifdef TARGET_NR_adjtimex
+static inline abi_long target_to_host_timex(struct timex *host_buf,
+                                            abi_long target_addr)
+{
+    struct target_timex *target_buf;
+
+    if (!lock_user_struct(VERIFY_READ, target_buf, target_addr, 1)) {
+        return -TARGET_EFAULT;
+    }
+
+    host_buf->modes = tswap32(target_buf->modes);
+    host_buf->offset = tswapal(target_buf->offset);
+    host_buf->freq = tswapal(target_buf->freq);
+    host_buf->maxerror = tswapal(target_buf->maxerror);
+    host_buf->esterror = tswapal(target_buf->esterror);
+    host_buf->status = tswap32(target_buf->status);
+    host_buf->constant = tswapal(target_buf->constant);
+    host_buf->precision = tswapal(target_buf->precision);
+    host_buf->tolerance = tswapal(target_buf->tolerance);
+    host_buf->time.tv_sec = tswapal(target_buf->time.tv_sec);
+    host_buf->time.tv_usec = tswapal(target_buf->time.tv_usec);
+    host_buf->tick = tswapal(target_buf->tick);
+    host_buf->ppsfreq = tswapal(target_buf->ppsfreq);
+    host_buf->jitter = tswapal(target_buf->jitter);
+    host_buf->shift = tswap32(target_buf->shift);
+    host_buf->stabil = tswapal(target_buf->stabil);
+    host_buf->jitcnt = tswapal(target_buf->jitcnt);
+    host_buf->calcnt = tswapal(target_buf->calcnt);
+    host_buf->errcnt = tswapal(target_buf->errcnt);
+    host_buf->stbcnt = tswapal(target_buf->stbcnt);
+    host_buf->tai = tswap32(target_buf->tai);
+
+    unlock_user_struct(target_buf, target_addr, 0);
+    return 0;
+}
+
+static inline abi_long host_to_target_timex(abi_long target_addr,
+                                            struct timex *host_buf)
+{
+    struct target_timex *target_buf;
+
+    if (!lock_user_struct(VERIFY_WRITE, target_buf, target_addr, 0)) {
+        return -TARGET_EFAULT;
+    }
+
+    target_buf->modes = tswap32(host_buf->modes);
+    target_buf->offset = tswapal(host_buf->offset);
+    target_buf->freq = tswapal(host_buf->freq);
+    target_buf->maxerror = tswapal(host_buf->maxerror);
+    target_buf->esterror = tswapal(host_buf->esterror);
+    target_buf->status = tswap32(host_buf->status);
+    target_buf->constant = tswapal(host_buf->constant);
+    target_buf->precision = tswapal(host_buf->precision);
+    target_buf->tolerance = tswapal(host_buf->tolerance);
+    target_buf->time.tv_sec = tswapal(host_buf->time.tv_sec);
+    target_buf->time.tv_usec = tswapal(host_buf->time.tv_usec);
+    target_buf->tick = tswapal(host_buf->tick);
+    target_buf->ppsfreq = tswapal(host_buf->ppsfreq);
+    target_buf->jitter = tswapal(host_buf->jitter);
+    target_buf->shift = tswap32(host_buf->shift);
+    target_buf->stabil = tswapal(host_buf->stabil);
+    target_buf->jitcnt = tswapal(host_buf->jitcnt);
+    target_buf->calcnt = tswapal(host_buf->calcnt);
+    target_buf->errcnt = tswapal(host_buf->errcnt);
+    target_buf->stbcnt = tswapal(host_buf->stbcnt);
+    target_buf->tai = tswap32(host_buf->tai);
+
+    unlock_user_struct(target_buf, target_addr, 1);
+    return 0;
+}
+#endif
+
 static inline abi_long target_to_host_timespec(struct timespec *host_ts,
                                                abi_ulong target_addr)
 {
@@ -9419,8 +9492,23 @@  abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         break;
 #endif
 #endif
+#ifdef TARGET_NR_adjtimex
     case TARGET_NR_adjtimex:
-        goto unimplemented;
+        {
+            struct timex host_buf;
+
+            if (target_to_host_timex(&host_buf, arg1) != 0) {
+                goto efault;
+            }
+            ret = get_errno(adjtimex(&host_buf));
+            if (!is_error(ret) && arg1) {
+                if (host_to_target_timex(arg1, &host_buf) != 0) {
+                    goto efault;
+                }
+            }
+        }
+        break;
+#endif
 #ifdef TARGET_NR_create_module
     case TARGET_NR_create_module:
 #endif
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 7835654..afd9191 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -207,6 +207,34 @@  struct target_itimerspec {
     struct target_timespec it_value;
 };
 
+struct target_timex {
+    unsigned int modes;          /* Mode selector */
+    abi_long offset;             /* Time offset */
+    abi_long freq;               /* Frequency offset */
+    abi_long maxerror;           /* Maximum error (microseconds) */
+    abi_long esterror;           /* Estimated error (microseconds) */
+    int status;                  /* Clock command/status */
+    abi_long constant;           /* PLL (phase-locked loop) time constant */
+    abi_long precision;          /* Clock precision (microseconds, ro) */
+    abi_long tolerance;          /* Clock freq. tolerance (ppm, ro) */
+    struct target_timeval time;  /* Current time */
+    abi_long tick;               /* Microseconds between clock ticks */
+    abi_long ppsfreq;            /* PPS (pulse per second) frequency */
+    abi_long jitter;             /* PPS jitter (ro); nanoseconds */
+    int shift;                   /* PPS interval duration (seconds) */
+    abi_long stabil;             /* PPS stability */
+    abi_long jitcnt;             /* PPS jitter limit exceeded (ro) */
+    abi_long calcnt;             /* PPS calibration intervals */
+    abi_long errcnt;             /* PPS calibration errors */
+    abi_long stbcnt;             /* PPS stability limit exceeded */
+    int tai;                     /* TAI offset */
+
+    /* Further padding bytes to allow for future expansion */
+    int:32; int:32; int:32; int:32;
+    int:32; int:32; int:32; int:32;
+    int:32; int:32; int:32;
+};
+
 typedef abi_long target_clock_t;
 
 #define TARGET_HZ 100