[PATCH 0/4] x86, head_32: Some cleanups

[PATCH 0/4] x86, head_32: Some cleanups

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Hi,

here are some initial low-hanging fruits wrt head_32.S cleanup. I've
made them as easily digestible as possible; after all, this is boot asm
and meddling with it tends to upset kernels.

Also, I've made the assumption that having boot_cpu_data.cpuid_level
contain the CPUID level for the boot cpu means that the APs have the
same CPUID level. This should be the case on X86.

They boot fine 486 and 486SX in qemu but I'd like to hear whether
the direction I'm going is ok before I continue testing them on real
hardware.

Thanks.


Borislav Petkov (4):
  x86, head_32: Remove i386 pieces
  x86: Detect CPUID support early at boot
  x86, head_32: Remove CPUID detection from default_entry
  x86, 32-bit: Drop new_cpu_data

 arch/x86/include/asm/processor.h |   1 -
 arch/x86/kernel/head_32.S        | 105 ++++++++++++++++-----------------------
 arch/x86/kernel/setup.c          |   3 --
 arch/x86/lguest/boot.c           |   6 +--
 arch/x86/xen/enlighten.c         |   8 +--
 5 files changed, 51 insertions(+), 72 deletions(-)

-- 
1.8.1.2.422.g08c0e7f

[PATCH 1/4] x86, head_32: Remove i386 pieces

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Remove code fragments detecting a 386 CPU since we don't support those
anymore. Also, do not do alignment checks because they're done only at
CPL3. Also, no need to preserve EFLAGS.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/head_32.S | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
index c8932c79e78b..a9c5cc851285 100644
--- a/arch/x86/kernel/head_32.S
+++ b/arch/x86/kernel/head_32.S
@@ -394,30 +394,21 @@ default_entry:
 	jz 1f				# Did we do this already?
 	call *%eax
 1:
-	
-/* check if it is 486 or 386. */
+
 /*
- * XXX - this does a lot of unnecessary setup.  Alignment checks don't
- * apply at our cpl of 0 and the stack ought to be aligned already, and
- * we don't need to preserve eflags.
+ * Check if it is 486
  */
 	movl $-1,X86_CPUID	# -1 for no CPUID initially
-	movb $3,X86		# at least 386
+	movb $4,X86		# at least 486
 	pushfl			# push EFLAGS
 	popl %eax		# get EFLAGS
 	movl %eax,%ecx		# save original EFLAGS
-	xorl $0x240000,%eax	# flip AC and ID bits in EFLAGS
+	xorl $0x200000,%eax	# flip ID bit in EFLAGS
 	pushl %eax		# copy to EFLAGS
 	popfl			# set EFLAGS
 	pushfl			# get new EFLAGS
 	popl %eax		# put it in eax
 	xorl %ecx,%eax		# change in flags
-	pushl %ecx		# restore original EFLAGS
-	popfl
-	testl $0x40000,%eax	# check if AC bit changed
-	je is386
-
-	movb $4,X86		# at least 486
 	testl $0x200000,%eax	# check if ID bit changed
 	je is486
 
@@ -445,10 +436,7 @@ default_entry:
 	movl %edx,X86_CAPABILITY
 
 is486:	movl $0x50022,%ecx	# set AM, WP, NE and MP
-	jmp 2f
-
-is386:	movl $2,%ecx		# set MP
-2:	movl %cr0,%eax
+	movl %cr0,%eax
 	andl $0x80000011,%eax	# Save PG,PE,ET
 	orl %ecx,%eax
 	movl %eax,%cr0
-- 
1.8.1.2.422.g08c0e7f

[PATCH 2/4] x86: Detect CPUID support early at boot

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

We detect CPUID function support on the boot CPU and save it for later
use, obviating the need to play the toggle EFLAGS.ID game every time. C
code is looking at ->cpuid_level anyway.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/head_32.S | 36 +++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
index a9c5cc851285..ce6b557017f4 100644
--- a/arch/x86/kernel/head_32.S
+++ b/arch/x86/kernel/head_32.S
@@ -29,16 +29,16 @@
 /*
  * References to members of the new_cpu_data structure.
  */
-
 #define X86		new_cpu_data+CPUINFO_x86
 #define X86_VENDOR	new_cpu_data+CPUINFO_x86_vendor
 #define X86_MODEL	new_cpu_data+CPUINFO_x86_model
 #define X86_MASK	new_cpu_data+CPUINFO_x86_mask
 #define X86_HARD_MATH	new_cpu_data+CPUINFO_hard_math
