[Qemu-devel] [PATCH v4] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels

Laurent Vivier posted 1 patch 4 years, 8 months ago
Test docker-clang@ubuntu passed
Test asan passed
Test docker-mingw@fedora passed
Test checkpatch failed
Test s390x passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190711173131.6347-1-laurent@vivier.eu
There is a newer version of this series
linux-user/ioctls.h        | 15 +++++++++++++++
linux-user/syscall.c       |  1 +
linux-user/syscall_defs.h  |  4 ++++
linux-user/syscall_types.h |  4 ++++
4 files changed, 24 insertions(+)
[Qemu-devel] [PATCH v4] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels
Posted by Laurent Vivier 4 years, 8 months ago
From: Daniel P. Berrangé <berrange@redhat.com>

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>
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---

Notes:
    v4: [lv] timeval64 and timespec64 are { long long , long }
    
    v3: [lv] redefine TARGET_SIOCGSTAMP_NEW, TARGET_SIOCGSTAMPNS_NEW,
        timeval64 and timespec64 to use 0x89 type and abi_llong[2]
    
    v2: [dpb] implement _NEW and _OLD variants

 linux-user/ioctls.h        | 15 +++++++++++++++
 linux-user/syscall.c       |  1 +
 linux-user/syscall_defs.h  |  4 ++++
 linux-user/syscall_types.h |  4 ++++
 4 files changed, 24 insertions(+)

diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h
index 5e84dc7c3a77..5a6d6def7e3f 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 39a37496fed5..aa18ac4b2389 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 fffa89f2564b..e0326923a018 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -751,6 +751,10 @@ 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_IOR(0x89, 0x06, abi_llong[2]) /* Get stamp (timeval64) */
+#define TARGET_SIOCGSTAMPNS_NEW TARGET_IOR(0x89, 0x07, abi_llong[2]) /* 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 b98a23b0f1b0..de4c5a5b6f5b 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,
-- 
2.21.0


Re: [Qemu-devel] [PATCH v4] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels
Posted by no-reply@patchew.org 4 years, 8 months ago
Patchew URL: https://patchew.org/QEMU/20190711173131.6347-1-laurent@vivier.eu/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20190711173131.6347-1-laurent@vivier.eu
Type: series
Subject: [Qemu-devel] [PATCH v4] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels

=== 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'
adb3405a06 linux-user: fix to handle variably sized SIOCGSTAMP with new kernels

=== OUTPUT BEGIN ===
ERROR: line over 90 characters
#79: FILE: linux-user/syscall_defs.h:756:
+#define TARGET_SIOCGSTAMP_NEW   TARGET_IOR(0x89, 0x06, abi_llong[2]) /* Get stamp (timeval64) */

ERROR: line over 90 characters
#80: FILE: linux-user/syscall_defs.h:757:
+#define TARGET_SIOCGSTAMPNS_NEW TARGET_IOR(0x89, 0x07, abi_llong[2]) /* Get stamp (timespec64) */

total: 2 errors, 0 warnings, 50 lines checked

