Date:   Wed, 17 May 2017 13:30:27 +0200
From:   Sascha Hauer <s.hauer@...gutronix.de>
To:     "Mirea, Bogdan-Stefan" <Bogdan-Stefan_Mirea@...tor.com>
Cc:     Oleksij Rempel <ore@...gutronix.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "john.stultz@...aro.org" <john.stultz@...aro.org>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>
Subject: Re: [PATCH v2] Added "Preserve Boot Time Support"

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 |

Powered by blists - more mailing lists