Make the x86_{phys,virt}_bits common for 32- and 64-bits, and use the
former in ioremap's phys_addr_valid() check also on 32bit/PAE.

Signed-off-by: Jan Beulich <jbeulich@novell.com>

---
 arch/x86/kernel/cpu/common.c |   17 +++++++++++++++--
 arch/x86/mm/ioremap.c        |   15 +++++++--------
 include/asm-x86/processor.h  |    4 ++--
 3 files changed, 24 insertions(+), 12 deletions(-)

--- linux-tip.orig/arch/x86/kernel/cpu/common.c
+++ linux-tip/arch/x86/kernel/cpu/common.c
@@ -439,6 +439,11 @@ void __cpuinit cpu_detect(struct cpuinfo
 			c->x86_cache_alignment = c->x86_clflush_size;
 		}
 	}
+
+#ifdef CONFIG_X86_32
+	if (cpu_has(c, X86_FEATURE_PAE) || cpu_has(c, X86_FEATURE_PSE36))
+		c->x86_phys_bits = 36;
+#endif
 }
 
 static void __cpuinit get_cpu_cap(struct cpuinfo_x86 *c)
@@ -464,14 +469,18 @@ static void __cpuinit get_cpu_cap(struct
 		}
 	}
 
-#ifdef CONFIG_X86_64
 	if (c->extended_cpuid_level >= 0x80000008) {
 		u32 eax = cpuid_eax(0x80000008);
 
 		c->x86_virt_bits = (eax >> 8) & 0xff;
 		c->x86_phys_bits = eax & 0xff;
+		/* CPUID workaround for Intel 0F33/0F34 CPU */
+		if (c->x86_vendor == X86_VENDOR_INTEL
+		    && c->x86 == 0xF && c->x86_model == 0x3
+		    && (c->x86_mask == 0x3
+			|| c->x86_mask == 0x4))
+			c->x86_phys_bits = 36;
 	}
-#endif
 
 	if (c->extended_cpuid_level >= 0x80000007)
 		c->x86_power = cpuid_edx(0x80000007);
@@ -519,6 +528,8 @@ static void __init early_identify_cpu(st
 	c->x86_clflush_size = 64;
 #else
 	c->x86_clflush_size = 32;
+	c->x86_phys_bits = 32;
+	c->x86_virt_bits = 32;
 #endif
 	c->x86_cache_alignment = c->x86_clflush_size;
 
@@ -641,6 +652,8 @@ static void __cpuinit identify_cpu(struc
 #else
 	c->cpuid_level = -1;	/* CPUID not detected */
 	c->x86_clflush_size = 32;
+	c->x86_phys_bits = 32;
+	c->x86_virt_bits = 32;
 #endif
 	c->x86_cache_alignment = c->x86_clflush_size;
 	memset(&c->x86_capability, 0, sizeof c->x86_capability);
--- linux-tip.orig/arch/x86/mm/ioremap.c
+++ ...
[ message continues ]

applied to tip/x86/core, thanks Jan.

	Ingo
--

[ message continues ]

hm, this commit quickly broke a -tip testsystem. The system booted up 
fine but had no networking afterwards. (the eth0 interface came up fine 
but no interrupts were emitted)

i've pushed out the bad tree into tip/tmp.virt_bits.bad. The bad config 
is attached.

just check out tip/tmp.virt_bits.bad, boot into the kernel and see 
forcedeth networking break (if you have a box with forcedeth). It's 
reproducible. I've attached a 'good' and a 'bad' boot log as well.

the 2.6.27-rc6-tip-00469-g09375dc-dirty kernel (boot.good) had your 
commit reverted.

	Ingo
[ message continues ]

I'm really sorry for that, yet another merge oversight (not caught because
only re-tested on x86-64). Here's a better one.

********************************************************

Make the x86_{phys,virt}_bits common for 32- and 64-bits, and use the
former in ioremap's phys_addr_valid() check also on 32bit/PAE.

Signed-off-by: Jan Beulich <jbeulich@novell.com>

---
 arch/x86/kernel/cpu/common.c |   17 +++++++++++++++--
 arch/x86/mm/ioremap.c        |   17 ++++++++---------
 include/asm-x86/processor.h  |    4 ++--
 3 files changed, 25 insertions(+), 13 deletions(-)

--- linux-tip.orig/arch/x86/kernel/cpu/common.c
+++ linux-tip/arch/x86/kernel/cpu/common.c
@@ -439,6 +439,11 @@ void __cpuinit cpu_detect(struct cpuinfo
 			c->x86_cache_alignment = c->x86_clflush_size;
 		}
 	}
+
+#ifdef CONFIG_X86_32
+	if (cpu_has(c, X86_FEATURE_PAE) || cpu_has(c, X86_FEATURE_PSE36))
+		c->x86_phys_bits = 36;
+#endif
 }
 
 static void __cpuinit get_cpu_cap(struct cpuinfo_x86 *c)
@@ -464,14 +469,18 @@ static void __cpuinit get_cpu_cap(struct
 		}
 	}
 
-#ifdef CONFIG_X86_64
 	if (c->extended_cpuid_level >= 0x80000008) {
 		u32 eax = cpuid_eax(0x80000008);
 
 		c->x86_virt_bits = (eax >> 8) & 0xff;
 		c->x86_phys_bits = eax & 0xff;
+		/* CPUID workaround for Intel 0F33/0F34 CPU */
+		if (c->x86_vendor == X86_VENDOR_INTEL
+		    && c->x86 == 0xF && c->x86_model == 0x3
+		    && (c->x86_mask == 0x3
+			|| c->x86_mask == 0x4))
+			c->x86_phys_bits = 36;
 	}