-#define X86_CPUID	new_cpu_data+CPUINFO_cpuid_level
 #define X86_CAPABILITY	new_cpu_data+CPUINFO_x86_capability
 #define X86_VENDOR_ID	new_cpu_data+CPUINFO_x86_vendor_id
 
+#define X86_CPUID	boot_cpu_data+CPUINFO_cpuid_level
+
 /*
  * This is how much memory in addition to the memory covered up to
  * and including _end we need mapped initially.
@@ -263,6 +263,33 @@ subarch_entries:
 num_subarch_entries = (. - subarch_entries) / 4
 .previous
 #else
+
+/*
+ * Initialize EFLAGS. Some BIOS's leave bits like NT set. This would confuse the
+ * debugger if this code is traced.
+ */
+	pushl $0
+	popfl
+
+/*
+ * Check whether this CPU supports CPUID, and, if so, save the highest standard
+ * CPUID function number for later.
+ */
+	movl $X86_EFLAGS_ID,%ecx	/* EFLAGS.ID */
+	pushl %ecx
+	popfl				/* set EFLAGS=ID */
+	pushfl				/* get EFLAGS */
+	popl %eax
+	xorl %ecx,%eax
+	jnz 1f				/* hw disallowed setting of ID bit */
+
+	xorl %eax,%eax
+	cpuid
+	movl %eax,pa(X86_CPUID)		/* save largest std CPUID function */
+	jmp default_entry
+
+1:
+	movl $-1,pa(X86_CPUID)
 	jmp default_entry
 #endif /* CONFIG_PARAVIRT */
 
@@ -377,11 +404,6 @@ default_entry:
 	/* Shift the stack pointer to a virtual address */
 	addl $__PAGE_OFFSET, %esp
 
-/*
- * Initialize eflags.  Some BIOS's leave bits like NT set.  This would
- * confuse the debugger if this code is traced.
- * XXX - best to initialize before switching to protected mode.
- */
 	pushl $0
 	popfl
 
-- 
1.8.1.2.422.g08c0e7f

[PATCH 3/4] x86, head_32: Remove CPUID detection from default_entry

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

We do that once earlier now and cache it into boot_cpu_data.cpuid_level
so no need for the EFLAGS.ID toggling dance anymore.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/head_32.S | 40 +++++++---------------------------------
 1 file changed, 7 insertions(+), 33 deletions(-)

diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
index ce6b557017f4..0dba3598cf02 100644
--- a/arch/x86/kernel/head_32.S
+++ b/arch/x86/kernel/head_32.S
@@ -334,25 +334,14 @@ default_entry:
 	movl %eax,%cr0
 
 /*
- *	New page tables may be in 4Mbyte page mode and may
- *	be using the global pages. 
+ * New page tables may be in 4Mbyte page mode and may be using the global pages.
  *
- *	NOTE! If we are on a 486 we may have no cr4 at all!
- *	Specifically, cr4 exists if and only if CPUID exists
- *	and has flags other than the FPU flag set.
+ * NOTE! If we are on a 486 we may have no cr4 at all! Specifically, cr4 exists
+ * if and only if CPUID exists (which has been checked above) and has flags
+ * other than the FPU flag set.
  */
-	movl $X86_EFLAGS_ID,%ecx
-	pushl %ecx
-	popfl
-	pushfl
-	popl %eax
-	pushl $0
-	popfl
-	pushfl
-	popl %edx
-	xorl %edx,%eax
-	testl %ecx,%eax
-	jz 6f			# No ID flag = no CPUID = no CR4
+	cmpl $-1, pa(X86_CPUID)
+	je 6f			# No CPUID = no CR4
 
 	movl $1,%eax
 	cpuid
@@ -389,7 +378,6 @@ default_entry:
 	btsl $_EFER_NX, %eax
 	/* Make changes effective */
 	wrmsr
