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

[Joel Fernandes]

On 2/25/2026 9:26 PM, Alexandre Courbot wrote:
> 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?

Done, good idea.

> 
>>
>>>> +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,
>             }
>         }
>     }

I took this bit from  yours(more comments below).
> 
>     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,
>     }
> 
I took this bit from  yours(more comments below).

> 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(),
>     };
> 
I took this bit from  yours(more comments below).

> 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,
>     )?;

Makes sense, this is indeed better, I'll do it this way.

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

How/why would it break C users? Currently top down + range is silently ignored,
implementing it is unlikely to break them.

I also wouldn't call it speculative: top-down within a range is a natural
feature the C allocator could add right? By modeling modes as a mutually
exclusive enum, we're disallowing a flag combination that could become
valid in the future. That's fine for now, but something to keep in mind as we
choose this design. We could add a new RangeTopDown mode variant in the future,
though. That said, I've made the switch to the enum as
you suggested since it is cleaner code! And is more Rust-like as you pointed.

> 
> 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. :)
> 

Sure, no argument on that one. ;-)

[...]
>>>> +    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.

Good catch, done!

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

Changed to pub. thanks,

-- 
Joel Fernandes