Commit adb3405a06a4 (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/20190711173131.6347-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
Re: [Qemu-devel] [PATCH v4] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels
Posted by Arnd Bergmann 4 years, 8 months ago
On Thu, Jul 11, 2019 at 7:32 PM Laurent Vivier <laurent@vivier.eu> wrote:

>
> Notes:
>     v4: [lv] timeval64 and timespec64 are { long long , long }

>
> +STRUCT(timeval64, TYPE_LONGLONG, TYPE_LONG)
> +
> +STRUCT(timespec64, TYPE_LONGLONG, TYPE_LONG)
> +

This still doesn't look right, see my earlier comment about padding
on big-endian architectures.

Note that the in-kernel 'timespec64' is different from the uapi
'__kernel_timespec' exported by the kernel. I also still think you may
need to convert between SIOCGSTAMP_NEW and SIOCGSTAMP_OLD,
e.g. when emulating a 32-bit riscv process (which only use
SIOCGSTAMP_NEW) on a kernel that only understands
SIOCGSTAMP_OLD.

     Arnd

Re: [Qemu-devel] [PATCH v4] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels
Posted by Laurent Vivier 4 years, 8 months ago
Le 11/07/2019 à 23:05, Arnd Bergmann a écrit :
> On Thu, Jul 11, 2019 at 7:32 PM Laurent Vivier <laurent@vivier.eu> wrote:
> 
>>
>> Notes:
>>     v4: [lv] timeval64 and timespec64 are { long long , long }
> 
>>
>> +STRUCT(timeval64, TYPE_LONGLONG, TYPE_LONG)
>> +
>> +STRUCT(timespec64, TYPE_LONGLONG, TYPE_LONG)
>> +
> 
> This still doesn't look right, see my earlier comment about padding
> on big-endian architectures.
> 
> Note that the in-kernel 'timespec64' is different from the uapi
> '__kernel_timespec' exported by the kernel. I also still think you may
> need to convert between SIOCGSTAMP_NEW and SIOCGSTAMP_OLD,
> e.g. when emulating a 32-bit riscv process (which only use
> SIOCGSTAMP_NEW) on a kernel that only understands
> SIOCGSTAMP_OLD.

I agree.
I'm preparing a patch always using SIOCGSTAMP and SIOCGSTAMPNS on the
host (converting the structure when needed).

I've added the SH4 variant.
I've added the sparc64 variant too: does it means sparc64 use the same
structure internally for the OLD and NEW version?
What about sparc 32bit?

For big-endian, I didn't find in the kernel where the difference is
managed: a byte swapping of the 64bit value is not enough?

Thanks,
Laurent


Re: [Qemu-devel] [PATCH v4] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels
Posted by Arnd Bergmann 4 years, 8 months ago
On Fri, Jul 12, 2019 at 2:17 PM Laurent Vivier <laurent@vivier.eu> wrote:
>
> Le 11/07/2019 à 23:05, Arnd Bergmann a écrit :
> > On Thu, Jul 11, 2019 at 7:32 PM Laurent Vivier <laurent@vivier.eu> wrote:
> >
> >>
> >> Notes:
> >>     v4: [lv] timeval64 and timespec64 are { long long , long }
> >
> >>
> >> +STRUCT(timeval64, TYPE_LONGLONG, TYPE_LONG)
> >> +
> >> +STRUCT(timespec64, TYPE_LONGLONG, TYPE_LONG)
> >> +
> >
> > This still doesn't look right, see my earlier comment about padding
> > on big-endian architectures.
> >
> > Note that the in-kernel 'timespec64' is different from the uapi
> > '__kernel_timespec' exported by the kernel. I also still think you may
> > need to convert between SIOCGSTAMP_NEW and SIOCGSTAMP_OLD,
> > e.g. when emulating a 32-bit riscv process (which only use
> > SIOCGSTAMP_NEW) on a kernel that only understands
> > SIOCGSTAMP_OLD.
>
> I agree.
> I'm preparing a patch always using SIOCGSTAMP and SIOCGSTAMPNS on the
> host (converting the structure when needed).

That in turn would have the problem of breaking in 2038 when the
timestamp overflows.

> I've added the SH4 variant.

What is special about SH4?

> I've added the sparc64 variant too: does it means sparc64 use the same
> structure internally for the OLD and NEW version?

I had to look it up in the code. Yes, it does, timeval is 64/32/pad32,
timespec is 64/64 in both OLD and NEW for sparc.

> What about sparc 32bit?

sparc32 is like all other 32-bit targets: OLD uses 32/32, and
new uses 64/64.

> For big-endian, I didn't find in the kernel where the difference is
> managed: a byte swapping of the 64bit value is not enough?

No, you don't need to swap. The difference is only in the padding.
Since the kernel uses a 64/64 structure here, and user space
may have use 'long tv_nsec', you need to add the padding on
the correct side, like

struct timeval64 {
   long long tv_sec;
#if 32bit && big-endian
   long :32; /* anonymous padding */
#endif
   suseconds_t tv_usec;
#if (32bit && little-endian) || sparc64
   long :32;
#endif
};

         Arnd

Re: [Qemu-devel] [PATCH v4] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels
Posted by Laurent Vivier 4 years, 8 months ago
Le 12/07/2019 à 14:47, Arnd Bergmann a écrit :
> On Fri, Jul 12, 2019 at 2:17 PM Laurent Vivier <laurent@vivier.eu> wrote:
>>
>> Le 11/07/2019 à 23:05, Arnd Bergmann a écrit :
>>> On Thu, Jul 11, 2019 at 7:32 PM Laurent Vivier <laurent@vivier.eu> wrote:
>>>
>>>>
>>>> Notes:
>>>>     v4: [lv] timeval64 and timespec64 are { long long , long }
>>>
>>>>
>>>> +STRUCT(timeval64, TYPE_LONGLONG, TYPE_LONG)
>>>> +
>>>> +STRUCT(timespec64, TYPE_LONGLONG, TYPE_LONG)
>>>> +
>>>
>>> This still doesn't look right, see my earlier comment about padding
>>> on big-endian architectures.
>>>
>>> Note that the in-kernel 'timespec64' is different from the uapi
>>> '__kernel_timespec' exported by the kernel. I also still think you may
>>> need to convert between SIOCGSTAMP_NEW and SIOCGSTAMP_OLD,
>>> e.g. when emulating a 32-bit riscv process (which only use
>>> SIOCGSTAMP_NEW) on a kernel that only understands
>>> SIOCGSTAMP_OLD.
>>
>> I agree.
>> I'm preparing a patch always using SIOCGSTAMP and SIOCGSTAMPNS on the
>> host (converting the structure when needed).
> 
> That in turn would have the problem of breaking in 2038 when the
> timestamp overflows.

No, because SIOCGSTAMP and SIOCGSTAMPNS are aliased to the _NEW versions 
on system supporting them (yes, we need to rebuild the binary, but we have 19 
years to do that).

#define SIOCGSTAMP      ((sizeof(struct timeval))  == 8 ? \
                         SIOCGSTAMP_OLD   : SIOCGSTAMP_NEW)
