Re: [PATCH v2 06/15] gpu: nova-core: add the GIN interrupt tree and allocate its vectors

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

On Sat Aug 29, 2026 at 10:33 AM JST, John Hubbard wrote:
> From: Joel Fernandes <joelagnelf@nvidia.com>
>
> Servicing a GIN leaf has a required order: read its pending bits, then
> clear them. Clearing a leaf before reading it discards every vector
> latched in it, and nothing reports the loss.
>
> The driver must also allocate a PCI vector for every subtree it enables
> at TOP, and register a handler on that vector. MSI-X gives each subtree
> its own table entry. Linux masks every entry the driver did not
> allocate. An enabled subtree with no entry of its own raises interrupts
> that never arrive, and its leaf and TOP bits stay pending and enabled.
> MSI instead has one message that the whole tree raises, so a single
> entry serves every subtree.
>
> Add an API for one PCIe function's CPU interrupt tree, in which reading
> a leaf yields the handle that clears it. Size the vector allocation to
> the serviced subtrees, requesting MSI-X entries up to the highest
> serviced subtree and falling back to a single MSI rather than a shared
> INTx line.
>
> Reviewed-by: Will Pierce <wpierce@nvidia.com>
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> [jhubbard: name the module interrupt_tree with a Tree type that owns the
>  BAR mapping, use the canonical NV_VIRTUAL_FUNCTION_PRIV_CPU_INTR_*
>  register names, express vectors, leaves and subtrees as newtypes, let
>  the read of a leaf produce the handle that clears it, add the enable
>  guards, take the leaf count and the rearm method from the interrupt
>  HAL, and read every implemented leaf in drain() rather than descending
>  from the TOP registers, which cannot see a vector that latched while
>  disabled]
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  drivers/gpu/nova-core/irq.rs                |  89 ++++++
>  drivers/gpu/nova-core/irq/interrupt_tree.rs | 284 +++++++++++++++++++-
>  2 files changed, 369 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/irq.rs b/drivers/gpu/nova-core/irq.rs
> index 02ecfc47f4d0..c6bf1dbacabe 100644
> --- a/drivers/gpu/nova-core/irq.rs
> +++ b/drivers/gpu/nova-core/irq.rs
> @@ -11,3 +11,92 @@
>  mod hal;
>  mod interrupt_tree;
>  mod regs;
> +
> +use kernel::{
> +    device::Bound,
> +    irq,
> +    pci::{
> +        self,
> +        IrqType, //
> +    },
> +    prelude::*, //
> +};
> +
> +use interrupt_tree::{
> +    Subtree,
> +    SubtreeSet, //
> +};
> +
> +/// The PCI interrupt vector that delivers each serviced subtree.
> +///
> +/// MSI-X raises a separate table entry per subtree, so subtree `N` arrives on entry `N`. MSI has a
> +/// single message that every subtree raises, so all of them arrive on the one allocated entry.
> +pub(crate) struct SubtreeVectors<'a> {
> +    vectors: pci::IrqVectorRegistration<'a>,
> +    /// Every subtree nova-core services.
> +    serviced: SubtreeSet,

Here we would also store the `MsiType` I proposed on the previous patch
and return it in `irq_type`.

> +}
> +
> +impl SubtreeVectors<'_> {
> +    /// Returns the interrupt type the PCI core selected for these vectors.
> +    pub(crate) fn irq_type(&self) -> IrqType {
> +        self.vectors.irq_type()
> +    }
> +
> +    /// Returns an [`irq::IrqRequest`] for the vector that delivers `subtree`.
> +    ///
> +    /// # Errors
> +    ///
> +    /// `EINVAL` if `subtree` is not one nova-core services.
> +    pub(crate) fn request_for(&self, subtree: Subtree) -> Result<irq::IrqRequest<'_>> {
> +        if !self.serviced.contains(subtree) {
> +            return Err(EINVAL);
> +        }
> +
> +        self.vectors
> +            .index(entry_index(self.irq_type(), subtree))
> +            .map(Into::into)
> +    }
> +}
> +
> +/// Returns the index of the allocated entry that `subtree` raises.
> +///
> +/// MSI-X gives subtree `N` its own table entry `N`. MSI raises its one message from every subtree,
> +/// and nova-core allocates a single entry for it. nova-core never allocates INTx.
> +fn entry_index(irq_type: IrqType, subtree: Subtree) -> usize {
> +    match irq_type {
> +        IrqType::MsiX => crate::num::u32_as_usize(subtree.index()),
> +        IrqType::Msi | IrqType::Intx => 0,
> +    }
> +}

