Re: [coreboot] proposal to enable running vsa in geode northbridge

[Carl-Daniel Hailfinger]

On 26.01.2008 00:30, ron minnich wrote:
> Attached.
>
> It's pretty simple and short and untested :-)
>
> ron

Comments below.
> This change will support stage2 running LAR files. The initial example is running the VSA in 
> the geode lx northbridge.
>
> lar.h: make LAR functions SHARED
> lar.c: make process_file non-static
> vsmsetup.c: modify to use LAR functions. 
> stage1.c: new function, init_archive, which is SHARED and will set up the initial archive struct.
>
> Signed-off-by: Ronald G. Minnich
> Index: include/lar.h
>
> --- include/lar.h	(revision 560)
> +++ include/lar.h	(working copy)
> @@ -51,6 +51,7 @@
>  #define LAR_H
>
> #include <types.h>
> +#include <shared.h>
>
> #define MAGIC "LARCHIVE"
>  #define MAX_PATHLEN 1024
> @@ -82,11 +83,15 @@
>  };
>
> /* Prototypes. */
> -int find_file(const struct mem_file *archive, const char *filename, struct mem_file *result);
> -int copy_file(const struct mem_file *archive, const char *filename, void *where);
> -int run_file(const struct mem_file *archive, const char *filename, void *where);
> -int execute_in_place(const struct mem_file *archive, const char *filename);
> -int run_address(void *f);
> -void *load_file(const struct mem_file *archive, const char *filename);
> -void *load_file_segments(const struct mem_file *archive, const char *filename);
> +/* architecture-defined */
> +SHARED(init_archive, void, struct mem_file *);
> +/* architecture-independent */
> +SHARED(find_file, int, const struct mem_file *archive, const char *filename, struct mem_file *result);
> +SHARED(copy_file, int, const struct mem_file *archive, const char *filename, void *where);
> +SHARED(run_file, int, const struct mem_file *archive, const char *filename, void *where);
> +SHARED(execute_in_place, int, const struct mem_file *archive, const char *filename);
> +SHARED(run_address, int, void *f);
> +SHARED(load_file, void *, const struct mem_file *archive, const char *filename);
> +SHARED(load_file_segments, void *, const struct mem_file *archive, const char *filename);
> +SHARED(process_file, int, const struct mem_file *archive, void *where);
>  #endif /* LAR_H */
> Index: lib/lar.c
> ===================================================================
> --- lib/lar.c	(revision 560)
> +++ lib/lar.c	(working copy)
> @@ -145,7 +145,7 @@
>    return 1;
>  }
>
> -static int process_file(const struct mem_file *archive, void *where)
> +int process_file(const struct mem_file *archive, void *where)
>  {
>    printk(BIOS_SPEW, "LAR: Compression algorithm #%i used\n", archive->compression);
>    /* no compression */
> Index: northbridge/amd/geodelx/vsmsetup.c
> ===================================================================
> --- northbridge/amd/geodelx/vsmsetup.c	(revision 560)
> +++ northbridge/amd/geodelx/vsmsetup.c	(working copy)
> @@ -29,6 +29,7 @@
>  #include <device/pci_ids.h>
>  #include <msr.h>
>  #include <amd_geodelx.h>
> +#include <lar.h>
>
> #define VSA2_BUFFER			0x60000
>  #define VSA2_ENTRY_POINT	0x60020
> @@ -169,25 +170,32 @@
>
> void do_vsmbios(void *bios)
>  {
> struct device *dev;
> unsigned long busdevfn;
> unsigned int rom = 0;
> unsigned char *buf;
> unsigned int size = SMM_SIZE * 1024;
> int i;
> unsigned long ilen, olen;
>
> unsigned long olen;
>
> struct mem_file archive;
>
> struct mem_file file;
>
> printk(BIOS_ERR, "do_vsmbios\n");
> /* clear vsm bios data area */
> for (i = 0x400; i < 0x500; i++) {
> *(volatile unsigned char *)i = 0;
> }
>
> init_archive(&archive);
> rom = (unsigned long) bios;
> if (find_file(&archive, "/blob/vsa", &file)){
> printk(BIOS_ERR, "NO VSA found!\n");
> return;
> }
> if (process_file(&file, (void *)VSA2_BUFFER)) {
> printk(BIOS_ERR, "Processing /blob/vsa failed\n");
> return;
> }
> buf = (unsigned char *)VSA2_BUFFER;
> // FIXME
> //olen = unrv2b((u8 *) rom, buf, &ilen);
> printk(BIOS_DEBUG, "buf ilen %d olen%d\n", ilen, olen);
> olen = file.reallen;
> printk(BIOS_DEBUG, "buf ilen %d olen%ld\n", file.len, olen);

Hm. Why is olen an unsigned long when reallen is u32?
> printk(BIOS_DEBUG, "buf %p *buf %d buf[256k] %d\n",
>    	     buf, buf[0], buf[SMM_SIZE * 1024]);
>    printk(BIOS_DEBUG, "buf[0x20] signature is %x:%x:%x:%x\n",
> @@ -253,16 +261,16 @@
>    	     eax, ebx, ecx, edx);
>    printk(BIOS_DEBUG, "biosint: ebp 0x%lx esp 0x%lx edi 0x%lx esi 0x%lx\n",
>    	     ebp, esp, edi, esi);
> printk(BIOS_DEBUG, "biosint:  ip 0x%x   cs 0x%x  flags 0x%x\n",
> printk(BIOS_DEBUG, "biosint:  ip 0x%lx   cs 0x%lx  flags 0x%lx\n",
>     ip, cs, flags);
> printk(BIOS_DEBUG, "biosint: gs 0x%x fs 0x%x ds 0x%x es 0x%x\n",
> printk(BIOS_DEBUG, "biosint: gs 0x%lx fs 0x%lx ds 0x%lx es 0x%lx\n",
>     gsfs >> 16, gsfs & 0xffff, dses >> 16, dses & 0xffff);

If cs, ds, es, fs are all 16bit, then dses and gsfs should be explicitly
32bit and not long. That would fix compile warnings as well. I
understand that biosint is x86emu code, though.
> // cases in a good compiler are just as good as your own tables.
>    switch (intnumber) {
>    case 0 ... 15:
>    	// These are not BIOS service, but the CPU-generated exceptions
> printk(BIOS_INFO, "biosint: Oops, exception %u\n", intnumber);
> printk(BIOS_INFO, "biosint: Oops, exception %lu\n", intnumber);
> if (esp < 0x1000) {
> 	printk(BIOS_DEBUG, "Stack contents: ");
> 	while (esp < 0x1000) {
>
> @@ -290,7 +298,7 @@
>    			  &ebx, &edx, &ecx, &eax, &flags);
>    	break;
>    default:
>  printk(BIOS_INFO, "BIOSINT: Unsupport int #0x%x\n", intnumber);
>  printk(BIOS_INFO, "BIOSINT: Unsupport int #0x%lx\n", intnumber);
> break;
>  }
>  if (ret)
> Index: arch/x86/stage1.c
>
> --- arch/x86/stage1.c	(revision 560)
> +++ arch/x86/stage1.c	(working copy)
> @@ -50,7 +50,21 @@
>    post_code(POST_STAGE1_ENABLE_ROM);
>  }
>
> +void init_archive(struct mem_file *archive)
> +{
>
> // FIXME this should be defined in the VPD area
>
> // but NOT IN THE CODE.
>
> /* The len field starts behind the reset vector on x86.
> * The start is not correct for all platforms. sc520 will
> * need some hands on here.
> */
>
> archive->len = *(u32 *)0xfffffff4;
>
> archive->start =(void *)(0UL-archive->len);
>
> // FIXME check integrity
>
> +}
>  /* until we get rid of elf */
>  int legacy(struct mem_file *archive, char *name, void *where, struct lb_memory *mem)
>  {
> @@ -121,19 +135,8 @@
>
> // location and size of image.
> // FIXME this should be defined in the VPD area
> // but NOT IN THE CODE.
> init_archive(&archive);
> /* The len field starts behind the reset vector on x86.
> * The start is not correct for all platforms. sc520 will
> * need some hands on here.
> */
> archive.len = *(u32 *)0xfffffff4;
> archive.start =(void *)(0UL-archive.len);
> // FIXME check integrity
> // find first initram
>  if (check_normal_boot_flag()) {
>   printk(BIOS_DEBUG, "Choosing normal boot.\n");

Nice refactoring.

Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel