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

[Laurent Vivier]

Le 12/07/2019 à 14:47, Arnd Bergmann a écrit :
> On Fri, Jul 12, 2019 at 2:17 PM Laurent Vivier <address@hidden> wrote:
>>
>> Le 11/07/2019 à 23:05, Arnd Bergmann a écrit :
>>> On Thu, Jul 11, 2019 at 7:32 PM Laurent Vivier <address@hidden> 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