i386 very early memory detection cleanup patch breaks the build

i386 very early memory detection cleanup patch breaks the build

[James Bottomley]

The attached should fix it again.

This tampering with the trampoline was extraneous to the actual patch. 
The rule should be that if you don't understand what something is doing,
don't try to fix it.

In this case CONFIG_X86_TRAMPOLINE is needed for the subarch's that
provide their own SMP code but still use the standard trampoline.  I
always thought the visws used the trampoline even in UP boot, but if it
doesn't, just take out the X86_VISWS dependency.

James

P.S.  I'm still compiling, I'll yell again if the patch breaks at
runtime too.

===== arch/i386/Kconfig 1.108 vs edited =====
--- 1.108/arch/i386/Kconfig	Fri Mar 12 03:33:00 2004
+++ edited/arch/i386/Kconfig	Sat Mar 13 10:55:36 2004
@@ -1332,6 +1332,11 @@
 	depends on !(X86_VISWS || X86_VOYAGER)
 	default y
 
+config X86_TRAMPOLINE
+	bool
+	depends on SMP || X86_VISWS
+	default y
+
 config PC
 	bool
 	depends on X86 && !EMBEDDED
===== arch/i386/defconfig 1.107 vs edited =====
--- 1.107/arch/i386/defconfig	Fri Mar 12 03:30:22 2004
+++ edited/arch/i386/defconfig	Sat Mar 13 10:55:37 2004
@@ -1212,4 +1212,5 @@
 CONFIG_X86_SMP=y
 CONFIG_X86_HT=y
 CONFIG_X86_BIOS_REBOOT=y
+CONFIG_X86_TRAMPOLINE=y
 CONFIG_PC=y
===== arch/i386/kernel/Makefile 1.56 vs edited =====
--- 1.56/arch/i386/kernel/Makefile	Fri Mar 12 03:30:22 2004
+++ edited/arch/i386/kernel/Makefile	Sat Mar 13 10:55:37 2004
@@ -18,7 +18,8 @@
 obj-$(CONFIG_X86_CPUID)		+= cpuid.o
 obj-$(CONFIG_MICROCODE)		+= microcode.o
 obj-$(CONFIG_APM)		+= apm.o
-obj-$(CONFIG_X86_SMP)		+= smp.o smpboot.o trampoline.o
+obj-$(CONFIG_X86_SMP)		+= smp.o smpboot.o
+obj-$(CONFIG_X86_TRAMPOLINE)	+= trampoline.o
 obj-$(CONFIG_X86_MPPARSE)	+= mpparse.o
 obj-$(CONFIG_X86_LOCAL_APIC)	+= apic.o nmi.o
 obj-$(CONFIG_X86_IO_APIC)	+= io_apic.o

Re: i386 very early memory detection cleanup patch breaks the build

[James Bottomley]

On Sat, 2004-03-13 at 12:15, James Bottomley wrote:
> P.S.  I'm still compiling, I'll yell again if the patch breaks at
> runtime too.

OK, I confirm a total boot time failure as well.  It looks like the CPUs
are unable to map their CPI memory.

Reversing the patch entirely fixes the problem.

I'll dig around and see if I can uncover the reason.

James

Re: i386 very early memory detection cleanup patch breaks the build

[H. Peter Anvin]

James Bottomley wrote:
> The attached should fix it again.

Could you perhaps describe which architecture this is a problem on, and 
what its entry condition looks like?

> This tampering with the trampoline was extraneous to the actual patch. 
> The rule should be that if you don't understand what something is doing,
> don't try to fix it.

I removed it because I removed the VISWS dependency, thus making it 
redundant.  What you seem to be saying is that the dependency should 
have been on SMP not X86_SMP; if that's the issue then please make it so.

I think you just needed to apply your own rule to the above statement.

> In this case CONFIG_X86_TRAMPOLINE is needed for the subarch's that
> provide their own SMP code but still use the standard trampoline.  I
> always thought the visws used the trampoline even in UP boot, but if it
> doesn't, just take out the X86_VISWS dependency.

It doesn't anymore.  The only reason it did was because of stupid 
partitioning between head.S and trampoline.S, which the patch cleans up.

	-hpa

Re: i386 very early memory detection cleanup patch breaks the build

[James Bottomley]