#define SIOCGSTAMPNS    ((sizeof(struct timespec)) == 8 ? \
                         SIOCGSTAMPNS_OLD : SIOCGSTAMPNS_NEW)

> 
>> I've added the SH4 variant.> What is special about SH4?

The definition of _OLD is different:

#define SIOCGSTAMP_OLD  _IOR('s', 100, struct timeval) /* Get stamp (timeval) */
#define SIOCGSTAMPNS_OLD _IOR('s', 101, struct timespec) /* Get stamp (timespec) */


> 
>> I've added the sparc64 variant too: does it means sparc64 use the same
>> structure internally for the OLD and NEW version?
> 
> I had to look it up in the code. Yes, it does, timeval is 64/32/pad32,
> timespec is 64/64 in both OLD and NEW for sparc.
> 
>> What about sparc 32bit?
> 
> sparc32 is like all other 32-bit targets: OLD uses 32/32, and
> new uses 64/64.
> 
>> For big-endian, I didn't find in the kernel where the difference is
>> managed: a byte swapping of the 64bit value is not enough?
> 
> No, you don't need to swap. The difference is only in the padding.
> Since the kernel uses a 64/64 structure here, and user space
> may have use 'long tv_nsec', you need to add the padding on
> the correct side, like
> 
> struct timeval64 {
>    long long tv_sec;
> #if 32bit && big-endian
>    long :32; /* anonymous padding */
> #endif
>    suseconds_t tv_usec;
> #if (32bit && little-endian) || sparc64
>    long :32;
> #endif
> };

