Re: [PATCH 1/1] drm: nova: Align GEM memory allocation to system page size

[Brendan Shephard <bshephar@bne-home.net>]

On Wed, Nov 26, 2025 at 11:00:00PM +0900, Alexandre Courbot wrote:
> On Wed Nov 26, 2025 at 10:36 PM JST, Alice Ryhl wrote:
> > On Wed, Nov 26, 2025 at 10:22:14PM +0900, Alexandre Courbot wrote:
> >> On Wed Nov 26, 2025 at 6:54 PM JST, Alice Ryhl wrote:
> >> > On Wed, Nov 26, 2025 at 09:31:46AM +0900, Alexandre Courbot wrote:
> >> >> On Tue Nov 25, 2025 at 11:59 PM JST, Alice Ryhl wrote:
> >> >> > On Tue, Nov 25, 2025 at 3:55 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
> >> >> >>
> >> >> >> On Tue Nov 25, 2025 at 11:41 PM JST, Alice Ryhl wrote:
> >> >> >> > On Tue, Nov 25, 2025 at 3:29 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
> >> >> >> >>
> >> >> >> >> On Fri Nov 21, 2025 at 1:04 PM JST, bshephar wrote:
> >> >> >> >> >              return Err(EINVAL);
> >> >> >> >> >          }
> >> >> >> >> >
> >> >> >> >> > +        let aligned_size = page_align(size);
> >> >> >> >>
> >> >> >> >> `page_align` won't panic on overflow, but it will still return an
> >> >> >> >> invalid size. This is a job for `kernel::ptr::Alignment`, which let's
> >> >> >> >> you return an error when an overflow occurs.
> >> >> >> >
> >> >> >> > The Rust implementation of page_align() is implemented as (addr +
> >> >> >> > (PAGE_SIZE - 1)) & PAGE_MASK, which definitely will panic on overflow
> >> >> >> > if the appropriate config options are enabled.
> >> >> >>
> >> >> >> That's right, I skimmed its code too fast. ^_^; All the more reason to
> >> >> >> use `Alignment`.
> >> >> >
> >> >> > Alignment stores values that are powers of two, not multiples of PAGE_SIZE.
> >> >> 
> >> >> Isn't PAGE_SIZE always a power of two though?
> >> >
> >> > Yes it is. Maybe you can elaborate on how you wanted to use Alignment?
> >> > It sounds like you have something different in mind than what I thought.
> >> 
> >> I thought we could just do something like this:
> >> 
> >>     use kernel::ptr::{Alignable, Alignment};
> >> 
> >>     let aligned_size = size
> >>         .align_up(Alignment::new::<PAGE_SIZE>())
> >>         .ok_or(EOVERFLOW)?;
> >> 
> >> (maybe we could also have that `Alignment<PAGE_SIZE>` stored as a const
> >> in `page.rs` for convenience, as it might be used often)
> >
> > If you're trying to round up a number to the next multiple of PAGE_SIZE,
> > then you should use page_align() because that's exactly what the
> > function does.
> >
> > If you think there is something wrong with the design of page_align(),
> > change it instead of reimplemtning it.
> 
> In that case I would suggest that `page_align` returns an `Option`
> instead of potentially panicking. Does that sound reasonable? I cannot
> find any user of it in the Rust code for now.
> 

This sounds reasonable, and I did wonder when I read the comment on
page_align() whether it should just implement that check within the
function rather than relying on callers. But, I think changing the
signature of that function is probably something that should be done
separately to this change. I can't see anyone using it atm either, but
there could be unmerged branches or pending reviews that are using it.
If that's the path we want to go down, and it sounds completely
reasonable, I'll send that separately before posting a v2 of this patch
to use it.

Happy to take your guidance on that to satisfy the logistics of the
kernels ML driven patch approach.