Re: [coreboot] Fwd: [PATCH v3 1/1] libpayload: add support for finding and parsing Coreboot BIOS tables

[Philip Prindeville]

Since libpayload seems to be ownerless (but still active), I'm copying the 5 top committers of the last 100 commits.

Plus Andres, who is knowledgeable about the Geode platforms (many of which use Coreboot).

On 8/19/11 4:23 PM, Andrew Morton wrote:
> On Thu, 18 Aug 2011 20:55:56 -0700
> Philip Prindeville  wrote:
> > Hi Andrew,
> >
> > Any chance this could make it in for 3.2?
>
> Perhaps.  But I'm not on the linux-geode list (few people are!) and I
> don't like to merge privately-sent patches.
>
> So please do a full resend, ccing myself and linux-kernel and then
> we'll see what happens.
>
> mini-review follows:
> > -------- Original Message --------
> > Subject: [PATCH v2 1/1] libpayload: add support for finding and parsing Coreboot BIOS tables
> > Date: Wed, 17 Aug 2011 23:11:00 -0700
> > From: Philip Prindeville 
> > To: [e-mail],        Patrick Georgi 
> > CC: Andres Salomon
> >
> > Many experimental and small design house SBC's use Coreboot; here we add linux support for it.  Platform drivers on generic processors such as Geode or C7 may interrogate Coreboot for the vendor and model.
> >
> > Removed the conditional compilation and non-kernel parts.
> >
> > Split into architecture indpendent and machine-specific parts as per Andres' comment.
> >
> > Signed-off-by: Philip Prindeville 
> > Acked-by: Patrick Georgi
> >
> > Taken from the coreboot project, largely unmodified.
>
> Given that it was taken from coreboot, are you sure that we have all
> the signed-off-by:s?  It seems that you're the primary/sole author so I
> guess "yes".

I'll add that when I post to linux-kernel after all review comments have been addressed.
> The changelog is way too terse.  I don't actually know what coreboot
> *is*, and if you don't tell me, who will?

I'll add that for the final review.
> What does the code do?  How does it work?  What interfaces does it
> offer and how does client code use them?  etc.
>
> Should there be an associated documentation file?
>
> None of the exported interfaces are documented at all.  This makes
> review harder and less effective and makes the code harder and less
> reliable to use.

Yeah, unfortunately that's how it is at the source, too (the coreboot project).

The upstream documentation is a little thin.

http://www.coreboot.org/
http://www.coreboot.org/Libpayload

but I can include a pointer to it.
> diff --git a/include/linux/libpayload/coreboot_tables.h b/include/linux/libpayload/coreboot_tables.h
> new file mode 100644
> index 0000000..ab1a652
> --- /dev/null
> +++ b/include/linux/libpayload/coreboot_tables.h
> @@ -0,0 +1,233 @@
> +/*
> This file is part of the libpayload project.
> Copyright (C) 2008 Advanced Micro Devices, Inc.
> Copyright (C) 2011 Philip Prindeville, Redfish Solutions, LLC.
> Redistribution and use in source and binary forms, with or without
> modification, are permitted provided that the following conditions
> are met:
> Redistributions of source code must retain the above copyright
> notice, this list of conditions and the following disclaimer.
> Redistributions in binary form must reproduce the above copyright
> notice, this list of conditions and the following disclaimer in the
> documentation and/or other materials provided with the distribution.
> The name of the author may not be used to endorse or promote products
> derived from this software without specific prior written permission.
> THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
> ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
> FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> SUCH DAMAGE.
> */
>
> +#ifndef _COREBOOT_TABLES_H
> +#define _COREBOOT_TABLES_H
>
> +#include <asm-generic/int-ll64.h>
>
> +struct cbuint64 {
> u32 lo;
> u32 hi;
>
> +};
>
> I assume that all the structures in this file are based on some
> coreboot spec?  Without that spec, the implementation is unreviewable! 
> If such a spec is available, please prominently describe how the
> reviewer can obtain it, within this file.

It might be documented, but the Doxygen stuff seems to be faulting...  I get a stack exception when I go to:

