linux-kernel - Re: [PATCH v2] drm/msm: Handle in-place remaps

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] [day] [month] [year] [list]

Message-ID: <CACu1E7FoQ1djuoqmjxJFHMLx1xuqsK4+a63gw34F-K8kca3-MQ@mail.gmail.com>
Date: Tue, 5 Aug 2025 17:32:02 -0400
From: Connor Abbott <cwabbott0@...il.com>
To: Rob Clark <robin.clark@....qualcomm.com>
Cc: dri-devel@...ts.freedesktop.org, linux-arm-msm@...r.kernel.org, 
	freedreno@...ts.freedesktop.org, Danilo Krummrich <dakr@...nel.org>, 
	Dmitry Baryshkov <lumag@...nel.org>, Abhinav Kumar <abhinav.kumar@...ux.dev>, 
	Jessica Zhang <jessica.zhang@....qualcomm.com>, Sean Paul <sean@...rly.run>, 
	Marijn Suijten <marijn.suijten@...ainline.org>, David Airlie <airlied@...il.com>, 
	Simona Vetter <simona@...ll.ch>, open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] drm/msm: Handle in-place remaps

On Tue, Aug 5, 2025 at 12:44 PM Rob Clark <robin.clark@....qualcomm.com> wrote:
>
> Detect and handle the special case of a MAP op simply updating the vma
> flags of an existing vma, and skip the pgtable updates.  This allows
> turnip to set the MSM_VMA_DUMP flag on an existing mapping without
> requiring additional synchronization against commands running on the
> GPU.
>
> Signed-off-by: Rob Clark <robin.clark@....qualcomm.com>

Tested that this fixes the occasional page faults with the zink a618
jobs on Mesa CI with my Mesa MR.

Tested-by: Connor Abbott <cwabbott0@...il.com>

> ---
>  drivers/gpu/drm/msm/msm_gem_vma.c | 41 ++++++++++++++++++++++++++++---
>  1 file changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
> index d1f5bb2e0a16..00d0f3b7ba32 100644
> --- a/drivers/gpu/drm/msm/msm_gem_vma.c
> +++ b/drivers/gpu/drm/msm/msm_gem_vma.c
> @@ -451,6 +451,8 @@ msm_gem_vm_bo_validate(struct drm_gpuvm_bo *vm_bo, struct drm_exec *exec)
>  struct op_arg {
>         unsigned flags;
>         struct msm_vm_bind_job *job;
> +       const struct msm_vm_bind_op *op;
> +       bool kept;
>  };
>
>  static void
> @@ -472,14 +474,18 @@ vma_from_op(struct op_arg *arg, struct drm_gpuva_op_map *op)
>  }
>
>  static int
> -msm_gem_vm_sm_step_map(struct drm_gpuva_op *op, void *arg)
> +msm_gem_vm_sm_step_map(struct drm_gpuva_op *op, void *_arg)
>  {
> -       struct msm_vm_bind_job *job = ((struct op_arg *)arg)->job;
> +       struct op_arg *arg = _arg;
> +       struct msm_vm_bind_job *job = arg->job;
>         struct drm_gem_object *obj = op->map.gem.obj;
>         struct drm_gpuva *vma;
>         struct sg_table *sgt;
>         unsigned prot;
>
> +       if (arg->kept)
> +               return 0;
> +
>         vma = vma_from_op(arg, &op->map);
>         if (WARN_ON(IS_ERR(vma)))
>                 return PTR_ERR(vma);
> @@ -599,15 +605,41 @@ msm_gem_vm_sm_step_remap(struct drm_gpuva_op *op, void *arg)
>  }
>
>  static int
> -msm_gem_vm_sm_step_unmap(struct drm_gpuva_op *op, void *arg)
> +msm_gem_vm_sm_step_unmap(struct drm_gpuva_op *op, void *_arg)
>  {
> -       struct msm_vm_bind_job *job = ((struct op_arg *)arg)->job;
> +       struct op_arg *arg = _arg;
> +       struct msm_vm_bind_job *job = arg->job;
>         struct drm_gpuva *vma = op->unmap.va;
>         struct msm_gem_vma *msm_vma = to_msm_vma(vma);
>
>         vm_dbg("%p:%p:%p: %016llx %016llx", vma->vm, vma, vma->gem.obj,
>                vma->va.addr, vma->va.range);
>
> +       /*
> +        * Detect in-place remap.  Turnip does this to change the vma flags,
> +        * in particular MSM_VMA_DUMP.  In this case we want to avoid actually
> +        * touching the page tables, as that would require synchronization
> +        * against SUBMIT jobs running on the GPU.
> +        */
> +       if (op->unmap.keep &&
> +           (arg->op->op == MSM_VM_BIND_OP_MAP) &&
> +           (vma->gem.obj == arg->op->obj) &&
> +           (vma->gem.offset == arg->op->obj_offset) &&
> +           (vma->va.addr == arg->op->iova) &&
> +           (vma->va.range == arg->op->range)) {
> +               /* We are only expecting a single in-place unmap+map cb pair: */
> +               WARN_ON(arg->kept);
> +
> +               /* Leave the existing VMA in place, but signal that to the map cb: */
> +               arg->kept = true;
> +
> +               /* Only flags are changing, so update that in-place: */
> +               unsigned orig_flags = vma->flags & (DRM_GPUVA_USERBITS - 1);
> +               vma->flags = orig_flags | arg->flags;
> +
> +               return 0;
> +       }
> +
>         if (!msm_vma->mapped)
>                 goto out_close;
>
> @@ -1268,6 +1300,7 @@ vm_bind_job_prepare(struct msm_vm_bind_job *job)
>                 const struct msm_vm_bind_op *op = &job->ops[i];
>                 struct op_arg arg = {
>                         .job = job,
> +                       .op = op,
>                 };
>
>                 switch (op->op) {
> --
> 2.50.1
>