| 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: <DE52APJTD9NN.A41RA78NMNIE@nvidia.com>
Date: Mon, 10 Nov 2025 22:50:49 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Joel Fernandes" <joelagnelf@...dia.com>,
<linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>,
<dri-devel@...ts.freedesktop.org>, <dakr@...nel.org>, <acourbot@...dia.com>
Cc: "Alistair Popple" <apopple@...dia.com>, "Miguel Ojeda"
<ojeda@...nel.org>, "Alex Gaynor" <alex.gaynor@...il.com>, "Boqun Feng"
<boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>,
<bjorn3_gh@...tonmail.com>, "Benno Lossin" <lossin@...nel.org>, "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>, "John Hubbard"
<jhubbard@...dia.com>, "Timur Tabi" <ttabi@...dia.com>,
<joel@...lfernandes.org>, <nouveau@...ts.freedesktop.org>, "Nouveau"
<nouveau-bounces@...ts.freedesktop.org>
Subject: Re: [PATCH v2 08/12] nova-core: sequencer: Add register opcodes
On Mon Nov 3, 2025 at 8:59 AM JST, Joel Fernandes wrote:
> These opcodes are used for register write, modify, poll and store (save)
> sequencer operations.
>
> Signed-off-by: Joel Fernandes <joelagnelf@...dia.com>
> ---
> drivers/gpu/nova-core/gsp/sequencer.rs | 138 +++++++++++++++++++++++--
> 1 file changed, 131 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/sequencer.rs b/drivers/gpu/nova-core/gsp/sequencer.rs
> index 48c40140876b..f429fee01f86 100644
> --- a/drivers/gpu/nova-core/gsp/sequencer.rs
> +++ b/drivers/gpu/nova-core/gsp/sequencer.rs
> @@ -5,6 +5,7 @@
> use core::mem::size_of;
> use kernel::alloc::flags::GFP_KERNEL;
> use kernel::device;
> +use kernel::io::poll::read_poll_timeout;
> use kernel::prelude::*;
> use kernel::time::Delta;
> use kernel::transmute::FromBytes;
> @@ -40,13 +41,36 @@ struct GspSequencerInfo<'a> {
>
> /// GSP Sequencer Command types with payload data.
> /// Commands have an opcode and a opcode-dependent struct.
> -#[allow(dead_code)]
> -pub(crate) enum GspSeqCmd {}
> +#[allow(clippy::enum_variant_names)]
> +pub(crate) enum GspSeqCmd {
> + RegWrite(fw::GSP_SEQ_BUF_PAYLOAD_REG_WRITE),
> + RegModify(fw::GSP_SEQ_BUF_PAYLOAD_REG_MODIFY),
> + RegPoll(fw::GSP_SEQ_BUF_PAYLOAD_REG_POLL),
> + RegStore(fw::GSP_SEQ_BUF_PAYLOAD_REG_STORE),
> +}
>
> impl GspSeqCmd {
> /// Creates a new GspSeqCmd from a firmware GSP_SEQUENCER_BUFFER_CMD.
> - pub(crate) fn from_fw_cmd(_cmd: &fw::GSP_SEQUENCER_BUFFER_CMD) -> Result<Self> {
> - Err(EINVAL)
> + pub(crate) fn from_fw_cmd(cmd: &fw::GSP_SEQUENCER_BUFFER_CMD) -> Result<Self> {
> + match cmd.opCode {
> + fw::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_REG_WRITE => {
> + // SAFETY: We're using the union field that corresponds to the opCode.
> + Ok(GspSeqCmd::RegWrite(unsafe { cmd.payload.regWrite }))
> + }
> + fw::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_REG_MODIFY => {
> + // SAFETY: We're using the union field that corresponds to the opCode.
> + Ok(GspSeqCmd::RegModify(unsafe { cmd.payload.regModify }))
> + }
> + fw::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_REG_POLL => {
> + // SAFETY: We're using the union field that corresponds to the opCode.
> + Ok(GspSeqCmd::RegPoll(unsafe { cmd.payload.regPoll }))
> + }
> + fw::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_REG_STORE => {
> + // SAFETY: We're using the union field that corresponds to the opCode.
> + Ok(GspSeqCmd::RegStore(unsafe { cmd.payload.regStore }))
> + }
I'd rather have these `unsafe` statements in the `fw` module - guess
that's all he more reason to add an abstraction layer there.
> + _ => Err(EINVAL),
> + }
> }
>
> pub(crate) fn new(data: &[u8], dev: &device::Device<device::Bound>) -> Result<Self> {
> @@ -64,7 +88,16 @@ pub(crate) fn new(data: &[u8], dev: &device::Device<device::Bound>) -> Result<Se
> /// Get the size of this command in bytes, the command consists of
> /// a 4-byte opcode, and a variable-sized payload.
> pub(crate) fn size_bytes(&self) -> usize {
> - 0
> + let opcode_size = size_of::<fw::GSP_SEQ_BUF_OPCODE>();
> + match self {
> + // For commands with payloads, add the payload size in bytes.
> + GspSeqCmd::RegWrite(_) => opcode_size + size_of::<fw::GSP_SEQ_BUF_PAYLOAD_REG_WRITE>(),
> + GspSeqCmd::RegModify(_) => {
> + opcode_size + size_of::<fw::GSP_SEQ_BUF_PAYLOAD_REG_MODIFY>()
> + }
> + GspSeqCmd::RegPoll(_) => opcode_size + size_of::<fw::GSP_SEQ_BUF_PAYLOAD_REG_POLL>(),
> + GspSeqCmd::RegStore(_) => opcode_size + size_of::<fw::GSP_SEQ_BUF_PAYLOAD_REG_STORE>(),
> + }
> }
> }
>
> @@ -83,12 +116,103 @@ pub(crate) trait GspSeqCmdRunner {
> fn run(&self, sequencer: &GspSequencer<'_>) -> Result;
> }
>
> -impl GspSeqCmdRunner for GspSeqCmd {
> - fn run(&self, _seq: &GspSequencer<'_>) -> Result {
> +impl GspSeqCmdRunner for fw::GSP_SEQ_BUF_PAYLOAD_REG_WRITE {
> + fn run(&self, sequencer: &GspSequencer<'_>) -> Result {
> + dev_dbg!(
> + sequencer.dev,
> + "RegWrite: addr=0x{:x}, val=0x{:x}\n",
> + self.addr,
> + self.val
> + );
Per the other feedback I believe you are going to remove these `dev_dbg`
anyway, but since the opcodes derive a `Debug` implementation, you could
have just printed that from the caller for a similar result and no extra
code.
> + let addr = self.addr as usize;
> + let val = self.val;
> + let _ = sequencer.bar.try_write32(val, addr);
> + Ok(())
> + }
> +}
> +
> +impl GspSeqCmdRunner for fw::GSP_SEQ_BUF_PAYLOAD_REG_MODIFY {
> + fn run(&self, sequencer: &GspSequencer<'_>) -> Result {
> + dev_dbg!(
> + sequencer.dev,
> + "RegModify: addr=0x{:x}, mask=0x{:x}, val=0x{:x}\n",
> + self.addr,
> + self.mask,
> + self.val
> + );
> +
> + let addr = self.addr as usize;
> + if let Ok(temp) = sequencer.bar.try_read32(addr) {
> + let _ = sequencer
> + .bar
> + .try_write32((temp & !self.mask) | self.val, addr);
> + }
> Ok(())
> }
> }
>
> +impl GspSeqCmdRunner for fw::GSP_SEQ_BUF_PAYLOAD_REG_POLL {
> + fn run(&self, sequencer: &GspSequencer<'_>) -> Result {
> + dev_dbg!(
> + sequencer.dev,
> + "RegPoll: addr=0x{:x}, mask=0x{:x}, val=0x{:x}, timeout=0x{:x}, error=0x{:x}\n",
> + self.addr,
> + self.mask,
> + self.val,
> + self.timeout,
> + self.error
> + );
> +
> + let addr = self.addr as usize;
> + let mut timeout_us = i64::from(self.timeout);
> +
> + // Default timeout to 4 seconds.
> + timeout_us = if timeout_us == 0 { 4000000 } else { timeout_us };
`let timeout_us = if self.timeout == 0 { 4_000_000 } else {
i64::from(self.timeout)`
(removes the `mut` on `timeout_us`)
> +
> + // First read.
> + sequencer.bar.try_read32(addr)?;
> +
> + // Poll the requested register with requested timeout.
> + read_poll_timeout(
> + || sequencer.bar.try_read32(addr),
> + |current| (current & self.mask) == self.val,
> + Delta::ZERO,
> + Delta::from_micros(timeout_us),
> + )
> + .map(|_| ())
> + }
> +}
> +
> +impl GspSeqCmdRunner for fw::GSP_SEQ_BUF_PAYLOAD_REG_STORE {
> + fn run(&self, sequencer: &GspSequencer<'_>) -> Result {
> + let addr = self.addr as usize;
> + let _index = self.index;
> +
> + let val = sequencer.bar.try_read32(addr)?;
> +
> + dev_dbg!(
> + sequencer.dev,
> + "RegStore: addr=0x{:x}, index=0x{:x}, value={:?}\n",
> + self.addr,
> + self.index,
> + val
> + );
> +
> + Ok(())
> + }
> +}
> +
> +impl GspSeqCmdRunner for GspSeqCmd {
> + fn run(&self, seq: &GspSequencer<'_>) -> Result {
> + match self {
> + GspSeqCmd::RegWrite(cmd) => cmd.run(seq),
> + GspSeqCmd::RegModify(cmd) => cmd.run(seq),
> + GspSeqCmd::RegPoll(cmd) => cmd.run(seq),
> + GspSeqCmd::RegStore(cmd) => cmd.run(seq),
> + }
> + }
> +}
This makes me wonder: do we need to store the deserialized version of
these operands, and make a second `match` on them? How about passing the
`bar` to the deserialization command and have it run the operand
immediately?
Powered by blists - more mailing lists