-
 6:
 
 /*
@@ -417,21 +405,7 @@ default_entry:
 	call *%eax
 1:
 
-/*
- * Check if it is 486
- */
-	movl $-1,X86_CPUID	# -1 for no CPUID initially
-	movb $4,X86		# at least 486
-	pushfl			# push EFLAGS
-	popl %eax		# get EFLAGS
-	movl %eax,%ecx		# save original EFLAGS
-	xorl $0x200000,%eax	# flip ID bit in EFLAGS
-	pushl %eax		# copy to EFLAGS
-	popfl			# set EFLAGS
-	pushfl			# get new EFLAGS
-	popl %eax		# put it in eax
-	xorl %ecx,%eax		# change in flags
-	testl $0x200000,%eax	# check if ID bit changed
+	cmpl $-1,X86_CPUID
 	je is486
 
 	/* get vendor info */
-- 
1.8.1.2.422.g08c0e7f

[PATCH 4/4] x86, 32-bit: Drop new_cpu_data

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

We copy it to boot_cpu_data anyway so use boot_cpu_data from the get-go.

Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/include/asm/processor.h |  1 -
 arch/x86/kernel/head_32.S        | 17 ++++++++---------
 arch/x86/kernel/setup.c          |  3 ---
 arch/x86/lguest/boot.c           |  6 +++---
 arch/x86/xen/enlighten.c         |  8 ++++----
 5 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index cf500543f6ff..984223f93293 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -149,7 +149,6 @@ struct cpuinfo_x86 {
  * capabilities of CPUs
  */
 extern struct cpuinfo_x86	boot_cpu_data;
-extern struct cpuinfo_x86	new_cpu_data;
 
 extern struct tss_struct	doublefault_tss;
 extern __u32			cpu_caps_cleared[NCAPINTS];
diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
index 0dba3598cf02..fe77ec1202a5 100644
--- a/arch/x86/kernel/head_32.S
+++ b/arch/x86/kernel/head_32.S
@@ -27,17 +27,16 @@
 #define pa(X) ((X) - __PAGE_OFFSET)
 
 /*
- * References to members of the new_cpu_data structure.
+ * References to members of the boot_cpu_data structure.
  */
-#define X86		new_cpu_data+CPUINFO_x86
-#define X86_VENDOR	new_cpu_data+CPUINFO_x86_vendor
-#define X86_MODEL	new_cpu_data+CPUINFO_x86_model
-#define X86_MASK	new_cpu_data+CPUINFO_x86_mask
-#define X86_HARD_MATH	new_cpu_data+CPUINFO_hard_math
-#define X86_CAPABILITY	new_cpu_data+CPUINFO_x86_capability
-#define X86_VENDOR_ID	new_cpu_data+CPUINFO_x86_vendor_id
-
+#define X86		boot_cpu_data+CPUINFO_x86
+#define X86_VENDOR	boot_cpu_data+CPUINFO_x86_vendor
+#define X86_MODEL	boot_cpu_data+CPUINFO_x86_model
+#define X86_MASK	boot_cpu_data+CPUINFO_x86_mask
+#define X86_HARD_MATH	boot_cpu_data+CPUINFO_hard_math
 #define X86_CPUID	boot_cpu_data+CPUINFO_cpuid_level
+#define X86_CAPABILITY	boot_cpu_data+CPUINFO_x86_capability
+#define X86_VENDOR_ID	boot_cpu_data+CPUINFO_x86_vendor_id
 
 /*
  * This is how much memory in addition to the memory covered up to
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 4f322b3eb078..548044f751fc 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -171,8 +171,6 @@ static struct resource bss_resource = {
 
 
 #ifdef CONFIG_X86_32
-/* cpu data as detected by the assembly code in head.S */
-struct cpuinfo_x86 new_cpu_data __cpuinitdata = {0, 0, 0, 0, -1, 1, 0, 0, -1};
 /* common cpu data for all cpus */
 struct cpuinfo_x86 boot_cpu_data __read_mostly = {0, 0, 0, 0, -1, 1, 0, 0, -1};
 EXPORT_SYMBOL(boot_cpu_data);
@@ -749,7 +747,6 @@ early_param("reservelow", parse_reservelow);
 void __init setup_arch(char **cmdline_p)
 {
 #ifdef CONFIG_X86_32
-	memcpy(&boot_cpu_data, &new_cpu_data, sizeof(new_cpu_data));
 	visws_early_detect();
 
 	/*
diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c
index 1cbd89ca5569..bd222f2495f4 100644
--- a/arch/x86/lguest/boot.c
+++ b/arch/x86/lguest/boot.c
@@ -1404,12 +1404,12 @@ __init void lguest_init(void)
 	 * This is messy CPU setup stuff which the native boot code does before
 	 * start_kernel, so we have to do, too:
 	 */
-	cpu_detect(&new_cpu_data);
+	cpu_detect(&boot_cpu_data);
 	/* head.S usually sets up the first capability word, so do it here. */
-	new_cpu_data.x86_capability[0] = cpuid_edx(1);
+	boot_cpu_data.x86_capability[0] = cpuid_edx(1);
 
 	/* Math is always hard! */
-	new_cpu_data.hard_math = 1;
+	boot_cpu_data.hard_math = 1;
 
 	/* We don't have features.  We have puppies!  Puppies! */
 #ifdef CONFIG_X86_MCE
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 8b4c56d85ca0..85871df3cc68 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -1454,10 +1454,10 @@ asmlinkage void __init xen_start_kernel(void)
 
 #ifdef CONFIG_X86_32
 	/* set up basic CPUID stuff */
-	cpu_detect(&new_cpu_data);
-	new_cpu_data.hard_math = 1;
-	new_cpu_data.wp_works_ok = 1;
-	new_cpu_data.x86_capability[0] = cpuid_edx(1);
+	cpu_detect(&boot_cpu_data);
+	boot_cpu_data.hard_math = 1;
+	boot_cpu_data.wp_works_ok = 1;
+	boot_cpu_data.x86_capability[0] = cpuid_edx(1);
 #endif
 
 	/* Poke various useful things into boot_params */
-- 
1.8.1.2.422.g08c0e7f

Re: [PATCH 4/4] x86, 32-bit: Drop new_cpu_data

[H. Peter Anvin]

On 02/03/2013 08:14 AM, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
>
> We copy it to boot_cpu_data anyway so use boot_cpu_data from the get-go.
>

Hmm... this is the only part of this patchset I feel skeptical towards. 
  Overall, a lot of the early SMP code went way out of its way to have 
zero impact on the !CONFIG_SMP case, but that was a long time ago. 
Nowadays what we really should have is cpu_data being a percpu variable 
separate from boot_cpu_data (which is really "all_cpu_data") even on UP.

Another cleanup desperately needed in this area is a bitvector for bugs 
in addition to features.  In fact, I kind of suspect we should make it 
the *same* bitvector (different words) so we cpu_has(X) works on both 
without confusion (just put the BUGS at the end; it means that if we add 
feature words the bug numbers will shift but that is okay.)

I actually mean to do this when I did the CPU feature vector stuff over 
10 years ago, but never got around to it... and it still has never 
gotten done.

The difference between bugs and features, of course, is that the former 
should be combined across CPUs with an OR whereas the latter get 
combined with an AND.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH 4/4] x86, 32-bit: Drop new_cpu_data

[Borislav Petkov]

On Sun, Feb 03, 2013 at 03:44:29PM -0800, H. Peter Anvin wrote:
> On 02/03/2013 08:14 AM, Borislav Petkov wrote:
> >From: Borislav Petkov <bp@suse.de>
> >
> >We copy it to boot_cpu_data anyway so use boot_cpu_data from the get-go.
> >
> 
> Hmm... this is the only part of this patchset I feel skeptical
> towards.  Overall, a lot of the early SMP code went way out of its
> way to have zero impact on the !CONFIG_SMP case, but that was a long
> time ago. Nowadays what we really should have is cpu_data being a
> percpu variable separate from boot_cpu_data (which is really
> "all_cpu_data") even on UP.

Hmmkay.

My thought vector here was to use boot_cpu_data to cache stuff
here which is universally valid on the current system, i.e. like
all_cpu_data. IOW, cache here family (model and stepping could differ,
as we've come to realize over the years :)) vendor (btw, X86_VENDOR is
unused) CPUID_EAX(0) level, capability, etc and use them later instead
of querying them again.

So, so early and in this case, we're saving CPU data which is valid for
all CPUs on the system and thus it belongs into boot_cpu_data, right?

And then, btw, that data could've been used in verify_cpu.S only if the
damn thing wasn't being used in arch/x86/boot/...

> Another cleanup desperately needed in this area is a bitvector for
> bugs in addition to features.

Yeah, c->x86_unfeatures! :-)

