Re: [PATCH v11 4/4] rust: gpu: Add GPU buddy allocator bindings

[Alexandre Courbot]

On Thu Feb 26, 2026 at 5:41 AM JST, Joel Fernandes wrote:
>> This structure doesn't seem to be useful. I would understand using one
>> if `GpuBuddyParams` had lots of members, some of which have a sensible
>> default value - then we could implement `Default` and let users fill in
>> the parameters they need.
>>
>> But this structure has no constructor of any sort, requiring users to
>> fill its 3 members manually - which is actually heavier than having 3
>> parameters to `GpuBuddy::new`. It is even deconstructed in
>> `GpuBuddyInner` to store its members as 3 different fields! So let's
>> skip it.
>
> I'd prefer to keep the struct -- all three parameters are `u64`, so
> positional arguments would be easy to silently misorder. The struct
> also makes call sites more readable since Rust has no named function call
> parameters.

Fair point about the 3 parameters being easily confused. If you keep it,
can you also store it in `GpuBuddyInner` instead of deconstructing it
into 3 members?

>
>>> +pub struct GpuBuddyAllocParams {
>>
>> This one also feels like it could be rustified some more.
>>
>> By this I mean that it e.g. allows the user to specify a range even if
>> `RANGE_ALLOCATION` is not set. A C API rejects invalid combinations at
>> runtime. A Rust API should make it impossible to even express them.
>>
>> [...]
>>
>> That would turn `alloc_blocks` into something like:
>>
>>   `fn alloc_blocks(&self, alloc: AllocType, size: u64, min_block_size: Alignment, flags: AllocBlocksFlags)`
>
> The C API supports combining allocation modes with modifiers (e.g.
> RANGE+CLEAR, TOPDOWN+CLEAR), so modeling the mode as a
> mutually-exclusive enum would lose valid combinations. More importantly,

What I suggested does allow you to combine allocation modes with
modifiers. I should have pasted a bit of code for clarity, so here goes:

    #[derive(Copy, Clone, Debug, PartialEq, Eq)]
    pub enum GpuBuddyAllocMode {
        Simple,
        Range { start: u64, end: u64 },
        TopDown,
    }

    impl GpuBuddyAllocMode {
        // Returns the flag corresponding to the allocation mode.
        //
        // Intentionally private - for internal use.
        fn into_flags(self) -> usize {
            match self {
                Self::Simple => 0,
                Self::Range { .. } => bindings::GPU_BUDDY_RANGE_ALLOCATION,
                Self::TopDown => bindings::GPU_BUDDY_TOPDOWN_ALLOCATION,
            }
        }
    }

    impl_flags!(
        #[derive(Copy, Clone, PartialEq, Eq, Default)]
        pub struct GpuBuddyAllocFlags(u32);

        #[derive(Copy, Clone, PartialEq, Eq)]
        pub enum GpuBuddyAllocFlag {
            Contiguous = bindings::GPU_BUDDY_CONTIGUOUS_ALLOCATION as u32,
            Clear = bindings::GPU_BUDDY_CLEAR_ALLOCATION as u32,
            TrimDisable = bindings::GPU_BUDDY_TRIM_DISABLE as u32,
        }
    );

    pub struct GpuBuddyAllocParams {
        mode: GpuBuddyAllocMode,
        size: u64,
        min_block_size: u64,
        flags: GpuBuddyAllocFlags,
    }

Now instead of doing something like:

    let params = GpuBuddyAllocParams {
        start_range_address: 0,
        end_range_address: 0,
        size: SZ_16M as u64,
        min_block_size: SZ_16M as u64,
        buddy_flags: BuddyFlag::TopdownAllocation.into(),
    };

You would have:

    let params = GpuBuddyAllocParams {
        // No unneeded `start_range` and `end_range`!
        mode: GpuBuddyAllocMode::TopDown,
        size: SZ_16M as u64,
        min_block_size: SZ_16M as u64,
        flags: Default::default(),
    };

And for a cleared range allocation:

    let params = GpuBuddyAllocParams {
        mode: GpuBuddyAllocMode::Range {
            start: 0,
            end: SZ_16M as u64,
        },
        size: SZ_16M as u64,
        min_block_size: SZ_16M as u64,
        flags: GpuBuddyAllocFlag::Clear,
    };

Actually the parameters are now distinct enough that you don't need a
type to prevent confusion. A block allocation now just reads like a nice
sentence:

    buddy.alloc_blocks(
        GpuBuddyAllocMode::Range {
            start: 0,
            end: SZ_16M,
        },
        SZ_16M,
        // `min_block_size` should be an `Alignment`, the C API even
        // returns an error if it is not a power of 2.
        Alignment::new::<SZ_16M>(),
        GpuBuddyAllocFlag::Clear,
    )?;

And the job of `alloc_blocks` is also simplified:

    let (start, end) = match mode {
        GpuBuddyAllocMode::Range { start, end } => (start, end),
        _ => (0, 0),
    };
    let flags = mode.into_flags() | u32::from(flags) as usize;
    // ... and just invoke the C API with these parameters.

> if the C allocator evolves its flag semantics (new combinations become
> valid, or existing constraints change), an enum on the Rust side would
> break. It's simpler and more maintainable to pass combinable flags and
> let the C allocator validate -- which it already does. The switch to
> `impl_flags!` will work for us without over-constraining.

The evolution you describe is speculative and unlikely to happen as it
would break all C users just the same. And if the C API adds new flags
or allocation modes, we will have to update the Rust abstraction either
way.

Rust abstractions should model the C API correctly. By hardening the way
the C API can be used and stripping out invalid uses, we save headaches
to users of the API who don't need to worry about whether the flag they
pass will result in an error or simply be ignored, and we also save
maintainer time who don't have to explain the intricacies of their APIs
to confused users. :)

