This patchset adds an IMR driver to the kernel plus platform code for
Intel Galileo Gen1/Gen2 boards.
IMRs:
Quark SoC X1000 ships with a set of registers called Isolated Memory Regions
IMRs provide fine grained memory access control to various system agents
within the SoC such as CPU SMM/non-SMM mode, PCIe virtual channels, CPU snoop
cycles, eSRAM flush cycles and the RMU. In simple terms, IMRs provide a
mechanism to protect memory regions from unwarranted access by system agents
that should not have access to that memory.
IMRs support a lock bit. Once a lock bit is set for an individual IMR it is
not possible to tear down that IMR without performing a cold boot of the
system. IMRs support reporting of violations. The SoC system can be
configured to reboot immediately when an IMR violation has taken place.
Immediate reboot of the system on IMR violation is recommended and is
currently how Quark BIOS configures the system.
As an example Galileo boards ship with an IMR around the ACPI runtime
services memory and if a DMA read/write cycle were to occur to this region
of memory this would trigger the IMR violation mechansim.
Galileo:
Intel's Arduino compatible Galileo boards boot to Linux with IMRs protecting
the compressed kernel image and boot params data structure. The memory that
the compressed kernel and boot params data structure is in, is marked as
usable memory by the EFI memory map. As a result it is possible for memory
marked as processor read/write only in an IMR to be given to devices in the
SoC for the purposes of DMA by way of dma_alloc_coherent.
A DMA to a region of memory by a system agent which is not allowed access
this memory result in a system reset. Without tearing down the IMRs placed
around the compressed kernel image and boot params data structure there is a
high risk of triggering an inadvertent system reset when performing DMA
actions with any of the peripherals that support DMA in Quark such as the
MMC, Ethernet or USB host/device.
Therefore Galileo specific platform code is the second component of this
patchset. The platform code tears-down every unlocked IMR to ensure no
conflict exists between the IMR usage during boot and the EFI memory map. In
addition an IMR is placed around the kernel's .text section to ensure no
invalid access to kernel code can happen by way of spurious DMA, SMM or RMU
read/write cycles. This code gets compiled into the kernel because we want
to run the code early before any DMA has taken place. The prime examples of
DMA transactions resetting the system are mouting a root filesystem on MMC
or mouting a root filesystem over NFS.
Bryan O'Donoghue (2):
x86: Add Isolated Memory Regions for Quark X1000
platform/x86 Add Intel Galileo platform specific setup
arch/x86/Kconfig | 23 ++
arch/x86/include/asm/imr.h | 79 ++++++
arch/x86/include/asm/intel-quark.h | 31 ++
arch/x86/kernel/Makefile | 1 +
arch/x86/kernel/imr.c | 529 +++++++++++++++++++++++++++++++++++
drivers/platform/x86/Kconfig | 15 +
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/intel_galileo.c | 175 ++++++++++++
8 files changed, 854 insertions(+)
create mode 100644 arch/x86/include/asm/imr.h
create mode 100644 arch/x86/include/asm/intel-quark.h
create mode 100644 arch/x86/kernel/imr.c
create mode 100644 drivers/platform/x86/intel_galileo.c
--
1.9.1
Intel Galileo Gen1 and Gen2 boot to Linux from EFI and grub with IMR
registers enabled around the compressed kernel image and boot params data
structure.
The purpose of the IMRs around the compressed kernel and boot params data
structure is to ensure that no spurious data writes from any agent within
the Quark system can corrupt the kernel/boot-params data during boot.
The kernel needs to tear-down the IMRs placed around the compressed kernel
image and boot-params data structure since the EFI memory map marks the two
regions of memory as usable memory and the kernel will happily reuse these
memory regions. Without tearing down the boot-time IMRs drivers run the
significant risk of violating one of the stale IMRs since dma_alloc_coherent
can hand addresses to DMA capable south cluster peripherals such as the SD,
Ethernet, USB host/device, which will then cause an IMR access violation
when a DMA read/write occurs to the address ranges in question.
Since the Quark EFI bringup code configures the system to reset on an IMR
violation, this means that common operations such as mouting an SD based
root filesystem, communicating with a USB device or sending Ethernet traffic
can cause an immediate system reset.
IMR usage during system boot on Galileo is detailed in
Quark_SecureBoot_PRM_330234_001.pdf. This document details each IMR used
during the boot process and the data being protected by that IMR. The kernel
needs tidy-up IMRs used during the boot process to ensure an IMR violation
doesn't cause a system reset.
This platform code does two things.
Firstly it tears down the boot-time IMRs used to protect the compressed
kernel image and boot params data structure.
Secondly it sets up an IMR around the kernel's text section from &_sinittext
- &_text ensuring that on the CPU in non-SMM mode can read/write this
address range. A spurious DMA write to the kernel's .text section will
then cause an IMR violation and system reset, consistent with the current
Galileo BSP behaviour.
Signed-off-by: Bryan O'Donoghue <[email protected]>
---
drivers/platform/x86/Kconfig | 15 +++
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/intel_galileo.c | 175 +++++++++++++++++++++++++++++++++++
3 files changed, 191 insertions(+)
create mode 100644 drivers/platform/x86/intel_galileo.c
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 638e7970..e384dcd 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -804,6 +804,21 @@ config INTEL_OAKTRAIL
enable/disable the Camera, WiFi, BT etc. devices. If in doubt, say Y
here; it will only load on supported platforms.
+config INTEL_GALILEO
+ bool "Intel Galileo Platform Support"
+ depends on X86_32 && PCI
+ select IOSF_MBI
+ select IMR
+ ---help---
+ Intel Galileo platform support. This code is used to tear-down
+ BIOS and Grub Isolated Memory Regions used during bootup.
+ This sanitises the IMR memory map to agree with the EFI/e820
+ memory map, without this code your IMR memory map will conflict
+ with the EFI memory map and it's highly likely DMA accesses initiated
+ by Ethernet, SD and/or USB will result in a system reset.
+
+ If in doubt, say Y here, the code will only run on a Galileo Gen1/Gen2
+
config SAMSUNG_Q10
tristate "Samsung Q10 Extras"
depends on ACPI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index f82232b..a0c013d 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_SAMSUNG_LAPTOP) += samsung-laptop.o
obj-$(CONFIG_MXM_WMI) += mxm-wmi.o
obj-$(CONFIG_INTEL_MID_POWER_BUTTON) += intel_mid_powerbtn.o
obj-$(CONFIG_INTEL_OAKTRAIL) += intel_oaktrail.o
+obj-$(CONFIG_INTEL_GALILEO) += intel_galileo.o
obj-$(CONFIG_SAMSUNG_Q10) += samsung-q10.o
obj-$(CONFIG_APPLE_GMUX) += apple-gmux.o
obj-$(CONFIG_INTEL_RST) += intel-rst.o
diff --git a/drivers/platform/x86/intel_galileo.c b/drivers/platform/x86/intel_galileo.c
new file mode 100644
index 0000000..2a555aa
--- /dev/null
+++ b/drivers/platform/x86/intel_galileo.c
@@ -0,0 +1,175 @@
+/*
+ * intel_galileo.c - Intel Galileo platform support.
+ *
+ * Copyright(c) 2013 Intel Corporation.
+ * Copyright(c) 2014 Bryan O'Donoghue <[email protected]>
+ *
+ * This platform code provides an entry point to do Galileo specific
+ * setup. Critically IMRs are policed to ensure the EFI provided memory
+ * map informing the kernel of it's available memory is consistent with
+ * the IMR lock-down
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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.
+ */
+#include <asm-generic/sections.h>
+#include <asm/imr.h>
+#include <asm/intel-quark.h>
+#include <linux/dmi.h>
+#include <linux/module.h>
+#include <linux/mm.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+
+enum {
+ GALILEO_UNKNOWN = 0,
+ GALILEO_QRK_GEN1,
+ GALILEO_QRK_GEN2,
+};
+
+static struct dmi_system_id galileo_baseboards[] = {
+ {
+ .ident = "Galileo",
+ .matches = {
+ DMI_MATCH(DMI_BOARD_VENDOR, "Intel"),
+ DMI_MATCH(DMI_BOARD_NAME, "Galileo"),
+ },
+ .driver_data = (void *)GALILEO_QRK_GEN1,
+ },
+ {
+ .ident = "GalileoGen2",
+ .matches = {
+ DMI_MATCH(DMI_BOARD_VENDOR, "Intel"),
+ DMI_MATCH(DMI_BOARD_NAME, "GalileoGen2"),
+ },
+ .driver_data = (void *)GALILEO_QRK_GEN2,
+ },
+ {}
+};
+
+#ifdef DEBUG
+#define SANITY "IMR : sanity error "
+
+/**
+ * intel_galileo_imr_sanity
+ * Verify IMR sanity with some simple tests to verify
+ * overlap, zero sized allocations
+ *
+ * @base: Physical base address of the kernel text section
+ * @size: Extent of kernel memory to be covered from &_text to &_sinittext
+ * @return: none
+ */
+static void __init
+intel_galileo_imr_sanity(unsigned long base, unsigned long size)
+{
+ /* Test zero zero */
+ if (imr_add(0, 0, 0, 0, true) == 0)
+ pr_err(SANITY "zero sized IMR @ 0x00000000\n");
+
+ /* Test overlap */
+ if (imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true) == 0)
+ pr_err(SANITY "overlapped IMR @ (0x%08lx - 0x%08lx)\n",
+ base, base + size);
+
+ /* Try overlap - IMR_ALIGN */
+ base = base + size - IMR_ALIGN;
+ if (imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true) == 0)
+ pr_err(SANITY "overlapped IMR @ (0x%08lx - 0x%08lx)\n",
+ base, base + size);
+}
+#endif
+
+/**
+ * intel_galileo_imr_init
+ *
+ * Tear down IMRs used during bootup. BIOS and Grub
+ * both setup IMRs around compressed kernel, initrd memory
+ * that need to be removed before the kernel hands out one
+ * of the IMR encased addresses to a downstream DMA agent
+ * such as the SD or Ethernet. IMRs on Galileo are setup to
+ * immediately reset the system on violation - so if you're
+ * running a root filesystem from SD - you'll need the IMRs
+ * torn down or you'll find seemingly random resets when using
+ * your filesystem.
+ */
+static void __init intel_galileo_imr_init(void)
+{
+ unsigned long base = virt_to_phys(&_text);
+ unsigned long size = virt_to_phys(&_sinittext) - base - IMR_ALIGN;
+ int i, ret;
+
+ /* Tear down all existing unlocked IMRs */
+ for (i = 0; i <= QUARK_X1000_IMR_NUM; i++)
+ imr_del(i, 0, 0);
+
+ /*
+ * Setup an IMR around the physical extent of the kernel
+ * Non-SMM mode core read/write from/to kernel physical region.
+ * IMR locked.
+ */
+ ret = imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true);
+ if (ret)
+ pr_err("Unable to setup IMR for kernel: (%p - %p)\n",
+ &_text, &__init_begin);
+ else
+ pr_info("IMR protect kernel memory: %ldk (%p - %p)\n",
+ size >> 10, &_text, &__init_begin);
+#ifdef DEBUG
+ intel_galileo_imr_sanity(base, size);
+#endif
+}
+
+/**
+ * intel_galileo_init
+ *
+ * Identify a Galileo Gen1 or Gen2. If found run code to sanitise the
+ * kernel memory space of IMRs that are inconsistent with the EFI memory map.
+ */
+static int __init intel_galileo_init(void)
+{
+ int ret = 0, type = GALILEO_UNKNOWN;
+ struct cpuinfo_x86 *c = &cpu_data(cpu);
+ const struct dmi_system_id *system_id;
+
+ if (!cpu_is_quark(c))
+ return -ENODEV;
+
+ system_id = dmi_first_match(galileo_baseboards);
+
+ /* BIOS releases 0.7.5 and 0.8.0 do not provide DMI strings */
+ if (system_id != NULL) {
+ type = (int)system_id->driver_data;
+ } else {
+ pr_info("Galileo Gen1 BIOS version <= 0.8.0\n");
+ type = GALILEO_QRK_GEN1;
+ }
+
+ switch (type) {
+ case GALILEO_QRK_GEN1:
+ case GALILEO_QRK_GEN2:
+ intel_galileo_imr_init();
+ break;
+ default:
+ ret = -ENODEV;
+ }
+
+ return ret;
+}
+
+static void __exit intel_galileo_exit(void)
+{
+}
+
+module_init(intel_galileo_init);
+module_exit(intel_galileo_exit);
+
+MODULE_AUTHOR("Bryan O'Donoghue <[email protected]>");
+MODULE_DESCRIPTION("Intel Galileo platform driver");
+MODULE_LICENSE("GPL v2");
--
1.9.1
Intel's Quark X1000 SoC contains a set of registers called Isolated Memory
Regions. IMRs are accessed over the IOSF mailbox interface. IMRs are areas
carved out of memory that define read/write access rights to the various
system agents within the Quark system. For a given agent in the system it is
possible to specify if that agent may read or write an area of memory
defined by an IMR with a granularity of 1 kilobyte.
Quark_SecureBootPRM_330234_001.pdf section 4.5 details the concept of IMRs
eSRAM flush, CPU Snoop, CPU SMM Mode, CPU non-SMM mode, PMU and VC0/VC1 can
have individual read/write access masks applied to them for a given memory
region in Quark X1000. Quark supports eightIMR sets.
Since all of the DMA capable SoC components in the X1000 are mapped to VC0
it is possible to define sections of memory as invalid for DMA write
operations originating from Ethernet, USB, SD and any other DMA capable
south-cluster component on VC0. Similarly it is possible to mark kernel
memory as non-SMM mode read/write only or to mark BIOS runtime memory as SMM
mode accessible only depending on the particular memory footprint on a given
system.
On an IMR violation Quark SoC X1000 systems are configured to reset the
system, so ensuring that the IMR memory map agrees with the EFI provided
memory map is critical to ensure no IMR violations reset the system.
The API for accessing IMRs is based on MTRR code but doesn't provide a /proc
or /sys interface to manipulate IMRs. Defining the size and extent of IMRs
is exclusively the domain of in-kernel code.
Signed-off-by: Bryan O'Donoghue <[email protected]>
---
arch/x86/Kconfig | 23 ++
arch/x86/include/asm/imr.h | 79 ++++++
arch/x86/include/asm/intel-quark.h | 31 +++
arch/x86/kernel/Makefile | 1 +
arch/x86/kernel/imr.c | 529 +++++++++++++++++++++++++++++++++++++
5 files changed, 663 insertions(+)
create mode 100644 arch/x86/include/asm/imr.h
create mode 100644 arch/x86/include/asm/intel-quark.h
create mode 100644 arch/x86/kernel/imr.c
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ba397bd..8303ca2 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -526,6 +526,29 @@ config IOSF_MBI_DEBUG
If you don't require the option or are in doubt, say N.
+config IMR
+ tristate "Isolated Memory Region support"
+ default m
+ depends on IOSF_MBI
+ ---help---
+ This option enables support for Isolated Memory Regions.
+ IMRs are a set of registers that define read and write access masks
+ to prohibit certain system agents from accessing memory with 1k
+ granularity.
+ IMRs make it possible to control read/write access to an address
+ by hardware agents inside the SoC. Read and write masks can be
+ defined for
+ - SMM mode
+ - Non SMM mode
+ - PCI VC0/VC1
+ - CPU snoop
+ - eSRAM flush
+ - PMU access
+ Quark contains a set of IMR registers and makes use of those
+ registers during it's bootup process.
+
+ If you are running on a Galileo/Quark say Y here
+
config X86_RDC321X
bool "RDC R-321x SoC"
depends on X86_32
diff --git a/arch/x86/include/asm/imr.h b/arch/x86/include/asm/imr.h
new file mode 100644
index 0000000..fe8f3b8
--- /dev/null
+++ b/arch/x86/include/asm/imr.h
@@ -0,0 +1,79 @@
+/*
+ * imr.h: Isolated Memory Region API
+ *
+ * Copyright(c) 2013 Intel Corporation.
+ * Copyright(c) 2014 Bryan O'Donoghue <[email protected]>
+ *
+ * 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; version 2
+ * of the License.
+ */
+#ifndef _IMR_H
+#define _IMR_H
+
+#include <asm/intel-quark.h>
+#include <linux/types.h>
+
+/*
+ * Right now IMRs are not reported via CPUID though it'd be really great if
+ * future silicon did report via CPUID for this feature bringing it in-line with
+ * other similar features - like MTRRs, MSRs etc.
+ *
+ * A macro is defined here which is an analog to the other cpu_has_x type
+ * features
+ */
+#define cpu_has_imr cpu_is_quark
+
+/* IMR agent access mask bits */
+#define IMR_ESRAM_FLUSH 0x80000000
+#define IMR_CPU_SNOOP 0x40000000
+#define IMR_HB_ACCESS 0x20000000
+#define IMR_VC1_ID3 0x00008000
+#define IMR_VC1_ID2 0x00004000
+#define IMR_VC1_ID1 0x00002000
+#define IMR_VC1_ID0 0x00001000
+#define IMR_VC0_ID3 0x00000800
+#define IMR_VC0_ID2 0x00000400
+#define IMR_VC0_ID1 0x00000200
+#define IMR_VC0_ID0 0x00000100
+#define IMR_SMM 0x00000002
+#define IMR_NON_SMM 0x00000001
+#define IMR_ACCESS_NONE 0x00000000
+
+/* Definition of read/write masks from published BSP code */
+#define IMR_READ_ACCESS_ALL 0xBFFFFFFF
+#define IMR_WRITE_ACCESS_ALL 0xFFFFFFFF
+
+/* Number of IMRs provided by Quark X1000 SoC */
+#define QUARK_X1000_IMR_NUM 0x07
+#define QUARK_X1000_IMR_REGBASE 0x40
+
+/* IMR alignment bits - only bits 31:10 are checked for IMR validity */
+#define IMR_ALIGN 0x400
+#define IMR_MASK (IMR_ALIGN - 1)
+
+/**
+ * imr_add_range - Add an Isolated Memory Region
+ * @base: Physical base address of region aligned to 4k
+ * @size: Physical size of region in bytes
+ * @read_mask: Read access mask
+ * @write_mask: Write access mask
+ * @lock: Indicates whether or not to permenantly lock this region
+ * @return: Index of the IMR allocated or negative value indicating error
+ */
+int imr_add(unsigned long base, unsigned long size,
+ unsigned int rmask, unsigned int wmask, bool lock);
+
+/**
+ * imr_del_range - Delete an Isolated Memory Region
+ * @reg: IMR index to remove
+ * @base: Physical base address of region aligned to 4k
+ * @size: Physical size of region in bytes
+ * @return: -EINVAL on invalid range or out or range id
+ * -ENODEV if reg is valid but no IMR exists or is locked
+ * 0 on success
+ */
+int imr_del(int reg, unsigned long base, unsigned long size);
+
+#endif /* _IMR_H */
diff --git a/arch/x86/include/asm/intel-quark.h b/arch/x86/include/asm/intel-quark.h
new file mode 100644
index 0000000..f51ac8c
--- /dev/null
+++ b/arch/x86/include/asm/intel-quark.h
@@ -0,0 +1,31 @@
+/*
+ * intel-quark.h: Quark shared defines and helper functions
+ *
+ * Copyright 2014 Bryan O'Donoghue <[email protected]>
+ *
+ * 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; version 2
+ * of the License.
+ */
+
+#ifndef _ASM_X86_INTEL_QUARK_H
+#define _ASM_X86_INTEL_QUARK_H
+
+#include <asm/processor.h>
+
+#ifdef CONFIG_X86_32
+static inline int cpu_is_quark(const struct cpuinfo_x86 *c)
+{
+ return (c->x86_vendor == X86_VENDOR_INTEL &&
+ c->x86 == 5 && c->x86_model == 9);
+}
+#else
+static inline int cpu_is_quark(const struct cpuinfo_x86 *c)
+{
+ return 0;
+}
+#endif
+
+#endif /* _ASM_X86_INTEL_QUARK_H */
+
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 5d4502c..0252de5 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -104,6 +104,7 @@ obj-$(CONFIG_EFI) += sysfb_efi.o
obj-$(CONFIG_PERF_EVENTS) += perf_regs.o
obj-$(CONFIG_TRACING) += tracepoint.o
obj-$(CONFIG_IOSF_MBI) += iosf_mbi.o
+obj-$(CONFIG_IMR) += imr.o
obj-$(CONFIG_PMC_ATOM) += pmc_atom.o
###
diff --git a/arch/x86/kernel/imr.c b/arch/x86/kernel/imr.c
new file mode 100644
index 0000000..4115138
--- /dev/null
+++ b/arch/x86/kernel/imr.c
@@ -0,0 +1,529 @@
+/**
+ * intel_imr.c
+ *
+ * Copyright(c) 2013 Intel Corporation.
+ * Copyright(c) 2014 Bryan O'Donoghue <[email protected]>
+ *
+ * IMR registers define an isolated region of memory that can
+ * be masked to prohibit certain system agents from accessing memory.
+ * When a device behind a masked port performs an access - snooped or not, an
+ * IMR may optionally prevent that transaction from changing the state of memory
+ * or from getting correct data in response to the operation.
+ * Write data will be dropped and reads will return 0xFFFFFFFF, the system will
+ * reset and system BIOS will print out an error message to inform the user that
+ * an IMR has been violated.
+ * This code is based on the Linux MTRR code and refernece code from Intel's
+ * Quark BSP EFI, Linux and grub code.
+ */
+#include <asm/intel-quark.h>
+#include <asm/imr.h>
+#include <asm/iosf_mbi.h>
+#include <linux/debugfs.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+
+struct imr_device {
+ struct dentry *debugfs_dir;
+ struct mutex lock;
+ int num;
+ int used;
+ int reg_base;
+};
+
+static struct imr_device imr_dev;
+
+/**
+ * Values derived from published code in Quark BSP
+ *
+ * addr_lo
+ * 31 Lock bit
+ * 30 Enable bit - not implemented on Quark X1000
+ * 29:25 Reserved
+ * 24:2 1 kilobyte aligned lo address
+ * 1:0 Reserved
+ *
+ * addr_hi
+ * 31:25 Reserved
+ * 24:2 1 kilobyte aligned hi address
+ * 1:0 Reserved
+ */
+#define IMR_LOCK 0x80000000
+#define IMR_ENABLE 0x04000000
+
+struct imr {
+ u32 addr_lo;
+ u32 addr_hi;
+ u32 rmask;
+ u32 wmask;
+};
+
+#define IMR_NUM_REGS (sizeof(struct imr)/sizeof(u32))
+#define IMR_SHIFT 8
+#define imr_to_phys(x) (x << IMR_SHIFT)
+#define phys_to_imr(x) (x >> IMR_SHIFT)
+
+/**
+ * imr_enabled
+ * Determines if an IMR is enabled based on address range
+ *
+ * @imr: Pointer to IMR descriptor
+ * @return true if IMR enabled false if disabled
+ */
+static int imr_enabled(struct imr *imr)
+{
+ return (imr_to_phys(imr->addr_lo) && imr_to_phys(imr->addr_hi));
+}
+
+/**
+ * imr_read
+ * Read an IMR at a given imr index. Requires caller to hold imr mutex
+ *
+ * @imr_id: IMR entry to read
+ * @imr: IMR structure representing address and access masks
+ * @return 0 on success or error code passed from mbi_iosf on failure
+ */
+static int imr_read(u32 imr_id, struct imr *imr)
+{
+ u32 reg = imr_dev.reg_base + (imr_id * IMR_NUM_REGS);
+ int ret;
+
+ ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ,
+ reg, &imr->addr_lo);
+ if (ret)
+ return ret;
+
+ ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ,
+ ++reg, &imr->addr_hi);
+ if (ret)
+ return ret;
+
+ ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ,
+ ++reg, &imr->rmask);
+ if (ret)
+ return ret;
+
+ return iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ,
+ ++reg, &imr->wmask);
+}
+
+/**
+ * imr_write
+ * Write an IMR at a given imr index. Requires caller to hold imr mutex
+ * Note lock bits need to be written independently of address bits
+ *
+ * @imr_id: IMR entry to write
+ * @imr: IMR structure representing address and access masks
+ * @lock: Indicates if the IMR lock bit should be applied
+ * @return 0 on success or error code passed from mbi_iosf on failure
+ */
+static int imr_write(u32 imr_id, struct imr *imr, bool lock)
+{
+ unsigned long flags;
+ u32 reg = imr_dev.reg_base + (imr_id * IMR_NUM_REGS);
+ u32 reg_lock = reg;
+ int ret;
+
+ local_irq_save(flags);
+
+ ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE, reg,
+ imr->addr_lo);
+ if (ret)
+ goto done;
+
+ ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE,
+ ++reg, imr->addr_hi);
+ if (ret)
+ goto done;
+
+ ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE,
+ ++reg, imr->rmask);
+ if (ret)
+ goto done;
+
+ ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE,
+ ++reg, imr->wmask);
+ if (ret)
+ goto done;
+
+ /* Lock bit must be set separately to addr_lo address bits */
+ if (lock) {
+ imr->addr_lo |= IMR_LOCK;
+ ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE,
+ reg_lock, imr->addr_lo);
+ }
+
+done:
+ local_irq_restore(flags);
+
+ /*
+ * If writing to the IOSF failed then we're in an unknown state
+ * likely a very bad state. An IMR in an invalid state will almost
+ * certainly lead to a memory access violation.
+ */
+ if (ret)
+ WARN(1, "IOSF-MBI write fail range 0x%08x-0x%08x unreliable\n",
+ imr_to_phys(imr->addr_lo),
+ imr_to_phys(imr->addr_hi) + IMR_MASK);
+
+ return ret;
+}
+
+#ifdef CONFIG_DEBUG_FS
+
+/**
+ * imr_dbgfs_state_show
+ * Print state of IMR registers
+ *
+ * @s: pointer to seq_file for output
+ * @unused: unused parameter
+ * @return 0 on success or error code passed from mbi_iosf on failure
+ */
+static int imr_dbgfs_state_show(struct seq_file *s, void *unused)
+{
+ int i, ret = -ENODEV;
+ struct imr imr;
+ u32 size;
+
+ mutex_lock(&imr_dev.lock);
+
+ for (i = 0; i <= imr_dev.num; i++) {
+
+ ret = imr_read(i, &imr);
+ if (ret)
+ break;
+
+ /*
+ * Remember to add IMR_ALIGN bytes to size to indicate the
+ * inherent IMR_ALIGN size bytes contained in the masked away
+ * lower ten bits
+ */
+ size = 0;
+ if (imr_enabled(&imr)) {
+ size = imr_to_phys(imr.addr_hi) -
+ imr_to_phys(imr.addr_lo);
+ size += IMR_ALIGN;
+ }
+ seq_printf(s, "imr%02i: base=0x%08x, end=0x%08x, size=0x%08x "
+ "rmask=0x%08x, wmask=0x%08x, %s, %s\n", i,
+ imr_to_phys(imr.addr_lo),
+ imr_to_phys(imr.addr_hi) + IMR_MASK, size,
+ imr.rmask, imr.wmask,
+ imr_enabled(&imr) ? "enabled " : "disabled",
+ imr.addr_lo & IMR_LOCK ? "locked" : "unlocked");
+ }
+
+ mutex_unlock(&imr_dev.lock);
+
+ return ret;
+}
+
+/**
+ * imr_state_open
+ * Debugfs open callback
+ *
+ * @inode: pointer to struct inode
+ * @file: pointer to struct file
+ * @return result of single open
+ */
+static int imr_state_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, imr_dbgfs_state_show, inode->i_private);
+}
+
+static const struct file_operations imr_state_ops = {
+ .open = imr_state_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
+/**
+ * imr_debugfs_register
+ * Register debugfs hooks
+ *
+ * @imr: IMR structure representing address and access masks
+ * @return 0 on success - errno on failure
+ */
+static int imr_debugfs_register(struct imr_device *imr_dev)
+{
+ struct dentry *dir, *f;
+
+ dir = debugfs_create_dir("imr", NULL);
+ if (!dir)
+ return -ENOMEM;
+
+ f = debugfs_create_file("state", S_IFREG | S_IRUGO,
+ dir, imr_dev, &imr_state_ops);
+ if (!f)
+ goto err;
+
+ imr_dev->debugfs_dir = dir;
+
+ return 0;
+err:
+ return -ENODEV;
+}
+
+/**
+ * imr_debugfs_unregister
+ * Unregister debugfs hooks
+ *
+ * @imr: IMR structure representing address and access masks
+ * @return none
+ */
+static void imr_debugfs_unregister(struct imr_device *imr_dev)
+{
+ if (!imr_dev->debugfs_dir)
+ return;
+
+ debugfs_remove_recursive(imr_dev->debugfs_dir);
+ imr_dev->debugfs_dir = NULL;
+}
+
+#else
+
+/**
+ * imr_debugfs_register
+ * Register debugfs hooks
+ *
+ * @imr: IMR structure representing address and access masks
+ * @return 0 on success - errno on failure
+ */
+static int imr_debugfs_register(struct imr_device *imr_dev)
+{
+ return 0;
+}
+
+/**
+ * imr_debugfs_unregister
+ * Dummy hook for !CONFIG_DEBUG_FS
+ *
+ * @imr: IMR structure representing address and access masks
+ * @return none
+ */
+static void imr_debugfs_unregister(struct imr_device *imr_dev)
+{
+}
+
+#endif /* CONFIG_DEBUG_FS */
+
+/**
+ * imr_check_range
+ * Check the passed address range for an IMR is aligned
+ *
+ * @base: base address of intended IMR
+ * @size: size of intended IMR
+ * @return zero on valid range -EINVAL on unaligned base/size
+ */
+static int imr_check_range(unsigned long base, unsigned long size)
+{
+ if ((base & (IMR_MASK)) || (size & (IMR_MASK))) {
+ pr_warn("imr: base 0x%08lx size 0x%08lx must align to 1k\n",
+ base, size);
+ dump_stack();
+ return -EINVAL;
+ }
+ return 0;
+}
+
+/**
+ * imr_add_range
+ * Add an Isolated Memory Region
+ *
+ * @base: Physical base address of region aligned to 4k
+ * @size: Physical size of region in bytes
+ * @read_mask: Read access mask
+ * @write_mask: Write access mask
+ * @lock: Indicates whether or not to permenantly lock this region
+ * @return: Index of the IMR allocated or negative value indicating error
+ */
+int imr_add(unsigned long base, unsigned long size,
+ unsigned int rmask, unsigned int wmask, bool lock)
+{
+ unsigned long end = base + size;
+ struct imr imr;
+ int reg, i, overlap, ret;
+
+ if (imr_check_range(base, size))
+ return -EINVAL;
+
+ if (!size) {
+ pr_warn("imr: invalid size zero!\n");
+ return -EINVAL;
+ }
+
+ mutex_lock(&imr_dev.lock);
+
+ /* Find an free IMR while checking for an existing overlapping range */
+ overlap = 0;
+ reg = -1;
+ for (i = 0; i <= imr_dev.num; i++) {
+ ret = imr_read(i, &imr);
+ if (ret)
+ goto done;
+
+ /* Find overlap */
+ if (imr_enabled(&imr)) {
+ if (base >= imr_to_phys(imr.addr_lo) &&
+ base <= imr_to_phys(imr.addr_hi)) {
+ overlap = 1;
+ break;
+ }
+ if (end >= imr_to_phys(imr.addr_lo) &&
+ end <= imr_to_phys(imr.addr_hi)) {
+ overlap = 1;
+ break;
+ }
+ } else {
+ reg = i;
+ }
+ }
+
+ /* Error out if we have an overlap or no free IMR entries */
+ if (overlap) {
+ ret = -EINVAL;
+ goto done;
+ }
+ if (reg == -1) {
+ ret = -ENODEV;
+ goto done;
+ }
+
+ pr_debug("IMR %d phys 0x%08lx-0x%08lx rmask 0x%08x wmask 0x%08x\n",
+ reg, base, end, rmask, wmask);
+
+ /* Allocate IMR */
+ imr.addr_lo = IMR_ENABLE | phys_to_imr(base);
+ imr.addr_hi = phys_to_imr(end);
+ imr.rmask = rmask;
+ imr.wmask = wmask;
+
+ ret = imr_write(reg, &imr, lock);
+
+done:
+ mutex_unlock(&imr_dev.lock);
+ return ret;
+}
+EXPORT_SYMBOL(imr_add);
+
+/**
+ * imr_del_range
+ * Delete an Isolated Memory Region
+ *
+ * @reg: IMR index to remove
+ * @base: Physical base address of region aligned to 4k
+ * @size: Physical size of region in bytes
+ * @return: -EINVAL on invalid range or out or range id
+ * -ENODEV if reg is valid but no IMR exists or is locked
+ * 0 on success
+ */
+int imr_del(int reg, unsigned long base, unsigned long size)
+{
+ struct imr imr;
+ int found = 0, i, ret = 0;
+ unsigned long max = base + size;
+
+ if (imr_check_range(base, size))
+ return -EINVAL;
+
+ if (reg > imr_dev.num)
+ return -EINVAL;
+
+ mutex_lock(&imr_dev.lock);
+
+ /* if a specific IMR is given try to use it */
+ if (reg >= 0) {
+ ret = imr_read(reg, &imr);
+ if (ret)
+ goto done;
+
+ if (!imr_enabled(&imr) || imr.addr_lo & IMR_LOCK) {
+ ret = -ENODEV;
+ goto done;
+ }
+ found = 1;
+ }
+
+ /* search for match based on address range */
+ for (i = 0; i <= imr_dev.num && found == 0; i++) {
+ ret = imr_read(reg, &imr);
+ if (ret)
+ goto done;
+
+ if (!imr_enabled(&imr) || imr.addr_lo & IMR_LOCK)
+ continue;
+
+ if ((imr_to_phys(imr.addr_lo) == base) &&
+ (imr_to_phys(imr.addr_hi) == max)) {
+ found = 1;
+ reg = i;
+ }
+ }
+
+ if (found == 0) {
+ ret = -ENODEV;
+ goto done;
+ }
+
+ /* Tear down the IMR */
+ imr.addr_lo = 0;
+ imr.addr_hi = 0;
+ imr.rmask = IMR_READ_ACCESS_ALL;
+ imr.wmask = IMR_WRITE_ACCESS_ALL;
+
+ ret = imr_write(reg, &imr, false);
+
+done:
+ mutex_unlock(&imr_dev.lock);
+ return ret;
+}
+EXPORT_SYMBOL(imr_del);
+
+/**
+ * intel_imr_probe
+ * entry point for IMR driver
+ *
+ * return: -ENODEV for no IMR support 0 if good to go
+ */
+static int __init intel_imr_init(void)
+{
+ struct cpuinfo_x86 *c = &cpu_data(cpu);
+
+ if (!cpu_has_imr(c))
+ return -ENODEV;
+
+ if (iosf_mbi_available() == false)
+ return -ENODEV;
+
+ if (cpu_is_quark(c)) {
+ imr_dev.num = QUARK_X1000_IMR_NUM;
+ imr_dev.reg_base = QUARK_X1000_IMR_REGBASE;
+ } else {
+ pr_err("Unknown IMR implementation\n");
+ return -ENODEV;
+ }
+
+ mutex_init(&imr_dev.lock);
+
+ return imr_debugfs_register(&imr_dev);
+}
+
+/**
+ * intel_imr_exit
+ * exit point for IMR code. Deregisters debugfs, leave IMR state as-is.
+ *
+ * return: none
+ */
+static void __exit intel_imr_exit(void)
+{
+ imr_debugfs_unregister(&imr_dev);
+}
+
+module_init(intel_imr_init);
+module_exit(intel_imr_exit);
+
+MODULE_AUTHOR("Bryan O'Donoghue <[email protected]>");
+MODULE_DESCRIPTION("Intel Isolated Memory Region driver");
+MODULE_LICENSE("GPL");
--
1.9.1
On Mon, Dec 29, 2014 at 7:23 PM, Bryan O'Donoghue
<[email protected]> wrote:
> This patchset adds an IMR driver to the kernel plus platform code for
> Intel Galileo Gen1/Gen2 boards.
[]
> Bryan O'Donoghue (2):
> x86: Add Isolated Memory Regions for Quark X1000
> platform/x86 Add Intel Galileo platform specific setup
I'm going to review this soon, but here few comments below.
> arch/x86/Kconfig | 23 ++
> arch/x86/include/asm/imr.h | 79 ++++++
> arch/x86/include/asm/intel-quark.h | 31 ++
Could it be just a quark.h? Like for ce4100.
Those intel- prefixes in the modules looks awkward especially when
pathname consists x86 already.
> arch/x86/kernel/Makefile | 1 +
> arch/x86/kernel/imr.c | 529 +++++++++++++++++++++++++++++++++++
> drivers/platform/x86/Kconfig | 15 +
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/intel_galileo.c | 175 ++++++++++++
Here what about to make an hierarchy like:
intel/galileo.c
intel/mid/... would be those modules with intel_mid_ prefixes in
future. See my proposal regarding to drivers/mfd [1]
> 8 files changed, 854 insertions(+)
> create mode 100644 arch/x86/include/asm/imr.h
> create mode 100644 arch/x86/include/asm/intel-quark.h
> create mode 100644 arch/x86/kernel/imr.c
> create mode 100644 drivers/platform/x86/intel_galileo.c
[1] http://www.spinics.net/lists/kernel/msg1886819.html
--
With Best Regards,
Andy Shevchenko
On 31/12/14 10:12, Andy Shevchenko wrote:
> On Mon, Dec 29, 2014 at 7:23 PM, Bryan O'Donoghue
> <[email protected]> wrote:
>> This patchset adds an IMR driver to the kernel plus platform code for
>> Intel Galileo Gen1/Gen2 boards.
>
> []
>
>> Bryan O'Donoghue (2):
>> x86: Add Isolated Memory Regions for Quark X1000
>> platform/x86 Add Intel Galileo platform specific setup
>
> I'm going to review this soon, but here few comments below.
>
>> arch/x86/Kconfig | 23 ++
>> arch/x86/include/asm/imr.h | 79 ++++++
>> arch/x86/include/asm/intel-quark.h | 31 ++
>
> Could it be just a quark.h? Like for ce4100.
> Those intel- prefixes in the modules looks awkward especially when
> pathname consists x86 already.
Sure thing. I agree there's no real logic to prefixing with intel.
>> arch/x86/kernel/Makefile | 1 +
>> arch/x86/kernel/imr.c | 529 +++++++++++++++++++++++++++++++++++
>> drivers/platform/x86/Kconfig | 15 +
>> drivers/platform/x86/Makefile | 1 +
>> drivers/platform/x86/intel_galileo.c | 175 ++++++++++++
>
> Here what about to make an hierarchy like:
> intel/galileo.c
> intel/mid/... would be those modules with intel_mid_ prefixes in
> future. See my proposal regarding to drivers/mfd [1]
>
>> 8 files changed, 854 insertions(+)
>> create mode 100644 arch/x86/include/asm/imr.h
>> create mode 100644 arch/x86/include/asm/intel-quark.h
>> create mode 100644 arch/x86/kernel/imr.c
>> create mode 100644 drivers/platform/x86/intel_galileo.c
>
>
> [1] http://www.spinics.net/lists/kernel/msg1886819.html
That works for me will remember to include for V2.
BOD
On Mon, Dec 29, 2014 at 7:23 PM, Bryan O'Donoghue
<[email protected]> wrote:
> Intel's Quark X1000 SoC contains a set of registers called Isolated Memory
> Regions. IMRs are accessed over the IOSF mailbox interface. IMRs are areas
> carved out of memory that define read/write access rights to the various
> system agents within the Quark system. For a given agent in the system it is
> possible to specify if that agent may read or write an area of memory
> defined by an IMR with a granularity of 1 kilobyte.
1 KiB? (Check entire patchset).
>
> Quark_SecureBootPRM_330234_001.pdf section 4.5 details the concept of IMRs
Missed dot at the end of sentence.
> eSRAM flush, CPU Snoop, CPU SMM Mode, CPU non-SMM mode, PMU and VC0/VC1 can
> have individual read/write access masks applied to them for a given memory
> region in Quark X1000. Quark supports eightIMR sets.
Missed space.
> Since all of the DMA capable SoC components in the X1000 are mapped to VC0
> it is possible to define sections of memory as invalid for DMA write
> operations originating from Ethernet, USB, SD and any other DMA capable
> south-cluster component on VC0. Similarly it is possible to mark kernel
> memory as non-SMM mode read/write only or to mark BIOS runtime memory as SMM
> mode accessible only depending on the particular memory footprint on a given
> system.
>
> On an IMR violation Quark SoC X1000 systems are configured to reset the
> system, so ensuring that the IMR memory map agrees with the EFI provided
> memory map is critical to ensure no IMR violations reset the system.
>
> The API for accessing IMRs is based on MTRR code but doesn't provide a /proc
> or /sys interface to manipulate IMRs. Defining the size and extent of IMRs
> is exclusively the domain of in-kernel code.
>
> Signed-off-by: Bryan O'Donoghue <[email protected]>
> ---
> arch/x86/Kconfig | 23 ++
> arch/x86/include/asm/imr.h | 79 ++++++
> arch/x86/include/asm/intel-quark.h | 31 +++
> arch/x86/kernel/Makefile | 1 +
> arch/x86/kernel/imr.c | 529 +++++++++++++++++++++++++++++++++++++
> 5 files changed, 663 insertions(+)
> create mode 100644 arch/x86/include/asm/imr.h
> create mode 100644 arch/x86/include/asm/intel-quark.h
> create mode 100644 arch/x86/kernel/imr.c
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index ba397bd..8303ca2 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -526,6 +526,29 @@ config IOSF_MBI_DEBUG
>
> If you don't require the option or are in doubt, say N.
>
> +config IMR
> + tristate "Isolated Memory Region support"
> + default m
> + depends on IOSF_MBI
> + ---help---
> + This option enables support for Isolated Memory Regions.
> + IMRs are a set of registers that define read and write access masks
> + to prohibit certain system agents from accessing memory with 1k
1 KiB
> + granularity.
> + IMRs make it possible to control read/write access to an address
> + by hardware agents inside the SoC. Read and write masks can be
> + defined for
> + - SMM mode
> + - Non SMM mode
> + - PCI VC0/VC1
> + - CPU snoop
> + - eSRAM flush
> + - PMU access
> + Quark contains a set of IMR registers and makes use of those
> + registers during it's bootup process.
> +
> + If you are running on a Galileo/Quark say Y here
> +
> config X86_RDC321X
> bool "RDC R-321x SoC"
> depends on X86_32
> diff --git a/arch/x86/include/asm/imr.h b/arch/x86/include/asm/imr.h
> new file mode 100644
> index 0000000..fe8f3b8
> --- /dev/null
> +++ b/arch/x86/include/asm/imr.h
> @@ -0,0 +1,79 @@
> +/*
> + * imr.h: Isolated Memory Region API
> + *
> + * Copyright(c) 2013 Intel Corporation.
> + * Copyright(c) 2014 Bryan O'Donoghue <[email protected]>
2015 everywhere I guess.
> + *
> + * 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; version 2
> + * of the License.
> + */
> +#ifndef _IMR_H
> +#define _IMR_H
> +
> +#include <asm/intel-quark.h>
> +#include <linux/types.h>
> +
> +/*
> + * Right now IMRs are not reported via CPUID though it'd be really great if
> + * future silicon did report via CPUID for this feature bringing it in-line with
> + * other similar features - like MTRRs, MSRs etc.
> + *
> + * A macro is defined here which is an analog to the other cpu_has_x type
> + * features
> + */
> +#define cpu_has_imr cpu_is_quark
> +
> +/* IMR agent access mask bits */
> +#define IMR_ESRAM_FLUSH 0x80000000
> +#define IMR_CPU_SNOOP 0x40000000
> +#define IMR_HB_ACCESS 0x20000000
> +#define IMR_VC1_ID3 0x00008000
> +#define IMR_VC1_ID2 0x00004000
> +#define IMR_VC1_ID1 0x00002000
> +#define IMR_VC1_ID0 0x00001000
> +#define IMR_VC0_ID3 0x00000800
> +#define IMR_VC0_ID2 0x00000400
> +#define IMR_VC0_ID1 0x00000200
> +#define IMR_VC0_ID0 0x00000100
> +#define IMR_SMM 0x00000002
> +#define IMR_NON_SMM 0x00000001
> +#define IMR_ACCESS_NONE 0x00000000
Can it be defined via BIT(x)?
> +
> +/* Definition of read/write masks from published BSP code */
> +#define IMR_READ_ACCESS_ALL 0xBFFFFFFF
> +#define IMR_WRITE_ACCESS_ALL 0xFFFFFFFF
> +
> +/* Number of IMRs provided by Quark X1000 SoC */
> +#define QUARK_X1000_IMR_NUM 0x07
> +#define QUARK_X1000_IMR_REGBASE 0x40
> +
> +/* IMR alignment bits - only bits 31:10 are checked for IMR validity */
> +#define IMR_ALIGN 0x400
> +#define IMR_MASK (IMR_ALIGN - 1)
> +
> +/**
> + * imr_add_range - Add an Isolated Memory Region
> + * @base: Physical base address of region aligned to 4k
> + * @size: Physical size of region in bytes
> + * @read_mask: Read access mask
> + * @write_mask: Write access mask
> + * @lock: Indicates whether or not to permenantly lock this region
It would be nice to indent descriptions after colon and use small
letter at the beginning. (Check entire patchset)
> + * @return: Index of the IMR allocated or negative value indicating error
Usually it goes as a separate section called Return like:
* Return:
* foo if A, otherwise bar.
> + */
Entire comment block should be in *.c file only.
> +int imr_add(unsigned long base, unsigned long size,
Leave _range suffix here and in the other one. It would be better I think.
> + unsigned int rmask, unsigned int wmask, bool lock);
> +
> +/**
> + * imr_del_range - Delete an Isolated Memory Region
> + * @reg: IMR index to remove
> + * @base: Physical base address of region aligned to 4k
> + * @size: Physical size of region in bytes
> + * @return: -EINVAL on invalid range or out or range id
> + * -ENODEV if reg is valid but no IMR exists or is locked
> + * 0 on success
> + */
> +int imr_del(int reg, unsigned long base, unsigned long size);
Same comments as for add_range.
Could it be imr_remove_range() ?
> +
> +#endif /* _IMR_H */
> diff --git a/arch/x86/include/asm/intel-quark.h b/arch/x86/include/asm/intel-quark.h
> new file mode 100644
> index 0000000..f51ac8c
> --- /dev/null
> +++ b/arch/x86/include/asm/intel-quark.h
> @@ -0,0 +1,31 @@
> +/*
> + * intel-quark.h: Quark shared defines and helper functions
> + *
> + * Copyright 2014 Bryan O'Donoghue <[email protected]>
> + *
> + * 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; version 2
> + * of the License.
> + */
> +
> +#ifndef _ASM_X86_INTEL_QUARK_H
> +#define _ASM_X86_INTEL_QUARK_H
> +
> +#include <asm/processor.h>
> +
> +#ifdef CONFIG_X86_32
> +static inline int cpu_is_quark(const struct cpuinfo_x86 *c)
> +{
> + return (c->x86_vendor == X86_VENDOR_INTEL &&
> + c->x86 == 5 && c->x86_model == 9);
> +}
> +#else
> +static inline int cpu_is_quark(const struct cpuinfo_x86 *c)
> +{
> + return 0;
> +}
> +#endif
> +
> +#endif /* _ASM_X86_INTEL_QUARK_H */
Could we use x86_match_cpu() instead?
> +
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index 5d4502c..0252de5 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -104,6 +104,7 @@ obj-$(CONFIG_EFI) += sysfb_efi.o
> obj-$(CONFIG_PERF_EVENTS) += perf_regs.o
> obj-$(CONFIG_TRACING) += tracepoint.o
> obj-$(CONFIG_IOSF_MBI) += iosf_mbi.o
> +obj-$(CONFIG_IMR) += imr.o
> obj-$(CONFIG_PMC_ATOM) += pmc_atom.o
>
> ###
> diff --git a/arch/x86/kernel/imr.c b/arch/x86/kernel/imr.c
> new file mode 100644
> index 0000000..4115138
> --- /dev/null
> +++ b/arch/x86/kernel/imr.c
> @@ -0,0 +1,529 @@
> +/**
> + * intel_imr.c
> + *
> + * Copyright(c) 2013 Intel Corporation.
> + * Copyright(c) 2014 Bryan O'Donoghue <[email protected]>
> + *
> + * IMR registers define an isolated region of memory that can
> + * be masked to prohibit certain system agents from accessing memory.
> + * When a device behind a masked port performs an access - snooped or not, an
> + * IMR may optionally prevent that transaction from changing the state of memory
> + * or from getting correct data in response to the operation.
> + * Write data will be dropped and reads will return 0xFFFFFFFF, the system will
> + * reset and system BIOS will print out an error message to inform the user that
> + * an IMR has been violated.
> + * This code is based on the Linux MTRR code and refernece code from Intel's
> + * Quark BSP EFI, Linux and grub code.
> + */
> +#include <asm/intel-quark.h>
> +#include <asm/imr.h>
> +#include <asm/iosf_mbi.h>
> +#include <linux/debugfs.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +
> +struct imr_device {
> + struct dentry *debugfs_dir;
> + struct mutex lock;
> + int num;
> + int used;
> + int reg_base;
> +};
> +
> +static struct imr_device imr_dev;
> +
> +/**
Just /*.
> + * Values derived from published code in Quark BSP
> + *
> + * addr_lo
> + * 31 Lock bit
> + * 30 Enable bit - not implemented on Quark X1000
> + * 29:25 Reserved
> + * 24:2 1 kilobyte aligned lo address
> + * 1:0 Reserved
> + *
> + * addr_hi
> + * 31:25 Reserved
> + * 24:2 1 kilobyte aligned hi address
> + * 1:0 Reserved
> + */
> +#define IMR_LOCK 0x80000000
> +#define IMR_ENABLE 0x04000000
Use BIT(x).
> +
> +struct imr {
> + u32 addr_lo;
> + u32 addr_hi;
> + u32 rmask;
> + u32 wmask;
> +};
> +
> +#define IMR_NUM_REGS (sizeof(struct imr)/sizeof(u32))
> +#define IMR_SHIFT 8
> +#define imr_to_phys(x) (x << IMR_SHIFT)
> +#define phys_to_imr(x) (x >> IMR_SHIFT)
x -> (x)
> +
> +/**
> + * imr_enabled
> + * Determines if an IMR is enabled based on address range
> + *
> + * @imr: Pointer to IMR descriptor
> + * @return true if IMR enabled false if disabled
> + */
> +static int imr_enabled(struct imr *imr)
> +{
> + return (imr_to_phys(imr->addr_lo) && imr_to_phys(imr->addr_hi));
No need to surround the return value by parentheses.
> +}
> +
> +/**
> + * imr_read
> + * Read an IMR at a given imr index. Requires caller to hold imr mutex
Summary.
* imr_read - read an IMR at a given index
This one goes to the Description I guess.
* Description:
* Requires caller to hold IMR device mutex.
Same for imr_write().
> + *
> + * @imr_id: IMR entry to read
> + * @imr: IMR structure representing address and access masks
> + * @return 0 on success or error code passed from mbi_iosf on failure
> + */
> +static int imr_read(u32 imr_id, struct imr *imr)
> +{
> + u32 reg = imr_dev.reg_base + (imr_id * IMR_NUM_REGS);
No need to have parentheses.
Same for _write().
> + int ret;
> +
> + ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ,
> + reg, &imr->addr_lo);
> + if (ret)
> + return ret;
> +
> + ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ,
> + ++reg, &imr->addr_hi);
Could you use reg++ in previous line and so on?
One more idea, if you make a union inside the structure you may do
this in a loop.
struct imr {
union {
struct imr {
...
};
u32 regs[IMR_NUM_REGS];
};
}
Mostly same for _write().
> + if (ret)
> + return ret;
> +
> + ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ,
> + ++reg, &imr->rmask);
> + if (ret)
> + return ret;
> +
> + return iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ,
> + ++reg, &imr->wmask);
> +}
> +
> +/**
> + * imr_write
> + * Write an IMR at a given imr index. Requires caller to hold imr mutex
> + * Note lock bits need to be written independently of address bits
> + *
> + * @imr_id: IMR entry to write
> + * @imr: IMR structure representing address and access masks
> + * @lock: Indicates if the IMR lock bit should be applied
> + * @return 0 on success or error code passed from mbi_iosf on failure
> + */
> +static int imr_write(u32 imr_id, struct imr *imr, bool lock)
> +{
> + unsigned long flags;
> + u32 reg = imr_dev.reg_base + (imr_id * IMR_NUM_REGS);
> + u32 reg_lock = reg;
Do we need a separate variable? Would (reg - IMR_NUM_REGS) work for you?
> + int ret;
> +
> + local_irq_save(flags);
> +
> + ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE, reg,
> + imr->addr_lo);
> + if (ret)
> + goto done;
> +
> + ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE,
> + ++reg, imr->addr_hi);
> + if (ret)
> + goto done;
> +
> + ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE,
> + ++reg, imr->rmask);
> + if (ret)
> + goto done;
> +
> + ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE,
> + ++reg, imr->wmask);
> + if (ret)
> + goto done;
> +
> + /* Lock bit must be set separately to addr_lo address bits */
> + if (lock) {
> + imr->addr_lo |= IMR_LOCK;
> + ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE,
> + reg_lock, imr->addr_lo);
> + }
> +
> +done:
> + local_irq_restore(flags);
Could you do like
local_irq_restore(flags);
return 0;
done:
local_irq_restore(flags);
WARN(...)
return ret;
?
> +
> + /*
> + * If writing to the IOSF failed then we're in an unknown state
> + * likely a very bad state. An IMR in an invalid state will almost
> + * certainly lead to a memory access violation.
> + */
> + if (ret)
> + WARN(1, "IOSF-MBI write fail range 0x%08x-0x%08x unreliable\n",
> + imr_to_phys(imr->addr_lo),
> + imr_to_phys(imr->addr_hi) + IMR_MASK);
> +
> + return ret;
> +}
> +
> +#ifdef CONFIG_DEBUG_FS
> +
> +/**
> + * imr_dbgfs_state_show
> + * Print state of IMR registers
> + *
> + * @s: pointer to seq_file for output
> + * @unused: unused parameter
> + * @return 0 on success or error code passed from mbi_iosf on failure
> + */
> +static int imr_dbgfs_state_show(struct seq_file *s, void *unused)
> +{
> + int i, ret = -ENODEV;
> + struct imr imr;
> + u32 size;
> +
> + mutex_lock(&imr_dev.lock);
It seems you may get the imr_dev via parameters. I suggest to avoid
using global variables as much as possible.
> +
> + for (i = 0; i <= imr_dev.num; i++) {
num is not num, but last one? Sounds confusing for me.
> +
> + ret = imr_read(i, &imr);
> + if (ret)
> + break;
> +
> + /*
> + * Remember to add IMR_ALIGN bytes to size to indicate the
> + * inherent IMR_ALIGN size bytes contained in the masked away
> + * lower ten bits
> + */
> + size = 0;
It might be an else branch.
> + if (imr_enabled(&imr)) {
> + size = imr_to_phys(imr.addr_hi) -
> + imr_to_phys(imr.addr_lo);
Could it be one line?
> + size += IMR_ALIGN;
Could it fit this one too?
> + }
> + seq_printf(s, "imr%02i: base=0x%08x, end=0x%08x, size=0x%08x "
> + "rmask=0x%08x, wmask=0x%08x, %s, %s\n", i,
> + imr_to_phys(imr.addr_lo),
> + imr_to_phys(imr.addr_hi) + IMR_MASK, size,
> + imr.rmask, imr.wmask,
> + imr_enabled(&imr) ? "enabled " : "disabled",
> + imr.addr_lo & IMR_LOCK ? "locked" : "unlocked");
> + }
> +
> + mutex_unlock(&imr_dev.lock);
> +
> + return ret;
> +}
> +
> +/**
> + * imr_state_open
> + * Debugfs open callback
> + *
> + * @inode: pointer to struct inode
> + * @file: pointer to struct file
> + * @return result of single open
> + */
> +static int imr_state_open(struct inode *inode, struct file *file)
> +{
> + return single_open(file, imr_dbgfs_state_show, inode->i_private);
> +}
> +
> +static const struct file_operations imr_state_ops = {
> + .open = imr_state_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = single_release,
> +};
> +
> +/**
> + * imr_debugfs_register
> + * Register debugfs hooks
> + *
> + * @imr: IMR structure representing address and access masks
> + * @return 0 on success - errno on failure
> + */
> +static int imr_debugfs_register(struct imr_device *imr_dev)
> +{
> + struct dentry *dir, *f;
> +
> + dir = debugfs_create_dir("imr", NULL);
> + if (!dir)
> + return -ENOMEM;
if (IS_ERR(dir))
return PTR_ERR();
Though it seems not a case when you have this under ifdef.
> +
> + f = debugfs_create_file("state", S_IFREG | S_IRUGO,
> + dir, imr_dev, &imr_state_ops);
> + if (!f)
> + goto err;
Are you planing to extend the debugfs contents? Would it be okay to
use only one file for now?
> +
> + imr_dev->debugfs_dir = dir;
> +
> + return 0;
> +err:
> + return -ENODEV;
No need to have separate label for plain return.
> +}
> +
> +/**
> + * imr_debugfs_unregister
> + * Unregister debugfs hooks
> + *
> + * @imr: IMR structure representing address and access masks
> + * @return none
> + */
> +static void imr_debugfs_unregister(struct imr_device *imr_dev)
> +{
> + if (!imr_dev->debugfs_dir)
> + return;
Redundant check.
> +
> + debugfs_remove_recursive(imr_dev->debugfs_dir);
> + imr_dev->debugfs_dir = NULL;
No need to assign NULL.
> +}
No need to put this function under ifdef - debugfs has the stubs.
Or add ifdefs around places where they are called and remove those
dummy functions below.
> +
> +#else
> +
> +/**
> + * imr_debugfs_register
> + * Register debugfs hooks
> + *
> + * @imr: IMR structure representing address and access masks
> + * @return 0 on success - errno on failure
> + */
> +static int imr_debugfs_register(struct imr_device *imr_dev)
> +{
> + return 0;
> +}
> +
> +/**
> + * imr_debugfs_unregister
> + * Dummy hook for !CONFIG_DEBUG_FS
> + *
> + * @imr: IMR structure representing address and access masks
> + * @return none
> + */
> +static void imr_debugfs_unregister(struct imr_device *imr_dev)
> +{
> +}
> +
> +#endif /* CONFIG_DEBUG_FS */
> +
> +/**
> + * imr_check_range
> + * Check the passed address range for an IMR is aligned
> + *
> + * @base: base address of intended IMR
> + * @size: size of intended IMR
> + * @return zero on valid range -EINVAL on unaligned base/size
> + */
> +static int imr_check_range(unsigned long base, unsigned long size)
> +{
> + if ((base & (IMR_MASK)) || (size & (IMR_MASK))) {
Too many parentheses. Looks like you may do it less.
> + pr_warn("imr: base 0x%08lx size 0x%08lx must align to 1k\n",
Can you define pr_fmt() ?
1 KiB.
> + base, size);
> + dump_stack();
dump_stack is really needed here? In that case why not to use WARN()?
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +/**
> + * imr_add_range
> + * Add an Isolated Memory Region
> + *
> + * @base: Physical base address of region aligned to 4k
> + * @size: Physical size of region in bytes
> + * @read_mask: Read access mask
> + * @write_mask: Write access mask
> + * @lock: Indicates whether or not to permenantly lock this region
> + * @return: Index of the IMR allocated or negative value indicating error
> + */
> +int imr_add(unsigned long base, unsigned long size,
> + unsigned int rmask, unsigned int wmask, bool lock)
> +{
> + unsigned long end = base + size;
> + struct imr imr;
> + int reg, i, overlap, ret;
> +
> + if (imr_check_range(base, size))
> + return -EINVAL;
ret = imr_();
if (ret)
return ret;
> +
> + if (!size) {
> + pr_warn("imr: invalid size zero!\n");
> + return -EINVAL;
> + }
> +
> + mutex_lock(&imr_dev.lock);
> +
> + /* Find an free IMR while checking for an existing overlapping range */
> + overlap = 0;
> + reg = -1;
> + for (i = 0; i <= imr_dev.num; i++) {
> + ret = imr_read(i, &imr);
> + if (ret)
> + goto done;
> +
> + /* Find overlap */
> + if (imr_enabled(&imr)) {
> + if (base >= imr_to_phys(imr.addr_lo) &&
> + base <= imr_to_phys(imr.addr_hi)) {
> + overlap = 1;
> + break;
Maybe
ret = -EINVAL;
goto done;
> + }
> + if (end >= imr_to_phys(imr.addr_lo) &&
> + end <= imr_to_phys(imr.addr_hi)) {
> + overlap = 1;
> + break;
Ditto.
> + }
You may use one condition. If you still want to have them split you
may create a helper to check for overlap.
> + } else {
> + reg = i;
> + }
> + }
> +
> + /* Error out if we have an overlap or no free IMR entries */
> + if (overlap) {
> + ret = -EINVAL;
> + goto done;
> + }
...and remove overlap variable and this condition.
> + if (reg == -1) {
> + ret = -ENODEV;
> + goto done;
> + }
> +
> + pr_debug("IMR %d phys 0x%08lx-0x%08lx rmask 0x%08x wmask 0x%08x\n",
> + reg, base, end, rmask, wmask);
> +
> + /* Allocate IMR */
> + imr.addr_lo = IMR_ENABLE | phys_to_imr(base);
> + imr.addr_hi = phys_to_imr(end);
> + imr.rmask = rmask;
> + imr.wmask = wmask;
> +
> + ret = imr_write(reg, &imr, lock);
> +
> +done:
> + mutex_unlock(&imr_dev.lock);
> + return ret;
> +}
> +EXPORT_SYMBOL(imr_add);
> +
> +/**
> + * imr_del_range
> + * Delete an Isolated Memory Region
> + *
> + * @reg: IMR index to remove
> + * @base: Physical base address of region aligned to 4k
> + * @size: Physical size of region in bytes
> + * @return: -EINVAL on invalid range or out or range id
> + * -ENODEV if reg is valid but no IMR exists or is locked
> + * 0 on success
> + */
> +int imr_del(int reg, unsigned long base, unsigned long size)
> +{
> + struct imr imr;
> + int found = 0, i, ret = 0;
> + unsigned long max = base + size;
> +
> + if (imr_check_range(base, size))
> + return -EINVAL;
> +
> + if (reg > imr_dev.num)
> + return -EINVAL;
> +
> + mutex_lock(&imr_dev.lock);
> +
> + /* if a specific IMR is given try to use it */
Use capital letter to start a comment. Check a whole code.
> + if (reg >= 0) {
> + ret = imr_read(reg, &imr);
> + if (ret)
> + goto done;
> +
> + if (!imr_enabled(&imr) || imr.addr_lo & IMR_LOCK) {
> + ret = -ENODEV;
> + goto done;
> + }
> + found = 1;
You may put a loop to the else branch instead of checking found at
each iteration.
> + }
> +
> + /* search for match based on address range */
> + for (i = 0; i <= imr_dev.num && found == 0; i++) {
> + ret = imr_read(reg, &imr);
> + if (ret)
> + goto done;
> +
> + if (!imr_enabled(&imr) || imr.addr_lo & IMR_LOCK)
> + continue;
> +
> + if ((imr_to_phys(imr.addr_lo) == base) &&
> + (imr_to_phys(imr.addr_hi) == max)) {
> + reg = i;
> + found = 1;
break;
> + }
> + }
> +
> + if (found == 0) {
> + ret = -ENODEV;
> + goto done;
> + }
> +
> + /* Tear down the IMR */
> + imr.addr_lo = 0;
> + imr.addr_hi = 0;
> + imr.rmask = IMR_READ_ACCESS_ALL;
> + imr.wmask = IMR_WRITE_ACCESS_ALL;
> +
> + ret = imr_write(reg, &imr, false);
> +
> +done:
> + mutex_unlock(&imr_dev.lock);
> + return ret;
> +}
> +EXPORT_SYMBOL(imr_del);
> +
> +/**
> + * intel_imr_probe
> + * entry point for IMR driver
> + *
> + * return: -ENODEV for no IMR support 0 if good to go
> + */
> +static int __init intel_imr_init(void)
> +{
> + struct cpuinfo_x86 *c = &cpu_data(cpu);
> +
> + if (!cpu_has_imr(c))
> + return -ENODEV;
> +
> + if (iosf_mbi_available() == false)
> + return -ENODEV;
> +
> + if (cpu_is_quark(c)) {
> + imr_dev.num = QUARK_X1000_IMR_NUM;
> + imr_dev.reg_base = QUARK_X1000_IMR_REGBASE;
> + } else {
> + pr_err("Unknown IMR implementation\n");
> + return -ENODEV;
> + }
> +
> + mutex_init(&imr_dev.lock);
> +
> + return imr_debugfs_register(&imr_dev);
> +}
> +
> +/**
> + * intel_imr_exit
> + * exit point for IMR code. Deregisters debugfs, leave IMR state as-is.
> + *
> + * return: none
> + */
> +static void __exit intel_imr_exit(void)
> +{
> + imr_debugfs_unregister(&imr_dev);
> +}
> +
> +module_init(intel_imr_init);
> +module_exit(intel_imr_exit);
> +
> +MODULE_AUTHOR("Bryan O'Donoghue <[email protected]>");
> +MODULE_DESCRIPTION("Intel Isolated Memory Region driver");
> +MODULE_LICENSE("GPL");
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Happy New Year!
--
With Best Regards,
Andy Shevchenko
On Mon, Dec 29, 2014 at 7:23 PM, Bryan O'Donoghue
<[email protected]> wrote:
> Intel Galileo Gen1 and Gen2 boot to Linux from EFI and grub with IMR
> registers enabled around the compressed kernel image and boot params data
> structure.
>
> The purpose of the IMRs around the compressed kernel and boot params data
> structure is to ensure that no spurious data writes from any agent within
> the Quark system can corrupt the kernel/boot-params data during boot.
>
> The kernel needs to tear-down the IMRs placed around the compressed kernel
> image and boot-params data structure since the EFI memory map marks the two
> regions of memory as usable memory and the kernel will happily reuse these
> memory regions. Without tearing down the boot-time IMRs drivers run the
> significant risk of violating one of the stale IMRs since dma_alloc_coherent
> can hand addresses to DMA capable south cluster peripherals such as the SD,
> Ethernet, USB host/device, which will then cause an IMR access violation
> when a DMA read/write occurs to the address ranges in question.
>
> Since the Quark EFI bringup code configures the system to reset on an IMR
> violation, this means that common operations such as mouting an SD based
> root filesystem, communicating with a USB device or sending Ethernet traffic
> can cause an immediate system reset.
>
> IMR usage during system boot on Galileo is detailed in
> Quark_SecureBoot_PRM_330234_001.pdf. This document details each IMR used
> during the boot process and the data being protected by that IMR. The kernel
> needs tidy-up IMRs used during the boot process to ensure an IMR violation
> doesn't cause a system reset.
>
> This platform code does two things.
>
> Firstly it tears down the boot-time IMRs used to protect the compressed
> kernel image and boot params data structure.
>
> Secondly it sets up an IMR around the kernel's text section from &_sinittext
> - &_text ensuring that on the CPU in non-SMM mode can read/write this
> address range. A spurious DMA write to the kernel's .text section will
> then cause an IMR violation and system reset, consistent with the current
> Galileo BSP behaviour.
>
> Signed-off-by: Bryan O'Donoghue <[email protected]>
> ---
> drivers/platform/x86/Kconfig | 15 +++
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/intel_galileo.c | 175 +++++++++++++++++++++++++++++++++++
> 3 files changed, 191 insertions(+)
> create mode 100644 drivers/platform/x86/intel_galileo.c
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 638e7970..e384dcd 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -804,6 +804,21 @@ config INTEL_OAKTRAIL
> enable/disable the Camera, WiFi, BT etc. devices. If in doubt, say Y
> here; it will only load on supported platforms.
>
> +config INTEL_GALILEO
> + bool "Intel Galileo Platform Support"
> + depends on X86_32 && PCI
> + select IOSF_MBI
> + select IMR
> + ---help---
> + Intel Galileo platform support. This code is used to tear-down
> + BIOS and Grub Isolated Memory Regions used during bootup.
> + This sanitises the IMR memory map to agree with the EFI/e820
> + memory map, without this code your IMR memory map will conflict
> + with the EFI memory map and it's highly likely DMA accesses initiated
> + by Ethernet, SD and/or USB will result in a system reset.
> +
> + If in doubt, say Y here, the code will only run on a Galileo Gen1/Gen2
> +
> config SAMSUNG_Q10
> tristate "Samsung Q10 Extras"
> depends on ACPI
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index f82232b..a0c013d 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_SAMSUNG_LAPTOP) += samsung-laptop.o
> obj-$(CONFIG_MXM_WMI) += mxm-wmi.o
> obj-$(CONFIG_INTEL_MID_POWER_BUTTON) += intel_mid_powerbtn.o
> obj-$(CONFIG_INTEL_OAKTRAIL) += intel_oaktrail.o
> +obj-$(CONFIG_INTEL_GALILEO) += intel_galileo.o
> obj-$(CONFIG_SAMSUNG_Q10) += samsung-q10.o
> obj-$(CONFIG_APPLE_GMUX) += apple-gmux.o
> obj-$(CONFIG_INTEL_RST) += intel-rst.o
> diff --git a/drivers/platform/x86/intel_galileo.c b/drivers/platform/x86/intel_galileo.c
> new file mode 100644
> index 0000000..2a555aa
> --- /dev/null
> +++ b/drivers/platform/x86/intel_galileo.c
> @@ -0,0 +1,175 @@
> +/*
> + * intel_galileo.c - Intel Galileo platform support.
> + *
> + * Copyright(c) 2013 Intel Corporation.
> + * Copyright(c) 2014 Bryan O'Donoghue <[email protected]>
> + *
> + * This platform code provides an entry point to do Galileo specific
> + * setup. Critically IMRs are policed to ensure the EFI provided memory
> + * map informing the kernel of it's available memory is consistent with
> + * the IMR lock-down
Missed dot at the end.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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.
> + */
Define pr_fmt().
> +#include <asm-generic/sections.h>
> +#include <asm/imr.h>
> +#include <asm/intel-quark.h>
> +#include <linux/dmi.h>
> +#include <linux/module.h>
> +#include <linux/mm.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +
> +enum {
> + GALILEO_UNKNOWN = 0,
> + GALILEO_QRK_GEN1,
> + GALILEO_QRK_GEN2,
> +};
> +
> +static struct dmi_system_id galileo_baseboards[] = {
> + {
> + .ident = "Galileo",
> + .matches = {
> + DMI_MATCH(DMI_BOARD_VENDOR, "Intel"),
> + DMI_MATCH(DMI_BOARD_NAME, "Galileo"),
> + },
> + .driver_data = (void *)GALILEO_QRK_GEN1,
> + },
> + {
> + .ident = "GalileoGen2",
> + .matches = {
> + DMI_MATCH(DMI_BOARD_VENDOR, "Intel"),
> + DMI_MATCH(DMI_BOARD_NAME, "GalileoGen2"),
> + },
> + .driver_data = (void *)GALILEO_QRK_GEN2,
> + },
> + {}
> +};
> +
> +#ifdef DEBUG
Move this inside function.
> +#define SANITY "IMR : sanity error "
You may define this before function and undefine later. (Actually you
missed undef)
> +
> +/**
> + * intel_galileo_imr_sanity
> + * Verify IMR sanity with some simple tests to verify
> + * overlap, zero sized allocations
> + *
> + * @base: Physical base address of the kernel text section
> + * @size: Extent of kernel memory to be covered from &_text to &_sinittext
> + * @return: none
Redundant.
> + */
> +static void __init
> +intel_galileo_imr_sanity(unsigned long base, unsigned long size)
> +{
> + /* Test zero zero */
> + if (imr_add(0, 0, 0, 0, true) == 0)
> + pr_err(SANITY "zero sized IMR @ 0x00000000\n");
> +
> + /* Test overlap */
> + if (imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true) == 0)
> + pr_err(SANITY "overlapped IMR @ (0x%08lx - 0x%08lx)\n",
> + base, base + size);
> +
> + /* Try overlap - IMR_ALIGN */
> + base = base + size - IMR_ALIGN;
base += size ...
> + if (imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true) == 0)
> + pr_err(SANITY "overlapped IMR @ (0x%08lx - 0x%08lx)\n",
> + base, base + size);
> +}
> +#endif
> +
> +/**
> + * intel_galileo_imr_init
> + *
> + * Tear down IMRs used during bootup. BIOS and Grub
> + * both setup IMRs around compressed kernel, initrd memory
> + * that need to be removed before the kernel hands out one
> + * of the IMR encased addresses to a downstream DMA agent
> + * such as the SD or Ethernet. IMRs on Galileo are setup to
> + * immediately reset the system on violation - so if you're
> + * running a root filesystem from SD - you'll need the IMRs
> + * torn down or you'll find seemingly random resets when using
> + * your filesystem.
Split this to Summary and Description.
> + */
> +static void __init intel_galileo_imr_init(void)
> +{
> + unsigned long base = virt_to_phys(&_text);
> + unsigned long size = virt_to_phys(&_sinittext) - base - IMR_ALIGN;
> + int i, ret;
> +
> + /* Tear down all existing unlocked IMRs */
> + for (i = 0; i <= QUARK_X1000_IMR_NUM; i++)
NUM or last one? Again confusing.
> + imr_del(i, 0, 0);
> +
> + /*
> + * Setup an IMR around the physical extent of the kernel
> + * Non-SMM mode core read/write from/to kernel physical region.
> + * IMR locked.
> + */
> + ret = imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true);
> + if (ret)
> + pr_err("Unable to setup IMR for kernel: (%p - %p)\n",
> + &_text, &__init_begin);
> + else
> + pr_info("IMR protect kernel memory: %ldk (%p - %p)\n",
> + size >> 10, &_text, &__init_begin);
10 is a magic number. What is for?
> +#ifdef DEBUG
Move this inside the function.
> + intel_galileo_imr_sanity(base, size);
> +#endif
> +}
> +
> +/**
> + * intel_galileo_init
> + *
> + * Identify a Galileo Gen1 or Gen2. If found run code to sanitise the
> + * kernel memory space of IMRs that are inconsistent with the EFI memory map.
Split this to parts.
> + */
> +static int __init intel_galileo_init(void)
> +{
> + int ret = 0, type = GALILEO_UNKNOWN;
> + struct cpuinfo_x86 *c = &cpu_data(cpu);
> + const struct dmi_system_id *system_id;
> +
> + if (!cpu_is_quark(c))
x86_match_cpu() ?
> + return -ENODEV;
> +
> + system_id = dmi_first_match(galileo_baseboards);
> +
> + /* BIOS releases 0.7.5 and 0.8.0 do not provide DMI strings */
> + if (system_id != NULL) {
> + type = (int)system_id->driver_data;
> + } else {
> + pr_info("Galileo Gen1 BIOS version <= 0.8.0\n");
> + type = GALILEO_QRK_GEN1;
> + }
> +
> + switch (type) {
> + case GALILEO_QRK_GEN1:
> + case GALILEO_QRK_GEN2:
> + intel_galileo_imr_init();
> + break;
> + default:
> + ret = -ENODEV;
return -ENODEV; and remove ret variable.
> + }
> +
> + return ret;
return 0;
> +}
> +
> +static void __exit intel_galileo_exit(void)
> +{
> +}
Do we need empty exit function at all?
> +
> +module_init(intel_galileo_init);
> +module_exit(intel_galileo_exit);
> +
> +MODULE_AUTHOR("Bryan O'Donoghue <[email protected]>");
> +MODULE_DESCRIPTION("Intel Galileo platform driver");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
With Best Regards,
Andy Shevchenko