Re: [Qemu-devel] [PATCH v4] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels
Le 14/07/2019 à 13:33, Arnd Bergmann a écrit :
> On Sun, Jul 14, 2019 at 12:41 PM Richard Henderson
> 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
On Sun, Jul 14, 2019 at 12:41 PM Richard Henderson
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
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
Patchew URL: https://patchew.org/QEMU/20190711173131.6347-1-laur...@vivier.eu/ Hi, This series seems to have some coding style problems. See output below for more information: Message-id: 20190711173131.6347-1-laur...@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-laur...@vivier.eu/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [Qemu-devel] [PATCH v4] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels
On Fri, Jul 12, 2019 at 3:50 PM Laurent Vivier 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, _tv->tv_sec);
> >> __put_user(host_tv->tv_usec, _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
Le 12/07/2019 à 15:36, Arnd Bergmann a écrit :
> On Fri, Jul 12, 2019 at 3:23 PM Laurent Vivier 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, _tv->tv_sec);
>> __put_user(host_tv->tv_usec, _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
On Fri, Jul 12, 2019 at 3:23 PM Laurent Vivier wrote:
>
> Le 12/07/2019 à 14:47, Arnd Bergmann a écrit :
> > On Fri, Jul 12, 2019 at 2:17 PM Laurent Vivier wrote:
> >>
> >> Le 11/07/2019 à 23:05, Arnd Bergmann a écrit :
> >>> On Thu, Jul 11, 2019 at 7:32 PM Laurent Vivier 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, _tv->tv_sec);
> __put_user(host_tv->tv_usec, _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
Le 12/07/2019 à 14:47, Arnd Bergmann a écrit :
> On Fri, Jul 12, 2019 at 2:17 PM Laurent Vivier wrote:
>>
>> Le 11/07/2019 à 23:05, Arnd Bergmann a écrit :
>>> On Thu, Jul 11, 2019 at 7:32 PM Laurent Vivier 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, _tv->tv_sec);
__put_user(host_tv->tv_usec, _tv->tv_usec);
...
Thanks,
Laurent
Re: [Qemu-devel] [PATCH v4] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels
On Fri, Jul 12, 2019 at 2:17 PM Laurent Vivier wrote:
>
> Le 11/07/2019 à 23:05, Arnd Bergmann a écrit :
> > On Thu, Jul 11, 2019 at 7:32 PM Laurent Vivier 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
Le 11/07/2019 à 23:05, Arnd Bergmann a écrit :
> On Thu, Jul 11, 2019 at 7:32 PM Laurent Vivier 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
On Thu, Jul 11, 2019 at 7:32 PM Laurent Vivier 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
Patchew URL: https://patchew.org/QEMU/20190711173131.6347-1-laur...@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-laur...@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-laur...@vivier.eu/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
