[coreboot] [PATCH] Multiboot

[Robert Millan]

On Thu, Sep 18, 2008 at 11:59:13PM +0200, Carl-Daniel Hailfinger wrote:
> I thought Ron and Stefan asked for unconditional lbtable

They did, and I changed that in my last patch.  The rationale being:

  - lbtable is useful in some situations (Stefan)

  - size is not that important (Ron)

> and conditional multiboot.

They didn't.  You must be confused.

> This patch lacks all the documentation contained in the earlier
> versions. Please fix.

Can you be more specific?  I'm not sure which documentation are you
referring to.

> > +	/* FIXME: is this alignment needed for PIRQ table? */
> > +	rom_table_end = (rom_table_end + 1023) & ~1023;
> >   
> 
> Please explain the two lines above. They are not mentioned in the changelog.

Before my patch, PIRQ tables were implicitly 1k-aligned due to their hardcoded
location being 0xf0000.  Inmediately after writing them, this 1k-alignment is
enforced because it's apparantly needed for ACPI tables.  However, existing
code doesn't give any indication on whether PIRQ tables require the same
alignment or not.

Therefore, since my code pushes them, I thought it's better to be safe than
sorry and add the same alignment.

But if someone can answer the question on whether this alignment is needed or
not, then we can remove either one or both of these lines.

> > Index: arch/x86/stage1.c
> > ===================================================================
> > --- arch/x86/stage1.c	(revision 867)
> > +++ arch/x86/stage1.c	(working copy)
> > @@ -28,6 +28,7 @@
> >  #include <lib.h>
> >  #include <mc146818rtc.h>
> >  #include <cpu.h>
> > +#include <multiboot.h>
> >  
> >  #ifdef CONFIG_PAYLOAD_ELF_LOADER
> >  /* ah, well, what a mess! This is a hard code. FIX ME but how? 
> > @@ -139,6 +140,14 @@
> >  }
> >  #endif /* CONFIG_PAYLOAD_ELF_LOADER */
> >  
> > +
> > +int run_address_multiboot(void *f)
> > +{
> > +	int ret;
> > +	__asm__ __volatile__ ("call *%3" : "=a" (ret) : "a" (MB_MAGIC2), "b" (0xf0000), "c" (f));
> > +	return ret;
> > +}
> > +
> >   
> 
> The function above belongs in multiboot.c

Note the stage1/stage2 separation.  It would require either:

  - multiboot.c to be linked in twice, once for stage1 and once for stage2

  - multiboot.c to be split in two files, one for stage1 and one for stage2

this is why I initially used multiboot.h and made the function inline (which
surprisingly results in zero size increase).

Which way do we go?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."