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
