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

initializing SLB
In-Reply-To: [off-list ref]
From: Ishizaki Kou <redacted>
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:

quoted

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

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.

Ben-san,

On Thu, 2007-09-27 at 17:01 +0900, kou.ishizaki@toshiba.co.jp wrote:

quoted

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

On Thu, 2007-09-27 at 20:22 +0900, Ishizaki Kou wrote:

Ben-san,

quoted

On Thu, 2007-09-27 at 17:01 +0900, kou.ishizaki@toshiba.co.jp wrote:

quoted

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.

On Thu, 2007-09-27 at 20:22 +0900, Ishizaki Kou wrote:

Ben-san,

quoted

On Thu, 2007-09-27 at 17:01 +0900, kou.ishizaki@toshiba.co.jp wrote:

quoted

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.

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.
 

Ben-san,

quoted

It should have been set in setup_64.c, in setup_paca() (which is

called

quoted

twice) in that statement:

	local_paca = &paca[cpu];

As local_paca is defined as being a variable held in register r13.

Maybe

quoted

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

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.

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