http://qa.coreboot.org/docs/doxygen/
> u8 reserved_mask_pos;
> u8 reserved_mask_size;
>
> +};
>
> +#define CB_TAG_CMOS_OPTION_TABLE 0x00c8
> +struct cb_cmos_option_table {
> u32 tag;
> u32 size;
> u32 header_length;
>
> +};
>
> +#define CB_TAG_OPTION         0x00c9
> +#define CMOS_MAX_NAME_LENGTH    32
> +struct cb_cmos_entries {
> u32 tag;
> u32 size;
> u32 bit;
> u32 length;
> u32 config;
> u32 config_id;
> u8 name[CMOS_MAX_NAME_LENGTH];
>
> +};
>
> +#define CB_TAG_OPTION_ENUM    0x00ca
> +#define CMOS_MAX_TEXT_LENGTH 32
> +struct cb_cmos_enums {
> u32 tag;
> u32 size;
> u32 config_id;
> u32 value;
> u8 text[CMOS_MAX_TEXT_LENGTH];
>
> +};
>
> +#define CB_TAG_OPTION_DEFAULTS 0x00cb
> +#define CMOS_IMAGE_BUFFER_SIZE 128
> +struct cb_cmos_defaults {
> u32 tag;
> u32 size;
> u32 name_length;
> u8 name[CMOS_MAX_NAME_LENGTH];
> u8 default_set[CMOS_IMAGE_BUFFER_SIZE];
>
> +};
>
> +#define CB_TAG_OPTION_CHECKSUM 0x00cc
> +#define CHECKSUM_NONE	0
> +#define CHECKSUM_PCBIOS	1
> +struct	cb_cmos_checksum {
> u32 tag;
> u32 size;
> u32 range_start;
> u32 range_end;
> u32 location;
> u32 type;
>
> +};
>
> +/* Helpful macros */
>
> +#define MEM_RANGE_COUNT(_rec) \
> (((_rec)->size - sizeof(*(_rec))) / sizeof((_rec)->map[0]))
>
> +#define MEM_RANGE_PTR(_rec, _idx) \
> (((u8 *) (_rec)) + sizeof(*(_rec)) \
> (sizeof((_rec)->map[0]) * (_idx)))
>
> +#define MB_VENDOR_STRING(_mb) \
> (((unsigned char *) ((_mb)->strings)) + (_mb)->vendor_idx)
>
> +#define MB_PART_STRING(_mb) \
> (((unsigned char *) ((_mb)->strings)) + (_mb)->part_number_idx)
>
> +#define UNPACK_CB64(_in) \
> ( (((u64) _in.hi) << 32) | _in.lo )
>
> If at all possible, these should be implemented as (lower-cased) C
> functions, not as macros.  Because

The first two can't be, since they are polymorphous (i.e. the "sizeof()" needs to work directly on the typeof() the argument it's getting).
> for some reason, people are more likely to document functions than
> macros.
>
> C functions provide argument typechecking
>
> C functions are more readable
>
> C functions can prevent unused-var warnings in obscure situations
>
> C functions don't cause bugs when called using an expression with
> side-effects.  The above macros are in fact buggy if called with, for
> example, UNPACK_CB64(*foo++).
>
> C functions don't need vast amounts of parenthesis around argumetns
> to prevent compilation errors.  UNPACK_CB64() lacks these, for example.

Done for the remainder.
> +extern int get_coreboot_info(struct sysinfo_t *info);
> +extern int cb_parse_header(void *addr, void *end, struct sysinfo_t *info);
>
> +#endif
> diff --git a/include/linux/libpayload/libpayload.h b/include/linux/libpayload/libpayload.h
> new file mode 100644
> index 0000000..e2a4939
> --- /dev/null
> +++ b/include/linux/libpayload/libpayload.h
> @@ -0,0 +1,48 @@
> +/*
> This file is part of the libpayload project.
> Copyright (C) 2008 Advanced Micro Devices, Inc.
> Copyright (C) 2011 Philip Prindeville, Redfish Solutions, LLC.
> Redistribution and use in source and binary forms, with or without
> modification, are permitted provided that the following conditions
> are met:
> Redistributions of source code must retain the above copyright
> notice, this list of conditions and the following disclaimer.
> Redistributions in binary form must reproduce the above copyright
> notice, this list of conditions and the following disclaimer in the
> documentation and/or other materials provided with the distribution.
> The name of the author may not be used to endorse or promote products
> derived from this software without specific prior written permission.
> THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
> ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
> FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> SUCH DAMAGE.
> */
>
> +#ifndef _LIBPAYLOAD_H
> +#define _LIBPAYLOAD_H
>
> +#include <linux/types.h>
> +#include <linux/unistd.h>
> +#include <linux/string.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/printk.h>
> +#include <linux/mm.h>
> +#include <linux/highmem.h>
>
> +#include <asm/io.h>
> +#include <asm/page.h>
> +#include <asm/checksum.h>
>
> +#define ipchksum(start, len)    ip_compute_csum(start, len)
>
> +#endif
>
> I think this file needs to go away altogether.  In the kernel we don't
> like "#include <everything.h>" header files.  Just explicitly list all
> the needed includes directly, within each and every .c file.  Nice and
> simple.

