| Message ID | 20190617131103.1413-1-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels | expand |
Patchew URL: https://patchew.org/QEMU/20190617131103.1413-1-berrange@redhat.com/ Hi, This series seems to have some coding style problems. See output below for more information: Subject: [Qemu-devel] [PATCH v2] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels Type: series Message-id: 20190617131103.1413-1-berrange@redhat.com === 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 === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu - [tag update] patchew/20190615154352.26824-1-philmd@redhat.com -> patchew/20190615154352.26824-1-philmd@redhat.com Switched to a new branch 'test' aeb14b5 linux-user: fix to handle variably sized SIOCGSTAMP with new kernels === OUTPUT BEGIN === ERROR: line over 90 characters #78: FILE: linux-user/syscall_defs.h:755: +#define TARGET_SIOCGSTAMP_NEW TARGET_IOC(TARGET_IOC_READ, 's', 6, sizeof(long long) + sizeof(long)) /* Get stamp (timeval64) */ ERROR: line over 90 characters #79: FILE: linux-user/syscall_defs.h:756: +#define TARGET_SIOCGSTAMPNS_NEW TARGET_IOC(TARGET_IOC_READ, 's', 7, sizeof(long long) + sizeof(long)) /* Get stamp (timespec64) */ total: 2 errors, 0 warnings, 51 lines checked Commit aeb14b599d8b (linux-user: fix to handle variably sized SIOCGSTAMP with new kernels) 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/20190617131103.1413-1-berrange@redhat.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
On Mon, Jun 17, 2019 at 3:11 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > The SIOCGSTAMP symbol was previously defined in the > asm-generic/sockios.h header file. QEMU sees that header > indirectly via sys/socket.h > > In linux kernel commit 0768e17073dc527ccd18ed5f96ce85f9985e9115 > the asm-generic/sockios.h header no longer defines SIOCGSTAMP. > Instead it provides only SIOCGSTAMP_OLD, which only uses a > 32-bit time_t on 32-bit architectures. This is a bit misleading, as we still define SIOCGSTAMP in the right place. asm-generic/sockios.h should not be used by normal user space. > The linux/sockios.h header then defines SIOCGSTAMP using > either SIOCGSTAMP_OLD or SIOCGSTAMP_NEW as appropriate. If > SIOCGSTAMP_NEW is used, then the tv_sec field is 64-bit even > on 32-bit architectures > > To cope with this we must now define two separate syscalls, > with corresponding old and new sizes, as well as including > the new linux/sockios.h header. The overall concept seems right. A few more comments on details that may have gone wrong here. I'm not familiar with the qemu-user implementation, so it's mostly guesswork on my end. > IOCTL(SIOCGIWNAME, IOC_W | IOC_R, MK_PTR(MK_STRUCT(STRUCT_char_ifreq))) > IOCTL(SIOCSPGRP, IOC_W, MK_PTR(TYPE_INT)) /* pid_t */ > IOCTL(SIOCGPGRP, IOC_R, MK_PTR(TYPE_INT)) /* pid_t */ > + > +#ifdef SIOCGSTAMP_OLD > + IOCTL(SIOCGSTAMP_OLD, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval))) > +#else > IOCTL(SIOCGSTAMP, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval))) > +#endif > +#ifdef SIOCGSTAMPNS_OLD > + IOCTL(SIOCGSTAMPNS_OLD, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec))) > +#else > IOCTL(SIOCGSTAMPNS, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec))) > +#endif > +#ifdef SIOCGSTAMP_NEW > + IOCTL(SIOCGSTAMP_NEW, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval64))) > +#endif > +#ifdef SIOCGSTAMPNS_NEW > + IOCTL(SIOCGSTAMPNS_NEW, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec64))) > +#endif Is timespec64 a qemu type? How is it defined? > diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h > index 7f141f699c..7830b600e7 100644 > --- a/linux-user/syscall_defs.h > +++ b/linux-user/syscall_defs.h > @@ -750,6 +750,11 @@ struct target_pollfd { > > #define TARGET_SIOCGSTAMP 0x8906 /* Get stamp (timeval) */ > #define TARGET_SIOCGSTAMPNS 0x8907 /* Get stamp (timespec) */ > +#define TARGET_SIOCGSTAMP_OLD 0x8906 /* Get stamp (timeval) */ > +#define TARGET_SIOCGSTAMPNS_OLD 0x8907 /* Get stamp (timespec) */ Note that these types are architecture specific. It seems that only one architecture is left that has its own definitions though, so this is only broken on arch/sh for current linux (and remains broken). Future architectures, including 32-bit risc-v should only have the _NEW version and not support SIOCGSTAMP_OLD at all. When emulating risc-v user space on old kernels (pre-5.1), you may need to translate the ioctl command and all system calls that take a 64-bit time_t into the variants with a 32-bit time_t on the way into the kernel, and then back. Similarly, running an old user binary on a riscv32 machine, you may need to do the reverse translation. > +#define TARGET_SIOCGSTAMP_NEW TARGET_IOC(TARGET_IOC_READ, 's', 6, sizeof(long long) + sizeof(long)) /* Get stamp (timeval64) */ > +#define TARGET_SIOCGSTAMPNS_NEW TARGET_IOC(TARGET_IOC_READ, 's', 7, sizeof(long long) + sizeof(long)) /* Get stamp (timespec64) */ "sizeof(long long) + sizeof(long)" is not always the size of the argument to TARGET_SIOCGSTAMP{NS}_NEW. On 32-bit architectures, the size is two 64-bit values. sparc64 is potentially another special case, as 'struct timeval is 'long + int' there (12 bytes). On big-endian architectures, the nanoseconds are returned in the last four bytes of the 16-byte structure. > /* Networking ioctls */ > #define TARGET_SIOCADDRT 0x890B /* add routing table entry */ > diff --git a/linux-user/syscall_types.h b/linux-user/syscall_types.h > index b98a23b0f1..de4c5a5b6f 100644 > --- a/linux-user/syscall_types.h > +++ b/linux-user/syscall_types.h > @@ -20,6 +20,10 @@ STRUCT(timeval, > STRUCT(timespec, > MK_ARRAY(TYPE_LONG, 2)) > > +STRUCT(timeval64, TYPE_LONGLONG, TYPE_LONG) > + > +STRUCT(timespec64, TYPE_LONGLONG, TYPE_LONG) Same here. Arnd
Le 17/06/2019 à 15:11, Daniel P. Berrangé a écrit : > The SIOCGSTAMP symbol was previously defined in the > asm-generic/sockios.h header file. QEMU sees that header > indirectly via sys/socket.h > > In linux kernel commit 0768e17073dc527ccd18ed5f96ce85f9985e9115 > the asm-generic/sockios.h header no longer defines SIOCGSTAMP. > Instead it provides only SIOCGSTAMP_OLD, which only uses a > 32-bit time_t on 32-bit architectures. > > The linux/sockios.h header then defines SIOCGSTAMP using > either SIOCGSTAMP_OLD or SIOCGSTAMP_NEW as appropriate. If > SIOCGSTAMP_NEW is used, then the tv_sec field is 64-bit even > on 32-bit architectures > > To cope with this we must now define two separate syscalls, > with corresponding old and new sizes, as well as including > the new linux/sockios.h header. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > linux-user/ioctls.h | 15 +++++++++++++++ > linux-user/syscall.c | 1 + > linux-user/syscall_defs.h | 5 +++++ > linux-user/syscall_types.h | 4 ++++ > 4 files changed, 25 insertions(+) > > diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h > index 5e84dc7c3a..5a6d6def7e 100644 > --- a/linux-user/ioctls.h > +++ b/linux-user/ioctls.h > @@ -222,8 +222,23 @@ > IOCTL(SIOCGIWNAME, IOC_W | IOC_R, MK_PTR(MK_STRUCT(STRUCT_char_ifreq))) > IOCTL(SIOCSPGRP, IOC_W, MK_PTR(TYPE_INT)) /* pid_t */ > IOCTL(SIOCGPGRP, IOC_R, MK_PTR(TYPE_INT)) /* pid_t */ > + > +#ifdef SIOCGSTAMP_OLD > + IOCTL(SIOCGSTAMP_OLD, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval))) > +#else > IOCTL(SIOCGSTAMP, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval))) > +#endif > +#ifdef SIOCGSTAMPNS_OLD > + IOCTL(SIOCGSTAMPNS_OLD, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec))) > +#else > IOCTL(SIOCGSTAMPNS, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec))) > +#endif > +#ifdef SIOCGSTAMP_NEW > + IOCTL(SIOCGSTAMP_NEW, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval64))) > +#endif > +#ifdef SIOCGSTAMPNS_NEW > + IOCTL(SIOCGSTAMPNS_NEW, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec64))) > +#endif > > IOCTL(RNDGETENTCNT, IOC_R, MK_PTR(TYPE_INT)) > IOCTL(RNDADDTOENTCNT, IOC_W, MK_PTR(TYPE_INT)) > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index b187c1281d..f13e260b02 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -37,6 +37,7 @@ > #include <sched.h> > #include <sys/timex.h> > #include <sys/socket.h> > +#include <linux/sockios.h> > #include <sys/un.h> > #include <sys/uio.h> > #include <poll.h> > diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h > index 7f141f699c..7830b600e7 100644 > --- a/linux-user/syscall_defs.h > +++ b/linux-user/syscall_defs.h > @@ -750,6 +750,11 @@ struct target_pollfd { > > #define TARGET_SIOCGSTAMP 0x8906 /* Get stamp (timeval) */ > #define TARGET_SIOCGSTAMPNS 0x8907 /* Get stamp (timespec) */ > +#define TARGET_SIOCGSTAMP_OLD 0x8906 /* Get stamp (timeval) */ > +#define TARGET_SIOCGSTAMPNS_OLD 0x8907 /* Get stamp (timespec) */ > +#define TARGET_SIOCGSTAMP_NEW TARGET_IOC(TARGET_IOC_READ, 's', 6, sizeof(long long) + sizeof(long)) /* Get stamp (timeval64) */ > +#define TARGET_SIOCGSTAMPNS_NEW TARGET_IOC(TARGET_IOC_READ, 's', 7, sizeof(long long) + sizeof(long)) /* Get stamp (timespec64) */ kernel defines: #define SIOCGSTAMP_NEW _IOR(SOCK_IOC_TYPE, 0x06, long long[2]) #define SIOCGSTAMPNS_NEW _IOR(SOCK_IOC_TYPE, 0x07, long long[2]) So it should be TARGET_IOR(0x89, 0x6, abi_llong[2]) Their codes are 0x80108906 and 80108907. Thanks, Laurent
On Mon, Jun 17, 2019 at 5:35 PM Laurent Vivier <laurent@vivier.eu> wrote: > > Le 17/06/2019 à 15:11, Daniel P. Berrangé a écrit : > > The SIOCGSTAMP symbol was previously defined in the > > asm-generic/sockios.h header file. QEMU sees that header > > indirectly via sys/socket.h > > > > In linux kernel commit 0768e17073dc527ccd18ed5f96ce85f9985e9115 > > the asm-generic/sockios.h header no longer defines SIOCGSTAMP. > > Instead it provides only SIOCGSTAMP_OLD, which only uses a > > 32-bit time_t on 32-bit architectures. > > > > The linux/sockios.h header then defines SIOCGSTAMP using > > either SIOCGSTAMP_OLD or SIOCGSTAMP_NEW as appropriate. If > > SIOCGSTAMP_NEW is used, then the tv_sec field is 64-bit even > > on 32-bit architectures > > > > To cope with this we must now define two separate syscalls, > > with corresponding old and new sizes, as well as including > > the new linux/sockios.h header. > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > --- > > linux-user/ioctls.h | 15 +++++++++++++++ > > linux-user/syscall.c | 1 + > > linux-user/syscall_defs.h | 5 +++++ > > linux-user/syscall_types.h | 4 ++++ > > 4 files changed, 25 insertions(+) > > > > diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h > > index 5e84dc7c3a..5a6d6def7e 100644 > > --- a/linux-user/ioctls.h > > +++ b/linux-user/ioctls.h > > @@ -222,8 +222,23 @@ > > IOCTL(SIOCGIWNAME, IOC_W | IOC_R, MK_PTR(MK_STRUCT(STRUCT_char_ifreq))) > > IOCTL(SIOCSPGRP, IOC_W, MK_PTR(TYPE_INT)) /* pid_t */ > > IOCTL(SIOCGPGRP, IOC_R, MK_PTR(TYPE_INT)) /* pid_t */ > > + > > +#ifdef SIOCGSTAMP_OLD > > + IOCTL(SIOCGSTAMP_OLD, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval))) > > +#else > > IOCTL(SIOCGSTAMP, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval))) > > +#endif > > +#ifdef SIOCGSTAMPNS_OLD > > + IOCTL(SIOCGSTAMPNS_OLD, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec))) > > +#else > > IOCTL(SIOCGSTAMPNS, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec))) > > +#endif > > +#ifdef SIOCGSTAMP_NEW > > + IOCTL(SIOCGSTAMP_NEW, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval64))) > > +#endif > > +#ifdef SIOCGSTAMPNS_NEW > > + IOCTL(SIOCGSTAMPNS_NEW, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec64))) > > +#endif > > > > IOCTL(RNDGETENTCNT, IOC_R, MK_PTR(TYPE_INT)) > > IOCTL(RNDADDTOENTCNT, IOC_W, MK_PTR(TYPE_INT)) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > > index b187c1281d..f13e260b02 100644 > > --- a/linux-user/syscall.c > > +++ b/linux-user/syscall.c > > @@ -37,6 +37,7 @@ > > #include <sched.h> > > #include <sys/timex.h> > > #include <sys/socket.h> > > +#include <linux/sockios.h> > > #include <sys/un.h> > > #include <sys/uio.h> > > #include <poll.h> > > diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h > > index 7f141f699c..7830b600e7 100644 > > --- a/linux-user/syscall_defs.h > > +++ b/linux-user/syscall_defs.h > > @@ -750,6 +750,11 @@ struct target_pollfd { > > > > #define TARGET_SIOCGSTAMP 0x8906 /* Get stamp (timeval) */ > > #define TARGET_SIOCGSTAMPNS 0x8907 /* Get stamp (timespec) */ > > +#define TARGET_SIOCGSTAMP_OLD 0x8906 /* Get stamp (timeval) */ > > +#define TARGET_SIOCGSTAMPNS_OLD 0x8907 /* Get stamp (timespec) */ > > +#define TARGET_SIOCGSTAMP_NEW TARGET_IOC(TARGET_IOC_READ, 's', 6, sizeof(long long) + sizeof(long)) /* Get stamp (timeval64) */ > > +#define TARGET_SIOCGSTAMPNS_NEW TARGET_IOC(TARGET_IOC_READ, 's', 7, sizeof(long long) + sizeof(long)) /* Get stamp (timespec64) */ > kernel defines: > > #define SIOCGSTAMP_NEW _IOR(SOCK_IOC_TYPE, 0x06, long long[2]) > #define SIOCGSTAMPNS_NEW _IOR(SOCK_IOC_TYPE, 0x07, long long[2]) > > So it should be TARGET_IOR(0x89, 0x6, abi_llong[2]) > > Their codes are 0x80108906 and 80108907. Hi, I found the discussion around this topic being almost a month old. And related to this fedora bug [1] was closed by adding [2] which matches [3] that was nacked in the discussion here. Since I found nothing later (neither qemu commits nor further discussions) I wonder if it has fallen through the cracks OR if there was a kernel fix/change to resolve it (if that is the case a pointer to the related kernel change would be nice)? [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1718926 [2]: https://src.fedoraproject.org/rpms/qemu/blob/master/f/0005-NOT-UPSTREAM-Build-fix-with-latest-kernel.patch [3]: https://patchew.org/QEMU/20190604071915.288045-1-borntraeger@de.ibm.com/ > Thanks, > Laurent >
On Thu, Jul 11, 2019 at 10:02:01AM +0200, Christian Ehrhardt wrote: > On Mon, Jun 17, 2019 at 5:35 PM Laurent Vivier <laurent@vivier.eu> wrote: > > > > Le 17/06/2019 à 15:11, Daniel P. Berrangé a écrit : > > > The SIOCGSTAMP symbol was previously defined in the > > > asm-generic/sockios.h header file. QEMU sees that header > > > indirectly via sys/socket.h > > > > > > In linux kernel commit 0768e17073dc527ccd18ed5f96ce85f9985e9115 > > > the asm-generic/sockios.h header no longer defines SIOCGSTAMP. > > > Instead it provides only SIOCGSTAMP_OLD, which only uses a > > > 32-bit time_t on 32-bit architectures. > > > > > > The linux/sockios.h header then defines SIOCGSTAMP using > > > either SIOCGSTAMP_OLD or SIOCGSTAMP_NEW as appropriate. If > > > SIOCGSTAMP_NEW is used, then the tv_sec field is 64-bit even > > > on 32-bit architectures > > > > > > To cope with this we must now define two separate syscalls, > > > with corresponding old and new sizes, as well as including > > > the new linux/sockios.h header. > > > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > > --- > > > linux-user/ioctls.h | 15 +++++++++++++++ > > > linux-user/syscall.c | 1 + > > > linux-user/syscall_defs.h | 5 +++++ > > > linux-user/syscall_types.h | 4 ++++ > > > 4 files changed, 25 insertions(+) > > > > > > diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h > > > index 5e84dc7c3a..5a6d6def7e 100644 > > > --- a/linux-user/ioctls.h > > > +++ b/linux-user/ioctls.h > > > @@ -222,8 +222,23 @@ > > > IOCTL(SIOCGIWNAME, IOC_W | IOC_R, MK_PTR(MK_STRUCT(STRUCT_char_ifreq))) > > > IOCTL(SIOCSPGRP, IOC_W, MK_PTR(TYPE_INT)) /* pid_t */ > > > IOCTL(SIOCGPGRP, IOC_R, MK_PTR(TYPE_INT)) /* pid_t */ > > > + > > > +#ifdef SIOCGSTAMP_OLD > > > + IOCTL(SIOCGSTAMP_OLD, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval))) > > > +#else > > > IOCTL(SIOCGSTAMP, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval))) > > > +#endif > > > +#ifdef SIOCGSTAMPNS_OLD > > > + IOCTL(SIOCGSTAMPNS_OLD, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec))) > > > +#else > > > IOCTL(SIOCGSTAMPNS, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec))) > > > +#endif > > > +#ifdef SIOCGSTAMP_NEW > > > + IOCTL(SIOCGSTAMP_NEW, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval64))) > > > +#endif > > > +#ifdef SIOCGSTAMPNS_NEW > > > + IOCTL(SIOCGSTAMPNS_NEW, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec64))) > > > +#endif > > > > > > IOCTL(RNDGETENTCNT, IOC_R, MK_PTR(TYPE_INT)) > > > IOCTL(RNDADDTOENTCNT, IOC_W, MK_PTR(TYPE_INT)) > > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > > > index b187c1281d..f13e260b02 100644 > > > --- a/linux-user/syscall.c > > > +++ b/linux-user/syscall.c > > > @@ -37,6 +37,7 @@ > > > #include <sched.h> > > > #include <sys/timex.h> > > > #include <sys/socket.h> > > > +#include <linux/sockios.h> > > > #include <sys/un.h> > > > #include <sys/uio.h> > > > #include <poll.h> > > > diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h > > > index 7f141f699c..7830b600e7 100644 > > > --- a/linux-user/syscall_defs.h > > > +++ b/linux-user/syscall_defs.h > > > @@ -750,6 +750,11 @@ struct target_pollfd { > > > > > > #define TARGET_SIOCGSTAMP 0x8906 /* Get stamp (timeval) */ > > > #define TARGET_SIOCGSTAMPNS 0x8907 /* Get stamp (timespec) */ > > > +#define TARGET_SIOCGSTAMP_OLD 0x8906 /* Get stamp (timeval) */ > > > +#define TARGET_SIOCGSTAMPNS_OLD 0x8907 /* Get stamp (timespec) */ > > > +#define TARGET_SIOCGSTAMP_NEW TARGET_IOC(TARGET_IOC_READ, 's', 6, sizeof(long long) + sizeof(long)) /* Get stamp (timeval64) */ > > > +#define TARGET_SIOCGSTAMPNS_NEW TARGET_IOC(TARGET_IOC_READ, 's', 7, sizeof(long long) + sizeof(long)) /* Get stamp (timespec64) */ > > kernel defines: > > > > #define SIOCGSTAMP_NEW _IOR(SOCK_IOC_TYPE, 0x06, long long[2]) > > #define SIOCGSTAMPNS_NEW _IOR(SOCK_IOC_TYPE, 0x07, long long[2]) > > > > So it should be TARGET_IOR(0x89, 0x6, abi_llong[2]) > > > > Their codes are 0x80108906 and 80108907. > > Hi, > I found the discussion around this topic being almost a month old. > And related to this fedora bug [1] was closed by adding [2] which > matches [3] that was nacked in the discussion here. > > Since I found nothing later (neither qemu commits nor further > discussions) I wonder if it has fallen through the cracks OR if there > was a kernel fix/change to resolve it (if that is the case a pointer > to the related kernel change would be nice)? I didn't have time to address the feedback to this v2, and since the immediate problem for Fedora has a workaround, its been lower priority especially since my understanding of linux-iser is limited. IOW, If anyone wants to take over my patch proposal here feel free. It would obviously be nice to fix for this 4.1 release if practical. > > [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1718926 > [2]: https://src.fedoraproject.org/rpms/qemu/blob/master/f/0005-NOT-UPSTREAM-Build-fix-with-latest-kernel.patch > [3]: https://patchew.org/QEMU/20190604071915.288045-1-borntraeger@de.ibm.com/ > > > Thanks, > > Laurent > > > > > -- > Christian Ehrhardt > Software Engineer, Ubuntu Server > Canonical Ltd > Regards, Daniel
On 11.07.19 11:24, Daniel P. Berrangé wrote: > On Thu, Jul 11, 2019 at 10:02:01AM +0200, Christian Ehrhardt wrote: >> On Mon, Jun 17, 2019 at 5:35 PM Laurent Vivier <laurent@vivier.eu> wrote: >>> >>> Le 17/06/2019 à 15:11, Daniel P. Berrangé a écrit : >>>> The SIOCGSTAMP symbol was previously defined in the >>>> asm-generic/sockios.h header file. QEMU sees that header >>>> indirectly via sys/socket.h >>>> >>>> In linux kernel commit 0768e17073dc527ccd18ed5f96ce85f9985e9115 >>>> the asm-generic/sockios.h header no longer defines SIOCGSTAMP. >>>> Instead it provides only SIOCGSTAMP_OLD, which only uses a >>>> 32-bit time_t on 32-bit architectures. >>>> >>>> The linux/sockios.h header then defines SIOCGSTAMP using >>>> either SIOCGSTAMP_OLD or SIOCGSTAMP_NEW as appropriate. If >>>> SIOCGSTAMP_NEW is used, then the tv_sec field is 64-bit even >>>> on 32-bit architectures >>>> >>>> To cope with this we must now define two separate syscalls, >>>> with corresponding old and new sizes, as well as including >>>> the new linux/sockios.h header. >>>> >>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> >>>> --- >>>> linux-user/ioctls.h | 15 +++++++++++++++ >>>> linux-user/syscall.c | 1 + >>>> linux-user/syscall_defs.h | 5 +++++ >>>> linux-user/syscall_types.h | 4 ++++ >>>> 4 files changed, 25 insertions(+) >>>> >>>> diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h >>>> index 5e84dc7c3a..5a6d6def7e 100644 >>>> --- a/linux-user/ioctls.h >>>> +++ b/linux-user/ioctls.h >>>> @@ -222,8 +222,23 @@ >>>> IOCTL(SIOCGIWNAME, IOC_W | IOC_R, MK_PTR(MK_STRUCT(STRUCT_char_ifreq))) >>>> IOCTL(SIOCSPGRP, IOC_W, MK_PTR(TYPE_INT)) /* pid_t */ >>>> IOCTL(SIOCGPGRP, IOC_R, MK_PTR(TYPE_INT)) /* pid_t */ >>>> + >>>> +#ifdef SIOCGSTAMP_OLD >>>> + IOCTL(SIOCGSTAMP_OLD, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval))) >>>> +#else >>>> IOCTL(SIOCGSTAMP, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval))) >>>> +#endif >>>> +#ifdef SIOCGSTAMPNS_OLD >>>> + IOCTL(SIOCGSTAMPNS_OLD, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec))) >>>> +#else >>>> IOCTL(SIOCGSTAMPNS, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec))) >>>> +#endif >>>> +#ifdef SIOCGSTAMP_NEW >>>> + IOCTL(SIOCGSTAMP_NEW, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval64))) >>>> +#endif >>>> +#ifdef SIOCGSTAMPNS_NEW >>>> + IOCTL(SIOCGSTAMPNS_NEW, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec64))) >>>> +#endif >>>> >>>> IOCTL(RNDGETENTCNT, IOC_R, MK_PTR(TYPE_INT)) >>>> IOCTL(RNDADDTOENTCNT, IOC_W, MK_PTR(TYPE_INT)) >>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c >>>> index b187c1281d..f13e260b02 100644 >>>> --- a/linux-user/syscall.c >>>> +++ b/linux-user/syscall.c >>>> @@ -37,6 +37,7 @@ >>>> #include <sched.h> >>>> #include <sys/timex.h> >>>> #include <sys/socket.h> >>>> +#include <linux/sockios.h> >>>> #include <sys/un.h> >>>> #include <sys/uio.h> >>>> #include <poll.h> >>>> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h >>>> index 7f141f699c..7830b600e7 100644 >>>> --- a/linux-user/syscall_defs.h >>>> +++ b/linux-user/syscall_defs.h >>>> @@ -750,6 +750,11 @@ struct target_pollfd { >>>> >>>> #define TARGET_SIOCGSTAMP 0x8906 /* Get stamp (timeval) */ >>>> #define TARGET_SIOCGSTAMPNS 0x8907 /* Get stamp (timespec) */ >>>> +#define TARGET_SIOCGSTAMP_OLD 0x8906 /* Get stamp (timeval) */ >>>> +#define TARGET_SIOCGSTAMPNS_OLD 0x8907 /* Get stamp (timespec) */ >>>> +#define TARGET_SIOCGSTAMP_NEW TARGET_IOC(TARGET_IOC_READ, 's', 6, sizeof(long long) + sizeof(long)) /* Get stamp (timeval64) */ >>>> +#define TARGET_SIOCGSTAMPNS_NEW TARGET_IOC(TARGET_IOC_READ, 's', 7, sizeof(long long) + sizeof(long)) /* Get stamp (timespec64) */ >>> kernel defines: >>> >>> #define SIOCGSTAMP_NEW _IOR(SOCK_IOC_TYPE, 0x06, long long[2]) >>> #define SIOCGSTAMPNS_NEW _IOR(SOCK_IOC_TYPE, 0x07, long long[2]) >>> >>> So it should be TARGET_IOR(0x89, 0x6, abi_llong[2]) >>> >>> Their codes are 0x80108906 and 80108907. >> >> Hi, >> I found the discussion around this topic being almost a month old. >> And related to this fedora bug [1] was closed by adding [2] which >> matches [3] that was nacked in the discussion here. >> >> Since I found nothing later (neither qemu commits nor further >> discussions) I wonder if it has fallen through the cracks OR if there >> was a kernel fix/change to resolve it (if that is the case a pointer >> to the related kernel change would be nice)? > > I didn't have time to address the feedback to this v2, and since the > immediate problem for Fedora has a workaround, its been lower priority > especially since my understanding of linux-iser is limited. > > IOW, If anyone wants to take over my patch proposal here feel free. It > would obviously be nice to fix for this 4.1 release if practical. Same for me. I have never dealt with linux-user and my workaround was the simplest thing that I could come up with. Would be good it Laurent or other linux-user experts could do the "right thing". Adding Peter, for your awareness. qemu does not build with newer kernel headers. > >> >> [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1718926 >> [2]: https://src.fedoraproject.org/rpms/qemu/blob/master/f/0005-NOT-UPSTREAM-Build-fix-with-latest-kernel.patch >> [3]: https://patchew.org/QEMU/20190604071915.288045-1-borntraeger@de.ibm.com/ >> >>> Thanks, >>> Laurent >>> >> >> >> -- >> Christian Ehrhardt >> Software Engineer, Ubuntu Server >> Canonical Ltd >> > > Regards, > Daniel >
Le 11/07/2019 à 11:24, Daniel P. Berrangé a écrit : > On Thu, Jul 11, 2019 at 10:02:01AM +0200, Christian Ehrhardt wrote: >> On Mon, Jun 17, 2019 at 5:35 PM Laurent Vivier <laurent@vivier.eu> wrote: >>> >>> Le 17/06/2019 à 15:11, Daniel P. Berrangé a écrit : >>>> The SIOCGSTAMP symbol was previously defined in the >>>> asm-generic/sockios.h header file. QEMU sees that header >>>> indirectly via sys/socket.h >>>> >>>> In linux kernel commit 0768e17073dc527ccd18ed5f96ce85f9985e9115 >>>> the asm-generic/sockios.h header no longer defines SIOCGSTAMP. >>>> Instead it provides only SIOCGSTAMP_OLD, which only uses a >>>> 32-bit time_t on 32-bit architectures. >>>> >>>> The linux/sockios.h header then defines SIOCGSTAMP using >>>> either SIOCGSTAMP_OLD or SIOCGSTAMP_NEW as appropriate. If >>>> SIOCGSTAMP_NEW is used, then the tv_sec field is 64-bit even >>>> on 32-bit architectures >>>> >>>> To cope with this we must now define two separate syscalls, >>>> with corresponding old and new sizes, as well as including >>>> the new linux/sockios.h header. >>>> >>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> >>>> --- >>>> linux-user/ioctls.h | 15 +++++++++++++++ >>>> linux-user/syscall.c | 1 + >>>> linux-user/syscall_defs.h | 5 +++++ >>>> linux-user/syscall_types.h | 4 ++++ >>>> 4 files changed, 25 insertions(+) >>>> >>>> diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h >>>> index 5e84dc7c3a..5a6d6def7e 100644 >>>> --- a/linux-user/ioctls.h >>>> +++ b/linux-user/ioctls.h >>>> @@ -222,8 +222,23 @@ >>>> IOCTL(SIOCGIWNAME, IOC_W | IOC_R, MK_PTR(MK_STRUCT(STRUCT_char_ifreq))) >>>> IOCTL(SIOCSPGRP, IOC_W, MK_PTR(TYPE_INT)) /* pid_t */ >>>> IOCTL(SIOCGPGRP, IOC_R, MK_PTR(TYPE_INT)) /* pid_t */ >>>> + >>>> +#ifdef SIOCGSTAMP_OLD >>>> + IOCTL(SIOCGSTAMP_OLD, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval))) >>>> +#else >>>> IOCTL(SIOCGSTAMP, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval))) >>>> +#endif >>>> +#ifdef SIOCGSTAMPNS_OLD >>>> + IOCTL(SIOCGSTAMPNS_OLD, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec))) >>>> +#else >>>> IOCTL(SIOCGSTAMPNS, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec))) >>>> +#endif >>>> +#ifdef SIOCGSTAMP_NEW >>>> + IOCTL(SIOCGSTAMP_NEW, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval64))) >>>> +#endif >>>> +#ifdef SIOCGSTAMPNS_NEW >>>> + IOCTL(SIOCGSTAMPNS_NEW, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec64))) >>>> +#endif >>>> >>>> IOCTL(RNDGETENTCNT, IOC_R, MK_PTR(TYPE_INT)) >>>> IOCTL(RNDADDTOENTCNT, IOC_W, MK_PTR(TYPE_INT)) >>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c >>>> index b187c1281d..f13e260b02 100644 >>>> --- a/linux-user/syscall.c >>>> +++ b/linux-user/syscall.c >>>> @@ -37,6 +37,7 @@ >>>> #include <sched.h> >>>> #include <sys/timex.h> >>>> #include <sys/socket.h> >>>> +#include <linux/sockios.h> >>>> #include <sys/un.h> >>>> #include <sys/uio.h> >>>> #include <poll.h> >>>> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h >>>> index 7f141f699c..7830b600e7 100644 >>>> --- a/linux-user/syscall_defs.h >>>> +++ b/linux-user/syscall_defs.h >>>> @@ -750,6 +750,11 @@ struct target_pollfd { >>>> >>>> #define TARGET_SIOCGSTAMP 0x8906 /* Get stamp (timeval) */ >>>> #define TARGET_SIOCGSTAMPNS 0x8907 /* Get stamp (timespec) */ >>>> +#define TARGET_SIOCGSTAMP_OLD 0x8906 /* Get stamp (timeval) */ >>>> +#define TARGET_SIOCGSTAMPNS_OLD 0x8907 /* Get stamp (timespec) */ >>>> +#define TARGET_SIOCGSTAMP_NEW TARGET_IOC(TARGET_IOC_READ, 's', 6, sizeof(long long) + sizeof(long)) /* Get stamp (timeval64) */ >>>> +#define TARGET_SIOCGSTAMPNS_NEW TARGET_IOC(TARGET_IOC_READ, 's', 7, sizeof(long long) + sizeof(long)) /* Get stamp (timespec64) */ >>> kernel defines: >>> >>> #define SIOCGSTAMP_NEW _IOR(SOCK_IOC_TYPE, 0x06, long long[2]) >>> #define SIOCGSTAMPNS_NEW _IOR(SOCK_IOC_TYPE, 0x07, long long[2]) >>> >>> So it should be TARGET_IOR(0x89, 0x6, abi_llong[2]) >>> >>> Their codes are 0x80108906 and 80108907. >> >> Hi, >> I found the discussion around this topic being almost a month old. >> And related to this fedora bug [1] was closed by adding [2] which >> matches [3] that was nacked in the discussion here. >> >> Since I found nothing later (neither qemu commits nor further >> discussions) I wonder if it has fallen through the cracks OR if there >> was a kernel fix/change to resolve it (if that is the case a pointer >> to the related kernel change would be nice)? > > I didn't have time to address the feedback to this v2, and since the > immediate problem for Fedora has a workaround, its been lower priority > especially since my understanding of linux-iser is limited. > > IOW, If anyone wants to take over my patch proposal here feel free. It > would obviously be nice to fix for this 4.1 release if practical. I'm going to try to update your patch. Thanks, Laurent
diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h index 5e84dc7c3a..5a6d6def7e 100644 --- a/linux-user/ioctls.h +++ b/linux-user/ioctls.h @@ -222,8 +222,23 @@ IOCTL(SIOCGIWNAME, IOC_W | IOC_R, MK_PTR(MK_STRUCT(STRUCT_char_ifreq))) IOCTL(SIOCSPGRP, IOC_W, MK_PTR(TYPE_INT)) /* pid_t */ IOCTL(SIOCGPGRP, IOC_R, MK_PTR(TYPE_INT)) /* pid_t */ + +#ifdef SIOCGSTAMP_OLD + IOCTL(SIOCGSTAMP_OLD, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval))) +#else IOCTL(SIOCGSTAMP, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval))) +#endif +#ifdef SIOCGSTAMPNS_OLD + IOCTL(SIOCGSTAMPNS_OLD, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec))) +#else IOCTL(SIOCGSTAMPNS, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec))) +#endif +#ifdef SIOCGSTAMP_NEW + IOCTL(SIOCGSTAMP_NEW, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval64))) +#endif +#ifdef SIOCGSTAMPNS_NEW + IOCTL(SIOCGSTAMPNS_NEW, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec64))) +#endif IOCTL(RNDGETENTCNT, IOC_R, MK_PTR(TYPE_INT)) IOCTL(RNDADDTOENTCNT, IOC_W, MK_PTR(TYPE_INT)) diff --git a/linux-user/syscall.c b/linux-user/syscall.c index b187c1281d..f13e260b02 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -37,6 +37,7 @@ #include <sched.h> #include <sys/timex.h> #include <sys/socket.h> +#include <linux/sockios.h> #include <sys/un.h> #include <sys/uio.h> #include <poll.h> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h index 7f141f699c..7830b600e7 100644 --- a/linux-user/syscall_defs.h +++ b/linux-user/syscall_defs.h @@ -750,6 +750,11 @@ struct target_pollfd { #define TARGET_SIOCGSTAMP 0x8906 /* Get stamp (timeval) */ #define TARGET_SIOCGSTAMPNS 0x8907 /* Get stamp (timespec) */ +#define TARGET_SIOCGSTAMP_OLD 0x8906 /* Get stamp (timeval) */ +#define TARGET_SIOCGSTAMPNS_OLD 0x8907 /* Get stamp (timespec) */ +#define TARGET_SIOCGSTAMP_NEW TARGET_IOC(TARGET_IOC_READ, 's', 6, sizeof(long long) + sizeof(long)) /* Get stamp (timeval64) */ +#define TARGET_SIOCGSTAMPNS_NEW TARGET_IOC(TARGET_IOC_READ, 's', 7, sizeof(long long) + sizeof(long)) /* Get stamp (timespec64) */ + /* Networking ioctls */ #define TARGET_SIOCADDRT 0x890B /* add routing table entry */ diff --git a/linux-user/syscall_types.h b/linux-user/syscall_types.h index b98a23b0f1..de4c5a5b6f 100644 --- a/linux-user/syscall_types.h +++ b/linux-user/syscall_types.h @@ -20,6 +20,10 @@ STRUCT(timeval, STRUCT(timespec, MK_ARRAY(TYPE_LONG, 2)) +STRUCT(timeval64, TYPE_LONGLONG, TYPE_LONG) + +STRUCT(timespec64, TYPE_LONGLONG, TYPE_LONG) + STRUCT(rtentry, TYPE_ULONG, MK_STRUCT(STRUCT_sockaddr), MK_STRUCT(STRUCT_sockaddr), MK_STRUCT(STRUCT_sockaddr), TYPE_SHORT, TYPE_SHORT, TYPE_ULONG, TYPE_PTRVOID, TYPE_SHORT, TYPE_PTRVOID,
The SIOCGSTAMP symbol was previously defined in the asm-generic/sockios.h header file. QEMU sees that header indirectly via sys/socket.h In linux kernel commit 0768e17073dc527ccd18ed5f96ce85f9985e9115 the asm-generic/sockios.h header no longer defines SIOCGSTAMP. Instead it provides only SIOCGSTAMP_OLD, which only uses a 32-bit time_t on 32-bit architectures. The linux/sockios.h header then defines SIOCGSTAMP using either SIOCGSTAMP_OLD or SIOCGSTAMP_NEW as appropriate. If SIOCGSTAMP_NEW is used, then the tv_sec field is 64-bit even on 32-bit architectures To cope with this we must now define two separate syscalls, with corresponding old and new sizes, as well as including the new linux/sockios.h header. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- linux-user/ioctls.h | 15 +++++++++++++++ linux-user/syscall.c | 1 + linux-user/syscall_defs.h | 5 +++++ linux-user/syscall_types.h | 4 ++++ 4 files changed, 25 insertions(+)