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

[John Hubbard]

On 9/5/26 6:55 AM, Alexandre Courbot wrote:
> On Thu Sep 3, 2026 at 12:15 PM JST, John Hubbard wrote:
> <...>
> > +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.

Hi Alex,

Thanks for all these reviews! In addition to applying fixes for each item,
I'm also doing the following for the upcoming v4 spin:

a) Rebasing onto Gary's IO projections, and taking full advantage of them.

b) Fixing up all comment prose.

c) Using a type system instead of returning a bare u32 for some of the
interrupt status reading and clearing.

I'm keeping it up to date, including testing, so it's ready whenever
you get tired of looking at v3. :)

thanks,
--
John Hubbard

> > +
> > +    /// 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@xxxxxxxxxx/
>
> > +}
> > +
> > +/// 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.