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

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

On Thu Sep 3, 2026 at 12:15 PM JST, John Hubbard wrote:
<...>
> diff --git a/drivers/gpu/nova-core/irq.rs b/drivers/gpu/nova-core/irq.rs
> index d21dee1b89a0..f6ba883d72c5 100644
> --- a/drivers/gpu/nova-core/irq.rs
> +++ b/drivers/gpu/nova-core/irq.rs
> @@ -12,6 +12,23 @@
>  mod interrupt_tree;
>  mod regs;
>  
> +use kernel::{
> +    device::Bound,
> +    irq,
> +    pci::{
> +        self,
> +        IrqType, //
> +    },
> +    prelude::*, //
> +};
> +
> +use crate::num;
> +
> +use interrupt_tree::{
> +    Subtree,
> +    SubtreeSet, //
> +};
> +
>  /// The message-signaled interrupt type a vector allocation obtained.
>  ///
>  /// nova-core allocates MSI-X or MSI and nothing else, so the level-triggered INTx that
> @@ -24,3 +41,81 @@ pub(crate) enum MsiType {
>      /// One table entry per subtree.
>      MsiX,
>  }
> +
> +/// 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,
> +    /// The type [`alloc_vectors`] obtained, which fixes both the entry each subtree raises and the
> +    /// rearm write its handler owes.
> +    msi_type: MsiType,
> +}
> +
> +impl SubtreeVectors<'_> {
> +    /// Returns the interrupt type these vectors were allocated as.
> +    pub(crate) fn msi_type(&self) -> MsiType {
> +        self.msi_type
> +    }

This method is not needed. It is only used by sub-modules, which can
access the private `msi_type` directly.

> +
> +    /// Returns an [`irq::IrqRequest`] for the vector that delivers `subtree`.
> +    ///
> +    /// 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.
> +    ///
> +    /// # Errors
> +    ///
> +    /// `EINVAL` if `subtree` is not one nova-core services.
> +    pub(crate) fn request_for(&self, subtree: Subtree) -> Result<irq::IrqRequest<'_>> {

This method can be private.

