Re: [PATCH 3/8] SFI: core support

[Ingo Molnar <mingo@elte.hu>]

> --- /dev/null
> +++ b/arch/x86/kernel/sfi.c

> +#include <linux/init.h>
> +#include <linux/efi.h>
> +#include <linux/cpumask.h>
> +#include <linux/module.h>
> +#include <linux/irq.h>
> +#include <linux/bootmem.h>
> +#include <linux/io.h>
> +#include <linux/acpi.h>
> +#include <linux/efi.h>
> +#include <linux/sfi.h>
> +
> +#include <asm/pgtable.h>
> +#include <asm/io_apic.h>
> +#include <asm/apic.h>
> +#include <asm/mpspec.h>
> +
> +#include <asm/e820.h>
> +#include <asm/setup.h>

Please try to use the include files style in arch/x86/mm/fault.c. It 
is minimalistic and deterministically ordered as well, so it will 
look in a pleasant way long-term too .

> +#undef PREFIX

Why undefine it? No header should set 'PREFIX' - if it does it needs 
fixing.

> +#define PREFIX "SFI: "
>
> +#ifdef CONFIG_X86_LOCAL_APIC
> +static u64 sfi_lapic_addr __initdata = APIC_DEFAULT_PHYS_BASE;
> +#endif

if SFI adds a 'depends on X86_LOCAL_APIC' the ugly #ifdef can be 
dropped.

> +
> +extern int __init sfi_parse_xsdt(struct sfi_table_header *table);
> +static int __init sfi_parse_cpus(struct sfi_table_header *table);
> +static int __init sfi_parse_apic(struct sfi_table_header *table);

These should be in a header i guess.

> +
> +void __init __iomem *
> +arch_early_ioremap(unsigned long phys, unsigned long size)
> +{
> +	return early_ioremap(phys, size);
> +}
> +
> +void __init arch_early_iounmap(void __iomem *virt, unsigned long size)
> +{
> +	early_iounmap(virt, size);
> +}

Should be in a separate series and not added to sfi.c but to core 
x86.

> +
> +static ulong __init sfi_early_find_syst(void)

Please use C types such as 'unsigned long'.

