| 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
| ||
|
Date: Tue, 16 Apr 2024 10:17:31 +0000
From: "D, Lakshmi Sowjanya" <lakshmi.sowjanya.d@...el.com>
To: Thomas Gleixner <tglx@...utronix.de>, "jstultz@...gle.com"
<jstultz@...gle.com>, "giometti@...eenne.com" <giometti@...eenne.com>,
"corbet@....net" <corbet@....net>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>
CC: "x86@...nel.org" <x86@...nel.org>, "netdev@...r.kernel.org"
<netdev@...r.kernel.org>, "linux-doc@...r.kernel.org"
<linux-doc@...r.kernel.org>, "intel-wired-lan@...ts.osuosl.org"
<intel-wired-lan@...ts.osuosl.org>, "andriy.shevchenko@...ux.intel.com"
<andriy.shevchenko@...ux.intel.com>, "Dong, Eddie" <eddie.dong@...el.com>,
"Hall, Christopher S" <christopher.s.hall@...el.com>, "Brandeburg, Jesse"
<jesse.brandeburg@...el.com>, "davem@...emloft.net" <davem@...emloft.net>,
"alexandre.torgue@...s.st.com" <alexandre.torgue@...s.st.com>,
"joabreu@...opsys.com" <joabreu@...opsys.com>, "mcoquelin.stm32@...il.com"
<mcoquelin.stm32@...il.com>, "perex@...ex.cz" <perex@...ex.cz>,
"linux-sound@...r.kernel.org" <linux-sound@...r.kernel.org>, "Nguyen, Anthony
L" <anthony.l.nguyen@...el.com>, "peter.hilber@...nsynergy.com"
<peter.hilber@...nsynergy.com>, "N, Pandith" <pandith.n@...el.com>, "Mohan,
Subramanian" <subramanian.mohan@...el.com>, "T R, Thejesh Reddy"
<thejesh.reddy.t.r@...el.com>
Subject: RE: [PATCH v6 08/11] timekeeping: Add function to convert realtime to
base clock
> -----Original Message-----
> From: Thomas Gleixner <tglx@...utronix.de>
> Sent: Thursday, April 11, 2024 3:46 AM
> To: D, Lakshmi Sowjanya <lakshmi.sowjanya.d@...el.com>;
> jstultz@...gle.com; giometti@...eenne.com; corbet@....net; linux-
> kernel@...r.kernel.org
> Cc: x86@...nel.org; netdev@...r.kernel.org; linux-doc@...r.kernel.org; intel-
> wired-lan@...ts.osuosl.org; andriy.shevchenko@...ux.intel.com; Dong, Eddie
> <eddie.dong@...el.com>; Hall, Christopher S <christopher.s.hall@...el.com>;
> Brandeburg, Jesse <jesse.brandeburg@...el.com>; davem@...emloft.net;
> alexandre.torgue@...s.st.com; joabreu@...opsys.com;
> mcoquelin.stm32@...il.com; perex@...ex.cz; linux-sound@...r.kernel.org;
> Nguyen, Anthony L <anthony.l.nguyen@...el.com>;
> peter.hilber@...nsynergy.com; N, Pandith <pandith.n@...el.com>; Mohan,
> Subramanian <subramanian.mohan@...el.com>; T R, Thejesh Reddy
> <thejesh.reddy.t.r@...el.com>; D, Lakshmi Sowjanya
> <lakshmi.sowjanya.d@...el.com>
> Subject: Re: [PATCH v6 08/11] timekeeping: Add function to convert realtime to
> base clock
>
> On Wed, Apr 10 2024 at 17:18, lakshmi.sowjanya.d@...el.com wrote:
> > From: Lakshmi Sowjanya D <lakshmi.sowjanya.d@...el.com>
> >
> > PPS(Pulse Per Second) generates signals in realtime, but Timed IO
>
> ... generates signals based on CLOCK_REALTIME, but ...
>
> > hardware understands time in base clock reference.
>
> The hardware does not understand anything.
>
> > Add an interface,
> > ktime_real_to_base_clock() to convert realtime to base clock.
> >
> > Add the helper function timekeeping_clocksource_has_base(), to check
> > whether the current clocksource has the same base clock. This will be
> > used by Timed IO device to check if the base clock is X86_ART(Always
> > Running Timer).
>
> Again this fails to explain the rationale and as this is a core change which is
> hardware agnostic the whole Timed IO and ART reference is not really helpful.
> Something like this:
>
> "PPS (Pulse Per Second) generates a hardware pulse every second based
> on CLOCK_REALTIME. This works fine when the pulse is generated in
> software from a hrtimer callback function.
>
> For hardware which generates the pulse by programming a timer it's
> required to convert CLOCK_REALTIME to the underlying hardware clock.
>
> The X86 Timed IO device is based on the Always Running Timer (ART),
> which is the base clock of the TSC, which is usually the system
> clocksource on X86.
>
> The core code already has functionality to convert base clock
> timestamps to system clocksource timestamps, but there is no support
> for converting the other way around.
>
> Provide the required functionality to support such devices in a
> generic way to avoid code duplication in drivers:
>
> 1) ktime_real_to_base_clock() to convert a CLOCK_REALTIME
> timestamp to a base clock timestamp
>
> 2) timekeeping_clocksource_has_base() to allow drivers to validate
> that the system clocksource is based on a particular
> clocksource ID.
Thanks for the commit message.
I will update as suggested.
>
> > +static bool convert_cs_to_base(u64 *cycles, enum clocksource_ids
> > +base_id) {
> > + struct clocksource *cs = tk_core.timekeeper.tkr_mono.clock;
> > + struct clocksource_base *base = cs->base;
> > +
> > + /* Check whether base_id matches the base clock */
> > + if (!base || base->id != base_id)
> > + return false;
> > +
> > + *cycles -= base->offset;
> > + if (!convert_clock(cycles, base->denominator, base->numerator))
> > + return false;
> > + return true;
> > +}
> > +
> > +static u64 convert_ns_to_cs(u64 delta) {
> > + struct tk_read_base *tkr = &tk_core.timekeeper.tkr_mono;
> > +
> > + return div_u64((delta << tkr->shift) - tkr->xtime_nsec, tkr->mult);
> > +}
>
> > +bool ktime_real_to_base_clock(ktime_t treal, enum clocksource_ids
> > +base_id, u64 *cycles)
>
> As this is a kernel API function it really wants kernel-doc comment to explain the
> functionality, the arguments and the return value.
Will add the following documentation:
" ktime_real_to_base_clock()- Convert CLOCK_REALTIME timestamp to a base clock timestamp.
@treal: CLOCK_REALTIME timestamp to convert.
@base_id: base clocksource id.
@*cycles: pointer to store the converted base clock timestamp.
Converts a supplied, future realtime clock value to the corresponding base clock value.
Return: true if the conversion is successful, false otherwise."
>
> > +{
> > + struct timekeeper *tk = &tk_core.timekeeper;
> > + unsigned int seq;
> > + u64 delta;
> > +
> > + do {
> > + seq = read_seqcount_begin(&tk_core.seq);
> > + if ((u64)treal < tk->tkr_mono.base_real)
> > + return false;
> > + delta = (u64)treal - tk->tkr_mono.base_real;
>
> In the previous version you had a sanity check on delta:
>
> >>> + if (delta > tk->tkr_mono.clock->max_idle_ns)
> >>> + return false;
>
> And I told you:
>
> >> I don't think this cutoff is valid. There is no guarantee that this
> >> is linear unless:
> >>
> >> Treal[last timekeeper update] <= treal < Treal[next timekeeper
> >> update]
> >>
> >> Look at the dance in get_device_system_crosststamp() and
> >> adjust_historical_crosststamp() to see why.
>
> So now there is not even a check anymore whether the delta conversion can
> overflow.
>
> There is zero explanation why this conversion is considered to be correct.
Adding the following check for delta overflow in convert_ns_to_cs function.
if (BITS_TO_BYTES(fls64(*delta) + tkr->shift) >= sizeof(*delta))
return false;
>
> > + *cycles = tk->tkr_mono.cycle_last + convert_ns_to_cs(delta);
> > + if (!convert_cs_to_base(cycles, base_id))
> > + return false;
> > + } while (read_seqcount_retry(&tk_core.seq, seq));
> > +
> > + return true;
> > +}
>
> > +/**
> > + * timekeeping_clocksource_has_base - Check whether the current
> clocksource
> > + * has a base clock
>
> s/has a base clock/is based on a given base clock
>
> > + * @id: The clocksource ID to check for
>
> base clocksource ID
>
> > + *
> > + * Note: The return value is a snapshot which can become invalid right
> > + * after the function returns.
> > + *
> > + * Return: true if the timekeeper clocksource has a base clock with @id,
> > + * false otherwise
> > + */
>
> Thanks,
>
> tglx
Powered by blists - more mailing lists