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

[Philip Prindeville]

Below are some of the comments I got from Andrew after sending this patch to him (I had also sent it to linux-geode which he's not apparently on, even though he's the upstream merge-master for it).

Here are his comments.

Most of them I can apply...  A few of them I'm not so sure about.  I was hoping to keep the version in the linux kernel in close sync with the one in coreboot's repository, if only to make it easier to apply patches to both.

What are the feelings of Patrick, Sven, Steven, etc?

-------- Original Message --------
Subject: Re: Fwd: [PATCH v2 1/1] libpayload: add support for finding and parsing Coreboot BIOS tables
Date: Fri, 19 Aug 2011 16:23:27 -0700
From: Andrew Morton 
To: Philip Prindeville

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".

The changelog is way too terse.  I don't actually know what coreboot
*is*, and if you don't tell me, who will?

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.

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:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. 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.
+ * 3. 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.

+	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

- 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.

+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:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. 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.
+ * 3. 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.

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

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.
> ...
> +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".

+	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.

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:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. 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.
+ * 3. 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

+#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);

+static void cb_parse_memory(unsigned char *ptr, struct sysinfo_t *info)

Documentation...

+{
+	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.

+		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?

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

Ditto.

+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.

+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.

+        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.

+			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.

+		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.

+		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:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. 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.
+ * 3. 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.

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:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. 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.
+ * 3. 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.

+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?