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

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

On Sat Sep 12, 2026 at 5:43 AM BST, John Hubbard wrote:
<...>
> +/// Allocates the PCI interrupt vectors for the subtrees in `serviced`.
> +///
> +/// Requests MSI-X entries `0` through the highest subtree in `serviced`, since an allocation
> +/// cannot be sparse, and falls back to a single MSI message for the whole tree.
> +///
> +/// # Errors
> +///
> +/// `EINVAL` if `serviced` is empty. Otherwise, when neither type could be allocated, the error
> +/// from the MSI request.
> +pub(crate) fn alloc_vectors(
> +    pdev: &pci::Device<Bound>,
> +    serviced: SubtreeSet,
> +) -> Result<SubtreeVectors<'_>> {
> +    if serviced.is_empty() {
> +        return Err(EINVAL);
> +    }
> +
> +    let entries = serviced.span();
> +
> +    let (vectors, msi_type) = pdev
> +        .alloc_irq_vectors(entries, entries, IrqType::MsiX.into())
> +        .map(|vectors| (vectors, MsiType::MsiX))
> +        .or_else(|_| {
> +            pdev.alloc_irq_vectors(1, 1, IrqType::Msi.into())
> +                .map(|vectors| (vectors, MsiType::Msi))
> +        })?;

The logic here would benefit from a comment explaining why we only need
one vector in the MSI case, or a reference to the documentation.

<...>
> diff --git a/drivers/gpu/nova-core/irq/interrupt_tree.rs b/drivers/gpu/nova-core/irq/interrupt_tree.rs
> index 5c7829ea3bc5..2583b006019e 100644
> --- a/drivers/gpu/nova-core/irq/interrupt_tree.rs
> +++ b/drivers/gpu/nova-core/irq/interrupt_tree.rs
> @@ -1,22 +1,40 @@
>  // 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 [`GinVector`] names an interrupt source, a [`LeafIndex`] the leaf register that latches it,
> -//! a [`LeafMask`] a set of vectors within one leaf, and a [`Subtree`] one `TOP` bit. The types
> -//! keep the four from being confused with one another.

Why remove this last sentence?

