linux-kernel - Re: [PATCH v2 1/3] powerpc/time: Export boot

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] [day] [month] [year] [list]

Message-ID: <019d7c10173cd0db81f497e3cfa39491fa8cd2ee.camel@linux.ibm.com>
Date: Sat, 08 Feb 2025 07:59:11 +0530
From: Aboorva Devarajan <aboorvad@...ux.ibm.com>
To: Christophe Leroy <christophe.leroy@...roup.eu>,
        Kajol Jain
	 <kjain@...ux.ibm.com>, maddy@...ux.ibm.com
Cc: atrajeev@...ux.vnet.ibm.com, disgoel@...ux.ibm.com, hbathini@...ux.ibm.com,
        adubey@...ux.ibm.com, linuxppc-dev@...ts.ozlabs.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] powerpc/time: Export boot_tb and log initial
 timebase at boot

On Tue, 2025-02-04 at 11:28 +0100, Christophe Leroy wrote:

Hi Christophe,

> 
> Le 04/02/2025 à 07:39, Kajol Jain a écrit :
> > From: Aboorva Devarajan <aboorvad@...ux.ibm.com>
> > 
> > - Export `boot_tb` for external use, this is useful in perf vpa-dtl
> >    interface, where `boot_tb` can be used to convert raw timebase
> >    values to it's relative boot timestamp.
> 
> A modification of boot_tb can be catastrophic. I would not export it as 
> is as it doesn't need to be modified, would be better to provide it 
> through an accessor, for instance a function called get_boot_tb()
> 
> By the way, it is only modified by time_init() which is an __init 
> function, so boot_tb should be marked __ro_after_init instead of 
> __read_mostly, then in that case it would be more acceptable to export it.


Thanks for the review and suggestions. Yes `boot_tb` is just initialized once
and is never updated again, I’ll update the patch to mark it __ro_after_init.
I’ll also use an accessor function (get_boot_tb()) instead of exporting it,
as it will be clean and maintainable.

Will send a v3 with the updates.

Regards,
Aboorva

> 
> > - Log the initial timebase at `time_init` as it is a useful
> >    information which can be referred to as needed.
> > 
> > Signed-off-by: Aboorva Devarajan <aboorvad@...ux.ibm.com>
> > Signed-off-by: Kajol Jain <kjain@...ux.ibm.com>
> > ---
> >   arch/powerpc/include/asm/time.h | 1 +
> >   arch/powerpc/kernel/time.c      | 4 +++-
> >   2 files changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h
> > index 9bdd8080299b..b6fc5df01d53 100644
> > --- a/arch/powerpc/include/asm/time.h
> > +++ b/arch/powerpc/include/asm/time.h
> > @@ -23,6 +23,7 @@ extern u64 decrementer_max;
> >   extern unsigned long tb_ticks_per_jiffy;
> >   extern unsigned long tb_ticks_per_usec;
> >   extern unsigned long tb_ticks_per_sec;
> > +extern u64 boot_tb;
> >   extern struct clock_event_device decrementer_clockevent;
> >   extern u64 decrementer_max;
> >   
> > diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> > index 0727332ad86f..6e8548f0e48f 100644
> > --- a/arch/powerpc/kernel/time.c
> > +++ b/arch/powerpc/kernel/time.c
> > @@ -137,7 +137,8 @@ EXPORT_SYMBOL_GPL(rtc_lock);
> >   
> >   static u64 tb_to_ns_scale __read_mostly;
> >   static unsigned tb_to_ns_shift __read_mostly;
> > -static u64 boot_tb __read_mostly;
> > +u64 boot_tb __read_mostly;
> > +EXPORT_SYMBOL_GPL(boot_tb);
> >   
> >   extern struct timezone sys_tz;
> >   static long timezone_offset;
> > @@ -943,6 +944,7 @@ void __init time_init(void)
> >   	tb_to_ns_shift = shift;
> >   	/* Save the current timebase to pretty up CONFIG_PRINTK_TIME */
> >   	boot_tb = get_tb();
> > +	pr_debug("%s: timebase at boot: %llu\n", __func__, (unsigned long long)boot_tb);
> >   
> >   	/* If platform provided a timezone (pmac), we correct the time */
> >   	if (timezone_offset) {