Re: [PATCH 1/7] PowerPC64: Not to insert EA=0 entry at

Re: [PATCH 1/7] PowerPC64: Not to insert EA=0 entry at

[kou.ishizaki]

initializing SLB
In-Reply-To: <200709261415.35404.arnd@arndb.de>
From: Ishizaki Kou <kou.ishizaki@toshiba.co.jp>
X-Mailer: Mew version 4.2 on Emacs 21.3 / Mule 5.0 (SAKAKI)
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

> On Wednesday 26 September 2007, Ishizaki Kou wrote:
> > This is a workaround NOT to insert EA=0 entry at initializing SLB.
> > Without this patch, you can see /sbin/init hanging at a machine
> > which has less or equal than 256MB memory.

> Can you elaborate? I don't understand why /sbin/init will hang
> because of an incorrect SLB entry, or how that entry gets
> initialized from get_paca()->kstack.

For latter question, following scenario will occur:
mm/slb.c:slb_flush_and_rebolt() function rebolts SLB entry 1 and 2
by VMALLOC segment and kernel stack segment respectively, but
if we have get_paca()->kstack is set to zero, following condition
will be false,
------------------------------------------------------------------------
105: if ((ksp_esid_data & ESID_MASK) == PAGE_OFFSET) {
------------------------------------------------------------------------
so following line will create shadowed bolted SLB entry 2 with EA=0
------------------------------------------------------------------------
111: slb_shadow_update(get_paca()->kstack, lflags, 2);
------------------------------------------------------------------------
and following lines will create real bolted SLB entry 2.
------------------------------------------------------------------------
125: "r"(mk_vsid_data(ksp_esid_data, lflags),
126: "r"(ksp_esid_data)
------------------------------------------------------------------------

Celleb does not set get_paca()->kstack properly (I don't know which
function should set it up), so we need to workaround.

If we have more than 256MB, SLB entry 2 will be overridden with second
segment of the main storage, so the problem will be washed out.

For former question, bolted SLB entries should not be erased even
if /sbin/init is exec()'ed. Our /sbin/init starts at 0x01000000,
at Segment=0. So SLB entry 2 matches. And the processor fetches
instructions from the kernel, not /sbin/init.

Do above expressions make sense?

> Are you still trying to find a better fix for 2.6.24, or do
> you intend to have this patch go in for now?

I believe setting get_paca()->kstack properly will solve this problem.
Do you have any idea? 

> Also, I did not get a patch 4/7 and 5/7. Did you send them to
> some other mailing list, or did they get lost on the way?

I don't have them too. It seems that they are lost. I resent them.

Best regards,
Kou Ishizaki

Re: [PATCH 1/7] PowerPC64: Not to insert EA=0 entry at

[Benjamin Herrenschmidt]

On Thu, 2007-09-27 at 17:01 +0900, kou.ishizaki@toshiba.co.jp wrote:
> 
> Celleb does not set get_paca()->kstack properly (I don't know which
> function should set it up), so we need to workaround.

paca->kstack is set in asm (via the PACAKSAVE macro), from either
__secondary_start for non-boot CPUs or from start_here_common for the
boot CPU.

slb_flush_and_rebolt() should not be called before that happens.

How do you end up with kstack set to 0 ?

Ben.

Re: [PATCH 1/7] PowerPC64: Not to insert EA=0 entry at

[Ishizaki Kou]

Ben-san,

> On Thu, 2007-09-27 at 17:01 +0900, kou.ishizaki@toshiba.co.jp wrote:
>
> > Celleb does not set get_paca()->kstack properly (I don't know which
> > function should set it up), so we need to workaround.
>
> paca->kstack is set in asm (via the PACAKSAVE macro), from either
> __secondary_start for non-boot CPUs or from start_here_common for the
> boot CPU.
>
> slb_flush_and_rebolt() should not be called before that happens.
>
> How do you end up with kstack set to 0 ?

I found r13 is not set before entering start_here_common for boot cpu.
For non-boot threads, __secondary_start will set r13 properly.

So the problem is to set r13 correct PACA address before entering
start_here_common. Should it set before entering kernel or will some
patch make sense?

Best regards,
Kou Ishizaki

Re: [PATCH 1/7] PowerPC64: Not to insert EA=0 entry at

[Benjamin Herrenschmidt]

On Thu, 2007-09-27 at 20:22 +0900, Ishizaki Kou wrote:
> Ben-san,
> 
> > On Thu, 2007-09-27 at 17:01 +0900, kou.ishizaki@toshiba.co.jp wrote:
> >
> > > Celleb does not set get_paca()->kstack properly (I don't know which
> > > function should set it up), so we need to workaround.
> >
> > paca->kstack is set in asm (via the PACAKSAVE macro), from either
> > __secondary_start for non-boot CPUs or from start_here_common for the
> > boot CPU.
> >
> > slb_flush_and_rebolt() should not be called before that happens.
> >
> > How do you end up with kstack set to 0 ?
> 
> I found r13 is not set before entering start_here_common for boot cpu.
> For non-boot threads, __secondary_start will set r13 properly.
> 
> So the problem is to set r13 correct PACA address before entering
> start_here_common. Should it set before entering kernel or will some
> patch make sense?

That is scary... we seem to have lost an mfsprg r13,SPRN_SPRG3 somewhere
along the way, I'm surprised things boot at all, or maybe I'm missing
something here... Paul ?

Ben.

Re: [PATCH 1/7] PowerPC64: Not to insert EA=0 entry at

[Benjamin Herrenschmidt]

On Thu, 2007-09-27 at 20:22 +0900, Ishizaki Kou wrote:
> Ben-san,
> 
> > On Thu, 2007-09-27 at 17:01 +0900, kou.ishizaki@toshiba.co.jp wrote:
> >
> > > Celleb does not set get_paca()->kstack properly (I don't know which
> > > function should set it up), so we need to workaround.
> >
> > paca->kstack is set in asm (via the PACAKSAVE macro), from either
> > __secondary_start for non-boot CPUs or from start_here_common for the
> > boot CPU.
> >
> > slb_flush_and_rebolt() should not be called before that happens.
> >
> > How do you end up with kstack set to 0 ?
> 
> I found r13 is not set before entering start_here_common for boot cpu.
> For non-boot threads, __secondary_start will set r13 properly.
> 
> So the problem is to set r13 correct PACA address before entering
> start_here_common. Should it set before entering kernel or will some
> patch make sense?

It should have been set in setup_64.c, in setup_paca() (which is called
twice) in that statement:

	local_paca = &paca[cpu];

As local_paca is defined as being a variable held in register r13. Maybe
something bad's happening with the compiler ?

Ben.

Re: [PATCH 1/7] PowerPC64: Not to insert EA=0 entry at

[Benjamin Herrenschmidt]

> It should have been set in setup_64.c, in setup_paca() (which is called
> twice) in that statement:
> 
> 	local_paca = &paca[cpu];
> 
> As local_paca is defined as being a variable held in register r13. Maybe
> something bad's happening with the compiler ?

Can you check early_setup disassembly, see if r13 is assigned (it should
be in two spots) and if it's clobbered by the compiler somewhere ?

Also, you may want to try adding --ffixed-r13 to the CFLAGS in the
makefile and let us know if it makes a difference... r13 is marked
reserved by the ABI but segher seems to imply that gcc may decide to ues
it anyway (ouch !)

Ben.
 

Re: [PATCH 1/7] PowerPC64: Not to insert EA=0 entry at

[Ishizaki Kou]

Ben-san,

> > It should have been set in setup_64.c, in setup_paca() (which is
called
> > twice) in that statement:
> > 
> > 	local_paca = &paca[cpu];
> > 
> > As local_paca is defined as being a variable held in register r13.
Maybe
> > something bad's happening with the compiler ?

> Can you check early_setup disassembly, see if r13 is assigned (it
should
> be in two spots) and if it's clobbered by the compiler somewhere ?

> Also, you may want to try adding --ffixed-r13 to the CFLAGS in the
> makefile and let us know if it makes a difference... r13 is marked
> reserved by the ABI but segher seems to imply that gcc may decide to
ues
> it anyway (ouch !)

I reviewed early_setup() disassembly sets r13 correctly. Now I found
the kernel works well without our hack, so I think I can decline the
patch [1/7].

Commit edd0622bd2e8f755c960827e15aa6908c3c5aa94 seems to break the
kernel and commit 175587cca7447daf5a13e4a53d32360ed8cba804 backs out
the symptom.  Calling slb_flush_and_rebolt() at slb_initialize()
function breaks the SLB table since it is called before setting up
PACA.

Thank you for your hint.

Best regards,
Kou Ishizaki

Re: [PATCH 1/7] PowerPC64: Not to insert EA=0 entry at

[Benjamin Herrenschmidt]

On Fri, 2007-09-28 at 17:04 +0900, Ishizaki Kou wrote:
> Commit edd0622bd2e8f755c960827e15aa6908c3c5aa94 seems to break the
> kernel and commit 175587cca7447daf5a13e4a53d32360ed8cba804 backs out
> the symptom.  Calling slb_flush_and_rebolt() at slb_initialize()
> function breaks the SLB table since it is called before setting up
> PACA.
> 
> Thank you for your hint.

Ah yes, there was a temporary breakage there that got fixed indeed,
I remember.

Cheers,
Ben.

Re: [PATCH 1/7] PowerPC64: Not to insert EA=0 entry at

[Segher Boessenkool]

> Also, you may want to try adding --ffixed-r13 to the CFLAGS in the
> makefile and let us know if it makes a difference... r13 is marked
> reserved by the ABI but segher seems to imply that gcc may decide to 
> ues
> it anyway (ouch !)

That's what I thought for a second, but I misread the GCC sources.

This however brings up another point: for PPC32, -ffixed-r2 is
superfluous for all the same reasons as why we don't need -ffixed-r13
on PPC64.  Is there any reason to keep it or is this just historical
cruft?


Segher