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]
Date:   Wed, 30 Mar 2022 02:42:34 +0300
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Rob Clark <robdclark@...il.com>
Cc:     dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org,
        linux-arm-msm@...r.kernel.org, Rob Clark <robdclark@...omium.org>,
        Sean Paul <sean@...rly.run>,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Akhil P Oommen <quic_akhilpo@...cinc.com>,
        Jordan Crouse <jordan@...micpenguin.net>,
        Jonathan Marek <jonathan@...ek.ca>,
        Emma Anholt <emma@...olt.net>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 9/9] drm/msm: Add a way for userspace to allocate GPU iova

On Wed, 30 Mar 2022 at 02:00, Rob Clark <robdclark@...il.com> wrote:
>
> From: Rob Clark <robdclark@...omium.org>
>
> The motivation at this point is mainly native userspace mesa driver in a
> VM guest.  The one remaining synchronous "hotpath" is buffer allocation,
> because guest needs to wait to know the bo's iova before it can start
> emitting cmdstream/state that references the new bo.  By allocating the
> iova in the guest userspace, we no longer need to wait for a response
> from the host, but can just rely on the allocation request being
> processed before the cmdstream submission.  Allocation faulures (OoM,

failures

> etc) would just be treated as context-lost (ie. GL_GUILTY_CONTEXT_RESET)
> or subsequent allocations (or readpix, etc) can raise GL_OUT_OF_MEMORY.
>
> Signed-off-by: Rob Clark <robdclark@...omium.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>

Minor nits (above and below).