On Sat, 2004-03-13 at 16:43, H. Peter Anvin wrote:
> Could you perhaps describe which architecture this is a problem on, and 
> what its entry condition looks like?

This is the Voyager architecture (mach-voyager)

> I removed it because I removed the VISWS dependency, thus making it 
> redundant.  What you seem to be saying is that the dependency should 
> have been on SMP not X86_SMP; if that's the issue then please make it so.
> 
> I think you just needed to apply your own rule to the above statement.

If you mean the dependency should be

	depends X86_SMP || (VOYAGER && SMP)

Then yes, I'm happy with that.

I'm still debugging the boot time failure.  As far as I can tell it
looks like ioremap is failing (this is after paging_init); does this
trigger any associations?

James


James

Re: i386 very early memory detection cleanup patch breaks the build

[H. Peter Anvin]

James Bottomley wrote:
> 
>>I removed it because I removed the VISWS dependency, thus making it 
>>redundant.  What you seem to be saying is that the dependency should 
>>have been on SMP not X86_SMP; if that's the issue then please make it so.
>>
>>I think you just needed to apply your own rule to the above statement.
> 
> If you mean the dependency should be
> 
> 	depends X86_SMP || (VOYAGER && SMP)
> 
> Then yes, I'm happy with that.
> 

Actually, I just meant changing:

-obj-$(CONFIG_X86_SMP)		+= smp.o smpboot.o trampoline.o
+obj-$(CONFIG_X86_SMP)		+= smp.o smpboot.o
+obj-$(CONFIG_SMP)		+= trampoline.o

... which is more like what the original code is doing, minus VISWS.

> I'm still debugging the boot time failure.  As far as I can tell it
> looks like ioremap is failing (this is after paging_init); does this
> trigger any associations?

Nope.  It shouldn't be using the boot page tables after paging_init, and 
even so, the "new" boot page tables look just like the "old" ones except 
there might be more of them, so there are two resaons that shouldn't be 
happening.  I'd have been less surprised if you'd seen a problem with 
boot_ioremap(), although even that shouldn't really be different...

My main guess would be a porting problem (_end -> init_pg_tables_end) in 
discontig.c, which I believe Voyager uses, right?

I don't have access to any real subarchitectures (I have a visws now, 
but I haven't actually been able to run it yet), so the discontig stuff 
didn't get tested; on the other hand the change in there was quite trivial.

Sorry for not being able to be more helpful, but I'm surrounded by boxes 
and this is the last weekend I have to pack before I move houses...

	-hpa

Re: i386 very early memory detection cleanup patch breaks the build

[James Bottomley]

On Sat, 2004-03-13 at 17:29, H. Peter Anvin wrote:
> Actually, I just meant changing:
> 
> -obj-$(CONFIG_X86_SMP)		+= smp.o smpboot.o trampoline.o
> +obj-$(CONFIG_X86_SMP)		+= smp.o smpboot.o
> +obj-$(CONFIG_SMP)		+= trampoline.o
> 
> ... which is more like what the original code is doing, minus VISWS.

No, that looks more magical and even worse.  The idea of the subarch
code is to separate out pieces of i386/kernel that need to be added in
separately.  Having trampoline depend on CONFIG_X86_TRAMPOLINE which
then depends on (X86_SMP) || (VOYAGER && SMP) expresses exactly what's
going on.  The above code is begging for someone to try to "correct" it.

> Nope.  It shouldn't be using the boot page tables after paging_init, and 
> even so, the "new" boot page tables look just like the "old" ones except 
> there might be more of them, so there are two resaons that shouldn't be 
> happening.  I'd have been less surprised if you'd seen a problem with 
> boot_ioremap(), although even that shouldn't really be different...
> 
> My main guess would be a porting problem (_end -> init_pg_tables_end) in 
> discontig.c, which I believe Voyager uses, right?

Actually, no, it's not a discontig mem subarch.  It has a different SMP
setup (VICs instead of APICs...and the failing beast is the QIC which
uses cache line interference to transmit cross processor interrupts
using the last page of physical memory).

> I don't have access to any real subarchitectures (I have a visws now, 
> but I haven't actually been able to run it yet), so the discontig stuff 
> didn't get tested; on the other hand the change in there was quite trivial.
> 
> Sorry for not being able to be more helpful, but I'm surrounded by boxes 
> and this is the last weekend I have to pack before I move houses...

