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 <[email protected]>
>> > 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 <[email protected]>
>> >> > 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 <[email protected]>
>> >> >> > ---
>> >> >> > 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.
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)