Done.
> The ipchksum() macros hould be removed - jsut edit the code to call
> ip_compute_csum().

I used a convenience function for this: cb_checksum().
> Is ip_compute_csum() available without CONFIG_NET?  Seems to be "yes"
> for x86, I don't know if that's true for other architectures, should
> they developer coreboot support.

For now, non-x86 platforms aren't supported.  Worst case, we add a K-select or K-depends-on for it.
> > ...
> > +struct sysinfo_t {
>
> The _t should only be used in rare circumstances, for typedefs.  This
> should be "struct sysinfo".  But we already have one of those.  I'd
> suggest a rename to "struct coreboot_sysinfo".

Fixed.  Called cb_sysinfo.
> unsigned int cpu_khz;
> unsigned short ser_ioport;
> unsigned long ser_base; // for mmapped serial
> int n_memranges;
> struct memrange {
> unsigned long long base;
> unsigned long long size;
> unsigned int type;
> } memrange[SYSINFO_MAX_MEM_RANGES];
> struct cb_cmos_option_table *option_table;
> u32 cmos_range_start;
> u32 cmos_range_end;
> u32 cmos_checksum_location;
> struct cb_framebuffer *framebuffer;
> unsigned long *mbtable; /** Pointer to the multiboot table */
> struct cb_header *header;
> struct cb_mainboard *mainboard;
>
> +};
>
> +extern struct sysinfo_t lib_sysinfo;
> +extern int lib_get_sysinfo(void);
>
> All globally exported symbols from code such as this should identify
> what subsystem they belong to.  So all these things should start with
> "coreboot_", please.

Done as "cb_".
> Or "payload".  This subsystem seems to have two main identifiers:
> coreboot and payload.  This is confusing.  I prefer "coreboot", because
> "payload" is terribly vague and could refer to anything at all.  Looky:
>
> z:/usr/src/linux-3.1-rc2> grep -r payload . | wc -l
> 5368
>
> How do you distinguish this subsystem from all that?
>
> +#endif
>
> diff --git a/lib/Kconfig b/lib/Kconfig
> index 6c695ff..172c721 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -91,6 +91,8 @@ config AUDIT_GENERIC
>    depends on AUDIT && !AUDIT_ARCH
>    default y
>
> +source "lib/libpayload/Kconfig"
>
> #
>  # compression support is select'ed if needed
>  #
> diff --git a/lib/Makefile b/lib/Makefile
> index 15f7bb6..5af01ee 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -72,6 +72,7 @@ obj-$(CONFIG_BCH) += bch.o
>  obj-$(CONFIG_LZO_COMPRESS) += lzo/
>  obj-$(CONFIG_LZO_DECOMPRESS) += lzo/
>  obj-$(CONFIG_XZ_DEC) += xz/
> +obj-$(CONFIG_LIBPAYLOAD) += libpayload/
>  obj-$(CONFIG_RAID6_PQ) += raid6/
>
> lib-$(CONFIG_DECOMPRESS_GZIP) += decompress_inflate.o
> diff --git a/lib/libpayload/Kconfig b/lib/libpayload/Kconfig
> new file mode 100644
> index 0000000..89c0fd8
> --- /dev/null
> +++ b/lib/libpayload/Kconfig
> @@ -0,0 +1,5 @@
> +config LIBPAYLOAD
> bool "Libpayload support"
> depends on X86
> help
>  Libpayload support for locating coreboot tables.
>
> diff --git a/lib/libpayload/Makefile b/lib/libpayload/Makefile
> new file mode 100644
> index 0000000..d65f5d4
> --- /dev/null
> +++ b/lib/libpayload/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_LIBPAYLOAD) += coreboot.o sysinfo.o coreboot_x86.o
> diff --git a/lib/libpayload/coreboot.c b/lib/libpayload/coreboot.c
> new file mode 100644
> index 0000000..e1937e6
> --- /dev/null
> +++ b/lib/libpayload/coreboot.c
> @@ -0,0 +1,213 @@
> +/*
> This file is part of the libpayload project.
> Copyright (C) 2008 Advanced Micro Devices, Inc.
> Copyright (C) 2009 coresystems GmbH
> Copyright (C) 2011 Philip Prindeville, Redfish Solutions, LLC.
> Redistribution and use in source and binary forms, with or without
> modification, are permitted provided that the following conditions
> are met:
> Redistributions of source code must retain the above copyright
> notice, this list of conditions and the following disclaimer.
> Redistributions in binary form must reproduce the above copyright
> notice, this list of conditions and the following disclaimer in the
> documentation and/or other materials provided with the distribution.
> The name of the author may not be used to endorse or promote products
> derived from this software without specific prior written permission.
> THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
> ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
> FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> SUCH DAMAGE.
> */
>
> +#include <linux/libpayload/libpayload.h>
> +#include <linux/libpayload/sysinfo.h>
> +#include <linux/libpayload/coreboot_tables.h>
>
> +#include <linux/highmem.h>
> +#include <asm/page.h>
>
> +/*
> Some of this is x86 specific, and the rest of it is generic. Right now,
> since we only support x86, we'll avoid trying to make lots of infrastructure
> we don't need. If in the future, we want to use coreboot on some other
> architecture, then take out the generic parsing code and move it elsewhere.
> */
>
> +/* === Parsing code === */
> +/* This is the generic parsing code. */
>
> +#define page_base(addr)		((typeof(addr)) (((uintptr_t)(addr) & PAGE_MASK)))
>
> Remove, use PAGE_ALIGN

