linux-kernel - Re: [PATCH RFC v3 6/7] gpu: nova-core: add basic timer device

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]

Message-Id: <D8LPCCP6JNYU.28DYTEK5BB74K@nvidia.com>
Date: Fri, 21 Mar 2025 14:41:12 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Boqun Feng" <boqun.feng@...il.com>
Cc: "Danilo Krummrich" <dakr@...nel.org>, "David Airlie"
 <airlied@...il.com>, "John Hubbard" <jhubbard@...dia.com>, "Ben Skeggs"
 <bskeggs@...dia.com>, "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
 <alex.gaynor@...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>,
 "Simona Vetter" <simona@...ll.ch>, <linux-kernel@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>, <nouveau@...ts.freedesktop.org>,
 <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH RFC v3 6/7] gpu: nova-core: add basic timer device

Hi Boqun,

On Fri Mar 21, 2025 at 3:17 AM JST, Boqun Feng wrote:
> Hi Alexandre,
>
> On Thu, Mar 20, 2025 at 10:39:14PM +0900, Alexandre Courbot wrote:
>> Add a basic timer device and exercise it during device probing. This
>> first draft is probably very questionable.
>> 
>> One point in particular which should IMHO receive attention: the generic
>> wait_on() method aims at providing similar functionality to Nouveau's
>> nvkm_[num]sec() macros. Since this method will be heavily used with
>> different conditions to test, I'd like to avoid monomorphizing it
>> entirely with each instance ; that's something that is achieved in
>> nvkm_xsec() using functions that the macros invoke.
>> 
>> I have tried achieving the same result in Rust using closures (kept
>> as-is in the current code), but they seem to be monomorphized as well.
>> Calling extra functions could work better, but looks also less elegant
>> to me, so I am really open to suggestions here.
>> 
>> Signed-off-by: Alexandre Courbot <acourbot@...dia.com>
>> ---
>>  drivers/gpu/nova-core/driver.rs    |   4 +-
>>  drivers/gpu/nova-core/gpu.rs       |  55 +++++++++++++++-
>>  drivers/gpu/nova-core/nova_core.rs |   1 +
>>  drivers/gpu/nova-core/regs.rs      |  11 ++++
>>  drivers/gpu/nova-core/timer.rs     | 132 +++++++++++++++++++++++++++++++++++++
>>  5 files changed, 201 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs
>> index 63c19f140fbdd65d8fccf81669ac590807cc120f..0cd23aa306e4082405f480afc0530a41131485e7 100644
>> --- a/drivers/gpu/nova-core/driver.rs
>> +++ b/drivers/gpu/nova-core/driver.rs
>> @@ -10,7 +10,7 @@ pub(crate) struct NovaCore {
>>      pub(crate) gpu: Gpu,
>>  }
>>  
>> -const BAR0_SIZE: usize = 8;
>> +const BAR0_SIZE: usize = 0x9500;
>>  pub(crate) type Bar0 = pci::Bar<BAR0_SIZE>;
>>  
>>  kernel::pci_device_table!(
>> @@ -42,6 +42,8 @@ fn probe(pdev: &mut pci::Device, _info: &Self::IdInfo) -> Result<Pin<KBox<Self>>
>>              GFP_KERNEL,
>>          )?;
>>  
>> +        let _ = this.gpu.test_timer();
>> +
>>          Ok(this)
>>      }
>>  }
>> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
>> index d96901e5c8eace1e7c57c77da7def209e8149cd3..f010d3152530b1cec032ca620e59de51a2fc1a13 100644
>> --- a/drivers/gpu/nova-core/gpu.rs
>> +++ b/drivers/gpu/nova-core/gpu.rs
>> @@ -6,8 +6,10 @@
>>  
>>  use crate::driver::Bar0;
>>  use crate::regs;
>> +use crate::timer::Timer;
>>  use crate::util;
>>  use core::fmt;
>> +use core::time::Duration;
>>  
>
> Since there is already a Delta type proposed to represent the timestamp
> difference in kernel:
>
> 	https://lore.kernel.org/rust-for-linux/20250220070611.214262-4-fujita.tomonori@gmail.com/
>
> , could you please make your work based on that and avoid using
> core::time::Duration. Thanks!
>
>>  macro_rules! define_chipset {
>>      ({ $($variant:ident = $value:expr),* $(,)* }) =>
>> @@ -179,6 +181,7 @@ pub(crate) struct Gpu {
>>      /// MMIO mapping of PCI BAR 0
>>      bar: Devres<Bar0>,
>>      fw: Firmware,
>> +    timer: Timer,
>>  }
>>  
> [...]
>> +
>> +/// A timestamp with nanosecond granularity obtained from the GPU timer.
>> +///
>> +/// A timestamp can also be substracted to another in order to obtain a [`Duration`].
>> +///
>> +/// TODO: add Kunit tests!
>> +#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]
>> +pub(crate) struct Timestamp(u64);
>> +
>
> Also an Instant type has been proposed and reviewed for a while:
>
> 	https://lore.kernel.org/rust-for-linux/20250220070611.214262-5-fujita.tomonori@gmail.com/
>
> we should use that type instead of re-inventing the wheel here. Of
> course, it's currently not quite working because Instant is only for
> CLOCK_MONOTONIC. But there was a proposal to make `Instant` generic over
> clock:
>
> 	https://lore.kernel.org/rust-for-linux/20230714-rust-time-v2-1-f5aed84218c4@asahilina.net/
>
> if you follow that design, you can implement a `Instant<NovaGpu>`, where
>
>     ipml Now for NovaGpu {
>         fn now() -> Instant<Self> {
> 	    // your Timer::read() implementation.
> 	}
>     }

Ah, thanks for pointing this out. I'll keep track of these patches,
hopefully they get merged soon!