linux-kernel - Re: [PATCH v3 04/10] 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: <20240910164904.7ibuhbkcwh3jdzdd@hu-akhilpo-hyd.qualcomm.com>
Date: Tue, 10 Sep 2024 22:19:04 +0530
From: Akhil P Oommen <quic_akhilpo@...cinc.com>
To: Rob Clark <robdclark@...il.com>
CC: Connor Abbott <cwabbott0@...il.com>,
        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>,
        Jonathan Corbet <corbet@....net>, <linux-arm-msm@...r.kernel.org>,
        <dri-devel@...ts.freedesktop.org>, <freedreno@...ts.freedesktop.org>,
        <linux-kernel@...r.kernel.org>, <linux-doc@...r.kernel.org>,
        Sharat Masetty <smasetty@...eaurora.org>,
        "Neil
 Armstrong" <neil.armstrong@...aro.org>
Subject: Re: [PATCH v3 04/10] drm/msm/A6xx: Implement preemption for A7XX
 targets

On Mon, Sep 09, 2024 at 07:40:07AM -0700, Rob Clark wrote:
> On Mon, Sep 9, 2024 at 6:43 AM Connor Abbott <cwabbott0@...il.com> wrote:
> >
> > On Mon, Sep 9, 2024 at 2:15 PM Antonino Maniscalco
> > <antomani103@...il.com> wrote:
> > >
> > > On 9/6/24 9:54 PM, Akhil P Oommen wrote:
> > > > On Thu, Sep 05, 2024 at 04:51:22PM +0200, Antonino Maniscalco 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     | 293 +++++++++++++++++++++-
> > > >>   drivers/gpu/drm/msm/adreno/a6xx_gpu.h     | 161 ++++++++++++
> > > ...
> > > >
> > > > we can use the lighter smp variant here.
> > > >
> > > >> +
> > > >> +            if (a6xx_gpu->cur_ring == ring)
> > > >> +                    gpu_write(gpu, REG_A6XX_CP_RB_WPTR, wptr);
> > > >> +            else
> > > >> +                    ring->skip_inline_wptr = true;
> > > >> +    } else {
> > > >> +            ring->skip_inline_wptr = true;
> > > >> +    }
> > > >> +
> > > >> +    spin_unlock_irqrestore(&ring->preempt_lock, flags);
> > > >>   }
> > > >>
> > > >>   static void get_stats_counter(struct msm_ringbuffer *ring, u32 counter,
> > > >> @@ -138,12 +231,14 @@ static void a6xx_set_pagetable(struct a6xx_gpu *a6xx_gpu,
> > > >
> > > > set_pagetable checks "cur_ctx_seqno" to see if pt switch is needed or
> > > > not. This is currently not tracked separately for each ring. Can you
> > > > please check that?
> > >
> > > I totally missed that. Thanks for catching it!
> > >
> > > >
> > > > I wonder why that didn't cause any gpu errors in testing. Not sure if I
> > > > am missing something.
> > > >
> > >
> > > I think this is because, so long as a single context doesn't submit to
> > > two different rings with differenr priorities, we will only be incorrect
> > > in the sense that we emit more page table switches than necessary and
> > > never less. However untrusted userspace could create a context that
> > > submits to two different rings and that would lead to execution in the
> > > wrong context so we must fix this.

Yep, it would be a security bug!

-Akhil

> >
> > FWIW, in Mesa in the future we may want to expose multiple Vulkan
> > queues per device. Then this would definitely blow up.
> 
> This will actually be required by future android versions, with the
> switch to vk hwui backend (because apparently locking is hard, the
> solution was to use different queue's for different threads)
> 
> https://gitlab.freedesktop.org/mesa/mesa/-/issues/11326
> 
> BR,
> -R