Re: [coreboot] r783 - coreboot-v3/arch/x86

[Stefan Reinauer]

Carl-Daniel Hailfinger asked:
> /* we can't statically init this hack. */
> /* Why can't we statically init this hack? */

No global variables in stage 1.

r783 - coreboot-v3/arch/x86

[Carl-Daniel Hailfinger]

On 18.08.2008 15:56, Stefan Reinauer wrote:
> Carl-Daniel Hailfinger asked:
> > /* we can't statically init this hack. */
> > /* Why can't we statically init this hack? */
>
> No global variables in stage 1.

Hm. The variable is only used locally. We could at least use automatic
init for it.

Now I see it. The reason is that we would get a buffer overflow. Nasty.
struct lb_memory has a zero-sized map array, but we use one element.
struct lb_memory {
        u32 tag;
        u32 size;
        struct lb_memory_range map[0];
};

The fix is to declare
struct lb_memory_one_map {
        u32 tag;
        u32 size;
        struct lb_memory_range map[1];
};
and cast it to struct lb_memory.

Compile-tested patch follows.

Signed-off-by: Carl-Daniel Hailfinger

Index: corebootv3-stage1_elfload_cleanup/include/tables.h
===================================================================
--- corebootv3-stage1_elfload_cleanup/include/tables.h	(Revision 783)
+++ corebootv3-stage1_elfload_cleanup/include/tables.h	(Arbeitskopie)
@@ -135,6 +135,12 @@
    struct lb_memory_range map[0];
 };

+struct lb_memory_one_map {
+	u32 tag;
+	u32 size;
+	struct lb_memory_range map[1];
+};
+
 #define LB_TAG_HWRPB	0x0002
 struct lb_hwrpb {
    u32 tag;
Index: corebootv3-stage1_elfload_cleanup/arch/x86/stage1.c
===================================================================
--- corebootv3-stage1_elfload_cleanup/arch/x86/stage1.c	(Revision 783)
+++ corebootv3-stage1_elfload_cleanup/arch/x86/stage1.c	(Arbeitskopie)
@@ -127,18 +127,16 @@
    void *entry;
 #ifdef CONFIG_PAYLOAD_ELF_LOADER
    struct mem_file result;
-	int elfboot_mem(struct lb_memory *mem, void *where, int size);

/* Why can't we statically init this hack? */
-	unsigned char faker[64];
-	struct lb_memory *mem = (struct lb_memory*) faker;
+	struct lb_memory_one_map mem;

-	mem->tag = LB_TAG_MEMORY;
-	mem->size = 28;
-	mem->map[0].start.lo = mem->map[0].start.hi = 0;
-	mem->map[0].size.lo = (32*1024*1024);
-	mem->map[0].size.hi = 0;
-	mem->map[0].type = LB_MEM_RAM;
+	mem.tag = LB_TAG_MEMORY;
+	mem.size = sizeof(mem);
+	mem.map[0].start.lo = mem.map[0].start.hi = 0;
+	mem.map[0].size.lo = (32*1024*1024);
+	mem.map[0].size.hi = 0;
+	mem.map[0].type = LB_MEM_RAM;
 #endif /* CONFIG_PAYLOAD_ELF_LOADER */

post_code(POST_STAGE1_MAIN);
@@ -221,7 +219,7 @@
 #ifdef CONFIG_PAYLOAD_ELF_LOADER
    ret = find_file(&archive, "normal/payload", &result);
    if (! ret)
-		legacy(&archive, "normal/payload", (void *)UNCOMPRESS_AREA, mem);
+		legacy(&archive, "normal/payload", (void *)UNCOMPRESS_AREA, (struct lb_memory *)&mem);
 #endif /* CONFIG_PAYLOAD_ELF_LOADER */

entry = load_file_segments(&archive, "normal/payload");

r783 - coreboot-v3/arch/x86

[ron minnich]

On Mon, Aug 18, 2008 at 7:35 AM, Carl-Daniel Hailfinger
 wrote:
> On 18.08.2008 15:56, Stefan Reinauer wrote:
> > Carl-Daniel Hailfinger asked:
> > > /* we can't statically init this hack. */
> > > /* Why can't we statically init this hack? */
> >
> > No global variables in stage 1.
>
> Hm. The variable is only used locally. We could at least use automatic
> init for it.
>
> Now I see it. The reason is that we would get a buffer overflow. Nasty.

That's not really it. The issue is that a static initiializer in
stage1 requires data space we don't have in stage1, so I made it code.
> struct lb_memory has a zero-sized map array, but we use one element.
> struct lb_memory {
>        u32 tag;
>        u32 size;
>        struct lb_memory_range map[0];
> };
>
> The fix is to declare
> struct lb_memory_one_map {
>        u32 tag;
>        u32 size;
>        struct lb_memory_range map[1];
> };
> and cast it to struct lb_memory.

Turns out you do need this, ignore earlier email :-)