That's OK.  Subarch boot sequences are one of the most fragile things in
the kernel.  I suspect the ioremap problem is probably a manifestation
of something happening earlier ... I'll see if I can find it.

James

Re: i386 very early memory detection cleanup patch breaks the build

[James Bottomley]

OK, I found it, it was a swapper_pg_dir replacement assumption that
breaks if pg0 isn't in the known location.  Voyager still does odd
tricks with this because it also has a 486 SMP configuration (which I've
yet to test still boots...).

The attached patch fixes everything for me (do we agree on the
trampoline thing as the final form?)

James

===== arch/i386/Kconfig 1.108 vs edited =====
--- 1.108/arch/i386/Kconfig	Fri Mar 12 03:33:00 2004
+++ edited/arch/i386/Kconfig	Sat Mar 13 19:52:50 2004
@@ -1332,6 +1332,11 @@
 	depends on !(X86_VISWS || X86_VOYAGER)
 	default y
 
+config X86_TRAMPOLINE
+	bool
+	depends on X86_SMP || (X86_VOYAGER && SMP)
+	default y
+
 config PC
 	bool
 	depends on X86 && !EMBEDDED
===== arch/i386/defconfig 1.107 vs edited =====
--- 1.107/arch/i386/defconfig	Fri Mar 12 03:30:22 2004
+++ edited/arch/i386/defconfig	Sat Mar 13 14:14:02 2004
@@ -1212,4 +1212,5 @@
 CONFIG_X86_SMP=y
 CONFIG_X86_HT=y
 CONFIG_X86_BIOS_REBOOT=y
+CONFIG_X86_TRAMPOLINE=y
 CONFIG_PC=y
===== arch/i386/kernel/Makefile 1.56 vs edited =====
--- 1.56/arch/i386/kernel/Makefile	Fri Mar 12 03:30:22 2004
+++ edited/arch/i386/kernel/Makefile	Sat Mar 13 14:14:02 2004
@@ -18,7 +18,8 @@
 obj-$(CONFIG_X86_CPUID)		+= cpuid.o
 obj-$(CONFIG_MICROCODE)		+= microcode.o
 obj-$(CONFIG_APM)		+= apm.o
-obj-$(CONFIG_X86_SMP)		+= smp.o smpboot.o trampoline.o
+obj-$(CONFIG_X86_SMP)		+= smp.o smpboot.o
+obj-$(CONFIG_X86_TRAMPOLINE)	+= trampoline.o
 obj-$(CONFIG_X86_MPPARSE)	+= mpparse.o
 obj-$(CONFIG_X86_LOCAL_APIC)	+= apic.o nmi.o
 obj-$(CONFIG_X86_IO_APIC)	+= io_apic.o
===== arch/i386/mach-voyager/voyager_smp.c 1.18 vs edited =====
--- 1.18/arch/i386/mach-voyager/voyager_smp.c	Mon Jan 19 00:32:52 2004
+++ edited/arch/i386/mach-voyager/voyager_smp.c	Sat Mar 13 19:50:47 2004
@@ -623,7 +623,9 @@
 		((virt_to_phys(page_table_copies)) & PAGE_MASK)
 		| _PAGE_RW | _PAGE_USER | _PAGE_PRESENT;
 #else
-	((unsigned long *)swapper_pg_dir)[0] = 0x102007;
+	((unsigned long *)swapper_pg_dir)[0] = 
+		(virt_to_phys(pg0) & PAGE_MASK)
+		| _PAGE_RW | _PAGE_USER | _PAGE_PRESENT;
 #endif
 
 	if(quad_boot) {

Re: i386 very early memory detection cleanup patch breaks the build

[H. Peter Anvin]

James Bottomley wrote:
> OK, I found it, it was a swapper_pg_dir replacement assumption that
> breaks if pg0 isn't in the known location.  Voyager still does odd
> tricks with this because it also has a 486 SMP configuration (which I've
> yet to test still boots...).

Ick.  Hard-coded address error.

> The attached patch fixes everything for me (do we agree on the
> trampoline thing as the final form?)

It's a bit ugly, but good enough for me.

Andrew, I think this patch should go in.

Thanks,

	-hpa