Re: [PATCH v4 15/17] gpu: nova-core: service GSP events from the SWGEN0 interrupt

["Alexandre Courbot" <acourbot@nvidia.com>]

On Sat Sep 12, 2026 at 5:43 AM BST, John Hubbard wrote:
<...>
> diff --git a/drivers/gpu/nova-core/falcon/gsp.rs b/drivers/gpu/nova-core/falcon/gsp.rs
> index 4c96ae325fda..dfa08bc6867c 100644
> --- a/drivers/gpu/nova-core/falcon/gsp.rs
> +++ b/drivers/gpu/nova-core/falcon/gsp.rs
> @@ -5,6 +5,7 @@
>          io_project,
>          poll::read_poll_timeout,
>          register,
> +        register::Array,
>          Io,
>          Mmio, //
>      },
> @@ -18,9 +19,11 @@
>          NovaRegisters, //
>      },
>      falcon::{
> +        hal,
>          Falcon,
>          FalconEngine, //
>      },
> +    gpu::Chipset,
>      regs,
>  };
>  
> @@ -46,14 +49,72 @@ fn pfalcon2(io: Bar0<'_>) -> Mmio<'_, super::PFalcon2Registers> {
>      }
>  }
>  
> -impl<'a> Falcon<'a, Gsp> {
> -    /// Clears the SWGEN0 bit in the Falcon's IRQ status clear register to
> -    /// allow GSP to signal CPU for processing new messages in message queue.
> -    pub(crate) fn clear_swgen0_intr(&self) {
> -        self.pfalcon
> -            .write_reg(regs::NV_PFALCON_FALCON_IRQSCLR::zeroed().with_swgen0(true));
> +impl Gsp {
> +    /// Clears the SWGEN0 latch in the GSP falcon.
> +    ///
> +    /// While the latch is set, no later message signals the tree, so a caller that consumed a
> +    /// notification by polling must clear it.
> +    pub(crate) fn clear_swgen0_intr(bar: Bar0<'_>) {
> +        Self::pfalcon(bar).write_reg(regs::NV_PFALCON_FALCON_IRQSCLR::zeroed().with_swgen0(true));
> +    }

This hunk is the symptom of something we are losing in this series: you
don't need a reference to the Falcon instance anymore to manage
interrupts, just call a crate-public method with a BAR reference. I
think there is a way to design this properly but I don't trust the LLM
to do it right, so I'll fix it in a follow-up after the series is
merged.

> +
> +    /// Reads the GSP falcon causes that are routed to the host, without clearing any latch.
> +    ///
> +    /// Every one of them other than SWGEN0 reports a GSP fault.
> +    pub(crate) fn read_host_intr(
> +        bar: Bar0<'_>,
> +        chipset: Chipset,
> +    ) -> regs::NV_PFALCON_FALCON_IRQSTAT {
> +        let latched = Self::pfalcon(bar).read(regs::NV_PFALCON_FALCON_IRQSTAT);
> +
> +        hal::falcon_intr_hal(chipset)
> +            .riscv_routing()
> +            .host_routed_causes(Self::pfalcon2(bar), latched)
> +    }
> +
> +    /// Reads the host-routed causes and clears the SWGEN0 latch if it was set.
> +    ///
> +    /// Returns the causes as read, before the clear. No other latch changes.
> +    pub(crate) fn take_host_intr(
> +        bar: Bar0<'_>,
> +        chipset: Chipset,
> +    ) -> regs::NV_PFALCON_FALCON_IRQSTAT {
> +        let status = Self::read_host_intr(bar, chipset);
> +
> +        if status.swgen0() {
> +            Self::clear_swgen0_intr(bar);
> +        }
> +
> +        status
> +    }
> +
> +    /// Clears the latch of every interrupt cause set in `status`.
> +    ///
> +    /// A cause driven from outside the falcon is still set on return, and
> +    /// [`Self::read_host_intr`] reports the causes that remain.
> +    pub(crate) fn clear_intr(bar: Bar0<'_>, status: regs::NV_PFALCON_FALCON_IRQSTAT) {
> +        Self::pfalcon(bar).write_reg(regs::NV_PFALCON_FALCON_IRQSCLR::from(status.into_raw()));
> +    }
> +
> +    /// Retriggers the GSP falcon, which then re-emits its host-routed causes into the tree.
> +    ///
> +    /// Call this only once every host cause is clear. A cause still set is re-emitted at once, and
> +    /// its vector arrives again as soon as delivery is rearmed.
> +    ///
> +    /// Does nothing on Turing, whose falcons have no retrigger register.
> +    pub(crate) fn retrigger_intr(bar: Bar0<'_>, chipset: Chipset) {
> +        if !hal::falcon_intr_hal(chipset).has_intr_retrigger() {
> +            return;
> +        }
> +
> +        Self::pfalcon(bar).write(
> +            Array::at(0),
> +            regs::NV_PFALCON_FALCON_INTR_RETRIGGER::zeroed().with_trigger(true),
> +        );
>      }
> +}
>  
> +impl<'a> Falcon<'a, Gsp> {
>      /// Checks if GSP reload/resume has completed during the boot process.
>      pub(crate) fn check_reload_completed(&self, timeout: Delta) -> Result<bool> {
>          read_poll_timeout(
> diff --git a/drivers/gpu/nova-core/falcon/hal.rs b/drivers/gpu/nova-core/falcon/hal.rs
> index 052610c4a4da..3f1f509eccbd 100644
> --- a/drivers/gpu/nova-core/falcon/hal.rs
> +++ b/drivers/gpu/nova-core/falcon/hal.rs
> @@ -82,7 +82,6 @@ fn signature_reg_fuse_version(
>  
>  /// Offsets of a falcon's RISC-V interrupt routing registers.
>  #[derive(Clone, Copy, Debug, Eq, PartialEq)]
> -#[expect(dead_code)]
>  pub(crate) enum RiscvRouting {
>      /// The Turing offsets. GA100 uses them too.
>      Tu102,
> @@ -97,7 +96,6 @@ impl RiscvRouting {
>      ///
>      /// The causes routed to the core belong to the firmware running on it, and the host does not
>      /// service them.
> -    #[expect(dead_code)]
>      pub(crate) fn host_routed_causes(
>          self,
>          pfalcon2: Mmio<'_, PFalcon2Registers>,
> @@ -122,7 +120,6 @@ pub(crate) fn host_routed_causes(
>  ///
>  /// Separate from [`FalconHal`] because the GSP event handler calls these from hard interrupt
>  /// context, where it cannot make the heap allocation that a `FalconHal` takes.
> -#[expect(dead_code)]
>  pub(crate) trait FalconIntrHal {
>      /// Returns whether these falcons implement `NV_PFALCON_FALCON_INTR_RETRIGGER`.
>      fn has_intr_retrigger(&self) -> bool;
> @@ -135,7 +132,6 @@ pub(crate) trait FalconIntrHal {
>  ///
>  /// GA100 has its own arm: it has the retrigger register, which Turing lacks, and the Turing
>  /// routing offsets, which GA102 moved.
> -#[expect(dead_code)]
>  pub(crate) fn falcon_intr_hal(chipset: Chipset) -> &'static dyn FalconIntrHal {
>      match chipset.arch() {
>          Architecture::Turing => tu102::TU102_INTR_HAL,
> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
> index 3d796d6c7013..d1e0da7b8682 100644
> --- a/drivers/gpu/nova-core/gpu.rs
> +++ b/drivers/gpu/nova-core/gpu.rs
> @@ -38,6 +38,11 @@
>          Gsp,
>          GspBootContext, //
>      },
> +    irq::{
> +        self,
> +        gsp::GspIrq,
> +        SubtreeVectors, //
> +    },
>      mm::{
>          bar_user::BarUser,
>          pagetable::MmuVersion,
> @@ -301,6 +306,13 @@ struct GspResources<'gpu> {
>  #[pin_data]
>  pub(crate) struct Gpu<'gpu> {
>      spec: Spec,
> +    /// GSP event interrupt registration.
> +    ///
> +    /// Must be kept declared *before* `gsp_resources`, so that the handler is unregistered, and
> +    /// any in-flight run of it has finished, before the command queue it drains is freed and
> +    /// before the GSP is unloaded.
> +    #[pin]
> +    _gsp_irq: GspIrq<'gpu>,

Eventually this should belong to the `Gsp` instance. It looks weird that
the IRQ aspect is the only one handled separately.

(this is not an ask for fix, more of a self-note for later)

>      /// Static GPU information as provided by the GSP.
>      gsp_static_info: GetGspStaticInfoReply,
>      /// GPU memory manager owning memory management resources.
> @@ -319,6 +331,14 @@ pub(crate) struct Gpu<'gpu> {
>      /// Must be kept declared *after* `gsp_resources`, as the latter's `PinnedDrop` implementation
>      /// requires the sysmem flush page to be in place.
>      sysmem_flush: SysmemFlush<'gpu>,
> +    /// Borrow of `vectors` that `_gsp_irq` holds. A field that borrows a sibling field is
> +    /// self-referential, which `pin_init` cannot express, so the borrow is taken by hand.
> +    vectors_ref: &'gpu SubtreeVectors<'gpu>,
> +    /// PCI interrupt vector allocation.
> +    ///
> +    /// Must be kept declared *after* `_gsp_irq`, which holds a borrow of it.
> +    #[pin]
> +    vectors: SubtreeVectors<'gpu>,
>  }
>  
>  #[pinned_drop]
> @@ -358,6 +378,12 @@ pub(crate) fn new<'a>(
>          let dev = pdev.as_ref();
>  
>          try_pin_init!(Self {
> +            vectors: irq::alloc_vectors(pdev, irq::gsp::GSP_SUBTREE.into())?,
> +
> +            // SAFETY: `vectors` is initialized above, is pinned at a stable address, and is
> +            // dropped after every field that uses `vectors_ref` (struct field drop order).
> +            vectors_ref: unsafe { &*core::ptr::from_ref(vectors.as_ref().get_ref()) },
> +
>              spec: Spec::new(dev, bar).inspect(|spec| {
>                  dev_info!(dev,"NVIDIA ({})\n", spec);
>              })?,
> @@ -380,12 +406,7 @@ pub(crate) fn new<'a>(
>  
>                  bar,
>  
> -                gsp_falcon: Falcon::new(
> -                    dev,
> -                    spec.chipset,
> -                    bar
> -                )
> -                .inspect(|falcon| falcon.clear_swgen0_intr())?,
> +                gsp_falcon: Falcon::new(dev, spec.chipset, bar)?,
>  
>                  sec2_falcon: Falcon::new(dev, spec.chipset, bar)?,
>  
> @@ -409,6 +430,30 @@ pub(crate) fn new<'a>(
>                  })?,
>              }),
>  
> +            _: {
> +                irq::gsp::quiesce(bar, gsp_resources.spec.chipset, vectors_ref)?;
> +            },
> +
> +            // SAFETY: the command queue is a field of `gsp_resources`, which is initialized
> +            // above and pinned, so the reference outlives the registration. The registration is
> +            // a field of `Gpu` and is never leaked, so its `Drop` runs, and field drop order
> +            // runs it before the queue is freed.
> +            _gsp_irq <- unsafe {
> +                GspIrq::new(
> +                    pdev,
> +                    vectors_ref,
> +                    bar,
> +                    &*core::ptr::from_ref(&gsp_resources.gsp.cmdq),
> +                    gsp_resources.spec.chipset,
> +                )
> +            },
> +
> +            // No interrupt announces the messages that the GSP posted during boot, before the
> +            // SWGEN0 latch was cleared.
> +            _: {
> +                gsp_resources.gsp.cmdq.drain()?;
> +            },
> +
>              gsp_static_info: {
>                  // Obtain and display basic GPU information.
>                  let info = gsp_resources.gsp.get_static_info(bar)?;
> diff --git a/drivers/gpu/nova-core/gsp.rs b/drivers/gpu/nova-core/gsp.rs
> index 25ea43f1cbe9..fcfb4210d435 100644
> --- a/drivers/gpu/nova-core/gsp.rs
> +++ b/drivers/gpu/nova-core/gsp.rs
> @@ -152,7 +152,7 @@ pub(crate) struct Gsp<'gsp> {
>      /// Log buffers, optionally exposed via debugfs.
>      #[pin]
>      logs: debugfs::Scope<LogBuffers<'gsp>>,
> -    /// Command queue.
> +    /// Command queue, borrowed by the GSP event interrupt handler.

The borrow comment shouldn't matter here. 

>      #[pin]
>      pub(crate) cmdq: Cmdq<'gsp>,
>      /// RM arguments.
> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index acee444e898d..f1231569aa33 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -643,7 +643,6 @@ pub(crate) fn await_msg<M: MessageFromGsp>(&self) -> Result<M>
>      /// # Errors
>      ///
>      /// `EIO` if the queue is poisoned, or if a message fails framing or checksum validation.
> -    #[expect(dead_code)]

The patch adding the drain method is short and looks lonely as-is, let's
fold it into this one and remove this temporary dead code.

>      pub(crate) fn drain(&self) -> Result {
>          self.inner.lock().drain()
>      }
> diff --git a/drivers/gpu/nova-core/irq.rs b/drivers/gpu/nova-core/irq.rs
> index 7fb7d9f2e237..cafcc613770a 100644
> --- a/drivers/gpu/nova-core/irq.rs
> +++ b/drivers/gpu/nova-core/irq.rs
> @@ -11,6 +11,7 @@
>  
>  #[cfg(CONFIG_NOVA_CORE_SELFTESTS)]
>  pub(crate) mod doorbell_test;
> +pub(crate) mod gsp;
>  mod hal;
>  mod interrupt_tree;
>  mod regs;
> @@ -25,11 +26,16 @@
>      prelude::*, //
>  };
>  
> -use crate::num;
> +use crate::{
> +    driver::Bar0,
> +    gpu::Chipset,
> +    num, //
> +};
>  
>  use interrupt_tree::{
>      Subtree,
> -    SubtreeSet, //
> +    SubtreeSet,
> +    Tree, //
>  };
>  
>  /// The message-signaled interrupt type that Linux granted.
> @@ -56,6 +62,39 @@ pub(crate) struct SubtreeVectors<'a> {
>  }
>  
>  impl SubtreeVectors<'_> {
> +    /// Returns the tree of `chipset`, covering the serviced subtrees.
> +    ///
> +    /// # Errors
> +    ///
> +    /// `EINVAL` if `chipset` does not implement every serviced subtree.
> +    fn tree<'b>(&self, bar: Bar0<'b>, chipset: Chipset) -> Result<Tree<'b>> {
> +        Tree::new(bar, chipset, self)
> +    }

We are 4 revisions in, and I still don't understand the relationship
between `SubTreeVectors` and `Tree` clearly. And this method hints very
strongly to me that they should be merged into a single type.

And also this: `SubtreeVectors` already store HAL-dependent information
with the `MsiType` and the number of IRQ registrations, yet when we
invoke `tree` we need to pass it a chipset again. What happens if
`chipset` is not the same as the one with which the `SubtreeVectors` was
created?

I'm not asking for a redesign now because I think it would mess with
ownership of the interrupt handler and cascade into requiring more
changes, but adding a note to myself to address that in a follow-up
patch.

What should be done in v5 though: the doorbell test basically rewrites
this method at the beginning of `run_selftest`. Let's introduce it in
patch 6 so it can be used in the doorbell test as well, and then
`Tree::new` can be made `pub(super)`.

> +
> +    /// Disables every vector in the tree, clears every pending bit, and rearms PCI interrupt
> +    /// delivery.
> +    ///
> +    /// On return, the serviced subtrees are enabled at `TOP` under a `TOP` rearm method and
> +    /// disabled under the configuration-space one. A caller that needs delivery enables them
> +    /// itself.
> +    ///
> +    /// Call this only during probe, with no interrupt handler registered.
> +    ///
> +    /// # Errors
> +    ///
> +    /// `EINVAL` if `chipset` does not implement every serviced subtree.
> +    pub(crate) fn reset_tree(&self, bar: Bar0<'_>, chipset: Chipset) -> Result {
> +        let tree = self.tree(bar, chipset)?;
> +
> +        tree.disable_all_leaves();
> +        tree.drain();
> +        for subtree in self.serviced.iter() {
> +            tree.rearm_pci_irq(subtree);
> +        }
> +
> +        Ok(())
> +    }

So this calls `tree` and then just calls a bunch of methods on it
(except `serviced`, but `serviced` is copied into `Tree` and thus
available from it). IOW, this should just be a `reset` method of `Tree`.