Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1240139 > unrolled thread

Re: Missing operand for tlbie instruction on Power7

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: Missing operand for tlbie instruction on Power7 Michael Ellerman <mpe@ellerman.id.au> - 2015-10-06 05:40 +0200
    Re: Missing operand for tlbie instruction on Power7 Laura Abbott <labbott@redhat.com> - 2015-10-06 20:30 +0200
      Re: Missing operand for tlbie instruction on Power7 Michael Ellerman <mpe@ellerman.id.au> - 2015-10-07 08:10 +0200
        Re: Missing operand for tlbie instruction on Power7 Segher Boessenkool <segher@kernel.crashing.org> - 2015-10-07 10:30 +0200
          Re: Missing operand for tlbie instruction on Power7 Michael Ellerman <mpe@ellerman.id.au> - 2015-10-07 11:20 +0200
            Re: Missing operand for tlbie instruction on Power7 Josh Boyer <jwboyer@fedoraproject.org> - 2015-10-07 16:40 +0200
              Re: Missing operand for tlbie instruction on Power7 Josh Boyer <jwboyer@fedoraproject.org> - 2015-10-08 02:20 +0200
                Re: Missing operand for tlbie instruction on Power7 Michael Ellerman <mpe@ellerman.id.au> - 2015-10-08 02:40 +0200
              Re: Missing operand for tlbie instruction on Power7 Michael Ellerman <mpe@ellerman.id.au> - 2015-10-08 02:20 +0200

#1240139 — Re: Missing operand for tlbie instruction on Power7

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1240844

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241163

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241230

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241270

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



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241562

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241876

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241903

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241879

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web