This is only called by `request_for`, which is just above, so can we
inline it there?

> +
> +/// Allocates the interrupt vectors that the subtrees in `serviced` require.
> +///
> +/// Every subtree nova-core enables at `TOP` must have an allocated vector with a registered
> +/// handler, or the interrupts it raises are lost. Linux masks every MSI-X entry a driver did not
> +/// allocate, so the MSI-X request covers every entry up to the highest serviced subtree. A part
> +/// whose MSI-X table is smaller than that falls back to a single MSI, which serves the whole tree.
> +/// nova-core does not fall back to a shared INTx line.
> +///
> +/// # Errors
> +///
> +/// `EINVAL` if `serviced` is empty. The error from the MSI request if neither type can be
> +/// allocated.
> +pub(crate) fn alloc_vectors(
> +    pdev: &pci::Device<Bound>,
> +    serviced: SubtreeSet,
> +) -> Result<SubtreeVectors<'_>> {
> +    if serviced.is_empty() {
> +        return Err(EINVAL);
> +    }
> +
> +    // One entry per subtree up to and including the highest serviced one.
> +    let entries = serviced.span();
> +
> +    let vectors = match pdev.alloc_irq_vectors(entries, entries, IrqType::MsiX.into()) {
> +        Ok(vectors) => vectors,
> +        Err(_) => pdev.alloc_irq_vectors(1, 1, IrqType::Msi.into())?,
> +    };

Optional style nit:

    let vectors = pdev
        .alloc_irq_vectors(entries, entries, IrqType::MsiX.into())
        .or_else(|_| pdev.alloc_irq_vectors(1, 1, IrqType::Msi.into()))?;

