Re: [PATCH 06/17] gpu: nova-core: add the GIN interrupt tree API

On Sat Aug 8, 2026 at 5:11 AM CEST, John Hubbard wrote:
> From: Joel Fernandes <[email protected]>
>
> 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.
>
> Add an API for one PCIe function's CPU interrupt tree. The leaf handle
> carries that order as a type state, so the wrong order does not compile.
>
> The CPU doorbell self-test added later in this series is the first user.
>
> Reviewed-by: Will Pierce <[email protected]>
> Signed-off-by: Joel Fernandes <[email protected]>
> [jhubbard: use the canonical NV_VIRTUAL_FUNCTION_PRIV_CPU_INTR_*
>  register names, name the module interrupt_tree with a Tree type, drop
>  the type state from the Top handle, take the leaf count from the
>  chipset, define the vector encoding here, reject a trigger for a vector
>  outside the tree, 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 <[email protected]>

It would make more sense if this patch is merged into patch 8 after the HAL is
introduced in patch 7.

(The newtypes mentioned below need their own commit first though, as they'd also
be used by the HAL.)

> +/// Index of a leaf register, bounded to the `0..16` range covered by the leaf register arrays.
> +pub(super) type LeafIndex = Bounded<usize, 4>;
> +
> +/// Maps an interrupt `vector` to its position in the tree: the leaf that carries it
> +/// (`vector / 32`) and the bit index within that leaf (`vector % 32`).
> +///
> +/// The returned leaf is a raw index. [`LeafIndex::try_new`] bounds it to the leaf register
> +/// arrays, and the architecture's leaf count is a separate, narrower bound.
> +pub(super) const fn vector_leaf_bit(vector: u32) -> (usize, u32) {
> +    (crate::num::u32_as_usize(vector / 32), vector % 32)
> +}
> +
> +/// Maps an interrupt `vector` to the `TOP` enable mask of the subtree that carries it.
> +///
> +/// A subtree covers two adjacent leaves, so the vector's leaf is in subtree `vector / 64`. The
> +/// result has that subtree's bit set, in the form `TOP_EN_SET` and `TOP_EN_CLEAR` take as a
> +/// value.
> +///
> +/// The result is not validated against the subtrees that the architecture supports.
> +pub(super) const fn vector_subtree_mask(vector: u32) -> u32 {
> +    1 << (vector / 64)
> +}

I think we use new types for those. I'm thinking of:

	struct GinVector(u32);

	impl GinVector {
	    const fn leaf_index(&self) -> LeafIndex { ... }
	    const fn leaf_mask(&self) -> LeafMask { ... }
	    const fn subtree(&self) -> Subtree { ... }
	}

With additional new types LeafMask and Subtree.

This removes the need for

	const GSP_LEAF: usize = GSP_LOC.0;
	const GSP_BIT: u32 = 1 << GSP_LOC.1;

and subsequent

	LeafIndex::new::<GSP_LEAF>()

Subtree should represent a single TOP bit produced by GinVector::subtree().

This way Subtree already carries the invariant we need and we get rid of the
runtime count_ones() check in SubtreeVectors::request_for().

The semantics of serviced is different and can be represented by a SubtreeSet
type, so the check becomes serviced.contains(subtree).

Now, there's already an existing Subtree type, which represents an index. But I
think we should just get rid of it, as it doesn't really add any value. The two
methods it implements, iter_leaves() and iter_pending_leaves(), are rather Tree
methods.

If we add Subtree::index() they can still take a Subtree argument, but I'm not
sure it's worth. They are only called by drain(), which creates this new type
from a raw value, just to immediately convert it back to a raw value. So, here
I'd just work with the raw value.

> +
> +/// Type state of a [`Leaf`] handle: `Idle` before its pending bits are read, `Pending` after.
> +pub(super) trait State: private::Sealed {}
> +
> +/// State in which the handle holds no pending bits.
> +pub(super) struct Idle;
> +impl State for Idle {}
> +
> +/// State holding the pending bits read from hardware.
> +pub(super) struct Pending {
> +    pending_bits: u32,
> +}
> +impl State for Pending {}
> +
> +mod private {
> +    pub(in crate::irq) trait Sealed {}

This can just be pub.

> +    impl Sealed for super::Idle {}
> +    impl Sealed for super::Pending {}
> +}
> +
> +/// The GIN CPU interrupt tree for a single PCIe function.
> +#[derive(Clone)]

I think we don't need Clone on this, I know it is used in the doorbell test, but
we can simplify the doorbell test significantly by getting rid of the custom
SelftestGuard and replace it with

	struct SelftestResources<'a, 'r> {
	    _leaf_guard: LeafEnableGuard<'a>,
	    reg: Pin<KBox<irq::Registration<'r, DoorbellTestHandler<'a>>>>,
	    _top_guard: TopEnableGuard<'a>,
	}

Note that a LeafEnableGuard will also be very useful for the GspIrq introduced
later.

	pub(crate) struct GspIrq<'a> {
	    #[pin]
	    reg: irq::ThreadedRegistration<'a, GspInterrupt<'a>>,
	    bar: Bar0<'a>,
	    tree: Tree,
	}

