Re: [PATCH v2 12/15] gpu: nova-core: drive GSP events with the SWGEN0 interrupt

[Alexandre Courbot]

On Sat Aug 29, 2026 at 10:33 AM JST, John Hubbard wrote:
<...>
> @@ -79,13 +97,42 @@ fn probe<'bound>(
>              pdev.set_master();
>  
>              Ok(try_pin_init!(NovaCore {
> +                vectors: crate::irq::alloc_vectors(pdev, crate::irq::gsp::GSP_SUBTREE.into())?,
> +                // SAFETY: `vectors` is initialized above, lives at a pinned stable address, and
> +                // is dropped after all fields that use `vectors_ref` (struct field drop order).
> +                vectors_ref: unsafe { &*core::ptr::from_ref(vectors.as_ref().get_ref()) },
>                  bar: pdev.iomap_region_sized::<BAR0_SIZE>(0, c"nova-core/bar0")?,
>                  // TODO: Use `&bar` self-referential pin-init syntax once available.
>                  //
>                  // SAFETY: `bar` is initialized before this expression is evaluated
> -                // (`try_pin_init!()` initializes fields in declaration order), lives at a pinned
> -                // stable address, and is dropped after `gpu` (struct field drop order).
> -                gpu <- Gpu::new(pdev, unsafe { &*core::ptr::from_ref(bar) }),
> +                // (`try_pin_init!()` initializes fields in the order they appear here), lives at a
> +                // pinned stable address, and is dropped after `gpu` (struct field drop order).
> +                gpu <- Gpu::new(pdev, unsafe { &*core::ptr::from_ref(bar) }, vectors_ref),
> +                // Quiesce the interrupt tree before registering the handler below.
> +                _: {
> +                    // SAFETY: as for the `bar` borrow above.
> +                    let bar = unsafe { &*core::ptr::from_ref(bar) };
> +                    crate::irq::gsp::quiesce(bar, gpu.chipset(), vectors_ref.irq_type());
> +                },
> +                // Register the permanent GSP SWGEN0 handler, which enables the interrupt.
> +                //
> +                // SAFETY: `bar` and `vectors` are initialized and pinned (see above). `_gsp_irq`
> +                // is declared before `vectors` in the struct, so it is dropped first, ensuring
> +                // `free_irq` runs before the vectors are freed. The registration is stored in
> +                // `NovaCore` and never leaked.
> +                _gsp_irq <- unsafe {
> +                    GspIrq::new(
> +                        pdev,
> +                        vectors_ref,
> +                        &*core::ptr::from_ref(bar),
> +                        gpu.cmdq(),
> +                        gpu.chipset(),
> +                    )
> +                },
> +                // Drain the messages the GSP posted during boot, before relying on the interrupt.
> +                _: {
> +                    gpu.cmdq().drain()?;
> +                },

Can't these last 3 blocks (quiesce, _gsp_irq, and cmdq drain) be moved
inside `Gpu`? It seems to make sense in terms of ownership, as the `Gpu`
would own its interrupt handler, and things would be much cleaner as
well: there would be no need for the unsafe `bar` lifetime conversion,
`vectors_ref`, or the `chipset` and `cmdq` accessor methods.

Just gave it a quick try locally and it builds fine (with a net -20
LoCs), and AFAICT drop order is also preserved.

Pushing a bit further I could also put `vectors` into `Gpu`, which again
makes sense to me ownership-wise (because the set of interrupts we want
to serve might depend on e.g. the GPU architecture). The only drawback
is that I had to reintroduce `vectors_ref`, but that's a small and
temporary hack. I'd say this belongs in `gpu.rs` as well.

(after looking some more at the code) Ok, I'm now completely convinced
this belongs here. We could put these blocks right after `gsp_resources`
(which boots the GSP), with the benefit that the GSP interrupts will be
working to build `gsp_static_info`, which is obtained by sending a
regular GSP message! Right now we are still polling to build it, but
with the IRQ handler ready we could just wait for the signal to read the
reply. I am not saying this should be done for this series (let's do it
as a follow-up), but this is to illustrate that this is where the IRQ
setup should be done, not in `driver.rs`.

<...>
> +/// Clears the interrupt state that GSP boot left behind.
> +///
> +/// Disables every vector in every implemented leaf, clears the falcon's SWGEN0 latch, clears the
> +/// tree's pending bits, and rearms PCI interrupt delivery. On return no vector is enabled, so the
> +/// tree delivers nothing.
> +pub(crate) fn quiesce(bar: Bar0<'_>, chipset: Chipset, irq_type: pci::IrqType) {
> +    let tree = Tree::new(bar, chipset, irq_type, GSP_SUBTREE.into());
> +    tree.disable_all_leaves();
> +    // GSP boot consumes its notifications by polling the queue, which leaves SWGEN0 latched.
> +    // Clear it before the tree drain below, so the drain clears the tree state the clear sets.
> +    // Messages already posted raise no interrupt of their own, and the caller's queue drain
> +    // covers them.
> +    GspFalcon::clear_swgen0_intr(bar);
> +    tree.drain();
> +    // The `TOP_EN` cycle in `drain` is the rearm for the two enable-cycle methods, but pre-Hopper
> +    // MSI rearms through a configuration-space write instead. An interrupt delivered before probe
> +    // leaves delivery un-armed on that path, with no handler to have rearmed it.
> +    tree.rearm_pci_irq(GSP_SUBTREE);
> +}

The `clear_swgen0_intr` bit is interesting - note that we already do it
in `Gpu::new` (I had no idea why, now I understand! :)), so that
vestigial one can be removed (which should also simplify `falcon/gsp.rs`
a bit).

It also looks like this function could become a method of
`SubtreeVectors` that resets the tree covered by the allocation.

> +
> +/// Threaded IRQ handler for the GSP SWGEN0 event.
> +///
> +/// The top half clears the GIN leaf and reads the falcon SWGEN0 latch. The IRQ thread drains the
> +/// GSP-to-CPU message queue, which takes the command-queue lock.
> +#[pin_data]
> +pub(crate) struct GspInterrupt<'a> {

This type doesn't need `#[pin_data]`. Its constructor can also return
just `Self` - you will just need to wrap its call as a parameter of
`irq::ThreadedRegistration::new` into an `Ok(...)` to make it happy, but
it's simpler overall.

I'll stop here for this revision - I suppose there are more minor
things, but it will be easier to discover them with the bigger cleanups
applied.