Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1645493 > unrolled thread

[PATCH v3] Added "Preserve Boot Time Support"

Back to article view | Back to linux.kernel

Contents

  [PATCH v3] Added "Preserve Boot Time Support" Bogdan Mirea <Bogdan-Stefan_mirea@mentor.com> - 2017-05-19 12:20 +0200
    Re: [PATCH v3] Added "Preserve Boot Time Support" Thomas Gleixner <tglx@linutronix.de> - 2017-05-21 23:40 +0200
      RE: [PATCH v3] Added "Preserve Boot Time Support" "Mirea, Bogdan-Stefan" <Bogdan-Stefan_Mirea@mentor.com> - 2017-05-23 18:00 +0200
        RE: [PATCH v3] Added "Preserve Boot Time Support" Thomas Gleixner <tglx@linutronix.de> - 2017-05-23 18:40 +0200
          RE: [PATCH v3] Added "Preserve Boot Time Support" "Mirea, Bogdan-Stefan" <Bogdan-Stefan_Mirea@mentor.com> - 2017-05-24 11:40 +0200

#1645493 — [PATCH v3] Added "Preserve Boot Time Support"

This option enables Boot Time Preservation between Bootloader and
Linux Kernel. It is based on the idea that the Bootloader (or any
other early firmware) will start the HW Timer and Linux Kernel will
count the time starting with the cycles elapsed since timer start.

The sched_clock part is preserving boottime for kmsg which should be in
sync with system uptime. The system uptime part is driver specific and I
updated the arm_arch_timer with an arch_timer_setsystime() function
which will call do_settimeofday64() with the values read from arch timer
counter.

This way both kmsg and uptime will be in sync, otherwise inconsistencies
will appear between the two.

The "preserve_boot_time" parameter should be appended to kernel cmdline
from bootloader for kernel acknowledgment that the timer is running in
bootloader.

Signed-off-by: Bogdan Mirea <Bogdan-Stefan_mirea@mentor.com>
---
 drivers/clocksource/arm_arch_timer.c | 33 +++++++++++++++++++++++++++++++++
 kernel/time/Kconfig                  | 12 ++++++++++++
 kernel/time/sched_clock.c            | 14 ++++++++++++++
 3 files changed, 59 insertions(+)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 5152b38..95699cd 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -475,6 +475,35 @@ struct timecounter *arch_timer_get_timecounter(void)
 	return &timecounter;
 }
 
+#ifdef CONFIG_BOOT_TIME_PRESERVE
+/*
+ * Set the real system time(including the time spent in bootloader)
+ * based on the timer counter.
+ */
+
+#ifndef BOOT_TIME_PRESERVE_CMDLINE
+	#define BOOT_TIME_PRESERVE_CMDLINE "preserve_boot_time"
+#endif
+void arch_timer_setsystime(void)
+{
+	static struct timespec64 boot_ts;
+	static cycles_t cycles;
+	unsigned long long nsecs;
+
+	if (!strstr(boot_command_line, BOOT_TIME_PRESERVE_CMDLINE))
+		return;
+
+	cycles = arch_timer_read_counter() ? arch_timer_read_counter() : 0;
+
+	nsecs = clocksource_cyc2ns(cycles, clocksource_counter.mult,
+				   clocksource_counter.shift);
+	timespec64_add_ns(&boot_ts, nsecs);
+
+	if (do_settimeofday64(&boot_ts))
+		pr_warn("arch_timer: unable to set systime\n");
+}
+#endif /* CONFIG_BOOT_TIME_PRESERVE */
+
 static void __init arch_counter_register(unsigned type)
 {
 	u64 start_count;
@@ -504,6 +533,10 @@ static void __init arch_counter_register(unsigned type)
 
 	/* 56 bits minimum, so we assume worst case rollover */
 	sched_clock_register(arch_timer_read_counter, 56, arch_timer_rate);
+#ifdef CONFIG_BOOT_TIME_PRESERVE
+	/* Set systime */
+	arch_timer_setsystime();
+#endif /* CONFIG_BOOT_TIME_PRESERVE */
 }
 
 static void arch_timer_stop(struct clock_event_device *clk)
diff --git a/kernel/time/Kconfig b/kernel/time/Kconfig
index 4008d9f..1edd518 100644
--- a/kernel/time/Kconfig
+++ b/kernel/time/Kconfig
@@ -193,5 +193,17 @@ config HIGH_RES_TIMERS
 	  hardware is not capable then this option only increases
 	  the size of the kernel image.
 
