[PATCH] v3: Work around broken MTRR setup in VIA CAR

[Carl-Daniel Hailfinger]

Once we touch the MTRRs in VIA disable_car(), the CPU resets. Since
workarounds are better than instant reboots, mangle the code so that it
only switches stacks and flushes the cache.

There's also one genuine fix in there: Switch %esp before CAR is
disabled. That way, debugging becomes easier and the stack is always valid.

Many thanks to Corey for testing countless iterations of that code.

Signed-off-by: Carl-Daniel Hailfinger 
Attached for poor gmail users.

Index: corebootv3-via_car/arch/x86/via/stage1.c
===================================================================
--- corebootv3-via_car/arch/x86/via/stage1.c	(Revision 977)
+++ corebootv3-via_car/arch/x86/via/stage1.c	(Arbeitskopie)
@@ -35,6 +35,7 @@
  */
 void disable_car(void)
 {
+	printk(BIOS_DEBUG, "disable_car entry\n");
    /* Determine new global variable location. Stack organization from top
     * Top 4 bytes are reserved
     * Pointer to global variables
@@ -45,16 +46,25 @@
    const struct global_vars *newlocation = (struct global_vars *)((RAM_STACK_BASE - sizeof(struct global_vars *) - sizeof(struct global_vars)) & ~0x7);
    /* Copy global variables to new location. */
    memcpy(newlocation, global_vars(), sizeof(struct global_vars));
+	printk(BIOS_DEBUG, "disable_car global_vars copy done\n");
    /* Set the new global variable pointer. */
    *(struct global_vars **)(RAM_STACK_BASE - sizeof(struct global_vars *)) = newlocation;

+	printk(BIOS_DEBUG, "disable_car global_vars pointer adjusted\n")
+	printk(BIOS_DEBUG, "entering asm code now\n");
+
+#define HALT_AFTER 2
    __asm__ __volatile__(
+	"	movl	%[newesp], %%esp	\n"
+
+#if ENABLE_BROKEN_CAR_DISABLING
    /* We don't need cache as ram for now on */
    /* disable cache */
    "	movl    %%cr0, %%eax		\n"
    "	orl    $(0x1<<30),%%eax		\n"
    "	movl    %%eax, %%cr0		\n"

+	/* The MTRR setup below causes the machine to reset. Must investigate. */
    /* disable fixed mtrr from now on, it will be enabled by coreboot_ram again*/
    "	movl    %[_SYSCFG_MSR], %%ecx	\n"
    "	rdmsr				\n"
@@ -75,10 +85,10 @@
    "	movl    %%cr0, %%eax		\n"
    "	andl    $0x9fffffff,%%eax	\n"
    "	movl    %%eax, %%cr0		\n"
+#endif

"	wbinvd				\n"

-	"	movl	%[newesp], %%esp	\n"
    "	call stage1_phase3		\n"
    :: [newesp] "i" (newlocation),
     [_SYSCFG_MSR] "i" (SYSCFG_MSR),

[Carl-Daniel Hailfinger]

On 01.11.2008 23:21, Carl-Daniel Hailfinger wrote:
> Once we touch the MTRRs in VIA disable_car(), the CPU resets. Since
> workarounds are better than instant reboots, mangle the code so that it
> only switches stacks and flushes the cache.
>
> There are two genuine fixes in there as well: Switch %esp before CAR is
> disabled. That way, debugging becomes easier and the stack is always valid.

And one of the nastier bugs easily happening in C: We had a pointer to a
const struct, but we wanted a const pointer to a struct. This kills the
(correct) warning about that code.
> Many thanks to Corey for testing countless iterations of that code.
>
> Signed-off-by: Carl-Daniel Hailfinger 
> Attached for poor gmail users.

New version attached. It also fixes a missing semicolon that somehow
snuck into the earlier patch.

Regards,
Carl-Daniel

[Corey Osgood]

On Sat, Nov 1, 2008 at 6:32 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> On 01.11.2008 23:21, Carl-Daniel Hailfinger wrote:
> > Once we touch the MTRRs in VIA disable_car(), the CPU resets. Since
> > workarounds are better than instant reboots, mangle the code so that it
> > only switches stacks and flushes the cache.
> >
> > There are two genuine fixes in there as well: Switch %esp before CAR is
> > disabled. That way, debugging becomes easier and the stack is always
>
> valid.
> >
>
> And one of the nastier bugs easily happening in C: We had a pointer to a
> const struct, but we wanted a const pointer to a struct. This kills the
> (correct) warning about that code.
> > Many thanks to Corey for testing countless iterations of that code.
> >
> > Signed-off-by: Carl-Daniel Hailfinger <[e-mail]
> >
> > Attached for poor gmail users.
>
> New version attached. It also fixes a missing semicolon that somehow
> snuck into the earlier patch.

Index: corebootv3-via_car/arch/x86/via/stage1.c
> ===================================================================
> --- corebootv3-via_car/arch/x86/via/stage1.c    (Revision 977)
> +++ corebootv3-via_car/arch/x86/via/stage1.c    (Arbeitskopie)
> @@ -35,6 +35,7 @@
>   */
>  void disable_car(void)
>  {
> printk(BIOS_DEBUG, "disable_car entry\n");
> /* Determine new global variable location. Stack organization from top
> Top 4 bytes are reserved
> Pointer to global variables
>
> @@ -42,19 +43,28 @@
>       *
>        * Align the result to 8 bytes
>       */
> const struct global_vars *newlocation = (struct global_vars
>
> *)((RAM_STACK_BASE - sizeof(struct global_vars *) - sizeof(struct
> global_vars)) & ~0x7);
> struct global_vars *const newlocation = (struct global_vars
>
> *)((RAM_STACK_BASE - sizeof(struct global_vars *) - sizeof(struct
> global_vars)) & ~0x7);
>      /* Copy global variables to new location. */
>      memcpy(newlocation, global_vars(), sizeof(struct global_vars));
> printk(BIOS_DEBUG, "disable_car global_vars copy done\n");
> /* Set the new global variable pointer. */
> *(struct global_vars **)(RAM_STACK_BASE - sizeof(struct global_vars
>
> *)) = newlocation;
> printk(BIOS_DEBUG, "disable_car global_vars pointer adjusted\n");
> printk(BIOS_DEBUG, "entering asm code now\n");
>
> +#define HALT_AFTER 2

I don't think we need that anymore ;) Tested and
Acked-by: Corey Osgood

[Carl-Daniel Hailfinger]

On 02.11.2008 04:18, Corey Osgood wrote:
> On Sat, Nov 1, 2008 at 6:32 PM, Carl-Daniel Hailfinger <
> [e-mail]> wrote:
> > On 01.11.2008 23:21, Carl-Daniel Hailfinger wrote:
> > > Once we touch the MTRRs in VIA disable_car(), the CPU resets. Since
> > > workarounds are better than instant reboots, mangle the code so that it
> > > only switches stacks and flushes the cache.
> > >
> > > There are two genuine fixes in there as well: Switch %esp before CAR is
> > > disabled. That way, debugging becomes easier and the stack is always
> >
> > valid.
> >
> > And one of the nastier bugs easily happening in C: We had a pointer to a
> > const struct, but we wanted a const pointer to a struct. This kills the
> > (correct) warning about that code.
> > > Many thanks to Corey for testing countless iterations of that code.
> > >
> > > Signed-off-by: Carl-Daniel Hailfinger <[e-mail]
> > >
> > > Attached for poor gmail users.
> >
> > New version attached. It also fixes a missing semicolon that somehow
> > snuck into the earlier patch.
> >
> > [...]
>
> I don't think we need that anymore ;)

Thanks for the hint about the testing leftovers.
> Tested and
> Acked-by: Corey Osgood

Thanks, committed in r978.

Regards,
Carl-Daniel