linux-kernel - Re: [PATCH v2 4/9] drm/msm/A6xx: Implement preemption for A7XX targets

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: <CACu1E7FC_gPXHm4g7f0iv551orxfh=V_sJF47=6TC+nWdMyTMg@mail.gmail.com>
Date: Fri, 30 Aug 2024 19:54:01 +0100
From: Connor Abbott <cwabbott0@...il.com>
To: Rob Clark <robdclark@...il.com>
Cc: Antonino Maniscalco <antomani103@...il.com>, Sean Paul <sean@...rly.run>, 
	Konrad Dybcio <konrad.dybcio@...aro.org>, Abhinav Kumar <quic_abhinavk@...cinc.com>, 
	Dmitry Baryshkov <dmitry.baryshkov@...aro.org>, 
	Marijn Suijten <marijn.suijten@...ainline.org>, David Airlie <airlied@...il.com>, 
	Daniel Vetter <daniel@...ll.ch>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, linux-arm-msm@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org, Sharat Masetty <smasetty@...eaurora.org>, 
	Neil Armstrong <neil.armstrong@...aro.org>
Subject: Re: [PATCH v2 4/9] drm/msm/A6xx: Implement preemption for A7XX targets

On Fri, Aug 30, 2024 at 7:08 PM Rob Clark <robdclark@...il.com> wrote:
>
> On Fri, Aug 30, 2024 at 8:33 AM Antonino Maniscalco
> <antomani103@...il.com> wrote:
> >
> > This patch implements preemption feature for A6xx targets, this allows
> > the GPU to switch to a higher priority ringbuffer if one is ready. A6XX
> > hardware as such supports multiple levels of preemption granularities,
> > ranging from coarse grained(ringbuffer level) to a more fine grained
> > such as draw-call level or a bin boundary level preemption. This patch
> > enables the basic preemption level, with more fine grained preemption
> > support to follow.
> >
> > Signed-off-by: Sharat Masetty <smasetty@...eaurora.org>
> > Signed-off-by: Antonino Maniscalco <antomani103@...il.com>
> > Tested-by: Neil Armstrong <neil.armstrong@...aro.org> # on SM8650-QRD
> > ---
> >  drivers/gpu/drm/msm/Makefile              |   1 +
> >  drivers/gpu/drm/msm/adreno/a6xx_gpu.c     | 323 +++++++++++++++++++++-
> >  drivers/gpu/drm/msm/adreno/a6xx_gpu.h     | 168 ++++++++++++
> >  drivers/gpu/drm/msm/adreno/a6xx_preempt.c | 431 ++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/msm/msm_ringbuffer.h      |   7 +
> >  5 files changed, 921 insertions(+), 9 deletions(-)
> >
>
> [snip]
>
> > +
> > +int a6xx_preempt_submitqueue_setup(struct msm_gpu *gpu,
> > +               struct msm_gpu_submitqueue *queue)
> > +{
> > +       void *ptr;
> > +
> > +       /*
> > +        * Create a per submitqueue buffer for the CP to save and restore user
> > +        * specific information such as the VPC streamout data.
> > +        */
> > +       ptr = msm_gem_kernel_new(gpu->dev, A6XX_PREEMPT_USER_RECORD_SIZE,
> > +                       MSM_BO_WC, gpu->aspace, &queue->bo, &queue->bo_iova);
>
> Can this be MSM_BO_MAP_PRIV?  Otherwise it is visible (and writeable)
> by other proceess's userspace generated cmdstream.
>
> And a similar question for the scratch_bo..  I'd have to give some
> thought to what sort of mischief could be had, but generall kernel
> mappings that are not MAP_PRIV are a thing to be careful about.
>

It seems like the idea behind this is that it's supposed to be
per-context. kgsl allocates it as part of the context, as part of the
userspace address space, and then in order to know which user record
to use when preempting, before each submit (although really it only
needs to be done when setting the pagetable) it does a CP_MEM_WRITE of
the user record address to a scratch buffer holding an array of the
current user record for each ring. Then when preempting it reads the
address for the next ring from the scratch buffer and sets it. I think
we need to do that dance too.

Connor

> BR,
> -R