[PATCH] put coreboot table in high tables area

[Stefan Reinauer]

See patches

I'll gladly send another fix for libpayload if the two patches are accepted

[Peter Stuge]

Stefan Reinauer wrote:
> This patch adds "high coreboot table support" to coreboot version 2.

I like it.
> This patch also adds "table forward" support to flashrom and
> nvramtool.

The additions to flashrom aren't super clean. Please change the
variable name low_1MB now that it doesn't always map to the low 1MB.
Also please make the find_lb_table() call use the new #define instead
of hardcoded 1024*1024 since it was substituted everywhere else.
> if (!lb_table)
>    	lb_table = find_lb_table(low_1MB, 0xf0000, 1024*1024);

(This call.)

Please fix that, then it's:

Acked-by: Peter Stuge

[Stefan Reinauer]

On 16.03.2009 23:50 Uhr, Peter Stuge wrote:
> Stefan Reinauer wrote:
> > This patch adds "high coreboot table support" to coreboot version 2.
>
> I like it.
> > This patch also adds "table forward" support to flashrom and
> > nvramtool.
>
> The additions to flashrom aren't super clean. Please change the
> variable name low_1MB now that it doesn't always map to the low 1MB.
> Also please make the find_lb_table() call use the new #define instead
> of hardcoded 1024*1024 since it was substituted everywhere else.
> > if (!lb_table)
> >    	lb_table = find_lb_table(low_1MB, 0xf0000, 1024*1024);
>
> (This call.)
>
> Please fix that, then it's:
>
> Acked-by: Peter Stuge

Thanks a lot!

Fixed the issues, and committed as 4011-4013

[Kevin O'Connor]

Hi Stefan,

On Mon, Mar 16, 2009 at 05:23:35PM +0100, Stefan Reinauer wrote:
> See patches
>
> I'll gladly send another fix for libpayload if the two patches are accepted

[...]
> --- src/include/boot/coreboot_tables.h	(revision 1304)
> +++ src/include/boot/coreboot_tables.h	(working copy)
> @@ -155,6 +155,13 @@
>    uint16_t type;
>  };
>
> +#define LB_TAG_FORWARD		0x0011
> +struct lb_forward {
> uint32_t tag;
> uint32_t size;
> uint64_t forward;
>
> +};

One suggestion - instead of making the "floating table" be a sub-table
of a coreboot-table, make it a stand alone data structure.  As it
stands, I think existing code might get confused when it finds a
coreboot table without any real data in it.

-Kevin

[Stefan Reinauer]

On 17.03.2009 15:43 Uhr, Kevin O'Connor wrote:
> One suggestion - instead of making the "floating table" be a sub-table
> of a coreboot-table, make it a stand alone data structure.  As it
> stands, I think existing code might get confused when it finds a
> coreboot table without any real data in it.

No worries about that, almost all existing code is fixed up by now.
Also, the existing implementations are not less confused if they don't
find a table at all. The format was designed to be flexible enough to
handle any number of subtables dynamically.

Attached  is the patch against libpayload.

[Myles Watson]

2009/3/17 Stefan Reinauer :
> On 17.03.2009 15:43 Uhr, Kevin O'Connor wrote:
> > One suggestion - instead of making the "floating table" be a sub-table
> > of a coreboot-table, make it a stand alone data structure.  As it
> > stands, I think existing code might get confused when it finds a
> > coreboot table without any real data in it.
>
> No worries about that, almost all existing code is fixed up by now.
> Also, the existing implementations are not less confused if they don't
> find a table at all. The format was designed to be flexible enough to
> handle any number of subtables dynamically.
>
> Attached  is the patch against libpayload.

Acked-by: Myles Watson

Thanks,
Myles

[Stefan Reinauer]

On 17.03.2009 16:53 Uhr, Myles Watson wrote:
> 2009/3/17 Stefan Reinauer :
> > On 17.03.2009 15:43 Uhr, Kevin O'Connor wrote:
> > > One suggestion - instead of making the "floating table" be a sub-table
> > > of a coreboot-table, make it a stand alone data structure.  As it
> > > stands, I think existing code might get confused when it finds a
> > > coreboot table without any real data in it.
> >
> > No worries about that, almost all existing code is fixed up by now.
> > Also, the existing implementations are not less confused if they don't
> > find a table at all. The format was designed to be flexible enough to
> > handle any number of subtables dynamically.
> >
> > Attached  is the patch against libpayload.
>
> Acked-by: Myles Watson
>
> Thanks,
> Myles

Thanks, r4016

[Peter Stuge]

Stefan Reinauer wrote:
> +++ i386/coreboot.c	(working copy)

..
> case CB_TAG_FORWARD:
> 	return cb_parse_header((void *)(unsigned long)((struct cb_forward *)rec)->forward, len, info);
> 	continue;

Are these semantics correct? Will this recurse? Should it also
continue parsing after a forward tag?

I guess we should have decided on these things before committing
but.. :)

If the intent is to never have more than one forward tag and never
have anything but the forward tag in one table if there is a forward
tag at all I think we should at least document it but ideally codify
it, so that we don't end up with a situation where someone tries to
"misuse" the structures, or fail to parse them?

//Peter

[Stefan Reinauer]

On 18.03.2009 12:12 Uhr, Peter Stuge wrote:
> Stefan Reinauer wrote:
> > +++ i386/coreboot.c	(working copy)
>
> ..
> > case CB_TAG_FORWARD:
> > 	return cb_parse_header((void *)(unsigned long)((struct cb_forward *)rec)->forward, len, info);
> > 	continue;
>
> Are these semantics correct? Will this recurse? Should it also
> continue parsing after a forward tag?
>
> I guess we should have decided on these things before committing
> but.. :)

My word is code decides, not the comitee ;-)
> If the intent is to never have more than one forward tag and never
> have anything but the forward tag in one table if there is a forward
> tag at all I think we should at least document it but ideally codify
> it, so that we don't end up with a situation where someone tries to
> "misuse" the structures, or fail to parse them?

Unless someone else is going to create a Firmware that produces a
coreboot table in a different way than it is right now, we might want to
revisit. In the current scenario this would merely be academic.

Stefan

[Rudolf Marek]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Kevin,

Can you pickup the patch.

- --- src/coreboot.c	(revision 1300)
+++ src/coreboot.c	(working copy)

Its needed and I spent lot of time to figure it out whats wrong.

Thanks,
Rudolf
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAknE0ykACgkQ3J9wPJqZRNWb8QCgsbxGEZQ/EbR5T5uiQdxuHKOZ
zBgAn2S8culLaUG6zb9P1vZTiraUoeCz
=cvFv
-----END PGP SIGNATURE-----

[Kevin O'Connor]

On Sat, Mar 21, 2009 at 12:44:44PM +0100, Rudolf Marek wrote:
> Hi Kevin,
>
> Can you pickup the patch.
> --- src/coreboot.c	(revision 1300)
>
> +++ src/coreboot.c	(working copy)
>
> Its needed and I spent lot of time to figure it out whats wrong.

Sorry.  It's in latest SeaBIOS git now.

-Kevin