We don't do memcopy() but we set each field one by one, so the padding doesn't 
seem needed if we define correctly the user structure:

struct target_timeval64 {
    abi_llong tv_sec;
    abi_long tv_usec;
};

and do something like:

    struct target_timeval64 *target_tv;
    struct timeval *host_tv;
...
    __put_user(host_tv->tv_sec, &target_tv->tv_sec);
    __put_user(host_tv->tv_usec, &target_tv->tv_usec);
...

Thanks,
Laurent
    


Re: [Qemu-devel] [PATCH v4] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels
Posted by Arnd Bergmann 4 years, 8 months ago
On Fri, Jul 12, 2019 at 3:23 PM Laurent Vivier <laurent@vivier.eu> wrote:
>
> Le 12/07/2019 à 14:47, Arnd Bergmann a écrit :
> > On Fri, Jul 12, 2019 at 2:17 PM Laurent Vivier <laurent@vivier.eu> wrote:
> >>
> >> Le 11/07/2019 à 23:05, Arnd Bergmann a écrit :
> >>> On Thu, Jul 11, 2019 at 7:32 PM Laurent Vivier <laurent@vivier.eu> wrote:
> >>>
> >>>>
> >>>> Notes:
> >>>>     v4: [lv] timeval64 and timespec64 are { long long , long }
> >>>
> >>>>
> >>>> +STRUCT(timeval64, TYPE_LONGLONG, TYPE_LONG)
> >>>> +
> >>>> +STRUCT(timespec64, TYPE_LONGLONG, TYPE_LONG)
> >>>> +
> >>>
> >>> This still doesn't look right, see my earlier comment about padding
> >>> on big-endian architectures.
> >>>
> >>> Note that the in-kernel 'timespec64' is different from the uapi
> >>> '__kernel_timespec' exported by the kernel. I also still think you may
> >>> need to convert between SIOCGSTAMP_NEW and SIOCGSTAMP_OLD,
> >>> e.g. when emulating a 32-bit riscv process (which only use
> >>> SIOCGSTAMP_NEW) on a kernel that only understands
> >>> SIOCGSTAMP_OLD.
> >>
> >> I agree.
> >> I'm preparing a patch always using SIOCGSTAMP and SIOCGSTAMPNS on the
> >> host (converting the structure when needed).
> >
> > That in turn would have the problem of breaking in 2038 when the
> > timestamp overflows.
>
> No, because SIOCGSTAMP and SIOCGSTAMPNS are aliased to the _NEW versions
> on system supporting them (yes, we need to rebuild the binary, but we have 19
> years to do that).
>
> #define SIOCGSTAMP      ((sizeof(struct timeval))  == 8 ? \
>                          SIOCGSTAMP_OLD   : SIOCGSTAMP_NEW)
> #define SIOCGSTAMPNS    ((sizeof(struct timespec)) == 8 ? \
>                          SIOCGSTAMPNS_OLD : SIOCGSTAMPNS_NEW)

Right, makes sense.

> >
> >> I've added the SH4 variant.> What is special about SH4?
>
> The definition of _OLD is different:
>
> #define SIOCGSTAMP_OLD  _IOR('s', 100, struct timeval) /* Get stamp (timeval) */
> #define SIOCGSTAMPNS_OLD _IOR('s', 101, struct timespec) /* Get stamp (timespec) */

Ah, that one.


> > No, you don't need to swap. The difference is only in the padding.
> > Since the kernel uses a 64/64 structure here, and user space
> > may have use 'long tv_nsec', you need to add the padding on
> > the correct side, like
> >
> > struct timeval64 {
> >    long long tv_sec;
> > #if 32bit && big-endian
> >    long :32; /* anonymous padding */
> > #endif
> >    suseconds_t tv_usec;
> > #if (32bit && little-endian) || sparc64
> >    long :32;
> > #endif
> > };
>
> We don't do memcopy() but we set each field one by one, so the padding doesn't
> seem needed if we define correctly the user structure:
>
> struct target_timeval64 {
>     abi_llong tv_sec;
>     abi_long tv_usec;
> };
>
> and do something like:
>
>     struct target_timeval64 *target_tv;
>     struct timeval *host_tv;
> ...
>     __put_user(host_tv->tv_sec, &target_tv->tv_sec);
>     __put_user(host_tv->tv_usec, &target_tv->tv_usec);
> ...

