| 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
| ||
|
Message-ID: <aAeGdRvlm5EVJOw3@cassiopeiae>
Date: Tue, 22 Apr 2025 14:07:17 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Alexandre Courbot <acourbot@...dia.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Benno Lossin <benno.lossin@...ton.me>,
Andreas Hindborg <a.hindborg@...nel.org>,
Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
Jonathan Corbet <corbet@....net>,
John Hubbard <jhubbard@...dia.com>, Ben Skeggs <bskeggs@...dia.com>,
Joel Fernandes <joelagnelf@...dia.com>,
Timur Tabi <ttabi@...dia.com>, Alistair Popple <apopple@...dia.com>,
linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
nouveau@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH 10/16] gpu: nova-core: add basic timer device
On Sun, Apr 20, 2025 at 09:19:42PM +0900, Alexandre Courbot wrote:
> Add a timer that works with GPU time and provides the ability to wait on
> a condition with a specific timeout.
What can this timer do for us, what and HrTimer can't do for us?
>
> The `Duration` Rust type is used to keep track is differences between
> timestamps ; this will be replaced by the equivalent kernel type once it
> lands.
Fine for me -- can you please add a corresponding TODO and add it to your list
of follow-up patches?
> diff --git a/drivers/gpu/nova-core/timer.rs b/drivers/gpu/nova-core/timer.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..8987352f4192bc9b4b2fc0fb5f2e8e62ff27be68
> --- /dev/null
> +++ b/drivers/gpu/nova-core/timer.rs
> @@ -0,0 +1,133 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Nova Core Timer subdevice
> +
> +// To be removed when all code is used.
> +#![allow(dead_code)]
Please prefer 'expect'.
> +
> +use core::fmt::Display;
> +use core::ops::{Add, Sub};
> +use core::time::Duration;
> +
> +use kernel::devres::Devres;
> +use kernel::num::U64Ext;
> +use kernel::prelude::*;
> +
> +use crate::driver::Bar0;
> +use crate::regs;
> +
> +/// A timestamp with nanosecond granularity obtained from the GPU timer.
> +///
> +/// A timestamp can also be substracted to another in order to obtain a [`Duration`].
> +#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]
> +pub(crate) struct Timestamp(u64);
> +
> +impl Display for Timestamp {
> + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> + write!(f, "{}", self.0)
> + }
> +}
> +
> +impl Add<Duration> for Timestamp {
> + type Output = Self;
> +
> + fn add(mut self, rhs: Duration) -> Self::Output {
> + let mut nanos = rhs.as_nanos();
> + while nanos > u64::MAX as u128 {
> + self.0 = self.0.wrapping_add(nanos as u64);
> + nanos -= u64::MAX as u128;
> + }
> +
> + Timestamp(self.0.wrapping_add(nanos as u64))
> + }
> +}
> +
> +impl Sub for Timestamp {
> + type Output = Duration;
> +
> + fn sub(self, rhs: Self) -> Self::Output {
> + Duration::from_nanos(self.0.wrapping_sub(rhs.0))
> + }
> +}
> +
> +pub(crate) struct Timer {}
> +
> +impl Timer {
> + pub(crate) fn new() -> Self {
> + Self {}
> + }
> +
> + /// Read the current timer timestamp.
> + pub(crate) fn read(&self, bar: &Bar0) -> Timestamp {
> + loop {
> + let hi = regs::PtimerTime1::read(bar);
> + let lo = regs::PtimerTime0::read(bar);
> +
> + if hi.hi() == regs::PtimerTime1::read(bar).hi() {
> + return Timestamp(u64::from_u32s(hi.hi(), lo.lo()));
> + }
So, if hi did not change since we've read both hi and lo, we can trust both
values. Probably worth to add a brief comment.
Additionally, we may want to add that if we get unlucky, it takes around 4s to
get unlucky again, even though that's rather obvious.
> + }
> + }
> +
> + #[allow(dead_code)]
> + pub(crate) fn time(bar: &Bar0, time: u64) {
> + regs::PtimerTime1::default()
> + .set_hi(time.upper_32_bits())
> + .write(bar);
> + regs::PtimerTime0::default()
> + .set_lo(time.lower_32_bits())
> + .write(bar);
> + }
> +
> + /// Wait until `cond` is true or `timeout` elapsed, based on GPU time.
> + ///
> + /// When `cond` evaluates to `Some`, its return value is returned.
> + ///
> + /// `Err(ETIMEDOUT)` is returned if `timeout` has been reached without `cond` evaluating to
> + /// `Some`, or if the timer device is stuck for some reason.
> + pub(crate) fn wait_on<R, F: Fn() -> Option<R>>(
> + &self,
> + bar: &Devres<Bar0>,
> + timeout: Duration,
> + cond: F,
> + ) -> Result<R> {
> + // Number of consecutive time reads after which we consider the timer frozen if it hasn't
> + // moved forward.
> + const MAX_STALLED_READS: usize = 16;
Huh! Can't we trust the timer hardware? Probably one reason more to use HrTimer?
> +
> + let (mut cur_time, mut prev_time, deadline) = {
> + let cur_time = with_bar!(bar, |b| self.read(b))?;
> + let deadline = cur_time + timeout;
> +
> + (cur_time, cur_time, deadline)
> + };
> + let mut num_reads = 0;
> +
> + loop {
> + if let Some(ret) = cond() {
> + return Ok(ret);
> + }
> +
> + (|| {
> + cur_time = with_bar!(bar, |b| self.read(b))?;
> +
> + /* Check if the timer is frozen for some reason. */
> + if cur_time == prev_time {
> + if num_reads >= MAX_STALLED_READS {
> + return Err(ETIMEDOUT);
> + }
> + num_reads += 1;
> + } else {
> + if cur_time >= deadline {
> + return Err(ETIMEDOUT);
> + }
> +
> + num_reads = 0;
> + prev_time = cur_time;
> + }
> +
> + Ok(())
> + })()?;
> + }
> + }
> +}
>
> --
> 2.49.0
>
Powered by blists - more mailing lists