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

Philip Prindeville philipp_subx at redfish-solutions.com
Mon Aug 22 02:37:02 CEST 2011


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 <philipp at redfish-solutions.com> 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 <philipp_subx at redfish-solutions.com>
>> To: linux-geode at lists.infradead.org,        Patrick Georgi <patrick.georgi at secunet.com>
>> CC: Andres Salomon <dilinger at queued.net>
>>
>> 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 <philipp at redfish-solutions.com>
>> Acked-by: Patrick Georgi <patrick.georgi at secunet.com>
>> ---
>> 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:
> + * 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.

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

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:
> + * 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

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

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

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.



-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: libpayload.patch
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20110821/5f0e008d/attachment.ksh>


More information about the coreboot mailing list