That still seems wrong. The user application has a definition
of 'timeval' that contains the padding, so your definition has
to match that.

       Arnd

Re: [Qemu-devel] [PATCH v4] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels
Posted by Laurent Vivier 4 years, 8 months ago
Le 12/07/2019 à 15:36, Arnd Bergmann a écrit :
> On Fri, Jul 12, 2019 at 3:23 PM Laurent Vivier <laurent@vivier.eu> wrote:
>>
>> Le 12/07/2019 à 14:47, Arnd Bergmann a écrit :
...
>>> No, you don't need to swap. The difference is only in the padding.
>>> Since the kernel uses a 64/64 structure here, and user space
>>> may have use 'long tv_nsec', you need to add the padding on
>>> the correct side, like
>>>
>>> struct timeval64 {
>>>    long long tv_sec;
>>> #if 32bit && big-endian
>>>    long :32; /* anonymous padding */
>>> #endif
>>>    suseconds_t tv_usec;
>>> #if (32bit && little-endian) || sparc64
>>>    long :32;
>>> #endif
>>> };
>>
>> We don't do memcopy() but we set each field one by one, so the padding doesn't
>> seem needed if we define correctly the user structure:
>>
>> struct target_timeval64 {
>>     abi_llong tv_sec;
>>     abi_long tv_usec;
>> };
>>
>> and do something like:
>>
>>     struct target_timeval64 *target_tv;
>>     struct timeval *host_tv;
>> ...
>>     __put_user(host_tv->tv_sec, &target_tv->tv_sec);
>>     __put_user(host_tv->tv_usec, &target_tv->tv_usec);
>> ...
> 
> That still seems wrong. The user application has a definition
> of 'timeval' that contains the padding, so your definition has
> to match that.

I don't find this definition with the padding. Where it is defined?

We are at the syscall level, so structures are the ones provided by the
target to the syscall, and they can be converted by the libc if the one
from the user space differs.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH v4] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels
Posted by Arnd Bergmann 4 years, 8 months ago
On Fri, Jul 12, 2019 at 3:50 PM Laurent Vivier <laurent@vivier.eu> wrote:
> Le 12/07/2019 à 15:36, Arnd Bergmann a écrit :
> >> We don't do memcopy() but we set each field one by one, so the padding doesn't
> >> seem needed if we define correctly the user structure:
> >>
> >> struct target_timeval64 {
> >>     abi_llong tv_sec;
> >>     abi_long tv_usec;
> >> };
> >>
> >> and do something like:
> >>
> >>     struct target_timeval64 *target_tv;
> >>     struct timeval *host_tv;
> >> ...
> >>     __put_user(host_tv->tv_sec, &target_tv->tv_sec);
> >>     __put_user(host_tv->tv_usec, &target_tv->tv_usec);
> >> ...
> >
> > That still seems wrong. The user application has a definition
> > of 'timeval' that contains the padding, so your definition has
> > to match that.
>
> I don't find this definition with the padding. Where it is defined?
>
> We are at the syscall level, so structures are the ones provided by the
> target to the syscall, and they can be converted by the libc if the one
> from the user space differs.

glibc will have to create a definition that matches the kernel, which uses

struct __kernel_timespec {
    __s64 tv_sec;
    __s64 tv_nsec;
};

As posix requires tv_nsec to be 'long', you need padding between
tv_sec and tv_nsec to have a libc definition matching the kernel's
binary layout.

      Arnd

