linux-kernel - Re: [PATCH v3] Added "Preserve Boot Time Support"

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 26 May 2017 12:09:59 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Bogdan Mirea <Bogdan-Stefan_mirea@...tor.com>
Cc:     linux-kernel@...r.kernel.org, john.stultz@...aro.org,
        tglx@...utronix.de, ore@...gutronix.de, kernel@...gutronix.de
Subject: Re: [PATCH v3] Added "Preserve Boot Time Support"

On Fri, May 19, 2017 at 01:16:23PM +0300, 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 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@...tor.com>
> ---
>  drivers/clocksource/arm_arch_timer.c | 33 +++++++++++++++++++++++++++++++++

In future, *please* use get_maintainer.pl, and ensure all relevant
maintainers are on Cc. e.g.

[mark@...erpostej:~/src/linux]% ./scripts/get_maintainer.pl -f drivers/clocksource/arm_arch_timer.c
Mark Rutland <mark.rutland@....com> (maintainer:ARM ARCHITECTED TIMER DRIVER)
Marc Zyngier <marc.zyngier@....com> (maintainer:ARM ARCHITECTED TIMER DRIVER)
Daniel Lezcano <daniel.lezcano@...aro.org> (supporter:CLOCKSOURCE, CLOCKEVENT DRIVERS)
Thomas Gleixner <tglx@...utronix.de> (supporter:CLOCKSOURCE, CLOCKEVENT DRIVERS)
linux-arm-kernel@...ts.infradead.org (moderated list:ARM ARCHITECTED TIMER DRIVER)
linux-kernel@...r.kernel.org (open list:CLOCKSOURCE, CLOCKEVENT DRIVERS)

As I was not Cc'd, I only spotted this by chance.

[...]

> 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 */
>  }

Regardless of what this is trying to achieve, this does not belong in
the arch timer driver, as Thomas has already said.

Thanks,
Mark.