Can't.  PAGE_ALIGN() rounds *up* to the end of the page.  I need the base page address.
> +#define page_offset(addr)	(((uintptr_t)(addr) & ~PAGE_MASK))
>
> There might be an existing helper for this also.  It should at least be
> written in C so people can't do
>
> struct foo bar;
>    page_offset(bar);

Done.
> +static void cb_parse_memory(unsigned char *ptr, struct sysinfo_t *info)
>
> Documentation...

I'll defer this to upstream.  I'm trying to keep my code from becoming too divergent from what's upstream so it stays relatively simple to port bugfixes.

Patrick: can we merge some of these changes back into coreboot/payloads/libpayload/arch/x86 ?
> +{
> struct cb_memory *mem = (struct cb_memory *)ptr;
> int count = MEM_RANGE_COUNT(mem);
> int i;
> if (count > SYSINFO_MAX_MEM_RANGES)
> count = SYSINFO_MAX_MEM_RANGES;
> info->n_memranges = 0;
> for (i = 0; i < count; i++) {
> struct cb_memory_range *range =
>     (struct cb_memory_range *)MEM_RANGE_PTR(mem, i);
>
> Arrange for the MEM_RANGE_PTR() repalcement to return the correct type,
> or void* then lose the typecast.

Done.
> info->memrange[info->n_memranges].base =
>     UNPACK_CB64(range->start);
> info->memrange[info->n_memranges].size =
>     UNPACK_CB64(range->size);
> info->memrange[info->n_memranges].type = range->type;
> info->n_memranges++;
> }
>
> +}
>
> +static void cb_parse_serial(unsigned char *ptr, struct sysinfo_t *info)
> +{
> struct cb_serial *ser = (struct cb_serial *)ptr;
> info->ser_ioport = ser->ioport;
>
> +}
>
> Make the arg void* and lose the typecast?

Done.
> +static void cb_parse_optiontable(unsigned char *ptr, struct sysinfo_t *info)
> +{
> info->option_table = (struct cb_cmos_option_table *)ptr;
>
> +}
>
> Ditto.

Done.
> +static void cb_parse_checksum(unsigned char *ptr, struct sysinfo_t *info)
> +{
> struct cb_cmos_checksum *cmos_cksum = (struct cb_cmos_checksum *)ptr;
> info->cmos_range_start = cmos_cksum->range_start;
> info->cmos_range_end = cmos_cksum->range_end;
> info->cmos_checksum_location = cmos_cksum->location;
>
> +}
>
> +static void cb_parse_framebuffer(unsigned char *ptr, struct sysinfo_t *info)
> +{
> info->framebuffer = (struct cb_framebuffer *)ptr;
>
> +}
>
> More.

