[PATCH 0/7] gpu: nova-core: run unload sequence upon unbinding

[PATCH 0/7] gpu: nova-core: run unload sequence upon unbinding

[Alexandre Courbot]

Currently the GSP is left running and the WPR2 memory region untouched
as the driver is unbound. This is obviously not idea for at least two
reasons:

- Probing requires setting up the WPR2 region, which cannot be done if
  there is already one in place. Thus the current requirement to reset
  the GPU (using e.g. `echo 1 >/sys/bus/pci/devices/.../reset`) before
  the driver can be probed again after removal.
- The running GSP may still attempt to access shared memory regions,
  which the kernel might recycle.

This patchset does the necessary to leave the GPU in a clean state after
unbind.

First are a few preparatory patches:

- Running the unload sequence requires mutable access to the driver
  data, but the current device unbind method only passes a non-mutable
  reference to it. Since the driver data is destroyed after the call to
  `unbind`, we can just give ownership back to the driver at this stage
  to solve this issue.
  The need for mutable access is likely to go away in Nova after we
  support concurrency on the command queue, but for now we need it and
  it looks like a sensible design direction anyway.
- A `warn_on_err` macro is introduced to call `warn_on` if the passed
  `Result` is an error. This simplifies the unbind sequence's code as we
  need to proceed to the next step even if the previous one failed.
- A fix (?) to the automatically-generated pin-projected structures,
  suppressing the warnings when using them partially.

With these in place, the rest of the patchset is relatively trivial. We
change the signatures of methods related to unbinding to work with
mutable pinned driver data, then implement the two steps of the GPU
unbind sequence: asking the GSP to shut down, and removing the WPR2
protected memory area.

This series sits on top of the following:

- Nova fixes for this cycle [1].
- Nova misc improvements [2].
- Transmute on ZSTs [3].

A tree with all the required patches is available in [4].

[1] https://lore.kernel.org/all/20251216-nova-fixes-v3-0-c7469a71f7c4@nvidia.com/
[2] https://lore.kernel.org/all/20251216-nova-misc-v2-0-dc7b42586c04@nvidia.com/
[3] https://lore.kernel.org/all/20251215-transmute_unit-v4-0-477d71ec7c23@nvidia.com/
[4] https://github.com/Gnurou/linux/tree/b4/nova-unload

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
Alexandre Courbot (7):
      rust: pci: pass driver data by value to `unbind`
      rust: add warn_on_err macro
      gpu: nova-core: use warn_on_err macro
      [RFC] rust: pin-init: allow `dead_code` on projection structure
      gpu: nova-nova: use pin-init projections
      gpu: nova-core: send UNLOADING_GUEST_DRIVER GSP command GSP upon unloading
      gpu: nova-core: run Booter Unloader and FWSEC-SB upon unbinding

 drivers/gpu/nova-core/driver.rs                   |  4 +-
 drivers/gpu/nova-core/firmware/booter.rs          |  1 -
 drivers/gpu/nova-core/firmware/fwsec.rs           |  1 -
 drivers/gpu/nova-core/gpu.rs                      | 25 ++++++--
 drivers/gpu/nova-core/gsp/boot.rs                 | 77 +++++++++++++++++++++++
 drivers/gpu/nova-core/gsp/commands.rs             | 42 +++++++++++++
 drivers/gpu/nova-core/gsp/fw.rs                   |  4 ++
 drivers/gpu/nova-core/gsp/fw/commands.rs          | 27 ++++++++
 drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs |  8 +++
 drivers/gpu/nova-core/regs.rs                     |  5 ++
 rust/kernel/bug.rs                                | 10 +++
 rust/kernel/pci.rs                                |  4 +-
 rust/pin-init/src/macros.rs                       |  1 +
 samples/rust/rust_driver_pci.rs                   |  2 +-
 14 files changed, 198 insertions(+), 13 deletions(-)
---
base-commit: 8d4031f6a53fe47449b91f30cd7aa5b439558874
change-id: 20251216-nova-unload-4029b3b76950

Best regards,
-- 
Alexandre Courbot <acourbot@nvidia.com>

[PATCH 1/7] rust: pci: pass driver data by value to `unbind`

[Alexandre Courbot]

When unbinding a PCI driver, the `T::unbind` callback is invoked by the
driver framework, passing the driver data as a `Pin<&T>`.

This artificially restricts what the driver can do, as it cannot mutate
any state on the data. This becomes a problem in e.g. Nova, which needs
to invoke mutable methods when unbinding.

`remove_callback` retrieves the driver data by value, and drops it right
after the call to `T::unbind`, meaning it is the only reference to the
driver data by the time `T::unbind` is called.

There is thus no reason for not granting full ownership of the data to
`T::unbind`, so do it. This allows the driver mutate the state of its
data while unbinding, and also makes the API more symmetric as the
framework grants back to the driver the data it created in the first
place.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpu/nova-core/driver.rs | 2 +-
 rust/kernel/pci.rs              | 4 ++--
 samples/rust/rust_driver_pci.rs | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs
index b8b0cc0f2d93..b0e3e41ae9d8 100644
--- a/drivers/gpu/nova-core/driver.rs
+++ b/drivers/gpu/nova-core/driver.rs
@@ -98,7 +98,7 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> impl PinInit<Self, E
         })
     }
 
-    fn unbind(pdev: &pci::Device<Core>, this: Pin<&Self>) {
+    fn unbind(pdev: &pci::Device<Core>, this: Pin<KBox<Self>>) {
         this.gpu.unbind(pdev.as_ref());
     }
 }
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 82e128431f08..39e70adde484 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -116,7 +116,7 @@ extern "C" fn remove_callback(pdev: *mut bindings::pci_dev) {
         // and stored a `Pin<KBox<T>>`.
         let data = unsafe { pdev.as_ref().drvdata_obtain::<T>() };
 
-        T::unbind(pdev, data.as_ref());
+        T::unbind(pdev, data);
     }
 }
 
@@ -303,7 +303,7 @@ pub trait Driver: Send {
     /// operations to gracefully tear down the device.
     ///
     /// Otherwise, release operations for driver resources should be performed in `Self::drop`.
-    fn unbind(dev: &Device<device::Core>, this: Pin<&Self>) {
+    fn unbind(dev: &Device<device::Core>, this: Pin<KBox<Self>>) {
         let _ = (dev, this);
     }
 }
diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
index 5823787bea8e..ee8fc8d48ff3 100644
--- a/samples/rust/rust_driver_pci.rs
+++ b/samples/rust/rust_driver_pci.rs
@@ -95,7 +95,7 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Er
         })
     }
 
-    fn unbind(pdev: &pci::Device<Core>, this: Pin<&Self>) {
+    fn unbind(pdev: &pci::Device<Core>, this: Pin<KBox<Self>>) {
         if let Ok(bar) = this.bar.access(pdev.as_ref()) {
             // Reset pci-testdev by writing a new test index.
             bar.write8(this.index.0, Regs::TEST);

-- 
2.52.0

Re: [PATCH 1/7] rust: pci: pass driver data by value to `unbind`

[Danilo Krummrich]

On Tue Dec 16, 2025 at 6:13 AM CET, Alexandre Courbot wrote:
> When unbinding a PCI driver, the `T::unbind` callback is invoked by the
> driver framework, passing the driver data as a `Pin<&T>`.
>
> This artificially restricts what the driver can do, as it cannot mutate
> any state on the data. This becomes a problem in e.g. Nova, which needs
> to invoke mutable methods when unbinding.
>
> `remove_callback` retrieves the driver data by value, and drops it right
> after the call to `T::unbind`, meaning it is the only reference to the
> driver data by the time `T::unbind` is called.
>
> There is thus no reason for not granting full ownership of the data to
> `T::unbind`, so do it.

There are multiple reasons I did avoid this for:

(1) Race conditions

A driver can call Device::drvdata() and obtain a reference to the driver's
device private data as long as it has a &Device<Bound> and asserts the correct
type of the driver's device private data [1].

Assume you have an IRQ registration, for instance, that lives within this device
private data.  Within the IRQ handler, nothing prevents us from calling
Device::drvdata() given that the IRQ handler has a Device<Bound> reference.

Consequently, with passing the device private data by value to unbind() it can
happen that we have both a mutable and immutable reference at of the device
private data at the same time.

The same is true for a lot of other cases, such as work items or workqueues that
are scoped to the Device being bound living within the device private data.

More generally, you can't take full ownership of the device private data as long
as the device is not yet fully unbound (which is not yet the case in unbind()).

(2) Design

It is intentional that the device private data has a defined lifetime that ends
with the device being unbound from its driver. This way we get the guarantee
that any object stored in the device private data won't survive device / driver
unbind. If we give back the ownership to the driver, this guarantee is lost.

Conclusion:

Having that said, if you need mutable access to the fields of the device private
data within unbind() the corresponding field(s) should be protected by a lock.

Alternatively, you have mutable access within the destructor as well, but there
you don't have a bound device anymore. Which is only consequent, since we can't
call the destructor of the device private data before the device is fully
unbound.

(In fact, as by now, there is a bug with this, which I noticed a few days ago
when I still was on vacation: The bus implementations call the destructor of the
device private data too early, i.e. when the device is not fully unbound yet.

I'm working on a fix for this. Luckily, as by now, this is not a real bug in
practice, yet it has to be fixed.)

From your end you don't have to worry about this though. nova-core should just
employ a lock for this, as we will need it in the future anyways, since we will
have concurrent access to the GSP.

[1] https://rust.docs.kernel.org/kernel/device/struct.Device.html#method.drvdata

Re: [PATCH 1/7] rust: pci: pass driver data by value to `unbind`

[Alexandre Courbot]

On Tue Dec 16, 2025 at 9:14 PM JST, Danilo Krummrich wrote:
> On Tue Dec 16, 2025 at 6:13 AM CET, Alexandre Courbot wrote:
>> When unbinding a PCI driver, the `T::unbind` callback is invoked by the
>> driver framework, passing the driver data as a `Pin<&T>`.
>>
>> This artificially restricts what the driver can do, as it cannot mutate
>> any state on the data. This becomes a problem in e.g. Nova, which needs
>> to invoke mutable methods when unbinding.
>>
>> `remove_callback` retrieves the driver data by value, and drops it right
>> after the call to `T::unbind`, meaning it is the only reference to the
>> driver data by the time `T::unbind` is called.
>>
>> There is thus no reason for not granting full ownership of the data to
>> `T::unbind`, so do it.
>
> There are multiple reasons I did avoid this for:
>
> (1) Race conditions
>
> A driver can call Device::drvdata() and obtain a reference to the driver's
> device private data as long as it has a &Device<Bound> and asserts the correct
> type of the driver's device private data [1].
>
> Assume you have an IRQ registration, for instance, that lives within this device
> private data.  Within the IRQ handler, nothing prevents us from calling
> Device::drvdata() given that the IRQ handler has a Device<Bound> reference.
>
> Consequently, with passing the device private data by value to unbind() it can
> happen that we have both a mutable and immutable reference at of the device
> private data at the same time.
>
> The same is true for a lot of other cases, such as work items or workqueues that
> are scoped to the Device being bound living within the device private data.
>
> More generally, you can't take full ownership of the device private data as long
> as the device is not yet fully unbound (which is not yet the case in unbind()).

Ah, I completely ignored the fact that we can indeed have other
references to the private data! The fact that `unbind` works with
`Device<Bounded>` should have given me a hint, but somehow I blissfully
ignored that. ^_^;

I will implement some basic locking on the command queue so we can work
with a non-mutable reference.

[PATCH 2/7] rust: add warn_on_err macro

[Alexandre Courbot]

While we already have the `warn_on` macro, a common usage patterns in
Rust is to check whether a `Result` is an error. Add a helper macro that
allows this.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 rust/kernel/bug.rs | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/rust/kernel/bug.rs b/rust/kernel/bug.rs
index 36aef43e5ebe..255c780bb4f7 100644
--- a/rust/kernel/bug.rs
+++ b/rust/kernel/bug.rs
@@ -124,3 +124,13 @@ macro_rules! warn_on {
         cond
     }};
 }
+
+/// Report a warning if `res` is an error and return it unmodified.
+#[macro_export]
+macro_rules! warn_on_err {
+    ($res:expr) => {{
+        let res = $res;
+        let _ = $crate::warn_on!(res.is_err());
+        res
+    }};
+}

-- 
2.52.0

[PATCH 3/7] gpu: nova-core: use warn_on_err macro

[Alexandre Courbot]

Use the warn_on_err macro in the unbind sequence, and refactor it to
early-exit during the failure path as we will need to use the bar for
other purposes.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpu/nova-core/gpu.rs | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 629c9d2dc994..e16d3c2a6b38 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -293,10 +293,10 @@ pub(crate) fn new<'a>(
     ///
     /// Note: This method must only be called from `Driver::unbind`.
     pub(crate) fn unbind(&self, dev: &device::Device<device::Core>) {
-        kernel::warn_on!(self
-            .bar
-            .access(dev)
-            .inspect(|bar| self.sysmem_flush.unregister(bar))
-            .is_err());
+        let Ok(bar) = kernel::warn_on_err!(self.bar.access(dev)) else {
+            return;
+        };
+
+        self.sysmem_flush.unregister(bar);
     }
 }

