Missing operand for tlbie instruction on Power7

Missing operand for tlbie instruction on Power7

[Laura Abbott]

Hi,

We received a report (https://bugzilla.redhat.com/show_bug.cgi?id=1267395) of bad assembly
when compiling on powerpc with little endian

[labbott@labbott-redhat-machine linux_upstream]$ make ARCH=powerpc CROSS_COMPILE=powerpc64-linux-gnu-
   CHK     include/config/kernel.release
   CHK     include/generated/uapi/linux/version.h
   CHK     include/generated/utsrelease.h
   CHK     include/generated/bounds.h
   CHK     include/generated/timeconst.h
   CHK     include/generated/asm-offsets.h
   CALL    scripts/checksyscalls.sh
   CHK     include/generated/compile.h
   CALL    arch/powerpc/kernel/systbl_chk.sh
   AS      arch/powerpc/kernel/swsusp_asm64.o
arch/powerpc/kernel/swsusp_asm64.S: Assembler messages:
arch/powerpc/kernel/swsusp_asm64.S:188: Error: missing operand
scripts/Makefile.build:294: recipe for target 'arch/powerpc/kernel/swsusp_asm64.o' failed
make[1]: *** [arch/powerpc/kernel/swsusp_asm64.o] Error 1
Makefile:941: recipe for target 'arch/powerpc/kernel' failed
make: *** [arch/powerpc/kernel] Error 2

This problem started happening after a binutils update:

[labbott@labbott-redhat-machine linux_upstream]$ powerpc64-linux-gnu-as --version
GNU assembler version 2.25.1-1.fc22
Copyright (C) 2014 Free Software Foundation, Inc.
This program is free software; you may redistribute it under the terms of
the GNU General Public License version 3 or later.
This program has absolutely no warranty.
This assembler was configured for a target of `powerpc64-linux-gnu'.
[labbott@labbott-redhat-machine linux_upstream]$

After some discussion with the binutils folks, it turns out that the tlbie
instruction actually requires another operand and binutils was updated to
check for this https://sourceware.org/ml/binutils/2015-05/msg00133.html .

The code sequence in arch/powerpc/include/asm/ppc_asm.h now needs to be updated:

#if !defined(CONFIG_4xx) && !defined(CONFIG_8xx)
#define tlbia                                   \
         li      r4,1024;                        \
         mtctr   r4;                             \
         lis     r4,KERNELBASE@h;                \
0:      tlbie   r4;                             \
         addi    r4,r4,0x1000;                   \
         bdnz    0b
#endif

I don't know enough ppc assembly to properly fix this but I can test.

Thanks,
Laura

Re: Missing operand for tlbie instruction on Power7

[Denis Kirjanov]

[-- Attachment #1: Type: text/plain, Size: 2685 bytes --]

On 10/2/15, Laura Abbott <labbott@redhat.com> wrote:
> Hi,
>
> We received a report (https://bugzilla.redhat.com/show_bug.cgi?id=1267395)
> of bad assembly
> when compiling on powerpc with little endian
>
> [labbott@labbott-redhat-machine linux_upstream]$ make ARCH=powerpc
> CROSS_COMPILE=powerpc64-linux-gnu-
>    CHK     include/config/kernel.release
>    CHK     include/generated/uapi/linux/version.h
>    CHK     include/generated/utsrelease.h
>    CHK     include/generated/bounds.h
>    CHK     include/generated/timeconst.h
>    CHK     include/generated/asm-offsets.h
>    CALL    scripts/checksyscalls.sh
>    CHK     include/generated/compile.h
>    CALL    arch/powerpc/kernel/systbl_chk.sh
>    AS      arch/powerpc/kernel/swsusp_asm64.o
> arch/powerpc/kernel/swsusp_asm64.S: Assembler messages:
> arch/powerpc/kernel/swsusp_asm64.S:188: Error: missing operand
> scripts/Makefile.build:294: recipe for target
> 'arch/powerpc/kernel/swsusp_asm64.o' failed
> make[1]: *** [arch/powerpc/kernel/swsusp_asm64.o] Error 1
> Makefile:941: recipe for target 'arch/powerpc/kernel' failed
> make: *** [arch/powerpc/kernel] Error 2
>
> This problem started happening after a binutils update:
>
> [labbott@labbott-redhat-machine linux_upstream]$ powerpc64-linux-gnu-as
> --version
> GNU assembler version 2.25.1-1.fc22
> Copyright (C) 2014 Free Software Foundation, Inc.
> This program is free software; you may redistribute it under the terms of
> the GNU General Public License version 3 or later.
> This program has absolutely no warranty.
> This assembler was configured for a target of `powerpc64-linux-gnu'.
> [labbott@labbott-redhat-machine linux_upstream]$
>
> After some discussion with the binutils folks, it turns out that the tlbie
> instruction actually requires another operand and binutils was updated to
> check for this https://sourceware.org/ml/binutils/2015-05/msg00133.html .
>
> The code sequence in arch/powerpc/include/asm/ppc_asm.h now needs to be
> updated:
>
> #if !defined(CONFIG_4xx) && !defined(CONFIG_8xx)
> #define tlbia                                   \
>          li      r4,1024;                        \
>          mtctr   r4;                             \
>          lis     r4,KERNELBASE@h;                \
> 0:      tlbie   r4;                             \
>          addi    r4,r4,0x1000;                   \
>          bdnz    0b
> #endif
>
> I don't know enough ppc assembly to properly fix this but I can test.

Could you please test the patch attached?



>
> Thanks,
> Laura
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

[-- Attachment #2: tlbie_fix.patch --]
[-- Type: text/x-patch, Size: 422 bytes --]

diff --git a/arch/powerpc/include/asm/ppc_asm.h b/arch/powerpc/include/asm/ppc_asm.h
index dd0fc18..240557a 100644
--- a/arch/powerpc/include/asm/ppc_asm.h
+++ b/arch/powerpc/include/asm/ppc_asm.h
@@ -445,7 +445,7 @@ END_FTR_SECTION_NESTED(CPU_FTR_HAS_PPR,CPU_FTR_HAS_PPR,945)
 	li	r4,1024;			\
 	mtctr	r4;				\
 	lis	r4,KERNELBASE@h;		\
-0:	tlbie	r4;				\
+0:	tlbie	r4, 0;				\
 	addi	r4,r4,0x1000;			\
 	bdnz	0b
 #endif

Re: Missing operand for tlbie instruction on Power7

[Peter Bergner]

On Fri, 2015-10-02 at 22:03 +0300, Denis Kirjanov wrote:
> arch/powerpc/kernel/swsusp_asm64.S: Assembler messages:
>> arch/powerpc/kernel/swsusp_asm64.S:188: Error: missing operand
>> scripts/Makefile.build:294: recipe for target
>> 'arch/powerpc/kernel/swsusp_asm64.o' failed
>> make[1]: *** [arch/powerpc/kernel/swsusp_asm64.o] Error 1
>> Makefile:941: recipe for target 'arch/powerpc/kernel' failed
>> make: *** [arch/powerpc/kernel] Error 2
[snip]
>> I don't know enough ppc assembly to properly fix this but I can test.
> 
> Could you please test the patch attached?
[snip]
> -0:     tlbie   r4;                             \
> +0:     tlbie   r4, 0;                          \

This isn't correct.  With POWER7 and later (which this compile
is, since it's on LE), the tlbie instruction takes two register
operands:

    tlbie RB, RS

The tlbie instruction on pre POWER7 cpus had one required register
operand (RB) and an optional second L operand, where if you omitted
it, it was the same as using "0":

    tlbie RB, L

This is a POWER7 and later build, so your change which adds the "0"
above is really adding r0 for RS.  The new tlbie instruction doesn't
treat r0 specially, so you'll be using whatever random bits which
happen to be in r0 which I don't think that is what you want.


Peter

Re: Missing operand for tlbie instruction on Power7

[Denis Kirjanov]

[-- Attachment #1: Type: text/plain, Size: 1496 bytes --]

On 10/2/15, Peter Bergner <bergner@vnet.ibm.com> wrote:
> On Fri, 2015-10-02 at 22:03 +0300, Denis Kirjanov wrote:
>> arch/powerpc/kernel/swsusp_asm64.S: Assembler messages:
>>> arch/powerpc/kernel/swsusp_asm64.S:188: Error: missing operand
>>> scripts/Makefile.build:294: recipe for target
>>> 'arch/powerpc/kernel/swsusp_asm64.o' failed
>>> make[1]: *** [arch/powerpc/kernel/swsusp_asm64.o] Error 1
>>> Makefile:941: recipe for target 'arch/powerpc/kernel' failed
>>> make: *** [arch/powerpc/kernel] Error 2
> [snip]
>>> I don't know enough ppc assembly to properly fix this but I can test.
>>
>> Could you please test the patch attached?
> [snip]
>> -0:     tlbie   r4;                             \
>> +0:     tlbie   r4, 0;                          \
>
> This isn't correct.  With POWER7 and later (which this compile
> is, since it's on LE), the tlbie instruction takes two register
> operands:
>
>     tlbie RB, RS
>
> The tlbie instruction on pre POWER7 cpus had one required register
> operand (RB) and an optional second L operand, where if you omitted
> it, it was the same as using "0":
>
>     tlbie RB, L
>
> This is a POWER7 and later build, so your change which adds the "0"
> above is really adding r0 for RS.  The new tlbie instruction doesn't
> treat r0 specially, so you'll be using whatever random bits which
> happen to be in r0 which I don't think that is what you want.

Ok, than we can just zero out r5 for example and use it in tlbie as RS,
right?


>
>
> Peter
>
>
>
>

[-- Attachment #2: tlbie_fix2.patch --]
[-- Type: text/x-patch, Size: 512 bytes --]

diff --git a/arch/powerpc/include/asm/ppc_asm.h b/arch/powerpc/include/asm/ppc_asm.h
index dd0fc18..cb0f627 100644
--- a/arch/powerpc/include/asm/ppc_asm.h
+++ b/arch/powerpc/include/asm/ppc_asm.h
@@ -443,9 +443,10 @@ END_FTR_SECTION_NESTED(CPU_FTR_HAS_PPR,CPU_FTR_HAS_PPR,945)
 #if !defined(CONFIG_4xx) && !defined(CONFIG_8xx)
 #define tlbia					\
 	li	r4,1024;			\
+	li  r5,0;				\
 	mtctr	r4;				\
 	lis	r4,KERNELBASE@h;		\
-0:	tlbie	r4;				\
+0:	tlbie	r4, r5;				\
 	addi	r4,r4,0x1000;			\
 	bdnz	0b
 #endif

Re: Missing operand for tlbie instruction on Power7

[Segher Boessenkool]

On Sat, Oct 03, 2015 at 12:37:35AM +0300, Denis Kirjanov wrote:
> >> -0:     tlbie   r4;                             \
> >> +0:     tlbie   r4, 0;                          \
> >
> > This isn't correct.  With POWER7 and later (which this compile
> > is, since it's on LE), the tlbie instruction takes two register
> > operands:
> >
> >     tlbie RB, RS
> >
> > The tlbie instruction on pre POWER7 cpus had one required register
> > operand (RB) and an optional second L operand, where if you omitted
> > it, it was the same as using "0":
> >
> >     tlbie RB, L
> >
> > This is a POWER7 and later build, so your change which adds the "0"
> > above is really adding r0 for RS.  The new tlbie instruction doesn't
> > treat r0 specially, so you'll be using whatever random bits which
> > happen to be in r0 which I don't think that is what you want.
> 
> Ok, than we can just zero out r5 for example and use it in tlbie as RS,
> right?

That won't assemble _unless_ your assembler is in POWER7 mode.  It also
won't do the right thing at run time on older machines.

Where is this tlbia macro used at all, for 64-bit machines?


Segher

Re: Missing operand for tlbie instruction on Power7

[Laura Abbott]

On 10/02/2015 03:00 PM, Segher Boessenkool wrote:
> On Sat, Oct 03, 2015 at 12:37:35AM +0300, Denis Kirjanov wrote:
>>>> -0:     tlbie   r4;                             \
>>>> +0:     tlbie   r4, 0;                          \
>>>
>>> This isn't correct.  With POWER7 and later (which this compile
>>> is, since it's on LE), the tlbie instruction takes two register
>>> operands:
>>>
>>>      tlbie RB, RS
>>>
>>> The tlbie instruction on pre POWER7 cpus had one required register
>>> operand (RB) and an optional second L operand, where if you omitted
>>> it, it was the same as using "0":
>>>
>>>      tlbie RB, L
>>>
>>> This is a POWER7 and later build, so your change which adds the "0"
>>> above is really adding r0 for RS.  The new tlbie instruction doesn't
>>> treat r0 specially, so you'll be using whatever random bits which
>>> happen to be in r0 which I don't think that is what you want.
>>
>> Ok, than we can just zero out r5 for example and use it in tlbie as RS,
>> right?
>
> That won't assemble _unless_ your assembler is in POWER7 mode.  It also
> won't do the right thing at run time on older machines.
>
> Where is this tlbia macro used at all, for 64-bit machines?
>


[labbott@labbott-redhat-machine linux_upstream]$ make ARCH=powerpc CROSS_COMPILE=powerpc64-linux-gnu-
   CHK     include/config/kernel.release
   CHK     include/generated/uapi/linux/version.h
   CHK     include/generated/utsrelease.h
   CHK     include/generated/bounds.h
   CHK     include/generated/timeconst.h
   CHK     include/generated/asm-offsets.h
   CALL    scripts/checksyscalls.sh
   CHK     include/generated/compile.h
   CALL    arch/powerpc/kernel/systbl_chk.sh
   AS      arch/powerpc/kernel/swsusp_asm64.o
arch/powerpc/kernel/swsusp_asm64.S: Assembler messages:
arch/powerpc/kernel/swsusp_asm64.S:188: Error: missing operand
scripts/Makefile.build:294: recipe for target 'arch/powerpc/kernel/swsusp_asm64.o' failed
make[1]: *** [arch/powerpc/kernel/swsusp_asm64.o] Error 1
Makefile:941: recipe for target 'arch/powerpc/kernel' failed
make: *** [arch/powerpc/kernel] Error 2

This is piece of code protected by CONFIG_PPC_BOOK3S_64.
  
>
> Segher
>

Re: Missing operand for tlbie instruction on Power7

[Peter Bergner]

On Fri, 2015-10-02 at 17:00 -0500, Segher Boessenkool wrote:
> On Sat, Oct 03, 2015 at 12:37:35AM +0300, Denis Kirjanov wrote:
> > >> -0:     tlbie   r4;                             \
> > >> +0:     tlbie   r4, 0;                          \
> > >
> > > This isn't correct.  With POWER7 and later (which this compile
> > > is, since it's on LE), the tlbie instruction takes two register
> > > operands:
> > >
> > >     tlbie RB, RS
> > >
> > > The tlbie instruction on pre POWER7 cpus had one required register
> > > operand (RB) and an optional second L operand, where if you omitted
> > > it, it was the same as using "0":
> > >
> > >     tlbie RB, L
> > >
> > > This is a POWER7 and later build, so your change which adds the "0"
> > > above is really adding r0 for RS.  The new tlbie instruction doesn't
> > > treat r0 specially, so you'll be using whatever random bits which
> > > happen to be in r0 which I don't think that is what you want.
> > 
> > Ok, than we can just zero out r5 for example and use it in tlbie as RS,
> > right?
> 
> That won't assemble _unless_ your assembler is in POWER7 mode.  It also
> won't do the right thing at run time on older machines.

Correct, getting this to work on both pre-power7 and power7 and later
is tricky.  One really horrible hack would be to do:

  li r0,0
  tlbie r4,0

On pre-power7, the "0" will be taken as a zero L operand and on
power7 and later, it'll be r0, but with a zero value we loaded in
the insn before.  I know, really ugly. :-)

Peter

Re: Missing operand for tlbie instruction on Power7

[Segher Boessenkool]

On Fri, Oct 02, 2015 at 09:24:46PM -0500, Peter Bergner wrote:
> > > Ok, than we can just zero out r5 for example and use it in tlbie as RS,
> > > right?
> > 
> > That won't assemble _unless_ your assembler is in POWER7 mode.  It also
> > won't do the right thing at run time on older machines.
> 
> Correct, getting this to work on both pre-power7 and power7 and later
> is tricky.  One really horrible hack would be to do:
> 
>   li r0,0
>   tlbie r4,0
> 
> On pre-power7, the "0" will be taken as a zero L operand and on
> power7 and later, it'll be r0, but with a zero value we loaded in
> the insn before.  I know, really ugly. :-)

Hide the "li 0,0" somewhere earlier, and write it as "tlbie 4,0", and
don't write a comment -- we *like* tricky!

It should really be a separate macro define for power7 and 4xx etc.;
and the macro should not be called "tlbia", but something that makes
it obvious at the usage sites that it is in fact a macro; and why a
macro anyway, a function call might be better here?


Segher

Re: Missing operand for tlbie instruction on Power7

[Laura Abbott]

On 10/03/2015 05:00 PM, Segher Boessenkool wrote:
> On Fri, Oct 02, 2015 at 09:24:46PM -0500, Peter Bergner wrote:
>>>> Ok, than we can just zero out r5 for example and use it in tlbie as RS,
>>>> right?
>>>
>>> That won't assemble _unless_ your assembler is in POWER7 mode.  It also
>>> won't do the right thing at run time on older machines.
>>
>> Correct, getting this to work on both pre-power7 and power7 and later
>> is tricky.  One really horrible hack would be to do:
>>
>>    li r0,0
>>    tlbie r4,0
>>
>> On pre-power7, the "0" will be taken as a zero L operand and on
>> power7 and later, it'll be r0, but with a zero value we loaded in
>> the insn before.  I know, really ugly. :-)
>
> Hide the "li 0,0" somewhere earlier, and write it as "tlbie 4,0", and
> don't write a comment -- we *like* tricky!
>
> It should really be a separate macro define for power7 and 4xx etc.;
> and the macro should not be called "tlbia", but something that makes
> it obvious at the usage sites that it is in fact a macro; and why a
> macro anyway, a function call might be better here?
>
>
> Segher
>

I can't speculate on why it is a macro but would something such as the
following work?

Alternatively, we could move the assembly into swusp_asm64.S which appears to be
the only 64-bit caller of tlbia

diff --git a/arch/powerpc/include/asm/ppc_asm.h b/arch/powerpc/include/asm/ppc_asm.h
index dd0fc18..53e5f59 100644
--- a/arch/powerpc/include/asm/ppc_asm.h
+++ b/arch/powerpc/include/asm/ppc_asm.h
@@ -434,14 +434,30 @@ END_FTR_SECTION_NESTED(CPU_FTR_HAS_PPR,CPU_FTR_HAS_PPR,945)
  #endif
  
  /*
- * This instruction is not implemented on the PPC 603 or 601; however, on
+ * tlbia is not implemented on the PPC 603 or 601; however, on
   * the 403GCX and 405GP tlbia IS defined and tlbie is not.
   * All of these instructions exist in the 8xx, they have magical powers,
   * and they must be used.
+ *
+ * The ISA 2.06 update for POWER7 changed the number of arguments to tlbie
+ * so it gets its own special case as well as any config that may set
+ * mtune=power7 such as CONFIG_PPC_BOOK3S_64
   */
  
-#if !defined(CONFIG_4xx) && !defined(CONFIG_8xx)
-#define tlbia					\
+#if defined(CONFIG_4xx) || defined(CONFIG_8xx)
+#define TLBIA_ACTION	tlbia
+#elif defined(CONFIG_POWER7_CPU) || defined(CONFIG_PPC_BOOK3S_64)
+#define TLBIA_ACTION				\
+	li	r4,1024;			\
+	mtctr	r4;				\
+	lis	r4,KERNELBASE@h;		\
+	li	r0,0;				\
+0:	tlbie	r4,r0;				\
+	addi	r4,r4,0x1000;			\
+	bdnz	0b
+
+#else
+#define TLBIA_ACTION				\
  	li	r4,1024;			\
  	mtctr	r4;				\
  	lis	r4,KERNELBASE@h;		\
diff --git a/arch/powerpc/kernel/head_32.S b/arch/powerpc/kernel/head_32.S
index dc0488b..40c3b33 100644
--- a/arch/powerpc/kernel/head_32.S
+++ b/arch/powerpc/kernel/head_32.S
@@ -901,7 +901,7 @@ _ENTRY(__restore_cpu_setup)
  load_up_mmu:
  	sync			/* Force all PTE updates to finish */
  	isync
-	tlbia			/* Clear all TLB entries */
+	TLBIA_ACTION		/* Clear all TLB entries */
  	sync			/* wait for tlbia/tlbie to finish */
  	TLBSYNC			/* ... on all CPUs */
  	/* Load the SDR1 register (hash table base & size) */
diff --git a/arch/powerpc/kernel/head_40x.S b/arch/powerpc/kernel/head_40x.S
index 7d7d863..6a83ec2 100644
--- a/arch/powerpc/kernel/head_40x.S
+++ b/arch/powerpc/kernel/head_40x.S
@@ -869,7 +869,7 @@ start_here:
  /* Load up the kernel context */
  2:
  	sync			/* Flush to memory before changing TLB */
-	tlbia
+	TLBIA_ACTION
  	isync			/* Flush shadow TLBs */
  
  	/* set up the PTE pointers for the Abatron bdiGDB.
@@ -897,7 +897,7 @@ start_here:
   * virtual to physical and more importantly sets the cache mode.
   */
  initial_mmu:
-	tlbia			/* Invalidate all TLB entries */
+	TLBIA_ACTION		/* Invalidate all TLB entries */
  	isync
  
  	/* We should still be executing code at physical address 0x0000xxxx
diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S
index 78c1eba..533d736 100644
--- a/arch/powerpc/kernel/head_8xx.S
+++ b/arch/powerpc/kernel/head_8xx.S
@@ -711,7 +711,7 @@ start_here:
  /* Load up the kernel context */
  2:
  	SYNC			/* Force all PTE updates to finish */
-	tlbia			/* Clear all TLB entries */
+	TLBIA_ACTION		/* Clear all TLB entries */
  	sync			/* wait for tlbia/tlbie to finish */
  	TLBSYNC			/* ... on all CPUs */
  
@@ -741,7 +741,7 @@ start_here:
   * these mappings is mapped by page tables.
   */
  initial_mmu:
-	tlbia			/* Invalidate all TLB entries */
+	TLBIA_ACTION		/* Invalidate all TLB entries */
  /* Always pin the first 8 MB ITLB to prevent ITLB
     misses while mucking around with SRR0/SRR1 in asm
  */
diff --git a/arch/powerpc/kernel/swsusp_asm64.S b/arch/powerpc/kernel/swsusp_asm64.S
index 988f38d..2bcc49e 100644
--- a/arch/powerpc/kernel/swsusp_asm64.S
+++ b/arch/powerpc/kernel/swsusp_asm64.S
@@ -185,7 +185,7 @@ nothing_to_copy:
  
  	sync
  
-	tlbia
+	TLBIA_ACTION
  #endif
  
  	ld	r11,swsusp_save_area_ptr@toc(r2)
diff --git a/arch/powerpc/mm/hash_low_32.S b/arch/powerpc/mm/hash_low_32.S
index 115347f..9805c4c 100644
--- a/arch/powerpc/mm/hash_low_32.S
+++ b/arch/powerpc/mm/hash_low_32.S
@@ -696,7 +696,7 @@ _GLOBAL(_tlbia)
  	stwcx.	r8,0,r9
  	bne-	10b
  	sync
-	tlbia
+	TLBIA_ACTION
  	sync
  	TLBSYNC
  	li	r0,0
@@ -706,7 +706,7 @@ _GLOBAL(_tlbia)
  	isync
  #else /* CONFIG_SMP */
  	sync
-	tlbia
+	TLBIA_ACTION
  	sync
  #endif /* CONFIG_SMP */
  	blr
-- 
2.4.3

Re: Missing operand for tlbie instruction on Power7

[Michael Ellerman]

On Fri, 2015-10-02 at 08:43 -0700, Laura Abbott wrote:
> Hi,
> 
> We received a report (https://bugzilla.redhat.com/show_bug.cgi?id=1267395) of bad assembly
> when compiling on powerpc with little endian

...

> After some discussion with the binutils folks, it turns out that the tlbie
> instruction actually requires another operand and binutils was updated to
> check for this https://sourceware.org/ml/binutils/2015-05/msg00133.html .
> 
> The code sequence in arch/powerpc/include/asm/ppc_asm.h now needs to be updated:
> 
> #if !defined(CONFIG_4xx) && !defined(CONFIG_8xx)
> #define tlbia                                   \
>          li      r4,1024;                        \
>          mtctr   r4;                             \
>          lis     r4,KERNELBASE@h;                \
> 0:      tlbie   r4;                             \
>          addi    r4,r4,0x1000;                   \
>          bdnz    0b
> #endif
> 
> I don't know enough ppc assembly to properly fix this but I can test.

How are you testing? This code is fairly old and I'm dubious if it still works.

These days we have a ppc_md hook for flushing the TLB, ppc_md.flush_tlb().
Ideally the swsusp code would use that.

cheers

Re: Missing operand for tlbie instruction on Power7

[Laura Abbott]

On 10/05/2015 08:35 PM, Michael Ellerman wrote:
> On Fri, 2015-10-02 at 08:43 -0700, Laura Abbott wrote:
>> Hi,
>>
>> We received a report (https://bugzilla.redhat.com/show_bug.cgi?id=1267395) of bad assembly
>> when compiling on powerpc with little endian
>
> ...
>
>> After some discussion with the binutils folks, it turns out that the tlbie
>> instruction actually requires another operand and binutils was updated to
>> check for this https://sourceware.org/ml/binutils/2015-05/msg00133.html .
>>
>> The code sequence in arch/powerpc/include/asm/ppc_asm.h now needs to be updated:
>>
>> #if !defined(CONFIG_4xx) && !defined(CONFIG_8xx)
>> #define tlbia                                   \
>>           li      r4,1024;                        \
>>           mtctr   r4;                             \
>>           lis     r4,KERNELBASE@h;                \
>> 0:      tlbie   r4;                             \
>>           addi    r4,r4,0x1000;                   \
>>           bdnz    0b
>> #endif
>>
>> I don't know enough ppc assembly to properly fix this but I can test.
>
> How are you testing? This code is fairly old and I'm dubious if it still works.
>
> These days we have a ppc_md hook for flushing the TLB, ppc_md.flush_tlb().
> Ideally the swsusp code would use that.
>
> cheers
>
>

Testing would probably just be compile and maybe boot. I don't have regular
access to the hardware. This problem just showed up for me when someone
tried to compile Fedora rawhide with the latest binutils.

 From what I can tell, it looks like the .flush_tlb of the cpu_spec is only
defined for power7 and power8 and I don't see a ppc_md.flush_tlb on the
master branch. It's not clear what to do for the case where there is no
flush_tlb function. Would filling in a .flush_tlb for all the PPC_BOOK3S_64
with the existing tlbia sequence work? It's also worth noting that the
__flush_power7 uses tlbiel instead of tlbie.

Thanks,
Laura

Re: Missing operand for tlbie instruction on Power7

[Michael Ellerman]

On Tue, 2015-10-06 at 11:25 -0700, Laura Abbott wrote:
> On 10/05/2015 08:35 PM, Michael Ellerman wrote:
> > On Fri, 2015-10-02 at 08:43 -0700, Laura Abbott wrote:
> >> Hi,
> >>
> >> We received a report (https://bugzilla.redhat.com/show_bug.cgi?id=1267395) of bad assembly
> >> when compiling on powerpc with little endian
> >
> > ...
> >
> >> After some discussion with the binutils folks, it turns out that the tlbie
> >> instruction actually requires another operand and binutils was updated to
> >> check for this https://sourceware.org/ml/binutils/2015-05/msg00133.html .
> >>
> >> The code sequence in arch/powerpc/include/asm/ppc_asm.h now needs to be updated:
> >>
> >> #if !defined(CONFIG_4xx) && !defined(CONFIG_8xx)
> >> #define tlbia                                   \
> >>           li      r4,1024;                        \
> >>           mtctr   r4;                             \
> >>           lis     r4,KERNELBASE@h;                \
> >> 0:      tlbie   r4;                             \
> >>           addi    r4,r4,0x1000;                   \
> >>           bdnz    0b
> >> #endif
> >>
> >> I don't know enough ppc assembly to properly fix this but I can test.
> >
> > How are you testing? This code is fairly old and I'm dubious if it still works.
> >
> > These days we have a ppc_md hook for flushing the TLB, ppc_md.flush_tlb().
> > Ideally the swsusp code would use that.
> 
> Testing would probably just be compile and maybe boot. I don't have regular
> access to the hardware. This problem just showed up for me when someone
> tried to compile Fedora rawhide with the latest binutils.

Right. The code in question is for software suspend, ie. hibernation, so that's
what needs testing if the code is going to change.

It was mostly written for G5 (543b9fd3528f6), though it later gained support
for 64-bit BookE (5a31057fc06c3).

I just tested it on a G5 here and amazingly it worked.

So it is working code, even if it is old and crufty.

>  From what I can tell, it looks like the .flush_tlb of the cpu_spec is only
> defined for power7 and power8 and I don't see a ppc_md.flush_tlb on the
> master branch.

Yes it's only defined for Power7 and Power8 at the moment. It definitely does
exist in Linus' master branch, but I'm not sure if that's the master branch
you're referring to.

> It's not clear what to do for the case where there is no
> flush_tlb function. Would filling in a .flush_tlb for all the PPC_BOOK3S_64
> with the existing tlbia sequence work?

It might, but it's not much of an improvement. Ideally we'd have an actually
correct sequence for each cpu type.

> It's also worth noting that the __flush_power7 uses tlbiel instead of tlbie.

Yeah that's a good point. It's not clear if the swsusp code wants to a local or
a global invalidate.


As an alternative, can you try adding a .machine push / .machine "power4" /
.machine pop, around the tlbie. That should tell the assembler to drop back to
power4 mode for that instruction, which should then do the right thing. There
are some examples in that file.

cheers

Re: Missing operand for tlbie instruction on Power7

[Segher Boessenkool]

On Wed, Oct 07, 2015 at 05:00:49PM +1100, Michael Ellerman wrote:
> > It's also worth noting that the __flush_power7 uses tlbiel instead of tlbie.
> 
> Yeah that's a good point. It's not clear if the swsusp code wants to a local or
> a global invalidate.

If I read the code right, this is called on the boot CPU when all the
non-boot CPUs are still (potentially) down, so if you would do a global
invalidate the non-boot CPUs might not even notice, so those need to do
a (local) invalidate after being brought up anyway?  Or they probably
need it before being brought down at all?  You figure it out, it makes
my brain hurt :-)

> As an alternative, can you try adding a .machine push / .machine "power4" /
> .machine pop, around the tlbie. That should tell the assembler to drop back to
> power4 mode for that instruction, which should then do the right thing. There
> are some examples in that file.

That will get the assembler to not complain, but it will assemble the wrong
instruction: the power7 instruction has the same opcode (but different
semantics).  So if you assemble a "tlbie r4" in power4 mode, a newer CPU
will see it as a "tlbie r4,r0" and do the wrong thing.


Segher

Re: Missing operand for tlbie instruction on Power7

[Michael Ellerman]

On Wed, 2015-10-07 at 02:19 -0500, Segher Boessenkool wrote:
> On Wed, Oct 07, 2015 at 05:00:49PM +1100, Michael Ellerman wrote:
> > > It's also worth noting that the __flush_power7 uses tlbiel instead of tlbie.
> > 
> > Yeah that's a good point. It's not clear if the swsusp code wants to a local or
> > a global invalidate.
> 
> If I read the code right, this is called on the boot CPU when all the
> non-boot CPUs are still (potentially) down, so if you would do a global
> invalidate the non-boot CPUs might not even notice, so those need to do
> a (local) invalidate after being brought up anyway?  Or they probably
> need it before being brought down at all?  You figure it out, it makes
> my brain hurt :-)

A good rule would be that every cpu does a local invalidate before turning on
the MMU. That would work for this case and also for kexec, kdump, junk left by
firmare etc. But I don't think we do that consistently in a way that works for
this code at the moment.

> > As an alternative, can you try adding a .machine push / .machine "power4" /
> > .machine pop, around the tlbie. That should tell the assembler to drop back to
> > power4 mode for that instruction, which should then do the right thing. There
> > are some examples in that file.
> 
> That will get the assembler to not complain, but it will assemble the wrong
> instruction: the power7 instruction has the same opcode (but different
> semantics).  So if you assemble a "tlbie r4" in power4 mode, a newer CPU
> will see it as a "tlbie r4,r0" and do the wrong thing.

Yeah, it would basically maintain the existing behaviour which is wrong but a
known quantity. I suspect no one has ever run this on Power7 or in fact
anything other than G5 or Book3E.

cheers

Re: Missing operand for tlbie instruction on Power7

[Josh Boyer]

On Wed, Oct 7, 2015 at 5:13 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> On Wed, 2015-10-07 at 02:19 -0500, Segher Boessenkool wrote:
>> On Wed, Oct 07, 2015 at 05:00:49PM +1100, Michael Ellerman wrote:
>> > > It's also worth noting that the __flush_power7 uses tlbiel instead of tlbie.
>> >
>> > Yeah that's a good point. It's not clear if the swsusp code wants to a local or
>> > a global invalidate.
>>
>> If I read the code right, this is called on the boot CPU when all the
>> non-boot CPUs are still (potentially) down, so if you would do a global
>> invalidate the non-boot CPUs might not even notice, so those need to do
>> a (local) invalidate after being brought up anyway?  Or they probably
>> need it before being brought down at all?  You figure it out, it makes
>> my brain hurt :-)
>
> A good rule would be that every cpu does a local invalidate before turning on
> the MMU. That would work for this case and also for kexec, kdump, junk left by
> firmare etc. But I don't think we do that consistently in a way that works for
> this code at the moment.
>
>> > As an alternative, can you try adding a .machine push / .machine "power4" /
>> > .machine pop, around the tlbie. That should tell the assembler to drop back to
>> > power4 mode for that instruction, which should then do the right thing. There
>> > are some examples in that file.
>>
>> That will get the assembler to not complain, but it will assemble the wrong
>> instruction: the power7 instruction has the same opcode (but different
>> semantics).  So if you assemble a "tlbie r4" in power4 mode, a newer CPU
>> will see it as a "tlbie r4,r0" and do the wrong thing.
>
> Yeah, it would basically maintain the existing behaviour which is wrong but a
> known quantity. I suspect no one has ever run this on Power7 or in fact
> anything other than G5 or Book3E.

Likely not, but leaving it broken just because it is known behavior
seems pretty weird to me.  I think Fedora will look at simply
disabling hibernation on ppc64 so the file isn't built at all.  Seems
to be a safer option.

josh

Re: Missing operand for tlbie instruction on Power7

[Michael Ellerman]

On Wed, 2015-10-07 at 10:31 -0400, Josh Boyer wrote:
> On Wed, Oct 7, 2015 at 5:13 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> > On Wed, 2015-10-07 at 02:19 -0500, Segher Boessenkool wrote:
> >> On Wed, Oct 07, 2015 at 05:00:49PM +1100, Michael Ellerman wrote:
> >> > > It's also worth noting that the __flush_power7 uses tlbiel instead of tlbie.
> >> >
> >> > Yeah that's a good point. It's not clear if the swsusp code wants to a local or
> >> > a global invalidate.
> >>
> >> If I read the code right, this is called on the boot CPU when all the
> >> non-boot CPUs are still (potentially) down, so if you would do a global
> >> invalidate the non-boot CPUs might not even notice, so those need to do
> >> a (local) invalidate after being brought up anyway?  Or they probably
> >> need it before being brought down at all?  You figure it out, it makes
> >> my brain hurt :-)
> >
> > A good rule would be that every cpu does a local invalidate before turning on
> > the MMU. That would work for this case and also for kexec, kdump, junk left by
> > firmare etc. But I don't think we do that consistently in a way that works for
> > this code at the moment.
> >
> >> > As an alternative, can you try adding a .machine push / .machine "power4" /
> >> > .machine pop, around the tlbie. That should tell the assembler to drop back to
> >> > power4 mode for that instruction, which should then do the right thing. There
> >> > are some examples in that file.
> >>
> >> That will get the assembler to not complain, but it will assemble the wrong
> >> instruction: the power7 instruction has the same opcode (but different
> >> semantics).  So if you assemble a "tlbie r4" in power4 mode, a newer CPU
> >> will see it as a "tlbie r4,r0" and do the wrong thing.
> >
> > Yeah, it would basically maintain the existing behaviour which is wrong but a
> > known quantity. I suspect no one has ever run this on Power7 or in fact
> > anything other than G5 or Book3E.
> 
> Likely not, but leaving it broken just because it is known behavior
> seems pretty weird to me.

