| Message ID | 20181119214443.25175-2-pasha.tatashin@soleen.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Early boot time stamps for arm64 | expand |
Hi Pavel, On 19/11/2018 21:44, Pavel Tatashin wrote: > Allow printk time stamps/sched_clock() to be available from the early > boot. > > Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com> > --- > arch/arm64/kernel/setup.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c > index f4fc1e0544b7..4df41a66b403 100644 > --- a/arch/arm64/kernel/setup.c > +++ b/arch/arm64/kernel/setup.c > @@ -40,6 +40,7 @@ > #include <linux/efi.h> > #include <linux/psci.h> > #include <linux/sched/task.h> > +#include <linux/sched_clock.h> > #include <linux/mm.h> > > #include <asm/acpi.h> > @@ -279,8 +280,26 @@ arch_initcall(reserve_memblock_reserved_regions); > > u64 __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = INVALID_HWID }; > > +/* > + * Get time stamps available early in boot, useful to identify boot time issues > + * from the early boot. > + */ > +static __init void sched_clock_early_init(void) > +{ > + u64 freq = arch_timer_get_cntfrq(); > + u64 (*read_time)(void) = arch_counter_get_cntvct; We already have arch_timer_read_counter which is exposed from arm_arch_timer.h. > + > + /* Early clock is available only on platforms with known freqs */ This comment is misleading. It should read something like: /* * The arm64 boot protocol mandates that CNTFRQ_EL0 reflects * the timer frequency. To avoid breakage on misconfigured * systems, do not register the early sched_clock if the * programmed value if zero. Other random values will just * result in random output. */ > + if (!freq) > + return; > + > + sched_clock_register(read_time, BITS_PER_LONG, freq); This doesn't seem right. The counter has an architected minimum of 56 bits, and you can't assume that it is going to be more than that. > +} > + > void __init setup_arch(char **cmdline_p) > { > + sched_clock_early_init(); > + > init_mm.start_code = (unsigned long) _text; > init_mm.end_code = (unsigned long) _etext; > init_mm.end_data = (unsigned long) _edata; > Thanks, M.
> > +static __init void sched_clock_early_init(void) > > +{ > > + u64 freq = arch_timer_get_cntfrq(); > > + u64 (*read_time)(void) = arch_counter_get_cntvct; > > We already have arch_timer_read_counter which is exposed from > arm_arch_timer.h. OK > > > + > > + /* Early clock is available only on platforms with known freqs */ > > This comment is misleading. It should read something like: > > /* > * The arm64 boot protocol mandates that CNTFRQ_EL0 reflects > * the timer frequency. To avoid breakage on misconfigured > * systems, do not register the early sched_clock if the > * programmed value if zero. Other random values will just > * result in random output. > */ > OK > > + if (!freq) > > + return; > > + > > + sched_clock_register(read_time, BITS_PER_LONG, freq); > > This doesn't seem right. The counter has an architected minimum of 56 > bits, and you can't assume that it is going to be more than that. Yeah, I saw 56 is used in arm_arch_timer.c, but I could not find where this minimum is defined in aarch64 specs. I will change it to 56. I will send v2 soon. Thank you, Pasha
On Tue, Nov 20, 2018 at 09:40:10AM -0500, Pavel Tatashin wrote: > > > +static __init void sched_clock_early_init(void) > > > +{ > > > + u64 freq = arch_timer_get_cntfrq(); > > > + u64 (*read_time)(void) = arch_counter_get_cntvct; > > > > We already have arch_timer_read_counter which is exposed from > > arm_arch_timer.h. > > OK > > > > > > + > > > + /* Early clock is available only on platforms with known freqs */ > > > > This comment is misleading. It should read something like: > > > > /* > > * The arm64 boot protocol mandates that CNTFRQ_EL0 reflects > > * the timer frequency. To avoid breakage on misconfigured > > * systems, do not register the early sched_clock if the > > * programmed value if zero. Other random values will just > > * result in random output. > > */ > > > > OK > > > > + if (!freq) > > > + return; > > > + > > > + sched_clock_register(read_time, BITS_PER_LONG, freq); > > > > This doesn't seem right. The counter has an architected minimum of 56 > > bits, and you can't assume that it is going to be more than that. > > Yeah, I saw 56 is used in arm_arch_timer.c, but I could not find where > this minimum is defined in aarch64 specs. I will change it to 56. See section G5.1.2 of the ARM ARM for details. Thanks, Andrew Murray > > I will send v2 soon. > > Thank you, > Pasha
On 20/11/2018 14:40, Pavel Tatashin wrote: >>> +static __init void sched_clock_early_init(void) >>> +{ >>> + u64 freq = arch_timer_get_cntfrq(); >>> + u64 (*read_time)(void) = arch_counter_get_cntvct; >> >> We already have arch_timer_read_counter which is exposed from >> arm_arch_timer.h. > > OK > >> >>> + >>> + /* Early clock is available only on platforms with known freqs */ >> >> This comment is misleading. It should read something like: >> >> /* >> * The arm64 boot protocol mandates that CNTFRQ_EL0 reflects >> * the timer frequency. To avoid breakage on misconfigured >> * systems, do not register the early sched_clock if the >> * programmed value if zero. Other random values will just >> * result in random output. >> */ >> > > OK > >>> + if (!freq) >>> + return; >>> + >>> + sched_clock_register(read_time, BITS_PER_LONG, freq); >> >> This doesn't seem right. The counter has an architected minimum of 56 >> bits, and you can't assume that it is going to be more than that. > > Yeah, I saw 56 is used in arm_arch_timer.c, but I could not find where > this minimum is defined in aarch64 specs. I will change it to 56. See D10.1.2 (The system counter) in the ARM ARM[1], which says: <quote> The Generic Timer provides a system counter with the following specification: Width At least 56 bits wide. The value returned by any 64-bit read of the counter is zero-extended to 64 bits. </quote> This is not AArch64-specific though, and 32bit systems implementing the generic timers have the exact same requirements. > I will send v2 soon. Please wait a bit and give others a chance to review this too. Thanks, M. [1] https://static.docs.arm.com/ddi0487/da/DDI0487D_a_armv8_arm.pdf
On 18-11-20 14:55:23, Marc Zyngier wrote: > > Yeah, I saw 56 is used in arm_arch_timer.c, but I could not find where > > this minimum is defined in aarch64 specs. I will change it to 56. > > See D10.1.2 (The system counter) in the ARM ARM[1], which says: > > <quote> > The Generic Timer provides a system counter with the following > specification: > Width At least 56 bits wide. > The value returned by any 64-bit read of the counter is > zero-extended to 64 bits. > </quote> > > This is not AArch64-specific though, and 32bit systems implementing the > generic timers have the exact same requirements. Thank you, makes sense. I was thinking there could be AArch64 specific minimum limit. > > > I will send v2 soon. > > Please wait a bit and give others a chance to review this too. I already sent, but will wait in the future before sending v3. > > Thanks, > > M. > > [1] https://static.docs.arm.com/ddi0487/da/DDI0487D_a_armv8_arm.pdf > -- > Jazz is not dead. It just smells funny...
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index f4fc1e0544b7..4df41a66b403 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -40,6 +40,7 @@ #include <linux/efi.h> #include <linux/psci.h> #include <linux/sched/task.h> +#include <linux/sched_clock.h> #include <linux/mm.h> #include <asm/acpi.h> @@ -279,8 +280,26 @@ arch_initcall(reserve_memblock_reserved_regions); u64 __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = INVALID_HWID }; +/* + * Get time stamps available early in boot, useful to identify boot time issues + * from the early boot. + */ +static __init void sched_clock_early_init(void) +{ + u64 freq = arch_timer_get_cntfrq(); + u64 (*read_time)(void) = arch_counter_get_cntvct; + + /* Early clock is available only on platforms with known freqs */ + if (!freq) + return; + + sched_clock_register(read_time, BITS_PER_LONG, freq); +} + void __init setup_arch(char **cmdline_p) { + sched_clock_early_init(); + init_mm.start_code = (unsigned long) _text; init_mm.end_code = (unsigned long) _etext; init_mm.end_data = (unsigned long) _edata;
Allow printk time stamps/sched_clock() to be available from the early boot. Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com> --- arch/arm64/kernel/setup.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)