Linux-MIPS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] MIPS: cavium_octeon: Fix syncw generation.
@ 2020-02-11 21:24 Mark Tomlinson
  2020-02-11 21:37 ` Chris Packham
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Mark Tomlinson @ 2020-02-11 21:24 UTC (permalink / raw)
  To: paulburton, linux-mips; +Cc: chris.packham, linux-kernel, Mark Tomlinson

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 '-'.

Fixes: bf92927251b3 ("MIPS: barrier: Add __SYNC() infrastructure")
Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] MIPS: cavium_octeon: Fix syncw generation.
  2020-02-11 21:24 [PATCH] MIPS: cavium_octeon: Fix syncw generation Mark Tomlinson
@ 2020-02-11 21:37 ` Chris Packham
  2020-02-15 22:55 ` Paul Burton
  2020-02-17  0:22 ` Philippe Mathieu-Daudé
  2 siblings, 0 replies; 8+ messages in thread
From: Chris Packham @ 2020-02-11 21:37 UTC (permalink / raw)
  To: Mark Tomlinson, paulburton, linux-mips; +Cc: linux-kernel

On Wed, 2020-02-12 at 10:24 +1300, Mark Tomlinson 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 '-'.
> 
> Fixes: bf92927251b3 ("MIPS: barrier: Add __SYNC() infrastructure")
> Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>

For what it's worth

Tested-by: Chris Packham <chris.packham@alliedtelesis.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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] MIPS: cavium_octeon: Fix syncw generation.
  2020-02-11 21:24 [PATCH] MIPS: cavium_octeon: Fix syncw generation Mark Tomlinson
  2020-02-11 21:37 ` Chris Packham
@ 2020-02-15 22:55 ` Paul Burton
  2020-02-17  0:22 ` Philippe Mathieu-Daudé
  2 siblings, 0 replies; 8+ messages in thread
From: Paul Burton @ 2020-02-15 22:55 UTC (permalink / raw)
  To: Mark Tomlinson
  Cc: paulburton, linux-mips, chris.packham, linux-kernel,
	Mark Tomlinson, linux-mips

Hello,

Mark Tomlinson 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 '-'.

Applied to mips-fixes.

> commit 97e914b7de3c
> https://git.kernel.org/mips/c/97e914b7de3c
> 
> Fixes: bf92927251b3 ("MIPS: barrier: Add __SYNC() infrastructure")
> Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
> Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Signed-off-by: Paul Burton <paulburton@kernel.org>

Thanks,
    Paul

[ This message was auto-generated; if you believe anything is incorrect
  then please email paulburton@kernel.org to report it. ]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] MIPS: cavium_octeon: Fix syncw generation.
  2020-02-11 21:24 [PATCH] MIPS: cavium_octeon: Fix syncw generation Mark Tomlinson
  2020-02-11 21:37 ` Chris Packham
  2020-02-15 22:55 ` Paul Burton
@ 2020-02-17  0:22 ` Philippe Mathieu-Daudé
  2020-02-17  4:58   ` Mark Tomlinson
  2 siblings, 1 reply; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-02-17  0:22 UTC (permalink / raw)
  To: Mark Tomlinson
  Cc: Paul Burton, open list:BROADCOM NVRAM DRIVER, chris.packham, open list

Hi Mark,

On Tue, Feb 11, 2020 at 10:42 PM Mark Tomlinson
<mark.tomlinson@alliedtelesis.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.

> Fixes: bf92927251b3 ("MIPS: barrier: Add __SYNC() infrastructure")
> Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.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
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] MIPS: cavium_octeon: Fix syncw generation.
  2020-02-17  0:22 ` Philippe Mathieu-Daudé
@ 2020-02-17  4:58   ` Mark Tomlinson
  2020-02-17 20:01     ` Chris Packham
  0 siblings, 1 reply; 8+ messages in thread
From: Mark Tomlinson @ 2020-02-17  4:58 UTC (permalink / raw)
  To: f4bug; +Cc: linux-kernel, Chris Packham, paulburton, linux-mips

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@alliedtelesis.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.