> +        if !self.serviced.contains(subtree) {
> +            return Err(EINVAL);
> +        }
> +
> +        let entry = match self.msi_type {
> +            MsiType::MsiX => num::u32_as_usize(subtree.index()),
> +            MsiType::Msi => 0,
> +        };
> +
> +        self.vectors.index(entry).map(Into::into)
> +    }
> +}
> +
> +/// 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.
> +///
> +/// # 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, 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))
> +        })?;
> +
> +    Ok(SubtreeVectors {
> +        vectors,
> +        serviced,
> +        msi_type,
> +    })
> +}
> diff --git a/drivers/gpu/nova-core/irq/hal.rs b/drivers/gpu/nova-core/irq/hal.rs
> index 1ea677e37e56..07604458dbbb 100644
> --- a/drivers/gpu/nova-core/irq/hal.rs
> +++ b/drivers/gpu/nova-core/irq/hal.rs
> @@ -25,7 +25,7 @@
>          Subtree,
>          SubtreeSet, //
>      },
> -    regs,
> +    regs::*,
>      MsiType, //
>  };
>  
> @@ -63,7 +63,7 @@ pub(super) fn rearm(self, bar: Bar0<'_>, serviced: SubtreeSet, subtree: Subtree)
>          let subtrees = match self {
>              // The written value is ignored, so any write rearms delivery.
>              Self::ConfigMirrorEoi => {
> -                bar.write(regs::NV_XVE_CYA_2, 0u32.into());
> +                bar.write(NV_XVE_CYA_2, 0u32.into());
>                  return;
>              }
>              Self::TopEnableCycleServiced => serviced,
> @@ -71,10 +71,10 @@ pub(super) fn rearm(self, bar: Bar0<'_>, serviced: SubtreeSet, subtree: Subtree)
>          };
>  
>          bar.write_reg(
> -            regs::NV_VIRTUAL_FUNCTION_PRIV_CPU_INTR_TOP_EN_CLEAR::zeroed().with_subtrees(subtrees),
> +            NV_VIRTUAL_FUNCTION_PRIV_CPU_INTR_TOP_EN_CLEAR::zeroed().with_subtrees(subtrees),
>          );
>          bar.write_reg(
> -            regs::NV_VIRTUAL_FUNCTION_PRIV_CPU_INTR_TOP_EN_SET::zeroed().with_subtrees(subtrees),
> +            NV_VIRTUAL_FUNCTION_PRIV_CPU_INTR_TOP_EN_SET::zeroed().with_subtrees(subtrees),

There's a bit of unneeded churn here. Let's settle on the import style
in patch 5.

>          );
>      }
>  }
> diff --git a/drivers/gpu/nova-core/irq/interrupt_tree.rs b/drivers/gpu/nova-core/irq/interrupt_tree.rs
> index 5aa447cf0ec4..0b4dc2fc8ea8 100644
> --- a/drivers/gpu/nova-core/irq/interrupt_tree.rs
> +++ b/drivers/gpu/nova-core/irq/interrupt_tree.rs
> @@ -1,18 +1,42 @@
>  // 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,
>      prelude::*, //
>  };
>  
> -use crate::num;
> +use crate::{
> +    driver::Bar0,
> +    gpu::Chipset,
> +    num, //
> +};
> +
> +use super::{
> +    hal::{
> +        cpu_interrupt_hal,
> +        PciIrqRearmMethod, //
> +    },
> +    regs::*,
> +    MsiType, //
> +};
>  
>  /// Number of bits a leaf index occupies, covering the `0..16` leaf register arrays.
>  const LEAF_INDEX_BITS: u32 = 4;
> @@ -113,7 +137,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.
> @@ -131,7 +155,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.
> @@ -240,3 +264,262 @@ fn from(vector: GinVector) -> Self {
>          vector.0.extend()
>      }
>  }
> +
> +/// Clears the enables of the vectors set in `vectors` for `leaf` (`LEAF_EN_CLEAR`).
> +///
> +/// Shared by [`Tree::disable_leaf`] and by [`LeafEnableGuard`]'s [`Drop`], which has no tree to
> +/// reach through.
> +fn clear_leaf_enables(bar: Bar0<'_>, leaf: LeafIndex, vectors: LeafMask) {
> +    bar.write(
> +        NV_VIRTUAL_FUNCTION_PRIV_CPU_INTR_LEAF_EN_CLEAR::at(*leaf),
> +        NV_VIRTUAL_FUNCTION_PRIV_CPU_INTR_LEAF_EN_CLEAR::zeroed().with_vectors(vectors),
> +    );

Mmm, that's not the syntax I gave in my review of v2 [1].

You don't need to repeat the register name:

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

Please make sure all sites where this applies are fixed.

[1] https://lore.kernel.org/nova-gpu/DL3SD82Q6C81.3G32WDNS642Y3@nvidia.com/

> +}
> +
> +/// Clears the `TOP` enables of every subtree in `serviced` (`TOP_EN_CLEAR`).
> +fn clear_top_enables(bar: Bar0<'_>, serviced: SubtreeSet) {
> +    bar.write_reg(NV_VIRTUAL_FUNCTION_PRIV_CPU_INTR_TOP_EN_CLEAR::zeroed().with_subtrees(serviced));
> +}
> +
> +/// Clears the pending vectors set in `vectors` for `leaf` (write-1-to-clear).
> +fn clear_leaf_pending(bar: Bar0<'_>, leaf: LeafIndex, vectors: LeafMask) {
> +    if !vectors.is_empty() {
> +        bar.write(
> +            NV_VIRTUAL_FUNCTION_PRIV_CPU_INTR_LEAF::at(*leaf),
> +            NV_VIRTUAL_FUNCTION_PRIV_CPU_INTR_LEAF::zeroed().with_vectors(vectors),
> +        );
> +    }
> +}

This method is only ever used in `LeafPending::clear_vectors`, so let's
inline it there.

> +
> +/// Returns every leaf a tree of `leaves` leaves implements.
> +fn implemented_leaves(leaves: LeafCount) -> impl Iterator<Item = LeafIndex> {
> +    (0..leaves.into_raw()).filter_map(LeafIndex::try_new)
> +}

This looks like it should be a method of `LeafCount`. In this case, I
guess the name can be simply `iter`.

<...>
> +    /// Clears every pending bit in every implemented leaf.
> +    ///
> +    /// Disables this tree's serviced subtrees at `TOP` for the walk and leaves them disabled, so a
> +    /// caller that wants delivery enables them itself once it is ready to receive. The leaves
> +    /// cleared reach subtrees the driver does not service, and the `TOP_EN` write does 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 leaf in implemented_leaves(self.leaves) {
> +            let pending = self.read_pending(leaf);
> +            if !pending.vectors().is_empty() {

`clear` already does the same check (through the now-inlined
`clear_leaf_pending`), so it is redundant here.