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: <CACu1E7HC_u0WZ5ayXhm3z-Q5Do7tnwQLGdJ5feD99aOB52H1ug@mail.gmail.com>
Date: Fri, 30 Aug 2024 20:09:47 +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 8:00 PM Rob Clark <robdclark@...il.com> wrote:
>
> On Fri, Aug 30, 2024 at 11:54 AM Connor Abbott <cwabbott0@...il.com> wrote:
> >
> > 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.
>
> Moving it into userspace's address space (vm) would be better.
>
> I assume the preempt record is where state is saved/restored?  So
> would need to be in kernel aspace/vm?  Or is the fw changing ttbr0
> after saving state but before restoring?
>
> BR,
> -R

The preempt record is split into a number of pieces, each with their
own address. One of those pieces is the SMMU record with ttbr0 and
other SMMU things. Another piece is the "private" context record with
sensitive things like RB address/rptr/wptr, although actually the bulk
of the registers are saved here. Then the user or "non-private" record
is its own piece, which is presumably saved before switching ttbr0 and
restored after the SMMU record is restored and ttbr0 is switched.

Connor

>
> > Connor
> >
> > > BR,
> > > -R