[PATCH][v2] Unify CAR for K8 and Fam10h CPUs

[Patrick Georgi]

Hi,

the attached patch changes the ASM part of AMD CAR to do the K8/Fam10h 
decision at runtime, instead of compile time.

Ideally, this is enough to allow a single coreboot image to run on a board 
regardless of the CPU family. In practice, there might be more compile time 
decisions in later code that weren't exposed due to the unusual way we used 
the code. (Everything up to activating CAR is known to work, everything after 
real_main is fair game)
Yet, the patch should solve one of the harder issues as after this, coreboot 
is in C and in CAR mode, so there's plenty of space to keep a CPUID value 
around.

Regards,
Patrick

[Carl-Daniel Hailfinger]

Hi Patrick,

On 13.02.2009 13:02, Patrick Georgi wrote:
> the attached patch changes the ASM part of AMD CAR to do the K8/Fam10h 
> decision at runtime, instead of compile time.
>
> Ideally, this is enough to allow a single coreboot image to run on a board 
> regardless of the CPU family. In practice, there might be more compile time 
> decisions in later code that weren't exposed due to the unusual way we used 
> the code. (Everything up to activating CAR is known to work, everything after 
> real_main is fair game)
> Yet, the patch should solve one of the harder issues as after this, coreboot 
> is in C and in CAR mode, so there's plenty of space to keep a CPUID value 
> around.
>
> What this patch does:
> Enable SSE (to get some more registers to play with)
> Determine CPUID, and stash it in an XMM register, and reference
> value for comparison in another XMM register
> Add a macro jmp_if_k8, which jumps if the CPU is K8 (using an SSE compare)
> Replace #if CAR_FAM10 sections with runtime checks using jmp_if_k8.
> This is pretty mechanical work. The macro uses local labels (1: and 2:) to
> prevent namespace issues
> At one time, CPU_ADDR_BITS is used to fill a register. This is replaced
> with hardcoded values for both cases, and switched appropriately.
> Disable SSE
>
> Signed-off-by: Patrick Georgi

This is absolutely awesome! Not only did you manage to keep the code
readable, you also have kept the changes localized and really minimal.
Although I had something similar (untested, unfinished) on disk, your
solution is clearly the way to go. No doubt about that.

I'd ack straight away, but there is one thing I couldn't figure out
immediately from the code: Can the code handle Fam11h processors? AFAIK
they have to be treated like Fam10h.

(And I want to reread this beautiful patch again ;-) )

Regards,
Carl-Daniel

[Patrick Georgi]

Am Freitag 13 Februar 2009 13:42:15 schrieb Carl-Daniel Hailfinger:
> This is absolutely awesome! Not only did you manage to keep the code
> readable, you also have kept the changes localized and really minimal.
> Although I had something similar (untested, unfinished) on disk, your
> solution is clearly the way to go. No doubt about that.

Thank you!
> I'd ack straight away, but there is one thing I couldn't figure out
> immediately from the code: Can the code handle Fam11h processors? AFAIK
> they have to be treated like Fam10h.

Right. A better way would be to compare against the K8 CPUID, and switch the 
jump direction in jmp_if_k8. That should be enough, until AMD decides to 
devise a new scheme for future CPU generations.

Attached patch does that, but is totally untested (in fact, I patched the 
patch directly)

Regards,
Patrick

[Carl-Daniel Hailfinger]

On 13.02.2009 14:01, Patrick Georgi wrote:
> Am Freitag 13 Februar 2009 13:42:15 schrieb Carl-Daniel Hailfinger:
> > This is absolutely awesome! Not only did you manage to keep the code
> > readable, you also have kept the changes localized and really minimal.
> > Although I had something similar (untested, unfinished) on disk, your
> > solution is clearly the way to go. No doubt about that.
>
> Thank you!
> > I'd ack straight away, but there is one thing I couldn't figure out
> > immediately from the code: Can the code handle Fam11h processors? AFAIK
> > they have to be treated like Fam10h.
>
> Right. A better way would be to compare against the K8 CPUID, and switch the 
> jump direction in jmp_if_k8. That should be enough, until AMD decides to 
> devise a new scheme for future CPU generations.