Actually, given all this ugliness, can we kill the ELF loader? This
would solve this mess. :-)

ron

r783 - coreboot-v3/arch/x86

[Stefan Reinauer]

ron minnich wrote:
> Actually, given all this ugliness, can we kill the ELF loader? This
> would solve this mess. :-)

My friend over at one of the 3 letter companies used to joke: "LinuxBIOS
is not Linux, it's just an ELF loader".

I enjoyed the looks when I told him "and for version 3 we even got rid
of the ELF loading" ...

Since we agreed upon SELF, I think the patch that implements SELF in v3
can also drop ELF loading in one step. Any takers?

Stefan

r783 - coreboot-v3/arch/x86

[ron minnich]

On Mon, Aug 18, 2008 at 9:12 AM, Stefan Reinauer  wrote:
> Since we agreed upon SELF, I think the patch that implements SELF in v3
> can also drop ELF loading in one step. Any takers?

get me the patch and I'll test it. ELF loading in the BIOS seemed like
a great idea but has been a real hassle since day one. It burned us
very badly on a linux alpha cluster we installed at sandia in 2001. I
would love to see it removed.

ron

r783 - coreboot-v3/arch/x86

[Carl-Daniel Hailfinger]

On 18.08.2008 18:15, ron minnich wrote:
> On Mon, Aug 18, 2008 at 9:12 AM, Stefan Reinauer  wrote
> > Since we agreed upon SELF, I think the patch that implements SELF in v3
> > can also drop ELF loading in one step. Any takers?
>
> get me the patch and I'll test it. ELF loading in the BIOS seemed like
> a great idea but has been a real hassle since day one. It burned us
> very badly on a linux alpha cluster we installed at sandia in 2001. I
> would love to see it removed.

Does the current SELF code allow per-segment compression selection? The
SELF wiki page http://www.coreboot.org/SELF suggests this is not the
case. How do we load data files to a prespecified location? The entry
point is a mandatory part of the SELF spec because it also works as
terminator.

Regards,
Carl-Daniel

r783 - coreboot-v3/arch/x86

[ron minnich]

On Mon, Aug 18, 2008 at 11:03 AM, Carl-Daniel Hailfinger
 wrote:
> Does the current SELF code allow per-segment compression selection? The
> SELF wiki page http://www.coreboot.org/SELF suggests this is not the
> case. How do we load data files to a prespecified location? The entry
> point is a mandatory part of the SELF spec because it also works as
> terminator.

"This segment has executable code to be copied from the data section
to memory. Each CODE segment has a corresponding block of data, which
may be compressed in the LAR. Multiple segments of this type are
allowed. "

I find the wording to mean that the data can be compressed.

ron

r783 - coreboot-v3/arch/x86

[Carl-Daniel Hailfinger]

On 18.08.2008 22:00, ron minnich wrote:
> On Mon, Aug 18, 2008 at 11:03 AM, Carl-Daniel Hailfinger
>  wrote:
> > Does the current SELF code allow per-segment compression selection? The
> > SELF wiki page http://www.coreboot.org/SELF suggests this is not the
> > case. How do we load data files to a prespecified location? The entry
> > point is a mandatory part of the SELF spec because it also works as
> > terminator.
>
> "This segment has executable code to be copied from the data section
> to memory. Each CODE segment has a corresponding block of data, which
> may be compressed in the LAR. Multiple segments of this type are
> allowed. "
>
> I find the wording to mean that the data can be compressed.

Yes, but where is the algorithm specified? If it's outside the SELF
header, we have to special-case "normal" non-SELF files.

Regards,
Carl-Daniel