> > Fixes: bf92927251b3 ("MIPS: barrier: Add __SYNC() infrastructure")
> > Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.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
> > 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] MIPS: cavium_octeon: Fix syncw generation.
  2020-02-17  4:58   ` Mark Tomlinson
@ 2020-02-17 20:01     ` Chris Packham
  2020-02-18 18:37       ` Nick Desaulniers
  0 siblings, 1 reply; 8+ messages in thread
From: Chris Packham @ 2020-02-17 20:01 UTC (permalink / raw)
  To: Mark Tomlinson, f4bug
  Cc: linux-kernel, clang-built-linux, paulburton, linux-mips

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@alliedtelesis.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@alliedtelesis.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
> > > 
> 
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] MIPS: cavium_octeon: Fix syncw generation.
  2020-02-17 20:01     ` Chris Packham
@ 2020-02-18 18:37       ` Nick Desaulniers
  2020-02-20 15:28         ` Dmitry Golovin
  0 siblings, 1 reply; 8+ messages in thread
From: Nick Desaulniers @ 2020-02-18 18:37 UTC (permalink / raw)
  To: Chris Packham
  Cc: Mark Tomlinson, f4bug, linux-kernel, clang-built-linux,
	paulburton, linux-mips

On Mon, Feb 17, 2020 at 12:01 PM Chris Packham
<Chris.Packham@alliedtelesis.co.nz> wrote:
>
> 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@alliedtelesis.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

Chris, thanks for CC'ing us.

Mark, we're building 32 bit MIPS kernels with Clang under CI (just
added big endian builds this morning).  We're actively looking into
supporting 64b MIPS.

The kernel uses GCC by default, but supports using any compiler via
`make CC=<foo>`.  There is extensive support in the kernel for
building with Clang.

GCC and Clang (when doing kernel builds, for clang we set
`-no-integrated-as`) will invoke GAS for inline assembly, but you can
set `AS=clang` for example for the out of line assembly files.  If the
C source files don't contain inline assembly (or `-no-integrated-as`
wasn't set) then Clang will skip invoking the assembler and stream out
an object file.

If you're actively supporting 64b mips, and want to give a Clang build
a try, we'd appreciate the bug reports:
https://github.com/ClangBuiltLinux/linux/issues
-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] MIPS: cavium_octeon: Fix syncw generation.
  2020-02-18 18:37       ` Nick Desaulniers
@ 2020-02-20 15:28         ` Dmitry Golovin
  0 siblings, 0 replies; 8+ messages in thread
From: Dmitry Golovin @ 2020-02-20 15:28 UTC (permalink / raw)
  To: Nick Desaulniers, Chris Packham
  Cc: Mark Tomlinson, f4bug, linux-kernel, clang-built-linux,
	paulburton, linux-mips

18.02.2020, 20:37, "'Nick Desaulniers' via Clang Built Linux" <clang-built-linux@googlegroups.com>:
>>  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.

I have a working build for MIPS64 (only mips64r6 at the moment), the
config is based on malta_defconfig and it boots in qemu. I still don't
have a matching buildroot image, but it shouldn't be a problem.

Regards,
Dima

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-11 21:24 [PATCH] MIPS: cavium_octeon: Fix syncw generation Mark Tomlinson
2020-02-11 21:37 ` Chris Packham
2020-02-15 22:55 ` Paul Burton
2020-02-17  0:22 ` Philippe Mathieu-Daudé
2020-02-17  4:58   ` Mark Tomlinson
2020-02-17 20:01     ` Chris Packham
2020-02-18 18:37       ` Nick Desaulniers
2020-02-20 15:28         ` Dmitry Golovin

Linux-MIPS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mips/0 linux-mips/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mips linux-mips/ https://lore.kernel.org/linux-mips \
		linux-mips@vger.kernel.org
	public-inbox-index linux-mips

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-mips


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git