> ---
>  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 10 ++++++
>  drivers/gpu/drm/msm/msm_drv.c           | 21 +++++++++++
>  drivers/gpu/drm/msm/msm_gem.c           | 48 +++++++++++++++++++++++++
>  drivers/gpu/drm/msm/msm_gem.h           |  8 +++++
>  drivers/gpu/drm/msm/msm_gem_vma.c       |  2 ++
>  include/uapi/drm/msm_drm.h              |  3 ++
>  6 files changed, 92 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 6385ab06632f..4caae0229518 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -281,6 +281,16 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
>         case MSM_PARAM_SUSPENDS:
>                 *value = gpu->suspend_count;
>                 return 0;
> +       case MSM_PARAM_VA_START:
> +               if (ctx->aspace == gpu->aspace)
> +                       return -EINVAL;
> +               *value = ctx->aspace->va_start;
> +               return 0;
> +       case MSM_PARAM_VA_SIZE:
> +               if (ctx->aspace == gpu->aspace)
> +                       return -EINVAL;
> +               *value = ctx->aspace->va_size;
> +               return 0;
>         default:
>                 DBG("%s: invalid param: %u", gpu->name, param);
>                 return -EINVAL;
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index a5eed5738ac8..7394312cf075 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -719,6 +719,23 @@ static int msm_ioctl_gem_info_iova(struct drm_device *dev,
>         return msm_gem_get_iova(obj, ctx->aspace, iova);
>  }
>
> +static int msm_ioctl_gem_info_set_iova(struct drm_device *dev,
> +               struct drm_file *file, struct drm_gem_object *obj,
> +               uint64_t iova)
> +{
> +       struct msm_drm_private *priv = dev->dev_private;
> +       struct msm_file_private *ctx = file->driver_priv;
> +
> +       if (!priv->gpu)
> +               return -EINVAL;
> +
> +       /* Only supported if per-process address space is supported: */
> +       if (priv->gpu->aspace == ctx->aspace)
> +               return -EINVAL;
> +
> +       return msm_gem_set_iova(obj, ctx->aspace, iova);
> +}
> +
>  static int msm_ioctl_gem_info(struct drm_device *dev, void *data,
>                 struct drm_file *file)
>  {
> @@ -733,6 +750,7 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data,
>         switch (args->info) {
>         case MSM_INFO_GET_OFFSET:
>         case MSM_INFO_GET_IOVA:
> +       case MSM_INFO_SET_IOVA:
>                 /* value returned as immediate, not pointer, so len==0: */
>                 if (args->len)
>                         return -EINVAL;
> @@ -757,6 +775,9 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data,
>         case MSM_INFO_GET_IOVA:
>                 ret = msm_ioctl_gem_info_iova(dev, file, obj, &args->value);
>                 break;
> +       case MSM_INFO_SET_IOVA:
> +               ret = msm_ioctl_gem_info_set_iova(dev, file, obj, args->value);
> +               break;
>         case MSM_INFO_SET_NAME:
>                 /* length check should leave room for terminating null: */
>                 if (args->len >= sizeof(msm_obj->name)) {
> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index bf4af17e2f1e..3122ba308f31 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -525,6 +525,54 @@ int msm_gem_get_iova(struct drm_gem_object *obj,
>         return ret;
>  }
>
> +static int clear_iova(struct drm_gem_object *obj,
> +                     struct msm_gem_address_space *aspace)
> +{
> +       struct msm_gem_vma *vma = lookup_vma(obj, aspace);
> +
> +       if (!vma)
> +               return 0;
> +
> +       if (vma->inuse)

msm_gem_vma_inuse() ?

> +               return -EBUSY;
> +
> +       msm_gem_purge_vma(vma->aspace, vma);
> +       msm_gem_close_vma(vma->aspace, vma);
> +       del_vma(vma);
> +
> +       return 0;
> +}
> +
> +/*
> + * Get the requested iova but don't pin it.  Fails if the requested iova is
> + * not available.  Doesn't need a put because iovas are currently valid for
> + * the life of the object.
> + *
> + * Setting an iova of zero will clear the vma.
> + */
> +int msm_gem_set_iova(struct drm_gem_object *obj,
> +                    struct msm_gem_address_space *aspace, uint64_t iova)
> +{
> +       int ret = 0;
> +
> +       msm_gem_lock(obj);
> +       if (!iova) {
> +               ret = clear_iova(obj, aspace);
> +       } else {
> +               struct msm_gem_vma *vma;
> +               vma = get_vma_locked(obj, aspace, iova, iova + obj->size);
> +               if (IS_ERR(vma)) {
> +                       ret = PTR_ERR(vma);
> +               } else if (GEM_WARN_ON(vma->iova != iova)) {
> +                       clear_iova(obj, aspace);
> +                       ret = -ENOSPC;
> +               }
> +       }
> +       msm_gem_unlock(obj);
> +
> +       return ret;
> +}
> +
>  /*
>   * Unpin a iova by updating the reference counts. The memory isn't actually
>   * purged until something else (shrinker, mm_notifier, destroy, etc) decides
> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> index 38d66e1248b1..efa2e5c19f1e 100644
> --- a/drivers/gpu/drm/msm/msm_gem.h
> +++ b/drivers/gpu/drm/msm/msm_gem.h
> @@ -38,6 +38,12 @@ struct msm_gem_address_space {
>
>         /* @faults: the number of GPU hangs associated with this address space */
>         int faults;
> +
> +       /** @va_start: lowest possible address to allocate */
> +       uint64_t va_start;
> +
> +       /** @va_size: the size of the address space (in bytes) */
> +       uint64_t va_size;
>  };
>
>  struct msm_gem_address_space *
> @@ -144,6 +150,8 @@ struct msm_gem_vma *msm_gem_get_vma_locked(struct drm_gem_object *obj,
>                                            struct msm_gem_address_space *aspace);
>  int msm_gem_get_iova(struct drm_gem_object *obj,
>                 struct msm_gem_address_space *aspace, uint64_t *iova);
> +int msm_gem_set_iova(struct drm_gem_object *obj,
> +               struct msm_gem_address_space *aspace, uint64_t iova);
>  int msm_gem_get_and_pin_iova_range(struct drm_gem_object *obj,
>                 struct msm_gem_address_space *aspace, uint64_t *iova,
>                 u64 range_start, u64 range_end);
> diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
> index 6f9a402450f9..354f91aff573 100644
> --- a/drivers/gpu/drm/msm/msm_gem_vma.c
> +++ b/drivers/gpu/drm/msm/msm_gem_vma.c
> @@ -182,6 +182,8 @@ msm_gem_address_space_create(struct msm_mmu *mmu, const char *name,
>         spin_lock_init(&aspace->lock);
>         aspace->name = name;
>         aspace->mmu = mmu;
> +       aspace->va_start = va_start;
> +       aspace->va_size  = size;
>
>         drm_mm_init(&aspace->mm, va_start, size);
>
> diff --git a/include/uapi/drm/msm_drm.h b/include/uapi/drm/msm_drm.h
> index 794ad1948497..3c7b097c4e3d 100644
> --- a/include/uapi/drm/msm_drm.h
> +++ b/include/uapi/drm/msm_drm.h
> @@ -84,6 +84,8 @@ struct drm_msm_timespec {
>  #define MSM_PARAM_SYSPROF    0x0b  /* WO: 1 preserves perfcntrs, 2 also disables suspend */
>  #define MSM_PARAM_COMM       0x0c  /* WO: override for task->comm */
>  #define MSM_PARAM_CMDLINE    0x0d  /* WO: override for task cmdline */
> +#define MSM_PARAM_VA_START   0x0e  /* RO: start of valid GPU iova range */
> +#define MSM_PARAM_VA_SIZE    0x0f  /* RO: size of valid GPU iova range (bytes) */
>
>  /* For backwards compat.  The original support for preemption was based on
>   * a single ring per priority level so # of priority levels equals the #
> @@ -135,6 +137,7 @@ struct drm_msm_gem_new {
>  #define MSM_INFO_GET_IOVA      0x01   /* get iova, returned by value */
>  #define MSM_INFO_SET_NAME      0x02   /* set the debug name (by pointer) */
>  #define MSM_INFO_GET_NAME      0x03   /* get debug name, returned by pointer */
> +#define MSM_INFO_SET_IOVA      0x04   /* set the iova, passed by value */
>
>  struct drm_msm_gem_info {
>         __u32 handle;         /* in */
> --
> 2.35.1
>


-- 
With best wishes
Dmitry

Powered by blists - more mailing lists