[Qemu-devel] [PULL 0/6] linux-user patches for 2.4 softfreeze

[Qemu-devel] [PULL 0/6] linux-user patches for 2.4 softfreeze

[riku.voipio]

From: Riku Voipio <riku.voipio@linaro.org>

The following changes since commit 0a2df857a7038c75379cc575de5d4be4c0ac629e:

  Merge remote-tracking branch 'remotes/stefanha/tags/net-pull-request' into staging (2015-06-12 15:39:05 +0100)

are available in the git repository at:

  git://git.linaro.org/people/riku.voipio/qemu.git tags/pull-linux-user-20150615

for you to fetch changes up to d2897da1f1e97d684f80ff62d473c31b79bc643a:

  linux-user: fix the breakpoint inheritance in spawned threads (2015-06-15 11:36:59 +0300)

----------------------------------------------------------------
linux-user patches for 2.4 softfreeze

----------------------------------------------------------------

Alexander Graf (1):
  linux-user: Allocate thunk size dynamically

Laurent Vivier (1):
  linux-user: ioctl() command type is int

Peter Maydell (2):
  linux-user: Fix length handling in host_to_target_cmsg
  linux-user: use __get_user and __put_user in cmsg conversions

Thierry Bultel (1):
  linux-user: fix the breakpoint inheritance in spawned threads

Yongbok Kim (1):
  linux-user: Use abi_ulong for TARGET_ELF_PAGESTART

 include/exec/user/thunk.h |   4 +-
 linux-user/elfload.c      |   3 +-
 linux-user/main.c         |   4 +-
 linux-user/syscall.c      | 110 +++++++++++++++++++++++++++++++++++-----------
 thunk.c                   |  16 +++++--
 5 files changed, 103 insertions(+), 34 deletions(-)

-- 
2.1.4

[Qemu-devel] [PULL 1/6] linux-user: Allocate thunk size dynamically

[riku.voipio]

From: Alexander Graf <agraf@suse.de>

We store all struct types in an array of static size without ever
checking whether we overrun it. Of course some day someone (like me
in another, ancient ALSA enabling patch set) will run into the limit
without realizing it.

So let's make the allocation dynamic. We already know the number of
structs that we want to allocate, so we only need to pass the variable
into the respective piece of code.

Also, to ensure we don't accidently overwrite random memory, add some
asserts to sanity check whether a thunk is actually part of our array.

Signed-off-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Riku Voipio <riku.voipio@linaro.org>
---
 include/exec/user/thunk.h |  4 +++-
 linux-user/syscall.c      |  3 +++
 thunk.c                   | 16 ++++++++++++----
 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/include/exec/user/thunk.h b/include/exec/user/thunk.h
index 87025c3..3b67462 100644
--- a/include/exec/user/thunk.h
+++ b/include/exec/user/thunk.h
@@ -74,7 +74,7 @@ const argtype *thunk_convert(void *dst, const void *src,
                              const argtype *type_ptr, int to_host);
 #ifndef NO_THUNK_TYPE_SIZE
 
-extern StructEntry struct_entries[];
+extern StructEntry *struct_entries;
 
 int thunk_type_size_array(const argtype *type_ptr, int is_host);
 int thunk_type_align_array(const argtype *type_ptr, int is_host);