I created a patch against a tree with your original patch applied. It
should be able to handle all generations since the invention of CPUID
just fine. Fam11h and later are handled as well.

Signed-off-by: Carl-Daniel Hailfinger

--- corebootv2/src/cpu/amd/car/cache_as_ram.inc	(Arbeitskopie)
+++ corebootv2/src/cpu/amd/car/cache_as_ram.inc	(working copy)
@@ -27,10 +27,10 @@
 /* for CAR_FAM10 */
 #define CacheSizeAPStack 0x400 /* 1K */

-#define jmp_if_k8(x)	comisd	%xmm1, %xmm2; jnz x
+#define jmp_if_k8(x)	comisd	%xmm1, %xmm2; jb x

#define CPUID_MASK		0x0ff00f00
-#define CPUID_MASK_FAM10	0x00100f00
+#define CPUID_VAL_FAM10_ROTATED	0x0f000010

#include <cpu/x86/mtrr.h>
 #include <cpu/amd/mtrr.h>
@@ -60,9 +60,12 @@
    cvtsi2sd %ebx, %xmm3
    movl	$0x01, %eax
    cpuid
+	/* base family is bits 8..11, extended family is bits 20..27 */
    andl	$CPUID_MASK, %eax
+	/* reorder bits for easier comparison by value */
+	rol	%eax, $16
    cvtsi2sd %eax, %xmm1
-	movl	$CPUID_MASK_FAM10, %eax
+	movl	$CPUID_VAL_FAM10_ROTATED, %eax
    cvtsi2sd %eax, %xmm2
    cvtsd2si %xmm3, %ebx

Regards,
Carl-Daniel

[Patrick Georgi]

Am Freitag 13 Februar 2009 14:44:20 schrieb Carl-Daniel Hailfinger:
> I created a patch against a tree with your original patch applied. It
> should be able to handle all generations since the invention of CPUID
> just fine. Fam11h and later are handled as well.

Looks nice, and I tested it in SimNow after some trivial changes to make it 
build. I think it's ready for integration now.

Regards,
Patrick

[Carl-Daniel Hailfinger]

On 13.02.2009 21:00, Patrick Georgi wrote:
> Am Freitag 13 Februar 2009 14:44:20 schrieb Carl-Daniel Hailfinger:
> > I created a patch against a tree with your original patch applied. It
> > should be able to handle all generations since the invention of CPUID
> > just fine. Fam11h and later are handled as well.
>
> Looks nice, and I tested it in SimNow after some trivial changes to make it 
> build. I think it's ready for integration now.

Thanks for integrating my patch. I think your version looks good.

Signed-off-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel

[Stefan Reinauer]

Patrick Georgi wrote:
> Am Freitag 13 Februar 2009 14:44:20 schrieb Carl-Daniel Hailfinger:
> > I created a patch against a tree with your original patch applied. It
> > should be able to handle all generations since the invention of CPUID
> > just fine. Fam11h and later are handled as well.
>
> Looks nice, and I tested it in SimNow after some trivial changes to make it 
> build. I think it's ready for integration now.
>
> Regards,
> Patrick
>
> What this patch does:
> Enable SSE (to get some more registers to play with)
> Determine CPUID, and stash it in an XMM register, and reference
> value for comparison in another XMM register (mangled somewhat to
> simplify inequality comparisons)
> Add a macro jmp_if_k8, which jumps if the CPU is K8 (using an SSE compare)
> Replace #if CAR_FAM10 sections with runtime checks using jmp_if_k8.
> This is pretty mechanical work. The macro uses local labels (1: and 2:) to
> prevent namespace issues
> At one time, CPU_ADDR_BITS is used to fill a register. This is replaced
> with hardcoded values for both cases, and switched appropriately.
> Disable SSE
>
> Signed-off-by: Patrick Georgi

Acked-by: Stefan Reinauer

[Patrick Georgi]

Am Dienstag 17 Februar 2009 13:50:31 schrieben Sie:
> Acked-by: Stefan Reinauer

Thanks, r3951.
Also thanks to Carl-Daniel for seeing the issue with Fam11 and providing the 
first version of a solution for that.

Patrick