> +{
> +	unsigned long i;

... like here.

> +	char *pchar = (char *)SFI_SYST_SEARCH_BEGIN;
> +
> +	for (i = 0; SFI_SYST_SEARCH_BEGIN + i < SFI_SYST_SEARCH_END; i += 16, pchar += 16) {

What does the magic constant '16' mean here?

> +		if (!strncmp(SFI_SIG_SYST, pchar, SFI_SIGNATURE_SIZE))
> +			return SFI_SYST_SEARCH_BEGIN + i;
> +	}
> +	return 0;
> +}
> +
> +/*
> + * called in a early boot phase before the paging table is created,
> + * setup a mmap table in e820 format
> + */
> +int __init sfi_init_memory_map(void)
> +{
> +	struct sfi_table_simple *syst, *mmapt;
> +	struct sfi_mem_entry *mentry;
> +	unsigned long long start, end, size;
> +	int i, num, type, tbl_cnt;
> +	u64 *pentry;
> +
> +	if (sfi_disabled)
> +		return -1;
> +
> +	/* first search the syst table */
> +	syst = (struct sfi_table_simple *)sfi_early_find_syst();

why doesnt sfi_early_find_syst() return the proper type?

> +	if (!syst)
> +		return -1;
> +
> +	tbl_cnt = (syst->header.length - sizeof(struct sfi_table_header)) / sizeof(u64);
> +	pentry = syst->pentry;
> +
> +	/* walk through the syst to search the mmap table */
> +	mmapt = NULL;
> +	for (i = 0; i < tbl_cnt; i++) {
> +		if (!strncmp(SFI_SIG_MMAP, (char *)(u32)*pentry, 4)) {
> +			mmapt = (struct sfi_table_simple *)(u32)*pentry;
> +			break;
> +		}
> +		pentry++;
> +	}
> +	if (!mmapt)
> +		return -1;
> +
> +	/* refer copy_e820_memory() */
> +	num = SFI_GET_ENTRY_NUM(mmapt, sfi_mem_entry);
> +	mentry = (struct sfi_mem_entry *)mmapt->pentry;
> +	for (i = 0; i < num; i++) {
> +		start = mentry->phy_start;
> +		size = mentry->pages << PAGE_SHIFT;
> +		end = start + size;
> +
> +		if (start > end)
> +			return -1;
> +
> +		pr_debug(PREFIX "start = 0x%08x end = 0x%08x type = %d\n",
> +			(u32)start, (u32)end, mentry->type);
> +
> +		/* translate SFI mmap type to E820 map type */
> +		switch (mentry->type) {
> +		case EFI_CONVENTIONAL_MEMORY:
> +			type = E820_RAM;
> +			break;
> +		case EFI_MEMORY_MAPPED_IO:
> +		case EFI_UNUSABLE_MEMORY:
> +		case EFI_RUNTIME_SERVICES_DATA:
> +			mentry++;
> +			continue;
> +		default:
> +			type = E820_RESERVED;
> +		}
> +
> +		e820_add_region(start, size, type);
> +		mentry++;
> +	}
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_X86_LOCAL_APIC
> +void __init mp_sfi_register_lapic_address(u64 address)
> +{
> +	mp_lapic_addr = (unsigned long) address;

mp_lapic_addr should have the proper type i guess.

> +
> +	set_fixmap_nocache(FIX_APIC_BASE, mp_lapic_addr);
> +
> +	if (boot_cpu_physical_apicid == -1U)
> +		boot_cpu_physical_apicid = read_apic_id();
> +
> +	pr_info(PREFIX "Boot CPU = %d\n", boot_cpu_physical_apicid);

Should be pr_debug() i guess - we know the boot CPU from a number of 
other places already.

> +}
> +
> +/* All CPUs enumerated by SFI must be present and enabled */
> +void __cpuinit mp_sfi_register_lapic(u8 id)
> +{
> +	struct mpc_cpu cpu;
> +	int boot_cpu = 0;
> +
> +	if (MAX_APICS - id <= 0) {
> +		printk(KERN_WARNING "Processor #%d invalid (max %d)\n",
> +			id, MAX_APICS);
> +		return;
> +	}
> +
> +	if (id == boot_cpu_physical_apicid)
> +		boot_cpu = 1;
> +
> +	cpu.type = MP_PROCESSOR;
> +	cpu.apicid = id;
> +	cpu.apicver = GET_APIC_VERSION(apic_read(APIC_LVR));
> +	cpu.cpuflag = CPU_ENABLED;
> +	cpu.cpuflag |= (boot_cpu ? CPU_BOOTPROCESSOR : 0);
> +	cpu.cpufeature = (boot_cpu_data.x86 << 8) |
> +		(boot_cpu_data.x86_model << 4) | boot_cpu_data.x86_mask;
> +	cpu.featureflag = boot_cpu_data.x86_capability[0];
> +	cpu.reserved[0] = 0;
> +	cpu.reserved[1] = 0;

Nice trick - MP-spec is still useful after all.

> +
> +	generic_processor_info(id, cpu.apicver);
> +}
> +
> +static int __init sfi_parse_cpus(struct sfi_table_header *table)
> +{
> +	struct sfi_table_simple *sb;
> +	struct sfi_cpu_table_entry *pentry;
> +	int i;
> +	int cpu_num;
> +
> +	BUG_ON(!table);

Can this happen? If yes, is a boot crash the best answer?

> +	sb = (struct sfi_table_simple *)table;
> +
> +	cpu_num = SFI_GET_ENTRY_NUM(sb, sfi_cpu_table_entry);
> +	pentry = (struct sfi_cpu_table_entry *) sb->pentry;

(unneeded space character)

> +
> +	for (i = 0; i < cpu_num; i++) {
> +		mp_sfi_register_lapic(pentry->apicid);
> +		pentry++;
> +	}
> +
> +	smp_found_config = 1;
> +	return 0;
> +}
> +#endif /* CONFIG_X86_LOCAL_APIC */
> +
> +#ifdef	CONFIG_X86_IO_APIC

Should SFI add 'depends on X86_IO_APIC' perhaps? (or do you 
anticipate IO-APIC-less implementations?)


> +static struct mp_ioapic_routing {
> +	int			apic_id;
> +	int			gsi_base;
> +	int			gsi_end;
> +	u32			pin_programmed[4];
> +} mp_ioapic_routing[MAX_IO_APICS];

Data structures should be defined at the top of the .c file, not 
hidden in the middle.

> +
> +/* refer acpi/boot.c */
> +static u8 __init uniq_ioapic_id(u8 id)
> +{
> +#ifdef CONFIG_X86_32
> +	if ((boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) &&
> +	    !APIC_XAPIC(apic_version[boot_cpu_physical_apicid]))
> +		return io_apic_get_unique_id(nr_ioapics, id);
> +	else
> +		return id;
> +#else
> +	int i;
> +	DECLARE_BITMAP(used, 256);
> +	bitmap_zero(used, 256);

Newline needed after local variabe definition blocks.

> +	for (i = 0; i < nr_ioapics; i++) {
> +		struct mpc_ioapic *ia = &mp_ioapics[i];
> +		__set_bit(ia->apicid, used);
> +	}
> +	if (!test_bit(id, used))
> +		return id;
> +	return find_first_zero_bit(used, 256);
> +#endif
> +}

Why is the 32-bit and 64-bit version so different? Please unify 
first before adding new functionality.

> +
> +
> +void __init mp_sfi_register_ioapic(u8 id, u32 paddr)
> +{
> +	int idx = 0;
> +	int tmpid;
> +	static u32 gsi_base;
> +
> +	if (nr_ioapics >= MAX_IO_APICS) {
> +		printk(KERN_ERR "ERROR: Max # of I/O APICs (%d) exceeded "
> +			"(found %d)\n", MAX_IO_APICS, nr_ioapics);
> +		panic("Recompile kernel with bigger MAX_IO_APICS!\n");
> +	}
> +	if (!paddr) {
> +		printk(KERN_ERR "WARNING: Bogus (zero) I/O APIC address"
> +			" found in MADT table, skipping!\n");
> +		return;
> +	}
> +
> +	idx = nr_ioapics;
> +
> +	mp_ioapics[idx].type = MP_IOAPIC;
> +	mp_ioapics[idx].flags = MPC_APIC_USABLE;
> +	mp_ioapics[idx].apicaddr = paddr;
> +
> +	set_fixmap_nocache(FIX_IO_APIC_BASE_0 + idx, paddr);
> +	tmpid = uniq_ioapic_id(id);
> +	if (tmpid == -1)
> +		return;
> +
> +	mp_ioapics[idx].apicid = tmpid;
> +#ifdef CONFIG_X86_32
> +	mp_ioapics[idx].apicver = io_apic_get_version(idx);
> +#else
> +	mp_ioapics[idx].apicver = 0;
> +#endif

Could this #ifdef be eliminated?

> +
> +	pr_info(PREFIX "IOAPIC[%d]: apic_id %d, version %d, address 0x%x\n",
> +		idx, mp_ioapics[idx].apicid,
> +		mp_ioapics[idx].apicver, (u32)mp_ioapics[idx].apicaddr);
> +	/*
> +	 * Build basic GSI lookup table to facilitate gsi->io_apic lookups
> +	 * and to prevent reprogramming of IOAPIC pins (PCI GSIs).
> +	 */
> +	mp_ioapic_routing[idx].apic_id = mp_ioapics[idx].apicid;
> +	mp_ioapic_routing[idx].gsi_base = gsi_base;
> +	mp_ioapic_routing[idx].gsi_end = gsi_base +
> +		io_apic_get_redir_entries(idx);
> +	gsi_base = mp_ioapic_routing[idx].gsi_end + 1;
> +	pr_info(PREFIX "IOAPIC[%d]: apic_id %d, version %d, address 0x%x, "
> +	       "GSI %d-%d\n", idx, mp_ioapics[idx].apicid,
> +	       mp_ioapics[idx].apicver, (u32)mp_ioapics[idx].apicaddr,
> +	       mp_ioapic_routing[idx].gsi_base,
> +	       mp_ioapic_routing[idx].gsi_end);
> +
> +	nr_ioapics++;
> +}
> +
> +static int __init sfi_parse_apic(struct sfi_table_header *table)
> +{
> +	struct sfi_table_simple *sb;
> +	struct sfi_apic_table_entry *pentry;
> +	int i, num;
> +
> +	BUG_ON(!table);

Same as comment above - is this case anticipated? If yes, is a crash 
the best answer?

> +	sb = (struct sfi_table_simple *)table;
> +
> +	num = SFI_GET_ENTRY_NUM(sb, sfi_apic_table_entry);
> +	pentry = (struct sfi_apic_table_entry *) sb->pentry;
> +	for (i = 0; i < num; i++) {
> +		mp_sfi_register_ioapic(i, pentry->phy_addr);
> +		pentry++;
> +	}
> +
> +	WARN_ON(pic_mode);
> +	pic_mode = 0;

If this warning can occor, please use WARN() instead with a 
user-parseable message.

> +	return 0;
> +}
> +#endif /* CONFIG_X86_IO_APIC */
> +
> +/*
> + * sfi_platform_init(): register lapics & io-apics
> + */
> +int __init sfi_platform_init(void)
> +{
> +#ifdef CONFIG_X86_LOCAL_APIC
> +	mp_sfi_register_lapic_address(sfi_lapic_addr);
> +	sfi_table_parse(SFI_SIG_CPUS, NULL, NULL, 0, sfi_parse_cpus);
> +#endif
> +#ifdef CONFIG_X86_IO_APIC
> +	sfi_table_parse(SFI_SIG_APIC, NULL, NULL, 0, sfi_parse_apic);
> +#endif

Both of these #ifdefs would go away with the 'depends on' 
suggestions i made above.

Also, sfi_parse_apic() should be named sfr_parse_ioapic() ?

> +	return 0;
> +}
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 1266ead..c3e39b5 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_PARISC)		+= parisc/
>  obj-$(CONFIG_RAPIDIO)		+= rapidio/
>  obj-y				+= video/
>  obj-$(CONFIG_ACPI)		+= acpi/
> +obj-$(CONFIG_SFI)		+= sfi/
>  # PnP must come after ACPI since it will eventually need to check if acpi
>  # was used and do nothing if so
>  obj-$(CONFIG_PNP)		+= pnp/
> diff --git a/drivers/sfi/Makefile b/drivers/sfi/Makefile
> new file mode 100644
> index 0000000..f176470
> --- /dev/null
> +++ b/drivers/sfi/Makefile
> @@ -0,0 +1 @@
> +obj-y	+= sfi_core.o
> diff --git a/drivers/sfi/sfi_core.c b/drivers/sfi/sfi_core.c
> new file mode 100644
> index 0000000..0a9b72d
> --- /dev/null
> +++ b/drivers/sfi/sfi_core.c
> @@ -0,0 +1,403 @@
> +/* sfi_core.c Simple Firmware Interface - core internals */
> +
> +/*
> + * Copyright (C) 2009, Intel Corp.
> + * All rights reserved.
> + *
> + * 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,
> + *    without modification.
> + * 2. Redistributions in binary form must reproduce at minimum a disclaimer
> + *    substantially similar to the "NO WARRANTY" disclaimer below
> + *    ("Disclaimer") and any redistribution must be conditioned upon
> + *    including a substantially similar Disclaimer requirement for further
> + *    binary redistribution.
> + * 3. Neither the names of the above-listed copyright holders nor the names
> + *    of any contributors may be used to endorse or promote products derived
> + *    from this software without specific prior written permission.
> + *
> + * Alternatively, this software may be distributed under the terms of the
> + * GNU General Public License ("GPL") version 2 as published by the Free
> + * Software Foundation.
> + *
> + * NO WARRANTY
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * HOLDERS OR CONTRIBUTORS BE LIABLE FOR 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 DAMAGES.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/smp.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/irq.h>
> +#include <linux/errno.h>
> +#include <linux/sfi.h>
> +#include <linux/bootmem.h>
> +#include <linux/module.h>
> +#include <asm/pgtable.h>
> +#include "sfi_core.h"
> +
> +#undef PREFIX
> +#define PREFIX	"SFI: "
> +
> +int sfi_disabled;

Should be __read_mostly? (other read-mostly variables below too i 
guess)

> +EXPORT_SYMBOL(sfi_disabled);
> +
> +#define SFI_MAX_TABLES		64

Kernels we release will live 5-10 years easily - new hw is never 
expected to have more than 64 tables?

> +struct sfi_internal_syst sfi_tblist;
> +static struct sfi_table_desc sfi_initial_tables[SFI_MAX_TABLES] __initdata;
> +
> +/*
> + * flag for whether using ioremap() to map the sfi tables, if yes
> + * each table only need be mapped once, otherwise each arch's
> + * early_ioremap  and early_iounmap should be used each time a
> + * table is visited
> + */
> +static u32 sfi_tbl_permanant_mapped;
> +
> +static void __iomem *sfi_map_memory(u32 phys, u32 size)
> +{
> +	if (!phys || !size)
> +		return NULL;
> +
> +	if (sfi_tbl_permanant_mapped)
> +		return ioremap((unsigned long)phys, size);
> +	else
> +		return arch_early_ioremap((unsigned long)phys, size);
> +}
> +
> +static void sfi_unmap_memory(void __iomem *virt, u32 size)
> +{
> +	if (!virt || !size)
> +		return;
> +
> +	if (sfi_tbl_permanant_mapped)
> +		iounmap(virt);
> +	else
> +		arch_early_iounmap(virt, size);
> +}
> +