> In fact, I kind of suspect we should make it the *same* bitvector
> (different words) so we cpu_has(X) works on both without confusion
> (just put the BUGS at the end; it means that if we add feature words
> the bug numbers will shift but that is okay.)
>
> I actually mean to do this when I did the CPU feature vector stuff
> over 10 years ago, but never got around to it... and it still has
> never gotten done.
>
> The difference between bugs and features, of course, is that the
> former should be combined across CPUs with an OR whereas the latter
> get combined with an AND.

Yeah, that should be pretty easy to do with the current machinery
already in place. I'll take a look.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 4/4] x86, 32-bit: Drop new_cpu_data

[H. Peter Anvin]

boot_cpu_data is ok for things that are indeed universally valid across.  That does not include CPUID level, for one.

Borislav Petkov <bp@alien8.de> wrote:

>On Sun, Feb 03, 2013 at 03:44:29PM -0800, H. Peter Anvin wrote:
>> On 02/03/2013 08:14 AM, Borislav Petkov wrote:
>> >From: Borislav Petkov <bp@suse.de>
>> >
>> >We copy it to boot_cpu_data anyway so use boot_cpu_data from the
>get-go.
>> >
>> 
>> Hmm... this is the only part of this patchset I feel skeptical
>> towards.  Overall, a lot of the early SMP code went way out of its
>> way to have zero impact on the !CONFIG_SMP case, but that was a long
>> time ago. Nowadays what we really should have is cpu_data being a
>> percpu variable separate from boot_cpu_data (which is really
>> "all_cpu_data") even on UP.
>
>Hmmkay.
>
>My thought vector here was to use boot_cpu_data to cache stuff
>here which is universally valid on the current system, i.e. like
>all_cpu_data. IOW, cache here family (model and stepping could differ,
>as we've come to realize over the years :)) vendor (btw, X86_VENDOR is
>unused) CPUID_EAX(0) level, capability, etc and use them later instead
>of querying them again.
>
>So, so early and in this case, we're saving CPU data which is valid for
>all CPUs on the system and thus it belongs into boot_cpu_data, right?
>
>And then, btw, that data could've been used in verify_cpu.S only if the
>damn thing wasn't being used in arch/x86/boot/...
>
>> Another cleanup desperately needed in this area is a bitvector for
>> bugs in addition to features.
>
>Yeah, c->x86_unfeatures! :-)
>
>> In fact, I kind of suspect we should make it the *same* bitvector
>> (different words) so we cpu_has(X) works on both without confusion
>> (just put the BUGS at the end; it means that if we add feature words
>> the bug numbers will shift but that is okay.)
>>
>> I actually mean to do this when I did the CPU feature vector stuff
>> over 10 years ago, but never got around to it... and it still has
>> never gotten done.
>>
>> The difference between bugs and features, of course, is that the
>> former should be combined across CPUs with an OR whereas the latter
>> get combined with an AND.
>
>Yeah, that should be pretty easy to do with the current machinery
>already in place. I'll take a look.
>
>Thanks.

