[PATCH v2] Added "Preserve Boot Time Support"

[PATCH v2] Added "Preserve Boot Time Support"

[Bogdan Mirea]

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 incosistencies
will appear between the two.

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

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 5152b38..7f9bf2a 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -475,6 +475,28 @@ 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.
+ */
+void arch_timer_setsystime(void)
+{
+	static struct timespec64 boot_ts;
+	static cycles_t cycles;
+	unsigned long long nsecs;
+
+	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 +526,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..7d70232 100644
--- a/kernel/time/Kconfig
+++ b/kernel/time/Kconfig
@@ -193,5 +193,13 @@ 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"
+	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.
+
 endmenu
 endif
diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
index a26036d..1d6e35a 100644
--- a/kernel/time/sched_clock.c
+++ b/kernel/time/sched_clock.c
@@ -193,7 +193,13 @@ 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
+	ns = rd.epoch_ns + cyc_to_ns((new_epoch - rd.epoch_cyc) & new_mask, new_mult, new_shift);
+#else
 	ns = rd.epoch_ns + cyc_to_ns((cyc - rd.epoch_cyc) & rd.sched_clock_mask, rd.mult, rd.shift);
+#endif /* CONFIG_BOOT_TIME_PRESERVE */
+
 	cd.actual_read_sched_clock = read;
 
 	rd.read_sched_clock	= read;
-- 
1.9.1

Re: [PATCH v2] Added "Preserve Boot Time Support"

[Oleksij Rempel]

Hi Bogdan,

are there any example what and how bootloader should do to provide 
correct values?

On 05/03/2017 12:59 PM, Bogdan Mirea wrote:
> 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 incosistencies
> will appear between the two.
>
> Signed-off-by: Bogdan Mirea <Bogdan-Stefan_mirea@mentor.com>
> ---
>  drivers/clocksource/arm_arch_timer.c | 26 ++++++++++++++++++++++++++
>  kernel/time/Kconfig                  |  8 ++++++++
>  kernel/time/sched_clock.c            |  6 ++++++
>  3 files changed, 40 insertions(+)
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 5152b38..7f9bf2a 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -475,6 +475,28 @@ 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.
> + */
> +void arch_timer_setsystime(void)
> +{
> +	static struct timespec64 boot_ts;
> +	static cycles_t cycles;
> +	unsigned long long nsecs;
> +
> +	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 +526,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..7d70232 100644
> --- a/kernel/time/Kconfig
> +++ b/kernel/time/Kconfig
> @@ -193,5 +193,13 @@ 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"
> +	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.
> +
>  endmenu
>  endif
> diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
> index a26036d..1d6e35a 100644
> --- a/kernel/time/sched_clock.c
> +++ b/kernel/time/sched_clock.c
> @@ -193,7 +193,13 @@ 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
> +	ns = rd.epoch_ns + cyc_to_ns((new_epoch - rd.epoch_cyc) & new_mask, new_mult, new_shift);
> +#else
>  	ns = rd.epoch_ns + cyc_to_ns((cyc - rd.epoch_cyc) & rd.sched_clock_mask, rd.mult, rd.shift);
> +#endif /* CONFIG_BOOT_TIME_PRESERVE */
> +
>  	cd.actual_read_sched_clock = read;
>
>  	rd.read_sched_clock	= read;
>

RE: [PATCH v2] Added "Preserve Boot Time Support"

[Mirea, Bogdan-Stefan]

Hi Oleksij,

On Thursday, May 04, 2017 12:27 PM, Oleksij Rempel wrote:
> Hi Bogdan,
>
> are there any example what and how bootloader should do to provide
> correct values?

I will give you an example with a real behavior on Renesas RCAR Gen3
Salvator-x:

We have an ARM64 SOC with the following boot stages:
ARM Trusted Firmware BL2 -> ARM Trusted Firmware BL31 -> U-Boot -> Linux

[First]
On ARM Trusted Firmware BL31 "programs the CNTFRQ_EL0 register with the
clock frequency of the system counter, which is provided by the
platform"(Aarch64 Bl31 documentation [1]). And after this step the timer
is up and running so every timer cycle is counted in the CCNT register.

[Step A]
After this step BL31 will load and start execution of U-Boot(BL33). In
U-Boot we will spend for example 4 seconds and then load and start
execution of Linux Kernel.

[Step B]
Linux Kernel starts and after kernel timer is initiallized the
sched_clock_register will be called.

[Step C]
Testing with "$: uptime > /dev/kmsg"



Test 1: Testing with default kernel (no patch added)
Log will be:
[Step A]
[    0.165573]
[    0.167127] U-Boot 2015.04 (Apr 06 2017 - 12:28:41)
...
[    4.364556]
[    4.366065] Starting kernel ...

[Step B]
...
[    0.000000] clocksource: arch_sys_counter: mask: 0xffffffffffffff
max_cycles: 0x1ec02923e, max_idle_ns: 440795202125 ns
[    0.000003] sched_clock: 56 bits at 8MHz, resolution 120ns, wraps
every 2199023255496ns
[    0.000214] Console: colour dummy device 80x25
[    0.000594] console [tty0] enabled
...
[Step C]
$ uptime  > /dev/kmsg
[   9.148458]  00:00:09 up 0 min,  load average: 0.00, 0.00, 0.00

There is no inconsistency between kmsg and uptime, but from [Step B] we
observe that the time spent before kernel start is not added.



Test 2: Testing only with preserve boot time "kernel/time/sched_clock" 
modifications:
Log will be:
[Step A]
[    0.164567]
[    0.166122] U-Boot 2015.04 (Apr 06 2017 - 12:28:41)
...
[    4.357793]
[    4.359301] Starting kernel ...

[Step B]
...
[    0.000000] clocksource: arch_sys_counter: mask: 0xffffffffffffff
max_cycles: 0x1ec02923e, max_idle_ns: 440795202125 ns
[    4.641512] sched_clock: 56 bits at 8MHz, resolution 120ns, wraps
every 2199023255496ns
[    4.641724] Console: colour dummy device 80x25
[    4.642105] console [tty0] enabled
...

[Step C]
uptime  > /dev/kmsg
[   13.933217]  00:00:09 up 0 min,  load average: 0.00, 0.00, 0.00

We can see that the preserve boottime changes in
"kernel/time/sched_clock" updates the kmsg time [Step B], but there is
an inconsistency between kmsg time and uptime since uptime is not
updated accordingly to the timer's CCNT value [Step C]. The uptime
starts from 0, and dt~=4sec inconsistency between kmsg and uptime is
observable.



Test 3: Testing with the full preserve boot time support with
"kernel/time/sched_clock" and "drivers/clocksource/arm_arch_timer":
Log will be:
[Step A]
[    0.164564]
[    0.166119] U-Boot 2015.04 (Apr 06 2017 - 12:28:41)
...
[    4.357751]
[    4.359259] Starting kernel ...
[Step B]
...
[    0.000000] clocksource: arch_sys_counter: mask: 0xffffffffffffff
max_cycles: 0x1ec02923e, max_idle_ns: 440795202125 ns
[    4.638667] sched_clock: 56 bits at 8MHz, resolution 120ns, wraps
every 2199023255496ns
[    4.638884] Console: colour dummy device 80x25
[    4.639265] console [tty0] enabled
...

[Step C]
$ uptime > /dev/kmsg
[   17.728591]  00:00:17 up 0 min,  load average: 0.00, 0.00, 0.00

We can observe that the patch updates kmsg time with the time spent
before kernel starts [Step B]("kernel/time/sched_clock") and also
updates kernel uptime("drivers/clocksource/arm_arch_timer") in [Step C]
no inconsistency being present between kmsg and uptime.


Best Regards,
Bogdan
[1]
https://github.com/ARM-software/arm-trusted-firmware/blob/master/docs/firmware-design.md

RE: [PATCH v2] Added "Preserve Boot Time Support"

[Mirea, Bogdan-Stefan]

Hello,

Any input on this topic?

Kind Regards,
Bogdan

On Thursday, May 04, 2017 1:55 PM Bogdan Mirea wrote:
> 
> Hi Oleksij,
> 
> On Thursday, May 04, 2017 12:27 PM, Oleksij Rempel wrote:
> > Hi Bogdan,
> >
> > are there any example what and how bootloader should do to provide
> > correct values?
> 
> I will give you an example with a real behavior on Renesas RCAR Gen3
> Salvator-x:
> 
> We have an ARM64 SOC with the following boot stages:
> ARM Trusted Firmware BL2 -> ARM Trusted Firmware BL31 -> U-Boot -> Linux
> 
> [First]
> On ARM Trusted Firmware BL31 "programs the CNTFRQ_EL0 register with the
> clock frequency of the system counter, which is provided by the
> platform"(Aarch64 Bl31 documentation [1]). And after this step the timer
> is up and running so every timer cycle is counted in the CCNT register.
> 
> [Step A]
> After this step BL31 will load and start execution of U-Boot(BL33). In
> U-Boot we will spend for example 4 seconds and then load and start
> execution of Linux Kernel.
> 
> [Step B]
> Linux Kernel starts and after kernel timer is initiallized the
> sched_clock_register will be called.
> 
> [Step C]
> Testing with "$: uptime > /dev/kmsg"
> 
> 
> 
> Test 1: Testing with default kernel (no patch added)
> Log will be:
> [Step A]
> [    0.165573]
> [    0.167127] U-Boot 2015.04 (Apr 06 2017 - 12:28:41)
> ...
> [    4.364556]
> [    4.366065] Starting kernel ...
> 
> [Step B]
> ...
> [    0.000000] clocksource: arch_sys_counter: mask: 0xffffffffffffff
> max_cycles: 0x1ec02923e, max_idle_ns: 440795202125 ns
> [    0.000003] sched_clock: 56 bits at 8MHz, resolution 120ns, wraps
> every 2199023255496ns
> [    0.000214] Console: colour dummy device 80x25
> [    0.000594] console [tty0] enabled
> ...
> [Step C]
> $ uptime  > /dev/kmsg
> [   9.148458]  00:00:09 up 0 min,  load average: 0.00, 0.00, 0.00
> 
> There is no inconsistency between kmsg and uptime, but from [Step B] we
> observe that the time spent before kernel start is not added.
> 
> 
> 
> Test 2: Testing only with preserve boot time "kernel/time/sched_clock"
> modifications:
> Log will be:
> [Step A]
> [    0.164567]
> [    0.166122] U-Boot 2015.04 (Apr 06 2017 - 12:28:41)
> ...
> [    4.357793]
> [    4.359301] Starting kernel ...
> 
> [Step B]
> ...
> [    0.000000] clocksource: arch_sys_counter: mask: 0xffffffffffffff
> max_cycles: 0x1ec02923e, max_idle_ns: 440795202125 ns
> [    4.641512] sched_clock: 56 bits at 8MHz, resolution 120ns, wraps
> every 2199023255496ns
> [    4.641724] Console: colour dummy device 80x25
> [    4.642105] console [tty0] enabled
> ...
> 
> [Step C]
> uptime  > /dev/kmsg
> [   13.933217]  00:00:09 up 0 min,  load average: 0.00, 0.00, 0.00
> 
> We can see that the preserve boottime changes in
> "kernel/time/sched_clock" updates the kmsg time [Step B], but there is
> an inconsistency between kmsg time and uptime since uptime is not
> updated accordingly to the timer's CCNT value [Step C]. The uptime
> starts from 0, and dt~=4sec inconsistency between kmsg and uptime is
> observable.
> 
> 
> 
> Test 3: Testing with the full preserve boot time support with
> "kernel/time/sched_clock" and "drivers/clocksource/arm_arch_timer":
> Log will be:
> [Step A]
> [    0.164564]
> [    0.166119] U-Boot 2015.04 (Apr 06 2017 - 12:28:41)
> ...
> [    4.357751]
> [    4.359259] Starting kernel ...
> [Step B]
> ...
> [    0.000000] clocksource: arch_sys_counter: mask: 0xffffffffffffff
> max_cycles: 0x1ec02923e, max_idle_ns: 440795202125 ns
> [    4.638667] sched_clock: 56 bits at 8MHz, resolution 120ns, wraps
> every 2199023255496ns
> [    4.638884] Console: colour dummy device 80x25
> [    4.639265] console [tty0] enabled
> ...
> 
> [Step C]
> $ uptime > /dev/kmsg
> [   17.728591]  00:00:17 up 0 min,  load average: 0.00, 0.00, 0.00
> 
> We can observe that the patch updates kmsg time with the time spent
> before kernel starts [Step B]("kernel/time/sched_clock") and also
> updates kernel uptime("drivers/clocksource/arm_arch_timer") in [Step C]
> no inconsistency being present between kmsg and uptime.
> 
> 
> Best Regards,
> Bogdan
> [1]
> https://github.com/ARM-software/arm-trusted-
> firmware/blob/master/docs/firmware-design.md

Re: [PATCH v2] Added "Preserve Boot Time Support"

[Sascha Hauer]

On Tue, May 16, 2017 at 08:22:29AM +0000, Mirea, Bogdan-Stefan wrote:
> Hello,
> 
> Any input on this topic?

As John already said, there's the read_boot_clock64() interface which
should be used here.

In your patch you provide a generic option (BOOT_TIME_PRESERVE) which
only works as expected in some special cases. You assume that the
bootloader has started the same timer with the same frequency as Linux
does. Also you assume that the timer startup code doesn't reset the
timer. When these assumptions are not true then you just get bogus
random timing information.

By using the read_boot_clock64() interface you can make sure that you
only provide the functionality when it's actually supposed to work.

Sascha

> 
> Kind Regards,
> Bogdan
> 
> On Thursday, May 04, 2017 1:55 PM Bogdan Mirea wrote:
> > 
> > Hi Oleksij,
> > 
> > On Thursday, May 04, 2017 12:27 PM, Oleksij Rempel wrote:
> > > Hi Bogdan,
> > >
> > > are there any example what and how bootloader should do to provide
> > > correct values?
> > 
> > I will give you an example with a real behavior on Renesas RCAR Gen3
> > Salvator-x:
> > 
> > We have an ARM64 SOC with the following boot stages:
> > ARM Trusted Firmware BL2 -> ARM Trusted Firmware BL31 -> U-Boot -> Linux
> > 
> > [First]
> > On ARM Trusted Firmware BL31 "programs the CNTFRQ_EL0 register with the
> > clock frequency of the system counter, which is provided by the
> > platform"(Aarch64 Bl31 documentation [1]). And after this step the timer
> > is up and running so every timer cycle is counted in the CCNT register.
> > 
> > [Step A]
> > After this step BL31 will load and start execution of U-Boot(BL33). In
> > U-Boot we will spend for example 4 seconds and then load and start
> > execution of Linux Kernel.
> > 
> > [Step B]
> > Linux Kernel starts and after kernel timer is initiallized the
> > sched_clock_register will be called.
> > 
> > [Step C]
> > Testing with "$: uptime > /dev/kmsg"
> > 
> > 
> > 
> > Test 1: Testing with default kernel (no patch added)
> > Log will be:
> > [Step A]
> > [    0.165573]
> > [    0.167127] U-Boot 2015.04 (Apr 06 2017 - 12:28:41)
> > ...
> > [    4.364556]
> > [    4.366065] Starting kernel ...
> > 
> > [Step B]
> > ...
> > [    0.000000] clocksource: arch_sys_counter: mask: 0xffffffffffffff
> > max_cycles: 0x1ec02923e, max_idle_ns: 440795202125 ns
> > [    0.000003] sched_clock: 56 bits at 8MHz, resolution 120ns, wraps
> > every 2199023255496ns
> > [    0.000214] Console: colour dummy device 80x25
> > [    0.000594] console [tty0] enabled
> > ...
> > [Step C]
> > $ uptime  > /dev/kmsg
> > [   9.148458]  00:00:09 up 0 min,  load average: 0.00, 0.00, 0.00
> > 
> > There is no inconsistency between kmsg and uptime, but from [Step B] we
> > observe that the time spent before kernel start is not added.
> > 
> > 
> > 
> > Test 2: Testing only with preserve boot time "kernel/time/sched_clock"
> > modifications:
> > Log will be:
> > [Step A]
> > [    0.164567]
> > [    0.166122] U-Boot 2015.04 (Apr 06 2017 - 12:28:41)
> > ...
> > [    4.357793]
> > [    4.359301] Starting kernel ...
> > 
> > [Step B]
> > ...
> > [    0.000000] clocksource: arch_sys_counter: mask: 0xffffffffffffff
> > max_cycles: 0x1ec02923e, max_idle_ns: 440795202125 ns
> > [    4.641512] sched_clock: 56 bits at 8MHz, resolution 120ns, wraps
> > every 2199023255496ns
> > [    4.641724] Console: colour dummy device 80x25
> > [    4.642105] console [tty0] enabled
> > ...
> > 
> > [Step C]
> > uptime  > /dev/kmsg
> > [   13.933217]  00:00:09 up 0 min,  load average: 0.00, 0.00, 0.00
> > 
> > We can see that the preserve boottime changes in
> > "kernel/time/sched_clock" updates the kmsg time [Step B], but there is
> > an inconsistency between kmsg time and uptime since uptime is not
> > updated accordingly to the timer's CCNT value [Step C]. The uptime
> > starts from 0, and dt~=4sec inconsistency between kmsg and uptime is
> > observable.
> > 
> > 
> > 
> > Test 3: Testing with the full preserve boot time support with
> > "kernel/time/sched_clock" and "drivers/clocksource/arm_arch_timer":
> > Log will be:
> > [Step A]
> > [    0.164564]
> > [    0.166119] U-Boot 2015.04 (Apr 06 2017 - 12:28:41)
> > ...
> > [    4.357751]
> > [    4.359259] Starting kernel ...
> > [Step B]
> > ...
> > [    0.000000] clocksource: arch_sys_counter: mask: 0xffffffffffffff
> > max_cycles: 0x1ec02923e, max_idle_ns: 440795202125 ns
> > [    4.638667] sched_clock: 56 bits at 8MHz, resolution 120ns, wraps
> > every 2199023255496ns
> > [    4.638884] Console: colour dummy device 80x25
> > [    4.639265] console [tty0] enabled
> > ...
> > 
> > [Step C]
> > $ uptime > /dev/kmsg
> > [   17.728591]  00:00:17 up 0 min,  load average: 0.00, 0.00, 0.00
> > 
> > We can observe that the patch updates kmsg time with the time spent
> > before kernel starts [Step B]("kernel/time/sched_clock") and also
> > updates kernel uptime("drivers/clocksource/arm_arch_timer") in [Step C]
> > no inconsistency being present between kmsg and uptime.
> > 
> > 
> > Best Regards,
> > Bogdan
> > [1]
> > https://github.com/ARM-software/arm-trusted-
> > firmware/blob/master/docs/firmware-design.md
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [PATCH v2] Added "Preserve Boot Time Support"

[Mirea, Bogdan-Stefan]

Hello Sascha,

On Wednesday, May 17, 2017 2:30 PM Sascha Hauer wrote:
> As John already said, there's the read_boot_clock64() interface which
> should be used here.
> By using the read_boot_clock64() interface you can make sure that you
> only provide the functionality when it's actually supposed to work.
The read_boot_clock64() function is called from timekeeping_init().
The arm arch timer init callback arch_timer_of_init() function is called
from time_init() function.
We will have an uninitialized arm arch timer (this is our only timer
source) at the read_boot_clock64() function call since both
timekeeping_init() and time_init() functions are called from
start_kernel() in the following order:
    asmlinkage __visible void __init start_kernel(void)
    {
        /* ... */
        timekeeping_init();
        time_init();
        /* ... */
    }
So in our case, we cannot use a read_boot_clock64() function to read cyc
value.


> In your patch you provide a generic option (BOOT_TIME_PRESERVE) which
> only works as expected in some special cases. You assume that the
> bootloader has started the same timer with the same frequency as Linux
> does.
Yes, I assume that the bootloader and Linux use and configure the same
timer at the same frequency. This is a must since we want to measure the
exact boottime. The reason the code is guarded with BOOT_TIME_PRESERVE
is because we want to avoid situations like the one you described.


> Also you assume that the timer startup code doesn't reset the
> timer. When these assumptions are not true then you just get bogus
> random timing information.
If the timer initialization code will reset the timer, the Linux system
will work exactly as before, showing boottime 0 in kmsg and 0 in
uptime, so no bugs/crashes will occur.

Kind Regards,
Bogdan

Re: [PATCH v2] Added "Preserve Boot Time Support"

[Sascha Hauer]

On Wed, May 17, 2017 at 02:42:24PM +0000, Mirea, Bogdan-Stefan wrote:
> Hello Sascha,
> 
> On Wednesday, May 17, 2017 2:30 PM Sascha Hauer wrote:
> > As John already said, there's the read_boot_clock64() interface which
> > should be used here.
> > By using the read_boot_clock64() interface you can make sure that you
> > only provide the functionality when it's actually supposed to work.
> The read_boot_clock64() function is called from timekeeping_init().
> The arm arch timer init callback arch_timer_of_init() function is called
> from time_init() function.
> We will have an uninitialized arm arch timer (this is our only timer
> source) at the read_boot_clock64() function call since both
> timekeeping_init() and time_init() functions are called from
> start_kernel() in the following order:
>     asmlinkage __visible void __init start_kernel(void)
>     {
>         /* ... */
>         timekeeping_init();
>         time_init();
>         /* ... */
>     }
> So in our case, we cannot use a read_boot_clock64() function to read cyc
> value.
> 
> 
> > In your patch you provide a generic option (BOOT_TIME_PRESERVE) which
> > only works as expected in some special cases. You assume that the
> > bootloader has started the same timer with the same frequency as Linux
> > does.
> Yes, I assume that the bootloader and Linux use and configure the same
> timer at the same frequency. This is a must since we want to measure the
> exact boottime. The reason the code is guarded with BOOT_TIME_PRESERVE
> is because we want to avoid situations like the one you described.

IMO Kernel options should be safe to enable everytime. They shouldn't
have side effects on boards that lack the necessary hardware or
bootloader support. I think we should at least have a commandline option
or device tree property in which the bootloader can signal that it has
initialized the timer suitable for calculating the boot time.

> 
> 
> > Also you assume that the timer startup code doesn't reset the
> > timer. When these assumptions are not true then you just get bogus
> > random timing information.
> If the timer initialization code will reset the timer, the Linux system
> will work exactly as before, showing boottime 0 in kmsg and 0 in
> uptime, so no bugs/crashes will occur.

It may also be that the timer initialization code switches to another
frequency, but doesn't reset the timer. All kinds of funny things can
happen when the timer initialization code hasn't been audited to support
this usecase. So really we shouldn't provide the user an option without
giving any hint if this can even work on his hardware

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [PATCH v2] Added "Preserve Boot Time Support"

[Mirea, Bogdan-Stefan]

On Wednesday, May 17, 2017 8:52 PM Sascha Hauer wrote:
> On Wed, May 17, 2017 at 02:42:24PM +0000, Mirea, Bogdan-Stefan wrote:
> > Hello Sascha,
> >
> > On Wednesday, May 17, 2017 2:30 PM Sascha Hauer wrote:
> > > As John already said, there's the read_boot_clock64() interface which
> > > should be used here.
> > > By using the read_boot_clock64() interface you can make sure that you
> > > only provide the functionality when it's actually supposed to work.
> > The read_boot_clock64() function is called from timekeeping_init().
> > The arm arch timer init callback arch_timer_of_init() function is called
> > from time_init() function.
> > We will have an uninitialized arm arch timer (this is our only timer
> > source) at the read_boot_clock64() function call since both
> > timekeeping_init() and time_init() functions are called from
> > start_kernel() in the following order:
> >     asmlinkage __visible void __init start_kernel(void)
> >     {
> >         /* ... */
> >         timekeeping_init();
> >         time_init();
> >         /* ... */
> >     }
> > So in our case, we cannot use a read_boot_clock64() function to read cyc
> > value.
> >
> >
> > > In your patch you provide a generic option (BOOT_TIME_PRESERVE) which
> > > only works as expected in some special cases. You assume that the
> > > bootloader has started the same timer with the same frequency as Linux
> > > does.
> > Yes, I assume that the bootloader and Linux use and configure the same
> > timer at the same frequency. This is a must since we want to measure the
> > exact boottime. The reason the code is guarded with BOOT_TIME_PRESERVE
> > is because we want to avoid situations like the one you described.
> 
> IMO Kernel options should be safe to enable everytime. They shouldn't
> have side effects on boards that lack the necessary hardware or
> bootloader support. I think we should at least have a commandline option
> or device tree property in which the bootloader can signal that it has
> initialized the timer suitable for calculating the boot time.
> 
> >
> >
> > > Also you assume that the timer startup code doesn't reset the
> > > timer. When these assumptions are not true then you just get bogus
> > > random timing information.
> > If the timer initialization code will reset the timer, the Linux system
> > will work exactly as before, showing boottime 0 in kmsg and 0 in
> > uptime, so no bugs/crashes will occur.
> 
> It may also be that the timer initialization code switches to another
> frequency, but doesn't reset the timer. All kinds of funny things can
> happen when the timer initialization code hasn't been audited to support
> this usecase. So really we shouldn't provide the user an option without
> giving any hint if this can even work on his hardware

I agree, I will add a check for a cmdline parameter passed from bootloader.
I will create a patch v3.

Kind Regards,
Bogdan