Missing operand for tlbie instruction on Power7

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

_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://lists.ozlabs.org/listinfo/linuxppc-dev

arch/powerpc/kernel/swsusp_asm64.S: Assembler messages:

Quoted 6 lines

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

Quoted 1 line

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

Could you please test the patch attached?

-0:     tlbie   r4;                             \
+0:     tlbie   r4, 0;                          \

Quoted 7 lines · Denis Kirjanov

arch/powerpc/kernel/swsusp_asm64.S: Assembler messages:

Quoted 6 lines

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]

Quoted 3 lines

Quoted 1 line

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

Could you please test the patch attached?

[snip]

Quoted 2 lines

-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

Quoted 15 lines

Quoted 2 lines

-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

Quoted 4 lines

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?

Quoted 20 lines · Denis Kirjanov

Quoted 19 lines

Quoted 2 lines

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

Quoted 2 lines

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

Quoted 20 lines · Denis Kirjanov

Quoted 19 lines

Quoted 2 lines

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

Quoted 2 lines

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.

Quoted 5 lines

Quoted 2 lines

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. :-)

Quoted 9 lines · Peter Bergner

Quoted 5 lines

Quoted 2 lines

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:

Quoted 6 lines

   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

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.

Quoted 4 lines · 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

...

Quoted 16 lines

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

Quoted 1 line

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

Quoted 26 lines · Michael Ellerman

Quoted 4 lines · 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

...

Quoted 17 lines

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.

Quoted 4 lines

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.

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.

Quoted 1 line

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.

Quoted 4 lines · Michael Ellerman

Quoted 1 line

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 :-)

Quoted 4 lines

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.

Quoted 6 lines · Segher Boessenkool

Quoted 4 lines · Michael Ellerman

Quoted 1 line

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.

Quoted 6 lines

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.

Quoted 5 lines

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 29 lines · Michael Ellerman

Quoted 6 lines · Segher Boessenkool

Quoted 4 lines · Michael Ellerman

Quoted 1 line

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.

Quoted 6 lines

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.

Quoted 9 lines

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 30 lines · Josh Boyer

Quoted 29 lines · Michael Ellerman

Quoted 6 lines · Segher Boessenkool

Quoted 4 lines · Michael Ellerman

Quoted 1 line

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.

Quoted 6 lines

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.

Quoted 9 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 3 lines

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 :)

Quoted 2 lines

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.

Quoted 44 lines · Michael Ellerman

Quoted 30 lines · Josh Boyer

Quoted 29 lines · Michael Ellerman

Quoted 6 lines · Segher Boessenkool

Quoted 4 lines · Michael Ellerman

Quoted 1 line

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.

Quoted 6 lines

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.

Quoted 9 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 3 lines

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 :)

Quoted 2 lines

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.

Quoted 2 lines

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.