>
>>> +/// The internal [`GpuBuddyGuard`] ensures that the lock is held for all
>>
>> `GpuBuddyGuard` is exported and not internal though.
>
> Fixed, removed "internal" wording.
>
>>> +    base_offset: u64,
>>
>> This does not appear to be used in the C API - does it belong here? It
>> looks like an additional convenience, but I'm not convinced that's the
>> role of this type to provide this. But if it really is needed by all
>> users (guess I'll find out after looking the Nova code :)), then keeping
>> it is fair I guess.
>
> Yes, `base_offset` is needed by nova-core. The GPU's usable VRAM
> starts at `usable_vram_start` from the GSP firmware parameters:
>
>     GpuBuddyParams {
>         base_offset: params.usable_vram_start,
>         physical_memory_size: params.usable_vram_size,
>         chunk_size: SZ_4K.into_safe_cast(),
>     }
>
> `AllocatedBlock::offset()` then adds `base_offset` to return absolute
> VRAM addresses, so callers don't need to track the offset themselves.

Sounds fair, I'll check how this is used in Nova.

Ah, another thing I've noticed while writing the example above:

> +#[pinned_drop]
> +impl PinnedDrop for AllocatedBlocks {
> +    fn drop(self: Pin<&mut Self>) {
> +        let guard = self.buddy.lock();
> +
> +        // SAFETY:
> +        // - list is valid per the type's invariants.
> +        // - guard provides exclusive access to the allocator.
> +        // CAST: BuddyFlags were validated to fit in u32 at construction.
> +        unsafe {
> +            bindings::gpu_buddy_free_list(
> +                guard.as_raw(),
> +                self.list.as_raw(),
> +                self.flags.as_raw() as u32,

`gpu_buddy_free_list` only expects the `CLEARED` flag - actually it
silently masks other flags. So you probably want to just pass `0` here -
adding a `Cleared` field to `GpuBuddyAllocFlag` would also do the trick,
but it looks risky to me as it relies on the promise that the user has
cleared the buffer, which is not something we can guarantee. So I don't
think we can support this safely.

If you just pass `0`, then the `flags` member of `AllocatedBlocks`
becomes unused and you can just drop it.

And another small one - some methods of `Block` are `pub(crate)` - I
believe they should either be `pub` or kept private.