That's a disgusting facility. Should be separated out ...

> +static void sfi_print_table_header(u32 address,
> +			struct sfi_table_header *header)
> +{
> +	pr_info(PREFIX
> +		"%4.4s %08lX, %04X (r%d %6.6s %8.8s)\n",
> +		header->signature, (unsigned long)address,
> +		header->length, header->revision, header->oem_id,
> +		header->oem_table_id);
> +}

Why do we need the type cast above?

> +
> +static u8 sfi_checksum_table(u8 *buffer, u32 length)
> +{
> +	u8 sum = 0;
> +
> +	while (length--)
> +		sum += *buffer++;
> +	return sum;
> +}
> +
> +/* Verifies if the table checksums is zero */
> +static int sfi_tb_verify_checksum(struct sfi_table_header *table, u32 length)
> +{
> +	u8 checksum;
> +
> +	checksum = sfi_checksum_table((u8 *)table, length);

Why not declare sfi_checksum_table() with a void * input type 
instead and lose the cast above? That way a potential source of bugs 
gets found. (say accidentally passing in an 'address' to the 
function instead of a table pointer)

> +	if (checksum) {
> +		pr_warning(PREFIX
> +			"Incorrect checksum in table [%4.4s] -  %2.2X,"
> +			" should be %2.2X\n", table->signature,
> +			table->checksum, (u8)(table->checksum - checksum));
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> + /* find the right table based on signaure, return the mapped table */
> +int sfi_get_table(char *signature, char *oem_id, char *oem_table_id,
> +	unsigned int flags, struct sfi_table_header **out_table)
> +{
> +	struct sfi_table_desc *tdesc;
> +	struct sfi_table_header *th;
> +	u32 i;
> +
> +	if (!signature || !out_table)
> +		return -1;
> +
> +	/* Walk the global SFI table list */
> +	for (i = 0; i < sfi_tblist.count; i++) {
> +		tdesc = &sfi_tblist.tables[i];
> +		th = &tdesc->header;
> +
> +		if ((flags & SFI_ACPI_TABLE) != (tdesc->flags & SFI_ACPI_TABLE))
> +			continue;
> +
> +		if (strncmp(th->signature, signature, SFI_SIGNATURE_SIZE))
> +			continue;
> +
> +		if (oem_id && strncmp(th->oem_id, oem_id, SFI_OEM_ID_SIZE))
> +			continue;
> +
> +		if (oem_table_id && strncmp(th->oem_table_id, oem_table_id,
> +			SFI_OEM_TABLE_ID_SIZE))
> +			continue;
> +
> +		if (!tdesc->pointer) {
> +			tdesc->pointer = sfi_map_memory(tdesc->address,
> +							th->length);
> +			if (!tdesc->pointer)
> +				return -ENOMEM;
> +		}
> +		*out_table = tdesc->pointer;
> +
> +		if (!sfi_tbl_permanant_mapped)
> +			tdesc->pointer = NULL;
> +
> +		return 0;
> +	}
> +
> +	return -1;
> +}
> +
> +void sfi_put_table(struct sfi_table_header *table)
> +{
> +	if (!sfi_tbl_permanant_mapped)
> +		sfi_unmap_memory(table, table->length);
> +}
> +
> +/* find table with signature, run handler on it */
> +int sfi_table_parse(char *signature, char *oem_id, char* oem_table_id,
> +	unsigned int flags, sfi_table_handler handler)
> +{
> +	int ret = 0;
> +	struct sfi_table_header *table = NULL;
> +
> +	if (!handler)
> +		return -EINVAL;
> +
> +	sfi_get_table(signature, oem_id, oem_table_id, flags, &table);
> +	if (!table)
> +		return -1;

the error return is a bit unclean here. First we use -EINVAL, then 
-1 - which is -EPERM which doesnt make sense. I'd suggest to return 
-EINVAL or -ENOTFOUND instead of -1 - not mix the return codes like 
that.

> +
> +	ret = handler(table);
> +	sfi_put_table(table);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(sfi_table_parse);
> +
> +void sfi_tb_install_table(u64 addr, u32 flags)
> +{
> +	struct sfi_table_header *table;
> +	u32 length;
> +
> +	/* only map table header before knowing actual length */
> +	table = sfi_map_memory(addr, sizeof(struct sfi_table_header));
> +	if (!table)
> +		return;
> +
> +	length = table->length;
> +	sfi_unmap_memory(table, sizeof(struct sfi_table_header));
> +
> +	table = sfi_map_memory(addr, length);
> +	if (!table)
> +		return;
> +
> +	if (sfi_tb_verify_checksum(table, length))
> +		goto unmap_and_exit;
> +
> +	/* Initialize sfi_tblist entry */
> +	sfi_tblist.tables[sfi_tblist.count].flags = flags;
> +	sfi_tblist.tables[sfi_tblist.count].address = addr;
> +	sfi_tblist.tables[sfi_tblist.count].pointer = NULL;
> +	memcpy(&sfi_tblist.tables[sfi_tblist.count].header,
> +		table, sizeof(struct sfi_table_header));
> +
> +	sfi_print_table_header(addr, table);
> +	sfi_tblist.count++;
> +
> +unmap_and_exit:
> +	sfi_unmap_memory(table, length);
> +	return;
> +}
> +
> +/*
> + * Copy system table and associated table headers to internal format
> + */
> +static int __init
> +sfi_tb_parse_syst(unsigned long syst_addr)
> +{
> +	struct sfi_table_simple *syst;
> +	struct sfi_table_header *table;
> +	u64 *paddr;
> +	u32 length, tbl_cnt;
> +	int status;
> +	int i;
> +
> +	/* 1. map and get the total length of SYST */
> +	syst = sfi_map_memory(syst_addr, sizeof(struct sfi_table_simple));
> +	if (!syst)
> +		return -ENOMEM;
> +
> +	sfi_print_table_header(syst_addr, (struct sfi_table_header *)syst);
> +	table = (struct sfi_table_header *)syst;
> +	length = table->length;
> +	sfi_unmap_memory(syst, sizeof(struct sfi_table_simple));
> +
> +	/* 2. remap/verify the SYST and parse the number of entry */
> +	syst = sfi_map_memory(syst_addr, length);
> +	if (!syst)
> +		return -ENOMEM;
> +
> +	table = (struct sfi_table_header *)syst;
> +	status = sfi_tb_verify_checksum(table, length);
> +	if (status) {
> +		pr_err(PREFIX "SYST checksum error!!\n");
> +		sfi_unmap_memory(table, length);
> +		return status;
> +	}
> +
> +	/* Calculate the number of tables */
> +	tbl_cnt = (length - sizeof(struct sfi_table_header)) / sizeof(u64);
> +	paddr = (u64 *) syst->pentry;
> +
> +	sfi_tblist.count = 1;
> +	sfi_tblist.tables[0].address = syst_addr;
> +	sfi_tblist.tables[0].pointer = NULL;
> +	memcpy(&sfi_tblist.tables[0].header,
> +		syst, sizeof(struct sfi_table_header));
> +
> +	/* 3. save all tables info to the global sfi_tblist structure */
> +	for (i = 1; i <= tbl_cnt; i++)
> +		sfi_tb_install_table(*paddr++, 0);
> +
> +	sfi_unmap_memory(syst, length);
> +
> +	return 0;
> +}
> +
> +
> +/*
> + * The OS finds the System Table by searching 16-byte boundaries between physical
> + * address 0x000E0000 and 0x000FFFFF. The OS shall search this region starting at the
> + * low address and shall stop searching when the 1st valid SFI System Table is found.

(Way-) overlong lines here.

> + */
> +static __init unsigned long sfi_find_syst(void)
> +{
> +	unsigned long offset, len;
> +	void *start;
> +
> +	len = SFI_SYST_SEARCH_END - SFI_SYST_SEARCH_BEGIN;
> +	start = sfi_map_memory(SFI_SYST_SEARCH_BEGIN, len);
> +	if (!start)
> +		return 0;
> +
> +	for (offset = 0; offset < len; offset += 16) {
> +		struct sfi_table_header *syst;
> +
> +		syst = (struct sfi_table_header *)(start + offset);

No need for the cast here - void pointers are unitype.

> +
> +		if (strncmp(syst->signature, SFI_SIG_SYST, SFI_SIGNATURE_SIZE))
> +				continue;

(one too many tabs)

> +
> +		if (!sfi_tb_verify_checksum(syst, syst->length)) {
> +			sfi_unmap_memory(start, len);
> +			return SFI_SYST_SEARCH_BEGIN + offset;
> +		}
> +	}
> +
> +	sfi_unmap_memory(start, len);
> +	return 0;
> +}
> +
> +int __init sfi_table_init(void)
> +{
> +	unsigned long syst_paddr;
> +	int status;
> +
> +	/* set up the SFI table array */
> +	sfi_tblist.tables = sfi_initial_tables;
> +	sfi_tblist.size = SFI_MAX_TABLES;
> +
> +	syst_paddr = sfi_find_syst();
> +	if (!syst_paddr) {
> +		pr_warning(PREFIX "No system table\n");
> +		goto err_exit;
> +	}
> +
> +	status = sfi_tb_parse_syst(syst_paddr);
> +	if (status)
> +		goto err_exit;
> +
> +	return 0;
> +err_exit:
> +	disable_sfi();
> +	return -1;
> +}
> +
> +static void sfi_realloc_tblist(void)
> +{
> +	int size;
> +	struct sfi_table_desc *table;
> +
> +	size = (sfi_tblist.count + 8) * sizeof(struct sfi_table_desc);

magic, unexplained 8.

> +	table = kzalloc(size, GFP_KERNEL);
> +	if (!table) {
> +		disable_sfi();
> +		return;
> +	}
> +
> +	memcpy(table, sfi_tblist.tables,
> +		sfi_tblist.count * sizeof(struct sfi_table_desc));
> +	sfi_tblist.tables = table;
> +	return;
> +}
> +
> +int __init sfi_init(void)
> +{
> +	if(!acpi_disabled){
> +		disable_sfi();
> +		return -1;
> +	}
> +
> +	if(sfi_disabled)
> +		return -1;

Coding style problems.

> +
> +	pr_info(PREFIX "Simple Firmware Interface v0.5\n");
> +
> +	if (sfi_table_init())
> +		return -1;
> +
> +	return sfi_platform_init();
> +}
> +/* after most of the system is up, abandon the static array */
> +void __init sfi_init_late(void)
> +{
> +	if (sfi_disabled)
> +		return;
> +	sfi_tbl_permanant_mapped = 1;
> +	sfi_realloc_tblist();
> +}
> +
> +static int __init sfi_parse_cmdline(char *arg)
> +{
> +	if (!arg)
> +		return -EINVAL;
> +
> +	if (!strcmp(arg, "off"))
> +		sfi_disabled = 1;
> +
> +	return 0;
> +}
> +early_param("sfi", sfi_parse_cmdline);
> diff --git a/drivers/sfi/sfi_core.h b/drivers/sfi/sfi_core.h
> new file mode 100644
> index 0000000..36703b0
> --- /dev/null
> +++ b/drivers/sfi/sfi_core.h
> @@ -0,0 +1,63 @@
> +/* sfi_core.h Simple Firmware Interface, internal header */
> +
> +/*
> + * Copyright (C) 2009, Intel Corp.
> + * All rights reserved.
> + *
> + * 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,
> + *    without modification.
> + * 2. Redistributions in binary form must reproduce at minimum a disclaimer
> + *    substantially similar to the "NO WARRANTY" disclaimer below
> + *    ("Disclaimer") and any redistribution must be conditioned upon
> + *    including a substantially similar Disclaimer requirement for further
> + *    binary redistribution.
> + * 3. Neither the names of the above-listed copyright holders nor the names
> + *    of any contributors may be used to endorse or promote products derived
> + *    from this software without specific prior written permission.
> + *
> + * Alternatively, this software may be distributed under the terms of the
> + * GNU General Public License ("GPL") version 2 as published by the Free
> + * Software Foundation.
> + *
> + * NO WARRANTY
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * HOLDERS OR CONTRIBUTORS BE LIABLE FOR 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 DAMAGES.
> + */
> +
> +struct sfi_table_desc {
> +	struct sfi_table_header header;	/* copy of the headef info */

s/headef/header

> +	struct sfi_table_header *pointer;
> +	u64 address;
> +	u32 flags;
> +};
> +
> +/* SFI internal root SYSTem table */
> +struct sfi_internal_syst {
> +	struct sfi_table_desc *tables;
> +	u32 count;
> +	u32 size;
> +};

i'd suggest using the structure definition style you use in 
arch/x86/kernel/sfi.c:

> +struct sfi_table_desc {
> +	struct sfi_table_header		header;	/* copy of the headef info */
> +	struct sfi_table_header		*pointer;
> +	u64				address;
> +	u32				flags;
> +};


> +
> +extern int sfi_get_table(char *signature, char *oem_id, char *oem_table_id,
> +	uint flags, struct sfi_table_header **out_table);
> +extern void sfi_put_table(struct sfi_table_header *table);
> +
> +extern int sfi_acpi_init(void);
> +extern struct sfi_internal_syst sfi_tblist;
> +void sfi_tb_install_table(u64 address, u32 flags);
> +
> +#define SFI_ACPI_TABLE	1

In general, nice stuff - basically SFI is cleanly implemented ACPI 
tables without any of the run-code-in-acpi-tables complications, 
right?

	Ingo