linux-kernel - Re: [PATCH] drm/msm/adreno: De-spaghettify the use of memory barriers

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: <20240625180221.irtao4s6i7xurzsx@hu-akhilpo-hyd.qualcomm.com>
Date: Tue, 25 Jun 2024 23:32:21 +0530
From: Akhil P Oommen <quic_akhilpo@...cinc.com>
To: Will Deacon <will@...nel.org>
CC: Andrew Halaney <ahalaney@...hat.com>,
        Konrad Dybcio
	<konrad.dybcio@...aro.org>,
        Rob Clark <robdclark@...il.com>, Sean Paul
	<sean@...rly.run>,
        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>,
        Rob Clark <robdclark@...omium.org>, <linux-arm-msm@...r.kernel.org>,
        <dri-devel@...ts.freedesktop.org>, <freedreno@...ts.freedesktop.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/msm/adreno: De-spaghettify the use of memory barriers

On Thu, Jun 20, 2024 at 02:04:01PM +0100, Will Deacon wrote:
> On Tue, Jun 18, 2024 at 09:41:58PM +0530, Akhil P Oommen wrote:
> > On Tue, Jun 04, 2024 at 03:40:56PM +0100, Will Deacon wrote:
> > > On Thu, May 16, 2024 at 01:55:26PM -0500, Andrew Halaney wrote:
> > > > On Thu, May 16, 2024 at 08:20:05PM GMT, Akhil P Oommen wrote:
> > > > > On Thu, May 16, 2024 at 08:15:34AM -0500, Andrew Halaney wrote:
> > > > > > If I understand correctly, you don't need any memory barrier.
> > > > > > writel()/readl()'s are ordered to the same endpoint. That goes for all
> > > > > > the reordering/barrier comments mentioned below too.
> > > > > > 
> > > > > > device-io.rst:
> > > > > > 
> > > > > >     The read and write functions are defined to be ordered. That is the
> > > > > >     compiler is not permitted to reorder the I/O sequence. When the ordering
> > > > > >     can be compiler optimised, you can use __readb() and friends to
> > > > > >     indicate the relaxed ordering. Use this with care.
> > > > > > 
> > > > > > memory-barriers.txt:
> > > > > > 
> > > > > >      (*) readX(), writeX():
> > > > > > 
> > > > > > 	    The readX() and writeX() MMIO accessors take a pointer to the
> > > > > > 	    peripheral being accessed as an __iomem * parameter. For pointers
> > > > > > 	    mapped with the default I/O attributes (e.g. those returned by
> > > > > > 	    ioremap()), the ordering guarantees are as follows:
> > > > > > 
> > > > > > 	    1. All readX() and writeX() accesses to the same peripheral are ordered
> > > > > > 	       with respect to each other. This ensures that MMIO register accesses
> > > > > > 	       by the same CPU thread to a particular device will arrive in program
> > > > > > 	       order.
> > > > > > 
> > > > > 
> > > > > In arm64, a writel followed by readl translates to roughly the following
> > > > > sequence: dmb_wmb(), __raw_writel(), __raw_readl(), dmb_rmb(). I am not
> > > > > sure what is stopping compiler from reordering  __raw_writel() and __raw_readl()
> > > > > above? I am assuming iomem cookie is ignored during compilation.
> > > > 
> > > > It seems to me that is due to some usage of volatile there in
> > > > __raw_writel() etc, but to be honest after reading about volatile and
> > > > some threads from gcc mailing lists, I don't have a confident answer :)
> > > > 
> > > > > 
> > > > > Added Will to this thread if he can throw some light on this.
> > > > 
> > > > Hopefully Will can school us.
> > > 
> > > The ordering in this case is ensured by the memory attributes used for
> > > ioremap(). When an MMIO region is mapped using Device-nGnRE attributes
> > > (as it the case for ioremap()), the "nR" part means "no reordering", so
> > > readX() and writeX() to that region are ordered wrt each other.
> > 
> > But that avoids only HW reordering, doesn't it? What about *compiler reordering* in the
> > case of a writel following by a readl which translates to:
> > 	1: dmb_wmb()
> > 	2: __raw_writel() -> roughly "asm volatile('str')
> > 	3: __raw_readl() -> roughly "asm volatile('ldr')
> > 	4: dmb_rmb()
> > 
> > Is the 'volatile' keyword sufficient to avoid reordering between (2) and (3)? Or
> > do we need a "memory" clobber to inhibit reordering?
> > 
> > This is still not clear to me even after going through some compiler documentions.
> 
> I don't think the compiler should reorder volatile asm blocks wrt each
> other.
> 

Thanks Will for confirmation.

-Akhil.

> Will