> +
> +    Ok(SubtreeVectors { vectors, serviced })
> +}
> diff --git a/drivers/gpu/nova-core/irq/interrupt_tree.rs b/drivers/gpu/nova-core/irq/interrupt_tree.rs
> index da24f3d35893..523b26d55137 100644
> --- a/drivers/gpu/nova-core/irq/interrupt_tree.rs
> +++ b/drivers/gpu/nova-core/irq/interrupt_tree.rs
> @@ -1,17 +1,49 @@
>  // SPDX-License-Identifier: GPL-2.0
>  // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
>  
> -//! Vector addressing in the GIN CPU interrupt tree.
> +//! The GIN CPU interrupt tree for one PCIe function.
>  //!
>  //! A vector's number fixes where it latches: leaf `vector / 32` at bit `vector % 32`, and that
>  //! leaf belongs to subtree `vector / 64`. The types here keep those three views apart, so a leaf
>  //! index, a set of vectors within one leaf, and a `TOP` bit cannot stand in for one another.
> +//!
> +//! Servicing a leaf has a required order: read its pending bits, then clear them. Clearing a leaf
> +//! before reading it discards every vector latched in it, and nothing reports the loss. Only
> +//! [`Tree::read_pending`] produces a [`LeafPending`], and only a [`LeafPending`] can clear, so the
> +//! wrong order does not compile.
> +//!
> +//! Serializing access to the tree is the caller's responsibility.
>  
>  use kernel::{
> +    io::{
> +        register::Array,
> +        Io, //
> +    },
>      num::Bounded,
> +    pci::IrqType,
>      prelude::*, //
>  };
>  
> +use crate::{
> +    driver::Bar0,
> +    gpu::Chipset, //
> +};
> +
> +use super::{
> +    hal::{
> +        cpu_interrupt_hal,
> +        PciIrqRearmMethod, //
> +    },
> +    regs::{
> +        NV_VIRTUAL_FUNCTION_PRIV_CPU_INTR_LEAF as CPU_INTR_LEAF,
> +        NV_VIRTUAL_FUNCTION_PRIV_CPU_INTR_LEAF_EN_CLEAR as CPU_INTR_LEAF_EN_CLEAR,
> +        NV_VIRTUAL_FUNCTION_PRIV_CPU_INTR_LEAF_EN_SET as CPU_INTR_LEAF_EN_SET,
> +        NV_VIRTUAL_FUNCTION_PRIV_CPU_INTR_LEAF_TRIGGER as CPU_INTR_LEAF_TRIGGER,
> +        NV_VIRTUAL_FUNCTION_PRIV_CPU_INTR_TOP_EN_CLEAR as CPU_INTR_TOP_EN_CLEAR,
> +        NV_VIRTUAL_FUNCTION_PRIV_CPU_INTR_TOP_EN_SET as CPU_INTR_TOP_EN_SET, //

I'm not a big fan of these long imports - can we just `use regs::*` and
access the registers using their full name?

We used to just import `regs` and access registers using `regs::NV_FOO`
back when they were all declared in a single big file, but now that
registers are properly confined to their module, using a local `regs::*`
is much more appropriate and should become the default IMHO.

> +    }, //
> +};
> +
>  /// Index of a leaf register, bounded to the `0..16` range covered by the leaf register arrays.
>  pub(super) type LeafIndex = Bounded<usize, 4>;
>  
> @@ -97,7 +129,7 @@ pub(super) const fn contains(self, other: Self) -> bool {
>  ///
>  /// Exactly one bit is set.
>  #[derive(Clone, Copy, Debug, Eq, PartialEq)]
> -pub(super) struct Subtree(u32);
> +pub(crate) struct Subtree(u32);
>  
>  impl Subtree {
>      /// Returns this subtree's index within the tree.
> @@ -115,7 +147,7 @@ pub(super) const fn into_raw(self) -> u32 {
>  
>  /// Set of subtrees, one bit per subtree, in the layout the `TOP` enable registers take.
>  #[derive(Clone, Copy, Debug, Eq, PartialEq)]
> -pub(super) struct SubtreeSet(u32);
> +pub(crate) struct SubtreeSet(u32);
>  
>  impl SubtreeSet {
>      /// Returns whether `subtree` belongs to this set.
> @@ -199,7 +231,7 @@ pub(super) const fn subtree(self) -> Subtree {
>      /// # Errors
>      ///
>      /// `EINVAL` if the vector lies beyond the last leaf such a tree implements.
> -    pub(super) const fn validate(self, leaves: LeafCount) -> Result {
> +    pub(super) fn validate(self, leaves: LeafCount) -> Result {

Why are we losing the const here?

>          if self.0 >= leaves.vector_count() {
>              return Err(EINVAL);
>          }
> @@ -207,3 +239,247 @@ pub(super) const fn validate(self, leaves: LeafCount) -> Result {
>          Ok(())
>      }
>  }
> +
> +/// Returns the leaves that subtree `index` covers.
> +///
> +/// An index beyond the leaf register arrays yields nothing rather than panicking.
> +fn subtree_leaves(index: u32) -> impl Iterator<Item = LeafIndex> {
> +    let first = index * LEAVES_PER_SUBTREE;
> +
> +    (first..first + LEAVES_PER_SUBTREE)
> +        .filter_map(|leaf| LeafIndex::try_new(crate::num::u32_as_usize(leaf)))
> +}
> +
> +/// The GIN CPU interrupt tree for a single PCIe function.
> +pub(super) struct Tree<'a> {
> +    /// Borrowed BAR0, through which every tree register is reached.
> +    bar: Bar0<'a>,
> +    /// Number of leaves this tree implements.
> +    leaves: LeafCount,
> +    /// The subtrees this tree enables and services.
> +    serviced: SubtreeSet,
> +    /// Method that rearms PCI interrupt delivery, or `None` if the interrupt type needs no rearm
> +    /// write.
> +    rearm: Option<PciIrqRearmMethod>,

With the proposed changes in the previous patch, this can hopefully
become a `PciIrqRearmMethod`.

> +}
> +
> +impl<'a> Tree<'a> {
> +    /// Creates a `Tree` for `chipset` covering `serviced`, with the rearm method that `irq_type`
> +    /// requires.
> +    ///
> +    /// Each serviced subtree must have an allocated PCI vector and a registered handler, which
> +    /// [`super::alloc_vectors`] sizes the allocation for. Subtrees the architecture does not
> +    /// implement are dropped.
> +    pub(super) fn new(
> +        bar: Bar0<'a>,
> +        chipset: Chipset,
> +        irq_type: IrqType,
> +        serviced: SubtreeSet,
> +    ) -> Self {
> +        let hal = cpu_interrupt_hal(chipset);
> +        let leaves = hal.leaf_count();
> +
> +        Self {
> +            bar,
> +            leaves,
> +            serviced: serviced.intersection(leaves.subtree_set()),

Shouldn't we error if we cannot service some of the requested vectors?

> +            rearm: hal.pci_irq_rearm_method(irq_type),
> +        }
> +    }
> +
> +    /// Returns the subtrees this tree services.
> +    pub(super) fn serviced(&self) -> SubtreeSet {
> +        self.serviced
> +    }
> +
> +    /// Rearms PCI interrupt delivery to the CPU after servicing `subtree`, the one subtree the
> +    /// calling handler serves.
> +    ///
> +    /// A handler must call this before it returns, or it receives no further interrupts.
> +    pub(super) fn rearm_pci_irq(&self, subtree: Subtree) {
> +        if let Some(method) = self.rearm {
> +            method.rearm(self.bar, self.serviced, subtree);
> +        }
> +    }
> +
> +    /// Enables this tree's serviced subtrees (`TOP_EN_SET`).
> +    pub(super) fn enable_top(&self) {
> +        self.bar
> +            .write(CPU_INTR_TOP_EN_SET, self.serviced.into_raw().into());

With the changes in patch 3, this becomes

    self.bar
        .write_reg(CPU_INTR_TOP_EN_SET::zeroed().with_subtrees(self.serviced));

(also applies to `disable_top`).

> +    }
> +
> +    /// Disables this tree's serviced subtrees (`TOP_EN_CLEAR`).
> +    pub(super) fn disable_top(&self) {
> +        self.bar
> +            .write(CPU_INTR_TOP_EN_CLEAR, self.serviced.into_raw().into());
> +    }
> +
> +    /// Enables this tree's serviced subtrees until the returned guard drops.
> +    pub(super) fn enable_top_guarded(&self) -> TopEnableGuard<'_> {
> +        self.enable_top();
> +
> +        TopEnableGuard { tree: self }
> +    }
> +
> +    /// Enables the vectors set in `vectors` for `leaf` (`LEAF_EN_SET`).
> +    ///
> +    /// This is the per-vector counterpart of [`Self::enable_top`], which enables whole subtrees.
> +    pub(super) fn enable_leaf(&self, leaf: LeafIndex, vectors: LeafMask) {
> +        if let Some(loc) = CPU_INTR_LEAF_EN_SET::try_at(leaf.get()) {
> +            self.bar.write(loc, vectors.into_raw().into());
> +        }
> +    }

There are a couple optimizations you can do here.

`LeafIndex` is 4-bits bounded, so its value is guaranteed to be < 16.
But `Bounded::get` does not express this guarantee because it has to be
usable in const context.

`Bounded`'s `Deref` implementation, otoh, *does* include some assertions
that tell the optimizer that the returned value is < 16. So you can use
that, and the build-time checked `at` method, to get rid of the
`unwrap_or`.

And with the register field types set in patch 3, you can set the field
direction without converting to the raw value. So the above `if`
statement becomes just:

    self.bar.write(
        Array::at(*leaf),
        CPU_INTR_LEAF_EN_SET::zeroed().with_vectors(vectors),
    );

(also applies to `disable_leaf`)

> +
> +    /// Disables the vectors set in `vectors` for `leaf` (`LEAF_EN_CLEAR`).
> +    pub(super) fn disable_leaf(&self, leaf: LeafIndex, vectors: LeafMask) {
> +        if let Some(loc) = CPU_INTR_LEAF_EN_CLEAR::try_at(leaf.get()) {
> +            self.bar.write(loc, vectors.into_raw().into());
> +        }
> +    }
> +
> +    /// Enables `vectors` for `leaf` until the returned guard drops.
> +    pub(super) fn enable_leaf_guarded(
> +        &self,
> +        leaf: LeafIndex,
> +        vectors: LeafMask,
> +    ) -> LeafEnableGuard<'_> {
> +        self.enable_leaf(leaf, vectors);
> +
> +        LeafEnableGuard {
> +            tree: self,
> +            leaf,
> +            vectors,
> +        }
> +    }
> +
> +    /// Reads the vectors pending in `leaf`.
> +    pub(super) fn read_pending(&self, leaf: LeafIndex) -> LeafPending<'_> {
> +        let pending = CPU_INTR_LEAF::try_at(leaf.get())
> +            .map(|loc| self.bar.read(loc).into_raw())
> +            .unwrap_or(0);

Similarly, here you can just do:

    let pending = self.bar.read(CPU_INTR_LEAF::at(*leaf)).vectors();

And drop the `unwrap_or(0)` which looks a bit sus to me.

> +
> +        LeafPending {
> +            tree: self,
> +            leaf,
> +            pending: LeafMask::from_raw(pending),

... and you can now assign `pending` as-is.

> +        }
> +    }
> +
> +    /// Injects a software interrupt for `vector` via the trigger register.
> +    ///
> +    /// # Errors
> +    ///
> +    /// `EINVAL` if `vector` lies outside this tree. `EOVERFLOW` if `vector` does not fit in the
> +    /// trigger register's vector field.

Hopefully this error becomes unneeded if we convert `GinVector` to use
`Bounded`.

> +    // Only the interrupt self-test injects a software interrupt.
> +    #[cfg_attr(not(CONFIG_NOVA_CORE_IRQ_SELFTEST), expect(dead_code))]

This Kconfig option does not exist yet as of this patch IIUC.

> +    pub(super) fn trigger(&self, vector: GinVector) -> Result {
> +        vector.validate(self.leaves)?;
> +        self.bar
> +            .write_reg(CPU_INTR_LEAF_TRIGGER::zeroed().try_with_vector(vector.into_raw())?);
> +
> +        Ok(())
> +    }
> +
> +    /// Disables every vector in every implemented leaf (`LEAF_EN_CLEAR`).
> +    ///
> +    /// Boot, or a driver that ran before this one, can leave leaf enables set for vectors
> +    /// nova-core does not service, and such a vector delivers to nova-core's handler once its
> +    /// subtree is enabled.
> +    ///
> +    /// This clears enables outside the subtrees nova-core services, so it is a probe-time
> +    /// operation only.
> +    pub(super) fn disable_all_leaves(&self) {
> +        for index in 0..self.leaves.into_raw() {
> +            if let Some(leaf) = LeafIndex::try_new(index) {
> +                self.disable_leaf(leaf, LeafMask::all());
> +            }
> +        }
> +    }
> +
> +    /// Clears every pending bit in every implemented leaf.
> +    ///
> +    /// Disables this tree's serviced subtrees at `TOP` across the walk, then enables them,
> +    /// whatever their state on entry. The leaves cleared reach subtrees the driver does not
> +    /// service, and the `TOP_EN` writes do not.
> +    ///
> +    /// Call `drain()` only during probe. It must not run concurrently with an interrupt handler.
> +    pub(super) fn drain(&self) {
> +        self.disable_top();
> +
> +        // `TOP` summarizes enabled leaf bits, so a vector that latched while it was disabled does
> +        // not appear there.
> +        for index in 0..self.leaves.subtree_count() {
> +            for leaf in subtree_leaves(index) {

I guess you don't need this double-loop and can replace it with what
`disable_all_leaves` does?

This would let you drop `subtree_leaves` and its associated tests.
Actually it could be replaced by an iterator function returning the
leaves directly.

> +                let pending = self.read_pending(leaf);
> +                if !pending.vectors().is_empty() {
> +                    pending.clear();
> +                }
> +            }
> +        }
> +
> +        self.enable_top();
> +    }
> +}
> +
> +/// The vectors read pending from one leaf.
> +///
> +/// Holding one is the proof that the leaf was read, which is what [`Self::clear`] and
> +/// [`Self::clear_vectors`] require.
> +pub(super) struct LeafPending<'a> {
> +    tree: &'a Tree<'a>,
> +    leaf: LeafIndex,
> +    pending: LeafMask,
> +}
> +
> +impl LeafPending<'_> {
> +    /// Returns the vectors that were pending.
> +    pub(super) fn vectors(&self) -> LeafMask {
> +        self.pending
> +    }
> +
> +    /// Clears every vector that was pending, by writing its bits back (write-1-to-clear).
> +    pub(super) fn clear(&self) {
> +        self.clear_vectors(self.pending);
> +    }
> +
> +    /// Clears the vectors set in `vectors` (write-1-to-clear), leaving every other pending bit
> +    /// set.
> +    ///
> +    /// A handler that services one vector uses this rather than [`Self::clear`], which clears
> +    /// every vector the leaf had pending.
> +    pub(super) fn clear_vectors(&self, vectors: LeafMask) {
> +        if !vectors.is_empty() {
> +            if let Some(loc) = CPU_INTR_LEAF::try_at(self.leaf.get()) {
> +                self.tree.bar.write(loc, vectors.into_raw().into());
> +            }
> +        }

Here this would become:

    if !vectors.is_empty() {
        bar.write(
            Array::at(*self.leaf),
            CPU_INTR_LEAF::zeroed().with_vectors(vectors),
        );
    }