Done.
> +int cb_parse_header(void *addr, void *end, struct sysinfo_t *info)
> +{
>    struct cb_header *header;
>    unsigned char *ptr = addr;
>
> make this void* then remove lots of typecasts.

Done.
>    void *forward;
>    int i;
>    for (i = 0; (void *)ptr < end; i += 16, ptr += 16) {
>            header = (struct cb_header *)ptr;
>            if (!strncmp((const char *)header->signature, "LBIO", 4))
>                    break;
>    }
>    /* We walked the entire space and didn't find anything. */
>    if ((void *)ptr >= end)
>            return -1;
> printk(KERN_DEBUG "coreboot: signature %.4s at %p\n",
>       header->signature, header);
> if (!header->table_bytes)
> return 0;
> /* Make sure the checksums match. */
> if (ipchksum(header, sizeof(*header)) != 0)
> return -1;
> printk(KERN_DEBUG "coreboot: header checksum verified\n");
> if (ipchksum((char *)header + sizeof(*header),
>      header->table_bytes) != header->table_checksum)
> return -1;
> printk(KERN_DEBUG "coreboot: table checksum verified\n");
> info->header = header;
> /* Now, walk the tables. */
> ptr += header->header_bytes;
> printk(KERN_DEBUG "coreboot: table start %p\n", ptr);
> for (i = 0; i < header->table_entries; i++) {
> struct cb_record *rec = (struct cb_record *)ptr;
> printk(KERN_DEBUG "coreboot: entry[%u] @ %p type %04x\n",
>        i, rec, rec->tag);
> /* We only care about a few tags here (maybe more later). */
> switch (rec->tag) {
> case CB_TAG_FORWARD:
> 	forward = (void *)(uintptr_t)((struct cb_forward *)rec)->forward;
>
> omigod.  Your task is to get all the types sorted out and , make this
> sort of thing vanish, please.

Hard to do... "forward" is a 64-bit quantity, so it needs to be down-converted to the same width as your native pointer size, then cast back to a void *.

Otherwise, we'll get compilation warnings which I hate to see... they're sloppy.
> 	printk(KERN_DEBUG "coreboot: forward to %p\n",
> 	       forward);
> 	if (phys_to_virt((phys_addr_t) forward) >= high_memory)
> 		addr = ioremap((phys_addr_t) page_base(forward), 0x1000)
> 			+ page_offset(forward);
> 	else
> 		addr = phys_to_virt((phys_addr_t) forward);
> 	/* we will leave the pages mapped */
> 	return cb_parse_header(addr, addr + 0x1000, info);
> 	continue;
>
> All the playing with highmem internals is a bit obscure.  I can't tell
> what it's up to.  Code comments, please.

Done.
> case CB_TAG_MEMORY:
> 	cb_parse_memory(ptr, info);
> 	break;
> case CB_TAG_SERIAL:
> 	cb_parse_serial(ptr, info);
> 	break;
> case CB_TAG_CMOS_OPTION_TABLE:
> 	cb_parse_optiontable(ptr, info);
> 	break;
> case CB_TAG_OPTION_CHECKSUM:
> 	cb_parse_checksum(ptr, info);
> 	break;
> // FIXME we should warn on serial if coreboot set up a
> // framebuffer buf the payload does not know about it.
>
> Please run the entire patch through scripts/checkpatch.pl.

Done.
> case CB_TAG_FRAMEBUFFER:
> 	cb_parse_framebuffer(ptr, info);
> 	break;
> case CB_TAG_MAINBOARD:
> 	info->mainboard = (struct cb_mainboard *)ptr;
> 	break;
> case CB_TAG_VERSION:
> case CB_TAG_EXTRA_VERSION:
> case CB_TAG_BUILD:
> case CB_TAG_COMPILE_TIME:
> case CB_TAG_COMPILE_BY:
> case CB_TAG_COMPILE_HOST:
> case CB_TAG_COMPILE_DOMAIN:
> case CB_TAG_COMPILER:
> case CB_TAG_LINKER:
> case CB_TAG_ASSEMBLER:
> 	printk(KERN_DEBUG "cb_parse_string: [%04x] %s\n",
> 	       rec->tag, ((struct cb_string *)rec)->string);
> 	break;
> }
> ptr += rec->size;
> if ((void *)ptr >= end)
> 	return -1;
> }
> printk(KERN_DEBUG "coreboot: success (%u entries)!\n",
>       header->table_entries);
> return 1;
>
> +}
>
> diff --git a/lib/libpayload/coreboot_x86.c b/lib/libpayload/coreboot_x86.c
> new file mode 100644
> index 0000000..fa68041
> --- /dev/null
> +++ b/lib/libpayload/coreboot_x86.c
> @@ -0,0 +1,54 @@
> +/*
> This file is part of the libpayload project.
> Copyright (C) 2008 Advanced Micro Devices, Inc.
> Copyright (C) 2009 coresystems GmbH
> Copyright (C) 2011 Philip Prindeville, Redfish Solutions, LLC.
> Redistribution and use in source and binary forms, with or without
> modification, are permitted provided that the following conditions
> are met:
> Redistributions of source code must retain the above copyright
> notice, this list of conditions and the following disclaimer.
> Redistributions in binary form must reproduce the above copyright
> notice, this list of conditions and the following disclaimer in the
> documentation and/or other materials provided with the distribution.
> The name of the author may not be used to endorse or promote products
> derived from this software without specific prior written permission.
> THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
> ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
> FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> SUCH DAMAGE.
> */
>
> +#include <linux/libpayload/libpayload.h>
> +#include <linux/libpayload/sysinfo.h>
> +#include <linux/libpayload/coreboot_tables.h>
>
> +#include <linux/highmem.h>
> +#include <asm/page.h>
>
> +/* == Architecture specific == */
> +/* This is the x86 specific stuff. */
>
> +int get_coreboot_info(struct sysinfo_t *info)
>
> The globally-visible symbol should start with the subsystem identifier.
> coreboot_get_info() would suit.