+config BOOT_TIME_PRESERVE
+	bool "Preserve Boot Time Support"
+	default n
+	help
+	  This option enables Boot Time Preservation between Bootloader and
+	  Linux Kernel. It is based on the idea that the Bootloader (or any
+	  other early firmware) will start the HW Timer and Linux Kernel will
+	  count the time starting with the cycles elapsed since timer start.
+
+	  The "preserve_boot_time" parameter should be appended to kernel cmdline
+	  from bootloader for kernel acknowledgment that the timer is running in
+	  bootloader.
 endmenu
 endif
diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
index a26036d..efd66bf 100644
--- a/kernel/time/sched_clock.c
+++ b/kernel/time/sched_clock.c
@@ -193,6 +193,20 @@ sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
 	/* Update epoch for new counter and update 'epoch_ns' from old counter*/
 	new_epoch = read();
 	cyc = cd.actual_read_sched_clock();
+
+#ifdef CONFIG_BOOT_TIME_PRESERVE
+
+#ifndef BOOT_TIME_PRESERVE_CMDLINE
+	#define BOOT_TIME_PRESERVE_CMDLINE "preserve_boot_time"
+#endif
+	if (strstr(boot_command_line, BOOT_TIME_PRESERVE_CMDLINE)) {
+		cyc = new_epoch;
+		rd.sched_clock_mask = new_mask;
+		rd.mult			= new_mult;
+		rd.shift		= new_shift;
+	}
+#endif /* CONFIG_BOOT_TIME_PRESERVE */
+
 	ns = rd.epoch_ns + cyc_to_ns((cyc - rd.epoch_cyc) & rd.sched_clock_mask, rd.mult, rd.shift);
 	cd.actual_read_sched_clock = read;
 
-- 
1.9.1

[toc] | [next] | [standalone]

#1646452