-#endif
 
 	if (c->extended_cpuid_level >= 0x80000007)
 		c->x86_power = cpuid_edx(0x80000007);
@@ -519,6 +528,8 @@ static void __init early_identify_cpu(st
 	c->x86_clflush_size = 64;
 #else
 	c->x86_clflush_size = 32;
+	c->x86_phys_bits = 32;
+	c->x86_virt_bits = 32;
 #endif
 	c->x86_cache_alignment = c->x86_clflush_size;
 
@@ -641,6 +652,8 @@ static void __cpuinit identify_cpu(struc
 #else
 	c->cpuid_level = -1;	/* CPUID not detected */
 	c->x86_clflush_size = 32;
+	c->x86_phys_bits = 32;
+	c->x86_virt_bits = ...
[ message continues ]

ah, i see, the delta below. Nasty.

	Ingo

------------->
 arch/x86/mm/ioremap.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 8703468..63fb796 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -22,7 +22,7 @@
 #include <asm/pgalloc.h>
 #include <asm/pat.h>
 
-static inline int phys_addr_valid(unsigned long addr)
+static inline int phys_addr_valid(resource_size_t addr)
 {
 #ifdef CONFIG_RESOURCES_64BIT
 	return !(addr >> boot_cpu_data.x86_phys_bits);
--

[ message continues ]

the attached config fails in a similar way.

	Ingo
[ message continues ]

Hmm, yes, other than in .27, -tip derives resource_size_t from phys_addr_t,
regardless of CONFIG_RESOURCES_64BIT (and the config you provided
is a non-PAE one). I have to question that change, which I'm sure is
responsible for this failure. If there's a good reason for this, then
phys_addr_valid() should use phys_addr_t as its parameter type (and
so should ioremap() & Co), and the pre-processor conditional should
then change to depend on CONFIG_PHYS_ADDR_T_64BIT. Since ioremap()
would need to change first, I'd have to withdraw the patch until that
gets sorted out.

Jan

--

[ message continues ]

(Cc:-ing Jeremy and Hugh).

	Ingo
--

[ message continues ]

I take it we're talking about this chunk:

-static inline int phys_addr_valid(unsigned long addr)
+static inline int phys_addr_valid(resource_size_t addr)
 {
-	return addr < (1UL << boot_cpu_data.x86_phys_bits);
+#ifdef CONFIG_RESOURCES_64BIT
+	return !(addr >> boot_cpu_data.x86_phys_bits);
+#else
+	return 1;
+#endif


Is x86_phys_bits defined to be the actual number of address lines poking
out of the CPU package, or the number of address bits we can
meaningfully put into a pte?

I would say the simplest thing to do here is be explicit:

	if (sizeof(addr) == sizeof(u64))
		return !(addr >> boot_cpu_data.x86_phys_bits);
	else
		return 1; 

That's not ideal, but I guess its good enough.  I assume x86_phys_bits
can never be less than 32?

    J
--

[ message continues ]

My opinion is that it should be the number of physical address bits 

Technically speaking the 386SX had 24 physical address bits.  We do not 
actually set it that way, and I'm not even sure how to detect the 386SX 
programmatically.

	-hpa

--

[ message continues ]

Yes, one could do it that way. But what's the point of having RESOURCES_64BIT
set and resource_size_t nevertheless being a 32-bit quantity? And why,
independent of that, was ioremap() not changed to use phys_addr_t?

Jan

--

[ message continues ]

CONFIG_RESOURCES_64BIT was removed, so testing for it makes no sense. 
(Not being able to distinguish between non-existent and unset config
variables is an outstanding Kconfig problem.)

Directly testing the size of the type is the most robust approach,
though it would be simpler if shifting a variable by more bits than its

Well, ioremap is supposed to be used for IO mappings, so taking a
resource_size_t still makes sense.

The question of whether resource_size_t should be the same as a
phys_addr_t is still a bit undecided. Andrew's of the opinion that they
should be separate, and that it could make sense to have 32-bit resource
addresses in an otherwise 64-bit system.  I think that's a pretty narrow
special case (32-bit PAE system with no 64-bit IO devices), and its not
worth having the extra definition complexity for it.

    J
--

[ message continues ]

IMO we should change it to use #if instead of #ifdef.  The transition 
problem has been discussed elsewhere.

	-hpa

--

[ message continues ]

I'm confused about this.

There is no reason for this to be #ifdef'd; but it should be conditional 
on this information not being otherwise available (which will always be 
the case on 64 bits), I would assume.  If we are setting a value there, 
we should presumably also set 32 in the other case.

	-hpa
--

[ message continues ]

It does get set to 32 first (and only for 32-bits - 64bits didn't set any
default so far, so I didn't want to make it to now), and the code fragment
above updates that default after feature flags have been obtained, but
before other capabilities (including the phys/virt bit widths) are being
gathered.

Jan

--

[ message continues ]

Set the default on 64 bits as well.  It's cleaner to not leave a 
pointless #ifdef in there.

	-hpa

--

[ message continues ]

please move those lines to intel.c

YH
--

[ message continues ]