[RFC/PATCH 2.6.32] Simple Firmware Interface (SFI): initial support

[RFC/PATCH 2.6.32] Simple Firmware Interface (SFI): initial support

[Len Brown]

This patch series contains the initial SFI support for Linux.

You can read about SFI on its home page: http://simplefirmware.org

The latest SFI test patch is available in git:
git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux-sfi-2.6.git sfi-test

A consolidated plain patch is available here:
http://ftp.kernel.org/pub/linux/kernel/people/lenb/sfi/patches/2.6.30/-2.6.30.diff.gz

Diffstat for this series:

 Documentation/kernel-parameters.txt |    5 +
 MAINTAINERS                         |   12 +
 arch/x86/Kconfig                    |    4 +-
 arch/x86/include/asm/io_apic.h      |    4 +-
 arch/x86/kernel/Makefile            |    1 +
 arch/x86/kernel/apic/io_apic.c      |    4 +-
 arch/x86/kernel/e820.c              |    5 +
 arch/x86/kernel/setup.c             |    3 +
 arch/x86/kernel/sfi.c               |  335 +++++++++++++++++++++++++++++
 arch/x86/pci/mmconfig-shared.c      |    4 +-
 drivers/Makefile                    |    1 +
 drivers/acpi/tables.c               |    3 +
 drivers/sfi/Kconfig                 |   16 ++
 drivers/sfi/Makefile                |    3 +
 drivers/sfi/sfi_acpi.c              |   94 ++++++++
 drivers/sfi/sfi_core.c              |  403 +++++++++++++++++++++++++++++++++++
 drivers/sfi/sfi_core.h              |   63 ++++++
 include/acpi/acpi_drivers.h         |    3 +
 include/linux/acpi.h                |    5 +-
 include/linux/sfi.h                 |  161 ++++++++++++++
 include/linux/sfi_acpi.h            |   56 +++++
 init/main.c                         |    2 +
 22 files changed, 1179 insertions(+), 8 deletions(-)
 create mode 100644 arch/x86/kernel/sfi.c
 create mode 100644 drivers/sfi/Kconfig
 create mode 100644 drivers/sfi/Makefile
 create mode 100644 drivers/sfi/sfi_acpi.c
 create mode 100644 drivers/sfi/sfi_core.c
 create mode 100644 drivers/sfi/sfi_core.h
 create mode 100644 include/linux/sfi.h
 create mode 100644 include/linux/sfi_acpi.h

commits:

Feng Tang (7):
      SFI: include/linux/sfi.h
      SFI: core support
      SFI: Hook boot-time initialization
      SFI: Hook e820 memory map initialization
      SFI: add ACPI extensions
      SFI, PCI: Hook MMCONFIG
      SFI: expose IO-APIC routines to SFI, not just ACPI

Len Brown (1):
      SFI: Simple Firmware Interface - new Linux sub-system

[PATCH 1/8] SFI: Simple Firmware Interface - new Linux sub-system

[Len Brown]

From: Len Brown <len.brown@intel.com>

CONFIG_SFI=y enables the kernel to boot and run optimally
on platforms that support the Simple Firmware Interface.

Thanks to Jacob Pan for prototyping the initial Linux SFI support,
and to Feng Tang for Linux bring-up and debug in emulation
and on hardware.

See http://simplefirmware.org for more information on SFI.

Signed-off-by: Feng Tang <feng.tang@intel.com>
Signed-off-by: Len Brown <len.brown@intel.com>
---
 Documentation/kernel-parameters.txt |    5 +++++
 MAINTAINERS                         |   12 ++++++++++++
 arch/x86/Kconfig                    |    2 ++
 drivers/sfi/Kconfig                 |   16 ++++++++++++++++
 4 files changed, 35 insertions(+), 0 deletions(-)
 create mode 100644 drivers/sfi/Kconfig

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index fd5cac0..a4797d6 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -92,6 +92,7 @@ parameter is applicable:
 	SECURITY Different security models are enabled.
 	SELINUX SELinux support is enabled.
 	SERIAL	Serial support is enabled.
+	SFI	Simple Firmware Interface
 	SH	SuperH architecture is enabled.
 	SMP	The kernel is an SMP kernel.
 	SPARC	Sparc architecture is enabled.
@@ -2082,6 +2083,10 @@ and is between 256 and 4096 characters. It is defined in the file
 			If enabled at boot time, /selinux/disable can be used
 			later to disable prior to initial policy load.
 
+	sfi=		[SFI,X86] Simple Firmware Interface
+			Format: { "off" }
+			off -- disable SFI
+
 	serialnumber	[BUGS=X86-32]
 
 	shapers=	[NET]
diff --git a/MAINTAINERS b/MAINTAINERS
index cf4abdd..1939b4c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5105,6 +5105,18 @@ L:	linux-pci@vger.kernel.org
 S:	Supported
 F:	drivers/pci/hotplug/shpchp*
 
+SIMPLE FIRMWARE INTERFACE (SFI)
+P:	Len Brown
+M:	lenb@kernel.org
+L:	sfi-devel@simplefirmware.org
+W:	http://simplefirmware.org/
+T:	git git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux-sfi-2.6.git
+S:	Supported
+F:	arch/x86/kernel/*sfi*
+F:	drivers/sfi/
+F:	include/linux/sfi*.h
+
+
 SIMTEC EB110ATX (Chalice CATS)
 P:	Ben Dooks
 P:	Vincent Sanders
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index a6efe0a..06341a5 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1641,6 +1641,8 @@ source "kernel/power/Kconfig"
 
 source "drivers/acpi/Kconfig"
 
+source "drivers/sfi/Kconfig"
+
 config X86_APM_BOOT
 	bool
 	default y
diff --git a/drivers/sfi/Kconfig b/drivers/sfi/Kconfig
new file mode 100644
index 0000000..b217f32
--- /dev/null
+++ b/drivers/sfi/Kconfig
@@ -0,0 +1,16 @@
+#
+# SFI Configuration
+#
+
+menuconfig SFI
+	bool "SFI (Simple Firmware Interface) Support"
+	depends on X86
+	default n
+	---help---
+	The Simple Firmware Interface (SFI) provides a lightweight method
+	for platform firmware to pass information to the Operating System
+	via static tables in memory.
+
+	For more information, see http://simplefirmware.org
+
+	Say 'Y' here to enable the kernel to boot on SFI-only platforms.
-- 
1.6.0.6

[PATCH 2/8] SFI: include/linux/sfi.h

[Len Brown]

From: Feng Tang <feng.tang@intel.com>

linux/include/sfi.h defines everything that customers
of SFI need to know in order to use the SFI suport in  the kernel.

The primary API is sfi_table_parse(), where a driver or another part
of the kernel can supply a handler to parse the named table.

Signed-off-by: Feng Tang <feng.tang@intel.com>
Signed-off-by: Len Brown <len.brown@intel.com>
---
 include/linux/sfi.h |  161 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 161 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/sfi.h

diff --git a/include/linux/sfi.h b/include/linux/sfi.h
new file mode 100644
index 0000000..2164fc1
--- /dev/null
+++ b/include/linux/sfi.h
@@ -0,0 +1,161 @@
+/* sfi.h Simple Firmware Interface */
+
+/*
+ * 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.
+ */
+
+#ifndef _LINUX_SFI_H
+#define _LINUX_SFI_H
+
+#ifdef	CONFIG_SFI
+
+#define SFI_SYST_SEARCH_BEGIN		0x000E0000
+#define SFI_SYST_SEARCH_END		0x000FFFFF
+
+/* Table signatures reserved by the SFI specification */
+#define SFI_SIG_SYST		"SYST"
+#define SFI_SIG_FREQ		"FREQ"
+#define SFI_SIG_IDLE		"IDLE"
+#define SFI_SIG_CPUS		"CPUS"
+#define SFI_SIG_MTMR		"MTMR"
+#define SFI_SIG_MRTC		"MRTC"
+#define SFI_SIG_MMAP		"MMAP"
+#define SFI_SIG_APIC		"APIC"
+#define SFI_SIG_XSDT		"XSDT"	/* ACPI Extended System Description Table */
+#define SFI_SIG_WAKE		"WAKE"
+
+#define SFI_SIGNATURE_SIZE	4
+#define SFI_OEM_ID_SIZE		6
+#define SFI_OEM_TABLE_ID_SIZE	8
+
+#define SFI_GET_ENTRY_NUM(ptable, entry) \
+	((ptable->header.length - sizeof(struct sfi_table_header)) / \
+	(sizeof(struct entry)))
+/*
+ * Table structures must be byte-packed to match the SFI specification,
+ * as they are provided by the BIOS.
+ */
+#pragma pack(1)
+struct sfi_table_header {
+	char signature[SFI_SIGNATURE_SIZE];
+	u32 length;
+	u8 revision;
+	u8 checksum;
+	char oem_id[SFI_OEM_ID_SIZE];
+	char oem_table_id[SFI_OEM_TABLE_ID_SIZE];
+};
+
+struct sfi_table_simple {
+	struct sfi_table_header header;
+	u64 pentry[1];
+};
+
+/* comply with UEFI spec 2.1 */
+struct sfi_mem_entry {
+	u32 type;
+	u64 phy_start;
+	u64 vir_start;
+	u64 pages;
+	u64 attrib;
+};
+
+struct sfi_cpu_table_entry {
+	u32 apicid;
+};
+
+struct sfi_cstate_table_entry {
+	u32 hint;	/* MWAIT hint */
+	u32 latency;	/* latency in ms */
+};
+
+struct sfi_apic_table_entry {
+	u64 phy_addr;	/* phy base addr for APIC reg */
+};
+
+struct sfi_freq_table_entry {
+	u32 freq;
+	u32 latency;	/* transition latency in ms */
+	u32 ctrl_val;	/* value to write to PERF_CTL to enter this state */
+};
+
+struct sfi_wake_table_entry {
+	u64 phy_addr;
+};
+
+struct sfi_timer_table_entry {
+	u64 phy_addr;	/* phy base addr for the timer */
+	u32 freq;	/* in HZ */
+	u32 irq;
+};
+
+struct sfi_rtc_table_entry {
+	u64 phy_addr;	/* phy base addr for the RTC */
+	u32 irq;
+};
+#pragma pack()
+
+extern int __init sfi_init_memory_map(void);
+extern int __init sfi_init(void);
+extern int __init sfi_platform_init(void);
+extern void __init sfi_init_late(void);
+
+typedef int (*sfi_table_handler) (struct sfi_table_header *table);
+
+int sfi_table_parse(char *signature, char *oem_id, char* oem_table_id,
+	uint flag, sfi_table_handler handler);
+
+void __init __iomem *
+arch_early_ioremap(unsigned long phys, unsigned long size);
+void __init arch_early_iounmap(void __iomem *virt, unsigned long size);
+
+
+extern int sfi_disabled;
+static inline void disable_sfi(void) { sfi_disabled = 1; }
+
+
+#else /* !CONFIG_SFI */
+
+static inline int sfi_init_memory_map(void) { return -1; }
+static inline int sfi_init(void) { return 0; }
+static inline void sfi_init_late(void)	{}
+#define sfi_disabled	0
+
+static inline int sfi_table_parse(char *signature, char *oem_id, char* oem_table_id,
+	unsigned int flags, sfi_table_handler handler) { return -1; }
+
+#endif	/* CONFIG_SFI */
+
+#endif	/*_LINUX_SFI_H*/
-- 
1.6.0.6

