lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAA+WOBvu2Gq=SM2TBdahsQ-RVi+vn_U-oDa7-DG6kj9Arq5tpA@mail.gmail.com>
Date: Thu, 9 Oct 2025 19:51:34 +0300
From: Mohamed Ahmed <mohamedahmedegypt2001@...il.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org, 
	Mary Guillemard <mary@...y.zone>, Faith Ekstrand <faith.ekstrand@...labora.com>, 
	Lyude Paul <lyude@...hat.com>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, 
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, nouveau@...ts.freedesktop.org
Subject: Re: [PATCH 2/5] drm/nouveau/uvmm: Allow larger pages

Hey,

Thank you so much for the review! I applied everything for v2, except this one:

> Let's move the call to select_page_shift() into op_map_prepare().

How would this work? Originally when we were working on this, we did
place it in op_map_prepare() but we ran into the issue where
nouveau_uvmm_vmm_put() needed the page_shift retrieved (see
nouveau_uvmm_sm_prepare_unwind()).

Thanks.

On Mon, Oct 6, 2025 at 11:26 PM Danilo Krummrich <dakr@...nel.org> wrote:
>
> On Mon Oct 6, 2025 at 9:13 PM CEST, Mohamed Ahmed wrote:
> > From: Mary Guillemard <mary@...y.zone>
> >
> > Now that everything in UVMM knows about the variable page shift, we can
> > select larger values.
> >
> > The proposed approach rely on nouveau_bo::page unless it would cause
> > alignment issues (in which case we fall back to searching an appropriate
> > shift)
> >
> > Co-developed-by: Mohamed Ahmed <mohamedahmedegypt2001@...il.com>
> > Signed-off-by: Mohamed Ahmed <mohamedahmedegypt2001@...il.com>
> > Signed-off-by: Mary Guillemard <mary@...y.zone>
>
> NIT: Both of your tags should come after Mary's tag. The same applied to some of
> the other patches.
>
> > ---
> >  drivers/gpu/drm/nouveau/nouveau_uvmm.c | 55 +++++++++++++++++++++++++-
> >  1 file changed, 53 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> > index a92c729600d6..c336a121e320 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> > @@ -454,6 +454,56 @@ op_unmap_prepare_unwind(struct drm_gpuva *va)
> >       drm_gpuva_insert(va->vm, va);
> >  }
> >
> > +static bool
> > +op_map_aligned_to_page_shift(const struct drm_gpuva_op_map *op, u8 page_shift)
> > +{
> > +     u64 page_size = 1ULL << page_shift;
> > +
> > +     return op->va.addr % page_size == 0 && op->va.range % page_size == 0 &&
> > +                op->gem.offset % page_size == 0;
> > +}
> > +
> > +static u8
> > +select_page_shift(struct nouveau_uvmm *uvmm, struct drm_gpuva_op_map *op)
> > +{
> > +     struct nouveau_bo *nvbo = nouveau_gem_object(op->gem.obj);
> > +
> > +     if (nvbo) {
>
> In nouveau a struct drm_gpuva_op_map always has a valid GEM object set; we bail
> out if userspace gives us an invalid GEM handle.
>
> > +             /* If the BO preferred page shift already fits, use it. */
> > +             if (op_map_aligned_to_page_shift(op, nvbo->page))
> > +                     return nvbo->page;
> > +
> > +             struct nouveau_mem *mem = nouveau_mem(nvbo->bo.resource);
> > +             struct nvif_vmm *vmm = &uvmm->vmm.vmm;
> > +             int i;
> > +
> > +             /* Otherwise let's find a granuality that will fit. */
>
> Do we ever run into the "otherwise" case? nouveau_bo_fixup_align() seems to
> already ensure that your previous call will never fail?
>
> > +             for (i = 0; i < vmm->page_nr; i++) {
> > +                     /* Ignore anything that is bigger or identical to the BO preference. */
> > +                     if (vmm->page[i].shift >= nvbo->page)
> > +                             continue;
> > +
> > +                     /* Skip incompatible domains. */
> > +                     if ((mem->mem.type & NVIF_MEM_VRAM) && !vmm->page[i].vram)
> > +                             continue;
> > +                     if ((mem->mem.type & NVIF_MEM_HOST) &&
> > +                         (!vmm->page[i].host || vmm->page[i].shift > PAGE_SHIFT))
> > +                             continue;
> > +
> > +                     /* If it fits, return the proposed shift. */
> > +                     if (op_map_aligned_to_page_shift(op, vmm->page[i].shift))
> > +                             return vmm->page[i].shift;
> > +             }
> > +
> > +             /* If we get here then nothing can reconcile the requirements. This should never
> > +              * happen.
> > +              */
> > +             WARN_ON(1);
> > +     }
> > +
> > +     return PAGE_SHIFT;
> > +}
> > +
> >  static void
> >  nouveau_uvmm_sm_prepare_unwind(struct nouveau_uvmm *uvmm,
> >                              struct nouveau_uvma_prealloc *new,
> > @@ -506,7 +556,7 @@ nouveau_uvmm_sm_prepare_unwind(struct nouveau_uvmm *uvmm,
> >                       if (vmm_get_range)
> >                               nouveau_uvmm_vmm_put(uvmm, vmm_get_start,
> >                                                    vmm_get_range,
> > -                                                  PAGE_SHIFT);
> > +                                                  select_page_shift(uvmm, &op->map));
> >                       break;
> >               }
> >               case DRM_GPUVA_OP_REMAP: {
> > @@ -636,7 +686,8 @@ nouveau_uvmm_sm_prepare(struct nouveau_uvmm *uvmm,
> >               case DRM_GPUVA_OP_MAP: {
> >                       u64 vmm_get_range = vmm_get_end - vmm_get_start;
> >
> > -                     ret = op_map_prepare(uvmm, &new->map, &op->map, args, PAGE_SHIFT);
> > +                     ret = op_map_prepare(uvmm, &new->map, &op->map, args,
> > +                                          select_page_shift(uvmm, &op->map));
>
> Let's move the call to select_page_shift() into op_map_prepare().
>
> >                       if (ret)
> >                               goto unwind;
> >
> > --
> > 2.51.0
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