* [PULL 0/1] Linux user for 5.0 patches
@ 2020-03-30 10:29 Laurent Vivier
2020-03-30 10:29 ` [PULL 1/1] linux-user: Support futex_time64 Laurent Vivier
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Laurent Vivier @ 2020-03-30 10:29 UTC (permalink / raw)
To: qemu-devel; +Cc: Riku Voipio, Laurent Vivier
The following changes since commit cfe68ae025f704f336d7dd3d1903ce37b445831d:
Merge remote-tracking branch 'remotes/vivier2/tags/linux-user-for-5.0-pull-request' into staging (2020-03-26 20:55:54 +0000)
are available in the Git repository at:
git://github.com/vivier/qemu.git tags/linux-user-for-5.0-pull-request
for you to fetch changes up to 146902964d66881da797e45a5950732e38532e3e:
linux-user: Support futex_time64 (2020-03-30 12:28:30 +0200)
----------------------------------------------------------------
Add support for futex_time64
----------------------------------------------------------------
Alistair Francis (1):
linux-user: Support futex_time64
linux-user/syscall.c | 140 ++++++++++++++++++++++++++++++++++++++-----
1 file changed, 126 insertions(+), 14 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PULL 1/1] linux-user: Support futex_time64
2020-03-30 10:29 [PULL 0/1] Linux user for 5.0 patches Laurent Vivier
@ 2020-03-30 10:29 ` Laurent Vivier
2020-11-02 18:15 ` Peter Maydell
2020-03-30 11:32 ` [PULL 0/1] Linux user for 5.0 patches no-reply
2020-03-30 12:14 ` Peter Maydell
2 siblings, 1 reply; 9+ messages in thread
From: Laurent Vivier @ 2020-03-30 10:29 UTC (permalink / raw)
To: qemu-devel; +Cc: Riku Voipio, Alistair Francis, Laurent Vivier
From: Alistair Francis <alistair.francis@wdc.com>
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 futex
syscall.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Message-Id: <d9390e368a9a1fd32d52aa771815e6e3d40cb1d4.1584571250.git.alistair.francis@wdc.com>
[lv: define sys_futex() if __NR_futex is defined (fix bug on 32bit host),
remove duplicate get_errno()]
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
linux-user/syscall.c | 140 ++++++++++++++++++++++++++++++++++++++-----
1 file changed, 126 insertions(+), 14 deletions(-)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 49395dcea978..5af55fca7811 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
#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,14 @@ _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(TARGET_NR_exit)) && defined(__NR_futex)
+#if defined(__NR_futex)
_syscall6(int,sys_futex,int *,uaddr,int,op,int,val,
const struct timespec *,timeout,int *,uaddr2,int,val3)
#endif
+#if defined(__NR_futex_time64)
+_syscall6(int,sys_futex_time64,int *,uaddr,int,op,int,val,
+ const struct timespec *,timeout,int *,uaddr2,int,val3)
+#endif
#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 +771,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
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)
@@ -1229,7 +1242,7 @@ static inline abi_long target_to_host_timespec(struct timespec *host_ts,
}
#endif
-#if defined(TARGET_NR_clock_settime64)
+#if defined(TARGET_NR_clock_settime64) || defined(TARGET_NR_futex_time64)
static inline abi_long target_to_host_timespec64(struct timespec *host_ts,
abi_ulong target_addr)
{
@@ -6916,6 +6929,55 @@ static inline abi_long host_to_target_statx(struct target_statx *host_stx,
}
#endif
+static int do_sys_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 sys_futex(uaddr, op, val, timeout, uaddr2, val3);
+
+#endif
+#else /* HOST_LONG_BITS == 64 */
+#if defined(__NR_futex_time64)
+ if (sizeof(timeout->tv_sec) == 8) {
+ /* _time64 function on 32bit arch */
+ return sys_futex_time64(uaddr, op, val, timeout, uaddr2, val3);
+ }
+#endif
+#if defined(__NR_futex)
+ /* old function on 32bit arch */
+ return sys_futex(uaddr, op, val, timeout, uaddr2, val3);
+#endif
+#endif /* HOST_LONG_BITS == 64 */
+ g_assert_not_reached();
+}
+
+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 /* HOST_LONG_BITS == 64 */
+#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 /* HOST_LONG_BITS == 64 */
+ return -TARGET_ENOSYS;
+}
/* ??? Using host futex calls even when target atomic operations
are not really atomic probably breaks things. However implementing
@@ -6945,12 +7007,11 @@ static int do_futex(target_ulong uaddr, int op, int val, target_ulong timeout,
} else {
pts = NULL;
}
- return get_errno(safe_futex(g2h(uaddr), op, tswap32(val),
- pts, NULL, val3));
+ return do_safe_futex(g2h(uaddr), op, tswap32(val), pts, NULL, val3);
case FUTEX_WAKE:
- return get_errno(safe_futex(g2h(uaddr), op, val, NULL, NULL, 0));
+ return do_safe_futex(g2h(uaddr), op, val, NULL, NULL, 0);
case FUTEX_FD:
- return get_errno(safe_futex(g2h(uaddr), op, val, NULL, NULL, 0));
+ return do_safe_futex(g2h(uaddr), op, val, NULL, NULL, 0);
case FUTEX_REQUEUE:
case FUTEX_CMP_REQUEUE:
case FUTEX_WAKE_OP:
@@ -6960,16 +7021,63 @@ static int do_futex(target_ulong uaddr, int op, int val, target_ulong timeout,
to satisfy the compiler. We do not need to tswap TIMEOUT
since it's not compared to guest memory. */
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)));
+ return do_safe_futex(g2h(uaddr), op, val, pts, g2h(uaddr2),
+ (base_op == FUTEX_CMP_REQUEUE
+ ? tswap32(val3)
+ : val3));
default:
return -TARGET_ENOSYS;
}
}
#endif
+
+#if defined(TARGET_NR_futex_time64)
+static int do_futex_time64(target_ulong uaddr, int op, int val, target_ulong timeout,
+ target_ulong uaddr2, int val3)
+{
+ struct timespec ts, *pts;
+ int base_op;
+
+ /* ??? We assume FUTEX_* constants are the same on both host
+ and target. */
+#ifdef FUTEX_CMD_MASK
+ base_op = op & FUTEX_CMD_MASK;
+#else
+ base_op = op;
+#endif
+ switch (base_op) {
+ case FUTEX_WAIT:
+ case FUTEX_WAIT_BITSET:
+ if (timeout) {
+ pts = &ts;
+ target_to_host_timespec64(pts, timeout);
+ } else {
+ pts = NULL;
+ }
+ return do_safe_futex(g2h(uaddr), op, tswap32(val), pts, NULL, val3);
+ case FUTEX_WAKE:
+ return do_safe_futex(g2h(uaddr), op, val, NULL, NULL, 0);
+ case FUTEX_FD:
+ return do_safe_futex(g2h(uaddr), op, val, NULL, NULL, 0);
+ case FUTEX_REQUEUE:
+ case FUTEX_CMP_REQUEUE:
+ case FUTEX_WAKE_OP:
+ /* For FUTEX_REQUEUE, FUTEX_CMP_REQUEUE, and FUTEX_WAKE_OP, the
+ TIMEOUT parameter is interpreted as a uint32_t by the kernel.
+ 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. */
+ pts = (struct timespec *)(uintptr_t) timeout;
+ return do_safe_futex(g2h(uaddr), op, val, pts, g2h(uaddr2),
+ (base_op == FUTEX_CMP_REQUEUE
+ ? tswap32(val3)
+ : val3));
+ default:
+ return -TARGET_ENOSYS;
+ }
+}
+#endif
+
#if defined(TARGET_NR_name_to_handle_at) && defined(CONFIG_OPEN_BY_HANDLE)
static abi_long do_name_to_handle_at(abi_long dirfd, abi_long pathname,
abi_long handle, abi_long mount_id,
@@ -7541,7 +7649,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
ts = cpu->opaque;
if (ts->child_tidptr) {
put_user_u32(0, ts->child_tidptr);
- sys_futex(g2h(ts->child_tidptr), FUTEX_WAKE, INT_MAX,
+ do_sys_futex(g2h(ts->child_tidptr), FUTEX_WAKE, INT_MAX,
NULL, NULL, 0);
}
thread_cpu = NULL;
@@ -11635,6 +11743,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_time64(arg1, arg2, arg3, arg4, arg5, arg6);
+#endif
#if defined(TARGET_NR_inotify_init) && defined(__NR_inotify_init)
case TARGET_NR_inotify_init:
ret = get_errno(sys_inotify_init());
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PULL 0/1] Linux user for 5.0 patches
2020-03-30 10:29 [PULL 0/1] Linux user for 5.0 patches Laurent Vivier
2020-03-30 10:29 ` [PULL 1/1] linux-user: Support futex_time64 Laurent Vivier
@ 2020-03-30 11:32 ` no-reply
2020-03-30 12:14 ` Peter Maydell
2 siblings, 0 replies; 9+ messages in thread
From: no-reply @ 2020-03-30 11:32 UTC (permalink / raw)
To: laurent; +Cc: riku.voipio, qemu-devel, laurent
Patchew URL: https://patchew.org/QEMU/20200330102945.2388294-1-laurent@vivier.eu/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Subject: [PULL 0/1] Linux user for 5.0 patches
Message-id: 20200330102945.2388294-1-laurent@vivier.eu
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Switched to a new branch 'test'
68faea1 linux-user: Support futex_time64
=== OUTPUT BEGIN ===
WARNING: architecture specific defines should be avoided
#27: FILE: linux-user/syscall.c:248:
+#if defined(__NR_futex)
WARNING: architecture specific defines should be avoided
#30: FILE: linux-user/syscall.c:251:
+#if defined(__NR_futex_time64)
WARNING: architecture specific defines should be avoided
#41: FILE: linux-user/syscall.c:303:
+#if defined(__NR_futex)
WARNING: architecture specific defines should be avoided
#45: FILE: linux-user/syscall.c:307:
+#if defined(__NR_futex_time64)
ERROR: space required after that ',' (ctx:VxV)
#46: FILE: linux-user/syscall.c:308:
+_syscall6(int,sys_futex_time64,int *,uaddr,int,op,int,val,
^
ERROR: space required after that ',' (ctx:VxV)
#46: FILE: linux-user/syscall.c:308:
+_syscall6(int,sys_futex_time64,int *,uaddr,int,op,int,val,
^
ERROR: space required after that ',' (ctx:OxV)
#46: FILE: linux-user/syscall.c:308:
+_syscall6(int,sys_futex_time64,int *,uaddr,int,op,int,val,
^
ERROR: space required after that ',' (ctx:VxV)
#46: FILE: linux-user/syscall.c:308:
+_syscall6(int,sys_futex_time64,int *,uaddr,int,op,int,val,
^
ERROR: space required after that ',' (ctx:VxV)
#46: FILE: linux-user/syscall.c:308:
+_syscall6(int,sys_futex_time64,int *,uaddr,int,op,int,val,
^
ERROR: space required after that ',' (ctx:VxV)
#46: FILE: linux-user/syscall.c:308:
+_syscall6(int,sys_futex_time64,int *,uaddr,int,op,int,val,
^
ERROR: space required after that ',' (ctx:VxV)
#46: FILE: linux-user/syscall.c:308:
+_syscall6(int,sys_futex_time64,int *,uaddr,int,op,int,val,
^
ERROR: space required after that ',' (ctx:OxV)
#47: FILE: linux-user/syscall.c:309:
+ const struct timespec *,timeout,int *,uaddr2,int,val3)
^
ERROR: space required after that ',' (ctx:VxV)
#47: FILE: linux-user/syscall.c:309:
+ const struct timespec *,timeout,int *,uaddr2,int,val3)
^
ERROR: space required after that ',' (ctx:OxV)
#47: FILE: linux-user/syscall.c:309:
+ const struct timespec *,timeout,int *,uaddr2,int,val3)
^
ERROR: space required after that ',' (ctx:VxV)
#47: FILE: linux-user/syscall.c:309:
+ const struct timespec *,timeout,int *,uaddr2,int,val3)
^
ERROR: space required after that ',' (ctx:VxV)
#47: FILE: linux-user/syscall.c:309:
+ const struct timespec *,timeout,int *,uaddr2,int,val3)
^
WARNING: architecture specific defines should be avoided
#57: FILE: linux-user/syscall.c:774:
+#if defined(__NR_futex)
WARNING: architecture specific defines should be avoided
#61: FILE: linux-user/syscall.c:778:
+#if defined(__NR_futex_time64)
ERROR: space required after that ',' (ctx:VxV)
#62: FILE: linux-user/syscall.c:779:
+safe_syscall6(int,futex_time64,int *,uaddr,int,op,int,val, \
^
ERROR: space required after that ',' (ctx:VxV)
#62: FILE: linux-user/syscall.c:779:
+safe_syscall6(int,futex_time64,int *,uaddr,int,op,int,val, \
^
ERROR: space required after that ',' (ctx:OxV)
#62: FILE: linux-user/syscall.c:779:
+safe_syscall6(int,futex_time64,int *,uaddr,int,op,int,val, \
^
ERROR: space required after that ',' (ctx:VxV)
#62: FILE: linux-user/syscall.c:779:
+safe_syscall6(int,futex_time64,int *,uaddr,int,op,int,val, \
^
ERROR: space required after that ',' (ctx:VxV)
#62: FILE: linux-user/syscall.c:779:
+safe_syscall6(int,futex_time64,int *,uaddr,int,op,int,val, \
^
ERROR: space required after that ',' (ctx:VxV)
#62: FILE: linux-user/syscall.c:779:
+safe_syscall6(int,futex_time64,int *,uaddr,int,op,int,val, \
^
ERROR: space required after that ',' (ctx:VxV)
#62: FILE: linux-user/syscall.c:779:
+safe_syscall6(int,futex_time64,int *,uaddr,int,op,int,val, \
^
ERROR: space required after that ',' (ctx:OxV)
#63: FILE: linux-user/syscall.c:780:
+ const struct timespec *,timeout,int *,uaddr2,int,val3)
^
ERROR: space required after that ',' (ctx:VxV)
#63: FILE: linux-user/syscall.c:780:
+ const struct timespec *,timeout,int *,uaddr2,int,val3)
^
ERROR: space required after that ',' (ctx:OxV)
#63: FILE: linux-user/syscall.c:780:
+ const struct timespec *,timeout,int *,uaddr2,int,val3)
^
ERROR: space required after that ',' (ctx:VxV)
#63: FILE: linux-user/syscall.c:780:
+ const struct timespec *,timeout,int *,uaddr2,int,val3)
^
ERROR: space required after that ',' (ctx:VxV)
#63: FILE: linux-user/syscall.c:780:
+ const struct timespec *,timeout,int *,uaddr2,int,val3)
^
WARNING: architecture specific defines should be avoided
#86: FILE: linux-user/syscall.c:6937:
+#if defined(__NR_futex)
WARNING: architecture specific defines should be avoided
#92: FILE: linux-user/syscall.c:6943:
+#if defined(__NR_futex_time64)
WARNING: architecture specific defines should be avoided
#98: FILE: linux-user/syscall.c:6949:
+#if defined(__NR_futex)
WARNING: architecture specific defines should be avoided
#111: FILE: linux-user/syscall.c:6962:
+#if defined(__NR_futex)
WARNING: architecture specific defines should be avoided
#116: FILE: linux-user/syscall.c:6967:
+#if defined(__NR_futex_time64)
WARNING: architecture specific defines should be avoided
#123: FILE: linux-user/syscall.c:6974:
+#if defined(__NR_futex)
WARNING: line over 80 characters
#169: FILE: linux-user/syscall.c:7035:
+static int do_futex_time64(target_ulong uaddr, int op, int val, target_ulong timeout,
WARNING: Block comments use a leading /* on a separate line
#175: FILE: linux-user/syscall.c:7041:
+ /* ??? We assume FUTEX_* constants are the same on both host
WARNING: Block comments use * on subsequent lines
#176: FILE: linux-user/syscall.c:7042:
+ /* ??? We assume FUTEX_* constants are the same on both host
+ and target. */
WARNING: Block comments use a trailing */ on a separate line
#176: FILE: linux-user/syscall.c:7042:
+ and target. */
WARNING: Block comments use a leading /* on a separate line
#199: FILE: linux-user/syscall.c:7065:
+ /* For FUTEX_REQUEUE, FUTEX_CMP_REQUEUE, and FUTEX_WAKE_OP, the
WARNING: Block comments use * on subsequent lines
#200: FILE: linux-user/syscall.c:7066:
+ /* For FUTEX_REQUEUE, FUTEX_CMP_REQUEUE, and FUTEX_WAKE_OP, the
+ TIMEOUT parameter is interpreted as a uint32_t by the kernel.
WARNING: Block comments use a trailing */ on a separate line
#203: FILE: linux-user/syscall.c:7069:
+ since it's not compared to guest memory. */
total: 24 errors, 19 warnings, 207 lines checked
Commit 68faea14e7cf (linux-user: Support futex_time64) has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20200330102945.2388294-1-laurent@vivier.eu/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PULL 0/1] Linux user for 5.0 patches
2020-03-30 10:29 [PULL 0/1] Linux user for 5.0 patches Laurent Vivier
2020-03-30 10:29 ` [PULL 1/1] linux-user: Support futex_time64 Laurent Vivier
2020-03-30 11:32 ` [PULL 0/1] Linux user for 5.0 patches no-reply
@ 2020-03-30 12:14 ` Peter Maydell
2 siblings, 0 replies; 9+ messages in thread
From: Peter Maydell @ 2020-03-30 12:14 UTC (permalink / raw)
To: Laurent Vivier; +Cc: Riku Voipio, QEMU Developers
On Mon, 30 Mar 2020 at 11:31, Laurent Vivier <laurent@vivier.eu> wrote:
>
> The following changes since commit cfe68ae025f704f336d7dd3d1903ce37b445831d:
>
> Merge remote-tracking branch 'remotes/vivier2/tags/linux-user-for-5.0-pull-request' into staging (2020-03-26 20:55:54 +0000)
>
> are available in the Git repository at:
>
> git://github.com/vivier/qemu.git tags/linux-user-for-5.0-pull-request
>
> for you to fetch changes up to 146902964d66881da797e45a5950732e38532e3e:
>
> linux-user: Support futex_time64 (2020-03-30 12:28:30 +0200)
>
> ----------------------------------------------------------------
> Add support for futex_time64
>
> ----------------------------------------------------------------
>
> Alistair Francis (1):
> linux-user: Support futex_time64
>
> linux-user/syscall.c | 140 ++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 126 insertions(+), 14 deletions(-)
Applied, thanks.
Please update the changelog at https://wiki.qemu.org/ChangeLog/5.0
for any user-visible changes.
-- PMM
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PULL 1/1] linux-user: Support futex_time64
2020-03-30 10:29 ` [PULL 1/1] linux-user: Support futex_time64 Laurent Vivier
@ 2020-11-02 18:15 ` Peter Maydell
2020-11-03 7:28 ` Laurent Vivier
0 siblings, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2020-11-02 18:15 UTC (permalink / raw)
To: Laurent Vivier; +Cc: Riku Voipio, Alistair Francis, QEMU Developers
On Mon, 30 Mar 2020 at 11:31, Laurent Vivier <laurent@vivier.eu> wrote:
>
> From: Alistair Francis <alistair.francis@wdc.com>
>
> 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 futex
> syscall.
Hi; I dunno why Coverity's only just noticed this, but in
CID 1432339 it points out:
> +#if defined(TARGET_NR_futex_time64)
> +static int do_futex_time64(target_ulong uaddr, int op, int val, target_ulong timeout,
> + target_ulong uaddr2, int val3)
> +{
> + struct timespec ts, *pts;
> + int base_op;
> +
> + /* ??? We assume FUTEX_* constants are the same on both host
> + and target. */
> +#ifdef FUTEX_CMD_MASK
> + base_op = op & FUTEX_CMD_MASK;
> +#else
> + base_op = op;
> +#endif
> + switch (base_op) {
> + case FUTEX_WAIT:
> + case FUTEX_WAIT_BITSET:
> + if (timeout) {
> + pts = &ts;
> + target_to_host_timespec64(pts, timeout);
...that here we call target_to_host_timespec64(), which can
fail with -TARGET_EFAULT, but (unlike all the other times we call
the function) we aren't checking its return value.
Is there missing error handling code here ?
> + } else {
> + pts = NULL;
> + }
thanks
-- PMM
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PULL 1/1] linux-user: Support futex_time64
2020-11-02 18:15 ` Peter Maydell
@ 2020-11-03 7:28 ` Laurent Vivier
2020-11-03 15:40 ` Alistair Francis
0 siblings, 1 reply; 9+ messages in thread
From: Laurent Vivier @ 2020-11-03 7:28 UTC (permalink / raw)
To: Peter Maydell; +Cc: Riku Voipio, Alistair Francis, QEMU Developers
Le 02/11/2020 à 19:15, Peter Maydell a écrit :
> On Mon, 30 Mar 2020 at 11:31, Laurent Vivier <laurent@vivier.eu> wrote:
>>
>> From: Alistair Francis <alistair.francis@wdc.com>
>>
>> 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 futex
>> syscall.
>
> Hi; I dunno why Coverity's only just noticed this, but in
> CID 1432339 it points out:
>
>> +#if defined(TARGET_NR_futex_time64)
>> +static int do_futex_time64(target_ulong uaddr, int op, int val, target_ulong timeout,
>> + target_ulong uaddr2, int val3)
>> +{
>> + struct timespec ts, *pts;
>> + int base_op;
>> +
>> + /* ??? We assume FUTEX_* constants are the same on both host
>> + and target. */
>> +#ifdef FUTEX_CMD_MASK
>> + base_op = op & FUTEX_CMD_MASK;
>> +#else
>> + base_op = op;
>> +#endif
>> + switch (base_op) {
>> + case FUTEX_WAIT:
>> + case FUTEX_WAIT_BITSET:
>> + if (timeout) {
>> + pts = &ts;
>> + target_to_host_timespec64(pts, timeout);
>
> ...that here we call target_to_host_timespec64(), which can
> fail with -TARGET_EFAULT, but (unlike all the other times we call
> the function) we aren't checking its return value.
> Is there missing error handling code here ?
>
I think the code is like that because this is a cut&paste of function
do_futex() witl "s/timespec/timespec64/".
And yes I think we should check for the return value.
I'm going to fix that.
Thanks,
Laurent
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PULL 1/1] linux-user: Support futex_time64
2020-11-03 7:28 ` Laurent Vivier
@ 2020-11-03 15:40 ` Alistair Francis
2020-11-03 16:00 ` Laurent Vivier
0 siblings, 1 reply; 9+ messages in thread
From: Alistair Francis @ 2020-11-03 15:40 UTC (permalink / raw)
To: Laurent Vivier
Cc: Peter Maydell, Riku Voipio, Alistair Francis, QEMU Developers
On Mon, Nov 2, 2020 at 11:29 PM Laurent Vivier <laurent@vivier.eu> wrote:
>
> Le 02/11/2020 à 19:15, Peter Maydell a écrit :
> > On Mon, 30 Mar 2020 at 11:31, Laurent Vivier <laurent@vivier.eu> wrote:
> >>
> >> From: Alistair Francis <alistair.francis@wdc.com>
> >>
> >> 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 futex
> >> syscall.
> >
> > Hi; I dunno why Coverity's only just noticed this, but in
> > CID 1432339 it points out:
> >
> >> +#if defined(TARGET_NR_futex_time64)
> >> +static int do_futex_time64(target_ulong uaddr, int op, int val, target_ulong timeout,
> >> + target_ulong uaddr2, int val3)
> >> +{
> >> + struct timespec ts, *pts;
> >> + int base_op;
> >> +
> >> + /* ??? We assume FUTEX_* constants are the same on both host
> >> + and target. */
> >> +#ifdef FUTEX_CMD_MASK
> >> + base_op = op & FUTEX_CMD_MASK;
> >> +#else
> >> + base_op = op;
> >> +#endif
> >> + switch (base_op) {
> >> + case FUTEX_WAIT:
> >> + case FUTEX_WAIT_BITSET:
> >> + if (timeout) {
> >> + pts = &ts;
> >> + target_to_host_timespec64(pts, timeout);
> >
> > ...that here we call target_to_host_timespec64(), which can
> > fail with -TARGET_EFAULT, but (unlike all the other times we call
> > the function) we aren't checking its return value.
> > Is there missing error handling code here ?
> >
>
> I think the code is like that because this is a cut&paste of function
> do_futex() witl "s/timespec/timespec64/".
>
> And yes I think we should check for the return value.
> I'm going to fix that.
Thanks! Let me know if you want me to do it and I can send a patch instead.
Alistair
>
> Thanks,
> Laurent
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PULL 1/1] linux-user: Support futex_time64
2020-11-03 15:40 ` Alistair Francis
@ 2020-11-03 16:00 ` Laurent Vivier
2020-11-03 19:46 ` Alistair Francis
0 siblings, 1 reply; 9+ messages in thread
From: Laurent Vivier @ 2020-11-03 16:00 UTC (permalink / raw)
To: Alistair Francis
Cc: Peter Maydell, Riku Voipio, Alistair Francis, QEMU Developers
Le 03/11/2020 à 16:40, Alistair Francis a écrit :
> On Mon, Nov 2, 2020 at 11:29 PM Laurent Vivier <laurent@vivier.eu> wrote:
>>
>> Le 02/11/2020 à 19:15, Peter Maydell a écrit :
>>> On Mon, 30 Mar 2020 at 11:31, Laurent Vivier <laurent@vivier.eu> wrote:
>>>>
>>>> From: Alistair Francis <alistair.francis@wdc.com>
>>>>
>>>> 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 futex
>>>> syscall.
>>>
>>> Hi; I dunno why Coverity's only just noticed this, but in
>>> CID 1432339 it points out:
>>>
>>>> +#if defined(TARGET_NR_futex_time64)
>>>> +static int do_futex_time64(target_ulong uaddr, int op, int val, target_ulong timeout,
>>>> + target_ulong uaddr2, int val3)
>>>> +{
>>>> + struct timespec ts, *pts;
>>>> + int base_op;
>>>> +
>>>> + /* ??? We assume FUTEX_* constants are the same on both host
>>>> + and target. */
>>>> +#ifdef FUTEX_CMD_MASK
>>>> + base_op = op & FUTEX_CMD_MASK;
>>>> +#else
>>>> + base_op = op;
>>>> +#endif
>>>> + switch (base_op) {
>>>> + case FUTEX_WAIT:
>>>> + case FUTEX_WAIT_BITSET:
>>>> + if (timeout) {
>>>> + pts = &ts;
>>>> + target_to_host_timespec64(pts, timeout);
>>>
>>> ...that here we call target_to_host_timespec64(), which can
>>> fail with -TARGET_EFAULT, but (unlike all the other times we call
>>> the function) we aren't checking its return value.
>>> Is there missing error handling code here ?
>>>
>>
>> I think the code is like that because this is a cut&paste of function
>> do_futex() witl "s/timespec/timespec64/".
>>
>> And yes I think we should check for the return value.
>> I'm going to fix that.
>
> Thanks! Let me know if you want me to do it and I can send a patch instead.
>
If you have time, please do.
Thanks
Laurent
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PULL 1/1] linux-user: Support futex_time64
2020-11-03 16:00 ` Laurent Vivier
@ 2020-11-03 19:46 ` Alistair Francis
0 siblings, 0 replies; 9+ messages in thread
From: Alistair Francis @ 2020-11-03 19:46 UTC (permalink / raw)
To: Laurent Vivier
Cc: Peter Maydell, Riku Voipio, Alistair Francis, QEMU Developers
On Tue, Nov 3, 2020 at 8:00 AM Laurent Vivier <laurent@vivier.eu> wrote:
>
> Le 03/11/2020 à 16:40, Alistair Francis a écrit :
> > On Mon, Nov 2, 2020 at 11:29 PM Laurent Vivier <laurent@vivier.eu> wrote:
> >>
> >> Le 02/11/2020 à 19:15, Peter Maydell a écrit :
> >>> On Mon, 30 Mar 2020 at 11:31, Laurent Vivier <laurent@vivier.eu> wrote:
> >>>>
> >>>> From: Alistair Francis <alistair.francis@wdc.com>
> >>>>
> >>>> 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 futex
> >>>> syscall.
> >>>
> >>> Hi; I dunno why Coverity's only just noticed this, but in
> >>> CID 1432339 it points out:
> >>>
> >>>> +#if defined(TARGET_NR_futex_time64)
> >>>> +static int do_futex_time64(target_ulong uaddr, int op, int val, target_ulong timeout,
> >>>> + target_ulong uaddr2, int val3)
> >>>> +{
> >>>> + struct timespec ts, *pts;
> >>>> + int base_op;
> >>>> +
> >>>> + /* ??? We assume FUTEX_* constants are the same on both host
> >>>> + and target. */
> >>>> +#ifdef FUTEX_CMD_MASK
> >>>> + base_op = op & FUTEX_CMD_MASK;
> >>>> +#else
> >>>> + base_op = op;
> >>>> +#endif
> >>>> + switch (base_op) {
> >>>> + case FUTEX_WAIT:
> >>>> + case FUTEX_WAIT_BITSET:
> >>>> + if (timeout) {
> >>>> + pts = &ts;
> >>>> + target_to_host_timespec64(pts, timeout);
> >>>
> >>> ...that here we call target_to_host_timespec64(), which can
> >>> fail with -TARGET_EFAULT, but (unlike all the other times we call
> >>> the function) we aren't checking its return value.
> >>> Is there missing error handling code here ?
> >>>
> >>
> >> I think the code is like that because this is a cut&paste of function
> >> do_futex() witl "s/timespec/timespec64/".
> >>
> >> And yes I think we should check for the return value.
> >> I'm going to fix that.
> >
> > Thanks! Let me know if you want me to do it and I can send a patch instead.
> >
>
> If you have time, please do.
Sending the patch now.
Alistair
>
> Thanks
> Laurent
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-11-03 19:59 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-30 10:29 [PULL 0/1] Linux user for 5.0 patches Laurent Vivier
2020-03-30 10:29 ` [PULL 1/1] linux-user: Support futex_time64 Laurent Vivier
2020-11-02 18:15 ` Peter Maydell
2020-11-03 7:28 ` Laurent Vivier
2020-11-03 15:40 ` Alistair Francis
2020-11-03 16:00 ` Laurent Vivier
2020-11-03 19:46 ` Alistair Francis
2020-03-30 11:32 ` [PULL 0/1] Linux user for 5.0 patches no-reply
2020-03-30 12:14 ` Peter Maydell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.