Re: [PATCH 2/8] SFI: include/linux/sfi.h

[Ingo Molnar]

* Len Brown <lenb@kernel.org> wrote:

> +/*
> + * Table structures must be byte-packed to match the SFI specification,
> + * as they are provided by the BIOS.
> + */
> +#pragma pack(1)

We use __attribute__((packed)) for that generally.

> +struct sfi_table_header {
> +	char signature[SFI_SIGNATURE_SIZE];
> +	u32 length;
> +	u8 revision;
> +	u8 checksum;
> +	char oem_id[SFI_OEM_ID_SIZE];
> +	char oem_table_id[SFI_OEM_TABLE_ID_SIZE];
> +};

Please use more readable structure definitions, such as:

> +struct sfi_table_header {
> +	char			signature[SFI_SIGNATURE_SIZE];
> +	u32			length;
> +	u8			revision;
> +	u8			checksum;
> +	char			oem_id[SFI_OEM_ID_SIZE];
> +	char			oem_table_id[SFI_OEM_TABLE_ID_SIZE];
> +};

> +void __init __iomem *
> +arch_early_ioremap(unsigned long phys, unsigned long size);
> +void __init arch_early_iounmap(void __iomem *virt, unsigned long size);

Why is this in sfr.h? Should be in a separate series.

> +static inline int sfi_init_memory_map(void) { return -1; }
> +static inline int sfi_init(void) { return 0; }
> +static inline void sfi_init_late(void)	{}

Seems half-aligned. Please align consistently, such as:

static inline int sfi_init_memory_map(void)	{ return -1; }
static inline int sfi_init(void)		{ return  0; }
static inline void sfi_init_late(void)		{ }

or dont align at all.

	Ingo

Re: [PATCH 2/8] SFI: include/linux/sfi.h

[Feng Tang]

On Tue, 23 Jun 2009 15:28:11 +0800
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Len Brown <lenb@kernel.org> wrote:
> 
> > +/*
> > + * Table structures must be byte-packed to match the SFI
> > specification,
> > + * as they are provided by the BIOS.
> > + */
> > +#pragma pack(1)
> 
> We use __attribute__((packed)) for that generally.
> 
> > +struct sfi_table_header {
> > +	char signature[SFI_SIGNATURE_SIZE];
> > +	u32 length;
> > +	u8 revision;
> > +	u8 checksum;
> > +	char oem_id[SFI_OEM_ID_SIZE];
> > +	char oem_table_id[SFI_OEM_TABLE_ID_SIZE];
> > +};
> 
> Please use more readable structure definitions, such as:
> 
> > +struct sfi_table_header {
> > +	char			signature[SFI_SIGNATURE_SIZE];
> > +	u32			length;
> > +	u8			revision;
> > +	u8			checksum;
> > +	char			oem_id[SFI_OEM_ID_SIZE];
> > +	char
> > oem_table_id[SFI_OEM_TABLE_ID_SIZE]; +};
> 
> > +void __init __iomem *
> > +arch_early_ioremap(unsigned long phys, unsigned long size);
> > +void __init arch_early_iounmap(void __iomem *virt, unsigned long
> > size);
> 
> Why is this in sfr.h? Should be in a separate series.

Thanks for your great comments, will address them.

For these arch_early_ioremap/arch_early_iounmap API, do you mean we should
put it in a sfi.h under "asm" folder? The reason we put it here is to
give a arch independent API declaration here and let each arch implement
its own func.

Thanks,
Feng

> 
> > +static inline int sfi_init_memory_map(void) { return -1; }
> > +static inline int sfi_init(void) { return 0; }
> > +static inline void sfi_init_late(void)	{}
> 
> Seems half-aligned. Please align consistently, such as:
> 
> static inline int sfi_init_memory_map(void)	{ return -1; }
> static inline int sfi_init(void)		{ return  0; }
> static inline void sfi_init_late(void)		{ }
> 
> or dont align at all.
> 
> 	Ingo

Re: [PATCH 2/8] SFI: include/linux/sfi.h

[Ingo Molnar]

* Feng Tang <feng.tang@intel.com> wrote:

> For these arch_early_ioremap/arch_early_iounmap API, do you mean 
> we should put it in a sfi.h under "asm" folder? The reason we put 
> it here is to give a arch independent API declaration here and let 
> each arch implement its own func.

Yeah, i'd suggest to create a new early-remap.h and early-remap.c 
kind of file to collect the existing bits for that. It's a bit ugly 
(not really the fault of SFI - these are pre-existing facilities) 
and might need some love - we better move it apart so that the light 
of attention shines on it.

What's the target merge of the SFI stuff, 2.6.31?

	Ingo

Re: [PATCH 2/8] SFI: include/linux/sfi.h

[Feng Tang]

On Tue, 23 Jun 2009 16:00:47 +0800
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Feng Tang <feng.tang@intel.com> wrote:
> 
> > For these arch_early_ioremap/arch_early_iounmap API, do you mean 
> > we should put it in a sfi.h under "asm" folder? The reason we put 
> > it here is to give a arch independent API declaration here and let 
> > each arch implement its own func.
> 
> Yeah, i'd suggest to create a new early-remap.h and early-remap.c 
> kind of file to collect the existing bits for that. It's a bit ugly 
> (not really the fault of SFI - these are pre-existing facilities) 
> and might need some love - we better move it apart so that the light 
> of attention shines on it.
> 
> What's the target merge of the SFI stuff, 2.6.31?

AFAIK, Len's target is for 2.6.32

Thanks,
Feng

> 
> 	Ingo

Re: [PATCH 2/8] SFI: include/linux/sfi.h

[Ingo Molnar]

* Feng Tang <feng.tang@intel.com> wrote:

> On Tue, 23 Jun 2009 16:00:47 +0800
> Ingo Molnar <mingo@elte.hu> wrote:
> 
> > 
> > * Feng Tang <feng.tang@intel.com> wrote:
> > 
> > > For these arch_early_ioremap/arch_early_iounmap API, do you mean 
> > > we should put it in a sfi.h under "asm" folder? The reason we put 
> > > it here is to give a arch independent API declaration here and let 
> > > each arch implement its own func.
> > 
> > Yeah, i'd suggest to create a new early-remap.h and 
> > early-remap.c kind of file to collect the existing bits for 
> > that. It's a bit ugly (not really the fault of SFI - these are 
> > pre-existing facilities) and might need some love - we better 
> > move it apart so that the light of attention shines on it.
> > 
> > What's the target merge of the SFI stuff, 2.6.31?
> 
> AFAIK, Len's target is for 2.6.32

Ah, ok - then there's time. The code is almost good for .31 btw ;-)

	Ingo

Re: [PATCH 2/8] SFI: include/linux/sfi.h

[H. Peter Anvin]

Ingo Molnar wrote:
> * Feng Tang <feng.tang@intel.com> wrote:
> 
>> On Tue, 23 Jun 2009 16:00:47 +0800
>> Ingo Molnar <mingo@elte.hu> wrote:
>>
>>> * Feng Tang <feng.tang@intel.com> wrote:
>>>
>>>> For these arch_early_ioremap/arch_early_iounmap API, do you mean 
>>>> we should put it in a sfi.h under "asm" folder? The reason we put 
>>>> it here is to give a arch independent API declaration here and let 
>>>> each arch implement its own func.
>>> Yeah, i'd suggest to create a new early-remap.h and 
>>> early-remap.c kind of file to collect the existing bits for 
>>> that. It's a bit ugly (not really the fault of SFI - these are 
>>> pre-existing facilities) and might need some love - we better 
>>> move it apart so that the light of attention shines on it.
>>>
>>> What's the target merge of the SFI stuff, 2.6.31?
>> AFAIK, Len's target is for 2.6.32
> 
> Ah, ok - then there's time. The code is almost good for .31 btw ;-)
> 

The code is really quite clean; I recommended to Len that he target
2.6.32 not because of the quality of the code, but just to get it plenty
of time to get properly reviewed and any architectural issues getting
fixed properly.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH 2/8] SFI: include/linux/sfi.h

[Andrew Morton]

On Tue, 23 Jun 2009 09:28:11 +0200
Ingo Molnar <mingo@elte.hu> wrote:

> > +/*
> > + * Table structures must be byte-packed to match the SFI specification,
> > + * as they are provided by the BIOS.
> > + */
> > +#pragma pack(1)
> 
> We use __attribute__((packed)) for that generally.

Or __packed.  I usually recommend __packed because I can never remember
the weird arrangement of underscores and parentheses in the gcc thing.

Re: [PATCH 2/8] SFI: include/linux/sfi.h

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue, 23 Jun 2009 09:28:11 +0200
> Ingo Molnar <mingo@elte.hu> wrote:
> 
> > > +/*
> > > + * Table structures must be byte-packed to match the SFI specification,
> > > + * as they are provided by the BIOS.
> > > + */
> > > +#pragma pack(1)
> > 
> > We use __attribute__((packed)) for that generally.
> 
> Or __packed.  I usually recommend __packed because I can never 
> remember the weird arrangement of underscores and parentheses in 
> the gcc thing.

Agreed - it's also shorter and more in line with stuff like 
__read_mostly, etc.

	Ingo

Re: [PATCH 2/8] SFI: include/linux/sfi.h

[Sam Ravnborg]

> +/*
> + * Table structures must be byte-packed to match the SFI specification,
> + * as they are provided by the BIOS.
> + */
> +#pragma pack(1)
> +struct sfi_table_header {
> +	char signature[SFI_SIGNATURE_SIZE];
> +	u32 length;
> +	u8 revision;
> +	u8 checksum;
> +	char oem_id[SFI_OEM_ID_SIZE];
> +	char oem_table_id[SFI_OEM_TABLE_ID_SIZE];
> +};
Are they endian specific?
In that case use the correct endian specific types.

Same for the other types later in this file.

	Sam

Re: [PATCH 2/8] SFI: include/linux/sfi.h

[Feng Tang]

On Tue, 23 Jun 2009 17:06:54 +0800
Sam Ravnborg <sam@ravnborg.org> wrote:

> > +/*
> > + * Table structures must be byte-packed to match the SFI
> > specification,
> > + * as they are provided by the BIOS.
> > + */
> > +#pragma pack(1)
> > +struct sfi_table_header {
> > +	char signature[SFI_SIGNATURE_SIZE];
> > +	u32 length;
> > +	u8 revision;
> > +	u8 checksum;
> > +	char oem_id[SFI_OEM_ID_SIZE];
> > +	char oem_table_id[SFI_OEM_TABLE_ID_SIZE];
> > +};
> Are they endian specific?
> In that case use the correct endian specific types.