-- 
2.52.0

[PATCH RFC 4/7] rust: pin-init: allow `dead_code` on projection structure

[Alexandre Courbot]

Projection structures are not necessarily (and often not) used in their
entirety. At the moment partial uses result in warnings about the unused
members.

Discard them by allowing `dead_code` on the projection structure

To: Benno Lossin <lossin@kernel.org>
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

---
Benno, please let me know if this looks good to you and I will send you
a Github PR for this.
---
 rust/pin-init/src/macros.rs | 1 +
 1 file changed, 1 insertion(+)

diff --git a/rust/pin-init/src/macros.rs b/rust/pin-init/src/macros.rs
index 682c61a587a0..fe60e570c729 100644
--- a/rust/pin-init/src/macros.rs
+++ b/rust/pin-init/src/macros.rs
@@ -1004,6 +1004,7 @@ fn drop(&mut self) {
         @not_pinned($($(#[$($attr:tt)*])* $fvis:vis $field:ident : $type:ty),* $(,)?),
     ) => {
         $crate::macros::paste! {
+            #[allow(dead_code)]
             #[doc(hidden)]
             $vis struct [< $name Projection >] <'__pin, $($decl_generics)*> {
                 $($(#[$($p_attr)*])* $pvis $p_field : ::core::pin::Pin<&'__pin mut $p_type>,)*

-- 
2.52.0

Re: [PATCH RFC 4/7] rust: pin-init: allow `dead_code` on projection structure

[Benno Lossin]

On Tue Dec 16, 2025 at 6:13 AM CET, Alexandre Courbot wrote:
> Projection structures are not necessarily (and often not) used in their
> entirety. At the moment partial uses result in warnings about the unused
> members.
>
> Discard them by allowing `dead_code` on the projection structure
>
> To: Benno Lossin <lossin@kernel.org>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>
> ---
> Benno, please let me know if this looks good to you and I will send you
> a Github PR for this.

Looks good :)

Cheers,
Benno

> ---
>  rust/pin-init/src/macros.rs | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/rust/pin-init/src/macros.rs b/rust/pin-init/src/macros.rs
> index 682c61a587a0..fe60e570c729 100644
> --- a/rust/pin-init/src/macros.rs
> +++ b/rust/pin-init/src/macros.rs
> @@ -1004,6 +1004,7 @@ fn drop(&mut self) {
>          @not_pinned($($(#[$($attr:tt)*])* $fvis:vis $field:ident : $type:ty),* $(,)?),
>      ) => {
>          $crate::macros::paste! {
> +            #[allow(dead_code)]
>              #[doc(hidden)]
>              $vis struct [< $name Projection >] <'__pin, $($decl_generics)*> {
>                  $($(#[$($p_attr)*])* $pvis $p_field : ::core::pin::Pin<&'__pin mut $p_type>,)*

[PATCH 5/7] gpu: nova-nova: use pin-init projections

[Alexandre Courbot]

Unbinding happens on pinned data, but some of our methods still take
mutable references. Right now we can rely on the fact that `Gpu` is
`Unpin` to get a mutable reference to its members, but this is going to
change when e.g. mutexes are introduced.

Take advantage of the `project` method of pin-init to obtain
pin-projected data, and update our methods signature to work with pinned
data when relevant.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpu/nova-core/driver.rs | 4 ++--
 drivers/gpu/nova-core/gpu.rs    | 8 +++++---
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs
index b0e3e41ae9d8..c53e96173890 100644
--- a/drivers/gpu/nova-core/driver.rs
+++ b/drivers/gpu/nova-core/driver.rs
@@ -98,7 +98,7 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> impl PinInit<Self, E
         })
     }
 
-    fn unbind(pdev: &pci::Device<Core>, this: Pin<KBox<Self>>) {
-        this.gpu.unbind(pdev.as_ref());
+    fn unbind(pdev: &pci::Device<Core>, mut this: Pin<KBox<Self>>) {
+        this.as_mut().project().gpu.unbind(pdev.as_ref());
     }
 }
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index e16d3c2a6b38..ef6ceced350e 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -292,11 +292,13 @@ pub(crate) fn new<'a>(
     /// Called when the corresponding [`Device`](device::Device) is unbound.
     ///
     /// Note: This method must only be called from `Driver::unbind`.
-    pub(crate) fn unbind(&self, dev: &device::Device<device::Core>) {
-        let Ok(bar) = kernel::warn_on_err!(self.bar.access(dev)) else {
+    pub(crate) fn unbind(self: Pin<&mut Self>, dev: &device::Device<device::Core>) {
+        let this = self.project();
+
+        let Ok(bar) = kernel::warn_on_err!(this.bar.access(dev)) else {
             return;
         };
 
-        self.sysmem_flush.unregister(bar);
+        this.sysmem_flush.unregister(bar);
     }
 }

-- 
2.52.0

[PATCH 6/7] gpu: nova-core: send UNLOADING_GUEST_DRIVER GSP command GSP upon unloading

[Alexandre Courbot]

Currently, the GSP is left running after the driver is unbound. This is
not great for several reasons, notably that it can still access shared
memory areas that the kernel will now reclaim (especially problematic on
setups without an IOMMU).

Fix this by sending the `UNLOADING_GUEST_DRIVER` GSP command when
unbinding. This stops the GSP and let us proceed with the rest of the
unbind sequence in the next patch.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpu/nova-core/gpu.rs                      |  5 +++
 drivers/gpu/nova-core/gsp/boot.rs                 | 42 +++++++++++++++++++++++
 drivers/gpu/nova-core/gsp/commands.rs             | 42 +++++++++++++++++++++++
 drivers/gpu/nova-core/gsp/fw.rs                   |  4 +++
 drivers/gpu/nova-core/gsp/fw/commands.rs          | 27 +++++++++++++++
 drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs |  8 +++++
 6 files changed, 128 insertions(+)

diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index ef6ceced350e..b94784f57b36 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -291,6 +291,9 @@ pub(crate) fn new<'a>(
 
     /// Called when the corresponding [`Device`](device::Device) is unbound.
     ///
+    /// Prepares the GPU for unbinding by shutting down the GSP and unregistering the sysmem flush
+    /// memory page.
+    ///
     /// Note: This method must only be called from `Driver::unbind`.
     pub(crate) fn unbind(self: Pin<&mut Self>, dev: &device::Device<device::Core>) {
         let this = self.project();
@@ -299,6 +302,8 @@ pub(crate) fn unbind(self: Pin<&mut Self>, dev: &device::Device<device::Core>) {
             return;
         };
 
+        let _ = kernel::warn_on_err!(this.gsp.unload(dev, bar, this.gsp_falcon,));
+
         this.sysmem_flush.unregister(bar);
     }
 }
diff --git a/drivers/gpu/nova-core/gsp/boot.rs b/drivers/gpu/nova-core/gsp/boot.rs
index ca7efdaa753b..e12e1d3fd53f 100644
--- a/drivers/gpu/nova-core/gsp/boot.rs
+++ b/drivers/gpu/nova-core/gsp/boot.rs
@@ -32,6 +32,7 @@
     },
     gpu::Chipset,
     gsp::{
+        cmdq::Cmdq,
         commands,
         sequencer::{
             GspSequencer,
@@ -231,4 +232,45 @@ pub(crate) fn boot(
 
         Ok(())
     }
+
+    /// Shutdowns the GSP and wait until it is offline.
+    fn shutdown_gsp(
+        cmdq: &mut Cmdq,
+        bar: &Bar0,
+        gsp_falcon: &Falcon<Gsp>,
+        suspend: bool,
+    ) -> Result<()> {
+        // Send command to shutdown GSP and wait for response.
+        commands::unload_guest_driver(cmdq, bar, suspend)?;
+
+        // Wait until GSP signals it is suspended.
+        const LIBOS_INTERRUPT_PROCESSOR_SUSPENDED: u32 = 0x8000_0000;
+        read_poll_timeout(
+            || Ok(gsp_falcon.read_mailbox0(bar)),
+            |&mb0| mb0 == LIBOS_INTERRUPT_PROCESSOR_SUSPENDED,
+            Delta::ZERO,
+            Delta::from_secs(5),
+        )
+        .map(|_| ())
+    }
+
+    /// Attempts to unload the GSP firmware.
+    ///
+    /// This stops all activity on the GSP.
+    pub(crate) fn unload(
+        self: Pin<&mut Self>,
+        dev: &device::Device<device::Bound>,
+        bar: &Bar0,
+        gsp_falcon: &Falcon<Gsp>,
+    ) -> Result {
+        let this = self.project();
+
+        /* Shutdown the GSP */
+
+        Self::shutdown_gsp(this.cmdq, bar, gsp_falcon, false)
+            .inspect_err(|e| dev_err!(dev, "unload guest driver failed: {:?}", e))?;
+        dev_dbg!(dev, "GSP shut down\n");
+
+        Ok(())
+    }
 }
diff --git a/drivers/gpu/nova-core/gsp/commands.rs b/drivers/gpu/nova-core/gsp/commands.rs
index 2050771f9b53..0bcfca8c7e42 100644
--- a/drivers/gpu/nova-core/gsp/commands.rs
+++ b/drivers/gpu/nova-core/gsp/commands.rs
@@ -225,3 +225,45 @@ pub(crate) fn get_gsp_info(cmdq: &mut Cmdq, bar: &Bar0) -> Result<GetGspStaticIn
         }
     }
 }
+
+impl CommandToGsp for UnloadingGuestDriver {
+    const FUNCTION: MsgFunction = MsgFunction::UnloadingGuestDriver;
+    type Command = Self;
+    type InitError = Infallible;
+
+    fn init(&self) -> impl Init<Self::Command, Self::InitError> {
+        *self
+    }
+}
+
+pub(crate) struct UnloadingGuestDriverReply;
+
+impl MessageFromGsp for UnloadingGuestDriverReply {
+    const FUNCTION: MsgFunction = MsgFunction::UnloadingGuestDriver;
+    type InitError = Infallible;
+    type Message = ();
+
+    fn read(
+        _msg: &Self::Message,
+        _sbuffer: &mut SBufferIter<array::IntoIter<&[u8], 2>>,
+    ) -> Result<Self, Self::InitError> {
+        Ok(UnloadingGuestDriverReply)
+    }
+}
+
+/// Send the [`UnloadingGuestDriver`] command to the GSP and await the reply.
+pub(crate) fn unload_guest_driver(
+    cmdq: &mut Cmdq,
+    bar: &Bar0,
+    suspend: bool,
+) -> Result<UnloadingGuestDriverReply> {
+    cmdq.send_command(bar, UnloadingGuestDriver::new(suspend))?;
+
+    loop {
+        match cmdq.receive_msg::<UnloadingGuestDriverReply>(Delta::from_secs(5)) {
+            Ok(resp) => return Ok(resp),
+            Err(ERANGE) => continue,
+            Err(e) => return Err(e),
+        }
+    }
+}
diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
index 09549f7db52d..228464fd47a0 100644
--- a/drivers/gpu/nova-core/gsp/fw.rs
+++ b/drivers/gpu/nova-core/gsp/fw.rs
@@ -209,6 +209,7 @@ pub(crate) enum MsgFunction {
     GspInitPostObjGpu = bindings::NV_VGPU_MSG_FUNCTION_GSP_INIT_POST_OBJGPU,
     GspRmControl = bindings::NV_VGPU_MSG_FUNCTION_GSP_RM_CONTROL,
     GetStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO,
+    UnloadingGuestDriver = bindings::NV_VGPU_MSG_FUNCTION_UNLOADING_GUEST_DRIVER,
 
     // Event codes
     GspInitDone = bindings::NV_VGPU_MSG_EVENT_GSP_INIT_DONE,
@@ -249,6 +250,9 @@ fn try_from(value: u32) -> Result<MsgFunction> {
             }
             bindings::NV_VGPU_MSG_FUNCTION_GSP_RM_CONTROL => Ok(MsgFunction::GspRmControl),
             bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO => Ok(MsgFunction::GetStaticInfo),
+            bindings::NV_VGPU_MSG_FUNCTION_UNLOADING_GUEST_DRIVER => {
+                Ok(MsgFunction::UnloadingGuestDriver)
+            }
             bindings::NV_VGPU_MSG_EVENT_GSP_INIT_DONE => Ok(MsgFunction::GspInitDone),
             bindings::NV_VGPU_MSG_EVENT_GSP_RUN_CPU_SEQUENCER => {
                 Ok(MsgFunction::GspRunCpuSequencer)
diff --git a/drivers/gpu/nova-core/gsp/fw/commands.rs b/drivers/gpu/nova-core/gsp/fw/commands.rs
index 85465521de32..c7df4783ad21 100644
--- a/drivers/gpu/nova-core/gsp/fw/commands.rs
+++ b/drivers/gpu/nova-core/gsp/fw/commands.rs
@@ -125,3 +125,30 @@ unsafe impl AsBytes for GspStaticConfigInfo {}
 // SAFETY: This struct only contains integer types for which all bit patterns
 // are valid.
 unsafe impl FromBytes for GspStaticConfigInfo {}
+
+/// Payload of the `UnloadingGuestDriver` command and message.
+#[repr(transparent)]
+#[derive(Clone, Copy, Debug, Zeroable)]
+pub(crate) struct UnloadingGuestDriver {
+    inner: bindings::rpc_unloading_guest_driver_v1F_07,
+}
+
+impl UnloadingGuestDriver {
+    pub(crate) fn new(suspend: bool) -> Self {
+        Self {
+            inner: bindings::rpc_unloading_guest_driver_v1F_07 {
+                bInPMTransition: if suspend { 1 } else { 0 },
+                bGc6Entering: 0,
+                newLevel: if suspend { 3 } else { 0 },
+                ..Zeroable::zeroed()
+            },
+        }
+    }
+}
+
+// SAFETY: Padding is explicit and will not contain uninitialized data.
+unsafe impl AsBytes for UnloadingGuestDriver {}
+
+// SAFETY: This struct only contains integer types for which all bit patterns
+// are valid.
+unsafe impl FromBytes for UnloadingGuestDriver {}
diff --git a/drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs b/drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs
index 6d25fe0bffa9..212ccc13c0cc 100644
--- a/drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs
+++ b/drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs
@@ -879,6 +879,14 @@ fn default() -> Self {
     }
 }
 #[repr(C)]
+#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
+pub struct rpc_unloading_guest_driver_v1F_07 {
+    pub bInPMTransition: u8_,
+    pub bGc6Entering: u8_,
+    pub __bindgen_padding_0: [u8; 2usize],
+    pub newLevel: u32_,
+}
+#[repr(C)]
 #[derive(Debug, Default, MaybeZeroable)]
 pub struct rpc_run_cpu_sequencer_v17_00 {
     pub bufferSizeDWord: u32_,

-- 
2.52.0

Re: [PATCH 6/7] gpu: nova-core: send UNLOADING_GUEST_DRIVER GSP command GSP upon unloading

[Joel Fernandes]

Hi Alex,

Just did a quick pass, will do a proper review later:

> On Dec 16, 2025, at 12:13 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> 
> Currently, the GSP is left running after the driver is unbound. This is
> not great for several reasons, notably that it can still access shared
> memory areas that the kernel will now reclaim (especially problematic on
> setups without an IOMMU).
> 
> Fix this by sending the `UNLOADING_GUEST_DRIVER` GSP command when
> unbinding. This stops the GSP and let us proceed with the rest of the
> unbind sequence in the next patch.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> drivers/gpu/nova-core/gpu.rs                      |  5 +++
> drivers/gpu/nova-core/gsp/boot.rs                 | 42 +++++++++++++++++++++++
> drivers/gpu/nova-core/gsp/commands.rs             | 42 +++++++++++++++++++++++
> drivers/gpu/nova-core/gsp/fw.rs                   |  4 +++
> drivers/gpu/nova-core/gsp/fw/commands.rs          | 27 +++++++++++++++
> drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs |  8 +++++
> 6 files changed, 128 insertions(+)
> 
> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
> index ef6ceced350e..b94784f57b36 100644
> --- a/drivers/gpu/nova-core/gpu.rs
> +++ b/drivers/gpu/nova-core/gpu.rs
> @@ -291,6 +291,9 @@ pub(crate) fn new<'a>(
> 
>     /// Called when the corresponding [`Device`](device::Device) is unbound.
>     ///
> +    /// Prepares the GPU for unbinding by shutting down the GSP and unregistering the sysmem flush
> +    /// memory page.
> +    ///
>     /// Note: This method must only be called from `Driver::unbind`.
>     pub(crate) fn unbind(self: Pin<&mut Self>, dev: &device::Device<device::Core>) {
>         let this = self.project();
> @@ -299,6 +302,8 @@ pub(crate) fn unbind(self: Pin<&mut Self>, dev: &device::Device<device::Core>) {
>             return;
>         };
> 
> +        let _ = kernel::warn_on_err!(this.gsp.unload(dev, bar, this.gsp_falcon,));
> +
>         this.sysmem_flush.unregister(bar);
>     }
> }
> diff --git a/drivers/gpu/nova-core/gsp/boot.rs b/drivers/gpu/nova-core/gsp/boot.rs
> index ca7efdaa753b..e12e1d3fd53f 100644
> --- a/drivers/gpu/nova-core/gsp/boot.rs
> +++ b/drivers/gpu/nova-core/gsp/boot.rs
> @@ -32,6 +32,7 @@
>     },
>     gpu::Chipset,
>     gsp::{
> +        cmdq::Cmdq,
>         commands,
>         sequencer::{
>             GspSequencer,
> @@ -231,4 +232,45 @@ pub(crate) fn boot(
> 
>         Ok(())
>     }
> +
> +    /// Shutdowns the GSP and wait until it is offline.
> +    fn shutdown_gsp(
> +        cmdq: &mut Cmdq,
> +        bar: &Bar0,
> +        gsp_falcon: &Falcon<Gsp>,
> +        suspend: bool,
> +    ) -> Result<()> {
> +        // Send command to shutdown GSP and wait for response.
> +        commands::unload_guest_driver(cmdq, bar, suspend)?;
> +
> +        // Wait until GSP signals it is suspended.
> +        const LIBOS_INTERRUPT_PROCESSOR_SUSPENDED: u32 = 0x8000_0000;
> +        read_poll_timeout(
> +            || Ok(gsp_falcon.read_mailbox0(bar)),
> +            |&mb0| mb0 == LIBOS_INTERRUPT_PROCESSOR_SUSPENDED,
> +            Delta::ZERO,
> +            Delta::from_secs(5),
> +        )
> +        .map(|_| ())
> +    }
> +
> +    /// Attempts to unload the GSP firmware.
> +    ///
> +    /// This stops all activity on the GSP.
> +    pub(crate) fn unload(
> +        self: Pin<&mut Self>,
> +        dev: &device::Device<device::Bound>,
> +        bar: &Bar0,
> +        gsp_falcon: &Falcon<Gsp>,
> +    ) -> Result {
> +        let this = self.project();
> +
> +        /* Shutdown the GSP */
> +
> +        Self::shutdown_gsp(this.cmdq, bar, gsp_falcon, false)
> +            .inspect_err(|e| dev_err!(dev, "unload guest driver failed: {:?}", e))?;
> +        dev_dbg!(dev, "GSP shut down\n");
> +
> +        Ok(())
> +    }
> }
> diff --git a/drivers/gpu/nova-core/gsp/commands.rs b/drivers/gpu/nova-core/gsp/commands.rs
> index 2050771f9b53..0bcfca8c7e42 100644
> --- a/drivers/gpu/nova-core/gsp/commands.rs
> +++ b/drivers/gpu/nova-core/gsp/commands.rs
> @@ -225,3 +225,45 @@ pub(crate) fn get_gsp_info(cmdq: &mut Cmdq, bar: &Bar0) -> Result<GetGspStaticIn
>         }
>     }
> }
> +
> +impl CommandToGsp for UnloadingGuestDriver {
> +    const FUNCTION: MsgFunction = MsgFunction::UnloadingGuestDriver;
> +    type Command = Self;
> +    type InitError = Infallible;
> +
> +    fn init(&self) -> impl Init<Self::Command, Self::InitError> {
> +        *self
> +    }
> +}
> +
> +pub(crate) struct UnloadingGuestDriverReply;
> +
> +impl MessageFromGsp for UnloadingGuestDriverReply {
> +    const FUNCTION: MsgFunction = MsgFunction::UnloadingGuestDriver;
> +    type InitError = Infallible;
> +    type Message = ();
> +
> +    fn read(
> +        _msg: &Self::Message,
> +        _sbuffer: &mut SBufferIter<array::IntoIter<&[u8], 2>>,
> +    ) -> Result<Self, Self::InitError> {
> +        Ok(UnloadingGuestDriverReply)
> +    }
> +}
> +
> +/// Send the [`UnloadingGuestDriver`] command to the GSP and await the reply.
> +pub(crate) fn unload_guest_driver(
> +    cmdq: &mut Cmdq,
> +    bar: &Bar0,
> +    suspend: bool,
> +) -> Result<UnloadingGuestDriverReply> {
> +    cmdq.send_command(bar, UnloadingGuestDriver::new(suspend))?;
> +
> +    loop {
> +        match cmdq.receive_msg::<UnloadingGuestDriverReply>(Delta::from_secs(5)) {
> +            Ok(resp) => return Ok(resp),
> +            Err(ERANGE) => continue,
> +            Err(e) => return Err(e),
> +        }

This outer loop can go on infinitely, lets bound it?

Either outer timeout or bounded number of tries. Bounded tries better since it has inner timeout.

> +    }
> +}
> diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
> index 09549f7db52d..228464fd47a0 100644
> --- a/drivers/gpu/nova-core/gsp/fw.rs
> +++ b/drivers/gpu/nova-core/gsp/fw.rs
> @@ -209,6 +209,7 @@ pub(crate) enum MsgFunction {
>     GspInitPostObjGpu = bindings::NV_VGPU_MSG_FUNCTION_GSP_INIT_POST_OBJGPU,
>     GspRmControl = bindings::NV_VGPU_MSG_FUNCTION_GSP_RM_CONTROL,
>     GetStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO,
> +    UnloadingGuestDriver = bindings::NV_VGPU_MSG_FUNCTION_UNLOADING_GUEST_DRIVER,
> 
>     // Event codes
>     GspInitDone = bindings::NV_VGPU_MSG_EVENT_GSP_INIT_DONE,
> @@ -249,6 +250,9 @@ fn try_from(value: u32) -> Result<MsgFunction> {
>             }
>             bindings::NV_VGPU_MSG_FUNCTION_GSP_RM_CONTROL => Ok(MsgFunction::GspRmControl),
>             bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO => Ok(MsgFunction::GetStaticInfo),
> +            bindings::NV_VGPU_MSG_FUNCTION_UNLOADING_GUEST_DRIVER => {
> +                Ok(MsgFunction::UnloadingGuestDriver)
> +            }
>             bindings::NV_VGPU_MSG_EVENT_GSP_INIT_DONE => Ok(MsgFunction::GspInitDone),
>             bindings::NV_VGPU_MSG_EVENT_GSP_RUN_CPU_SEQUENCER => {
>                 Ok(MsgFunction::GspRunCpuSequencer)
> diff --git a/drivers/gpu/nova-core/gsp/fw/commands.rs b/drivers/gpu/nova-core/gsp/fw/commands.rs
> index 85465521de32..c7df4783ad21 100644
> --- a/drivers/gpu/nova-core/gsp/fw/commands.rs
> +++ b/drivers/gpu/nova-core/gsp/fw/commands.rs
> @@ -125,3 +125,30 @@ unsafe impl AsBytes for GspStaticConfigInfo {}
> // SAFETY: This struct only contains integer types for which all bit patterns
> // are valid.
> unsafe impl FromBytes for GspStaticConfigInfo {}
> +
> +/// Payload of the `UnloadingGuestDriver` command and message.
> +#[repr(transparent)]
> +#[derive(Clone, Copy, Debug, Zeroable)]
> +pub(crate) struct UnloadingGuestDriver {
> +    inner: bindings::rpc_unloading_guest_driver_v1F_07,
> +}
> +
> +impl UnloadingGuestDriver {
> +    pub(crate) fn new(suspend: bool) -> Self {
> +        Self {
> +            inner: bindings::rpc_unloading_guest_driver_v1F_07 {

We should go through intermediate firmware representation than direct bindings access?


> +                bInPMTransition: if suspend { 1 } else { 0 },

Then this can just be passed as a bool.

> +                bGc6Entering: 0,
> +                newLevel: if suspend { 3 } else { 0 },
> +                ..Zeroable::zeroed()
> +            },
> +        }
> +    }
> +}
> +
> +// SAFETY: Padding is explicit and will not contain uninitialized data.
> +unsafe impl AsBytes for UnloadingGuestDriver {}
> +
> +// SAFETY: This struct only contains integer types for which all bit patterns
> +// are valid.
> +unsafe impl FromBytes for UnloadingGuestDriver {}
> diff --git a/drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs b/drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs
> index 6d25fe0bffa9..212ccc13c0cc 100644
> --- a/drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs
> +++ b/drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs
> @@ -879,6 +879,14 @@ fn default() -> Self {
>     }
> }
> #[repr(C)]
> +#[derive(Debug, Default, Copy, Clone, MaybeZeroable)]
> +pub struct rpc_unloading_guest_driver_v1F_07 {
> +    pub bInPMTransition: u8_,
> +    pub bGc6Entering: u8_,
> +    pub __bindgen_padding_0: [u8; 2usize],
> +    pub newLevel: u32_,

When these are intermediate represented, documentation would be nice on the fields.

thanks,

 - Joel

> +}
> +#[repr(C)]
> #[derive(Debug, Default, MaybeZeroable)]
> pub struct rpc_run_cpu_sequencer_v17_00 {
>     pub bufferSizeDWord: u32_,
> 
> --
> 2.52.0
> 

Re: [PATCH 6/7] gpu: nova-core: send UNLOADING_GUEST_DRIVER GSP command GSP upon unloading

[Alexandre Courbot]

On Wed Dec 17, 2025 at 12:39 AM JST, Joel Fernandes wrote:
> Hi Alex,
>
> Just did a quick pass, will do a proper review later:
>
>> On Dec 16, 2025, at 12:13 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
>> 
>> Currently, the GSP is left running after the driver is unbound. This is
>> not great for several reasons, notably that it can still access shared
>> memory areas that the kernel will now reclaim (especially problematic on
>> setups without an IOMMU).
>> 
>> Fix this by sending the `UNLOADING_GUEST_DRIVER` GSP command when
>> unbinding. This stops the GSP and let us proceed with the rest of the
>> unbind sequence in the next patch.
>> 
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>> drivers/gpu/nova-core/gpu.rs                      |  5 +++
>> drivers/gpu/nova-core/gsp/boot.rs                 | 42 +++++++++++++++++++++++
>> drivers/gpu/nova-core/gsp/commands.rs             | 42 +++++++++++++++++++++++
>> drivers/gpu/nova-core/gsp/fw.rs                   |  4 +++
>> drivers/gpu/nova-core/gsp/fw/commands.rs          | 27 +++++++++++++++
>> drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs |  8 +++++
>> 6 files changed, 128 insertions(+)
>> 
>> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
>> index ef6ceced350e..b94784f57b36 100644
>> --- a/drivers/gpu/nova-core/gpu.rs
>> +++ b/drivers/gpu/nova-core/gpu.rs
>> @@ -291,6 +291,9 @@ pub(crate) fn new<'a>(
>> 
>>     /// Called when the corresponding [`Device`](device::Device) is unbound.
>>     ///
>> +    /// Prepares the GPU for unbinding by shutting down the GSP and unregistering the sysmem flush
>> +    /// memory page.
>> +    ///
>>     /// Note: This method must only be called from `Driver::unbind`.
>>     pub(crate) fn unbind(self: Pin<&mut Self>, dev: &device::Device<device::Core>) {
>>         let this = self.project();
>> @@ -299,6 +302,8 @@ pub(crate) fn unbind(self: Pin<&mut Self>, dev: &device::Device<device::Core>) {
>>             return;
>>         };
>> 
>> +        let _ = kernel::warn_on_err!(this.gsp.unload(dev, bar, this.gsp_falcon,));
>> +
>>         this.sysmem_flush.unregister(bar);
>>     }
>> }
>> diff --git a/drivers/gpu/nova-core/gsp/boot.rs b/drivers/gpu/nova-core/gsp/boot.rs
>> index ca7efdaa753b..e12e1d3fd53f 100644
>> --- a/drivers/gpu/nova-core/gsp/boot.rs
>> +++ b/drivers/gpu/nova-core/gsp/boot.rs
>> @@ -32,6 +32,7 @@
>>     },
>>     gpu::Chipset,
>>     gsp::{
>> +        cmdq::Cmdq,
>>         commands,
>>         sequencer::{
>>             GspSequencer,
>> @@ -231,4 +232,45 @@ pub(crate) fn boot(
>> 
>>         Ok(())
>>     }
>> +
>> +    /// Shutdowns the GSP and wait until it is offline.
>> +    fn shutdown_gsp(
>> +        cmdq: &mut Cmdq,
>> +        bar: &Bar0,
>> +        gsp_falcon: &Falcon<Gsp>,
>> +        suspend: bool,
>> +    ) -> Result<()> {
>> +        // Send command to shutdown GSP and wait for response.
>> +        commands::unload_guest_driver(cmdq, bar, suspend)?;
>> +
>> +        // Wait until GSP signals it is suspended.
>> +        const LIBOS_INTERRUPT_PROCESSOR_SUSPENDED: u32 = 0x8000_0000;
>> +        read_poll_timeout(
>> +            || Ok(gsp_falcon.read_mailbox0(bar)),
>> +            |&mb0| mb0 == LIBOS_INTERRUPT_PROCESSOR_SUSPENDED,
>> +            Delta::ZERO,
>> +            Delta::from_secs(5),
>> +        )
>> +        .map(|_| ())
>> +    }
>> +
>> +    /// Attempts to unload the GSP firmware.
>> +    ///
>> +    /// This stops all activity on the GSP.
>> +    pub(crate) fn unload(
>> +        self: Pin<&mut Self>,
>> +        dev: &device::Device<device::Bound>,
>> +        bar: &Bar0,
>> +        gsp_falcon: &Falcon<Gsp>,
>> +    ) -> Result {
>> +        let this = self.project();
>> +
>> +        /* Shutdown the GSP */
>> +
>> +        Self::shutdown_gsp(this.cmdq, bar, gsp_falcon, false)
>> +            .inspect_err(|e| dev_err!(dev, "unload guest driver failed: {:?}", e))?;
>> +        dev_dbg!(dev, "GSP shut down\n");
>> +
>> +        Ok(())
>> +    }
>> }
>> diff --git a/drivers/gpu/nova-core/gsp/commands.rs b/drivers/gpu/nova-core/gsp/commands.rs
>> index 2050771f9b53..0bcfca8c7e42 100644
>> --- a/drivers/gpu/nova-core/gsp/commands.rs
>> +++ b/drivers/gpu/nova-core/gsp/commands.rs
>> @@ -225,3 +225,45 @@ pub(crate) fn get_gsp_info(cmdq: &mut Cmdq, bar: &Bar0) -> Result<GetGspStaticIn
>>         }
>>     }
>> }
>> +
>> +impl CommandToGsp for UnloadingGuestDriver {
>> +    const FUNCTION: MsgFunction = MsgFunction::UnloadingGuestDriver;
>> +    type Command = Self;
>> +    type InitError = Infallible;
>> +
>> +    fn init(&self) -> impl Init<Self::Command, Self::InitError> {
>> +        *self
>> +    }
>> +}
>> +
>> +pub(crate) struct UnloadingGuestDriverReply;
>> +
>> +impl MessageFromGsp for UnloadingGuestDriverReply {
>> +    const FUNCTION: MsgFunction = MsgFunction::UnloadingGuestDriver;
>> +    type InitError = Infallible;
>> +    type Message = ();
>> +
>> +    fn read(
>> +        _msg: &Self::Message,
>> +        _sbuffer: &mut SBufferIter<array::IntoIter<&[u8], 2>>,
>> +    ) -> Result<Self, Self::InitError> {
>> +        Ok(UnloadingGuestDriverReply)
>> +    }
>> +}
>> +
>> +/// Send the [`UnloadingGuestDriver`] command to the GSP and await the reply.
>> +pub(crate) fn unload_guest_driver(
>> +    cmdq: &mut Cmdq,
>> +    bar: &Bar0,
>> +    suspend: bool,
>> +) -> Result<UnloadingGuestDriverReply> {
>> +    cmdq.send_command(bar, UnloadingGuestDriver::new(suspend))?;
>> +
>> +    loop {
>> +        match cmdq.receive_msg::<UnloadingGuestDriverReply>(Delta::from_secs(5)) {
>> +            Ok(resp) => return Ok(resp),
>> +            Err(ERANGE) => continue,
>> +            Err(e) => return Err(e),
>> +        }
>
> This outer loop can go on infinitely, lets bound it?
>
> Either outer timeout or bounded number of tries. Bounded tries better since it has inner timeout.

Yes. And what we really want is a more generic way to do this, because
this pattern occurs with several commands so far (and more to come).

>
>> +    }
>> +}
>> diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
>> index 09549f7db52d..228464fd47a0 100644
>> --- a/drivers/gpu/nova-core/gsp/fw.rs
>> +++ b/drivers/gpu/nova-core/gsp/fw.rs
>> @@ -209,6 +209,7 @@ pub(crate) enum MsgFunction {
>>     GspInitPostObjGpu = bindings::NV_VGPU_MSG_FUNCTION_GSP_INIT_POST_OBJGPU,
>>     GspRmControl = bindings::NV_VGPU_MSG_FUNCTION_GSP_RM_CONTROL,
>>     GetStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO,
>> +    UnloadingGuestDriver = bindings::NV_VGPU_MSG_FUNCTION_UNLOADING_GUEST_DRIVER,
>> 
>>     // Event codes
>>     GspInitDone = bindings::NV_VGPU_MSG_EVENT_GSP_INIT_DONE,
>> @@ -249,6 +250,9 @@ fn try_from(value: u32) -> Result<MsgFunction> {
>>             }
>>             bindings::NV_VGPU_MSG_FUNCTION_GSP_RM_CONTROL => Ok(MsgFunction::GspRmControl),
>>             bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO => Ok(MsgFunction::GetStaticInfo),
>> +            bindings::NV_VGPU_MSG_FUNCTION_UNLOADING_GUEST_DRIVER => {
>> +                Ok(MsgFunction::UnloadingGuestDriver)
>> +            }
>>             bindings::NV_VGPU_MSG_EVENT_GSP_INIT_DONE => Ok(MsgFunction::GspInitDone),
>>             bindings::NV_VGPU_MSG_EVENT_GSP_RUN_CPU_SEQUENCER => {
>>                 Ok(MsgFunction::GspRunCpuSequencer)
>> diff --git a/drivers/gpu/nova-core/gsp/fw/commands.rs b/drivers/gpu/nova-core/gsp/fw/commands.rs
>> index 85465521de32..c7df4783ad21 100644
>> --- a/drivers/gpu/nova-core/gsp/fw/commands.rs
>> +++ b/drivers/gpu/nova-core/gsp/fw/commands.rs
>> @@ -125,3 +125,30 @@ unsafe impl AsBytes for GspStaticConfigInfo {}
>> // SAFETY: This struct only contains integer types for which all bit patterns
>> // are valid.
>> unsafe impl FromBytes for GspStaticConfigInfo {}
>> +
>> +/// Payload of the `UnloadingGuestDriver` command and message.
>> +#[repr(transparent)]
>> +#[derive(Clone, Copy, Debug, Zeroable)]
>> +pub(crate) struct UnloadingGuestDriver {
>> +    inner: bindings::rpc_unloading_guest_driver_v1F_07,
>> +}
>> +
>> +impl UnloadingGuestDriver {
>> +    pub(crate) fn new(suspend: bool) -> Self {
>> +        Self {
>> +            inner: bindings::rpc_unloading_guest_driver_v1F_07 {
>
> We should go through intermediate firmware representation than direct bindings access?

Only if the size of the bindings justifies it - here having an opaque
wrapper just just well, and spares us some code down the line as we
would have to initialize the bindings anyway.

>
>
>> +                bInPMTransition: if suspend { 1 } else { 0 },
>
> Then this can just be passed as a bool.
>
>> +                bGc6Entering: 0,
>> +                newLevel: if suspend { 3 } else { 0 },

Note to self to figure out these magic numbers. :)

Re: [PATCH 6/7] gpu: nova-core: send UNLOADING_GUEST_DRIVER GSP command GSP upon unloading

[Joel Fernandes]

Hi Alex,

>>> +    }
>>> +}
>>> diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
>>> index 09549f7db52d..228464fd47a0 100644
>>> --- a/drivers/gpu/nova-core/gsp/fw.rs
>>> +++ b/drivers/gpu/nova-core/gsp/fw.rs
>>> @@ -209,6 +209,7 @@ pub(crate) enum MsgFunction {
>>>    GspInitPostObjGpu = bindings::NV_VGPU_MSG_FUNCTION_GSP_INIT_POST_OBJGPU,
>>>    GspRmControl = bindings::NV_VGPU_MSG_FUNCTION_GSP_RM_CONTROL,
>>>    GetStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO,
>>> +    UnloadingGuestDriver = bindings::NV_VGPU_MSG_FUNCTION_UNLOADING_GUEST_DRIVER,
>>> 
>>>    // Event codes
>>>    GspInitDone = bindings::NV_VGPU_MSG_EVENT_GSP_INIT_DONE,
>>> @@ -249,6 +250,9 @@ fn try_from(value: u32) -> Result<MsgFunction> {
>>>            }
>>>            bindings::NV_VGPU_MSG_FUNCTION_GSP_RM_CONTROL => Ok(MsgFunction::GspRmControl),
>>>            bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO => Ok(MsgFunction::GetStaticInfo),
>>> +            bindings::NV_VGPU_MSG_FUNCTION_UNLOADING_GUEST_DRIVER => {
>>> +                Ok(MsgFunction::UnloadingGuestDriver)
>>> +            }
>>>            bindings::NV_VGPU_MSG_EVENT_GSP_INIT_DONE => Ok(MsgFunction::GspInitDone),
>>>            bindings::NV_VGPU_MSG_EVENT_GSP_RUN_CPU_SEQUENCER => {
>>>                Ok(MsgFunction::GspRunCpuSequencer)
>>> diff --git a/drivers/gpu/nova-core/gsp/fw/commands.rs b/drivers/gpu/nova-core/gsp/fw/commands.rs
>>> index 85465521de32..c7df4783ad21 100644
>>> --- a/drivers/gpu/nova-core/gsp/fw/commands.rs
>>> +++ b/drivers/gpu/nova-core/gsp/fw/commands.rs
>>> @@ -125,3 +125,30 @@ unsafe impl AsBytes for GspStaticConfigInfo {}
>>> // SAFETY: This struct only contains integer types for which all bit patterns
>>> // are valid.
>>> unsafe impl FromBytes for GspStaticConfigInfo {}
>>> +
>>> +/// Payload of the `UnloadingGuestDriver` command and message.
>>> +#[repr(transparent)]
>>> +#[derive(Clone, Copy, Debug, Zeroable)]
>>> +pub(crate) struct UnloadingGuestDriver {
>>> +    inner: bindings::rpc_unloading_guest_driver_v1F_07,
>>> +}
>>> +
>>> +impl UnloadingGuestDriver {
>>> +    pub(crate) fn new(suspend: bool) -> Self {
>>> +        Self {
>>> +            inner: bindings::rpc_unloading_guest_driver_v1F_07 {
>> 
>> We should go through intermediate firmware representation than direct bindings access?
> 
> Only if the size of the bindings justifies it - here having an opaque
> wrapper just just well, and spares us some code down the line as we
> would have to initialize the bindings anyway.

I am not sure about that, it sounds like a layering violation. It would be good not to keep the rules fuzzy about this, because then we could do it either way in all cases.

Another reason is that we cannot anticipate in advance which specific helper functions we will need to add in the future. Down the line, we may need to add some helper functions to the struct as well.  Also having V1F07 in the name sounds very magic number-ish. It would be good to abstract that out with a better-named struct anyway.

Thanks,

- Joel



> 
>> 
>> 
>>> +                bInPMTransition: if suspend { 1 } else { 0 },
>> 
>> Then this can just be passed as a bool.
>> 
>>> +                bGc6Entering: 0,
>>> +                newLevel: if suspend { 3 } else { 0 },
> 
> Note to self to figure out these magic numbers. :)
> 

Re: [PATCH 6/7] gpu: nova-core: send UNLOADING_GUEST_DRIVER GSP command GSP upon unloading

[Alexandre Courbot]

On Fri Dec 19, 2025 at 5:52 AM JST, Joel Fernandes wrote:
> Hi Alex,
>
>>>> +    }
>>>> +}
>>>> diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
>>>> index 09549f7db52d..228464fd47a0 100644
>>>> --- a/drivers/gpu/nova-core/gsp/fw.rs
>>>> +++ b/drivers/gpu/nova-core/gsp/fw.rs
>>>> @@ -209,6 +209,7 @@ pub(crate) enum MsgFunction {
>>>>    GspInitPostObjGpu = bindings::NV_VGPU_MSG_FUNCTION_GSP_INIT_POST_OBJGPU,
>>>>    GspRmControl = bindings::NV_VGPU_MSG_FUNCTION_GSP_RM_CONTROL,
>>>>    GetStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO,
>>>> +    UnloadingGuestDriver = bindings::NV_VGPU_MSG_FUNCTION_UNLOADING_GUEST_DRIVER,
>>>> 
>>>>    // Event codes
>>>>    GspInitDone = bindings::NV_VGPU_MSG_EVENT_GSP_INIT_DONE,
>>>> @@ -249,6 +250,9 @@ fn try_from(value: u32) -> Result<MsgFunction> {
>>>>            }
>>>>            bindings::NV_VGPU_MSG_FUNCTION_GSP_RM_CONTROL => Ok(MsgFunction::GspRmControl),
>>>>            bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO => Ok(MsgFunction::GetStaticInfo),
>>>> +            bindings::NV_VGPU_MSG_FUNCTION_UNLOADING_GUEST_DRIVER => {
>>>> +                Ok(MsgFunction::UnloadingGuestDriver)
>>>> +            }
>>>>            bindings::NV_VGPU_MSG_EVENT_GSP_INIT_DONE => Ok(MsgFunction::GspInitDone),
>>>>            bindings::NV_VGPU_MSG_EVENT_GSP_RUN_CPU_SEQUENCER => {
>>>>                Ok(MsgFunction::GspRunCpuSequencer)
>>>> diff --git a/drivers/gpu/nova-core/gsp/fw/commands.rs b/drivers/gpu/nova-core/gsp/fw/commands.rs
>>>> index 85465521de32..c7df4783ad21 100644
>>>> --- a/drivers/gpu/nova-core/gsp/fw/commands.rs
>>>> +++ b/drivers/gpu/nova-core/gsp/fw/commands.rs
>>>> @@ -125,3 +125,30 @@ unsafe impl AsBytes for GspStaticConfigInfo {}
>>>> // SAFETY: This struct only contains integer types for which all bit patterns
>>>> // are valid.
>>>> unsafe impl FromBytes for GspStaticConfigInfo {}
>>>> +
>>>> +/// Payload of the `UnloadingGuestDriver` command and message.
>>>> +#[repr(transparent)]
>>>> +#[derive(Clone, Copy, Debug, Zeroable)]
>>>> +pub(crate) struct UnloadingGuestDriver {
>>>> +    inner: bindings::rpc_unloading_guest_driver_v1F_07,
>>>> +}
>>>> +
>>>> +impl UnloadingGuestDriver {
>>>> +    pub(crate) fn new(suspend: bool) -> Self {
>>>> +        Self {
>>>> +            inner: bindings::rpc_unloading_guest_driver_v1F_07 {
>>> 
>>> We should go through intermediate firmware representation than direct bindings access?
>> 
>> Only if the size of the bindings justifies it - here having an opaque
>> wrapper just just well, and spares us some code down the line as we
>> would have to initialize the bindings anyway.
>
> I am not sure about that, it sounds like a layering violation. It would be good not to keep the rules fuzzy about this, because then we could do it either way in all cases.
>
> Another reason is that we cannot anticipate in advance which specific helper functions we will need to add in the future. Down the line, we may need to add some helper functions to the struct as well.  Also having V1F07 in the name sounds very magic number-ish. It would be good to abstract that out with a better-named struct anyway.

The rules are not fuzzy. The only thing modules outside of `fw` are
seeing is a struct named `UnloadingGuestDriver`, and the name with
`V1F07` is not leaked.

Even if we had a different structure, we would still need to write the
rpc_unloading_guest_driver_v1F_07 at some point, so we would need to
refer to it in `fw` anyway. The current design is not any more lax than
that, it just removes one step.

Re: [PATCH 6/7] gpu: nova-core: send UNLOADING_GUEST_DRIVER GSP command GSP upon unloading

[Joel Fernandes]

> On Dec 18, 2025, at 10:26 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> 
> On Fri Dec 19, 2025 at 5:52 AM JST, Joel Fernandes wrote:
>> Hi Alex,
>> 
>>>>> +    }
>>>>> +}
>>>>> diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
>>>>> index 09549f7db52d..228464fd47a0 100644
>>>>> --- a/drivers/gpu/nova-core/gsp/fw.rs
>>>>> +++ b/drivers/gpu/nova-core/gsp/fw.rs
>>>>> @@ -209,6 +209,7 @@ pub(crate) enum MsgFunction {
>>>>>   GspInitPostObjGpu = bindings::NV_VGPU_MSG_FUNCTION_GSP_INIT_POST_OBJGPU,
>>>>>   GspRmControl = bindings::NV_VGPU_MSG_FUNCTION_GSP_RM_CONTROL,
>>>>>   GetStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO,
>>>>> +    UnloadingGuestDriver = bindings::NV_VGPU_MSG_FUNCTION_UNLOADING_GUEST_DRIVER,
>>>>> 
>>>>>   // Event codes
>>>>>   GspInitDone = bindings::NV_VGPU_MSG_EVENT_GSP_INIT_DONE,
>>>>> @@ -249,6 +250,9 @@ fn try_from(value: u32) -> Result<MsgFunction> {
>>>>>           }
>>>>>           bindings::NV_VGPU_MSG_FUNCTION_GSP_RM_CONTROL => Ok(MsgFunction::GspRmControl),
>>>>>           bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO => Ok(MsgFunction::GetStaticInfo),
>>>>> +            bindings::NV_VGPU_MSG_FUNCTION_UNLOADING_GUEST_DRIVER => {
>>>>> +                Ok(MsgFunction::UnloadingGuestDriver)
>>>>> +            }
>>>>>           bindings::NV_VGPU_MSG_EVENT_GSP_INIT_DONE => Ok(MsgFunction::GspInitDone),
>>>>>           bindings::NV_VGPU_MSG_EVENT_GSP_RUN_CPU_SEQUENCER => {
>>>>>               Ok(MsgFunction::GspRunCpuSequencer)
>>>>> diff --git a/drivers/gpu/nova-core/gsp/fw/commands.rs b/drivers/gpu/nova-core/gsp/fw/commands.rs
>>>>> index 85465521de32..c7df4783ad21 100644
>>>>> --- a/drivers/gpu/nova-core/gsp/fw/commands.rs
>>>>> +++ b/drivers/gpu/nova-core/gsp/fw/commands.rs
>>>>> @@ -125,3 +125,30 @@ unsafe impl AsBytes for GspStaticConfigInfo {}
>>>>> // SAFETY: This struct only contains integer types for which all bit patterns
>>>>> // are valid.
>>>>> unsafe impl FromBytes for GspStaticConfigInfo {}
>>>>> +
>>>>> +/// Payload of the `UnloadingGuestDriver` command and message.
>>>>> +#[repr(transparent)]
>>>>> +#[derive(Clone, Copy, Debug, Zeroable)]
>>>>> +pub(crate) struct UnloadingGuestDriver {
>>>>> +    inner: bindings::rpc_unloading_guest_driver_v1F_07,
>>>>> +}
>>>>> +
>>>>> +impl UnloadingGuestDriver {
>>>>> +    pub(crate) fn new(suspend: bool) -> Self {
>>>>> +        Self {
>>>>> +            inner: bindings::rpc_unloading_guest_driver_v1F_07 {
>>>> 
>>>> We should go through intermediate firmware representation than direct bindings access?
>>> 
>>> Only if the size of the bindings justifies it - here having an opaque
>>> wrapper just just well, and spares us some code down the line as we
>>> would have to initialize the bindings anyway.
>> 
>> I am not sure about that, it sounds like a layering violation. It would be good not to keep the rules fuzzy about this, because then we could do it either way in all cases.
>> 
>> Another reason is that we cannot anticipate in advance which specific helper functions we will need to add in the future. Down the line, we may need to add some helper functions to the struct as well.  Also having V1F07 in the name sounds very magic number-ish. It would be good to abstract that out with a better-named struct anyway.
> 
> The rules are not fuzzy. The only thing modules outside of `fw` are
> seeing is a struct named `UnloadingGuestDriver`, and the name with
> `V1F07` is not leaked.
> 
> Even if we had a different structure, we would still need to write the
> rpc_unloading_guest_driver_v1F_07 at some point, so we would need to
> refer to it in `fw` anyway. The current design is not any more lax than
> that, it just removes one step.

Ah, I missed that. This is all in FW, so I think that clarifies the rule for me now.

Thank you.

Re: [PATCH 6/7] gpu: nova-core: send UNLOADING_GUEST_DRIVER GSP command GSP upon unloading

[Timur Tabi]

On Thu, 2025-12-18 at 22:27 +0900, Alexandre Courbot wrote:
> > > +    loop {
> > > +        match cmdq.receive_msg::<UnloadingGuestDriverReply>(Delta::from_secs(5)) {
> > > +            Ok(resp) => return Ok(resp),
> > > +            Err(ERANGE) => continue,
> > > +            Err(e) => return Err(e),
> > > +        }
> > 
> > This outer loop can go on infinitely, lets bound it?
> > 
> > Either outer timeout or bounded number of tries. Bounded tries better since it has inner
> > timeout.
> 
> Yes. And what we really want is a more generic way to do this, because
> this pattern occurs with several commands so far (and more to come).

Isn't the real problem that we are polling for a specific message, when all message should be
handled asynchronously as events, like Nouveau does?

          Err(ERANGE) => continue,

This effectively throws out all other messages, including errors and anything else important.

Re: [PATCH 6/7] gpu: nova-core: send UNLOADING_GUEST_DRIVER GSP command GSP upon unloading

[Joel Fernandes]

> On Dec 18, 2025, at 5:33 PM, Timur Tabi <ttabi@nvidia.com> wrote:
> 
> On Thu, 2025-12-18 at 22:27 +0900, Alexandre Courbot wrote:
>>>> +    loop {
>>>> +        match cmdq.receive_msg::<UnloadingGuestDriverReply>(Delta::from_secs(5)) {
>>>> +            Ok(resp) => return Ok(resp),
>>>> +            Err(ERANGE) => continue,
>>>> +            Err(e) => return Err(e),
>>>> +        }
>>> 
>>> This outer loop can go on infinitely, lets bound it?
>>> 
>>> Either outer timeout or bounded number of tries. Bounded tries better since it has inner
>>> timeout.
>> 
>> Yes. And what we really want is a more generic way to do this, because
>> this pattern occurs with several commands so far (and more to come).
> 
> Isn't the real problem that we are polling for a specific message, when all message should be
> handled asynchronously as events, like Nouveau does?
> 
>          Err(ERANGE) => continue,
> 
> This effectively throws out all other messages, including errors and anything else important.
> 

Indeed, for that we need Interrupts. For the rest of the patterns where we need the message synchronously, we should bound this. Hanging in the driver is unacceptable.

Interrupts are almost here now, considering we/others have upstreamed all the required patches for Rust interrupt support.

Re: [PATCH 6/7] gpu: nova-core: send UNLOADING_GUEST_DRIVER GSP command GSP upon unloading

[Timur Tabi]

On Thu, 2025-12-18 at 22:44 +0000, Joel Fernandes wrote:
> > Isn't the real problem that we are polling for a specific message, when all message should be
> > handled asynchronously as events, like Nouveau does?
> > 
> >           Err(ERANGE) => continue,
> > 
> > This effectively throws out all other messages, including errors and anything else important.
> > 
> 
> Indeed, for that we need Interrupts. For the rest of the patterns where we need the message
> synchronously, we should bound this. Hanging in the driver is unacceptable.

It's going to be difficulty to have a running asynchronous message handler in the background *and*
poll synchronously for a specific message on occasional.  I would say that even in this case, we
should handle the message asynchronously.  So instead of polling on the message queue, we just wait
on a semaphore, with a timeout.

> Interrupts are almost here now, considering we/others have upstreamed all the required patches for
> Rust interrupt support.

So my suggestion is that we don't overcomplicate this code today, since it's all going to be ripped
out later.

Re: [PATCH 6/7] gpu: nova-core: send UNLOADING_GUEST_DRIVER GSP command GSP upon unloading

[Joel Fernandes]

On 12/18/2025 6:34 PM, Timur Tabi wrote:
> On Thu, 2025-12-18 at 22:44 +0000, Joel Fernandes wrote:
>>> Isn't the real problem that we are polling for a specific message, when all message should be
>>> handled asynchronously as events, like Nouveau does?
>>>
>>>           Err(ERANGE) => continue,
>>>
>>> This effectively throws out all other messages, including errors and anything else important.
>>>
>>
>> Indeed, for that we need Interrupts. For the rest of the patterns where we need the message
>> synchronously, we should bound this. Hanging in the driver is unacceptable.
> 
> It's going to be difficulty to have a running asynchronous message handler in the background *and*
> poll synchronously for a specific message on occasional.  I would say that even in this case, we
> should handle the message asynchronously.  So instead of polling on the message queue, we just wait
> on a semaphore, with a timeout.

I think we don't strictly need a semaphore for synchronous polling - the wait is
expected to be short AFAIK and if not we should just error out. What we need is
a registration mechanism that registers different event types and their
handlers, and if the message received is not an expected one, we simply call the
event handler registered while continuing to poll for the message we are
expecting until it is received: See how Nouveau does it in r535_gsp_msg_recv().
Anyway, the wait should be expected to be short and if not, we'd break out of
the loop { }.

Interestingly, Nouveau inserts 2 micro second sleeps while polling AFAICS. Where
as OpenRM simply spins without sleeps. I would even say that sleeping in the
loop is risky due to the dependency on atomic context, so we'd have to be
careful there (I am guessing all our usecases for these loops are non-atomic
context?).

We still need the interrupt handling for cases where we don't need synchronous
polling. During then, we will directly call the event handlers from
IRQ->workqueue path. The event handler registration/calling code in both cases
should be the same.

So in the loop { }, nova needs something like this:

  Err(ERANGE) => {
      // Dispatch to notification
      dispatch_async_message(msg); // Same ones called by Async handling.
      continue;
  }

 - Joel

Re: [PATCH 6/7] gpu: nova-core: send UNLOADING_GUEST_DRIVER GSP command GSP upon unloading

[Joel Fernandes]

On 12/18/2025 8:46 PM, Joel Fernandes wrote:
> 
> 
> On 12/18/2025 6:34 PM, Timur Tabi wrote:
>> On Thu, 2025-12-18 at 22:44 +0000, Joel Fernandes wrote:
>>>> Isn't the real problem that we are polling for a specific message, when all message should be
>>>> handled asynchronously as events, like Nouveau does?
>>>>
>>>>           Err(ERANGE) => continue,
>>>>
>>>> This effectively throws out all other messages, including errors and anything else important.
>>>>
>>>
>>> Indeed, for that we need Interrupts. For the rest of the patterns where we need the message
>>> synchronously, we should bound this. Hanging in the driver is unacceptable.
>>
>> It's going to be difficulty to have a running asynchronous message handler in the background *and*
>> poll synchronously for a specific message on occasional.  I would say that even in this case, we
>> should handle the message asynchronously.  So instead of polling on the message queue, we just wait
>> on a semaphore, with a timeout.
> 
> I think we don't strictly need a semaphore for synchronous polling - the wait is
> expected to be short AFAIK and if not we should just error out. What we need is
> a registration mechanism that registers different event types and their
> handlers, and if the message received is not an expected one, we simply call the
> event handler registered while continuing to poll for the message we are
> expecting until it is received: See how Nouveau does it in r535_gsp_msg_recv().
> Anyway, the wait should be expected to be short and if not, we'd break out of
> the loop { }.
> 
> Interestingly, Nouveau inserts 2 micro second sleeps while polling AFAICS. Where
> as OpenRM simply spins without sleeps. I would even say that sleeping in the
> loop is risky due to the dependency on atomic context, so we'd have to be
> careful there (I am guessing all our usecases for these loops are non-atomic
> context?).
> 
> We still need the interrupt handling for cases where we don't need synchronous
> polling. During then, we will directly call the event handlers from
> IRQ->workqueue path. The event handler registration/calling code in both cases
> should be the same.
> 
> So in the loop { }, nova needs something like this:
> 
>   Err(ERANGE) => {
>       // Dispatch to notification
>       dispatch_async_message(msg); // Same ones called by Async handling.
>       continue;
>   }
Btw, I can work on this tomorrow and send out some patches.

Re: [PATCH 6/7] gpu: nova-core: send UNLOADING_GUEST_DRIVER GSP command GSP upon unloading

[Alexandre Courbot]

On Fri Dec 19, 2025 at 7:33 AM JST, Timur Tabi wrote:
> On Thu, 2025-12-18 at 22:27 +0900, Alexandre Courbot wrote:
>> > > +    loop {
>> > > +        match cmdq.receive_msg::<UnloadingGuestDriverReply>(Delta::from_secs(5)) {
>> > > +            Ok(resp) => return Ok(resp),
>> > > +            Err(ERANGE) => continue,
>> > > +            Err(e) => return Err(e),
>> > > +        }
>> > 
>> > This outer loop can go on infinitely, lets bound it?
>> > 
>> > Either outer timeout or bounded number of tries. Bounded tries better since it has inner
>> > timeout.
>> 
>> Yes. And what we really want is a more generic way to do this, because
>> this pattern occurs with several commands so far (and more to come).
>
> Isn't the real problem that we are polling for a specific message, when all message should be
> handled asynchronously as events, like Nouveau does?

Does Nouveau really handle all messages asynchronously? Just taking a
look at `r535_gsp_rpc_send` I see:

* A potential busy-loop with `r535_gsp_rpc_handle_reply`, An argument to
* define whether we should wait for a reply (`policy`).

So it seems like each GSP command expecting a reply is effectively
looping until it arrives, with some messages (LIBOS_PRINT, SEQUENCER,
etc.) being managed by a notifier registered with the command queue. But
messages sent explicitly by the driver don't seem to make use of it and
instead process messages until they find their reply.

This seems to work because IIUC the GSP sends replies in the same order
as it received the messages (so one caller cannot consume the reply
intended to another) and GSP messages are a replacement for the CPU
messing with the hardware itself anyway. So in that context that design
is not particularly awful, but maybe we will want to switch to something
interrupt-based in Nova long-term anyway.

Re: [PATCH 6/7] gpu: nova-core: send UNLOADING_GUEST_DRIVER GSP command GSP upon unloading

[Timur Tabi]

On Fri, 2025-12-19 at 12:39 +0900, Alexandre Courbot wrote:



> Does Nouveau really handle all messages asynchronously? Just taking a
> look at `r535_gsp_rpc_send` I see:
> 
> * A potential busy-loop with `r535_gsp_rpc_handle_reply`, An argument to
> * define whether we should wait for a reply (`policy`).
> 
> So it seems like each GSP command expecting a reply is effectively
> looping until it arrives, with some messages (LIBOS_PRINT, SEQUENCER,
> etc.) being managed by a notifier registered with the command queue. But
> messages sent explicitly by the driver don't seem to make use of it and
> instead process messages until they find their reply.

Yes, you're right.  But the difference is that in Nouveau, all message processing is handled by
r535_gsp_msg_recv(), which always also handles all of the asynchronous "other" messages.

The above `loop` expression in Nova doesn't do that.  It's missing the asynchronous handler. 
This is the crux of my concern.

> This seems to work because IIUC the GSP sends replies in the same order
> as it received the messages (so one caller cannot consume the reply
> intended to another) and GSP messages are a replacement for the CPU
> messing with the hardware itself anyway. So in that context that design
> is not particularly awful, but maybe we will want to switch to something
> interrupt-based in Nova long-term anyway.

Sure, but we still need to do it the way Nouveau handles it.  We need our own version of
r535_gsp_rpc_handle_reply() which unifies handling of all incoming messages, either polling or
interrupt-based.

For now, we can always pass NVKM_GSP_RPC_REPLY_POLL or NVKM_GSP_RPC_REPLY_RECV, which are
polling-based.  NVKM_GSP_RPC_REPLY_NOWAIT is apparently interrupt-based, which I believe is
triggered via r535_gsp_msgq_work().

Re: [PATCH 6/7] gpu: nova-core: send UNLOADING_GUEST_DRIVER GSP command GSP upon unloading

[Alexandre Courbot]

On Sun Dec 21, 2025 at 6:30 AM JST, Timur Tabi wrote:
> On Fri, 2025-12-19 at 12:39 +0900, Alexandre Courbot wrote:
>
>
>
>> Does Nouveau really handle all messages asynchronously? Just taking a
>> look at `r535_gsp_rpc_send` I see:
>> 
>> * A potential busy-loop with `r535_gsp_rpc_handle_reply`, An argument to
>> * define whether we should wait for a reply (`policy`).
>> 
>> So it seems like each GSP command expecting a reply is effectively
>> looping until it arrives, with some messages (LIBOS_PRINT, SEQUENCER,
>> etc.) being managed by a notifier registered with the command queue. But
>> messages sent explicitly by the driver don't seem to make use of it and
>> instead process messages until they find their reply.
>
> Yes, you're right.  But the difference is that in Nouveau, all message processing is handled by
> r535_gsp_msg_recv(), which always also handles all of the asynchronous "other" messages.
>
> The above `loop` expression in Nova doesn't do that.  It's missing the asynchronous handler. 
> This is the crux of my concern.

I agree with you that this is something we want to improve (either by
adding a handler to the loop, or some other way). It should be its own
patch series once we have better visibility about how we want to handle
message, as the current implementation is still very crude and ad-hoc.

[PATCH 7/7] gpu: nova-core: run Booter Unloader and FWSEC-SB upon unbinding

[Alexandre Courbot]

When probing the driver, the FWSEC-FRTS firmware created a WPR2 secure
memory region to store the GSP firmware, and the booter loader loaded
and started that firmware into the GSP, making it run in RISC-V mode.

These operations need to be reverted upon unloading, particularly the
WPR2 secure region creation, as its presence will prevent the driver
from subsequently probing.

Thus, load and run the Booter Unloader and FWSEC-SB firmwares at unbind
time to put the GPU into a state where it can be probed again.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpu/nova-core/firmware/booter.rs |  1 -
 drivers/gpu/nova-core/firmware/fwsec.rs  |  1 -
 drivers/gpu/nova-core/gpu.rs             |  8 +++++++-
 drivers/gpu/nova-core/gsp/boot.rs        | 35 ++++++++++++++++++++++++++++++++
 drivers/gpu/nova-core/regs.rs            |  5 +++++
 5 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/nova-core/firmware/booter.rs b/drivers/gpu/nova-core/firmware/booter.rs
index f107f753214a..d435ff11c5ec 100644
--- a/drivers/gpu/nova-core/firmware/booter.rs
+++ b/drivers/gpu/nova-core/firmware/booter.rs
@@ -270,7 +270,6 @@ fn new_booter(dev: &device::Device<device::Bound>, data: &[u8]) -> Result<Self>
 #[derive(Copy, Clone, Debug, PartialEq)]
 pub(crate) enum BooterKind {
     Loader,
-    #[expect(unused)]
     Unloader,
 }
 
diff --git a/drivers/gpu/nova-core/firmware/fwsec.rs b/drivers/gpu/nova-core/firmware/fwsec.rs
index b98b1286dc94..0758867a57a6 100644
--- a/drivers/gpu/nova-core/firmware/fwsec.rs
+++ b/drivers/gpu/nova-core/firmware/fwsec.rs
@@ -150,7 +150,6 @@ pub(crate) enum FwsecCommand {
     /// image into it.
     Frts { frts_addr: u64, frts_size: u64 },
     /// Asks [`FwsecFirmware`] to load pre-OS apps on the PMU.
-    #[expect(dead_code)]
     Sb,
 }
 
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index b94784f57b36..f98b195994ce 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -302,7 +302,13 @@ pub(crate) fn unbind(self: Pin<&mut Self>, dev: &device::Device<device::Core>) {
             return;
         };
 
-        let _ = kernel::warn_on_err!(this.gsp.unload(dev, bar, this.gsp_falcon,));
+        let _ = kernel::warn_on_err!(this.gsp.unload(
+            dev,
+            bar,
+            this.spec.chipset,
+            this.gsp_falcon,
+            this.sec2_falcon,
+        ));
 
         this.sysmem_flush.unregister(bar);
     }
diff --git a/drivers/gpu/nova-core/gsp/boot.rs b/drivers/gpu/nova-core/gsp/boot.rs
index e12e1d3fd53f..9525ac912ee9 100644
--- a/drivers/gpu/nova-core/gsp/boot.rs
+++ b/drivers/gpu/nova-core/gsp/boot.rs
@@ -261,7 +261,9 @@ pub(crate) fn unload(
         self: Pin<&mut Self>,
         dev: &device::Device<device::Bound>,
         bar: &Bar0,
+        chipset: Chipset,
         gsp_falcon: &Falcon<Gsp>,
+        sec2_falcon: &Falcon<Sec2>,
     ) -> Result {
         let this = self.project();
 
@@ -271,6 +273,39 @@ pub(crate) fn unload(
             .inspect_err(|e| dev_err!(dev, "unload guest driver failed: {:?}", e))?;
         dev_dbg!(dev, "GSP shut down\n");
 
+        /* Run FWSEC-SB to reset the GSP falcon to its pre-libos state. */
+
+        let bios = Vbios::new(dev, bar)?;
+        let fwsec_sb = FwsecFirmware::new(dev, gsp_falcon, bar, &bios, FwsecCommand::Sb)?;
+        fwsec_sb.run(dev, gsp_falcon, bar)?;
+        dev_dbg!(dev, "FWSEC SB completed\n");
+
+        /* Remove WPR2 region if set. */
+
+        let wpr2_hi = regs::NV_PFB_PRI_MMU_WPR2_ADDR_HI::read(bar);
+        dev_dbg!(dev, "WPR2 HI: {:?}\n", wpr2_hi);
+        if wpr2_hi.is_wpr2_set() {
+            let booter_unloader = BooterFirmware::new(
+                dev,
+                BooterKind::Unloader,
+                chipset,
+                FIRMWARE_VERSION,
+                sec2_falcon,
+                bar,
+            )?;
+
+            dev_dbg!(dev, "Booter unloader created\n");
+
+            sec2_falcon.reset(bar)?;
+            sec2_falcon.dma_load(bar, &booter_unloader)?;
+            let _ = sec2_falcon.boot(bar, Some(0xff), Some(0xff))?;
+            dev_dbg!(
+                dev,
+                "WPR2 HI: {:?}\n",
+                regs::NV_PFB_PRI_MMU_WPR2_ADDR_HI::read(bar)
+            );
+        }
+
         Ok(())
     }
 }
diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
index 82cc6c0790e5..6f53ac096e1e 100644
--- a/drivers/gpu/nova-core/regs.rs
+++ b/drivers/gpu/nova-core/regs.rs
@@ -156,6 +156,11 @@ impl NV_PFB_PRI_MMU_WPR2_ADDR_HI {
     pub(crate) fn higher_bound(self) -> u64 {
         u64::from(self.hi_val()) << 12
     }
+
+    /// Returns whether the WPR2 regions is currently set.
+    pub(crate) fn is_wpr2_set(self) -> bool {
+        self.hi_val() != 0
+    }
 }
 
 // PGC6 register space.

-- 
2.52.0