On Fri, 19 May 2017, Bogdan Mirea wrote:
> +#ifdef CONFIG_BOOT_TIME_PRESERVE
> +/*
> + * Set the real system time(including the time spent in bootloader)
> + * based on the timer counter.
> + */
> +
> +#ifndef BOOT_TIME_PRESERVE_CMDLINE
> +	#define BOOT_TIME_PRESERVE_CMDLINE "preserve_boot_time"
> +#endif
> +void arch_timer_setsystime(void)
> +{
> +	static struct timespec64 boot_ts;
> +	static cycles_t cycles;
> +	unsigned long long nsecs;
> +
> +	if (!strstr(boot_command_line, BOOT_TIME_PRESERVE_CMDLINE))
> +		return;

This adds a arch_timer specific command line option. Why is this arch_timer
specific? So if any other platform wants to gain this feature then we end
up copying that mess to every single timer implementation? Certainly NOT!

> +
> +	cycles = arch_timer_read_counter() ? arch_timer_read_counter() : 0;
> +
> +	nsecs = clocksource_cyc2ns(cycles, clocksource_counter.mult,
> +				   clocksource_counter.shift);
> +	timespec64_add_ns(&boot_ts, nsecs);
> +
> +	if (do_settimeofday64(&boot_ts))
> +		pr_warn("arch_timer: unable to set systime\n");

What the heck is this? What has boot_ts to do with do_settimeofday()?

Exactly nothing. settimeofday() modifies CLOCK_REALTIME and if the platform
has an early accessible RTC, you hereby wreckaged wall_time. If the RTC
readout comes later then CLOCK_REALTIME is overwritten. So what is this
supposed to do?

It has absolutely nothing to do with CLOCK_BOOTTIME. /proc/uptime is based
on CLOCK_BOOTTIME, which is the CLOCK_MONOTONIC time since system boot. The
difference between CLOCK_MONOTONIC and CLOCK_BOOTTIME is that
CLOCK_MONOTONIC does not advance during suspend, but CLOCK_BOOTTIME takes
the suspended time into account.

I have not the faintest idea how you can claim that this patch actually
does what it is supposed to do. It's simply crap and CANNOT work at all.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1648227

On Monday, May 22, 2017 12:36 AM, Thomas Gleixner wrote:
> On Fri, 19 May 2017, Bogdan Mirea wrote:
> This adds a arch_timer specific command line option. Why is this
> arch_timer
> specific? So if any other platform wants to gain this feature then we
> end
> up copying that mess to every single timer implementation? Certainly
> NOT!
> Exactly nothing. settimeofday() modifies CLOCK_REALTIME and if the
> platform
> has an early accessible RTC, you hereby wreckaged wall_time. If the
> RTC
> readout comes later then CLOCK_REALTIME is overwritten. So what is
> this
> supposed to do?
>
> It has absolutely nothing to do with CLOCK_BOOTTIME. /proc/uptime is
> based
> on CLOCK_BOOTTIME, which is the CLOCK_MONOTONIC time since system
> boot. The
> difference between CLOCK_MONOTONIC and CLOCK_BOOTTIME is that
> CLOCK_MONOTONIC does not advance during suspend, but CLOCK_BOOTTIME
> takes
> the suspended time into account.
Thanks for feedback.
The idea of this patch was of a POC and this is why the code was
isolated in timer driver, which I agree is not a good idea for other
platforms to copy this since we can simply do all the things in
sched_clock_register() function guarded with CONFIG_BOOT_TIME_PRESERVE.
The patch was created for an internal project where no RTC was available
and no user-space apps were making any settimeofday(), and the use of
do_settimeofday() seemed safe. But yes, considering that the
CLOCK_REALTIME can be easily changed it should not be used here.



A way of setting the CLOCK_BOOTTIME is through the
timekeeping_inject_sleeptime64(delta) hook, but the problem that arise
here is that this hook is intended to be used on rtc_resume() code.
Adding delta to CLOCK_BOOTTIME this way, the CONFIG_BOOT_TIME_PRESERVE
will depend on PM_SLEEP && RTC_HCTOSYS.


The changes will be the following:
diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
index a26036d..6c8ad44 100644
--- a/kernel/time/sched_clock.c
+++ b/kernel/time/sched_clock.c
@@ -193,6 +193,26 @@ sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
 	/* Update epoch for new counter and update 'epoch_ns' from old counter*/
 	new_epoch = read();
 	cyc = cd.actual_read_sched_clock();
+
+#ifdef CONFIG_BOOT_TIME_PRESERVE
+
+#ifndef BOOT_TIME_PRESERVE_CMDLINE
+	#define BOOT_TIME_PRESERVE_CMDLINE "preserve_boot_time"
+#endif
+	if (strstr(boot_command_line, BOOT_TIME_PRESERVE_CMDLINE)) {
+		static struct timespec64 delta_ts;
+		cyc = new_epoch;
+		rd.sched_clock_mask = new_mask;
+		rd.mult = new_mult;
+		rd.shift = new_shift;
+
+		timespec64_add_ns(&delta_ts, clocksource_cyc2ns(cyc, rd.mult, rd.shift));
+		timekeeping_inject_sleeptime64(&delta_ts);
+			
+	}
+#endif /* CONFIG_BOOT_TIME_PRESERVE */
+
 	ns = rd.epoch_ns + cyc_to_ns((cyc - rd.epoch_cyc) & rd.sched_clock_mask, rd.mult, rd.shift);
 	cd.actual_read_sched_clock = read;

diff --git a/kernel/time/Kconfig b/kernel/time/Kconfig
index 4008d9f..2e392aa 100644
--- a/kernel/time/Kconfig
+++ b/kernel/time/Kconfig
@@ -193,5 +193,18 @@ config HIGH_RES_TIMERS
 	  hardware is not capable then this option only increases
 	  the size of the kernel image.
 
+config BOOT_TIME_PRESERVE
+	bool "Preserve Boot Time Support"
+	default n
+	depends on PM_SLEEP && RTC_HCTOSYS
+	help
+	  This option enables Boot Time Preservation between Bootloader and
+	  Linux Kernel. It is based on the idea that the Bootloader (or any
+	  other early firmware) will start the HW Timer and Linux Kernel will
+	  count the time starting with the cycles elapsed since timer start.
+
+	  The "preserve_boot_time" parameter should be appended to kernel cmdline
+	  from bootloader for kernel acknowledgment that the timer is running in
+	  bootloader.
 endmenu
 endif

Waiting for feedback.

Thanks,
Bogdan

[toc] | [prev] | [next] | [standalone]

#1648255

On Tue, 23 May 2017, Mirea, Bogdan-Stefan wrote:
> On Monday, May 22, 2017 12:36 AM, Thomas Gleixner wrote:
> > On Fri, 19 May 2017, Bogdan Mirea wrote:
> > This adds a arch_timer specific command line option. Why is this
> > arch_timer
> > specific? So if any other platform wants to gain this feature then we
> > end
> > up copying that mess to every single timer implementation? Certainly
> > NOT!
> > Exactly nothing. settimeofday() modifies CLOCK_REALTIME and if the
> > platform
> > has an early accessible RTC, you hereby wreckaged wall_time. If the
> > RTC
> > readout comes later then CLOCK_REALTIME is overwritten. So what is
> > this
> > supposed to do?
> >
> > It has absolutely nothing to do with CLOCK_BOOTTIME. /proc/uptime is
> > based
> > on CLOCK_BOOTTIME, which is the CLOCK_MONOTONIC time since system
> > boot. The
> > difference between CLOCK_MONOTONIC and CLOCK_BOOTTIME is that
> > CLOCK_MONOTONIC does not advance during suspend, but CLOCK_BOOTTIME
> > takes
> > the suspended time into account.
> Thanks for feedback.
> The idea of this patch was of a POC and this is why the code was
> isolated in timer driver, which I agree is not a good idea for other
> platforms to copy this since we can simply do all the things in
> sched_clock_register() function guarded with CONFIG_BOOT_TIME_PRESERVE.
> The patch was created for an internal project where no RTC was available
> and no user-space apps were making any settimeofday(), and the use of
> do_settimeofday() seemed safe. But yes, considering that the
> CLOCK_REALTIME can be easily changed it should not be used here.

It does not matter at all whether you have a RTC or settimeofday() is used
or not.

Again:

> > It has absolutely nothing to do with CLOCK_BOOTTIME. /proc/uptime is
> > based on CLOCK_BOOTTIME, which is the CLOCK_MONOTONIC time since system
> > boot. The difference between CLOCK_MONOTONIC and CLOCK_BOOTTIME is that
> > CLOCK_MONOTONIC does not advance during suspend, but CLOCK_BOOTTIME
> > takes the suspended time into account.

So using settimeofday() for any of what you want to do is bogus and
useless.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1649378

On Tuesday, May 23, 2017 7:39 PM, Thomas Gleixner wrote:
> On Tue, 23 May 2017, Mirea, Bogdan-Stefan wrote:
> > On Monday, May 22, 2017 12:36 AM, Thomas Gleixner wrote:
> > > On Fri, 19 May 2017, Bogdan Mirea wrote:
> > > This adds a arch_timer specific command line option. Why is this
> > > arch_timer
> > > specific? So if any other platform wants to gain this feature then we
> > > end
> > > up copying that mess to every single timer implementation? Certainly
> > > NOT!
> > > Exactly nothing. settimeofday() modifies CLOCK_REALTIME and if the
> > > platform
> > > has an early accessible RTC, you hereby wreckaged wall_time. If the
> > > RTC
> > > readout comes later then CLOCK_REALTIME is overwritten. So what is
> > > this
> > > supposed to do?
> > >
> > > It has absolutely nothing to do with CLOCK_BOOTTIME. /proc/uptime is
> > > based
> > > on CLOCK_BOOTTIME, which is the CLOCK_MONOTONIC time since system
> > > boot. The
> > > difference between CLOCK_MONOTONIC and CLOCK_BOOTTIME is that
> > > CLOCK_MONOTONIC does not advance during suspend, but
> CLOCK_BOOTTIME
> > > takes
> > > the suspended time into account.
> > Thanks for feedback.
> > The idea of this patch was of a POC and this is why the code was
> > isolated in timer driver, which I agree is not a good idea for other
> > platforms to copy this since we can simply do all the things in
> > sched_clock_register() function guarded with
> CONFIG_BOOT_TIME_PRESERVE.
> > The patch was created for an internal project where no RTC was available
> > and no user-space apps were making any settimeofday(), and the use of
> > do_settimeofday() seemed safe. But yes, considering that the
> > CLOCK_REALTIME can be easily changed it should not be used here.
> 
> It does not matter at all whether you have a RTC or settimeofday() is used
> or not.
> 
> Again:
> 
> > > It has absolutely nothing to do with CLOCK_BOOTTIME. /proc/uptime is
> > > based on CLOCK_BOOTTIME, which is the CLOCK_MONOTONIC time since
> system
> > > boot. The difference between CLOCK_MONOTONIC and CLOCK_BOOTTIME
> is that
> > > CLOCK_MONOTONIC does not advance during suspend, but
> CLOCK_BOOTTIME
> > > takes the suspended time into account.
> 
> So using settimeofday() for any of what you want to do is bogus and
> useless.

Got it. Thanks!

I am thinking about using timekeeping_inject_sleeptime64(delta) hook to
add a delta time at boot to the CLOCK_BOOTTIME, but the problem that
arise here is that this hook is intended to be used on rtc_resume()
code (to add the time spent in suspend to CLOCK_BOOTTIME).
From my point of view this will do the trick even if the
CONFIG_BOOT_TIME_PRESERVE will then depend on PM_SLEEP && RTC_HCTOSYS
(needed by timekeeping_inject_sleeptime64 to work).
Do you think this is a good approach?

Regards,
Bogdan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web