Re: [Qemu-devel] [PATCH v4] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels
Posted by Richard Henderson 4 years, 8 months ago
On 7/12/19 3:55 PM, Arnd Bergmann wrote:
> glibc will have to create a definition that matches the kernel, which uses
> 
> struct __kernel_timespec {
>     __s64 tv_sec;
>     __s64 tv_nsec;
> };
> 
> As posix requires tv_nsec to be 'long', you need padding between
> tv_sec and tv_nsec to have a libc definition matching the kernel's
> binary layout.

Yes, but that's glibc's lookout.  All qemu cares about emulating is the kernel
interface.  So I think Laurent is right here, in that two reads handle the
above structure just fine.


r~

Re: [Qemu-devel] [PATCH v4] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels
Posted by Arnd Bergmann 4 years, 8 months ago
On Sun, Jul 14, 2019 at 12:41 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 7/12/19 3:55 PM, Arnd Bergmann wrote:
> > glibc will have to create a definition that matches the kernel, which uses
> >
> > struct __kernel_timespec {
> >     __s64 tv_sec;
> >     __s64 tv_nsec;
> > };
> >
> > As posix requires tv_nsec to be 'long', you need padding between
> > tv_sec and tv_nsec to have a libc definition matching the kernel's
> > binary layout.
>
> Yes, but that's glibc's lookout.  All qemu cares about emulating is the kernel
> interface.  So I think Laurent is right here, in that two reads handle the
> above structure just fine.

But that only works if the structure defined by qemu matches the kernel's.

The structure that Laurent proposed

struct target_timeval64 {
    abi_llong tv_sec;
    abi_long tv_usec;
};

is not compatible with the kernel or the glibc structure.

      Arnd

Re: [Qemu-devel] [PATCH v4] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels
Posted by Laurent Vivier 4 years, 8 months ago
Le 14/07/2019 à 13:33, Arnd Bergmann a écrit :
> On Sun, Jul 14, 2019 at 12:41 PM Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 7/12/19 3:55 PM, Arnd Bergmann wrote:
>>> glibc will have to create a definition that matches the kernel, which uses
>>>
>>> struct __kernel_timespec {
>>>     __s64 tv_sec;
>>>     __s64 tv_nsec;
>>> };
>>>
>>> As posix requires tv_nsec to be 'long', you need padding between
>>> tv_sec and tv_nsec to have a libc definition matching the kernel's
>>> binary layout.
>>
>> Yes, but that's glibc's lookout.  All qemu cares about emulating is the kernel
>> interface.  So I think Laurent is right here, in that two reads handle the
>> above structure just fine.
> 
> But that only works if the structure defined by qemu matches the kernel's.
> 
> The structure that Laurent proposed
> 
> struct target_timeval64 {
>     abi_llong tv_sec;
>     abi_long tv_usec;
> };
> 
> is not compatible with the kernel or the glibc structure.

Yes, you're right. The next patch revision will change this to the
kernel structure one.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH v4] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels
Posted by no-reply@patchew.org 4 years, 8 months ago
Patchew URL: https://patchew.org/QEMU/20190711173131.6347-1-laurent@vivier.eu/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [PATCH v4] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels
Message-id: 20190711173131.6347-1-laurent@vivier.eu

=== 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'
adb3405 linux-user: fix to handle variably sized SIOCGSTAMP with new kernels

=== OUTPUT BEGIN ===
ERROR: line over 90 characters
#79: FILE: linux-user/syscall_defs.h:756:
+#define TARGET_SIOCGSTAMP_NEW   TARGET_IOR(0x89, 0x06, abi_llong[2]) /* Get stamp (timeval64) */

ERROR: line over 90 characters
#80: FILE: linux-user/syscall_defs.h:757:
+#define TARGET_SIOCGSTAMPNS_NEW TARGET_IOR(0x89, 0x07, abi_llong[2]) /* Get stamp (timespec64) */

total: 2 errors, 0 warnings, 50 lines checked

Commit adb3405a06a4 (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/20190711173131.6347-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