[PATCH] v3: correct K8 stack preservation asm

[Carl-Daniel Hailfinger]

The current K8 stack preservation code in disable_car() works by chance,
but that's not something we should rely on.

The new code is entirely rewritten, fixes a few missing constraints in
the asm and should be a lot more readable. However, the generated code
should be mostly identical.

Attached for Ron.

Signed-off-by: Carl-Daniel Hailfinger

Index: corebootv3-k8_car_disable_inline_asm/arch/x86/amd/k8/stage1.c
===================================================================
--- corebootv3-k8_car_disable_inline_asm/arch/x86/amd/k8/stage1.c	(Revision 856)
+++ corebootv3-k8_car_disable_inline_asm/arch/x86/amd/k8/stage1.c	(Arbeitskopie)
@@ -34,20 +34,31 @@
  */
 void disable_car(void)
 {
-	/* OK, here is the theory: we should be able to copy 
-	 * the data back over itself, and the wbinvd should then 
-	 * flush to memory. Let's see. 
-	 */
-	/* nope. 
-	__asm__ __volatile__("cld; rep movsl" ::"D" (CONFIG_CARBASE), "S" (CONFIG_CARBASE), "c" (CONFIG_CARSIZE/4): "memory");
-	 */
    /* call the inlined function */
    disable_cache_as_ram();

-	/* copy it down, wbinvd, copy it back? */
-	__asm__ __volatile__("cld; rep movsl" ::"D" (0x88000), "S" (CONFIG_CARBASE), "c" (CONFIG_CARSIZE/4): "memory");
-	__asm__ __volatile__ ("wbinvd\n");
-	__asm__ __volatile__("cld; rep movsl" ::"D" (CONFIG_CARBASE), "S" (0x88000), "c" (CONFIG_CARSIZE/4): "memory");
+	/* The BKDG says that a WBINVD will not flush CAR to RAM (because the
+	 * cache tags are not dirty).
+	 * Solution:
+	 * - Two subsequent memcpy in the same inline asm block, one from stack
+	 *   to backup, one from backup to stack.
+	 * The solution for geode of using a inline asm memcpy of the stack
+	 * onto itself will not mark the cache tags as dirty on K8.
+	 */
+	__asm__ __volatile__(
+	"	movl %[carbase], %%esi		\n"
+	"	movl %[backuplocation], %%edi	\n"
+	"	movl %[carsizequads], %%ecx	\n"
+	"	cld				\n"
+	"	rep movsl			\n"
+	"	wbinvd				\n"
+	"	movl %[backuplocation], %%esi	\n"
+	"	movl %[carbase], %%edi		\n"
+	"	movl %[carsizequads], %%ecx	\n"
+	"	rep movsl			\n"
+	:: [carbase] "i" (CONFIG_CARBASE), [backuplocation] "i" (0x88000),
+	   [carsizequads] "i" (CONFIG_CARSIZE/4)
+	: "memory", "%edi", "%esi", "%ecx");
    banner(BIOS_DEBUG, "Disable_car: done wbinvd");
    banner(BIOS_DEBUG, "disable_car: done");
 }

[Carl-Daniel Hailfinger]

On 05.09.2008 03:49, Carl-Daniel Hailfinger wrote:
> The current K8 stack preservation code in disable_car() works by chance,
> but that's not something we should rely on.
>
> The new code is entirely rewritten, fixes a few missing constraints in
> the asm and should be a lot more readable. However, the generated code
> should be mostly identical.

I hate to say it, but the generated code is NOT identical. The old code
was broken because of the missing ecx clobber constraint and it did not
copy the stack back (ecx was zero at the beginning of the copy-back loop
and so the loop executed exactly zero times).
So this is a genuine bug fix. v2 may be affected as well.
> Signed-off-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel

[ron minnich]

wow! nice catch!

Acked-by: Ronald G. Minnich

We also need disable_car_and_halt, which only disables car and halts,
for the APs (i.e. no need to copy stack back)

hint, hint, hint :-)

ron

[Carl-Daniel Hailfinger]

On 05.09.2008 05:25, ron minnich wrote:
> wow! nice catch!
>
> Acked-by: Ronald G. Minnich

Thanks, r858.
> We also need disable_car_and_halt, which only disables car and halts,
> for the APs (i.e. no need to copy stack back)
>
> hint, hint, hint :-)

Yes, I'll look into that.

Regards,
Carl-Daniel