> +//! a [`LeafMask`] a set of vectors within one leaf, and a [`Subtree`] one `TOP` bit.
> +//!
> +//! Servicing a leaf requires reading its pending bits before clearing them. Only
> +//! [`Tree::read_pending`] produces a [`LeafPending`], and only a [`LeafPending`] clears a leaf,
> +//! so the wrong order does not compile. Nothing in this module serializes access to the tree.
>  //!
>  //! See `Documentation/gpu/nova/core/interrupts.rst`.
>  
>  use kernel::{
> +    io::{
> +        register::Array,
> +        Io, //
> +    },
>      num::Bounded,
>      prelude::*, //
>  };
>  
> -use crate::num;
> +use crate::{
> +    driver::Bar0,
> +    gpu::Chipset,
> +    num, //
> +};
>  
> -use super::regs::*;
> +use super::{
> +    hal::{
> +        cpu_interrupt_hal,
> +        PciIrqRearmMethod, //
> +    },
> +    regs::*,
> +    SubtreeVectors, //
> +};
>  
>  /// Number of vectors one leaf register carries, one per bit.
>  const VECTORS_PER_LEAF: u32 = u32::BITS;
> @@ -78,6 +96,11 @@ pub(super) const fn subtree_set(self) -> SubtreeSet {
>      pub(super) const fn vector_count(self) -> u32 {
>          self.into_u32() * VECTORS_PER_LEAF
>      }
> +
> +    /// Returns every leaf a tree of this size implements.
> +    pub(super) fn iter(self) -> impl Iterator<Item = LeafIndex> {
> +        (0..self.into_raw()).filter_map(LeafIndex::try_new)
> +    }
>  }
>  
>  // `VECTOR_BITS` and `LeafCount::Sixteen` are written separately. This assert keeps them in
> @@ -130,7 +153,7 @@ fn from(vectors: LeafMask) -> Self {
>  ///
>  /// Exactly one bit is set.
>  #[derive(Clone, Copy, Debug, Eq, PartialEq)]
> -pub(super) struct Subtree(u32);
> +pub(crate) struct Subtree(u32);
>  
>  impl Subtree {
>      /// Returns the subtree at index `idx`.
> @@ -151,7 +174,7 @@ pub(super) const fn into_raw(self) -> u32 {
>  
>  /// Set of subtrees, one bit per subtree, in the layout of the `TOP` registers.
>  #[derive(Clone, Copy, Debug, Eq, PartialEq)]
> -pub(super) struct SubtreeSet(u32);
> +pub(crate) struct SubtreeSet(u32);
>  
>  impl SubtreeSet {
>      pub(super) const fn contains(self, subtree: Subtree) -> bool {
> @@ -250,3 +273,218 @@ fn from(vector: GinVector) -> Self {
>          vector.0.extend()
>      }
>  }
> +
> +/// Disables `vectors` in `leaf`.
> +fn clear_leaf_enables(bar: Bar0<'_>, leaf: LeafIndex, vectors: LeafMask) {
> +    bar.write(
> +        Array::at(*leaf),
> +        NV_VIRTUAL_FUNCTION_PRIV_CPU_INTR_LEAF_EN_CLEAR::zeroed().with_vectors(vectors),
> +    );
> +}
> +
> +/// Disables the subtrees in `serviced` at `TOP`.
> +fn clear_top_enables(bar: Bar0<'_>, serviced: SubtreeSet) {
> +    bar.write_reg(NV_VIRTUAL_FUNCTION_PRIV_CPU_INTR_TOP_EN_CLEAR::zeroed().with_subtrees(serviced));
> +}
> +
> +/// The CPU tree of one PCIe function, and the subtrees that nova-core services.
> +pub(super) struct Tree<'a> {
> +    bar: Bar0<'a>,
> +    leaves: LeafCount,

naming: num_leaves?

> +    serviced: SubtreeSet,
> +    rearm: PciIrqRearmMethod,

naming: rearm_method.

> +}
> +
> +impl<'a> Tree<'a> {
> +    /// Creates the tree of `chipset`, covering the subtrees that `vectors` services.
> +    ///
> +    /// # Errors
> +    ///
> +    /// `EINVAL` if `chipset` does not implement every subtree that `vectors` services.
> +    pub(super) fn new(
> +        bar: Bar0<'a>,
> +        chipset: Chipset,
> +        vectors: &SubtreeVectors<'_>,
> +    ) -> Result<Self> {
> +        let hal = cpu_interrupt_hal(chipset);
> +        let leaves = hal.leaf_count();
> +        let serviced = vectors.serviced;
> +
> +        if serviced.intersection(leaves.subtree_set()) != serviced {
> +            return Err(EINVAL);
> +        }
> +
> +        Ok(Self {
> +            bar,
> +            leaves,
> +            serviced,
> +            rearm: hal.pci_irq_rearm_method(vectors.msi_type),
> +        })
> +    }
> +
> +    /// Rearms PCI interrupt delivery to the CPU after servicing `subtree`, the one subtree that
> +    /// the calling handler serves.
> +    ///
> +    /// A handler must call this before returning, or it receives no further interrupts.
> +    pub(super) fn rearm_pci_irq(&self, subtree: Subtree) {
> +        self.rearm.rearm(self.bar, self.serviced, subtree);
> +    }
> +
> +    /// Enables the serviced subtrees at `TOP`.
> +    ///
> +    /// Each of them must have a handler registered on its PCI vector.
> +    pub(super) fn enable_top(&self) {
> +        self.bar.write_reg(
> +            NV_VIRTUAL_FUNCTION_PRIV_CPU_INTR_TOP_EN_SET::zeroed().with_subtrees(self.serviced),
> +        );
> +    }
> +
> +    /// Disables the serviced subtrees at `TOP`.
> +    pub(super) fn disable_top(&self) {
> +        clear_top_enables(self.bar, self.serviced);
> +    }
> +
> +    /// Enables the serviced subtrees at `TOP` until the returned guard drops.
> +    pub(super) fn enable_top_guarded(&self) -> TopEnableGuard<'a> {
> +        self.enable_top();
> +
> +        TopEnableGuard {
> +            bar: self.bar,
> +            serviced: self.serviced,
> +        }
> +    }
> +
> +    /// Enables `vectors` in `leaf`.
> +    pub(super) fn enable_leaf(&self, leaf: LeafIndex, vectors: LeafMask) {
> +        self.bar.write(
> +            Array::at(*leaf),
> +            NV_VIRTUAL_FUNCTION_PRIV_CPU_INTR_LEAF_EN_SET::zeroed().with_vectors(vectors),
> +        );
> +    }
> +
> +    /// Disables `vectors` in `leaf`.
> +    pub(super) fn disable_leaf(&self, leaf: LeafIndex, vectors: LeafMask) {
> +        clear_leaf_enables(self.bar, leaf, vectors);
> +    }
> +
> +    /// Enables `vectors` in `leaf` until the returned guard drops.
> +    pub(super) fn enable_leaf_guarded(
> +        &self,
> +        leaf: LeafIndex,
> +        vectors: LeafMask,
> +    ) -> LeafEnableGuard<'a> {
> +        self.enable_leaf(leaf, vectors);
> +
> +        LeafEnableGuard {
> +            bar: self.bar,
> +            leaf,
> +            vectors,
> +        }
> +    }
> +
> +    /// Reads the pending bits of `leaf`, and returns the handle that clears them.
> +    pub(super) fn read_pending(&self, leaf: LeafIndex) -> LeafPending<'a> {
> +        let pending = self
> +            .bar
> +            .read(NV_VIRTUAL_FUNCTION_PRIV_CPU_INTR_LEAF::at(*leaf))
> +            .vectors();
> +
> +        LeafPending {
> +            bar: self.bar,
> +            leaf,
> +            pending,
> +        }
> +    }
> +
> +    /// Latches `vector` as its own source would.
> +    ///
> +    /// # Errors
> +    ///
> +    /// `EINVAL` if this tree does not implement `vector`.
> +    // The interrupt self-test is the only caller.

Why this non-doc comment? We don't annotate every method with their list
of callers (and it will become obvious with the conditional dead_code of
the next patch) so let's remove it.

> +    #[expect(dead_code)]
> +    pub(super) fn trigger(&self, vector: GinVector) -> Result {

Let's add this method with the doorbell test so we can avoid the
temporary dead_code.