-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.

Re: [PATCH 4/4] x86, 32-bit: Drop new_cpu_data

[Borislav Petkov]

On Sun, Feb 03, 2013 at 09:44:02PM -0800, H. Peter Anvin wrote:
> boot_cpu_data is ok for things that are indeed universally valid
> across. That does not include CPUID level, for one.

Wait a minute, hold the phone! :-)

Are you saying that CPUID_EAX(0) could return different values in %eax
on the *same* system with mixed-silicon steppings? Or even on an uniform
system?

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 4/4] x86, 32-bit: Drop new_cpu_data

[H. Peter Anvin]

On 02/04/2013 01:02 AM, Borislav Petkov wrote:
> On Sun, Feb 03, 2013 at 09:44:02PM -0800, H. Peter Anvin wrote:
>> boot_cpu_data is ok for things that are indeed universally valid
>> across. That does not include CPUID level, for one.
> 
> Wait a minute, hold the phone! :-)
> 
> Are you saying that CPUID_EAX(0) could return different values in %eax
> on the *same* system with mixed-silicon steppings? Or even on an uniform
> system?
> 

Yes and yes (in the latter case due to inconsistent MSR programming.)

	-hpa

Re: [PATCH 4/4] x86, 32-bit: Drop new_cpu_data

[Borislav Petkov]

On Mon, Feb 04, 2013 at 08:55:17AM -0800, H. Peter Anvin wrote:
> Yes and yes (in the latter case due to inconsistent MSR programming.)

Ok, I'll drop the last one, redo 2/4 and run them on the hardware I have
here.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--