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

[Alice Ryhl <aliceryhl@google.com>]

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:
> >> >> > Use page::page_align for GEM object memory allocation to ensure the
> >> >> > allocation is page aligned. This ensures that the allocation is page
> >> >> > aligned with the system in cases where 4096 is not the default.
> >> >> > For example on 16k or 64k aarch64 systems this allocation should be
> >> >> > aligned accordingly.
> >> >> >
> >> >> > Signed-off-by: Brendan Shephard <bshephar@bne-home.net>
> >> >> > ---
> >> >> >  drivers/gpu/drm/nova/gem.rs | 11 ++++++++---
> >> >> >  1 file changed, 8 insertions(+), 3 deletions(-)
> >> >> >
> >> >> > diff --git a/drivers/gpu/drm/nova/gem.rs b/drivers/gpu/drm/nova/gem.rs
> >> >> > index 2760ba4f3450..a07e922e25ef 100644
> >> >> > --- a/drivers/gpu/drm/nova/gem.rs
> >> >> > +++ b/drivers/gpu/drm/nova/gem.rs
> >> >> > @@ -3,6 +3,10 @@
> >> >> >  use kernel::{
> >> >> >      drm,
> >> >> >      drm::{gem, gem::BaseObject},
> >> >> > +    page::{
> >> >> > +        page_align,
> >> >> > +        PAGE_SIZE, //
> >> >> > +    },
> >> >> >      prelude::*,
> >> >> >      sync::aref::ARef,
> >> >> >  };
> >> >> > @@ -27,12 +31,13 @@ fn new(_dev: &NovaDevice, _size: usize) -> impl PinInit<Self, Error> {
> >> >> >  impl NovaObject {
> >> >> >      /// Create a new DRM GEM object.
> >> >> >      pub(crate) fn new(dev: &NovaDevice, size: usize) -> Result<ARef<gem::Object<Self>>> {
> >> >> > -        let aligned_size = size.next_multiple_of(1 << 12);
> >> >> > -
> >> >> > -        if size == 0 || size > aligned_size {
> >> >> > +        // Check for 0 size or potential usize overflow before calling page_align
> >> >> > +        if size == 0 || size > usize::MAX - PAGE_SIZE + 1 {
> >> >>
> >> >> `PAGE_SIZE` here is no more correct than the hardcoded `1 << 12` - well,
> >> >> I'll admit it looks better as a placeholder. :) But the actual alignment
> >> >> will eventually be provided elsewhere.
> >> >
> >> > What about kernels with 16k pages?
> >>
> >> The actual alignment should IIUC be a mix of the GPU and kernel's
> >> requirements (GPU can also use a different page size). So no matter what
> >> we pick right now, it won't be great but you are right that PAGE_SIZE
> >> will at least accomodate the kernel.
> >
> > In that case, is PAGE_SIZE not the wrong constant? What's the actually
> > correct constant here?
> >
> >> >> >              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.

Alice