just becomes

	pub(crate) struct GspIrq<'a> {
	    _leaf_guard: LeafEnableGuard<'a>,
	    #[pin]
	    reg: irq::ThreadedRegistration<'a, GspInterrupt<'a>>,
	}

And the open-coded destructor of GspIrq goes away.

> +pub(super) struct Tree {
> +    /// Number of implemented leaves in this tree, either 8 or 16.
> +    num_leaves: usize,

If it is really just one or the other, maybe worth to consider something like:

	#[repr(usize)]
	enum LeafCount {
	    Eight = 8,
	    Sixteen = 16,
	}
	
	impl LeafCount {
	    const fn into_raw(self) -> usize {
	        self as usize
	    }
	
	    const fn subtree_count(self) -> usize {
	        self.into_raw() / 2
	    }

	    const fn subtree_set(self) -> SubtreeSet {
	        SubtreeSet((1u32 << self.subtree_count()) - 1)
	    }
	}

A HAL can then only ever pick an expected leaf count.

> +    /// Mask of subtree bits the architecture implements.
> +    subtree_mask: u32,

This should be SubtreeSet.

> +}
> +
> +impl Tree {
> +    /// Creates a `Tree` sized for `chipset`.
> +    pub(super) fn new(chipset: Chipset) -> Self {
> +        let num_leaves = match chipset.arch() {
> +            Architecture::Turing | Architecture::Ampere | Architecture::Ada => 8,
> +            Architecture::Hopper | Architecture::BlackwellGB10x | Architecture::BlackwellGB20x => {
> +                16
> +            }
> +        };
> +
> +        Self {
> +            num_leaves,
> +            // Each subtree covers two leaves, so one bit per pair of leaves.
> +            subtree_mask: (1u32 << (num_leaves / 2)) - 1,
> +        }
> +    }
> +
> +    /// Returns a [`Top`] handle for this tree.
> +    pub(super) fn top(&self) -> Top {
> +        Top {
> +            subtree_mask: self.subtree_mask,
> +        }
> +    }
> +
> +    /// Returns a [`Leaf`] handle in the [`Idle`] state for `index`.
> +    pub(super) fn leaf(&self, index: LeafIndex) -> Leaf<Idle> {
> +        Leaf::from_index(index)
> +    }
> +
> +    /// Injects a software interrupt for `vector` via the trigger register.
> +    ///
> +    /// # Errors
> +    ///
> +    /// `EINVAL` if `vector` lies outside this tree (`vector >= num_leaves * 32`). `EOVERFLOW` if
> +    /// `vector` does not fit in the trigger register's vector field.
> +    pub(super) fn trigger(&self, bar: Bar0<'_>, vector: u32) -> Result {

Most methods take a Bar0 argument. I think it would be cleaner if the Tree
constructor would just take a Bar0 and store it within the tree.

In case this gets into the way of some tests, we can also have a TreeInner type.

> +        if crate::num::u32_as_usize(vector) >= self.num_leaves * 32 {

This check could be replaced by:

	impl GinVector {
	    const fn validate(self, leaves: LeafCount) -> Result {
	        if self.as_raw() >= leaves.vector_count() { return Err(EINVAL) }
	        Ok(())
	    }
	}

so this becomes

	vector.validate(self.leaf_count)?;

> +/// View of a single interrupt leaf.
> +pub(super) struct Leaf<S: State = Idle> {
> +    index: LeafIndex,
> +    state: S,
> +}
> +
> +// The `try_at(...)` calls below cannot fail: `LeafIndex` is `Bounded<usize, 4>`, so its value is
> +// in 0..16, and every leaf register array has 16 elements.
> +impl Leaf<Idle> {
> +    /// Creates a [`Leaf`] handle for `index`.
> +    pub(super) fn from_index(index: LeafIndex) -> Self {
> +        Leaf { index, state: Idle }
> +    }
> +
> +    /// Enables the vectors set in `vectors` for this leaf (`LEAF_EN_SET`).
> +    ///
> +    /// This is the per-vector counterpart of [`Top::enable`], which enables a whole subtree.
> +    pub(super) fn enable(&self, bar: Bar0<'_>, vectors: u32) {
> +        if let Some(loc) = CPU_INTR_LEAF_EN_SET::try_at(self.index.get()) {
> +            bar.write(loc, vectors.into());
> +        }
> +    }
> +
> +    /// Disables the vectors set in `vectors` for this leaf (`LEAF_EN_CLEAR`).
> +    pub(super) fn disable(&self, bar: Bar0<'_>, vectors: u32) {
> +        if let Some(loc) = CPU_INTR_LEAF_EN_CLEAR::try_at(self.index.get()) {
> +            bar.write(loc, vectors.into());
> +        }
> +    }

IIUC, the type state exists only to guard clear_pending() from being called
before read_pending() has been called?

In this case, enable() and disable() are orthogonal and do not participate in
the state machine.

If that holds, I'd probably move enable(), disable() and enable_guarded() to
Tree or just remove the type state and make Leaf<Pending> a new type. (One can
obtain a new  Leaf<Idle> while still having a Leaf<Pending> anyways.)

Also, does the type state really properly fulfill its purpose? Once I called
read_pending() once I can hold on to Leaf<Pending> for as long as I want and
call clear_pending() for as often as I want, right?

I think the idea is that the read_pending() then clear_pending() sequence must
run within the interrupt handler?

So, I assume that we rather want read_pending() to take a token that only lives
for irq::ThreadedHandler::handle() and have a LeafPending that borrows from
this token?

I wonder if the core IRQ code should provide such a token in handle() and
handle_threaded().

This way we can enforce that certain things can only be executed in an IRQ
handler and that certain sequences like read and clear pending must complete
within a single execution of an IRQ.

> +
> +    /// Reads this leaf's pending bits and transitions to [`Pending`].
> +    pub(super) fn read_pending(self, bar: Bar0<'_>) -> Leaf<Pending> {
> +        let pending_bits = CPU_INTR_LEAF::try_at(self.index.get())
> +            .map(|loc| bar.read(loc).into_raw())
> +            .unwrap_or(0);
> +        Leaf {
> +            index: self.index,
> +            state: Pending { pending_bits },
> +        }
> +    }
> +}
> +
> +impl Leaf<Pending> {
> +    /// Returns the pending bits read from hardware.
> +    pub(super) fn pending_bits(&self) -> u32 {
> +        self.state.pending_bits
> +    }
> +
> +    /// Clears every pending vector by writing its bits back (write-1-to-clear).
> +    pub(super) fn clear_pending(&self, bar: Bar0<'_>) {
> +        if self.state.pending_bits != 0 {
> +            if let Some(loc) = CPU_INTR_LEAF::try_at(self.index.get()) {
> +                bar.write(loc, self.state.pending_bits.into());
> +            }
> +        }
> +    }
> +}