In a universe where I have infinite time to fix random things we would
obviously do a proper fix :)

> I think Fedora will look at simply disabling hibernation on ppc64 so the file
> isn't built at all.  Seems to be a safer option.

It's safer for sure. Though you might have some G5 users who are using it and
notice it being disabled.

cheers

Re: Missing operand for tlbie instruction on Power7

[Josh Boyer]

On Wed, Oct 7, 2015 at 8:10 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> On Wed, 2015-10-07 at 10:31 -0400, Josh Boyer wrote:
>> On Wed, Oct 7, 2015 at 5:13 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>> > On Wed, 2015-10-07 at 02:19 -0500, Segher Boessenkool wrote:
>> >> On Wed, Oct 07, 2015 at 05:00:49PM +1100, Michael Ellerman wrote:
>> >> > > It's also worth noting that the __flush_power7 uses tlbiel instead of tlbie.
>> >> >
>> >> > Yeah that's a good point. It's not clear if the swsusp code wants to a local or
>> >> > a global invalidate.
>> >>
>> >> If I read the code right, this is called on the boot CPU when all the
>> >> non-boot CPUs are still (potentially) down, so if you would do a global
>> >> invalidate the non-boot CPUs might not even notice, so those need to do
>> >> a (local) invalidate after being brought up anyway?  Or they probably
>> >> need it before being brought down at all?  You figure it out, it makes
>> >> my brain hurt :-)
>> >
>> > A good rule would be that every cpu does a local invalidate before turning on
>> > the MMU. That would work for this case and also for kexec, kdump, junk left by
>> > firmare etc. But I don't think we do that consistently in a way that works for
>> > this code at the moment.
>> >
>> >> > As an alternative, can you try adding a .machine push / .machine "power4" /
>> >> > .machine pop, around the tlbie. That should tell the assembler to drop back to
>> >> > power4 mode for that instruction, which should then do the right thing. There
>> >> > are some examples in that file.
>> >>
>> >> That will get the assembler to not complain, but it will assemble the wrong
>> >> instruction: the power7 instruction has the same opcode (but different
>> >> semantics).  So if you assemble a "tlbie r4" in power4 mode, a newer CPU
>> >> will see it as a "tlbie r4,r0" and do the wrong thing.
>> >
>> > Yeah, it would basically maintain the existing behaviour which is wrong but a
>> > known quantity. I suspect no one has ever run this on Power7 or in fact
>> > anything other than G5 or Book3E.
>>
>> Likely not, but leaving it broken just because it is known behavior
>> seems pretty weird to me.
>
> In a universe where I have infinite time to fix random things we would
> obviously do a proper fix :)
>
>> I think Fedora will look at simply disabling hibernation on ppc64 so the file
>> isn't built at all.  Seems to be a safer option.
>
> It's safer for sure. Though you might have some G5 users who are using it and
> notice it being disabled.

