On 26.11.2008 22:49, ron minnich wrote:
> Index: arch/x86/intel/core2/stage1.c
> ===================================================================
> --- arch/x86/intel/core2/stage1.c (revision 1059)
> +++ arch/x86/intel/core2/stage1.c (working copy)
> @@ -2,6 +2,7 @@
> * This file is part of the coreboot project.
> *
> * Copyright (C) 2008 Carl-Daniel Hailfinger
> + * Copyright (C) 2008 coresystems GmbH
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License as published by
> @@ -29,30 +30,23 @@
> #include <string.h>
> #include <mtrr.h>
>
> -/**
> - * Disable Cache As RAM (CAR) after memory is setup.
> - */
> 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
> - * Global variables
> - *
> - * Align the result to 8 bytes
> - */
> - 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;
> + struct global_vars *const new_globals =
> + (struct global_vars *)((RAM_STACK_BASE - sizeof(struct
> global_vars *) - sizeof(struct global_vars)) & ~0x7);
>
> - printk(BIOS_DEBUG, "disable_car global_vars pointer adjusted\n");
> - printk(BIOS_DEBUG, "entering asm code now\n");
> + /* Copy global variables */
> + memcpy(new_globals, global_vars(), sizeof(struct global_vars));
>
> + /* Update global variable pointer */
> + *(struct global_vars **)(RAM_STACK_BASE - sizeof(struct global_vars *))
> + = new_globals;
> +
> + printk(BIOS_DEBUG, "Disabling cache-as-ram\n");
> +
> __asm__ __volatile__(
> +"movb $0x30, %%al\noutb %%al, $0x80\n"
> +
> " movl %[newesp], %%esp \n"
>
> /* We don't need cache as ram for now on */
> @@ -61,6 +55,8 @@
> " orl $(0x1<<30),%%eax \n"
> " movl %%eax, %%cr0 \n"
>
> +"movb $0x31, %%al\noutb %%al, $0x80\n"
> +
> /* disable fixed mtrr from now on, it will be enabled by coreboot_ram
> again*/
> /* clear sth */
> " xorl %%eax, %%eax \n"
> @@ -70,6 +66,7 @@
> " movl $0x200, %%ecx \n"
> " wrmsr \n"
>
> +"movb $0x32, %%al\noutb %%al, $0x80\n"
> /* Set the default memory type and disable fixed and enable variable
> MTRRs */
> " movl %[_MTRRdefType_MSR], %%ecx \n"
> " xorl %%edx, %%edx \n"
> @@ -77,17 +74,22 @@
> " movl $0x00000800, %%eax \n"
> " wrmsr \n"
>
> +"movb $0x33, %%al\noutb %%al, $0x80\n"
> /* enable cache */
> " movl %%cr0, %%eax \n"
> " andl $0x9fffffff,%%eax \n"
> " movl %%eax, %%cr0 \n"
>
> - " wbinvd \n"
> +"movb $0x34, %%al\noutb %%al, $0x80\n"
> + " invd \n"
>
> +"movb $0x35, %%al\noutb %%al, $0x80\n"
> " call stage1_phase3 \n"
> - :: [newesp] "i" (newlocation),
> + :: [newesp] "i" (new_globals),
> [_MTRRdefType_MSR] "i" (MTRRdefType_MSR)
> : "memory");
> +
> + printk(BIOS_DEBUG, "Something went wrong. Still here.\n");
> }
>
> void stop_ap(void)
>
>
That part of the patch is really funny. It rewrites a few comments and
printk messages (no effect on execution), renames one variable and
changes some whitespace. A few POST codes are added for good measure.
The only _functional_ change (except cosmetics and debugging) is the
change from wbinvd to invd.
May I suggest to drop all arch/x86/intel/core2/stage1.c changes except
the one below (and this wbinvd->invd change should have no effect
according to the scarce publicly available docs).
Index: arch/x86/intel/core2/stage1.c
===================================================================
--- arch/x86/intel/core2/stage1.c (Revision 1061)
+++ arch/x86/intel/core2/stage1.c (Arbeitskopie)
@@ -82,7 +82,7 @@
" andl $0x9fffffff,%%eax \n"
" movl %%eax, %%cr0 \n"
- " wbinvd \n"
+ " invd \n"
" call stage1_phase3 \n"
:: [newesp] "i" (newlocation),
This keeps the changes to a minium (and Stefan opposed changing
whitespace, printk and comments just for the sake of it).
If the machine boots even without the change, we could drop it
altogether. Having disable_car implementations diverge without reasons
enforced by the hardware is suboptimal.
Thanks.
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
--
coreboot mailing list: [email protected]
http://www.coreboot.org/mailman/listinfo/coreboot
