Re: [coreboot] patch: add support for blobs; example SMM blob included

[Carl-Daniel Hailfinger]

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