@@ -186,4 +186,6 @@ unsigned int target_to_host_bitmask(unsigned int x86_mask,
 unsigned int host_to_target_bitmask(unsigned int alpha_mask,
                                     const bitmask_transtbl * trans_tbl);
 
+void thunk_init(unsigned int max_structs);
+
 #endif
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 1622ad6..f56f3e0 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -3277,6 +3277,7 @@ static abi_long do_ipc(unsigned int call, abi_long first,
 #define STRUCT_SPECIAL(name) STRUCT_ ## name,
 enum {
 #include "syscall_types.h"
+STRUCT_MAX
 };
 #undef STRUCT
 #undef STRUCT_SPECIAL
@@ -4879,6 +4880,8 @@ void syscall_init(void)
     int size;
     int i;
 
+    thunk_init(STRUCT_MAX);
+
 #define STRUCT(name, ...) thunk_register_struct(STRUCT_ ## name, #name, struct_ ## name ## _def);
 #define STRUCT_SPECIAL(name) thunk_register_struct_direct(STRUCT_ ## name, #name, &struct_ ## name ## _def);
 #include "syscall_types.h"
diff --git a/thunk.c b/thunk.c
index 3cca047..f501fd7 100644
--- a/thunk.c
+++ b/thunk.c
@@ -25,10 +25,8 @@
 
 //#define DEBUG
 
-#define MAX_STRUCTS 128
-
-/* XXX: make it dynamic */
-StructEntry struct_entries[MAX_STRUCTS];
+static unsigned int max_struct_entries;
+StructEntry *struct_entries;
 
 static const argtype *thunk_type_next_ptr(const argtype *type_ptr);
 
@@ -70,6 +68,7 @@ void thunk_register_struct(int id, const char *name, const argtype *types)
     StructEntry *se;
     int nb_fields, offset, max_align, align, size, i, j;
 
+    assert(id < max_struct_entries);
     se = struct_entries + id;
 
     /* first we count the number of fields */
@@ -117,6 +116,8 @@ void thunk_register_struct_direct(int id, const char *name,
                                   const StructEntry *se1)
 {
     StructEntry *se;
+
+    assert(id < max_struct_entries);
     se = struct_entries + id;
     *se = *se1;
     se->name = name;
@@ -244,6 +245,7 @@ const argtype *thunk_convert(void *dst, const void *src,
             const argtype *field_types;
             const int *dst_offsets, *src_offsets;
 
+            assert(*type_ptr < max_struct_entries);
             se = struct_entries + *type_ptr++;
             if (se->convert[0] != NULL) {
                 /* specific conversion is needed */
@@ -314,3 +316,9 @@ int thunk_type_align_array(const argtype *type_ptr, int is_host)
     return thunk_type_align(type_ptr, is_host);
 }
 #endif /* ndef NO_THUNK_TYPE_SIZE */
+
+void thunk_init(unsigned int max_structs)
+{
+    max_struct_entries = max_structs;
+    struct_entries = g_new0(StructEntry, max_structs);
+}
-- 
2.1.4

[Qemu-devel] [PULL 2/6] linux-user: Use abi_ulong for TARGET_ELF_PAGESTART

[riku.voipio]

From: Yongbok Kim <yongbok.kim@imgtec.com>

TARGET_ELF_PAGESTART is required to use abi_ulong to correctly handle
addresses for different target bits width.
This patch fixes a problem when running a 64-bit user mode application
on 32-bit host machines.

Signed-off-by: Yongbok Kim <yongbok.kim@imgtec.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Riku Voipio <riku.voipio@linaro.org>
---
 linux-user/elfload.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index b71e866..1788368 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1256,7 +1256,8 @@ struct exec
 
 /* Necessary parameters */
 #define TARGET_ELF_EXEC_PAGESIZE TARGET_PAGE_SIZE
-#define TARGET_ELF_PAGESTART(_v) ((_v) & ~(unsigned long)(TARGET_ELF_EXEC_PAGESIZE-1))
+#define TARGET_ELF_PAGESTART(_v) ((_v) & \
+                                 ~(abi_ulong)(TARGET_ELF_EXEC_PAGESIZE-1))
 #define TARGET_ELF_PAGEOFFSET(_v) ((_v) & (TARGET_ELF_EXEC_PAGESIZE-1))
 
 #define DLINFO_ITEMS 14
-- 
2.1.4

[Qemu-devel] [PULL 3/6] linux-user: ioctl() command type is int

[riku.voipio]

From: Laurent Vivier <laurent@vivier.eu>

When executing a 64bit target chroot on 64bit host,
the ioctl() command can mismatch.

It seems the previous commit doesn't solve the problem in
my case:

	9c6bf9c7 linux-user: Fix ioctl cmd type mismatch on 64-bit targets

For example, a ppc64 chroot on an x86_64 host:

bash-4.3# ls
Unsupported ioctl: cmd=0x80087467
Unsupported ioctl: cmd=0x802c7415

The origin of the problem is in syscall.c:do_ioctl().

	static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)

In this case (ppc64) abi_long is long (on the x86_64), and

    cmd = 0x0000000080087467

then
	if (ie->target_cmd == cmd)

target_cmd is int, so target_cmd = 0x80087467
and to compare an int with a long, the sign is extended to 64bit,
so the comparison is:

	if (0xffffffff80087467 == 0x0000000080087467)

which doesn't match whereas it should.

This patch uses int in the case of the target command type
instead of abi_long (and for consistency, update IOCTLEntry).

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Riku Voipio <riku.voipio@linaro.org>
---
 linux-user/syscall.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index f56f3e0..ed8c423 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -3291,11 +3291,11 @@ STRUCT_MAX
 typedef struct IOCTLEntry IOCTLEntry;
 
 typedef abi_long do_ioctl_fn(const IOCTLEntry *ie, uint8_t *buf_temp,
-                             int fd, abi_long cmd, abi_long arg);
+                             int fd, int cmd, abi_long arg);
 
 struct IOCTLEntry {
     int target_cmd;
-    unsigned int host_cmd;
+    int host_cmd;
     const char *name;
     int access;
     do_ioctl_fn *do_ioctl;
@@ -3317,7 +3317,7 @@ struct IOCTLEntry {
                             / sizeof(struct fiemap_extent))
 
 static abi_long do_ioctl_fs_ioc_fiemap(const IOCTLEntry *ie, uint8_t *buf_temp,
-                                       int fd, abi_long cmd, abi_long arg)
+                                       int fd, int cmd, abi_long arg)
 {
     /* The parameter for this ioctl is a struct fiemap followed
      * by an array of struct fiemap_extent whose size is set
@@ -3398,7 +3398,7 @@ static abi_long do_ioctl_fs_ioc_fiemap(const IOCTLEntry *ie, uint8_t *buf_temp,
 #endif
 
 static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
-                                int fd, abi_long cmd, abi_long arg)
+                                int fd, int cmd, abi_long arg)
 {
     const argtype *arg_type = ie->arg_type;
     int target_size;
@@ -3492,7 +3492,7 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
 }
 
 static abi_long do_ioctl_dm(const IOCTLEntry *ie, uint8_t *buf_temp, int fd,
-                            abi_long cmd, abi_long arg)
+                            int cmd, abi_long arg)
 {
     void *argptr;
     struct dm_ioctl *host_dm;
@@ -3717,7 +3717,7 @@ out:
 }
 
 static abi_long do_ioctl_blkpg(const IOCTLEntry *ie, uint8_t *buf_temp, int fd,
-                               abi_long cmd, abi_long arg)
+                               int cmd, abi_long arg)
 {
     void *argptr;
     int target_size;
@@ -3770,7 +3770,7 @@ out:
 }
 
 static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
-                                int fd, abi_long cmd, abi_long arg)
+                                int fd, int cmd, abi_long arg)
 {
     const argtype *arg_type = ie->arg_type;
     const StructEntry *se;
@@ -3833,7 +3833,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
 }
 
 static abi_long do_ioctl_kdsigaccept(const IOCTLEntry *ie, uint8_t *buf_temp,
-                                     int fd, abi_long cmd, abi_long arg)
+                                     int fd, int cmd, abi_long arg)
 {
     int sig = target_to_host_signal(arg);
     return get_errno(ioctl(fd, ie->host_cmd, sig));
@@ -3850,7 +3850,7 @@ static IOCTLEntry ioctl_entries[] = {
 
 /* ??? Implement proper locking for ioctls.  */
 /* do_ioctl() Must return target values and target errnos. */
-static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)
+static abi_long do_ioctl(int fd, int cmd, abi_long arg)
 {
     const IOCTLEntry *ie;
     const argtype *arg_type;
-- 
2.1.4

[Qemu-devel] [PULL 4/6] linux-user: Fix length handling in host_to_target_cmsg

[riku.voipio]

From: Peter Maydell <peter.maydell@linaro.org>

The previous code for handling payload length when converting
cmsg structures from host to target had a number of problems:
 * we required the msg->msg_controllen to declare the buffer
   to have enough space for final trailing padding (we were
   checking against CMSG_SPACE), whereas the kernel does not
   require this, and common userspace code assumes this. (In
   particular, glibc's "try to talk to nscd" code that it will
   run on startup will receive a cmsg with a 4 byte payload and
   only allocate 4 bytes for it, which was causing us to do
   the wrong thing on architectures that need 8-alignment.)
 * we weren't correctly handling the fact that the SO_TIMESTAMP
   payload may be larger for the target than the host
 * we weren't marking the messages with MSG_CTRUNC when we did
   need to truncate a message that wasn't truncated by the host,
   but were instead logging a QEMU message; since truncation is
   always the result of a guest giving us an insufficiently
   sized buffer, we should report it to the guest as the kernel
   does and don't log anything

Rewrite the parts of the function that deal with length to
fix these issues, and add a comment in target_to_host_cmsg
to explain why the overflow logging it does is a QEMU bug,
not a guest issue.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Riku Voipio <riku.voipio@linaro.org>
---
 linux-user/syscall.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 61 insertions(+), 8 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index ed8c423..839ce93 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1202,6 +1202,15 @@ static inline abi_long target_to_host_cmsg(struct msghdr *msgh,
         space += CMSG_SPACE(len);
         if (space > msgh->msg_controllen) {
             space -= CMSG_SPACE(len);
+            /* This is a QEMU bug, since we allocated the payload
+             * area ourselves (unlike overflow in host-to-target
+             * conversion, which is just the guest giving us a buffer
+             * that's too small). It can't happen for the payload types
+             * we currently support; if it becomes an issue in future
+             * we would need to improve our allocation strategy to
+             * something more intelligent than "twice the size of the
+             * target buffer we're reading from".
+             */
             gemu_log("Host cmsg overflow\n");
             break;
         }
@@ -1267,11 +1276,16 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
         void *target_data = TARGET_CMSG_DATA(target_cmsg);
 
         int len = cmsg->cmsg_len - CMSG_ALIGN(sizeof (struct cmsghdr));
+        int tgt_len, tgt_space;
 
-        space += TARGET_CMSG_SPACE(len);
-        if (space > msg_controllen) {
-            space -= TARGET_CMSG_SPACE(len);
-            gemu_log("Target cmsg overflow\n");
+        /* We never copy a half-header but may copy half-data;
+         * this is Linux's behaviour in put_cmsg(). Note that
+         * truncation here is a guest problem (which we report
+         * to the guest via the CTRUNC bit), unlike truncation
+         * in target_to_host_cmsg, which is a QEMU bug.
+         */
+        if (msg_controllen < sizeof(struct cmsghdr)) {
+            target_msgh->msg_flags |= tswap32(MSG_CTRUNC);
             break;
         }
 
@@ -1281,8 +1295,35 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
             target_cmsg->cmsg_level = tswap32(cmsg->cmsg_level);
         }
         target_cmsg->cmsg_type = tswap32(cmsg->cmsg_type);
-        target_cmsg->cmsg_len = tswapal(TARGET_CMSG_LEN(len));
 
+        tgt_len = TARGET_CMSG_LEN(len);
+
+        /* Payload types which need a different size of payload on
+         * the target must adjust tgt_len here.
+         */
+        switch (cmsg->cmsg_level) {
+        case SOL_SOCKET:
+            switch (cmsg->cmsg_type) {
+            case SO_TIMESTAMP:
+                tgt_len = sizeof(struct target_timeval);
+                break;
+            default:
+                break;
+            }
+        default:
+            break;
+        }
+
+        if (msg_controllen < tgt_len) {
+            target_msgh->msg_flags |= tswap32(MSG_CTRUNC);
+            tgt_len = msg_controllen;
+        }
+
+        /* We must now copy-and-convert len bytes of payload
+         * into tgt_len bytes of destination space. Bear in mind
+         * that in both source and destination we may be dealing
+         * with a truncated value!
+         */
         switch (cmsg->cmsg_level) {
         case SOL_SOCKET:
             switch (cmsg->cmsg_type) {
@@ -1290,7 +1331,7 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
             {
                 int *fd = (int *)data;
                 int *target_fd = (int *)target_data;
-                int i, numfds = len / sizeof(int);
+                int i, numfds = tgt_len / sizeof(int);
 
                 for (i = 0; i < numfds; i++)
                     target_fd[i] = tswap32(fd[i]);
@@ -1302,8 +1343,10 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
                 struct target_timeval *target_tv =
                     (struct target_timeval *)target_data;
 
-                if (len != sizeof(struct timeval))
+                if (len != sizeof(struct timeval) ||
+                    tgt_len != sizeof(struct target_timeval)) {
                     goto unimplemented;
+                }
 
                 /* copy struct timeval to target */
                 target_tv->tv_sec = tswapal(tv->tv_sec);
@@ -1330,9 +1373,19 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
         unimplemented:
             gemu_log("Unsupported ancillary data: %d/%d\n",
                                         cmsg->cmsg_level, cmsg->cmsg_type);
-            memcpy(target_data, data, len);
+            memcpy(target_data, data, MIN(len, tgt_len));
+            if (tgt_len > len) {
+                memset(target_data + len, 0, tgt_len - len);
+            }
         }
 
+        target_cmsg->cmsg_len = tswapal(tgt_len);
+        tgt_space = TARGET_CMSG_SPACE(tgt_len);
+        if (msg_controllen < tgt_space) {
+            tgt_space = msg_controllen;
+        }
+        msg_controllen -= tgt_space;
+        space += tgt_space;
         cmsg = CMSG_NXTHDR(msgh, cmsg);
         target_cmsg = TARGET_CMSG_NXTHDR(target_msgh, target_cmsg);
     }
-- 
2.1.4

[Qemu-devel] [PULL 5/6] linux-user: use __get_user and __put_user in cmsg conversions

[riku.voipio]

From: Peter Maydell <peter.maydell@linaro.org>

The target payloads in cmsg conversions may not have the alignment
required by the host. Using the get_user and put_user functions is
the easiest way to handle this and also do the byte-swapping we
require.

(Note that prior to this commit target_to_host_cmsg was incorrectly
using __put_user() rather than __get_user() for the SCM_CREDENTIALS
conversion, which meant it wasn't getting the benefit of the
misalignment handling.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Riku Voipio <riku.voipio@linaro.org>
---
 linux-user/syscall.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 839ce93..ed99104 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1228,17 +1228,18 @@ static inline abi_long target_to_host_cmsg(struct msghdr *msgh,
             int *target_fd = (int *)target_data;
             int i, numfds = len / sizeof(int);
 
-            for (i = 0; i < numfds; i++)
-                fd[i] = tswap32(target_fd[i]);
+            for (i = 0; i < numfds; i++) {
+                __get_user(fd[i], target_fd + i);
+            }
         } else if (cmsg->cmsg_level == SOL_SOCKET
                &&  cmsg->cmsg_type == SCM_CREDENTIALS) {
             struct ucred *cred = (struct ucred *)data;
             struct target_ucred *target_cred =
                 (struct target_ucred *)target_data;
 
-            __put_user(target_cred->pid, &cred->pid);
-            __put_user(target_cred->uid, &cred->uid);
-            __put_user(target_cred->gid, &cred->gid);
+            __get_user(cred->pid, &target_cred->pid);
+            __get_user(cred->uid, &target_cred->uid);
+            __get_user(cred->gid, &target_cred->gid);
         } else {
             gemu_log("Unsupported ancillary data: %d/%d\n",
                                         cmsg->cmsg_level, cmsg->cmsg_type);
@@ -1333,8 +1334,9 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
                 int *target_fd = (int *)target_data;
                 int i, numfds = tgt_len / sizeof(int);
 
-                for (i = 0; i < numfds; i++)
-                    target_fd[i] = tswap32(fd[i]);
+                for (i = 0; i < numfds; i++) {
+                    __put_user(fd[i], target_fd + i);
+                }
                 break;
             }
             case SO_TIMESTAMP:
@@ -1349,8 +1351,8 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
                 }
 
                 /* copy struct timeval to target */
-                target_tv->tv_sec = tswapal(tv->tv_sec);
-                target_tv->tv_usec = tswapal(tv->tv_usec);
+                __put_user(tv->tv_sec, &target_tv->tv_sec);
+                __put_user(tv->tv_usec, &target_tv->tv_usec);
                 break;
             }
             case SCM_CREDENTIALS:
-- 
2.1.4

[Qemu-devel] [PULL 6/6] linux-user: fix the breakpoint inheritance in spawned threads

[riku.voipio]

From: Thierry Bultel <thierry.bultel@basystemes.fr>

When a thread is spawned, cpu_copy re-initializes
the bp & wp lists of current thread, instead of the ones
of the new thread.
The effect is that breakpoints are no longer hit.

Signed-off-by: Thierry Bultel <thierry.bultel@basystemes.fr>
Signed-off-by: Riku Voipio <riku.voipio@linaro.org>
---
 linux-user/main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index a0d3e58..c855bcc 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -3459,8 +3459,8 @@ CPUArchState *cpu_copy(CPUArchState *env)
     /* Clone all break/watchpoints.
        Note: Once we support ptrace with hw-debug register access, make sure
        BP_CPU break/watchpoints are handled correctly on clone. */
-    QTAILQ_INIT(&cpu->breakpoints);
-    QTAILQ_INIT(&cpu->watchpoints);
+    QTAILQ_INIT(&new_cpu->breakpoints);
+    QTAILQ_INIT(&new_cpu->watchpoints);
     QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
         cpu_breakpoint_insert(new_cpu, bp->pc, bp->flags, NULL);
     }
-- 
2.1.4

Re: [Qemu-devel] [PULL 0/6] linux-user patches for 2.4 softfreeze

[Peter Maydell]

On 15 June 2015 at 13:20,  <riku.voipio@linaro.org> wrote:
> From: Riku Voipio <riku.voipio@linaro.org>
>
> The following changes since commit 0a2df857a7038c75379cc575de5d4be4c0ac629e:
>
>   Merge remote-tracking branch 'remotes/stefanha/tags/net-pull-request' into staging (2015-06-12 15:39:05 +0100)
>
> are available in the git repository at:
>
>   git://git.linaro.org/people/riku.voipio/qemu.git tags/pull-linux-user-20150615
>
> for you to fetch changes up to d2897da1f1e97d684f80ff62d473c31b79bc643a:
>
>   linux-user: fix the breakpoint inheritance in spawned threads (2015-06-15 11:36:59 +0300)
>
> ----------------------------------------------------------------
> linux-user patches for 2.4 softfreeze
>
> ----------------------------------------------------------------

I get a lot of build errors with clang:

/home/petmay01/linaro/qemu-for-merges/linux-user/syscall.c:3610:10:
error: overflow
      converting case value to switch condition type (3241737481 to
18446744072656321801)
      [-Werror,-Wswitch]
    case DM_TABLE_LOAD:
         ^
/usr/include/linux/dm-ioctl.h:259:26: note: expanded from macro 'DM_TABLE_LOAD'
#define DM_TABLE_LOAD    _IOWR(DM_IOCTL, DM_TABLE_LOAD_CMD, struct dm_ioctl)
                         ^
/usr/include/asm-generic/ioctl.h:77:29: note: expanded from macro '_IOWR'
#define _IOWR(type,nr,size)
_IOC(_IOC_READ|_IOC_WRITE,(type),(nr),(_IOC_TYPECHE...
                                ^
/usr/include/asm-generic/ioctl.h:66:2: note: expanded from macro '_IOC'
        (((dir)  << _IOC_DIRSHIFT) | \
        ^
/home/petmay01/linaro/qemu-for-merges/linux-user/syscall.c:3606:10:
error: overflow
      converting case value to switch condition type (3241737486 to
18446744072656321806)
      [-Werror,-Wswitch]
    case DM_TARGET_MSG:
         ^
(etc etc for all the cases until clang gives up because it's emitted
too many errors).

Guessing this is the result of the ioctl patch?

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/6] linux-user patches for 2.4 softfreeze

[Laurent Vivier]

Le 15/06/2015 17:14, Peter Maydell a écrit :
> On 15 June 2015 at 13:20,  <riku.voipio@linaro.org> wrote:
>> From: Riku Voipio <riku.voipio@linaro.org>
>>
>> The following changes since commit 0a2df857a7038c75379cc575de5d4be4c0ac629e:
>>
>>   Merge remote-tracking branch 'remotes/stefanha/tags/net-pull-request' into staging (2015-06-12 15:39:05 +0100)
>>
>> are available in the git repository at:
>>
>>   git://git.linaro.org/people/riku.voipio/qemu.git tags/pull-linux-user-20150615
>>
>> for you to fetch changes up to d2897da1f1e97d684f80ff62d473c31b79bc643a:
>>
>>   linux-user: fix the breakpoint inheritance in spawned threads (2015-06-15 11:36:59 +0300)
>>
>> ----------------------------------------------------------------
>> linux-user patches for 2.4 softfreeze
>>
>> ----------------------------------------------------------------
> 
> I get a lot of build errors with clang:
> 
> /home/petmay01/linaro/qemu-for-merges/linux-user/syscall.c:3610:10:
> error: overflow
>       converting case value to switch condition type (3241737481 to
> 18446744072656321801)
>       [-Werror,-Wswitch]
>     case DM_TABLE_LOAD:
>          ^
> /usr/include/linux/dm-ioctl.h:259:26: note: expanded from macro 'DM_TABLE_LOAD'
> #define DM_TABLE_LOAD    _IOWR(DM_IOCTL, DM_TABLE_LOAD_CMD, struct dm_ioctl)
>                          ^
> /usr/include/asm-generic/ioctl.h:77:29: note: expanded from macro '_IOWR'
> #define _IOWR(type,nr,size)
> _IOC(_IOC_READ|_IOC_WRITE,(type),(nr),(_IOC_TYPECHE...
>                                 ^
> /usr/include/asm-generic/ioctl.h:66:2: note: expanded from macro '_IOC'
>         (((dir)  << _IOC_DIRSHIFT) | \
>         ^
> /home/petmay01/linaro/qemu-for-merges/linux-user/syscall.c:3606:10:
> error: overflow
>       converting case value to switch condition type (3241737486 to
> 18446744072656321806)
>       [-Werror,-Wswitch]
>     case DM_TARGET_MSG:
>          ^
> (etc etc for all the cases until clang gives up because it's emitted
> too many errors).
> 
> Guessing this is the result of the ioctl patch?

Yes, I guess too.

Could drop this patch out ? I will rework it.

Laurent

[Qemu-devel] [PATCH v2] linux-user: ioctl() command type is int

[Laurent Vivier]

When executing a 64bit target chroot on 64bit host,
the ioctl() command can mismatch.

It seems the previous commit doesn't solve the problem in
my case:

	9c6bf9c7 linux-user: Fix ioctl cmd type mismatch on 64-bit targets

For example, a ppc64 chroot on an x86_64 host:

bash-4.3# ls
Unsupported ioctl: cmd=0x80087467
Unsupported ioctl: cmd=0x802c7415

The origin of the problem is in syscall.c:do_ioctl().

	static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)

In this case (ppc64) abi_long is long (on the x86_64), and

    cmd = 0x0000000080087467

then
	if (ie->target_cmd == cmd)

target_cmd is int, so target_cmd = 0x80087467
and to compare an int with a long, the sign is extended to 64bit,
so the comparison is:

	if (0xffffffff80087467 == 0x0000000080087467)

which doesn't match whereas it should.

This patch uses int in the case of the target command type
instead of abi_long.

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
v2: Don't modify IOCTLEntry type (useless and introduce clang errors)
 linux-user/syscall.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index b98b7e7..5a280c3 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -3645,7 +3645,7 @@ enum {
 typedef struct IOCTLEntry IOCTLEntry;
 
 typedef abi_long do_ioctl_fn(const IOCTLEntry *ie, uint8_t *buf_temp,
-                             int fd, abi_long cmd, abi_long arg);
+                             int fd, int cmd, abi_long arg);
 
 struct IOCTLEntry {
     int target_cmd;
@@ -3671,7 +3671,7 @@ struct IOCTLEntry {
                             / sizeof(struct fiemap_extent))
 
 static abi_long do_ioctl_fs_ioc_fiemap(const IOCTLEntry *ie, uint8_t *buf_temp,
-                                       int fd, abi_long cmd, abi_long arg)
+                                       int fd, int cmd, abi_long arg)
 {
     /* The parameter for this ioctl is a struct fiemap followed
      * by an array of struct fiemap_extent whose size is set
@@ -3752,7 +3752,7 @@ static abi_long do_ioctl_fs_ioc_fiemap(const IOCTLEntry *ie, uint8_t *buf_temp,
 #endif
 
 static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
-                                int fd, abi_long cmd, abi_long arg)
+                                int fd, int cmd, abi_long arg)
 {
     const argtype *arg_type = ie->arg_type;
     int target_size;
@@ -3846,7 +3846,7 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
 }
 
 static abi_long do_ioctl_dm(const IOCTLEntry *ie, uint8_t *buf_temp, int fd,
-                            abi_long cmd, abi_long arg)
+                            int cmd, abi_long arg)
 {
     void *argptr;
     struct dm_ioctl *host_dm;
@@ -4071,7 +4071,7 @@ out:
 }
 
 static abi_long do_ioctl_blkpg(const IOCTLEntry *ie, uint8_t *buf_temp, int fd,
-                               abi_long cmd, abi_long arg)
+                               int cmd, abi_long arg)
 {
     void *argptr;
     int target_size;
@@ -4124,7 +4124,7 @@ out:
 }
 
 static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
-                                int fd, abi_long cmd, abi_long arg)
+                                int fd, int cmd, abi_long arg)
 {
     const argtype *arg_type = ie->arg_type;
     const StructEntry *se;
@@ -4187,7 +4187,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
 }
 
 static abi_long do_ioctl_kdsigaccept(const IOCTLEntry *ie, uint8_t *buf_temp,
-                                     int fd, abi_long cmd, abi_long arg)
+                                     int fd, int cmd, abi_long arg)
 {
     int sig = target_to_host_signal(arg);
     return get_errno(ioctl(fd, ie->host_cmd, sig));
@@ -4204,7 +4204,7 @@ static IOCTLEntry ioctl_entries[] = {
 
 /* ??? Implement proper locking for ioctls.  */
 /* do_ioctl() Must return target values and target errnos. */
-static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)
+static abi_long do_ioctl(int fd, int cmd, abi_long arg)
 {
     const IOCTLEntry *ie;
     const argtype *arg_type;
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2] linux-user: ioctl() command type is int

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1295 bytes --]

On 06/15/2015 04:35 PM, Laurent Vivier wrote:
> When executing a 64bit target chroot on 64bit host,
> the ioctl() command can mismatch.
> 

> 
> The origin of the problem is in syscall.c:do_ioctl().
> 
> 	static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)

It's annoying that Linux picked ioctl(int, unsigned long request, ...),
particularly since POSIX picked ioctl(int, int request, ...) [1] and
therefore Linux is constrained to never accept a 'request' that doesn't
fit in 32 bits.  Especially so since the POSIX definition of ioctl()
applies only to the obsolete STREAMS interface that Linux never really
picked up on. (The gnulib project has determined ways to write an
ioctl() wrapper that always takes an int request, then widens to long as
necessary before calling the real syscall, with no ill effects [2])

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/ioctl.html
[2] http://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/ioctl.c

However, I don't feel comfortable enough with this code to give a
competent review, only to offer up that bit of trivia and the vague
impression that it looks like you are safe in this patch.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2] linux-user: ioctl() command type is int

[Riku Voipio]

On 16 June 2015 at 01:35, Laurent Vivier <laurent@vivier.eu> wrote:
> When executing a 64bit target chroot on 64bit host,
> the ioctl() command can mismatch.
>
> It seems the previous commit doesn't solve the problem in
> my case:
>
>         9c6bf9c7 linux-user: Fix ioctl cmd type mismatch on 64-bit targets
>
> For example, a ppc64 chroot on an x86_64 host:
>
> bash-4.3# ls
> Unsupported ioctl: cmd=0x80087467
> Unsupported ioctl: cmd=0x802c7415
>
> The origin of the problem is in syscall.c:do_ioctl().
>
>         static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)
>
> In this case (ppc64) abi_long is long (on the x86_64), and
>
>     cmd = 0x0000000080087467
>
> then
>         if (ie->target_cmd == cmd)
>
> target_cmd is int, so target_cmd = 0x80087467
> and to compare an int with a long, the sign is extended to 64bit,
> so the comparison is:
>
>         if (0xffffffff80087467 == 0x0000000080087467)
>
> which doesn't match whereas it should.
>
> This patch uses int in the case of the target command type
> instead of abi_long.
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
> v2: Don't modify IOCTLEntry type (useless and introduce clang errors)

Thanks, tested to build with clang, will refresh pull request in a moment.

>  linux-user/syscall.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index b98b7e7..5a280c3 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -3645,7 +3645,7 @@ enum {
>  typedef struct IOCTLEntry IOCTLEntry;
>
>  typedef abi_long do_ioctl_fn(const IOCTLEntry *ie, uint8_t *buf_temp,
> -                             int fd, abi_long cmd, abi_long arg);
> +                             int fd, int cmd, abi_long arg);
>
>  struct IOCTLEntry {
>      int target_cmd;
> @@ -3671,7 +3671,7 @@ struct IOCTLEntry {
>                              / sizeof(struct fiemap_extent))
>
>  static abi_long do_ioctl_fs_ioc_fiemap(const IOCTLEntry *ie, uint8_t *buf_temp,
> -                                       int fd, abi_long cmd, abi_long arg)
> +                                       int fd, int cmd, abi_long arg)
>  {
>      /* The parameter for this ioctl is a struct fiemap followed
>       * by an array of struct fiemap_extent whose size is set
> @@ -3752,7 +3752,7 @@ static abi_long do_ioctl_fs_ioc_fiemap(const IOCTLEntry *ie, uint8_t *buf_temp,
>  #endif
>
>  static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
> -                                int fd, abi_long cmd, abi_long arg)
> +                                int fd, int cmd, abi_long arg)
>  {
>      const argtype *arg_type = ie->arg_type;
>      int target_size;
> @@ -3846,7 +3846,7 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
>  }
>
>  static abi_long do_ioctl_dm(const IOCTLEntry *ie, uint8_t *buf_temp, int fd,
> -                            abi_long cmd, abi_long arg)
> +                            int cmd, abi_long arg)
>  {
>      void *argptr;
>      struct dm_ioctl *host_dm;
> @@ -4071,7 +4071,7 @@ out:
>  }
>
>  static abi_long do_ioctl_blkpg(const IOCTLEntry *ie, uint8_t *buf_temp, int fd,
> -                               abi_long cmd, abi_long arg)
> +                               int cmd, abi_long arg)
>  {
>      void *argptr;
>      int target_size;
> @@ -4124,7 +4124,7 @@ out:
>  }
>
>  static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
> -                                int fd, abi_long cmd, abi_long arg)
> +                                int fd, int cmd, abi_long arg)
>  {
>      const argtype *arg_type = ie->arg_type;
>      const StructEntry *se;
> @@ -4187,7 +4187,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
>  }
>
>  static abi_long do_ioctl_kdsigaccept(const IOCTLEntry *ie, uint8_t *buf_temp,
> -                                     int fd, abi_long cmd, abi_long arg)
> +                                     int fd, int cmd, abi_long arg)
>  {
>      int sig = target_to_host_signal(arg);
>      return get_errno(ioctl(fd, ie->host_cmd, sig));
> @@ -4204,7 +4204,7 @@ static IOCTLEntry ioctl_entries[] = {
>
>  /* ??? Implement proper locking for ioctls.  */
>  /* do_ioctl() Must return target values and target errnos. */
> -static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)
> +static abi_long do_ioctl(int fd, int cmd, abi_long arg)
>  {
>      const IOCTLEntry *ie;
>      const argtype *arg_type;
> --
> 2.4.3
>

[Qemu-devel] [PATCH] linux-user: ioctl() command type is int

[Laurent Vivier]

When executing a 64bit target chroot on 64bit host,
the ioctl() command can mismatch.

It seems the previous commit doesn't solve the problem in
my case:

	9c6bf9c7 linux-user: Fix ioctl cmd type mismatch on 64-bit targets

For example, a ppc64 chroot on an x86_64 host:

bash-4.3# ls
Unsupported ioctl: cmd=0x80087467
Unsupported ioctl: cmd=0x802c7415

The origin of the problem is in syscall.c:do_ioctl().

	static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)

In this case (ppc64) abi_long is long (on the x86_64), and

    cmd = 0x0000000080087467

then
	if (ie->target_cmd == cmd)

target_cmd is int, so target_cmd = 0x80087467
and to compare an int with a long, the sign is extended to 64bit,
so the comparison is:

	if (0xffffffff80087467 == 0x0000000080087467)

which doesn't match whereas it should.

This patch uses abi_int in the case of the target command type
instead of abi_long (and for consistency, update IOCTLEntry).

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 linux-user/syscall.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 1622ad6..68801cf 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -3293,8 +3293,8 @@ typedef abi_long do_ioctl_fn(const IOCTLEntry *ie, uint8_t *buf_temp,
                              int fd, abi_long cmd, abi_long arg);
 
 struct IOCTLEntry {
-    int target_cmd;
-    unsigned int host_cmd;
+    abi_int target_cmd;
+    int host_cmd;
     const char *name;
     int access;
     do_ioctl_fn *do_ioctl;
@@ -3849,7 +3849,7 @@ static IOCTLEntry ioctl_entries[] = {
 
 /* ??? Implement proper locking for ioctls.  */
 /* do_ioctl() Must return target values and target errnos. */
-static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)
+static abi_long do_ioctl(int fd, abi_int cmd, abi_long arg)
 {
     const IOCTLEntry *ie;
     const argtype *arg_type;
-- 
2.1.4

[Qemu-devel] [PATCH v2] linux-user: ioctl() command type is int

[Laurent Vivier]

When executing a 64bit target chroot on 64bit host,
the ioctl() command can mismatch.

It seems the previous commit doesn't solve the problem in
my case:

	9c6bf9c7 linux-user: Fix ioctl cmd type mismatch on 64-bit targets

For example, a ppc64 chroot on an x86_64 host:

bash-4.3# ls
Unsupported ioctl: cmd=0x80087467
Unsupported ioctl: cmd=0x802c7415

The origin of the problem is in syscall.c:do_ioctl().

	static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)

In this case (ppc64) abi_long is long (on the x86_64), and

    cmd = 0x0000000080087467

then
	if (ie->target_cmd == cmd)

target_cmd is int, so target_cmd = 0x80087467
and to compare an int with a long, the sign is extended to 64bit,
so the comparison is:

	if (0xffffffff80087467 == 0x0000000080087467)

which doesn't match whereas it should.

This patch uses int in the case of the target command type
instead of abi_long (and for consistency, update IOCTLEntry).

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
v2: use int instead of abi_int, as it is recommended by Peter Maydell.

 linux-user/syscall.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 1622ad6..c28cd05 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -3290,11 +3290,11 @@ enum {
 typedef struct IOCTLEntry IOCTLEntry;
 
 typedef abi_long do_ioctl_fn(const IOCTLEntry *ie, uint8_t *buf_temp,
-                             int fd, abi_long cmd, abi_long arg);
+                             int fd, int cmd, abi_long arg);
 
 struct IOCTLEntry {
     int target_cmd;
-    unsigned int host_cmd;
+    int host_cmd;
     const char *name;
     int access;
     do_ioctl_fn *do_ioctl;
@@ -3316,7 +3316,7 @@ struct IOCTLEntry {
                             / sizeof(struct fiemap_extent))
 
 static abi_long do_ioctl_fs_ioc_fiemap(const IOCTLEntry *ie, uint8_t *buf_temp,
-                                       int fd, abi_long cmd, abi_long arg)
+                                       int fd, int cmd, abi_long arg)
 {
     /* The parameter for this ioctl is a struct fiemap followed
      * by an array of struct fiemap_extent whose size is set
@@ -3397,7 +3397,7 @@ static abi_long do_ioctl_fs_ioc_fiemap(const IOCTLEntry *ie, uint8_t *buf_temp,
 #endif
 
 static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
-                                int fd, abi_long cmd, abi_long arg)
+                                int fd, int cmd, abi_long arg)
 {
     const argtype *arg_type = ie->arg_type;
     int target_size;
@@ -3491,7 +3491,7 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
 }
 
 static abi_long do_ioctl_dm(const IOCTLEntry *ie, uint8_t *buf_temp, int fd,
-                            abi_long cmd, abi_long arg)
+                            int cmd, abi_long arg)
 {
     void *argptr;
     struct dm_ioctl *host_dm;
@@ -3716,7 +3716,7 @@ out:
 }
 
 static abi_long do_ioctl_blkpg(const IOCTLEntry *ie, uint8_t *buf_temp, int fd,
-                               abi_long cmd, abi_long arg)
+                               int cmd, abi_long arg)
 {
     void *argptr;
     int target_size;
@@ -3769,7 +3769,7 @@ out:
 }
 
 static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
-                                int fd, abi_long cmd, abi_long arg)
+                                int fd, int cmd, abi_long arg)
 {
     const argtype *arg_type = ie->arg_type;
     const StructEntry *se;
@@ -3832,7 +3832,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
 }
 
 static abi_long do_ioctl_kdsigaccept(const IOCTLEntry *ie, uint8_t *buf_temp,
-                                     int fd, abi_long cmd, abi_long arg)
+                                     int fd, int cmd, abi_long arg)
 {
     int sig = target_to_host_signal(arg);
     return get_errno(ioctl(fd, ie->host_cmd, sig));
@@ -3849,7 +3849,7 @@ static IOCTLEntry ioctl_entries[] = {
 
 /* ??? Implement proper locking for ioctls.  */
 /* do_ioctl() Must return target values and target errnos. */
-static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)
+static abi_long do_ioctl(int fd, int cmd, abi_long arg)
 {
     const IOCTLEntry *ie;
     const argtype *arg_type;
-- 
2.1.4

Re: [Qemu-devel] [PATCH v2] linux-user: ioctl() command type is int

[Peter Maydell]

On 23 May 2015 at 14:17, Laurent Vivier <laurent@vivier.eu> wrote:
> When executing a 64bit target chroot on 64bit host,
> the ioctl() command can mismatch.
>
> It seems the previous commit doesn't solve the problem in
> my case:
>
>         9c6bf9c7 linux-user: Fix ioctl cmd type mismatch on 64-bit targets
>
> For example, a ppc64 chroot on an x86_64 host:
>
> bash-4.3# ls
> Unsupported ioctl: cmd=0x80087467
> Unsupported ioctl: cmd=0x802c7415
>
> The origin of the problem is in syscall.c:do_ioctl().
>
>         static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)
>
> In this case (ppc64) abi_long is long (on the x86_64), and
>
>     cmd = 0x0000000080087467
>
> then
>         if (ie->target_cmd == cmd)
>
> target_cmd is int, so target_cmd = 0x80087467
> and to compare an int with a long, the sign is extended to 64bit,
> so the comparison is:
>
>         if (0xffffffff80087467 == 0x0000000080087467)
>
> which doesn't match whereas it should.
>
> This patch uses int in the case of the target command type
> instead of abi_long (and for consistency, update IOCTLEntry).
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] linux-user: ioctl() command type is int

[Riku Voipio]

On Saturday, May 23, 2015 4:17:05 PM EEST, Laurent Vivier wrote:
> When executing a 64bit target chroot on 64bit host,
> the ioctl() command can mismatch.
>
> It seems the previous commit doesn't solve the problem in
> my case:
>
> 	9c6bf9c7 linux-user: Fix ioctl cmd type mismatch on 64-bit targets
>
> For example, a ppc64 chroot on an x86_64 host:
>
> bash-4.3# ls
> Unsupported ioctl: cmd=0x80087467
> Unsupported ioctl: cmd=0x802c7415
>
> The origin of the problem is in syscall.c:do_ioctl().
>
> 	static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)
>
> In this case (ppc64) abi_long is long (on the x86_64), and
>
>     cmd = 0x0000000080087467
>
> then
> 	if (ie->target_cmd == cmd)
>
> target_cmd is int, so target_cmd = 0x80087467
> and to compare an int with a long, the sign is extended to 64bit,
> so the comparison is:
>
> 	if (0xffffffff80087467 == 0x0000000080087467)
>
> which doesn't match whereas it should.
>
> This patch uses int in the case of the target command type
> instead of abi_long (and for consistency, update IOCTLEntry).
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>

Applied to linux-user que,

Thanks

> ---
> v2: use int instead of abi_int, as it is recommended by Peter Maydell.
>
>  linux-user/syscall.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 1622ad6..c28cd05 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -3290,11 +3290,11 @@ enum {
>  typedef struct IOCTLEntry IOCTLEntry;
>  
>  typedef abi_long do_ioctl_fn(const IOCTLEntry *ie, uint8_t *buf_temp,
> -                             int fd, abi_long cmd, abi_long arg);
> +                             int fd, int cmd, abi_long arg);
>  
>  struct IOCTLEntry {
>      int target_cmd;
> -    unsigned int host_cmd;
> +    int host_cmd;
>      const char *name;
>      int access;
>      do_ioctl_fn *do_ioctl;
> @@ -3316,7 +3316,7 @@ struct IOCTLEntry {
>                              / sizeof(struct fiemap_extent))
>  
>  static abi_long do_ioctl_fs_ioc_fiemap(const IOCTLEntry *ie, 
> uint8_t *buf_temp,
> -                                       int fd, abi_long cmd, abi_long arg)
> +                                       int fd, int cmd, abi_long arg)
>  {
>      /* The parameter for this ioctl is a struct fiemap followed
>       * by an array of struct fiemap_extent whose size is set
> @@ -3397,7 +3397,7 @@ static abi_long 
> do_ioctl_fs_ioc_fiemap(const IOCTLEntry *ie, uint8_t *buf_temp,
>  #endif
>  
>  static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
> -                                int fd, abi_long cmd, abi_long arg)
> +                                int fd, int cmd, abi_long arg)
>  {
>      const argtype *arg_type = ie->arg_type;
>      int target_size;
> @@ -3491,7 +3491,7 @@ static abi_long do_ioctl_ifconf(const 
> IOCTLEntry *ie, uint8_t *buf_temp,
>  }
>  
>  static abi_long do_ioctl_dm(const IOCTLEntry *ie, uint8_t 
> *buf_temp, int fd,
> -                            abi_long cmd, abi_long arg)
> +                            int cmd, abi_long arg)
>  {
>      void *argptr;
>      struct dm_ioctl *host_dm;
> @@ -3716,7 +3716,7 @@ out:
>  }
>  
>  static abi_long do_ioctl_blkpg(const IOCTLEntry *ie, uint8_t 
> *buf_temp, int fd,
> -                               abi_long cmd, abi_long arg)
> +                               int cmd, abi_long arg)
>  {
>      void *argptr;
>      int target_size;
> @@ -3769,7 +3769,7 @@ out:
>  }
>  
>  static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
> -                                int fd, abi_long cmd, abi_long arg)
> +                                int fd, int cmd, abi_long arg)
>  {
>      const argtype *arg_type = ie->arg_type;
>      const StructEntry *se;
> @@ -3832,7 +3832,7 @@ static abi_long do_ioctl_rt(const 
> IOCTLEntry *ie, uint8_t *buf_temp,
>  }
>  
>  static abi_long do_ioctl_kdsigaccept(const IOCTLEntry *ie, 
> uint8_t *buf_temp,
> -                                     int fd, abi_long cmd, abi_long arg)
> +                                     int fd, int cmd, abi_long arg)
>  {
>      int sig = target_to_host_signal(arg);
>      return get_errno(ioctl(fd, ie->host_cmd, sig));
> @@ -3849,7 +3849,7 @@ static IOCTLEntry ioctl_entries[] = {
>  
>  /* ??? Implement proper locking for ioctls.  */
>  /* do_ioctl() Must return target values and target errnos. */
> -static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)
> +static abi_long do_ioctl(int fd, int cmd, abi_long arg)
>  {
>      const IOCTLEntry *ie;
>      const argtype *arg_type;