Done.
> ALso, needs documentation.  *What* info?
>
> +{
> int ret;
> void *base = phys_to_virt(0x00000000);
> ret = cb_parse_header(base, base + 0x1000, info);
> if (ret != 1) {
> base = phys_to_virt(0x000f0000);
> ret = cb_parse_header(base, base + 0x1000, info);
> }
> return (ret == 1) ? 0 : -1;
>
> +}
>
> diff --git a/lib/libpayload/sysinfo.c b/lib/libpayload/sysinfo.c
> new file mode 100644
> index 0000000..21c22f9
> --- /dev/null
> +++ b/lib/libpayload/sysinfo.c
> @@ -0,0 +1,68 @@
> +/*
> This file is part of the libpayload project.
> Copyright (C) 2008 Advanced Micro Devices, Inc.
> Copyright (C) 2011 Philip Prindeville, Redfish Solutions, LLC.
> Redistribution and use in source and binary forms, with or without
> modification, are permitted provided that the following conditions
> are met:
> Redistributions of source code must retain the above copyright
> notice, this list of conditions and the following disclaimer.
> Redistributions in binary form must reproduce the above copyright
> notice, this list of conditions and the following disclaimer in the
> documentation and/or other materials provided with the distribution.
> The name of the author may not be used to endorse or promote products
> derived from this software without specific prior written permission.
> THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
> ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
> FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> SUCH DAMAGE.
> */
>
> +#include <linux/libpayload/libpayload.h>
> +#include <linux/libpayload/sysinfo.h>
> +#include <linux/libpayload/coreboot_tables.h>
>
> +/**
> This is a global structure that is used through the library - we set it
> up initially with some dummy values - hopefully they will be overridden.
> */
>
> +struct sysinfo_t lib_sysinfo = {
> .ser_ioport = 0x3f8,
>
> +};
> +EXPORT_SYMBOL(lib_sysinfo);
>
> Again, poor naming choices.  coreboot_sysinfo_lib, perhaps.

Done.
> +int lib_get_sysinfo(void)
> +{
> int ret;
> /* Get information from the coreboot tables,
> * if they exist */
> ret = get_coreboot_info(&lib_sysinfo);
> if (!lib_sysinfo.n_memranges) {
> /* If we can't get a good memory range, use the default. */
> lib_sysinfo.n_memranges = 2;
> lib_sysinfo.memrange[0].base = 0;
> lib_sysinfo.memrange[0].size = 640 * 1024;
> lib_sysinfo.memrange[0].type = CB_MEM_RAM;
> lib_sysinfo.memrange[1].base = 1024 * 1024;
> lib_sysinfo.memrange[1].size = 31 * 1024 * 1024;
> lib_sysinfo.memrange[1].type = CB_MEM_RAM;
> }
> return ret;
>
> +}
> +EXPORT_SYMBOL(lib_get_sysinfo);
>
> coreboot_sysinfo_lib_get?

Done.

Attached.