I can try to answer this question, Len, correct me if I'm wrong.

These SFI tables are just byte strings, and created by FW, FW can chose
to use big-endian or little-endian in these tables based on what platform
they are used. So it's transparent for SFI driver code.

Thanks,
Feng

> 
> Same for the other types later in this file.
> 
> 	Sam

Re: [PATCH 2/8] SFI: include/linux/sfi.h

[Sam Ravnborg]

On Tue, Jun 23, 2009 at 11:52:17PM +0800, Feng Tang wrote:
> On Tue, 23 Jun 2009 17:06:54 +0800
> Sam Ravnborg <sam@ravnborg.org> wrote:
> 
> > > +/*
> > > + * Table structures must be byte-packed to match the SFI
> > > specification,
> > > + * as they are provided by the BIOS.
> > > + */
> > > +#pragma pack(1)
> > > +struct sfi_table_header {
> > > +	char signature[SFI_SIGNATURE_SIZE];
> > > +	u32 length;
> > > +	u8 revision;
> > > +	u8 checksum;
> > > +	char oem_id[SFI_OEM_ID_SIZE];
> > > +	char oem_table_id[SFI_OEM_TABLE_ID_SIZE];
> > > +};
> > Are they endian specific?
> > In that case use the correct endian specific types.
> 
> I can try to answer this question, Len, correct me if I'm wrong.
> 
> These SFI tables are just byte strings, and created by FW, FW can chose
> to use big-endian or little-endian in these tables based on what platform
> they are used. So it's transparent for SFI driver code.

But we have "u32 length;" - but I guess the signature
tell the endian then.

	Sam

[PATCH 3/8] SFI: core support

[Len Brown]

From: Feng Tang <feng.tang@intel.com>

drivers/sfi/sfi_core.c contains the generic SFI implementation.
It has a private header, sfi_core.h, for its own use and the
private use of future files in drivers/sfi/

arch/x86/kernel/sfi.c serves the dual-purpose of supporting the
SFI core with arch specific code, as well as a home for the
arch-specific code that uses SFI.

Signed-off-by: Feng Tang <feng.tang@intel.com>
Signed-off-by: Len Brown <len.brown@intel.com>
---
 arch/x86/kernel/Makefile |    1 +
 arch/x86/kernel/sfi.c    |  335 ++++++++++++++++++++++++++++++++++++++
 drivers/Makefile         |    1 +
 drivers/sfi/Makefile     |    1 +
 drivers/sfi/sfi_core.c   |  403 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/sfi/sfi_core.h   |   63 +++++++
 6 files changed, 804 insertions(+), 0 deletions(-)
 create mode 100644 arch/x86/kernel/sfi.c
 create mode 100644 drivers/sfi/Makefile
 create mode 100644 drivers/sfi/sfi_core.c
 create mode 100644 drivers/sfi/sfi_core.h

diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 88d1bfc..75042a7 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -50,6 +50,7 @@ obj-y				+= step.o
 obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
 obj-y				+= cpu/
 obj-y				+= acpi/
+obj-$(CONFIG_SFI)		+= sfi.o
 obj-y				+= reboot.o
 obj-$(CONFIG_MCA)		+= mca_32.o
 obj-$(CONFIG_X86_MSR)		+= msr.o
diff --git a/arch/x86/kernel/sfi.c b/arch/x86/kernel/sfi.c
new file mode 100644
index 0000000..d940de2
--- /dev/null
+++ b/arch/x86/kernel/sfi.c
@@ -0,0 +1,335 @@
+/*
+ *  sfi.c - SFI Boot Support (refer acpi/boot.c)
+ *
+ *  Copyright (C) 2008-2009	Intel Corporation
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+#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>
+
+#undef PREFIX
+#define PREFIX "SFI: "
+
+#ifdef CONFIG_X86_LOCAL_APIC
+static u64 sfi_lapic_addr __initdata = APIC_DEFAULT_PHYS_BASE;
+#endif
+
+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);
+
+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);
+}
+
+static ulong __init sfi_early_find_syst(void)
+{
+	unsigned long i;
+	char *pchar = (char *)SFI_SYST_SEARCH_BEGIN;
+
+	for (i = 0; SFI_SYST_SEARCH_BEGIN + i < SFI_SYST_SEARCH_END; i += 16, pchar += 16) {
+		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();
+	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;
+
+	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);
+}
+
+/* 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;
+
+	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);
+	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;
+
+	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
+static struct mp_ioapic_routing {
+	int			apic_id;
+	int			gsi_base;
+	int			gsi_end;
+	u32			pin_programmed[4];
+} mp_ioapic_routing[MAX_IO_APICS];
+
+/* 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);
+	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
+}
+
+
+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
+
+	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);
+	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;
+	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
+	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;
+EXPORT_SYMBOL(sfi_disabled);
+
+#define SFI_MAX_TABLES		64
+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);
+}
+
+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);
+}
+
+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);
+	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;
+
+	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.
+ */
+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);
+
+		if (strncmp(syst->signature, SFI_SIG_SYST, SFI_SIGNATURE_SIZE))
+				continue;
+
+		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);
+	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;
+
+	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 */
+	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;
+};
+
+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
+
-- 
1.6.0.6

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

[Ingo Molnar]

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

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

[Feng Tang]

On Tue, 23 Jun 2009 15:56:43 +0800
Ingo Molnar <mingo@elte.hu> wrote:


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

When Len designed the SFI spec, he considered the possibility of being used by
multiple archs, so we chose not to add a x86 dependency, though adding these
#ifdef does make code ugly :P

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

My bad not puting clear comments here, the SFI spec defines SYST table starts
at a 16-byte boundary


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

Yes, usually table won't be NULL

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

Thanks for the comments, I really got inspired :). The expectation for SFI
is to be able to run cleanly with CONFIG_ACPI=n, and it works fine on some
intel platform.

Thanks,
Feng

> 
>         Ingo

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

[Ingo Molnar]

* Feng Tang <feng.tang@intel.com> wrote:

> On Tue, 23 Jun 2009 15:56:43 +0800
> Ingo Molnar <mingo@elte.hu> wrote:
> 
> 
> > >
> > > +#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.
> 
> When Len designed the SFI spec, he considered the possibility of 
> being used by multiple archs, so we chose not to add a x86 
> dependency, though adding these
> #ifdef does make code ugly :P

But the file i commented on is arch/x86/kernel/sfi.c, not 
drivers/sfi/.

Those #ifdefs arent _that_ bad (and are used elsewhere in x86 code 
too) - but generally some effort should be spent in new code on 
trying to eliminate them.

> > In general, nice stuff - basically SFI is cleanly implemented 
> > ACPI tables without any of the run-code-in-acpi-tables 
> > complications, right?
> 
> Thanks for the comments, I really got inspired :). The expectation 
> for SFI is to be able to run cleanly with CONFIG_ACPI=n, and it 
> works fine on some intel platform.

Ok, cool!

	Ingo

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

[Feng Tang]

On Tue, 23 Jun 2009 17:03:47 +0800
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Feng Tang <feng.tang@intel.com> wrote:
> 
> > On Tue, 23 Jun 2009 15:56:43 +0800
> > Ingo Molnar <mingo@elte.hu> wrote:
> > 
> > 
> > > >
> > > > +#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.
> > 
> > When Len designed the SFI spec, he considered the possibility of 
> > being used by multiple archs, so we chose not to add a x86 
> > dependency, though adding these
> > #ifdef does make code ugly :P
> 
> But the file i commented on is arch/x86/kernel/sfi.c, not 
> drivers/sfi/.

Now got your point, then we can think about adding a SFI_X86 Kconfig 
option specifically for x86 platform, which has dependency over the
LAPIC/IO_APIC

Thanks,
Feng

> 
> Those #ifdefs arent _that_ bad (and are used elsewhere in x86 code 
> too) - but generally some effort should be spent in new code on 
> trying to eliminate them.
> 
> > > In general, nice stuff - basically SFI is cleanly implemented 
> > > ACPI tables without any of the run-code-in-acpi-tables 
> > > complications, right?
> > 
> > Thanks for the comments, I really got inspired :). The expectation 
> > for SFI is to be able to run cleanly with CONFIG_ACPI=n, and it 
> > works fine on some intel platform.
> 
> Ok, cool!
> 
> 	Ingo

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

[Len Brown]

Re: #define PREFIX "SFI: "

The SFI code uses all pr_info/debug/warning/err format
and so it looks like the way to go is like this;

#define KMSG_COMPONENT "SFI"
#define pr_fmt(fmt) KMSG_COMPONENT ": " fmt

and get rid of the PREFIX stuff in the actual messages.
Is that the recommended way to go these days?

thanks,
-Len Brown, Intel Open Soruce Technolgy Center

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

[Ingo Molnar]

* Len Brown <lenb@kernel.org> wrote:

> Re: #define PREFIX "SFI: "
> 
> The SFI code uses all pr_info/debug/warning/err format
> and so it looks like the way to go is like this;
> 
> #define KMSG_COMPONENT "SFI"
> #define pr_fmt(fmt) KMSG_COMPONENT ": " fmt
> 
> and get rid of the PREFIX stuff in the actual messages.
> Is that the recommended way to go these days?

Yeah, overloadig pr_fmt looks entirely correct and per design. That 
will make it even cleaner.

	Ingo

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

[Andi Kleen]

Len Brown <lenb@kernel.org> writes:

> +static ulong __init sfi_early_find_syst(void)
> +{
> +	unsigned long i;
> +	char *pchar = (char *)SFI_SYST_SEARCH_BEGIN;
> +
> +	for (i = 0; SFI_SYST_SEARCH_BEGIN + i < SFI_SYST_SEARCH_END; i += 16, pchar += 16) {
> +		if (!strncmp(SFI_SIG_SYST, pchar, SFI_SIGNATURE_SIZE))
> +			return SFI_SYST_SEARCH_BEGIN + i;


Such additional memory scans are always a bit risky, e.g. if there's
stray hardware there. Has it been verified that existing kernels
already scan this area?



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

printk here?

> +
> +	/* 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++;


Surely UNUSTABLE and RUNTIME_SERVICES should be added somewhere to the resources,
otherwise MMIO allocation might put something there.

> +	}
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_X86_LOCAL_APIC
> +void __init mp_sfi_register_lapic_address(u64 address)
> +{
> +	mp_lapic_addr = (unsigned long) address;
> +
> +	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);

That's the same what ACPI does, isn't it? Some code sharing
would be good.

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

Another useless BUG_ON. Some more below too.

> +	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;
> +
> +	for (i = 0; i < cpu_num; i++) {
> +		mp_sfi_register_lapic(pentry->apicid);
> +		pentry++;
> +	}

Array overflow checking?


> +	mp_ioapics[idx].apicver = 0;
> +#endif
> +
> +	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);

That printk should be dependent on the runtime apic verbosity level?

> +
> +/*
> + * 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;

permanant is a typo?

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

imho it would be cleaner if the callers just called these functions
directly. Are the !phys !size checks really needed?


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


Since the mappings are always 4K you would only need to remap
if the size is > PAGE_SIZE

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

To be honest I'm not sure why this list exists at all.
Is it that difficult to just rewalk the firmware supplied
table as needed? 


-andi
-- 
ak@linux.intel.com -- Speaking for myself only.

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

[Len Brown]

On Tue, 23 Jun 2009, Andi Kleen wrote:

> Len Brown <lenb@kernel.org> writes:
> 
> > +static ulong __init sfi_early_find_syst(void)
> > +{
> > +	unsigned long i;
> > +	char *pchar = (char *)SFI_SYST_SEARCH_BEGIN;
> > +
> > +	for (i = 0; SFI_SYST_SEARCH_BEGIN + i < SFI_SYST_SEARCH_END; i += 16, pchar += 16) {
> > +		if (!strncmp(SFI_SIG_SYST, pchar, SFI_SIGNATURE_SIZE))
> > +			return SFI_SYST_SEARCH_BEGIN + i;
> 
> 
> Such additional memory scans are always a bit risky, e.g. if there's
> stray hardware there. Has it been verified that existing kernels
> already scan this area?

Yes, SFI is the same as ACPI here (actually, a proper sub-set).

Note that the UEFI folks have suggested that on UEFI systems,
that they hand us the address like they can do for ACPI,
and we'll probably add that at some point.

thanks,
-Len Brown, Intel Open Source Technology Center

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

[Feng Tang]

On Tue, 23 Jun 2009 20:32:56 +0800
Andi Kleen <andi@firstfloor.org> wrote:

> > +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); +}
> 
> imho it would be cleaner if the callers just called these functions
> directly. Are the !phys !size checks really needed?

Andi,

Thanks for many good comments, will address them.

For this question, this sfi_map_memory() may get called before and after
the ioremap() is ready, so we add a permanent flag to judge the
environment and chose the right API automatically. e.g. after system is 
booted, cpu freq driver will implicitly call this API to get freq info 
> 
> 
> > +
> > +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;
> 
> 
> Since the mappings are always 4K you would only need to remap
> if the size is > PAGE_SIZE

yes, some of the table may be in one page, but some may not start at page
boundary and cross pages, we do it this way as this map/unmap/remap/unmap
routine only happen few times in boot phase.

> 
> > +
> > +	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));
> 
> To be honest I'm not sure why this list exists at all.
> Is it that difficult to just rewalk the firmware supplied
> table as needed? 

Currently, there are about 10 SFI tables (more are expected), and most of
them will be parsed in driver initialization phase, like timer/cpu idle/
cpu frequency/rtc/system wake driver. Using a global list may save some
system overhead

Thanks,
Feng
> 
> 
> -andi

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

[Andi Kleen]

On Wed, Jun 24, 2009 at 11:34:40AM +0800, Feng Tang wrote:
> 
> > > +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); +}
> > 
> > imho it would be cleaner if the callers just called these functions
> > directly. Are the !phys !size checks really needed?
> 
> Andi,
> 
> Thanks for many good comments, will address them.
> 
> For this question, this sfi_map_memory() may get called before and after
> the ioremap() is ready, so we add a permanent flag to judge the

Yes, but the callers should know this and they can call the right 
function. I suspect only very few callers will need the early
variant.

> environment and chose the right API automatically. e.g. after system is 
> booted, cpu freq driver will implicitly call this API to get freq info 

cpufreq driver shouldn't be initialized before ioremap

> > 
> > Since the mappings are always 4K you would only need to remap
> > if the size is > PAGE_SIZE
> 
> yes, some of the table may be in one page, but some may not start at page
> boundary and cross pages, we do it this way as this map/unmap/remap/unmap
> routine only happen few times in boot phase.

The TLB flushes tend to be a few thousand cycles at least.

It's not much, but with all the recent focus on faster boot times it's
still better to not write unnecessarily inefficient initialization code.

> 
> > 
> > > +
> > > +	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));
> > 
> > To be honest I'm not sure why this list exists at all.
> > Is it that difficult to just rewalk the firmware supplied
> > table as needed? 
> 
> Currently, there are about 10 SFI tables (more are expected), and most of
> them will be parsed in driver initialization phase, like timer/cpu idle/
> cpu frequency/rtc/system wake driver. Using a global list may save some
> system overhead

Walking the tables as they are laid out in memory should be quite
equivalent to walking a list, shouldn't it?

It would be only a relatively small simplification agreed, but if you're
claiming to do a "Simple Firmware Interface" imho you should try
to make it as simple possible, and that includes not setting up
redundant data structures.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

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

[Feng Tang]

On Wed, 24 Jun 2009 15:12:20 +0800
Andi Kleen <andi@firstfloor.org> wrote:

> On Wed, Jun 24, 2009 at 11:34:40AM +0800, Feng Tang wrote:
> > 
> > > > +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); +}
> > > 
> > > imho it would be cleaner if the callers just called these
> > > functions directly. Are the !phys !size checks really needed?
> > 
> > Andi,
> > 
> > Thanks for many good comments, will address them.
> > 
> > For this question, this sfi_map_memory() may get called before and
> > after the ioremap() is ready, so we add a permanent flag to judge
> > the
> 
> Yes, but the callers should know this and they can call the right 
> function. I suspect only very few callers will need the early
> variant.
> 
There is one sfi_table_parse() API, which is a SFI core function, it is exported
out and used in both boot phase (parsing cpu/ioapic) and later driver phase
(parsing idle/freq ...), when it get called, it doesn't know in which phase it get
called, and need such a flag to judge.


> > environment and chose the right API automatically. e.g. after
> > system is booted, cpu freq driver will implicitly call this API to
> > get freq info 
> 
> cpufreq driver shouldn't be initialized before ioremap

Right, it called the sfi_table_parse() after ioremap is ready.

> 
> > > 
> > > Since the mappings are always 4K you would only need to remap
> > > if the size is > PAGE_SIZE
> > 
> > yes, some of the table may be in one page, but some may not start
> > at page boundary and cross pages, we do it this way as this
> > map/unmap/remap/unmap routine only happen few times in boot phase.
> 
> The TLB flushes tend to be a few thousand cycles at least.
> 
> It's not much, but with all the recent focus on faster boot times it's
> still better to not write unnecessarily inefficient initialization
> code.

good point, will take care of it

> 
> > 
> > > 
> > > > +
> > > > +	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));
> > > 
> > > To be honest I'm not sure why this list exists at all.
> > > Is it that difficult to just rewalk the firmware supplied
> > > table as needed? 
> > 
> > Currently, there are about 10 SFI tables (more are expected), and
> > most of them will be parsed in driver initialization phase, like
> > timer/cpu idle/ cpu frequency/rtc/system wake driver. Using a
> > global list may save some system overhead
> 
> Walking the tables as they are laid out in memory should be quite
> equivalent to walking a list, shouldn't it?
> 
> It would be only a relatively small simplification agreed, but if
> you're claiming to do a "Simple Firmware Interface" imho you should
> try to make it as simple possible, and that includes not setting up
> redundant data structures.

understand your concern, but to walk a list we still need have some global
parameter like SYST address, and do the map/unmap and checksum work.

another reason for the global sfi_table_desc[] is, we only do some time ioremap
for each table and save the mapped address for future use. this idea is
borrowed from ACPI table handling.

Thanks,
Feng

> 
> -Andi

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

[Andi Kleen]

> understand your concern, but to walk a list we still need have some global
> parameter like SYST address, and do the map/unmap and checksum work.

You just keep it mapped always. And the checksums only need to 
be checked once at the beginning. If there's a checksum error
anywhere it's better to complete disable SFI

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

[PATCH 4/8] SFI: Hook boot-time initialization

[Len Brown]

From: Feng Tang <feng.tang@intel.com>

Signed-off-by: Feng Tang <feng.tang@intel.com>
Signed-off-by: Len Brown <len.brown@intel.com>
---
 arch/x86/kernel/setup.c |    3 +++
 init/main.c             |    2 ++
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index b415843..e2e6794 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -27,6 +27,7 @@
 #include <linux/screen_info.h>
 #include <linux/ioport.h>
 #include <linux/acpi.h>
+#include <linux/sfi.h>
 #include <linux/apm_bios.h>
 #include <linux/initrd.h>
 #include <linux/bootmem.h>
@@ -952,6 +953,8 @@ void __init setup_arch(char **cmdline_p)
 	 */
 	acpi_boot_init();
 
+	sfi_init();
+
 #if defined(CONFIG_X86_MPPARSE) || defined(CONFIG_X86_VISWS)
 	/*
 	 * get boot-time SMP configuration:
diff --git a/init/main.c b/init/main.c
index f1b9f0f..36333c7 100644
--- a/init/main.c
+++ b/init/main.c
@@ -65,6 +65,7 @@
 #include <linux/idr.h>
 #include <linux/ftrace.h>
 #include <linux/async.h>
+#include <linux/sfi.h>
 #include <trace/boot.h>
 
 #include <asm/io.h>
@@ -688,6 +689,7 @@ asmlinkage void __init start_kernel(void)
 	check_bugs();
 
 	acpi_early_init(); /* before LAPIC and SMP init */
+	sfi_init_late();
 
 	ftrace_init();
 
-- 
1.6.0.6

[PATCH 5/8] SFI: Hook e820 memory map initialization

[Len Brown]

From: Feng Tang <feng.tang@intel.com>

Signed-off-by: Feng Tang <feng.tang@intel.com>
Signed-off-by: Len Brown <len.brown@intel.com>
---
 arch/x86/kernel/e820.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 0062813..f5f5ff2 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -20,6 +20,7 @@
 #include <linux/pfn.h>
 #include <linux/suspend.h>
 #include <linux/firmware-map.h>
+#include <linux/sfi.h>
 
 #include <asm/pgtable.h>
 #include <asm/page.h>
@@ -1403,6 +1404,10 @@ char *__init default_machine_specific_memory_setup(void)
 	  < 0) {
 		u64 mem_size;
 
+		/* if SFI mmap table exists, use SFI to setup e820 mmap */
+		if (!sfi_init_memory_map())
+			return "SFI";
+
 		/* compare results from other methods and take the greater */
 		if (boot_params.alt_mem_k
 		    < boot_params.screen_info.ext_mem_k) {
-- 
1.6.0.6

[PATCH 6/8] SFI: add ACPI extensions

[Len Brown]

From: Feng Tang <feng.tang@intel.com>

Extend SFI to access standard ACPI tables
such as the PCI MCFG using sfi_acpi_table_parse().

Note that SFI runs only with acpi_disabled=1,
but is independent of CONFIG_ACPI.

Signed-off-by: Feng Tang <feng.tang@intel.com>
Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/acpi/tables.c       |    3 +
 drivers/sfi/Makefile        |    2 +
 drivers/sfi/sfi_acpi.c      |   94 +++++++++++++++++++++++++++++++++++++++++++
 include/acpi/acpi_drivers.h |    3 +
 include/linux/acpi.h        |    5 +-
 include/linux/sfi_acpi.h    |   56 +++++++++++++++++++++++++
 6 files changed, 161 insertions(+), 2 deletions(-)
 create mode 100644 drivers/sfi/sfi_acpi.c
 create mode 100644 include/linux/sfi_acpi.h

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 646d39c..921746f 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -277,6 +277,9 @@ int __init acpi_table_parse(char *id, acpi_table_handler handler)
 	struct acpi_table_header *table = NULL;
 	acpi_size tbl_size;
 
+	if (acpi_disabled)
+		return 1;
+
 	if (!handler)
 		return -EINVAL;
 
diff --git a/drivers/sfi/Makefile b/drivers/sfi/Makefile
index f176470..2343732 100644
--- a/drivers/sfi/Makefile
+++ b/drivers/sfi/Makefile
@@ -1 +1,3 @@
+obj-y	+= sfi_acpi.o
 obj-y	+= sfi_core.o
+
diff --git a/drivers/sfi/sfi_acpi.c b/drivers/sfi/sfi_acpi.c
new file mode 100644
index 0000000..f972785
--- /dev/null
+++ b/drivers/sfi/sfi_acpi.c
@@ -0,0 +1,94 @@
+/* sfi_acpi.c Simple Firmware Interface - ACPI extensions */
+
+/*
+ * 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/acpi.h>
+#include <linux/kernel.h>
+#include <linux/sfi.h>
+#include "sfi_core.h"
+
+#undef PREFIX
+#define PREFIX	"SFI: "
+
+/*
+ * SFI can access ACPI-defined tables via an optional ACPI XSDT.
+ *
+ * This allows re-use, and avoids re-definition, of standard tables.
+ * For example, the "MCFG" table is defined by PCI, reserved by ACPI,
+ * and is expected to be present many SFI-only systems.
+ */
+
+/*
+ * sfi_acpi_parse_xsdt()
+ *
+ * Parse the ACPI XSDT for later access by sfi_acpi_table_parse().
+ */
+static int __init sfi_acpi_parse_xsdt(struct sfi_table_header *table)
+{
+	int num_entries, i;
+
+	struct acpi_table_xsdt *xsdt = (struct acpi_table_xsdt *) table;
+
+	BUG_ON(!xsdt);
+
+	num_entries = (xsdt->header.length - sizeof(struct acpi_table_header) /
+		sizeof(u64));
+
+	pr_debug(PREFIX "XSDT has %d entries\n", num_entries);
+
+	for (i = 0; i < num_entries; i++)
+		sfi_tb_install_table(xsdt->table_offset_entry[i], SFI_ACPI_TABLE);
+
+	return 0;
+}
+
+int sfi_acpi_init()
+{
+	sfi_table_parse(SFI_SIG_XSDT, NULL, NULL, 0, sfi_acpi_parse_xsdt);
+	return 0;
+}
+/*
+ * sfi_acpi_table_parse()
+ */
+int sfi_acpi_table_parse(char *signature, char *oem_id, char* oem_table_id,
+        unsigned int flags, acpi_table_handler handler)
+{
+	return sfi_table_parse(signature, oem_id, oem_table_id, SFI_ACPI_TABLE,
+		(sfi_table_handler)handler);
+}
+
diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
index 0352c8f..5d84a17 100644
--- a/include/acpi/acpi_drivers.h
+++ b/include/acpi/acpi_drivers.h
@@ -29,6 +29,8 @@
 #include <linux/acpi.h>
 #include <acpi/acpi_bus.h>
 
+#ifdef	CONFIG_ACPI
+
 #define ACPI_MAX_STRING			80
 
 /*
@@ -157,4 +159,5 @@ static inline void unregister_hotplug_dock_device(acpi_handle handle)
 }
 #endif
 
+#endif /* CONFIG_ACPI */
 #endif /*__ACPI_DRIVERS_H__*/
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index bf17681..be418f1 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -27,7 +27,6 @@
 
 #include <linux/ioport.h>	/* for struct resource */
 
-#ifdef	CONFIG_ACPI
 
 #ifndef _LINUX
 #define _LINUX
@@ -43,6 +42,9 @@
 #include <asm/acpi.h>
 #include <linux/dmi.h>
 
+typedef int (*acpi_table_handler) (struct acpi_table_header *table);
+
+#ifdef	CONFIG_ACPI
 
 enum acpi_irq_model_id {
 	ACPI_IRQ_MODEL_PIC = 0,
@@ -74,7 +76,6 @@ enum acpi_address_range_id {
 
 /* Table Handlers */
 
-typedef int (*acpi_table_handler) (struct acpi_table_header *table);
 
 typedef int (*acpi_table_entry_handler) (struct acpi_subtable_header *header, const unsigned long end);
 
diff --git a/include/linux/sfi_acpi.h b/include/linux/sfi_acpi.h
new file mode 100644
index 0000000..7a49380
--- /dev/null
+++ b/include/linux/sfi_acpi.h
@@ -0,0 +1,56 @@
+/* sfi.h Simple Firmware Interface */
+
+/*
+ * 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.
+ */
+
+#ifndef _LINUX_SFI_ACPI_H
+#define _LINUX_SFI_ACPI_H
+
+#ifdef	CONFIG_SFI
+#include <linux/acpi.h>		/* acpi_table_handler */
+
+int sfi_acpi_table_parse(char *signature, char *oem_id, char* oem_table_id,
+	uint flag, acpi_table_handler handler);
+
+#else /* !CONFIG_SFI */
+
+static inline int sfi_acpi_table_parse(char *signature, char *oem_id, char* oem_table_id,
+	unsigned int flags, acpi_table_handler handler) { return -1; }
+
+#endif	/* CONFIG_SFI */
+
+#endif	/*_LINUX_SFI_ACPI_H*/
-- 
1.6.0.6

Re: [PATCH 6/8] SFI: add ACPI extensions

[Andi Kleen]

Len Brown <lenb@kernel.org> writes:
>
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index 646d39c..921746f 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -277,6 +277,9 @@ int __init acpi_table_parse(char *id, acpi_table_handler handler)
>  	struct acpi_table_header *table = NULL;
>  	acpi_size tbl_size;
>  
> +	if (acpi_disabled)
> +		return 1;
> +

This seems like a weird place to hook this in. Shouldn't that be somewhere else, more
high level?


> +#include <linux/acpi.h>
> +#include <linux/kernel.h>
> +#include <linux/sfi.h>
> +#include "sfi_core.h"
> +
> +#undef PREFIX

Nobody should set PREFIX in headers? If they do better fix the headers.


> + * sfi_acpi_parse_xsdt()
> + *
> + * Parse the ACPI XSDT for later access by sfi_acpi_table_parse().
> + */
> +static int __init sfi_acpi_parse_xsdt(struct sfi_table_header *table)
> +{
> +	int num_entries, i;
> +
> +	struct acpi_table_xsdt *xsdt = (struct acpi_table_xsdt *) table;
> +
> +	BUG_ON(!xsdt);


Seems like a pointless BUG_ON, the NULL pointer reference next to it is
obvious enough.

> +
> +	num_entries = (xsdt->header.length - sizeof(struct acpi_table_header) /
> +		sizeof(u64));
> +
> +	pr_debug(PREFIX "XSDT has %d entries\n", num_entries);
> +
> +	for (i = 0; i < num_entries; i++)
> +		sfi_tb_install_table(xsdt->table_offset_entry[i], SFI_ACPI_TABLE);

Shouldn't this have some more sanity checking, e.g. for overflows?


-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 6/8] SFI: add ACPI extensions

[Len Brown]

On Tue, 23 Jun 2009, Andi Kleen wrote:
> Len Brown <lenb@kernel.org> writes:

> > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> > @@ -277,6 +277,9 @@ int __init acpi_table_parse(char *id, acpi_table_handler handler)

> > +	if (acpi_disabled)
> > +		return 1;

> This seems like a weird place to hook this in. Shouldn't that be somewhere else, more
> high level?

acpi_table_parse() is actually a high-level API available to drivers.

Today, drivers tend to test acpi_disabled on their own,
which may be too high level...

The catalyst for this change, IIR, was the PCI code calling
and not checking the return value:

--- a/arch/x86/pci/mmconfig-shared.c
...
@@ -606,7 +607,8 @@ static void __init __pci_mmcfg_init(int early)
...
-               acpi_table_parse(ACPI_SIG_MCFG, pci_parse_mcfg);
+               if (acpi_table_parse(ACPI_SIG_MCFG, pci_parse_mcfg))
+                       sfi_acpi_table_parse(ACPI_SIG_MCFG, NULL, NULL, 0, pci_parse_mcfg);
...


> > +	num_entries = (xsdt->header.length - sizeof(struct acpi_table_header) /
> > +		sizeof(u64));
> > +
> > +	pr_debug(PREFIX "XSDT has %d entries\n", num_entries);
> > +
> > +	for (i = 0; i < num_entries; i++)
> > +		sfi_tb_install_table(xsdt->table_offset_entry[i], SFI_ACPI_TABLE);
> 
> Shouldn't this have some more sanity checking, e.g. for overflows?

good observation. Although we did already check the signature and 
checksum, I don't see that we yet have a sanity check either here
or in sfi_tb_install_table() itself.

thanks,
-Len

[PATCH 7/8] SFI, PCI: Hook MMCONFIG

[Len Brown]

From: Feng Tang <feng.tang@intel.com>

Logically, if ACPI table parsing doesn't find the MCFG,
then try SFI.  In reality, the systemw will be in either
ACPI or SFI mode, so only one of these routines will run.

Signed-off-by: Feng Tang <feng.tang@intel.com>
Signed-off-by: Len Brown <len.brown@intel.com>
---
 arch/x86/Kconfig               |    2 +-
 arch/x86/pci/mmconfig-shared.c |    4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 06341a5..1143088 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1838,7 +1838,7 @@ config PCI_DIRECT
 
 config PCI_MMCONFIG
 	def_bool y
-	depends on X86_32 && PCI && ACPI && (PCI_GOMMCONFIG || PCI_GOANY)
+	depends on X86_32 && PCI && (ACPI || SFI) && (PCI_GOMMCONFIG || PCI_GOANY)
 
 config PCI_OLPC
 	def_bool y
diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
index 712443e..c09683d 100644
--- a/arch/x86/pci/mmconfig-shared.c
+++ b/arch/x86/pci/mmconfig-shared.c
@@ -13,6 +13,7 @@
 #include <linux/pci.h>
 #include <linux/init.h>
 #include <linux/acpi.h>
+#include <linux/sfi_acpi.h>
 #include <linux/bitmap.h>
 #include <linux/sort.h>
 #include <asm/e820.h>
@@ -606,7 +607,8 @@ static void __init __pci_mmcfg_init(int early)
 	}
 
 	if (!known_bridge)
-		acpi_table_parse(ACPI_SIG_MCFG, pci_parse_mcfg);
+		if (acpi_table_parse(ACPI_SIG_MCFG, pci_parse_mcfg))
+			sfi_acpi_table_parse(ACPI_SIG_MCFG, NULL, NULL, 0, pci_parse_mcfg);
 
 	pci_mmcfg_reject_broken(early);
 
-- 
1.6.0.6

[PATCH 8/8] SFI: expose IO-APIC routines to SFI, not just ACPI

[Len Brown]

From: Feng Tang <feng.tang@intel.com>

change condition from
	#ifdef CONFIG_ACPI
to
	#if defined(CONFIG_ACPI) || defined(CONFIG_SFI)

for several io_apic related APIs which will be used by both
ACPI and SFI.

this change will ensure the success kernel build whe
CONFIG_SFI=y and CONFIG_ACPI=n

Signed-off-by: Feng Tang <feng.tang@intel.com>
Signed-off-by: Len Brown <len.brown@intel.com>
---
 arch/x86/include/asm/io_apic.h |    4 ++--
 arch/x86/kernel/apic/io_apic.c |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
index 9d826e4..2e5ff36 100644
--- a/arch/x86/include/asm/io_apic.h
+++ b/arch/x86/include/asm/io_apic.h
@@ -150,13 +150,13 @@ extern int timer_through_8259;
 #define io_apic_assign_pci_irqs \
 	(mp_irq_entries && !skip_ioapic_setup && io_apic_irqs)
 
-#ifdef CONFIG_ACPI
+#if defined(CONFIG_ACPI) || defined(CONFIG_SFI)
 extern int io_apic_get_unique_id(int ioapic, int apic_id);
 extern int io_apic_get_version(int ioapic);
 extern int io_apic_get_redir_entries(int ioapic);
 extern int io_apic_set_pci_routing(int ioapic, int pin, int irq,
 				   int edge_level, int active_high_low);
-#endif /* CONFIG_ACPI */
+#endif /* CONFIG_ACPI || CONFIG_SFI */
 
 extern int (*ioapic_renumber_irq)(int ioapic, int irq);
 extern void ioapic_init_mappings(void);
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 30da617..2705476 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -3904,7 +3904,7 @@ int __init arch_probe_nr_irqs(void)
                           ACPI-based IOAPIC Configuration
    -------------------------------------------------------------------------- */
 
-#ifdef CONFIG_ACPI
+#if defined(CONFIG_ACPI) || defined(CONFIG_SFI)
 
 #ifdef CONFIG_X86_32
 int __init io_apic_get_unique_id(int ioapic, int apic_id)
@@ -4045,7 +4045,7 @@ int acpi_get_override_irq(int bus_irq, int *trigger, int *polarity)
 	return 0;
 }
 
-#endif /* CONFIG_ACPI */
+#endif /* CONFIG_ACPI || CONFIG_SFI */
 
 /*
  * This function currently is only a helper for the i386 smp boot process where
-- 
1.6.0.6

Re: [PATCH 8/8] SFI: expose IO-APIC routines to SFI, not just ACPI

[Ingo Molnar]

* Len Brown <lenb@kernel.org> wrote:

> From: Feng Tang <feng.tang@intel.com>
> 
> change condition from
> 	#ifdef CONFIG_ACPI
> to
> 	#if defined(CONFIG_ACPI) || defined(CONFIG_SFI)

Please ivent a properly named helper Kconfig entry that expresses 
that condition, instead of uglifying the source code.

Thanks,

	Ingo

Re: [PATCH 1/8] SFI: Simple Firmware Interface - new Linux sub-system

[Ingo Molnar]

* Len Brown <lenb@kernel.org> wrote:

> From: Len Brown <len.brown@intel.com>
> 
> CONFIG_SFI=y enables the kernel to boot and run optimally
> on platforms that support the Simple Firmware Interface.
> 
> Thanks to Jacob Pan for prototyping the initial Linux SFI support,
> and to Feng Tang for Linux bring-up and debug in emulation
> and on hardware.
> 
> See http://simplefirmware.org for more information on SFI.
> 
> Signed-off-by: Feng Tang <feng.tang@intel.com>
> Signed-off-by: Len Brown <len.brown@intel.com>
> ---
>  Documentation/kernel-parameters.txt |    5 +++++
>  MAINTAINERS                         |   12 ++++++++++++
>  arch/x86/Kconfig                    |    2 ++
>  drivers/sfi/Kconfig                 |   16 ++++++++++++++++
>  4 files changed, 35 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/sfi/Kconfig

Looks useful but this series needs review and acked-by from the x86 
maintainers, obviously. It touches a lot of x86 details.

	Ingo

Re: [RFC/PATCH 2.6.32] Simple Firmware Interface (SFI): initial support

[Matthew Garrett]

There seems to be a huge amount of overlap between SFI and ACPI. 
Couldn't this have simply taken the form of some additional ACPI tables 
and a decoupling of ACPI enumeration from runtime AML interpretation? 
How final is this spec?

I realise that we're pretty much constrained to implementing this if 
hardware actually ships with it, but it seems to be an additional 
firmware interface with no real benefit - as far as I can tell it's not 
possible for a platform to meaningfully implement both ACPI and SFI 
without duplicating information?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC/PATCH 2.6.32] Simple Firmware Interface (SFI): initial support

[Len Brown]

On Tue, 23 Jun 2009, Matthew Garrett wrote:

> There seems to be a huge amount of overlap between SFI and ACPI.
> Couldn't this have simply taken the form of some additional ACPI tables 
> and a decoupling of ACPI enumeration from runtime AML interpretation? 
> How final is this spec?

> I realise that we're pretty much constrained to implementing this if 
> hardware actually ships with it, but it seems to be an additional 
> firmware interface with no real benefit - as far as I can tell it's not 
> possible for a platform to meaningfully implement both ACPI and SFI 
> without duplicating information?

Please let me know if your questions are not thoroughly answered here:
http://simplefirmware.org/faq

thanks,
-Len Brown, Intel Open Source Technology Center

Re: [RFC/PATCH 2.6.32] Simple Firmware Interface (SFI): initial support

[Pavel Machek]

On Tue 2009-06-23 14:41:28, Len Brown wrote:
> On Tue, 23 Jun 2009, Matthew Garrett wrote:
> 
> > There seems to be a huge amount of overlap between SFI and ACPI.
> > Couldn't this have simply taken the form of some additional ACPI tables 
> > and a decoupling of ACPI enumeration from runtime AML interpretation? 
> > How final is this spec?
> 
> > I realise that we're pretty much constrained to implementing this if 
> > hardware actually ships with it, but it seems to be an additional 
> > firmware interface with no real benefit - as far as I can tell it's not 
> > possible for a platform to meaningfully implement both ACPI and SFI 
> > without duplicating information?
> 
> Please let me know if your questions are not thoroughly answered here:
> http://simplefirmware.org/faq

It really tells us nothing. I don't think flash got so expensive that
this is justified. ACPI can already do the job, right? and operating
systems already have to support ACPI. So what are the reasons to
reinvent the wheel?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC/PATCH 2.6.32] Simple Firmware Interface (SFI): initial support

[Len Brown]

On Mon, 22 Jun 2009, Pavel Machek wrote:

> On Tue 2009-06-23 14:41:28, Len Brown wrote:

> > Please let me know if your questions are not thoroughly answered here:
> > http://simplefirmware.org/faq
> 
> It really tells us nothing. I don't think flash got so expensive that
> this is justified. ACPI can already do the job, right? and operating
> systems already have to support ACPI. So what are the reasons to
> reinvent the wheel?

The price of flash, and the amount consumed, is not relevent
to the decision whether a platform should support SFI or ACPI.

The Moorestown platform doesn't use ACPI because its chip-set
fundamentally does not support it.  Not only is the required
register set missing, *all* IO accesses are missing, and there is
no SMM support present to emuate it.

Yes, the ACPI specification could have been edited to replace
every "must" with "could", "shall" with "may", and "required" with 
"optional" resulting in "ACPI compliance" for your toaster.
But doing so would have been a dis-service to the
platforms supporting ACPI, and would have made the
already hard job of supporting ACPI from the OS significantly harder.

thanks,
-Len Brown, Intel Open Source Technology Center

Re: [RFC/PATCH 2.6.32] Simple Firmware Interface (SFI): initial support

[Pavel Machek]

On Wed 2009-06-24 17:13:18, Len Brown wrote:
> On Mon, 22 Jun 2009, Pavel Machek wrote:
> 
> > On Tue 2009-06-23 14:41:28, Len Brown wrote:
> 
> > > Please let me know if your questions are not thoroughly answered here:
> > > http://simplefirmware.org/faq
> > 
> > It really tells us nothing. I don't think flash got so expensive that
> > this is justified. ACPI can already do the job, right? and operating
> > systems already have to support ACPI. So what are the reasons to
> > reinvent the wheel?
> 
> The price of flash, and the amount consumed, is not relevent
> to the decision whether a platform should support SFI or ACPI.
> 
> The Moorestown platform doesn't use ACPI because its chip-set
> fundamentally does not support it.  Not only is the required
> register set missing, *all* IO accesses are missing, and there is
> no SMM support present to emuate it.
> 
> Yes, the ACPI specification could have been edited to replace
> every "must" with "could", "shall" with "may", and "required" with 
> "optional" resulting in "ACPI compliance" for your toaster.
> But doing so would have been a dis-service to the
> platforms supporting ACPI, and would have made the
> already hard job of supporting ACPI from the OS significantly harder.

Well, you should have just selected subset of ACPI, documenting that
and implementing that. You would not have 'acpi compliant' logo, and
windows XP would not boot on that, but at least you would not have
created one more bios standard for people to support.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [SFI-devel] [RFC/PATCH 2.6.32] Simple Firmware Interface (SFI): initial support

[Peter Stuge]

Hi Pavel,

Pavel Machek wrote:
> > > ACPI can already do the job, right?

As Len explains, it can't.


> > > and operating systems already have to support ACPI.

I disagree here. How so? There are certainly other ways to accomplish
what ACPI does.


> > > So what are the reasons to reinvent the wheel?

Hardware developments push firmware and operating systems to new
places. I believe that's both neccessary and normal.


> > The Moorestown platform doesn't use ACPI because its chip-set
> > fundamentally does not support it.  Not only is the required
> > register set missing, *all* IO accesses are missing, and there is
> > no SMM support present to emuate it.
..
> Well, you should have just selected subset of ACPI, documenting that
> and implementing that. You would not have 'acpi compliant' logo, and
> windows XP would not boot on that, but at least you would not have
> created one more bios standard for people to support.

I don't think that's a practical option though.


I've been involved in the coreboot (formerly LinuxBIOS) project for a
good number of years, in part because I realized that the PC firmware
environment had lacked any serious developments for a very long time
and anything new, especially open source, was a good idea.

There's a certain stigma to coreboot in parts of the Linux kernel
community, because it's something very new and different in the x86
firmware landscape. Let me just say that I completely understand the
desire to resist changes in what is a somewhat functioning and at any
rate very wide spread technology. It seems that SFI is now facing
some of the same resistance, for much the same reason.

x86 architecture is changing, and firmware must change with it. It
will do operating systems good to join in, early on. Ideally there
will be much more cooperation across disciplines than in the past,
especially since hardware and software are already converging to a
point where good engineers on either side really must know also how
the other camp works. The only thing that remains with x86 is the
instruction set, every other part of the architecture has changed, is
changing, and will continue to change. That affects both firmware and
operating system of course.

ACPI is not always ideal, some may argue never. SFI strikes me as a
good complement. I think we will come to see further alternatives,
and finally I think that's a good thing, as software and hardware
continues to develop.

Of course it's not simple for operating systems. Nor for firmware.
But I don't believe the optimal and practical solution is to resist
rounder and lighter wheels.


//Peter

Re: [RFC/PATCH 2.6.32] Simple Firmware Interface (SFI): initial support

[Matthew Garrett]

On Tue, Jun 23, 2009 at 02:41:28PM -0400, Len Brown wrote:
> On Tue, 23 Jun 2009, Matthew Garrett wrote:
> 
> > There seems to be a huge amount of overlap between SFI and ACPI.
> > Couldn't this have simply taken the form of some additional ACPI tables 
> > and a decoupling of ACPI enumeration from runtime AML interpretation? 
> > How final is this spec?
> 
> > I realise that we're pretty much constrained to implementing this if 
> > hardware actually ships with it, but it seems to be an additional 
> > firmware interface with no real benefit - as far as I can tell it's not 
> > possible for a platform to meaningfully implement both ACPI and SFI 
> > without duplicating information?
> 
> Please let me know if your questions are not thoroughly answered here:
> http://simplefirmware.org/faq

Yeah, I read that and it didn't really seem to clear things up. There's 
no especially meaningful reason for a specialised platform to include 
any AML code - the closest equivalent case I'm thinking of is "acpi=ht" 
where we parse some static ACPI tables but don't do any runtime 
interpretation. In this universe, SFI would simply have been some 
specced additional tables and a build option to include table parsing 
but not the interpreter.

I appreciate that if this is what's going to be on the hardware then 
we're stuck with it, but I'm hugely unconvinced that this couldn't have 
taken the form of "embedded ACPI" rather than an entire new firmware 
interface.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC/PATCH 2.6.32] Simple Firmware Interface (SFI): initial support

[Len Brown]

On Tue, 23 Jun 2009, Matthew Garrett wrote:

> On Tue, Jun 23, 2009 at 02:41:28PM -0400, Len Brown wrote:
> > On Tue, 23 Jun 2009, Matthew Garrett wrote:
> > 
> > > There seems to be a huge amount of overlap between SFI and ACPI.
> > > Couldn't this have simply taken the form of some additional ACPI tables 
> > > and a decoupling of ACPI enumeration from runtime AML interpretation? 
> > > How final is this spec?
> > 
> > > I realise that we're pretty much constrained to implementing this if 
> > > hardware actually ships with it, but it seems to be an additional 
> > > firmware interface with no real benefit - as far as I can tell it's not 
> > > possible for a platform to meaningfully implement both ACPI and SFI 
> > > without duplicating information?
> > 
> > Please let me know if your questions are not thoroughly answered here:
> > http://simplefirmware.org/faq
> 
> Yeah, I read that and it didn't really seem to clear things up. There's 
> no especially meaningful reason for a specialised platform to include 
> any AML code - the closest equivalent case I'm thinking of is "acpi=ht" 
> where we parse some static ACPI tables but don't do any runtime 
> interpretation. In this universe, SFI would simply have been some 
> specced additional tables and a build option to include table parsing 
> but not the interpreter.
> 
> I appreciate that if this is what's going to be on the hardware then 
> we're stuck with it, but I'm hugely unconvinced that this couldn't have 
> taken the form of "embedded ACPI" rather than an entire new firmware 
> interface.

Fundamentally, Moorestown is an x86 processor
married to a cell-phone chipset.

It does not have x86 legacy PC compatibility,
and it does not have any ACPI registers.
It does not support SMM, and thus can't emulate the above.

One can argue the merits of the architecture..
The software person will always argue for compatibility (and be right)
and the hardware product person will have their own reasons (and be 
right).

But given that the hardware is fixed (it was fixed over a year ago),
the question becomes what does ACPI mean on such a platform?
It turns out that if you look at the ACPI spec and delete all the
things that could not possibly apply to MRST, then you are left
with very little.

Yes, the ACPI OS code could have been sliced up for the benefit
of MRST.  But doing so would have had an extremely small benefit
to MRST, and would have inflicted serious damage to CONFIG_ACPI
on the rest of the industry.  So I decided it was a better idea
to make a clean break for the benefit of both platforms.

Note that while it makes sense for a single kernel to support
both ACPI and SFI to be able to run on both platforms, it does
not make any sense for a platform to export both.  If that were
to occur (say in a prototype), the platform's SFI suport would
simply be ignored by the OS.

thanks,
-Len Brown, Intel Open Source Technolgy Center

Re: [RFC/PATCH 2.6.32] Simple Firmware Interface (SFI): initial support

[Matthew Garrett]

On Tue, Jun 23, 2009 at 04:00:55PM -0400, Len Brown wrote:

> But given that the hardware is fixed (it was fixed over a year ago),
> the question becomes what does ACPI mean on such a platform?
> It turns out that if you look at the ACPI spec and delete all the
> things that could not possibly apply to MRST, then you are left
> with very little.

Right, but instead you've effectively taken ACPI, done s/XSDT/SYST/ and 
then only supported a subset of the static tables and added some others. 
In return we gain two implementations to debug. I'm absolutely fine with 
the concept of a cut-down ACPI, but I'm pretty uncomfortable with it 
being implemented as a single-vendor spec. Right now SFI's a 
reimplementation of functionality we already have for the benefit of a 
single chipset, whereas instead it could have been a refactoring of the 
ACPI codebase to allow vendors to include whatever subset of the ACPI 
functionality they felt necessary.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC/PATCH 2.6.32] Simple Firmware Interface (SFI): initial support

[Matthew Garrett]

I think I've got a clearer understanding of my objections to this now. 
The first is that SFI is designed to support the subset of information 
that's in ACPI and which can't be intuited by the OS. However, that 
subset is predicated on the system looking like Moorestown. A system 
that wants to provide any information beyond that subset can't use SFI 
unless it defines additional tables.

And that brings me onto my second issue. ACPI is sufficiently 
generalised that there's little need for vendors to add additional 
tables. SFI isn't, and so vendor adoption is going to require 
vendor-specific tables. This potentially results in SFI bloating out to 
cover much of the functionality of ACPI, while at the same time turning 
into a namespacing nightmare. Without a formal process for adding new 
tables and without any kind of certification requirements before 
claiming SFI compatibility, we're looking at a real risk of collisions.

SFI appears to be presented as a generic firmware interface, but in 
reality it's currently tightly wed to Moorestown and I don't see any way 
that that can be fixed without reinventing chunks of ACPI. I'm certainly 
not enthusiastic about seeing this presented as a fait accompli in 
generic driver code, rather than under arch/x86/moorestown.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC/PATCH 2.6.32] Simple Firmware Interface (SFI): initial support

[Alan Cox]

> not enthusiastic about seeing this presented as a fait accompli in 
> generic driver code, rather than under arch/x86/moorestown.

Certainly wouldn't be a real problem if it started there and migrated
if/when it proves to be more generic.

Alan

Re: [RFC/PATCH 2.6.32] Simple Firmware Interface (SFI): initial support

[Len Brown]

> > not enthusiastic about seeing this presented as a fait accompli in 
> > generic driver code, rather than under arch/x86/moorestown.
> 
> Certainly wouldn't be a real problem if it started there and migrated
> if/when it proves to be more generic.

There are no drivers in drivers/sfi/, nor will there ever be.

I followed the broken topology used by ACPI, which has both drivers
and non-driver-platform-dependent-code in drivers/acpi/.

I did propose fixing ACPI at one point, but there was no consensus
on where platform dependent non-driver code that runs on multiple
architectures should live.  (eg, where to put drivers/acpi/acpica/*)

SFI is largely in the same boat.  Though one could argue
that all of SFI could live under arch/x86 someplace,
though it will be accessed by more than moorestown specific code.

cheers,
-Len Brown, Intel Open Source Technology Center

Re: [RFC/PATCH 2.6.32] Simple Firmware Interface (SFI): initial support

[Len Brown]

On Tue, 23 Jun 2009, Matthew Garrett wrote:

> I think I've got a clearer understanding of my objections to this now. 
> The first is that SFI is designed to support the subset of information 
> that's in ACPI and which can't be intuited by the OS. However, that 
> subset is predicated on the system looking like Moorestown. A system 
> that wants to provide any information beyond that subset can't use SFI 
> unless it defines additional tables.

Correct.

Per my previous message...

Should a platform require them, any and all of the ACPI
defined/reserved tables can be accessed on an SFI system
if needed.  Today, the PCI MCFG is the only ACPI table implemented
in the known universe of SFI systems.

WRT to native SFI table names...

int sfi_table_parse(char *signature, char *oem_id, char *oem_table_id,
                        uint flag, sfi_table_handler handler);

While the invocations in the tree today have NULL oem_id
and NULL oem_table_id, it is possible for a vendor to
stick their own name in there with their own table_id
and get the OS or a driver to find their home-brewed table
without reserving a table signature to the SFI spec.

However, should overwhemling demand for table signatures materialize,
I'm sure that a process can be put in place to manage collisions...

> And that brings me onto my second issue. ACPI is sufficiently 
> generalised that there's little need for vendors to add additional 
> tables. SFI isn't, and so vendor adoption is going to require 
> vendor-specific tables. This potentially results in SFI bloating out to 
> cover much of the functionality of ACPI, while at the same time turning 
> into a namespacing nightmare. Without a formal process for adding new 
> tables and without any kind of certification requirements before 
> claiming SFI compatibility, we're looking at a real risk of collisions.

The SFI table signatures are totally independent of the ACPI table
signatures -- they can not collide.  The only overlap is the XSDT
itself, which exists in SFI explicitly to separate the ACPI namespace
from the SFI namespace.

> SFI appears to be presented as a generic firmware interface, but in 
> reality it's currently tightly wed to Moorestown and I don't see any way 
> that that can be fixed without reinventing chunks of ACPI. I'm certainly 
> not enthusiastic about seeing this presented as a fait accompli in 
> generic driver code, rather than under arch/x86/moorestown.

SFI was invented with the goal to help a somewhat generic 
distro-supported kernel to boot and run optimally
on the Moorestown platform.  If SFI helps enable Moorestown
to participate in even just a small part of the vibrant
x86 open source development community, than it has been successful.

Yes, SFI is written to be "generic enough" so that it can be
used by more than Moorestown.  It is not trying to displace ACPI
on systems that are willing and able to support ACPI, but it
is freely available for those platforms that can't.

cheers,
-Len Brown, Intel Open Source Technology Center

Re: [RFC/PATCH 2.6.32] Simple Firmware Interface (SFI): initial support

[Matthew Garrett]

On Tue, Jun 23, 2009 at 06:20:11PM -0400, Len Brown wrote:
> On Tue, 23 Jun 2009, Matthew Garrett wrote:
> 
> > I think I've got a clearer understanding of my objections to this now. 
> > The first is that SFI is designed to support the subset of information 
> > that's in ACPI and which can't be intuited by the OS. However, that 
> > subset is predicated on the system looking like Moorestown. A system 
> > that wants to provide any information beyond that subset can't use SFI 
> > unless it defines additional tables.
> 
> Correct.
> 
> Per my previous message...
> 
> Should a platform require them, any and all of the ACPI
> defined/reserved tables can be accessed on an SFI system
> if needed.  Today, the PCI MCFG is the only ACPI table implemented
> in the known universe of SFI systems.

Right. But you've already got a potential conflict when it comes to 
wrapping the MADT - if someone wants a greater set of APIC information 
than you can provide via the SFI APIC table they're going to hit 
interesting problems.

> WRT to native SFI table names...
> 
> int sfi_table_parse(char *signature, char *oem_id, char *oem_table_id,
>                         uint flag, sfi_table_handler handler);
> 
> While the invocations in the tree today have NULL oem_id
> and NULL oem_table_id, it is possible for a vendor to
> stick their own name in there with their own table_id
> and get the OS or a driver to find their home-brewed table
> without reserving a table signature to the SFI spec.

Ok, that looks plausible. Can this be added to the spec as a best 
practice?

> However, should overwhemling demand for table signatures materialize,
> I'm sure that a process can be put in place to manage collisions...

Given the potential for vendors to ship customised Linux kernels, I 
think it'd be beneficial to have that in place before any hardware's 
shipped. The moment we have a collision we have a support nightmare.

> > And that brings me onto my second issue. ACPI is sufficiently 
> > generalised that there's little need for vendors to add additional 
> > tables. SFI isn't, and so vendor adoption is going to require 
> > vendor-specific tables. This potentially results in SFI bloating out to 
> > cover much of the functionality of ACPI, while at the same time turning 
> > into a namespacing nightmare. Without a formal process for adding new 
> > tables and without any kind of certification requirements before 
> > claiming SFI compatibility, we're looking at a real risk of collisions.
> 
> The SFI table signatures are totally independent of the ACPI table
> signatures -- they can not collide.  The only overlap is the XSDT
> itself, which exists in SFI explicitly to separate the ACPI namespace
> from the SFI namespace.

My concern is collisions within the SFI namespace, not collisions 
between ACPI and SFI.

> > SFI appears to be presented as a generic firmware interface, but in 
> > reality it's currently tightly wed to Moorestown and I don't see any way 
> > that that can be fixed without reinventing chunks of ACPI. I'm certainly 
> > not enthusiastic about seeing this presented as a fait accompli in 
> > generic driver code, rather than under arch/x86/moorestown.
> 
> SFI was invented with the goal to help a somewhat generic 
> distro-supported kernel to boot and run optimally
> on the Moorestown platform.  If SFI helps enable Moorestown
> to participate in even just a small part of the vibrant
> x86 open source development community, than it has been successful.
> 
> Yes, SFI is written to be "generic enough" so that it can be
> used by more than Moorestown.  It is not trying to displace ACPI
> on systems that are willing and able to support ACPI, but it
> is freely available for those platforms that can't.

If SFI's intended to support non-Moorestown platforms then I think we 
need to spend some more time determining what a non-Moorestown SFI 
implementation would look like, what changes would be required in the 
kernel to support that and how to ensure that vendors do this in a 
manner that doesn't make it likely that we'll have incompatible 
impleentations. If it's not then I think the Kconfig and documentation 
need to make that clearer.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

RE: [SFI-devel] [RFC/PATCH 2.6.32] Simple Firmware Interface (SFI): initial support

[Justen, Jordan L]

Len,

> Should a platform require them, any and all of the ACPI
> defined/reserved tables can be accessed on an SFI system
> if needed.  Today, the PCI MCFG is the only ACPI table
> implemented in the known universe of SFI systems.

When for ACPI tables are used in SFI, they retain the common
ACPI header format, including the OEM Revision, Creator ID and
Creator Revision fields unlike the other SFI structure, correct?

Regarding stripping those fields from the SFI structures,
how much space does it save in a typical system versus if they
had retained the common header format that ACPI defines?

-Jordan

-----Original Message-----
From: sfi-devel-bounces@simplefirmware.org [mailto:sfi-devel-bounces@simplefirmware.org] On Behalf Of Len Brown
Sent: Tuesday, June 23, 2009 3:20 PM
To: Matthew Garrett
Cc: sfi-devel@simplefirmware.org; linux-kernel@vger.kernel.org
Subject: Re: [SFI-devel] [RFC/PATCH 2.6.32] Simple Firmware Interface (SFI): initial support

On Tue, 23 Jun 2009, Matthew Garrett wrote:

> I think I've got a clearer understanding of my objections to this now. 
> The first is that SFI is designed to support the subset of information 
> that's in ACPI and which can't be intuited by the OS. However, that 
> subset is predicated on the system looking like Moorestown. A system 
> that wants to provide any information beyond that subset can't use SFI 
> unless it defines additional tables.

Correct.

Per my previous message...

Should a platform require them, any and all of the ACPI
defined/reserved tables can be accessed on an SFI system
if needed.  Today, the PCI MCFG is the only ACPI table implemented
in the known universe of SFI systems.

WRT to native SFI table names...

int sfi_table_parse(char *signature, char *oem_id, char *oem_table_id,
                        uint flag, sfi_table_handler handler);

While the invocations in the tree today have NULL oem_id
and NULL oem_table_id, it is possible for a vendor to
stick their own name in there with their own table_id
and get the OS or a driver to find their home-brewed table
without reserving a table signature to the SFI spec.

However, should overwhemling demand for table signatures materialize,
I'm sure that a process can be put in place to manage collisions...

> And that brings me onto my second issue. ACPI is sufficiently 
> generalised that there's little need for vendors to add additional 
> tables. SFI isn't, and so vendor adoption is going to require 
> vendor-specific tables. This potentially results in SFI bloating out to 
> cover much of the functionality of ACPI, while at the same time turning 
> into a namespacing nightmare. Without a formal process for adding new 
> tables and without any kind of certification requirements before 
> claiming SFI compatibility, we're looking at a real risk of collisions.

The SFI table signatures are totally independent of the ACPI table
signatures -- they can not collide.  The only overlap is the XSDT
itself, which exists in SFI explicitly to separate the ACPI namespace
from the SFI namespace.

> SFI appears to be presented as a generic firmware interface, but in 
> reality it's currently tightly wed to Moorestown and I don't see any way 
> that that can be fixed without reinventing chunks of ACPI. I'm certainly 
> not enthusiastic about seeing this presented as a fait accompli in 
> generic driver code, rather than under arch/x86/moorestown.

SFI was invented with the goal to help a somewhat generic 
distro-supported kernel to boot and run optimally
on the Moorestown platform.  If SFI helps enable Moorestown
to participate in even just a small part of the vibrant
x86 open source development community, than it has been successful.

Yes, SFI is written to be "generic enough" so that it can be
used by more than Moorestown.  It is not trying to displace ACPI
on systems that are willing and able to support ACPI, but it
is freely available for those platforms that can't.

cheers,
-Len Brown, Intel Open Source Technology Center

_______________________________________________
sfi-devel mailing list
sfi-devel@simplefirmware.org
http://lists.simplefirmware.org/listinfo/sfi-devel

RE: [SFI-devel] [RFC/PATCH 2.6.32] Simple Firmware Interface (SFI): initial support

[Len Brown]

On Tue, 23 Jun 2009, Justen, Jordan L wrote:

> > Should a platform require them, any and all of the ACPI
> > defined/reserved tables can be accessed on an SFI system
> > if needed.  Today, the PCI MCFG is the only ACPI table
> > implemented in the known universe of SFI systems.
> 
> When for ACPI tables are used in SFI, they retain the common
> ACPI header format, including the OEM Revision, Creator ID and
> Creator Revision fields  unlike the other SFI structure, correct?

Yes.  ACPI tables are real ACPI tables, per their spec.

> Regarding stripping those fields from the SFI structures,
> how much space does it save in a typical system versus if they
> had retained the common header format that ACPI defines?

While we do save 12-bytes per table, I don't think that is material,
even if firmware folks claim they turn backflips to save every byte...

(Though it is sort of snappy that an SFI table with
 a single 64-bit pointer fits exactly into 32-bytes;
 if density were the primary goal, we'd pack everything in the
 system into a single table with one shared header...)

So the motivation to remove unused fields was not part of
an effort to reduce total memory footprint.
They were removed because it was not possible
to justify their existence.

The benefit of the SFI header being a proper sub-set of
the ACPI header is that it allows the SFI-OS to not have a
special case to parse the XSDT.  It is both a valid SFI table
and a valid ACPI table...

However, as I've had to answer this question multiple
times, I'm thinking that it might have been less total effort
to use a table header that was arbitrarliy different
and have a special case for finding the ACPI XSDT:-)

cheers,
-Len Brown, Intel Open Source Technology Center

Re: [RFC/PATCH 2.6.32] Simple Firmware Interface (SFI): initial support

[Len Brown]

On Tue, 23 Jun 2009, Matthew Garrett wrote:

> On Tue, Jun 23, 2009 at 04:00:55PM -0400, Len Brown wrote:
> 
> > But given that the hardware is fixed (it was fixed over a year ago),
> > the question becomes what does ACPI mean on such a platform?
> > It turns out that if you look at the ACPI spec and delete all the
> > things that could not possibly apply to MRST, then you are left
> > with very little.
> 
> Right, but instead you've effectively taken ACPI, done s/XSDT/SYST/ and 
> then only supported a subset of the static tables and added some others.

sfi_acpi_table_parse(ACPI_SIG_MCFG, NULL, NULL, 0, pci_parse_mcfg);

is used today to provide access to a standard PCI-SIG-defined ACPI-wrapped
static MMCONFIG table via a standard ACPI XSDT.

No, this doesn't mean that the platform firmware supports ACPI mode.
The XSDT simply keeps the SFI table name space from conflicting
with the ACPI table name-space.

If there is a need to access another ACPI defined/reserved signature
table, the same mechanism can be used -- just fill in the signature
and the routine to parse the table.

> In return we gain two implementations to debug. I'm absolutely fine with 
> the concept of a cut-down ACPI, but I'm pretty uncomfortable with it 
> being implemented as a single-vendor spec.  Right now SFI's a 
> reimplementation of functionality we already have for the benefit of a 
> single chipset, whereas instead it could have been a refactoring of the 
> ACPI codebase to allow vendors to include whatever subset of the ACPI 
> functionality they felt necessary.

It is a fact that the list of things that SFI could have been,
and the list of things that SFI is not,
are _both_ much larger than the list of things it is.

cheers,
-Len Brown, Intel Open Source Technology Center