* [PATCH] y2038: replace timespec with timespec64 in clock_settime
@ 2020-09-21 12:32 chensong
2020-09-22 15:16 ` Jan Kiszka
0 siblings, 1 reply; 10+ messages in thread
From: chensong @ 2020-09-21 12:32 UTC (permalink / raw)
To: xenomai
Upstream has used timespec64 to replace timespec inside kernel and
used __kernel_timespec to replace timespect in syscalls, therefore
we must keep aligned with upstream.
clock_settime is a point to get started at, it involves 2 parts:
1, syscall in ./include/trace/events/cobalt-posix.h for 64bits
2, compat syscall in kernel/cobalt/posix/syscall32.c for 32bits
some new functions are implemented to keep the compatibility with
other syscalls which haven't been handled yet and will be removed
in the end of the work.
Signed-off-by: chensong <chensong@tj.kylinos.cn>
---
include/cobalt/kernel/clock.h | 6 +++---
include/cobalt/kernel/compat.h | 3 +++
kernel/cobalt/posix/clock.c | 28 ++++++++++++++++++++++------
kernel/cobalt/posix/clock.h | 12 +++++++++++-
kernel/cobalt/posix/compat.c | 11 +++++++++++
kernel/cobalt/posix/syscall32.c | 6 +++---
kernel/cobalt/trace/cobalt-posix.h | 24 ++++++++++++++++++++++--
7 files changed, 75 insertions(+), 15 deletions(-)
diff --git a/include/cobalt/kernel/clock.h b/include/cobalt/kernel/clock.h
index c26776a..cf9d5ec 100644
--- a/include/cobalt/kernel/clock.h
+++ b/include/cobalt/kernel/clock.h
@@ -52,7 +52,7 @@ struct xnclock {
xnticks_t (*read_raw)(struct xnclock *clock);
xnticks_t (*read_monotonic)(struct xnclock *clock);
int (*set_time)(struct xnclock *clock,
- const struct timespec *ts);
+ const struct timespec64 *ts);
xnsticks_t (*ns_to_ticks)(struct xnclock *clock,
xnsticks_t ns);
xnsticks_t (*ticks_to_ns)(struct xnclock *clock,
@@ -213,7 +213,7 @@ static inline xnticks_t xnclock_read_monotonic(struct xnclock *clock)
}
static inline int xnclock_set_time(struct xnclock *clock,
- const struct timespec *ts)
+ const struct timespec64 *ts)
{
if (likely(clock == &nkclock))
return -EINVAL;
@@ -266,7 +266,7 @@ static inline xnticks_t xnclock_read_monotonic(struct xnclock *clock)
}
static inline int xnclock_set_time(struct xnclock *clock,
- const struct timespec *ts)
+ const struct timespec64 *ts)
{
/*
* There is no way to change the core clock's idea of time.
diff --git a/include/cobalt/kernel/compat.h b/include/cobalt/kernel/compat.h
index 05754cb..70e2c03 100644
--- a/include/cobalt/kernel/compat.h
+++ b/include/cobalt/kernel/compat.h
@@ -89,6 +89,9 @@ struct compat_rtdm_mmap_request {
int sys32_get_timespec(struct timespec *ts,
const struct compat_timespec __user *cts);
+int sys64_get_timespec(struct timespec64 *ts,
+ const struct compat_timespec __user *cts);
+
int sys32_put_timespec(struct compat_timespec __user *cts,
const struct timespec *ts);
diff --git a/kernel/cobalt/posix/clock.c b/kernel/cobalt/posix/clock.c
index 561358e..c960d7b 100644
--- a/kernel/cobalt/posix/clock.c
+++ b/kernel/cobalt/posix/clock.c
@@ -194,7 +194,7 @@ COBALT_SYSCALL(clock_gettime, current,
return 0;
}
-int __cobalt_clock_settime(clockid_t clock_id, const struct timespec *ts)
+int __cobalt_clock_settime(clockid_t clock_id, const struct timespec64 *ts)
{
int _ret, ret = 0;
xnticks_t now;
@@ -207,7 +207,7 @@ int __cobalt_clock_settime(clockid_t clock_id, const struct timespec *ts)
case CLOCK_REALTIME:
xnlock_get_irqsave(&nklock, s);
now = xnclock_read_realtime(&nkclock);
- xnclock_adjust(&nkclock, (xnsticks_t) (ts2ns(ts) - now));
+ xnclock_adjust(&nkclock, (xnsticks_t) (ts2ns64(ts) - now));
xnlock_put_irqrestore(&nklock, s);
break;
default:
@@ -242,15 +242,31 @@ int __cobalt_clock_adjtime(clockid_t clock_id, struct timex *tx)
return 0;
}
+static int __cobalt_get_timespec64(struct timespec64 *ts,
+ const struct __kernel_timespec __user *uts)
+{
+ struct __kernel_timespec kts;
+ int ret;
+
+ ret = cobalt_copy_from_user(&kts, uts, sizeof(kts));
+ if (ret)
+ return -EFAULT;
+
+ ts->tv_sec = kts.tv_sec;
+ ts->tv_nsec = kts.tv_nsec;
+
+ return 0;
+
+}
COBALT_SYSCALL(clock_settime, current,
- (clockid_t clock_id, const struct timespec __user *u_ts))
+ (clockid_t clock_id, const struct __kernel_timespec __user *u_ts))
{
- struct timespec ts;
+ struct timespec64 new_ts;
- if (cobalt_copy_from_user(&ts, u_ts, sizeof(ts)))
+ if (__cobalt_get_timespec64(&new_ts, u_ts))
return -EFAULT;
- return __cobalt_clock_settime(clock_id, &ts);
+ return __cobalt_clock_settime(clock_id, &new_ts);
}
COBALT_SYSCALL(clock_adjtime, current,
diff --git a/kernel/cobalt/posix/clock.h b/kernel/cobalt/posix/clock.h
index 0b06b93..3b0f910 100644
--- a/kernel/cobalt/posix/clock.h
+++ b/kernel/cobalt/posix/clock.h
@@ -43,6 +43,16 @@ static inline xnticks_t ts2ns(const struct timespec *ts)
return nsecs;
}
+static inline xnticks_t ts2ns64(const struct timespec64 *ts)
+{
+ xnticks_t nsecs = ts->tv_nsec;
+
+ if (ts->tv_sec)
+ nsecs += (xnticks_t)ts->tv_sec * ONE_BILLION;
+
+ return nsecs;
+}
+
static inline xnticks_t tv2ns(const struct timeval *tv)
{
xnticks_t nsecs = tv->tv_usec * 1000;
@@ -86,7 +96,7 @@ int __cobalt_clock_gettime(clockid_t clock_id,
struct timespec *ts);
int __cobalt_clock_settime(clockid_t clock_id,
- const struct timespec *ts);
+ const struct timespec64 *ts);
int __cobalt_clock_adjtime(clockid_t clock_id,
struct timex *tx);
diff --git a/kernel/cobalt/posix/compat.c b/kernel/cobalt/posix/compat.c
index 17968bf..08aedcf 100644
--- a/kernel/cobalt/posix/compat.c
+++ b/kernel/cobalt/posix/compat.c
@@ -32,6 +32,17 @@ int sys32_get_timespec(struct timespec *ts,
}
EXPORT_SYMBOL_GPL(sys32_get_timespec);
+int sys64_get_timespec(struct timespec64 *ts,
+ const struct compat_timespec __user *cts)
+{
+ return (cts == NULL ||
+ !access_rok(cts, sizeof(*cts)) ||
+ __xn_get_user(ts->tv_sec, &cts->tv_sec) ||
+ __xn_get_user(ts->tv_nsec, &cts->tv_nsec)) ? -EFAULT : 0;
+}
+EXPORT_SYMBOL_GPL(sys64_get_timespec);
+
+
int sys32_put_timespec(struct compat_timespec __user *cts,
const struct timespec *ts)
{
diff --git a/kernel/cobalt/posix/syscall32.c b/kernel/cobalt/posix/syscall32.c
index faa7ef5..4b1dfcc 100644
--- a/kernel/cobalt/posix/syscall32.c
+++ b/kernel/cobalt/posix/syscall32.c
@@ -161,14 +161,14 @@ COBALT_SYSCALL32emu(clock_settime, current,
(clockid_t clock_id,
const struct compat_timespec __user *u_ts))
{
- struct timespec ts;
+ struct timespec64 new_ts;
int ret;
- ret = sys32_get_timespec(&ts, u_ts);
+ ret = sys64_get_timespec(&new_ts, u_ts);
if (ret)
return ret;
- return __cobalt_clock_settime(clock_id, &ts);
+ return __cobalt_clock_settime(clock_id, &new_ts);
}
COBALT_SYSCALL32emu(clock_adjtime, current,
diff --git a/kernel/cobalt/trace/cobalt-posix.h b/kernel/cobalt/trace/cobalt-posix.h
index aa78efb..f764872 100644
--- a/kernel/cobalt/trace/cobalt-posix.h
+++ b/kernel/cobalt/trace/cobalt-posix.h
@@ -748,6 +748,26 @@ DECLARE_EVENT_CLASS(cobalt_clock_timespec,
)
);
+DECLARE_EVENT_CLASS(cobalt_clock_timespec64,
+ TP_PROTO(clockid_t clk_id, const struct timespec64 *val),
+ TP_ARGS(clk_id, val),
+
+ TP_STRUCT__entry(
+ __field(clockid_t, clk_id)
+ __timespec_fields(val)
+ ),
+
+ TP_fast_assign(
+ __entry->clk_id = clk_id;
+ __assign_timespec(val, val);
+ ),
+
+ TP_printk("clock_id=%d timeval=(%ld.%09ld)",
+ __entry->clk_id,
+ __timespec_args(val)
+ )
+);
+
DEFINE_EVENT(cobalt_clock_timespec, cobalt_clock_getres,
TP_PROTO(clockid_t clk_id, const struct timespec *res),
TP_ARGS(clk_id, res)
@@ -758,8 +778,8 @@ DEFINE_EVENT(cobalt_clock_timespec, cobalt_clock_gettime,
TP_ARGS(clk_id, time)
);
-DEFINE_EVENT(cobalt_clock_timespec, cobalt_clock_settime,
- TP_PROTO(clockid_t clk_id, const struct timespec *time),
+DEFINE_EVENT(cobalt_clock_timespec64, cobalt_clock_settime,
+ TP_PROTO(clockid_t clk_id, const struct timespec64 *time),
TP_ARGS(clk_id, time)
);
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] y2038: replace timespec with timespec64 in clock_settime
2020-09-21 12:32 [PATCH] y2038: replace timespec with timespec64 in clock_settime chensong
@ 2020-09-22 15:16 ` Jan Kiszka
2020-09-23 1:40 ` song
0 siblings, 1 reply; 10+ messages in thread
From: Jan Kiszka @ 2020-09-22 15:16 UTC (permalink / raw)
To: chensong, xenomai
On 21.09.20 14:32, chensong wrote:
> Upstream has used timespec64 to replace timespec inside kernel and
> used __kernel_timespec to replace timespect in syscalls, therefore
> we must keep aligned with upstream.
>
> clock_settime is a point to get started at, it involves 2 parts:
> 1, syscall in ./include/trace/events/cobalt-posix.h for 64bits
> 2, compat syscall in kernel/cobalt/posix/syscall32.c for 32bits
>
> some new functions are implemented to keep the compatibility with
> other syscalls which haven't been handled yet and will be removed
> in the end of the work.
So, this switches also existing syscalls to 64-bit types, breaking the
current ABI, no? How much effort would it be to keep the old ABI, adding
a 64-bit one aside of it?
>
> Signed-off-by: chensong <chensong@tj.kylinos.cn>
> ---
> include/cobalt/kernel/clock.h | 6 +++---
> include/cobalt/kernel/compat.h | 3 +++
> kernel/cobalt/posix/clock.c | 28 ++++++++++++++++++++++------
> kernel/cobalt/posix/clock.h | 12 +++++++++++-
> kernel/cobalt/posix/compat.c | 11 +++++++++++
> kernel/cobalt/posix/syscall32.c | 6 +++---
> kernel/cobalt/trace/cobalt-posix.h | 24 ++++++++++++++++++++++--
> 7 files changed, 75 insertions(+), 15 deletions(-)
>
> diff --git a/include/cobalt/kernel/clock.h b/include/cobalt/kernel/clock.h
> index c26776a..cf9d5ec 100644
> --- a/include/cobalt/kernel/clock.h
> +++ b/include/cobalt/kernel/clock.h
> @@ -52,7 +52,7 @@ struct xnclock {
> xnticks_t (*read_raw)(struct xnclock *clock);
> xnticks_t (*read_monotonic)(struct xnclock *clock);
> int (*set_time)(struct xnclock *clock,
> - const struct timespec *ts);
> + const struct timespec64 *ts);
> xnsticks_t (*ns_to_ticks)(struct xnclock *clock,
> xnsticks_t ns);
> xnsticks_t (*ticks_to_ns)(struct xnclock *clock,
> @@ -213,7 +213,7 @@ static inline xnticks_t xnclock_read_monotonic(struct xnclock *clock)
> }
>
> static inline int xnclock_set_time(struct xnclock *clock,
> - const struct timespec *ts)
> + const struct timespec64 *ts)
> {
> if (likely(clock == &nkclock))
> return -EINVAL;
> @@ -266,7 +266,7 @@ static inline xnticks_t xnclock_read_monotonic(struct xnclock *clock)
> }
>
> static inline int xnclock_set_time(struct xnclock *clock,
> - const struct timespec *ts)
> + const struct timespec64 *ts)
> {
> /*
> * There is no way to change the core clock's idea of time.
> diff --git a/include/cobalt/kernel/compat.h b/include/cobalt/kernel/compat.h
> index 05754cb..70e2c03 100644
> --- a/include/cobalt/kernel/compat.h
> +++ b/include/cobalt/kernel/compat.h
> @@ -89,6 +89,9 @@ struct compat_rtdm_mmap_request {
> int sys32_get_timespec(struct timespec *ts,
> const struct compat_timespec __user *cts);
>
> +int sys64_get_timespec(struct timespec64 *ts,
> + const struct compat_timespec __user *cts);
> +
> int sys32_put_timespec(struct compat_timespec __user *cts,
> const struct timespec *ts);
>
> diff --git a/kernel/cobalt/posix/clock.c b/kernel/cobalt/posix/clock.c
> index 561358e..c960d7b 100644
> --- a/kernel/cobalt/posix/clock.c
> +++ b/kernel/cobalt/posix/clock.c
> @@ -194,7 +194,7 @@ COBALT_SYSCALL(clock_gettime, current,
> return 0;
> }
>
> -int __cobalt_clock_settime(clockid_t clock_id, const struct timespec *ts)
> +int __cobalt_clock_settime(clockid_t clock_id, const struct timespec64 *ts)
> {
> int _ret, ret = 0;
> xnticks_t now;
> @@ -207,7 +207,7 @@ int __cobalt_clock_settime(clockid_t clock_id, const struct timespec *ts)
> case CLOCK_REALTIME:
> xnlock_get_irqsave(&nklock, s);
> now = xnclock_read_realtime(&nkclock);
> - xnclock_adjust(&nkclock, (xnsticks_t) (ts2ns(ts) - now));
> + xnclock_adjust(&nkclock, (xnsticks_t) (ts2ns64(ts) - now));
> xnlock_put_irqrestore(&nklock, s);
> break;
> default:
> @@ -242,15 +242,31 @@ int __cobalt_clock_adjtime(clockid_t clock_id, struct timex *tx)
> return 0;
> }
>
> +static int __cobalt_get_timespec64(struct timespec64 *ts,
> + const struct __kernel_timespec __user *uts)
> +{
> + struct __kernel_timespec kts;
> + int ret;
> +
> + ret = cobalt_copy_from_user(&kts, uts, sizeof(kts));
> + if (ret)
> + return -EFAULT;
> +
> + ts->tv_sec = kts.tv_sec;
> + ts->tv_nsec = kts.tv_nsec;
> +
> + return 0;
> +
> +}
> COBALT_SYSCALL(clock_settime, current,
> - (clockid_t clock_id, const struct timespec __user *u_ts))
> + (clockid_t clock_id, const struct __kernel_timespec __user *u_ts))
> {
> - struct timespec ts;
> + struct timespec64 new_ts;
>
> - if (cobalt_copy_from_user(&ts, u_ts, sizeof(ts)))
> + if (__cobalt_get_timespec64(&new_ts, u_ts))
> return -EFAULT;
>
> - return __cobalt_clock_settime(clock_id, &ts);
> + return __cobalt_clock_settime(clock_id, &new_ts);
> }
>
> COBALT_SYSCALL(clock_adjtime, current,
> diff --git a/kernel/cobalt/posix/clock.h b/kernel/cobalt/posix/clock.h
> index 0b06b93..3b0f910 100644
> --- a/kernel/cobalt/posix/clock.h
> +++ b/kernel/cobalt/posix/clock.h
> @@ -43,6 +43,16 @@ static inline xnticks_t ts2ns(const struct timespec *ts)
> return nsecs;
> }
>
> +static inline xnticks_t ts2ns64(const struct timespec64 *ts)
> +{
> + xnticks_t nsecs = ts->tv_nsec;
> +
> + if (ts->tv_sec)
> + nsecs += (xnticks_t)ts->tv_sec * ONE_BILLION;
> +
> + return nsecs;
> +}
> +
> static inline xnticks_t tv2ns(const struct timeval *tv)
> {
> xnticks_t nsecs = tv->tv_usec * 1000;
> @@ -86,7 +96,7 @@ int __cobalt_clock_gettime(clockid_t clock_id,
> struct timespec *ts);
>
> int __cobalt_clock_settime(clockid_t clock_id,
> - const struct timespec *ts);
> + const struct timespec64 *ts);
>
> int __cobalt_clock_adjtime(clockid_t clock_id,
> struct timex *tx);
> diff --git a/kernel/cobalt/posix/compat.c b/kernel/cobalt/posix/compat.c
> index 17968bf..08aedcf 100644
> --- a/kernel/cobalt/posix/compat.c
> +++ b/kernel/cobalt/posix/compat.c
> @@ -32,6 +32,17 @@ int sys32_get_timespec(struct timespec *ts,
> }
> EXPORT_SYMBOL_GPL(sys32_get_timespec);
>
> +int sys64_get_timespec(struct timespec64 *ts,
> + const struct compat_timespec __user *cts)
> +{
> + return (cts == NULL ||
> + !access_rok(cts, sizeof(*cts)) ||
> + __xn_get_user(ts->tv_sec, &cts->tv_sec) ||
> + __xn_get_user(ts->tv_nsec, &cts->tv_nsec)) ? -EFAULT : 0;
> +}
> +EXPORT_SYMBOL_GPL(sys64_get_timespec);
> +
> +
> int sys32_put_timespec(struct compat_timespec __user *cts,
> const struct timespec *ts)
> {
> diff --git a/kernel/cobalt/posix/syscall32.c b/kernel/cobalt/posix/syscall32.c
> index faa7ef5..4b1dfcc 100644
> --- a/kernel/cobalt/posix/syscall32.c
> +++ b/kernel/cobalt/posix/syscall32.c
> @@ -161,14 +161,14 @@ COBALT_SYSCALL32emu(clock_settime, current,
> (clockid_t clock_id,
> const struct compat_timespec __user *u_ts))
> {
> - struct timespec ts;
> + struct timespec64 new_ts;
> int ret;
>
> - ret = sys32_get_timespec(&ts, u_ts);
> + ret = sys64_get_timespec(&new_ts, u_ts);
> if (ret)
> return ret;
>
> - return __cobalt_clock_settime(clock_id, &ts);
> + return __cobalt_clock_settime(clock_id, &new_ts);
> }
>
> COBALT_SYSCALL32emu(clock_adjtime, current,
> diff --git a/kernel/cobalt/trace/cobalt-posix.h b/kernel/cobalt/trace/cobalt-posix.h
> index aa78efb..f764872 100644
> --- a/kernel/cobalt/trace/cobalt-posix.h
> +++ b/kernel/cobalt/trace/cobalt-posix.h
> @@ -748,6 +748,26 @@ DECLARE_EVENT_CLASS(cobalt_clock_timespec,
> )
> );
>
> +DECLARE_EVENT_CLASS(cobalt_clock_timespec64,
> + TP_PROTO(clockid_t clk_id, const struct timespec64 *val),
> + TP_ARGS(clk_id, val),
> +
> + TP_STRUCT__entry(
> + __field(clockid_t, clk_id)
> + __timespec_fields(val)
> + ),
> +
> + TP_fast_assign(
> + __entry->clk_id = clk_id;
> + __assign_timespec(val, val);
> + ),
> +
> + TP_printk("clock_id=%d timeval=(%ld.%09ld)",
> + __entry->clk_id,
> + __timespec_args(val)
> + )
> +);
> +
> DEFINE_EVENT(cobalt_clock_timespec, cobalt_clock_getres,
> TP_PROTO(clockid_t clk_id, const struct timespec *res),
> TP_ARGS(clk_id, res)
> @@ -758,8 +778,8 @@ DEFINE_EVENT(cobalt_clock_timespec, cobalt_clock_gettime,
> TP_ARGS(clk_id, time)
> );
>
> -DEFINE_EVENT(cobalt_clock_timespec, cobalt_clock_settime,
> - TP_PROTO(clockid_t clk_id, const struct timespec *time),
> +DEFINE_EVENT(cobalt_clock_timespec64, cobalt_clock_settime,
> + TP_PROTO(clockid_t clk_id, const struct timespec64 *time),
> TP_ARGS(clk_id, time)
> );
>
>
Thanks for this first step! Did you also test that it works with older
kernels? I see no wrappers, so I suspect breakages.
Jan
--
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] y2038: replace timespec with timespec64 in clock_settime
2020-09-22 15:16 ` Jan Kiszka
@ 2020-09-23 1:40 ` song
2020-09-28 16:52 ` Jan Kiszka
0 siblings, 1 reply; 10+ messages in thread
From: song @ 2020-09-23 1:40 UTC (permalink / raw)
To: Jan Kiszka, xenomai
On 2020/9/22 下午11:16, Jan Kiszka wrote:
> On 21.09.20 14:32, chensong wrote:
>> Upstream has used timespec64 to replace timespec inside kernel and
>> used __kernel_timespec to replace timespect in syscalls, therefore
>> we must keep aligned with upstream.
>>
>> clock_settime is a point to get started at, it involves 2 parts:
>> 1, syscall in ./include/trace/events/cobalt-posix.h for 64bits
>> 2, compat syscall in kernel/cobalt/posix/syscall32.c for 32bits
>>
>> some new functions are implemented to keep the compatibility with
>> other syscalls which haven't been handled yet and will be removed
>> in the end of the work.
>
> So, this switches also existing syscalls to 64-bit types, breaking the
> current ABI, no? How much effort would it be to keep the old ABI, adding
> a 64-bit one aside of it?
Not breaking current ABI, i didn't add any new ABIs specific for 64bits,
i added some new static functions, like ts2ns and ts2ns64,
sys32_put_timespec and sys64_put_timespec, specific for clock_settime.
When all the syscalls has been finished, every one uses timespec64,
those new static functions can be removed.
e.g
clock_settime is the ABI i'm working on, it calls ts2ns, but ts2ns only
accepts timespec, clock_gettime uses ts2ns as well, therefore, i
couldn't make them both work, i have to add a new ts2ns64(timespec64),
after i finish clock_gettime and nobody else uses ts2ns, i can remove
ts2ns64 and switch back to ts2ns(timespec64 as its parameter)
Arnd Bergmann followed the similar process. He also added some ABIs
which 32bit is not compatible of, but not this one.
Effort, as far as i can tell, not too much, around 10 fucntions as my
estimation.
>
>>
>> Signed-off-by: chensong <chensong@tj.kylinos.cn>
>> ---
>> include/cobalt/kernel/clock.h | 6 +++---
>> include/cobalt/kernel/compat.h | 3 +++
>> kernel/cobalt/posix/clock.c | 28 ++++++++++++++++++++++------
>> kernel/cobalt/posix/clock.h | 12 +++++++++++-
>> kernel/cobalt/posix/compat.c | 11 +++++++++++
>> kernel/cobalt/posix/syscall32.c | 6 +++---
>> kernel/cobalt/trace/cobalt-posix.h | 24 ++++++++++++++++++++++--
>> 7 files changed, 75 insertions(+), 15 deletions(-)
>>
>> diff --git a/include/cobalt/kernel/clock.h
>> b/include/cobalt/kernel/clock.h
>> index c26776a..cf9d5ec 100644
>> --- a/include/cobalt/kernel/clock.h
>> +++ b/include/cobalt/kernel/clock.h
>> @@ -52,7 +52,7 @@ struct xnclock {
>> xnticks_t (*read_raw)(struct xnclock *clock);
>> xnticks_t (*read_monotonic)(struct xnclock *clock);
>> int (*set_time)(struct xnclock *clock,
>> - const struct timespec *ts);
>> + const struct timespec64 *ts);
>> xnsticks_t (*ns_to_ticks)(struct xnclock *clock,
>> xnsticks_t ns);
>> xnsticks_t (*ticks_to_ns)(struct xnclock *clock,
>> @@ -213,7 +213,7 @@ static inline xnticks_t
>> xnclock_read_monotonic(struct xnclock *clock)
>> }
>> static inline int xnclock_set_time(struct xnclock *clock,
>> - const struct timespec *ts)
>> + const struct timespec64 *ts)
>> {
>> if (likely(clock == &nkclock))
>> return -EINVAL;
>> @@ -266,7 +266,7 @@ static inline xnticks_t
>> xnclock_read_monotonic(struct xnclock *clock)
>> }
>> static inline int xnclock_set_time(struct xnclock *clock,
>> - const struct timespec *ts)
>> + const struct timespec64 *ts)
>> {
>> /*
>> * There is no way to change the core clock's idea of time.
>> diff --git a/include/cobalt/kernel/compat.h
>> b/include/cobalt/kernel/compat.h
>> index 05754cb..70e2c03 100644
>> --- a/include/cobalt/kernel/compat.h
>> +++ b/include/cobalt/kernel/compat.h
>> @@ -89,6 +89,9 @@ struct compat_rtdm_mmap_request {
>> int sys32_get_timespec(struct timespec *ts,
>> const struct compat_timespec __user *cts);
>> +int sys64_get_timespec(struct timespec64 *ts,
>> + const struct compat_timespec __user *cts);
>> +
>> int sys32_put_timespec(struct compat_timespec __user *cts,
>> const struct timespec *ts);
>> diff --git a/kernel/cobalt/posix/clock.c b/kernel/cobalt/posix/clock.c
>> index 561358e..c960d7b 100644
>> --- a/kernel/cobalt/posix/clock.c
>> +++ b/kernel/cobalt/posix/clock.c
>> @@ -194,7 +194,7 @@ COBALT_SYSCALL(clock_gettime, current,
>> return 0;
>> }
>> -int __cobalt_clock_settime(clockid_t clock_id, const struct timespec
>> *ts)
>> +int __cobalt_clock_settime(clockid_t clock_id, const struct
>> timespec64 *ts)
>> {
>> int _ret, ret = 0;
>> xnticks_t now;
>> @@ -207,7 +207,7 @@ int __cobalt_clock_settime(clockid_t clock_id,
>> const struct timespec *ts)
>> case CLOCK_REALTIME:
>> xnlock_get_irqsave(&nklock, s);
>> now = xnclock_read_realtime(&nkclock);
>> - xnclock_adjust(&nkclock, (xnsticks_t) (ts2ns(ts) - now));
>> + xnclock_adjust(&nkclock, (xnsticks_t) (ts2ns64(ts) - now));
>> xnlock_put_irqrestore(&nklock, s);
>> break;
>> default:
>> @@ -242,15 +242,31 @@ int __cobalt_clock_adjtime(clockid_t clock_id,
>> struct timex *tx)
>> return 0;
>> }
>> +static int __cobalt_get_timespec64(struct timespec64 *ts,
>> + const struct __kernel_timespec __user *uts)
>> +{
>> + struct __kernel_timespec kts;
>> + int ret;
>> +
>> + ret = cobalt_copy_from_user(&kts, uts, sizeof(kts));
>> + if (ret)
>> + return -EFAULT;
>> +
>> + ts->tv_sec = kts.tv_sec;
>> + ts->tv_nsec = kts.tv_nsec;
>> +
>> + return 0;
>> +
>> +}
>> COBALT_SYSCALL(clock_settime, current,
>> - (clockid_t clock_id, const struct timespec __user *u_ts))
>> + (clockid_t clock_id, const struct __kernel_timespec __user *u_ts))
>> {
>> - struct timespec ts;
>> + struct timespec64 new_ts;
>> - if (cobalt_copy_from_user(&ts, u_ts, sizeof(ts)))
>> + if (__cobalt_get_timespec64(&new_ts, u_ts))
>> return -EFAULT;
>> - return __cobalt_clock_settime(clock_id, &ts);
>> + return __cobalt_clock_settime(clock_id, &new_ts);
>> }
>> COBALT_SYSCALL(clock_adjtime, current,
>> diff --git a/kernel/cobalt/posix/clock.h b/kernel/cobalt/posix/clock.h
>> index 0b06b93..3b0f910 100644
>> --- a/kernel/cobalt/posix/clock.h
>> +++ b/kernel/cobalt/posix/clock.h
>> @@ -43,6 +43,16 @@ static inline xnticks_t ts2ns(const struct timespec
>> *ts)
>> return nsecs;
>> }
>> +static inline xnticks_t ts2ns64(const struct timespec64 *ts)
>> +{
>> + xnticks_t nsecs = ts->tv_nsec;
>> +
>> + if (ts->tv_sec)
>> + nsecs += (xnticks_t)ts->tv_sec * ONE_BILLION;
>> +
>> + return nsecs;
>> +}
>> +
>> static inline xnticks_t tv2ns(const struct timeval *tv)
>> {
>> xnticks_t nsecs = tv->tv_usec * 1000;
>> @@ -86,7 +96,7 @@ int __cobalt_clock_gettime(clockid_t clock_id,
>> struct timespec *ts);
>> int __cobalt_clock_settime(clockid_t clock_id,
>> - const struct timespec *ts);
>> + const struct timespec64 *ts);
>> int __cobalt_clock_adjtime(clockid_t clock_id,
>> struct timex *tx);
>> diff --git a/kernel/cobalt/posix/compat.c b/kernel/cobalt/posix/compat.c
>> index 17968bf..08aedcf 100644
>> --- a/kernel/cobalt/posix/compat.c
>> +++ b/kernel/cobalt/posix/compat.c
>> @@ -32,6 +32,17 @@ int sys32_get_timespec(struct timespec *ts,
>> }
>> EXPORT_SYMBOL_GPL(sys32_get_timespec);
>> +int sys64_get_timespec(struct timespec64 *ts,
>> + const struct compat_timespec __user *cts)
>> +{
>> + return (cts == NULL ||
>> + !access_rok(cts, sizeof(*cts)) ||
>> + __xn_get_user(ts->tv_sec, &cts->tv_sec) ||
>> + __xn_get_user(ts->tv_nsec, &cts->tv_nsec)) ? -EFAULT : 0;
>> +}
>> +EXPORT_SYMBOL_GPL(sys64_get_timespec);
>> +
>> +
>> int sys32_put_timespec(struct compat_timespec __user *cts,
>> const struct timespec *ts)
>> {
>> diff --git a/kernel/cobalt/posix/syscall32.c
>> b/kernel/cobalt/posix/syscall32.c
>> index faa7ef5..4b1dfcc 100644
>> --- a/kernel/cobalt/posix/syscall32.c
>> +++ b/kernel/cobalt/posix/syscall32.c
>> @@ -161,14 +161,14 @@ COBALT_SYSCALL32emu(clock_settime, current,
>> (clockid_t clock_id,
>> const struct compat_timespec __user *u_ts))
>> {
>> - struct timespec ts;
>> + struct timespec64 new_ts;
>> int ret;
>> - ret = sys32_get_timespec(&ts, u_ts);
>> + ret = sys64_get_timespec(&new_ts, u_ts);
>> if (ret)
>> return ret;
>> - return __cobalt_clock_settime(clock_id, &ts);
>> + return __cobalt_clock_settime(clock_id, &new_ts);
>> }
>> COBALT_SYSCALL32emu(clock_adjtime, current,
>> diff --git a/kernel/cobalt/trace/cobalt-posix.h
>> b/kernel/cobalt/trace/cobalt-posix.h
>> index aa78efb..f764872 100644
>> --- a/kernel/cobalt/trace/cobalt-posix.h
>> +++ b/kernel/cobalt/trace/cobalt-posix.h
>> @@ -748,6 +748,26 @@ DECLARE_EVENT_CLASS(cobalt_clock_timespec,
>> )
>> );
>> +DECLARE_EVENT_CLASS(cobalt_clock_timespec64,
>> + TP_PROTO(clockid_t clk_id, const struct timespec64 *val),
>> + TP_ARGS(clk_id, val),
>> +
>> + TP_STRUCT__entry(
>> + __field(clockid_t, clk_id)
>> + __timespec_fields(val)
>> + ),
>> +
>> + TP_fast_assign(
>> + __entry->clk_id = clk_id;
>> + __assign_timespec(val, val);
>> + ),
>> +
>> + TP_printk("clock_id=%d timeval=(%ld.%09ld)",
>> + __entry->clk_id,
>> + __timespec_args(val)
>> + )
>> +);
>> +
>> DEFINE_EVENT(cobalt_clock_timespec, cobalt_clock_getres,
>> TP_PROTO(clockid_t clk_id, const struct timespec *res),
>> TP_ARGS(clk_id, res)
>> @@ -758,8 +778,8 @@ DEFINE_EVENT(cobalt_clock_timespec,
>> cobalt_clock_gettime,
>> TP_ARGS(clk_id, time)
>> );
>> -DEFINE_EVENT(cobalt_clock_timespec, cobalt_clock_settime,
>> - TP_PROTO(clockid_t clk_id, const struct timespec *time),
>> +DEFINE_EVENT(cobalt_clock_timespec64, cobalt_clock_settime,
>> + TP_PROTO(clockid_t clk_id, const struct timespec64 *time),
>> TP_ARGS(clk_id, time)
>> );
>>
>
> Thanks for this first step! Did you also test that it works with older
> kernels? I see no wrappers, so I suspect breakages.
I only have 64bit system, both 32bit process and 64bit process are
working fine with clock_settime.
32bit process could still break unless it's recompiled as you said in
last email. But in theory, it could be 2038safe after recompiled because
process and system are both use 64bit sec. I will work on it when i
start clock_gettime.
I tried to use timespec64 explicitly in my userspace, but i got this
"storage size of ts isn't know"
>
> Jan
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] y2038: replace timespec with timespec64 in clock_settime
2020-09-23 1:40 ` song
@ 2020-09-28 16:52 ` Jan Kiszka
2020-09-29 6:27 ` chensong
0 siblings, 1 reply; 10+ messages in thread
From: Jan Kiszka @ 2020-09-28 16:52 UTC (permalink / raw)
To: song, xenomai
On 23.09.20 03:40, song wrote:
>
>
> On 2020/9/22 下午11:16, Jan Kiszka wrote:
>> On 21.09.20 14:32, chensong wrote:
>>> Upstream has used timespec64 to replace timespec inside kernel and
>>> used __kernel_timespec to replace timespect in syscalls, therefore
>>> we must keep aligned with upstream.
>>>
>>> clock_settime is a point to get started at, it involves 2 parts:
>>> 1, syscall in ./include/trace/events/cobalt-posix.h for 64bits
>>> 2, compat syscall in kernel/cobalt/posix/syscall32.c for 32bits
>>>
>>> some new functions are implemented to keep the compatibility with
>>> other syscalls which haven't been handled yet and will be removed
>>> in the end of the work.
>>
>> So, this switches also existing syscalls to 64-bit types, breaking the
>> current ABI, no? How much effort would it be to keep the old ABI,
>> adding a 64-bit one aside of it?
>
> Not breaking current ABI, i didn't add any new ABIs specific for 64bits,
> i added some new static functions, like ts2ns and ts2ns64,
> sys32_put_timespec and sys64_put_timespec, specific for clock_settime.
>
clock_settime accepts struct timespec so far, you are changing that to
struct __kernel_timespec. If I look at its definition in the kernel, it
uses __kernel_time64_t as tv_sec type. But our current 32-bit users
still expects a 32-bit type here, no? Would that change already require
a new clock_settime64 syscall if we wanted to stay binary compatible?
>
> When all the syscalls has been finished, every one uses timespec64,
> those new static functions can be removed.
>
> e.g
> clock_settime is the ABI i'm working on, it calls ts2ns, but ts2ns only
> accepts timespec, clock_gettime uses ts2ns as well, therefore, i
> couldn't make them both work, i have to add a new ts2ns64(timespec64),
> after i finish clock_gettime and nobody else uses ts2ns, i can remove
> ts2ns64 and switch back to ts2ns(timespec64 as its parameter)
>
> Arnd Bergmann followed the similar process. He also added some ABIs
> which 32bit is not compatible of, but not this one.
>
> Effort, as far as i can tell, not too much, around 10 fucntions as my
> estimation.
>
>>
>>>
>>> Signed-off-by: chensong <chensong@tj.kylinos.cn>
>>> ---
>>> include/cobalt/kernel/clock.h | 6 +++---
>>> include/cobalt/kernel/compat.h | 3 +++
>>> kernel/cobalt/posix/clock.c | 28 ++++++++++++++++++++++------
>>> kernel/cobalt/posix/clock.h | 12 +++++++++++-
>>> kernel/cobalt/posix/compat.c | 11 +++++++++++
>>> kernel/cobalt/posix/syscall32.c | 6 +++---
>>> kernel/cobalt/trace/cobalt-posix.h | 24 ++++++++++++++++++++++--
>>> 7 files changed, 75 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/include/cobalt/kernel/clock.h
>>> b/include/cobalt/kernel/clock.h
>>> index c26776a..cf9d5ec 100644
>>> --- a/include/cobalt/kernel/clock.h
>>> +++ b/include/cobalt/kernel/clock.h
>>> @@ -52,7 +52,7 @@ struct xnclock {
>>> xnticks_t (*read_raw)(struct xnclock *clock);
>>> xnticks_t (*read_monotonic)(struct xnclock *clock);
>>> int (*set_time)(struct xnclock *clock,
>>> - const struct timespec *ts);
>>> + const struct timespec64 *ts);
>>> xnsticks_t (*ns_to_ticks)(struct xnclock *clock,
>>> xnsticks_t ns);
>>> xnsticks_t (*ticks_to_ns)(struct xnclock *clock,
>>> @@ -213,7 +213,7 @@ static inline xnticks_t
>>> xnclock_read_monotonic(struct xnclock *clock)
>>> }
>>> static inline int xnclock_set_time(struct xnclock *clock,
>>> - const struct timespec *ts)
>>> + const struct timespec64 *ts)
>>> {
>>> if (likely(clock == &nkclock))
>>> return -EINVAL;
>>> @@ -266,7 +266,7 @@ static inline xnticks_t
>>> xnclock_read_monotonic(struct xnclock *clock)
>>> }
>>> static inline int xnclock_set_time(struct xnclock *clock,
>>> - const struct timespec *ts)
>>> + const struct timespec64 *ts)
>>> {
>>> /*
>>> * There is no way to change the core clock's idea of time.
>>> diff --git a/include/cobalt/kernel/compat.h
>>> b/include/cobalt/kernel/compat.h
>>> index 05754cb..70e2c03 100644
>>> --- a/include/cobalt/kernel/compat.h
>>> +++ b/include/cobalt/kernel/compat.h
>>> @@ -89,6 +89,9 @@ struct compat_rtdm_mmap_request {
>>> int sys32_get_timespec(struct timespec *ts,
>>> const struct compat_timespec __user *cts);
>>> +int sys64_get_timespec(struct timespec64 *ts,
>>> + const struct compat_timespec __user *cts);
>>> +
>>> int sys32_put_timespec(struct compat_timespec __user *cts,
>>> const struct timespec *ts);
>>> diff --git a/kernel/cobalt/posix/clock.c b/kernel/cobalt/posix/clock.c
>>> index 561358e..c960d7b 100644
>>> --- a/kernel/cobalt/posix/clock.c
>>> +++ b/kernel/cobalt/posix/clock.c
>>> @@ -194,7 +194,7 @@ COBALT_SYSCALL(clock_gettime, current,
>>> return 0;
>>> }
>>> -int __cobalt_clock_settime(clockid_t clock_id, const struct timespec
>>> *ts)
>>> +int __cobalt_clock_settime(clockid_t clock_id, const struct
>>> timespec64 *ts)
>>> {
>>> int _ret, ret = 0;
>>> xnticks_t now;
>>> @@ -207,7 +207,7 @@ int __cobalt_clock_settime(clockid_t clock_id,
>>> const struct timespec *ts)
>>> case CLOCK_REALTIME:
>>> xnlock_get_irqsave(&nklock, s);
>>> now = xnclock_read_realtime(&nkclock);
>>> - xnclock_adjust(&nkclock, (xnsticks_t) (ts2ns(ts) - now));
>>> + xnclock_adjust(&nkclock, (xnsticks_t) (ts2ns64(ts) - now));
>>> xnlock_put_irqrestore(&nklock, s);
>>> break;
>>> default:
>>> @@ -242,15 +242,31 @@ int __cobalt_clock_adjtime(clockid_t clock_id,
>>> struct timex *tx)
>>> return 0;
>>> }
>>> +static int __cobalt_get_timespec64(struct timespec64 *ts,
>>> + const struct __kernel_timespec __user *uts)
>>> +{
>>> + struct __kernel_timespec kts;
>>> + int ret;
>>> +
>>> + ret = cobalt_copy_from_user(&kts, uts, sizeof(kts));
>>> + if (ret)
>>> + return -EFAULT;
>>> +
>>> + ts->tv_sec = kts.tv_sec;
>>> + ts->tv_nsec = kts.tv_nsec;
>>> +
>>> + return 0;
>>> +
>>> +}
>>> COBALT_SYSCALL(clock_settime, current,
>>> - (clockid_t clock_id, const struct timespec __user *u_ts))
>>> + (clockid_t clock_id, const struct __kernel_timespec __user *u_ts))
>>> {
>>> - struct timespec ts;
>>> + struct timespec64 new_ts;
>>> - if (cobalt_copy_from_user(&ts, u_ts, sizeof(ts)))
>>> + if (__cobalt_get_timespec64(&new_ts, u_ts))
>>> return -EFAULT;
>>> - return __cobalt_clock_settime(clock_id, &ts);
>>> + return __cobalt_clock_settime(clock_id, &new_ts);
>>> }
>>> COBALT_SYSCALL(clock_adjtime, current,
>>> diff --git a/kernel/cobalt/posix/clock.h b/kernel/cobalt/posix/clock.h
>>> index 0b06b93..3b0f910 100644
>>> --- a/kernel/cobalt/posix/clock.h
>>> +++ b/kernel/cobalt/posix/clock.h
>>> @@ -43,6 +43,16 @@ static inline xnticks_t ts2ns(const struct
>>> timespec *ts)
>>> return nsecs;
>>> }
>>> +static inline xnticks_t ts2ns64(const struct timespec64 *ts)
>>> +{
>>> + xnticks_t nsecs = ts->tv_nsec;
>>> +
>>> + if (ts->tv_sec)
>>> + nsecs += (xnticks_t)ts->tv_sec * ONE_BILLION;
>>> +
>>> + return nsecs;
>>> +}
>>> +
>>> static inline xnticks_t tv2ns(const struct timeval *tv)
>>> {
>>> xnticks_t nsecs = tv->tv_usec * 1000;
>>> @@ -86,7 +96,7 @@ int __cobalt_clock_gettime(clockid_t clock_id,
>>> struct timespec *ts);
>>> int __cobalt_clock_settime(clockid_t clock_id,
>>> - const struct timespec *ts);
>>> + const struct timespec64 *ts);
>>> int __cobalt_clock_adjtime(clockid_t clock_id,
>>> struct timex *tx);
>>> diff --git a/kernel/cobalt/posix/compat.c b/kernel/cobalt/posix/compat.c
>>> index 17968bf..08aedcf 100644
>>> --- a/kernel/cobalt/posix/compat.c
>>> +++ b/kernel/cobalt/posix/compat.c
>>> @@ -32,6 +32,17 @@ int sys32_get_timespec(struct timespec *ts,
>>> }
>>> EXPORT_SYMBOL_GPL(sys32_get_timespec);
>>> +int sys64_get_timespec(struct timespec64 *ts,
>>> + const struct compat_timespec __user *cts)
>>> +{
>>> + return (cts == NULL ||
>>> + !access_rok(cts, sizeof(*cts)) ||
>>> + __xn_get_user(ts->tv_sec, &cts->tv_sec) ||
>>> + __xn_get_user(ts->tv_nsec, &cts->tv_nsec)) ? -EFAULT : 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(sys64_get_timespec);
>>> +
>>> +
>>> int sys32_put_timespec(struct compat_timespec __user *cts,
>>> const struct timespec *ts)
>>> {
>>> diff --git a/kernel/cobalt/posix/syscall32.c
>>> b/kernel/cobalt/posix/syscall32.c
>>> index faa7ef5..4b1dfcc 100644
>>> --- a/kernel/cobalt/posix/syscall32.c
>>> +++ b/kernel/cobalt/posix/syscall32.c
>>> @@ -161,14 +161,14 @@ COBALT_SYSCALL32emu(clock_settime, current,
>>> (clockid_t clock_id,
>>> const struct compat_timespec __user *u_ts))
>>> {
>>> - struct timespec ts;
>>> + struct timespec64 new_ts;
>>> int ret;
>>> - ret = sys32_get_timespec(&ts, u_ts);
>>> + ret = sys64_get_timespec(&new_ts, u_ts);
>>> if (ret)
>>> return ret;
>>> - return __cobalt_clock_settime(clock_id, &ts);
>>> + return __cobalt_clock_settime(clock_id, &new_ts);
>>> }
>>> COBALT_SYSCALL32emu(clock_adjtime, current,
>>> diff --git a/kernel/cobalt/trace/cobalt-posix.h
>>> b/kernel/cobalt/trace/cobalt-posix.h
>>> index aa78efb..f764872 100644
>>> --- a/kernel/cobalt/trace/cobalt-posix.h
>>> +++ b/kernel/cobalt/trace/cobalt-posix.h
>>> @@ -748,6 +748,26 @@ DECLARE_EVENT_CLASS(cobalt_clock_timespec,
>>> )
>>> );
>>> +DECLARE_EVENT_CLASS(cobalt_clock_timespec64,
>>> + TP_PROTO(clockid_t clk_id, const struct timespec64 *val),
>>> + TP_ARGS(clk_id, val),
>>> +
>>> + TP_STRUCT__entry(
>>> + __field(clockid_t, clk_id)
>>> + __timespec_fields(val)
>>> + ),
>>> +
>>> + TP_fast_assign(
>>> + __entry->clk_id = clk_id;
>>> + __assign_timespec(val, val);
>>> + ),
>>> +
>>> + TP_printk("clock_id=%d timeval=(%ld.%09ld)",
>>> + __entry->clk_id,
>>> + __timespec_args(val)
>>> + )
>>> +);
>>> +
>>> DEFINE_EVENT(cobalt_clock_timespec, cobalt_clock_getres,
>>> TP_PROTO(clockid_t clk_id, const struct timespec *res),
>>> TP_ARGS(clk_id, res)
>>> @@ -758,8 +778,8 @@ DEFINE_EVENT(cobalt_clock_timespec,
>>> cobalt_clock_gettime,
>>> TP_ARGS(clk_id, time)
>>> );
>>> -DEFINE_EVENT(cobalt_clock_timespec, cobalt_clock_settime,
>>> - TP_PROTO(clockid_t clk_id, const struct timespec *time),
>>> +DEFINE_EVENT(cobalt_clock_timespec64, cobalt_clock_settime,
>>> + TP_PROTO(clockid_t clk_id, const struct timespec64 *time),
>>> TP_ARGS(clk_id, time)
>>> );
>>>
>>
>> Thanks for this first step! Did you also test that it works with older
>> kernels? I see no wrappers, so I suspect breakages.
> I only have 64bit system, both 32bit process and 64bit process are
> working fine with clock_settime.
I meant: Please also try building your changes against kernel 4.4. If
that breaks because of some later introduced type, identify the kernel
version that did it and add that conditionally for older versions to
kernel/cobalt/include/asm-generic/xenomai/wrappers.h.
> 32bit process could still break unless it's recompiled as you said in
> last email. But in theory, it could be 2038safe after recompiled because
> process and system are both use 64bit sec. I will work on it when i
> start clock_gettime.
>
> I tried to use timespec64 explicitly in my userspace, but i got this
> "storage size of ts isn't know"
timespec64 is not userspace-exposed type, is it? It's kernel internal.
Jan
--
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] y2038: replace timespec with timespec64 in clock_settime
2020-09-28 16:52 ` Jan Kiszka
@ 2020-09-29 6:27 ` chensong
2020-09-30 19:19 ` Jan Kiszka
0 siblings, 1 reply; 10+ messages in thread
From: chensong @ 2020-09-29 6:27 UTC (permalink / raw)
To: Jan Kiszka, xenomai
On 2020年09月29日 00:52, Jan Kiszka wrote:
> On 23.09.20 03:40, song wrote:
>>
>>
>> On 2020/9/22 下午11:16, Jan Kiszka wrote:
>>> On 21.09.20 14:32, chensong wrote:
>>>> Upstream has used timespec64 to replace timespec inside kernel and
>>>> used __kernel_timespec to replace timespect in syscalls, therefore
>>>> we must keep aligned with upstream.
>>>>
>>>> clock_settime is a point to get started at, it involves 2 parts:
>>>> 1, syscall in ./include/trace/events/cobalt-posix.h for 64bits
>>>> 2, compat syscall in kernel/cobalt/posix/syscall32.c for 32bits
>>>>
>>>> some new functions are implemented to keep the compatibility with
>>>> other syscalls which haven't been handled yet and will be removed
>>>> in the end of the work.
>>>
>>> So, this switches also existing syscalls to 64-bit types, breaking the
>>> current ABI, no? How much effort would it be to keep the old ABI,
>>> adding a 64-bit one aside of it?
>>
>> Not breaking current ABI, i didn't add any new ABIs specific for 64bits,
>> i added some new static functions, like ts2ns and ts2ns64,
>> sys32_put_timespec and sys64_put_timespec, specific for clock_settime.
>>
>
> clock_settime accepts struct timespec so far, you are changing that to
> struct __kernel_timespec. If I look at its definition in the kernel, it
> uses __kernel_time64_t as tv_sec type. But our current 32-bit users
> still expects a 32-bit type here, no? Would that change already require
> a new clock_settime64 syscall if we wanted to stay binary compatible?
32 bit user space processes go to
COBALT_SYSCALL32emu(clock_settime,current,(clockid_t clock_id,const
struct compat_timespec __user *u_ts)) defined in
./kernel/cobalt/posix/syscall32.c. It uses compat_timespec which 32it is
compatible with.
I tested 32bit application and 64bit application on 64bit system
respeactively
32bit process -- clock_settime -- COBALT_SYSCALL32emu(clock_settime
64bit process -- clock_settime -- COBALT_SYSCALL(clock_settime)
>
>>
>> When all the syscalls has been finished, every one uses timespec64,
>> those new static functions can be removed.
>>
>> e.g
>> clock_settime is the ABI i'm working on, it calls ts2ns, but ts2ns only
>> accepts timespec, clock_gettime uses ts2ns as well, therefore, i
>> couldn't make them both work, i have to add a new ts2ns64(timespec64),
>> after i finish clock_gettime and nobody else uses ts2ns, i can remove
>> ts2ns64 and switch back to ts2ns(timespec64 as its parameter)
>>
>> Arnd Bergmann followed the similar process. He also added some ABIs
>> which 32bit is not compatible of, but not this one.
>>
>> Effort, as far as i can tell, not too much, around 10 fucntions as my
>> estimation.
>>
>>>
>>>>
>>>> Signed-off-by: chensong <chensong@tj.kylinos.cn>
>>>> ---
>>>> include/cobalt/kernel/clock.h | 6 +++---
>>>> include/cobalt/kernel/compat.h | 3 +++
>>>> kernel/cobalt/posix/clock.c | 28 ++++++++++++++++++++++------
>>>> kernel/cobalt/posix/clock.h | 12 +++++++++++-
>>>> kernel/cobalt/posix/compat.c | 11 +++++++++++
>>>> kernel/cobalt/posix/syscall32.c | 6 +++---
>>>> kernel/cobalt/trace/cobalt-posix.h | 24 ++++++++++++++++++++++--
>>>> 7 files changed, 75 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/include/cobalt/kernel/clock.h
>>>> b/include/cobalt/kernel/clock.h
>>>> index c26776a..cf9d5ec 100644
>>>> --- a/include/cobalt/kernel/clock.h
>>>> +++ b/include/cobalt/kernel/clock.h
>>>> @@ -52,7 +52,7 @@ struct xnclock {
>>>> xnticks_t (*read_raw)(struct xnclock *clock);
>>>> xnticks_t (*read_monotonic)(struct xnclock *clock);
>>>> int (*set_time)(struct xnclock *clock,
>>>> - const struct timespec *ts);
>>>> + const struct timespec64 *ts);
>>>> xnsticks_t (*ns_to_ticks)(struct xnclock *clock,
>>>> xnsticks_t ns);
>>>> xnsticks_t (*ticks_to_ns)(struct xnclock *clock,
>>>> @@ -213,7 +213,7 @@ static inline xnticks_t
>>>> xnclock_read_monotonic(struct xnclock *clock)
>>>> }
>>>> static inline int xnclock_set_time(struct xnclock *clock,
>>>> - const struct timespec *ts)
>>>> + const struct timespec64 *ts)
>>>> {
>>>> if (likely(clock == &nkclock))
>>>> return -EINVAL;
>>>> @@ -266,7 +266,7 @@ static inline xnticks_t
>>>> xnclock_read_monotonic(struct xnclock *clock)
>>>> }
>>>> static inline int xnclock_set_time(struct xnclock *clock,
>>>> - const struct timespec *ts)
>>>> + const struct timespec64 *ts)
>>>> {
>>>> /*
>>>> * There is no way to change the core clock's idea of time.
>>>> diff --git a/include/cobalt/kernel/compat.h
>>>> b/include/cobalt/kernel/compat.h
>>>> index 05754cb..70e2c03 100644
>>>> --- a/include/cobalt/kernel/compat.h
>>>> +++ b/include/cobalt/kernel/compat.h
>>>> @@ -89,6 +89,9 @@ struct compat_rtdm_mmap_request {
>>>> int sys32_get_timespec(struct timespec *ts,
>>>> const struct compat_timespec __user *cts);
>>>> +int sys64_get_timespec(struct timespec64 *ts,
>>>> + const struct compat_timespec __user *cts);
>>>> +
>>>> int sys32_put_timespec(struct compat_timespec __user *cts,
>>>> const struct timespec *ts);
>>>> diff --git a/kernel/cobalt/posix/clock.c b/kernel/cobalt/posix/clock.c
>>>> index 561358e..c960d7b 100644
>>>> --- a/kernel/cobalt/posix/clock.c
>>>> +++ b/kernel/cobalt/posix/clock.c
>>>> @@ -194,7 +194,7 @@ COBALT_SYSCALL(clock_gettime, current,
>>>> return 0;
>>>> }
>>>> -int __cobalt_clock_settime(clockid_t clock_id, const struct timespec
>>>> *ts)
>>>> +int __cobalt_clock_settime(clockid_t clock_id, const struct
>>>> timespec64 *ts)
>>>> {
>>>> int _ret, ret = 0;
>>>> xnticks_t now;
>>>> @@ -207,7 +207,7 @@ int __cobalt_clock_settime(clockid_t clock_id,
>>>> const struct timespec *ts)
>>>> case CLOCK_REALTIME:
>>>> xnlock_get_irqsave(&nklock, s);
>>>> now = xnclock_read_realtime(&nkclock);
>>>> - xnclock_adjust(&nkclock, (xnsticks_t) (ts2ns(ts) - now));
>>>> + xnclock_adjust(&nkclock, (xnsticks_t) (ts2ns64(ts) - now));
>>>> xnlock_put_irqrestore(&nklock, s);
>>>> break;
>>>> default:
>>>> @@ -242,15 +242,31 @@ int __cobalt_clock_adjtime(clockid_t clock_id,
>>>> struct timex *tx)
>>>> return 0;
>>>> }
>>>> +static int __cobalt_get_timespec64(struct timespec64 *ts,
>>>> + const struct __kernel_timespec __user *uts)
>>>> +{
>>>> + struct __kernel_timespec kts;
>>>> + int ret;
>>>> +
>>>> + ret = cobalt_copy_from_user(&kts, uts, sizeof(kts));
>>>> + if (ret)
>>>> + return -EFAULT;
>>>> +
>>>> + ts->tv_sec = kts.tv_sec;
>>>> + ts->tv_nsec = kts.tv_nsec;
>>>> +
>>>> + return 0;
>>>> +
>>>> +}
>>>> COBALT_SYSCALL(clock_settime, current,
>>>> - (clockid_t clock_id, const struct timespec __user *u_ts))
>>>> + (clockid_t clock_id, const struct __kernel_timespec __user *u_ts))
>>>> {
>>>> - struct timespec ts;
>>>> + struct timespec64 new_ts;
>>>> - if (cobalt_copy_from_user(&ts, u_ts, sizeof(ts)))
>>>> + if (__cobalt_get_timespec64(&new_ts, u_ts))
>>>> return -EFAULT;
>>>> - return __cobalt_clock_settime(clock_id, &ts);
>>>> + return __cobalt_clock_settime(clock_id, &new_ts);
>>>> }
>>>> COBALT_SYSCALL(clock_adjtime, current,
>>>> diff --git a/kernel/cobalt/posix/clock.h b/kernel/cobalt/posix/clock.h
>>>> index 0b06b93..3b0f910 100644
>>>> --- a/kernel/cobalt/posix/clock.h
>>>> +++ b/kernel/cobalt/posix/clock.h
>>>> @@ -43,6 +43,16 @@ static inline xnticks_t ts2ns(const struct
>>>> timespec *ts)
>>>> return nsecs;
>>>> }
>>>> +static inline xnticks_t ts2ns64(const struct timespec64 *ts)
>>>> +{
>>>> + xnticks_t nsecs = ts->tv_nsec;
>>>> +
>>>> + if (ts->tv_sec)
>>>> + nsecs += (xnticks_t)ts->tv_sec * ONE_BILLION;
>>>> +
>>>> + return nsecs;
>>>> +}
>>>> +
>>>> static inline xnticks_t tv2ns(const struct timeval *tv)
>>>> {
>>>> xnticks_t nsecs = tv->tv_usec * 1000;
>>>> @@ -86,7 +96,7 @@ int __cobalt_clock_gettime(clockid_t clock_id,
>>>> struct timespec *ts);
>>>> int __cobalt_clock_settime(clockid_t clock_id,
>>>> - const struct timespec *ts);
>>>> + const struct timespec64 *ts);
>>>> int __cobalt_clock_adjtime(clockid_t clock_id,
>>>> struct timex *tx);
>>>> diff --git a/kernel/cobalt/posix/compat.c b/kernel/cobalt/posix/compat.c
>>>> index 17968bf..08aedcf 100644
>>>> --- a/kernel/cobalt/posix/compat.c
>>>> +++ b/kernel/cobalt/posix/compat.c
>>>> @@ -32,6 +32,17 @@ int sys32_get_timespec(struct timespec *ts,
>>>> }
>>>> EXPORT_SYMBOL_GPL(sys32_get_timespec);
>>>> +int sys64_get_timespec(struct timespec64 *ts,
>>>> + const struct compat_timespec __user *cts)
>>>> +{
>>>> + return (cts == NULL ||
>>>> + !access_rok(cts, sizeof(*cts)) ||
>>>> + __xn_get_user(ts->tv_sec, &cts->tv_sec) ||
>>>> + __xn_get_user(ts->tv_nsec, &cts->tv_nsec)) ? -EFAULT : 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(sys64_get_timespec);
>>>> +
>>>> +
>>>> int sys32_put_timespec(struct compat_timespec __user *cts,
>>>> const struct timespec *ts)
>>>> {
>>>> diff --git a/kernel/cobalt/posix/syscall32.c
>>>> b/kernel/cobalt/posix/syscall32.c
>>>> index faa7ef5..4b1dfcc 100644
>>>> --- a/kernel/cobalt/posix/syscall32.c
>>>> +++ b/kernel/cobalt/posix/syscall32.c
>>>> @@ -161,14 +161,14 @@ COBALT_SYSCALL32emu(clock_settime, current,
>>>> (clockid_t clock_id,
>>>> const struct compat_timespec __user *u_ts))
>>>> {
>>>> - struct timespec ts;
>>>> + struct timespec64 new_ts;
>>>> int ret;
>>>> - ret = sys32_get_timespec(&ts, u_ts);
>>>> + ret = sys64_get_timespec(&new_ts, u_ts);
>>>> if (ret)
>>>> return ret;
>>>> - return __cobalt_clock_settime(clock_id, &ts);
>>>> + return __cobalt_clock_settime(clock_id, &new_ts);
>>>> }
>>>> COBALT_SYSCALL32emu(clock_adjtime, current,
>>>> diff --git a/kernel/cobalt/trace/cobalt-posix.h
>>>> b/kernel/cobalt/trace/cobalt-posix.h
>>>> index aa78efb..f764872 100644
>>>> --- a/kernel/cobalt/trace/cobalt-posix.h
>>>> +++ b/kernel/cobalt/trace/cobalt-posix.h
>>>> @@ -748,6 +748,26 @@ DECLARE_EVENT_CLASS(cobalt_clock_timespec,
>>>> )
>>>> );
>>>> +DECLARE_EVENT_CLASS(cobalt_clock_timespec64,
>>>> + TP_PROTO(clockid_t clk_id, const struct timespec64 *val),
>>>> + TP_ARGS(clk_id, val),
>>>> +
>>>> + TP_STRUCT__entry(
>>>> + __field(clockid_t, clk_id)
>>>> + __timespec_fields(val)
>>>> + ),
>>>> +
>>>> + TP_fast_assign(
>>>> + __entry->clk_id = clk_id;
>>>> + __assign_timespec(val, val);
>>>> + ),
>>>> +
>>>> + TP_printk("clock_id=%d timeval=(%ld.%09ld)",
>>>> + __entry->clk_id,
>>>> + __timespec_args(val)
>>>> + )
>>>> +);
>>>> +
>>>> DEFINE_EVENT(cobalt_clock_timespec, cobalt_clock_getres,
>>>> TP_PROTO(clockid_t clk_id, const struct timespec *res),
>>>> TP_ARGS(clk_id, res)
>>>> @@ -758,8 +778,8 @@ DEFINE_EVENT(cobalt_clock_timespec,
>>>> cobalt_clock_gettime,
>>>> TP_ARGS(clk_id, time)
>>>> );
>>>> -DEFINE_EVENT(cobalt_clock_timespec, cobalt_clock_settime,
>>>> - TP_PROTO(clockid_t clk_id, const struct timespec *time),
>>>> +DEFINE_EVENT(cobalt_clock_timespec64, cobalt_clock_settime,
>>>> + TP_PROTO(clockid_t clk_id, const struct timespec64 *time),
>>>> TP_ARGS(clk_id, time)
>>>> );
>>>>
>>>
>>> Thanks for this first step! Did you also test that it works with older
>>> kernels? I see no wrappers, so I suspect breakages.
>> I only have 64bit system, both 32bit process and 64bit process are
>> working fine with clock_settime.
>
> I meant: Please also try building your changes against kernel 4.4. If
> that breaks because of some later introduced type, identify the kernel
> version that did it and add that conditionally for older versions to
> kernel/cobalt/include/asm-generic/xenomai/wrappers.h.
>
good point, i tested and found __kernel_timespect was not introduced
until 4.18, as a result, 4.4 was failed to build.
i will submit a new patch with the definition in wrappers.h.
>> 32bit process could still break unless it's recompiled as you said in
>> last email. But in theory, it could be 2038safe after recompiled because
>> process and system are both use 64bit sec. I will work on it when i
>> start clock_gettime.
>>
>> I tried to use timespec64 explicitly in my userspace, but i got this
>> "storage size of ts isn't know"
>
> timespec64 is not userspace-exposed type, is it? It's kernel internal.
>
> Jan
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] y2038: replace timespec with timespec64 in clock_settime
2020-09-29 6:27 ` chensong
@ 2020-09-30 19:19 ` Jan Kiszka
2020-10-04 3:09 ` song
2020-10-12 9:45 ` chensong
0 siblings, 2 replies; 10+ messages in thread
From: Jan Kiszka @ 2020-09-30 19:19 UTC (permalink / raw)
To: chensong, xenomai
On 29.09.20 08:27, chensong wrote:
>
>
> On 2020年09月29日 00:52, Jan Kiszka wrote:
>> On 23.09.20 03:40, song wrote:
>>>
>>>
>>> On 2020/9/22 下午11:16, Jan Kiszka wrote:
>>>> On 21.09.20 14:32, chensong wrote:
>>>>> Upstream has used timespec64 to replace timespec inside kernel and
>>>>> used __kernel_timespec to replace timespect in syscalls, therefore
>>>>> we must keep aligned with upstream.
>>>>>
>>>>> clock_settime is a point to get started at, it involves 2 parts:
>>>>> 1, syscall in ./include/trace/events/cobalt-posix.h for 64bits
>>>>> 2, compat syscall in kernel/cobalt/posix/syscall32.c for 32bits
>>>>>
>>>>> some new functions are implemented to keep the compatibility with
>>>>> other syscalls which haven't been handled yet and will be removed
>>>>> in the end of the work.
>>>>
>>>> So, this switches also existing syscalls to 64-bit types, breaking the
>>>> current ABI, no? How much effort would it be to keep the old ABI,
>>>> adding a 64-bit one aside of it?
>>>
>>> Not breaking current ABI, i didn't add any new ABIs specific for 64bits,
>>> i added some new static functions, like ts2ns and ts2ns64,
>>> sys32_put_timespec and sys64_put_timespec, specific for clock_settime.
>>>
>>
>> clock_settime accepts struct timespec so far, you are changing that to
>> struct __kernel_timespec. If I look at its definition in the kernel, it
>> uses __kernel_time64_t as tv_sec type. But our current 32-bit users
>> still expects a 32-bit type here, no? Would that change already require
>> a new clock_settime64 syscall if we wanted to stay binary compatible?
>
> 32 bit user space processes go to
> COBALT_SYSCALL32emu(clock_settime,current,(clockid_t clock_id,const
> struct compat_timespec __user *u_ts)) defined in
> ./kernel/cobalt/posix/syscall32.c. It uses compat_timespec which 32it is
> compatible with.
>
> I tested 32bit application and 64bit application on 64bit system
> respeactively
> 32bit process -- clock_settime -- COBALT_SYSCALL32emu(clock_settime
> 64bit process -- clock_settime -- COBALT_SYSCALL(clock_settime)
>
You also need to consider 32-bit kernels, not on x86 anymore (i386 is
practically dead), but e.g. ARMv7. There we would have a breakage now,
don't we?
Jan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] y2038: replace timespec with timespec64 in clock_settime
2020-09-30 19:19 ` Jan Kiszka
@ 2020-10-04 3:09 ` song
2020-10-12 9:45 ` chensong
1 sibling, 0 replies; 10+ messages in thread
From: song @ 2020-10-04 3:09 UTC (permalink / raw)
To: Jan Kiszka, xenomai
On 2020/10/1 上午3:19, Jan Kiszka wrote:
> On 29.09.20 08:27, chensong wrote:
>>
>>
>> On 2020年09月29日 00:52, Jan Kiszka wrote:
>>> On 23.09.20 03:40, song wrote:
>>>>
>>>>
>>>> On 2020/9/22 下午11:16, Jan Kiszka wrote:
>>>>> On 21.09.20 14:32, chensong wrote:
>>>>>> Upstream has used timespec64 to replace timespec inside kernel and
>>>>>> used __kernel_timespec to replace timespect in syscalls, therefore
>>>>>> we must keep aligned with upstream.
>>>>>>
>>>>>> clock_settime is a point to get started at, it involves 2 parts:
>>>>>> 1, syscall in ./include/trace/events/cobalt-posix.h for 64bits
>>>>>> 2, compat syscall in kernel/cobalt/posix/syscall32.c for 32bits
>>>>>>
>>>>>> some new functions are implemented to keep the compatibility with
>>>>>> other syscalls which haven't been handled yet and will be removed
>>>>>> in the end of the work.
>>>>>
>>>>> So, this switches also existing syscalls to 64-bit types, breaking the
>>>>> current ABI, no? How much effort would it be to keep the old ABI,
>>>>> adding a 64-bit one aside of it?
>>>>
>>>> Not breaking current ABI, i didn't add any new ABIs specific for 64bits,
>>>> i added some new static functions, like ts2ns and ts2ns64,
>>>> sys32_put_timespec and sys64_put_timespec, specific for clock_settime.
>>>>
>>>
>>> clock_settime accepts struct timespec so far, you are changing that to
>>> struct __kernel_timespec. If I look at its definition in the kernel, it
>>> uses __kernel_time64_t as tv_sec type. But our current 32-bit users
>>> still expects a 32-bit type here, no? Would that change already require
>>> a new clock_settime64 syscall if we wanted to stay binary compatible?
>>
>> 32 bit user space processes go to
>> COBALT_SYSCALL32emu(clock_settime,current,(clockid_t clock_id,const
>> struct compat_timespec __user *u_ts)) defined in
>> ./kernel/cobalt/posix/syscall32.c. It uses compat_timespec which 32it is
>> compatible with.
>>
>> I tested 32bit application and 64bit application on 64bit system
>> respeactively
>> 32bit process -- clock_settime -- COBALT_SYSCALL32emu(clock_settime
>> 64bit process -- clock_settime -- COBALT_SYSCALL(clock_settime)
>>
>
> You also need to consider 32-bit kernels, not on x86 anymore (i386 is
> practically dead), but e.g. ARMv7. There we would have a breakage now,
> don't we?
if we still take 32-bit kernel into account, i will try to build one and
give it a run to see how it goes, and get it back to you.
>
> Jan
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] y2038: replace timespec with timespec64 in clock_settime
2020-09-30 19:19 ` Jan Kiszka
2020-10-04 3:09 ` song
@ 2020-10-12 9:45 ` chensong
2020-10-20 8:10 ` chensong
1 sibling, 1 reply; 10+ messages in thread
From: chensong @ 2020-10-12 9:45 UTC (permalink / raw)
To: Jan Kiszka, xenomai
On 2020年10月01日 03:19, Jan Kiszka wrote:
> On 29.09.20 08:27, chensong wrote:
>>
>>
>> On 2020年09月29日 00:52, Jan Kiszka wrote:
>>> On 23.09.20 03:40, song wrote:
>>>>
>>>>
>>>> On 2020/9/22 下午11:16, Jan Kiszka wrote:
>>>>> On 21.09.20 14:32, chensong wrote:
>>>>>> Upstream has used timespec64 to replace timespec inside kernel and
>>>>>> used __kernel_timespec to replace timespect in syscalls, therefore
>>>>>> we must keep aligned with upstream.
>>>>>>
>>>>>> clock_settime is a point to get started at, it involves 2 parts:
>>>>>> 1, syscall in ./include/trace/events/cobalt-posix.h for 64bits
>>>>>> 2, compat syscall in kernel/cobalt/posix/syscall32.c for 32bits
>>>>>>
>>>>>> some new functions are implemented to keep the compatibility with
>>>>>> other syscalls which haven't been handled yet and will be removed
>>>>>> in the end of the work.
>>>>>
>>>>> So, this switches also existing syscalls to 64-bit types, breaking the
>>>>> current ABI, no? How much effort would it be to keep the old ABI,
>>>>> adding a 64-bit one aside of it?
>>>>
>>>> Not breaking current ABI, i didn't add any new ABIs specific for 64bits,
>>>> i added some new static functions, like ts2ns and ts2ns64,
>>>> sys32_put_timespec and sys64_put_timespec, specific for clock_settime.
>>>>
>>>
>>> clock_settime accepts struct timespec so far, you are changing that to
>>> struct __kernel_timespec. If I look at its definition in the kernel, it
>>> uses __kernel_time64_t as tv_sec type. But our current 32-bit users
>>> still expects a 32-bit type here, no? Would that change already require
>>> a new clock_settime64 syscall if we wanted to stay binary compatible?
>>
>> 32 bit user space processes go to
>> COBALT_SYSCALL32emu(clock_settime,current,(clockid_t clock_id,const
>> struct compat_timespec __user *u_ts)) defined in
>> ./kernel/cobalt/posix/syscall32.c. It uses compat_timespec which 32it is
>> compatible with.
>>
>> I tested 32bit application and 64bit application on 64bit system
>> respeactively
>> 32bit process -- clock_settime -- COBALT_SYSCALL32emu(clock_settime
>> 64bit process -- clock_settime -- COBALT_SYSCALL(clock_settime)
>>
>
> You also need to consider 32-bit kernels, not on x86 anymore (i386 is
> practically dead), but e.g. ARMv7. There we would have a breakage now,
> don't we?
>
i tested 32bit process on a 32bit kernel (raspberry 2, armv7), it seems
timespec64 is working fine.
If this patch is acceptable for you, i will start clock_gettime.
meanwhile, i will email Arnd Bergmann <arnd@arndb.de> to see how to
enable 64bit timespec in userspace to see if 32bit process is 2038-safe
in both 32bit kernel and 64bit kernel.
> Jan
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] y2038: replace timespec with timespec64 in clock_settime
2020-10-12 9:45 ` chensong
@ 2020-10-20 8:10 ` chensong
2020-10-21 6:43 ` Jan Kiszka
0 siblings, 1 reply; 10+ messages in thread
From: chensong @ 2020-10-20 8:10 UTC (permalink / raw)
To: Jan Kiszka, xenomai
Hi Jan;
i talked to arnd Bergmann about how to build a 32bit 2038-safe process,
he told me that neither gcc nor glibc can fully support 64bit timespec
yet, he suggested me to use musl(https://wiki.musl-libc.org/), and he
also suggested me to use kernel5.4.
I followed his suggestion, built a 32bit process with musl-gcc and ran
it in a armv7l device, and it's working fine, safe.
Then i turned to xenomai, it took a lot time to build libcobalt with
musl's toolchain(https://musl.cc/), it worked anyway, but i found it
called linux's clock_settime(kernel/time/posix-timers.c) instead of
cobalt's clock_settime, i'm stuck here.
what's more, to enable 64bit __kernel_timespec should enable
CONFIG_64BIT_TIME, it's off by default, i turned it on but system can't
bootup.
Ideally, if we could verify a 32bit process 2038-safe in both 32bit
system and 64bit system, we could continue to work on other syscalls,
but now, what can we do? shall we create a new branch specific for y2038
which we can apply out patches on and mainline them after gcc/glibs is
ready for verification?
Let me know your opinion about this case, many thanks.
BR
chensong
On 2020年10月12日 17:45, chensong wrote:
>
>
> On 2020年10月01日 03:19, Jan Kiszka wrote:
>> On 29.09.20 08:27, chensong wrote:
>>>
>>>
>>> On 2020年09月29日 00:52, Jan Kiszka wrote:
>>>> On 23.09.20 03:40, song wrote:
>>>>>
>>>>>
>>>>> On 2020/9/22 下午11:16, Jan Kiszka wrote:
>>>>>> On 21.09.20 14:32, chensong wrote:
>>>>>>> Upstream has used timespec64 to replace timespec inside kernel and
>>>>>>> used __kernel_timespec to replace timespect in syscalls, therefore
>>>>>>> we must keep aligned with upstream.
>>>>>>>
>>>>>>> clock_settime is a point to get started at, it involves 2 parts:
>>>>>>> 1, syscall in ./include/trace/events/cobalt-posix.h for 64bits
>>>>>>> 2, compat syscall in kernel/cobalt/posix/syscall32.c for 32bits
>>>>>>>
>>>>>>> some new functions are implemented to keep the compatibility with
>>>>>>> other syscalls which haven't been handled yet and will be removed
>>>>>>> in the end of the work.
>>>>>>
>>>>>> So, this switches also existing syscalls to 64-bit types, breaking
>>>>>> the
>>>>>> current ABI, no? How much effort would it be to keep the old ABI,
>>>>>> adding a 64-bit one aside of it?
>>>>>
>>>>> Not breaking current ABI, i didn't add any new ABIs specific for
>>>>> 64bits,
>>>>> i added some new static functions, like ts2ns and ts2ns64,
>>>>> sys32_put_timespec and sys64_put_timespec, specific for clock_settime.
>>>>>
>>>>
>>>> clock_settime accepts struct timespec so far, you are changing that to
>>>> struct __kernel_timespec. If I look at its definition in the kernel, it
>>>> uses __kernel_time64_t as tv_sec type. But our current 32-bit users
>>>> still expects a 32-bit type here, no? Would that change already require
>>>> a new clock_settime64 syscall if we wanted to stay binary compatible?
>>>
>>> 32 bit user space processes go to
>>> COBALT_SYSCALL32emu(clock_settime,current,(clockid_t clock_id,const
>>> struct compat_timespec __user *u_ts)) defined in
>>> ./kernel/cobalt/posix/syscall32.c. It uses compat_timespec which 32it is
>>> compatible with.
>>>
>>> I tested 32bit application and 64bit application on 64bit system
>>> respeactively
>>> 32bit process -- clock_settime -- COBALT_SYSCALL32emu(clock_settime
>>> 64bit process -- clock_settime -- COBALT_SYSCALL(clock_settime)
>>>
>>
>> You also need to consider 32-bit kernels, not on x86 anymore (i386 is
>> practically dead), but e.g. ARMv7. There we would have a breakage now,
>> don't we?
>>
> i tested 32bit process on a 32bit kernel (raspberry 2, armv7), it seems
> timespec64 is working fine.
>
> If this patch is acceptable for you, i will start clock_gettime.
> meanwhile, i will email Arnd Bergmann <arnd@arndb.de> to see how to
> enable 64bit timespec in userspace to see if 32bit process is 2038-safe
> in both 32bit kernel and 64bit kernel.
>
>> Jan
>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] y2038: replace timespec with timespec64 in clock_settime
2020-10-20 8:10 ` chensong
@ 2020-10-21 6:43 ` Jan Kiszka
0 siblings, 0 replies; 10+ messages in thread
From: Jan Kiszka @ 2020-10-21 6:43 UTC (permalink / raw)
To: chensong, xenomai
On 20.10.20 10:10, chensong wrote:
> Hi Jan;
>
> i talked to arnd Bergmann about how to build a 32bit 2038-safe process,
> he told me that neither gcc nor glibc can fully support 64bit timespec
> yet, he suggested me to use musl(https://wiki.musl-libc.org/), and he
> also suggested me to use kernel5.4.
>
> I followed his suggestion, built a 32bit process with musl-gcc and ran
> it in a armv7l device, and it's working fine, safe.
>
> Then i turned to xenomai, it took a lot time to build libcobalt with
> musl's toolchain(https://musl.cc/), it worked anyway, but i found it
> called linux's clock_settime(kernel/time/posix-timers.c) instead of
> cobalt's clock_settime, i'm stuck here.
>
> what's more, to enable 64bit __kernel_timespec should enable
> CONFIG_64BIT_TIME, it's off by default, i turned it on but system can't
> bootup.
>
> Ideally, if we could verify a 32bit process 2038-safe in both 32bit
> system and 64bit system, we could continue to work on other syscalls,
> but now, what can we do? shall we create a new branch specific for y2038
> which we can apply out patches on and mainline them after gcc/glibs is
> ready for verification?
>
> Let me know your opinion about this case, many thanks.
>
I'll try to have a look soon - currently deep into FPU debugging for 5.4
(interrupts by other stuff, which doesn't make that easier).
Jan
> BR
>
> chensong
>
> On 2020年10月12日 17:45, chensong wrote:
>>
>>
>> On 2020年10月01日 03:19, Jan Kiszka wrote:
>>> On 29.09.20 08:27, chensong wrote:
>>>>
>>>>
>>>> On 2020年09月29日 00:52, Jan Kiszka wrote:
>>>>> On 23.09.20 03:40, song wrote:
>>>>>>
>>>>>>
>>>>>> On 2020/9/22 下午11:16, Jan Kiszka wrote:
>>>>>>> On 21.09.20 14:32, chensong wrote:
>>>>>>>> Upstream has used timespec64 to replace timespec inside kernel and
>>>>>>>> used __kernel_timespec to replace timespect in syscalls, therefore
>>>>>>>> we must keep aligned with upstream.
>>>>>>>>
>>>>>>>> clock_settime is a point to get started at, it involves 2 parts:
>>>>>>>> 1, syscall in ./include/trace/events/cobalt-posix.h for 64bits
>>>>>>>> 2, compat syscall in kernel/cobalt/posix/syscall32.c for 32bits
>>>>>>>>
>>>>>>>> some new functions are implemented to keep the compatibility with
>>>>>>>> other syscalls which haven't been handled yet and will be removed
>>>>>>>> in the end of the work.
>>>>>>>
>>>>>>> So, this switches also existing syscalls to 64-bit types, breaking
>>>>>>> the
>>>>>>> current ABI, no? How much effort would it be to keep the old ABI,
>>>>>>> adding a 64-bit one aside of it?
>>>>>>
>>>>>> Not breaking current ABI, i didn't add any new ABIs specific for
>>>>>> 64bits,
>>>>>> i added some new static functions, like ts2ns and ts2ns64,
>>>>>> sys32_put_timespec and sys64_put_timespec, specific for
>>>>>> clock_settime.
>>>>>>
>>>>>
>>>>> clock_settime accepts struct timespec so far, you are changing that to
>>>>> struct __kernel_timespec. If I look at its definition in the
>>>>> kernel, it
>>>>> uses __kernel_time64_t as tv_sec type. But our current 32-bit users
>>>>> still expects a 32-bit type here, no? Would that change already
>>>>> require
>>>>> a new clock_settime64 syscall if we wanted to stay binary compatible?
>>>>
>>>> 32 bit user space processes go to
>>>> COBALT_SYSCALL32emu(clock_settime,current,(clockid_t clock_id,const
>>>> struct compat_timespec __user *u_ts)) defined in
>>>> ./kernel/cobalt/posix/syscall32.c. It uses compat_timespec which
>>>> 32it is
>>>> compatible with.
>>>>
>>>> I tested 32bit application and 64bit application on 64bit system
>>>> respeactively
>>>> 32bit process -- clock_settime -- COBALT_SYSCALL32emu(clock_settime
>>>> 64bit process -- clock_settime -- COBALT_SYSCALL(clock_settime)
>>>>
>>>
>>> You also need to consider 32-bit kernels, not on x86 anymore (i386 is
>>> practically dead), but e.g. ARMv7. There we would have a breakage now,
>>> don't we?
>>>
>> i tested 32bit process on a 32bit kernel (raspberry 2, armv7), it seems
>> timespec64 is working fine.
>>
>> If this patch is acceptable for you, i will start clock_gettime.
>> meanwhile, i will email Arnd Bergmann <arnd@arndb.de> to see how to
>> enable 64bit timespec in userspace to see if 32bit process is 2038-safe
>> in both 32bit kernel and 64bit kernel.
>>
>>> Jan
>>>
>
>
--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-10-21 6:43 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-21 12:32 [PATCH] y2038: replace timespec with timespec64 in clock_settime chensong
2020-09-22 15:16 ` Jan Kiszka
2020-09-23 1:40 ` song
2020-09-28 16:52 ` Jan Kiszka
2020-09-29 6:27 ` chensong
2020-09-30 19:19 ` Jan Kiszka
2020-10-04 3:09 ` song
2020-10-12 9:45 ` chensong
2020-10-20 8:10 ` chensong
2020-10-21 6:43 ` Jan Kiszka
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.