Re: [PATCH 2/2] gpu: nova-core: add support for GPU timer block.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] gpu: nova-core: infinite loop on rollover in timer_read() and timer_time()
- [High] gpu: nova-core: unshifted nsec value in timer_time() causes EOVERFLOW
- [High] gpu: nova-core: wrong value and missing update bit in gh100::timer_time()
- [High] gpu: nova-core: u64 underflow on negative offset in gh100::timer_time()
--

commit 5b1f5a3805c6ab01f8b19ff029b873428252d6b5
Author: Dave Airlie <airlied@redhat.com>

gpu: nova-core: add support for GPU timer block.

This commit adds GPU timer block support to nova-core with HAL implementations
for TU102 and GH100 to initialize the GPU timer or calculate the system timer
offset for GSP initialization.

> diff --git a/drivers/gpu/nova-core/timer/hal/gh100.rs b/drivers/gpu/nova-core/timer/hal/gh100.rs
> new file mode 100644
> index 0000000000000..e917446e7b01e
> --- /dev/null
> +++ b/drivers/gpu/nova-core/timer/hal/gh100.rs
> @@ -0,0 +1,61 @@

[ ... ]

> +pub(crate) fn timer_time(bar: Bar0<'_>, chipset: Chipset, time_ns: u64) -> Result<u64> {
> +    let sec_hi2 = u32::from(bar.read(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_1));
> +    let sec_timer_ns: u64;
> +    loop {
> +        let sec_lo = u32::from(bar.read(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_0));
> +        let sec_hi = u32::from(bar.read(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_1));
> +        if sec_hi == sec_hi2 {
> +            sec_timer_ns = u64::from(sec_hi) << 32 | u64::from(sec_lo);
> +            break;
> +        }
> +    }

[Severity: High]
Can this loop in timer_time() in drivers/gpu/nova-core/timer/hal/gh100.rs
spin forever if NV_PGC6_SCI_SEC_TIMER_TIME_0 rolls over?

During Gpu::new() -> Timer::new() -> Gh100::init() -> timer_time(), sec_hi2
is read from NV_PGC6_SCI_SEC_TIMER_TIME_1 once before the loop and is never
updated if sec_hi != sec_hi2. If the lower 32-bit register wraps around
between the initial read of sec_hi2 and the in-loop read of sec_hi, sec_hi
increments while sec_hi2 remains at its old value, causing the loop to never
terminate. Should sec_hi2 be made mutable and updated to sec_hi at the end
of each iteration?

> +
> +    let sys_timer_offset_ns = time_ns - sec_timer_ns;
> +
> +    // kfspRequiresBug3957833WAR_HAL
> +    if chipset != Chipset::GH100 {
> +        bar.write_reg(
> +            regs::gh100::NV_PGC6_SCI_SYS_TIMER_OFFSET_1::zeroed()
> +                .try_with_nsec((sys_timer_offset_ns >> 32) as u32)?,
> +        );

[Severity: High]
What happens in timer_time() in drivers/gpu/nova-core/timer/hal/gh100.rs when
time_ns is smaller than sec_timer_ns?

In drivers/gpu/nova-core/timer.rs, get_cpu_time_ns() returns monotonic time
since OS boot via Instant::<Monotonic>::now().as_nanos(), whereas
sec_timer_ns is read from the always-on PGC6 timer which persists across warm
reboots, kexec, and guest VM restarts.

When time_ns < sec_timer_ns, the u64 subtraction time_ns - sec_timer_ns will
underflow and panic the kernel when CONFIG_RUST_OVERFLOW_CHECKS=y. Even if
wrapping subtraction is used, a negative 64-bit two's-complement offset has
its upper bits set to 1, so passing an unmasked (sys_timer_offset_ns >> 32)
as u32 to NV_PGC6_SCI_SYS_TIMER_OFFSET_1::zeroed().try_with_nsec() will
exceed the 29-bit (28:0 nsec) field and fail with EOVERFLOW.

> +        bar.write_reg(
> +            regs::gh100::NV_PGC6_SCI_SYS_TIMER_OFFSET_0::zeroed()
> +                .try_with_nsec((time_ns & 0xffffffff) as u32)?,
> +        );
> +    }
> +    Ok(sys_timer_offset_ns)
> +}

