linux-kernel - Re: [PATCH] MIPS: cavium_octeon: Fix syncw generation.

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: <8cb14684e2f774d9573c062f2d82ad5348c5fee7.camel@alliedtelesis.co.nz>
Date:   Mon, 17 Feb 2020 20:01:06 +0000
From:   Chris Packham <Chris.Packham@...iedtelesis.co.nz>
To:     Mark Tomlinson <Mark.Tomlinson@...iedtelesis.co.nz>,
        "f4bug@...at.org" <f4bug@...at.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "clang-built-linux@...glegroups.com" 
        <clang-built-linux@...glegroups.com>,
        "paulburton@...nel.org" <paulburton@...nel.org>,
        "linux-mips@...r.kernel.org" <linux-mips@...r.kernel.org>
Subject: Re: [PATCH] MIPS: cavium_octeon: Fix syncw generation.

On Mon, 2020-02-17 at 17:58 +1300, Mark Tomlinson wrote:
> Hi Phil,
> 
> On Mon, 2020-02-17 at 01:22 +0100, Philippe Mathieu-Daudé wrote:
> > Hi Mark,
> > 
> > On Tue, Feb 11, 2020 at 10:42 PM Mark Tomlinson
> > <mark.tomlinson@...iedtelesis.co.nz> wrote:
> > > 
> > > The Cavium Octeon CPU uses a special sync instruction for implementing
> > > wmb, and due to a CPU bug, the instruction must appear twice. A macro
> > > had been defined to hide this:
> > > 
> > >  #define __SYNC_rpt(type)     (1 + (type == __SYNC_wmb))
> > > 
> > > which was intended to evaluate to 2 for __SYNC_wmb, and 1 for any other
> > > type of sync. However, this expression is evaluated by the assembler,
> > > and not the compiler, and the result of '==' in the assembler is 0 or
> > > -1, not 0 or 1 as it is in C. The net result was wmb() producing no code
> > > at all. The simple fix in this patch is to change the '+' to '-'.
> > 
> > Isn't this particular to the assembler implementation?
> > Can you explicit the assembler you are using in the commit description?
> > Assuming we have to look at your commit in 3 years from now, we'll
> > wonder what assembler you were using.
> > 
> > Thanks,
> > 
> > Phil.
> 
> Yes, it is tied to the assembler. But the Linux kernel is tied to GCC,
> and GCC (I believe) is tied to GNU as. I can't see the specification of
> GNU as changing, since that could break anything written for it.
> 

There is an effort underway to build the kernel with clang[1]. I'm not
sure what that ends up using for an assembler or if it'll even be able
to target mips64 anytime soon.

For reference the relevant section from the GNU as manual[2] says "A
true results has a value of -1 whereas a false result has a value of
0".

[1] - https://clangbuiltlinux.github.io/
[2] - https://sourceware.org/binutils/docs/as/Infix-Ops.html#Infix-Ops



> 
> > > Fixes: bf92927251b3 ("MIPS: barrier: Add __SYNC() infrastructure")
> > > Signed-off-by: Mark Tomlinson <mark.tomlinson@...iedtelesis.co.nz>
> > > ---
> > >  arch/mips/include/asm/sync.h | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/mips/include/asm/sync.h b/arch/mips/include/asm/sync.h
> > > index 7c6a1095f5..aabd097933 100644
> > > --- a/arch/mips/include/asm/sync.h
> > > +++ b/arch/mips/include/asm/sync.h
> > > @@ -155,9 +155,11 @@
> > >   * effective barrier as noted by commit 6b07d38aaa52 ("MIPS: Octeon: Use
> > >   * optimized memory barrier primitives."). Here we specify that the affected
> > >   * sync instructions should be emitted twice.
> > > + * Note that this expression is evaluated by the assembler (not the compiler),
> > > + * and that the assembler evaluates '==' as 0 or -1, not 0 or 1.
> > >   */
> > >  #ifdef CONFIG_CPU_CAVIUM_OCTEON
> > > -# define __SYNC_rpt(type)      (1 + (type == __SYNC_wmb))
> > > +# define __SYNC_rpt(type)      (1 - (type == __SYNC_wmb))
> > >  #else
> > >  # define __SYNC_rpt(type)      1
> > >  #endif
> > > --
> > > 2.25.0
> > > 
> 
>