The 5 of them will notice it being disabled and then they'll realize
they either get a working kernel minus hibernation, or they get no
kernel at all because it doesn't compile.

josh

Re: Missing operand for tlbie instruction on Power7

[Michael Ellerman]

On Wed, 2015-10-07 at 20:15 -0400, Josh Boyer wrote:
> On Wed, Oct 7, 2015 at 8:10 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> > On Wed, 2015-10-07 at 10:31 -0400, Josh Boyer wrote:
> >> On Wed, Oct 7, 2015 at 5:13 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> >> > On Wed, 2015-10-07 at 02:19 -0500, Segher Boessenkool wrote:
> >> >> On Wed, Oct 07, 2015 at 05:00:49PM +1100, Michael Ellerman wrote:
> >> >> > > It's also worth noting that the __flush_power7 uses tlbiel instead of tlbie.
> >> >> >
> >> >> > Yeah that's a good point. It's not clear if the swsusp code wants to a local or
> >> >> > a global invalidate.
> >> >>
> >> >> If I read the code right, this is called on the boot CPU when all the
> >> >> non-boot CPUs are still (potentially) down, so if you would do a global
> >> >> invalidate the non-boot CPUs might not even notice, so those need to do
> >> >> a (local) invalidate after being brought up anyway?  Or they probably
> >> >> need it before being brought down at all?  You figure it out, it makes
> >> >> my brain hurt :-)
> >> >
> >> > A good rule would be that every cpu does a local invalidate before turning on
> >> > the MMU. That would work for this case and also for kexec, kdump, junk left by
> >> > firmare etc. But I don't think we do that consistently in a way that works for
> >> > this code at the moment.
> >> >
> >> >> > As an alternative, can you try adding a .machine push / .machine "power4" /
> >> >> > .machine pop, around the tlbie. That should tell the assembler to drop back to
> >> >> > power4 mode for that instruction, which should then do the right thing. There
> >> >> > are some examples in that file.
> >> >>
> >> >> That will get the assembler to not complain, but it will assemble the wrong
> >> >> instruction: the power7 instruction has the same opcode (but different
> >> >> semantics).  So if you assemble a "tlbie r4" in power4 mode, a newer CPU
> >> >> will see it as a "tlbie r4,r0" and do the wrong thing.
> >> >
> >> > Yeah, it would basically maintain the existing behaviour which is wrong but a
> >> > known quantity. I suspect no one has ever run this on Power7 or in fact
> >> > anything other than G5 or Book3E.
> >>
> >> Likely not, but leaving it broken just because it is known behavior
> >> seems pretty weird to me.
> >
> > In a universe where I have infinite time to fix random things we would
> > obviously do a proper fix :)
> >
> >> I think Fedora will look at simply disabling hibernation on ppc64 so the file
> >> isn't built at all.  Seems to be a safer option.
> >
> > It's safer for sure. Though you might have some G5 users who are using it and
> > notice it being disabled.
> 
> The 5 of them will notice it being disabled and then they'll realize
> they either get a working kernel minus hibernation, or they get no
> kernel at all because it doesn't compile.

Sure. But if we do the machine push thing they'll get both :)

And I doubt it's 5, 2 is more likely.

cheers