[Severity: High]
Are there a few issues with how NV_PGC6_SCI_SYS_TIMER_OFFSET_0 is written
here in timer_time() in drivers/gpu/nova-core/timer/hal/gh100.rs?

In drivers/gpu/nova-core/regs.rs, NV_PGC6_SCI_SYS_TIMER_OFFSET_0 is defined
as:

    pub(crate) NV_PGC6_SCI_SYS_TIMER_OFFSET_0(u32) @ 0x00118df4 {
        0:0 update;
        31:5 nsec;
    }

First, this call passes (time_ns & 0xffffffff) as u32 instead of the lower
bits of sys_timer_offset_ns, and it does not set the update bit (0:0 update)
to trigger hardware latching of the offset.

Second, because nsec is a 27-bit field at bits 31:5, try_with_nsec() checks
that the argument fits in 27 bits (< 1 << 27) and then shifts it left by 5.
Passing an unshifted 32-bit value causes try_with_nsec() to fail with
EOVERFLOW whenever any of bits 31:27 are non-zero, and shifts the value left
by an extra 5 bits when bits 31:27 happen to be zero. Should this pass
((sys_timer_offset_ns as u32) >> 5) and set update?

> diff --git a/drivers/gpu/nova-core/timer/hal/tu102.rs b/drivers/gpu/nova-core/timer/hal/tu102.rs
> new file mode 100644
> index 0000000000000..06ef88171f587
> --- /dev/null
> +++ b/drivers/gpu/nova-core/timer/hal/tu102.rs
> @@ -0,0 +1,55 @@

[ ... ]

> +pub(crate) fn timer_read(bar: Bar0<'_>) -> u64 {
> +    let hi2: u32 = u32::from(bar.read(regs::NV_PTIMER_TIME_1));
> +    loop {
> +        let lo = u32::from(bar.read(regs::NV_PTIMER_TIME_0));
> +        let hi = u32::from(bar.read(regs::NV_PTIMER_TIME_1));
> +        if hi == hi2 {
> +            return u64::from(hi) << 32 | u64::from(lo);
> +        }
> +    }
> +}

[Severity: High]
Similarly, in timer_read() in drivers/gpu/nova-core/timer/hal/tu102.rs, hi2
is read once before the loop and never updated to hi when hi != hi2. If
NV_PTIMER_TIME_0 wraps around between the initial read of hi2 and the in-loop
read of hi, won't this loop spin forever?

> +
> +pub(crate) fn timer_time(bar: Bar0<'_>, time_ns: u64) -> Result {
> +    if bar.read(regs::NV_PTIMER_TIME_PRIV_LEVEL_MASK).level0() {
> +        bar.write_reg(regs::NV_PTIMER_TIME_1::zeroed().try_with_nsec((time_ns >> 32) as u32)?);
> +        bar.write_reg(
> +            regs::NV_PTIMER_TIME_0::zeroed().try_with_nsec((time_ns & 0xffffffff) as u32)?,
> +        );

[Severity: High]
In timer_time() in drivers/gpu/nova-core/timer/hal/tu102.rs, NV_PTIMER_TIME_0
also defines nsec as a 27-bit field at bits 31:5:

    pub(crate) NV_PTIMER_TIME_0(u32) @ 0x00009400 {
        31:5 nsec;
    }

When called during probe via Gpu::new() -> Timer::new() -> Tu102::init() ->
timer_time(), won't passing the unshifted (time_ns & 0xffffffff) as u32 to
try_with_nsec() return EOVERFLOW whenever bits 31:27 of the low 32-bit
timestamp are non-zero, and otherwise shift the value left by an extra 5
bits?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-drm-rust-next-v1-0-14a4ae469175@redhat.com?part=2