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

[John Hubbard <jhubbard@nvidia.com>]

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@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.