[PATCH v2 0/1] x86: Add IMR support to Quark/Galileo

[PATCH v2 0/1] x86: Add IMR support to Quark/Galileo

[Bryan O'Donoghue]

This patchset adds support for Isolated Memory Regions to the kernel.

Quark SoC X1000 contains 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.

An example of IMRs in use is given with Arduino compatiable Galileo boards
which ship with an IMR around the ACPI runtime services memory. If a DMA
read/write cycle were to occur to this region of memory this would trigger
the IMR violation mechansim.

As part of the IMR init code all unlocked IMRs are removed to ensure the
EFI memory map and IMR memory map are consistent. This is necessary since at
various stages during the boot of Quark systems firmware and second stage
bootloader will place unlocked IMRs around various assets in memory, with
the expectation that subsequent phases of boot will tear-down unlocked/stale
IMRs before proceeding. The kernel needs to tear-down unlocked IMRs placed
around the boot params structure and compressed kernel in memory. Without
doing so DMA addresses given out by the kernel to DMA capable hardware runs
the risk of triggering an IMR fault when DMA happens to those addresses.
As a result any unlocked IMR must be torn down by the kernel early in the
boot process to sanitize the memory map. 

As an additional protection to the run-time kernel from unwarranted memory
transactions an IMR is placed around the kernel's .text and .rodata
sections. 

Changes since v1:
 - Galileo platform code
    Removed completely. Policy to tear-down unlocked IMRs and setup IMR
    around kernel .text and .rodata as part of IMR init code.
    Darren Hart/Ong, Boon Leong
 - imr_add/imr_del
    Renamed to imr_add_range and imr_del_range respectively.
    Andy Shevchenko
 - x86_match_cpu
    Used in place of DMI strings specific to Galileo.
    Andy Shevchenko/Ong, Boon Leong
 - Expanded git log definitions of IMRs
    Addition of more descriptive text to deliniate between different IMR
    types.
    Ong, Boon Leong
 - struct imr
    Renamed to struct imr_regs
    Andy Shevchenko/Darren Hart
 - imr_read/imr_write
    Flow reworked flow of register indexing
    Andy Shevchenko
 - debugfs hooks changed
    Andy Shevchenko
 - imr_enabled
    Definition of an enabled IMR updated to include read/write mask values
    present in IMR. Address @ zero and read/write mask in conjunction will
    be the definition of a disabled IMR on X1000 to be consistent with
    firmware both old and current which also defines a disabled IMR this
    way.
    Darren Hart/Ong, Boon Leong
 - Overlapping
    Comment added to code to explain the design decision not to allow IMR
    overlaps.
    Darren Hart/Ong, Boon Leong
 - CONFIG_DEBUG_IMR_SELFTEST
    Automated IMR self test moved from removed Galileo platform code and
    added to IMR init code. Option exists in the kernel hacking section.
    Darren Hart
 - IMR self test
    Expanded to over more scenarios
    Bryan O'Donoghue
 - Remove reference to IMR_ENABLE bit
    Undocumented bit with respect to Quark X1000
    Ong, Boon Leong
 - Expanded kernel IMR to encompass .text and .rodata
    IMR protecting both .text and .rodata as in the same way as .text and
    .rodata are marked read-only in the relevant page-table entries.
    Bryan O'Donoghue
 - Overlap bounds checking
    Moved range checking of overlap into a function
    Andy Shevchenko
 
Bryan O'Donoghue (1):
  x86: Add Isolated Memory Regions for Quark X1000

 arch/x86/Kconfig           |  25 ++
 arch/x86/Kconfig.debug     |  12 +
 arch/x86/include/asm/imr.h |  60 ++++
 arch/x86/kernel/Makefile   |   1 +
 arch/x86/kernel/imr.c      | 682 +++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 780 insertions(+)
 create mode 100644 arch/x86/include/asm/imr.h
 create mode 100644 arch/x86/kernel/imr.c

-- 
1.9.1

[PATCH v2 1/1] x86: Add Isolated Memory Regions for Quark X1000

[Bryan O'Donoghue]

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

Quark_SecureBootPRM_330234_001.pdf section 4.5 details the concept of IMRs
quark-x1000-datasheet.pdf section 12.7.4 details the implementation of IMRs
in silicon.

eSRAM flush, CPU Snoop, CPU SMM Mode, CPU non-SMM mode, RMU and PCIe Virtual
Channels (VC0 and VC1) can have individual read/write access masks applied
to them for a given memory region in Quark X1000. This enables IMRs to treat
each memory transaction type listed above on an individual basis and to
filter appropriately based on the IMR access mask for the memory region.
Quark supports eight IMRs.

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

Quark firmware sets up a series of locked IMRs around pieces of memory that
firmware owns such as ACPI runtime data. During boot a series of unlocked
IMRs are placed around items in memory to guarantee no DMA modification of
those items can take place. Grub also places an unlocked IMR around the
kernel boot-params data structure and compressed kernel image. It is
necessary for the kernel to tear down all unlocked IMRs in order to ensure
that the kernel's view of memory passed via the EFI memory map is consistent
with the IMR memory map. Without tearing down all unlocked IMRs on boot
transitory IMRs such as those used to protect the compressed kernel image
will cause IMR violations and system reboots.

The IMR init code tears down all unlocked IMRs and sets a protective IMR
around the kernel .text and .rodata as one contiguous block. This sanitizes
the IMR memory map with respect to the EFI memory map and protects the
read-only portions of the kernel from unwarranted DMA access.

Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
---
 arch/x86/Kconfig           |  25 ++
 arch/x86/Kconfig.debug     |  13 +
 arch/x86/include/asm/imr.h |  60 ++++
 arch/x86/kernel/Makefile   |   1 +
 arch/x86/kernel/imr.c      | 681 +++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 780 insertions(+)
 create mode 100644 arch/x86/include/asm/imr.h
 create mode 100644 arch/x86/kernel/imr.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ba397bd..5af669c 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -526,6 +526,31 @@ config IOSF_MBI_DEBUG
 
 	  If you don't require the option or are in doubt, say N.
 
+config IMR
+	bool "Isolated Memory Region support"
+	default n
+	depends on IOSF_MBI
+	---help---
+	  This option provides a means to manipulate 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 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:
+		- eSRAM flush
+		- Dirty CPU snoop (write only)
+		- RMU access
+		- PCI Virtual Channel 0/Virtual Channel 1
+		- SMM mode
+		- Non SMM mode
+
+	  Quark contains a set of eight IMR registers and makes use of those
+	  registers during its 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/Kconfig.debug b/arch/x86/Kconfig.debug
index 61bd2ad..be22820 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -313,6 +313,19 @@ config DEBUG_NMI_SELFTEST
 
 	  If unsure, say N.
 
+config DEBUG_IMR_SELFTEST
+	bool "Isolated Memory Region self test"
+	default n
+	depends on IMR
+	---help---
+	  This option enables automated sanity testing of the IMR code.
+	  Some simple tests are run to verify IMR bounds checking, alignment
+	  and overlapping. This option is really only useful if you are
+	  debugging an IMR memory map or are modifying the IMR code and want to
+	  test your changes.
+
+	  If unsure say N.
+
 config X86_DEBUG_STATIC_CPU_HAS
 	bool "Debug alternatives"
 	depends on DEBUG_KERNEL
diff --git a/arch/x86/include/asm/imr.h b/arch/x86/include/asm/imr.h
new file mode 100644
index 0000000..b572a81
--- /dev/null
+++ b/arch/x86/include/asm/imr.h
@@ -0,0 +1,60 @@
+/*
+ * imr.h: Isolated Memory Region API
+ *
+ * Copyright(c) 2013 Intel Corporation.
+ * Copyright(c) 2015 Bryan O'Donoghue <pure.logic@nexus-software.ie>
+ *
+ * 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 <linux/types.h>
+
+/*
+ * IMR agent access mask bits
+ * See section 12.7.4.7 from quark-x1000-datasheet.pdf for register
+ * definitions
+ */
+#define IMR_ESRAM_FLUSH		BIT(31)
+#define IMR_CPU_SNOOP		BIT(30)		/* Applicable only to write */
+#define IMR_RMU			BIT(29)
+#define IMR_VC1_SAI_ID3		BIT(15)
+#define IMR_VC1_SAI_ID2		BIT(14)
+#define IMR_VC1_SAI_ID1		BIT(13)
+#define IMR_VC1_SAI_ID0		BIT(12)
+#define IMR_VC0_SAI_ID3		BIT(11)
+#define IMR_VC0_SAI_ID2		BIT(10)
+#define IMR_VC0_SAI_ID1		BIT(9)
+#define IMR_VC0_SAI_ID0		BIT(8)
+#define IMR_CPU_0		BIT(1)		/* SMM mode */
+#define IMR_CPU			BIT(0)		/* Non SMM mode */
+#define IMR_ACCESS_NONE		0
+
+/*
+ * Read/Write access-all bits here include some reserved bits
+ * These are the values firmware uses and are accepted by hardware.
+ * The kernel defines read/write access-all in the same was as firmware
+ * in order to have a consistent and crisp definition across firmware,
+ * bootloader and kernel
+ */
+#define IMR_READ_ACCESS_ALL	0xBFFFFFFF
+#define IMR_WRITE_ACCESS_ALL	0xFFFFFFFF
+
+/* Number of IMRs provided by Quark X1000 SoC */
+#define QUARK_X1000_IMR_MAX	0x08
+#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)
+
+int imr_add_range(unsigned long base, unsigned long size,
+		  unsigned int rmask, unsigned int wmask, bool lock);
+
+int imr_remove_range(int reg, unsigned long base, unsigned long size);
+
+#endif /* _IMR_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..5d9bfc4
--- /dev/null
+++ b/arch/x86/kernel/imr.c
@@ -0,0 +1,681 @@
+/**
+ * intel_imr.c
+ *
+ * Copyright(c) 2013 Intel Corporation.
+ * Copyright(c) 2015 Bryan O'Donoghue <pure.logic@nexus-software.ie>
+ *
+ * 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 reference code from
+ * Intel's Quark BSP EFI, Linux and grub code.
+ *
+ * See quark-x1000-datasheet.pdf for register definitions
+ * http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/quark-x1000-datasheet.pdf
+ */
+
+#define pr_fmt(fmt) "imr: " fmt
+
+#include <asm-generic/sections.h>
+#include <asm/cpu_device_id.h>
+#include <asm/imr.h>
+#include <asm/iosf_mbi.h>
+#include <linux/debugfs.h>
+#include <linux/init.h>
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+
+struct imr_device {
+	struct dentry	*file;
+	bool		init;
+	struct mutex	lock;
+	int		max_imr;
+	int		reg_base;
+};
+
+static struct imr_device imr_dev;
+
+/*
+ * IMR read/write mask control registers.
+ * See quark-x1000-datasheet.pdf sections 12.7.4.5 and 12.7.4.6 for
+ * bit definitions.
+ *
+ * addr_hi
+ * 31		Lock bit
+ * 30:24	Reserved
+ * 23:2		1 KiB aligned lo address
+ * 1:0		Reserved
+ *
+ * addr_hi
+ * 31:24	Reserved
+ * 23:2		1 KiB aligned hi address
+ * 1:0		Reserved
+ */
+#define IMR_LOCK	BIT(31)
+
+struct imr_regs {
+	u32 addr_lo;
+	u32 addr_hi;
+	u32 rmask;
+	u32 wmask;
+};
+
+#define IMR_NUM_REGS	(sizeof(struct imr_regs)/sizeof(u32))
+#define IMR_LOCK_OFF	(IMR_NUM_REGS - 1)
+#define IMR_SHIFT	8
+#define imr_to_phys(x)	((x) << IMR_SHIFT)
+#define phys_to_imr(x)	((x) >> IMR_SHIFT)
+
+/**
+ * imr_enabled - true if an IMR is enabled false otherwise
+ *
+ * Determines if an IMR is enabled based on address range and read/write
+ * mask. An IMR set with an address range set to zero and a read/write
+ * access mask set to all is considered to be disabled. An IMR in any
+ * other state - for example set to zero but without read/write access
+ * all is considered to be enabled. This definition of disabled is how
+ * firmware switches off an IMR and is maintained in kernel for
+ * consistency.
+ *
+ * @imr:	pointer to IMR descriptor
+ * @return:	true if IMR enabled false if disabled
+ */
+static int imr_enabled(struct imr_regs *imr)
+{
+	return (imr->rmask != IMR_READ_ACCESS_ALL ||
+		imr->wmask != IMR_WRITE_ACCESS_ALL ||
+		imr_to_phys(imr->addr_lo) ||
+		imr_to_phys(imr->addr_hi));
+}
+
+/**
+ * imr_read - read an IMR at a given 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_regs *imr)
+{
+	u32 reg = imr_id * IMR_NUM_REGS + imr_dev.reg_base;
+	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 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_regs *imr, bool lock)
+{
+	unsigned long flags;
+	u32 reg = imr_id * IMR_NUM_REGS + imr_dev.reg_base;
+	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 - IMR_LOCK_OFF, imr->addr_lo);
+	}
+
+	local_irq_restore(flags);
+	return 0;
+done:
+	/*
+	 * 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.
+	 */
+	local_irq_restore(flags);
+	WARN(ret, "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;
+	struct imr_regs imr;
+	int ret = -ENODEV;
+	u32 size;
+
+	mutex_lock(&imr_dev.lock);
+
+	for (i = 0; i < imr_dev.max_imr; 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 = imr_to_phys(imr.addr_hi) - imr_to_phys(imr.addr_lo) + 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_enabled(&imr) ? imr_to_phys(imr.addr_hi) + IMR_MASK : 0,
+			   imr_enabled(&imr) ? size : 0,
+			   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(void)
+{
+	imr_dev.file = debugfs_create_file("imr_state", S_IFREG | S_IRUGO, NULL,
+					   &imr_dev, &imr_state_ops);
+	if (!imr_dev.file)
+		return -ENODEV;
+
+	return 0;
+}
+
+/**
+ * imr_debugfs_unregister
+ * Unregister debugfs hooks
+ *
+ * @imr:	IMR structure representing address and access masks
+ * @return:
+ */
+static void imr_debugfs_unregister(void)
+{
+	if (!imr_dev.file)
+		return;
+
+	debugfs_remove(imr_dev.file);
+}
+#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("base 0x%08lx size 0x%08lx must align to 1KiB\n",
+			base, size);
+		return -EINVAL;
+	}
+	return 0;
+}
+
+/**
+ * imr_fixup_size - account for the IMR_ALIGN bytes that addr_hi appends
+ *
+ * IMR addr_hi has a built in offset of plus IMR_ALIGN (0x400) bytes from the
+ * value in the register. We need to subtract IMR_ALIGN bytes from input sizes
+ * as a result
+ *
+ * @size:	input size bytes
+ * @return:	reduced size
+ */
+static unsigned long imr_fixup_size(unsigned long size)
+{
+	return size - IMR_ALIGN;
+}
+
+/**
+ * imr_address_overlap - detects an address overlap
+ *
+ * @addr:	address to check against an existing IMR
+ * @imr:	imr being checked
+ * @return:	true for overlap false for no overlap
+ */
+static int imr_address_overlap(unsigned long addr, struct imr_regs *imr)
+{
+	return addr >= imr_to_phys(imr->addr_lo) && addr <= imr_to_phys(imr->addr_hi);
+}
+
+/**
+ * imr_add_range - add an Isolated Memory Region
+ *
+ * @base:	physical base address of region aligned to 1KiB
+ * @size:	physical size of region in bytes must be aligned to 1KiB
+ * @read_mask:	read access mask
+ * @write_mask:	write access mask
+ * @lock:	indicates whether or not to permanently lock this region
+ * @return:	index of the IMR allocated or negative value indicating error
+ */
+int imr_add_range(unsigned long base, unsigned long size,
+	    unsigned int rmask, unsigned int wmask, bool lock)
+{
+	unsigned long end = base + size;
+	int i;
+	struct imr_regs imr;
+	int reg;
+	int ret;
+
+	if (imr_dev.init == false)
+		return -ENODEV;
+
+	ret = imr_check_range(base, size);
+	if (ret)
+		return ret;
+
+	if (size < IMR_ALIGN)
+		return -EINVAL;
+
+	/* Tweak the size value */
+	size = imr_fixup_size(size);
+
+	mutex_lock(&imr_dev.lock);
+
+	/*
+	 * Find a free IMR while checking for an existing overlapping range.
+	 * Note there's no restriction in silicon to prevent IMR overlaps.
+	 * For the sake of simplicity and ease in defining/debugging an IMR
+	 * memory map we exclude IMR overlaps.
+	 */
+	reg = -1;
+	for (i = 0; i < imr_dev.max_imr; i++) {
+		ret = imr_read(i, &imr);
+		if (ret)
+			goto done;
+
+		/* Find overlap @ base or end of requested range */
+		if (imr_enabled(&imr)) {
+			if (imr_address_overlap(base, &imr)) {
+				ret = -EINVAL;
+				goto done;
+			}
+			if (imr_address_overlap(end, &imr)) {
+				ret = -EINVAL;
+				goto done;
+			}
+		} else {
+			reg = i;
+		}
+	}
+
+	/* Error out if we have no free IMR entries */
+	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 = 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 == 0 ? reg : ret;
+}
+EXPORT_SYMBOL(imr_add_range);
+
+/**
+ * imr_remove_range - delete an Isolated Memory Region
+ *
+ * This function allows you to delete an IMR by it's index specified by reg or
+ * by address range specified by base and size respectively. If you specify an
+ * index on it's own the base and size parameters are ignored.
+ * imr_remove_range(0, size, base); delete IMR at index 0 base/size ignored
+ * imr_remove_range(-1, base, size); delete IMR from base to base+size
+ *
+ * @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_remove_range(int reg, unsigned long base, unsigned long size)
+{
+	struct imr_regs imr;
+	int found = 0, i, ret = 0;
+	unsigned long  max = base + size;
+
+	if (imr_dev.init == false)
+		return -ENODEV;
+
+	if (imr_check_range(base, size))
+		return -EINVAL;
+
+	if (reg >= imr_dev.max_imr)
+		return -EINVAL;
+
+	/* Tweak the size value */
+	size = imr_fixup_size(size);
+
+	mutex_lock(&imr_dev.lock);
+
+	if (reg >= 0) {
+		/* If a specific IMR is given try to use it */
+		ret = imr_read(reg, &imr);
+		if (ret)
+			goto done;
+
+		if (!imr_enabled(&imr) || imr.addr_lo & IMR_LOCK) {
+			ret = -ENODEV;
+			goto done;
+		}
+		found = 1;
+
+	} else {
+		/* Search for match based on address range */
+		for (i = 0; i < imr_dev.max_imr; 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;
+				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_remove_range);
+
+#ifdef CONFIG_DEBUG_IMR_SELFTEST
+
+#define SELFTEST "imr: self_test "
+
+/**
+ * imr_self_test_result - Print result string for self test
+ *
+ * @res:	result code - true if test passed false otherwise
+ * @fmt:	format string
+ * ...		variadic argument list
+ */
+static void __init imr_self_test_result(int res, const char *fmt, ...)
+{
+	va_list vlist;
+
+	va_start(vlist, fmt);
+	if (res)
+		printk(KERN_INFO SELFTEST "pass ");
+	else
+		printk(KERN_ERR SELFTEST "fail ");
+	vprintk(fmt, vlist);
+	va_end(vlist);
+}
+
+#undef SELFTEST
+
+/**
+ * imr_self_test
+ *
+ * Verify IMR self_test with some simple tests to verify overlap,
+ * zero sized allocations and 1 KiB sized areas.
+ *
+ * @base:	physical base address of the kernel text section
+ * @size:	extent of kernel memory to be covered from &_text to &__end_rodata
+ */
+static void __init imr_self_test(unsigned long base, unsigned long size)
+{
+	const char *fmt_over = "overlapped IMR @ (0x%08lx - 0x%08lx)\n";
+	int idx;
+
+	/* Test zero zero */
+	idx = imr_add_range(0, 0, 0, 0, false);
+	imr_self_test_result(idx < 0, "zero sized IMR\n");
+
+	/* Test exact overlap */
+	idx = imr_add_range(base, size, IMR_CPU, IMR_CPU, false);
+	imr_self_test_result(idx < 0, fmt_over, __va(base), __va(base + size));
+
+	/* Test overlap with base inside of existing */
+	base += size - IMR_ALIGN;
+	idx = imr_add_range(base, size, IMR_CPU, IMR_CPU, false);
+	imr_self_test_result(idx < 0, fmt_over, __va(base), __va(base + size));
+
+	/* Test overlap with end inside of existing */
+	base -= size + IMR_ALIGN * 2;
+	idx = imr_add_range(base, size, IMR_CPU, IMR_CPU, false);
+	imr_self_test_result(idx < 0, fmt_over, __va(base), __va(base + size));
+
+	/* Test 1 KiB works */
+	idx = imr_add_range(0, IMR_ALIGN, IMR_READ_ACCESS_ALL,
+			    IMR_WRITE_ACCESS_ALL, false);
+	imr_self_test_result(idx >= 0, "1KiB IMR @ 0x00000000\n");
+
+	/* Tear-tow 1 KiB if previous was successful */
+	if (idx >= 0) {
+		idx = imr_remove_range(idx, 0, 0);
+		imr_self_test_result(idx == 0, "teardown 1KiB @ 0x00000000\n");
+	}
+}
+#endif /* CONFIG_DEBUG_IMR_SELFTEST */
+
+/**
+ * imr_fixup_memmap - 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.
+ * As a result if you're running a root filesystem from SD - you'll need
+ * the boot-time IMRs torn down or you'll find seemingly random resets when
+ * using your filesystem.
+ */
+static void __init imr_fixup_memmap(void)
+{
+	unsigned long base  = virt_to_phys(&_text);
+	unsigned long size = virt_to_phys(&__end_rodata) - base;
+	int i, ret;
+
+	/* Tear down all existing unlocked IMRs */
+	for (i = 0; i < imr_dev.max_imr; i++)
+		imr_remove_range(i, 0, 0);
+
+	/*
+	 * Setup a locked IMR around the physical extent of the kernel
+	 * from the beginning of the .text secton to the end of the
+	 * .rodata section.
+	 *
+	 * This behaviour relies on the kernel .text and .rodata
+	 * sections being physically contiguous and .rodata ending on 1
+	 * KiB aligned boundary - such as a page boundary. Linker script
+	 * The definition of this memory map is in:
+	 * arch/x86/kernel/vmlinux.lds
+	 * .text begin = &_stext
+	 * .rodata end = &__end_rodata - aligned to 4KiB
+	 */
+	ret = imr_add_range(base, size, IMR_CPU, IMR_CPU, true);
+	if (ret < 0)
+		pr_err("unable to setup IMR for kernel: (%p - %p)\n",
+			&_text, &__end_rodata);
+	else
+		pr_info("protecting kernel .text - .rodata: %ldk (%p - %p)\n",
+			size / 1024, &_text, &__end_rodata);
+
+#ifdef CONFIG_DEBUG_IMR_SELFTEST
+	/* Run optional self test */
+	imr_self_test(base, size);
+#endif
+}
+
+static const struct x86_cpu_id imr_ids[] __initconst = {
+	{ X86_VENDOR_INTEL, 5, 9 },	/* Intel Quark SoC X1000 */
+	{}
+};
+MODULE_DEVICE_TABLE(x86cpu, imr_ids);
+
+/**
+ * imr_probe - entry point for IMR driver
+ *
+ * return: -ENODEV for no IMR support 0 if good to go
+ */
+static int __init imr_init(void)
+{
+	int ret;
+
+	if (!x86_match_cpu(imr_ids) || !iosf_mbi_available())
+		return -ENODEV;
+
+#ifdef CONFIG_DEBUG_FS
+	ret = imr_debugfs_register();
+	if (ret != 0)
+		return ret;
+#endif
+
+	imr_dev.max_imr = QUARK_X1000_IMR_MAX;
+	imr_dev.reg_base = QUARK_X1000_IMR_REGBASE;
+
+	mutex_init(&imr_dev.lock);
+
+	imr_dev.init = true;
+	imr_fixup_memmap();
+
+	return 0;
+}
+
+/**
+ * imr_exit - exit point for IMR code
+ * Deregisters debugfs, leave IMR state as-is.
+ *
+ * return:
+ */
+static void __exit imr_exit(void)
+{
+#ifdef CONFIG_DEBUG_FS
+	imr_debugfs_unregister();
+#endif
+}
+
+module_init(imr_init);
+module_exit(imr_exit);
+
+MODULE_AUTHOR("Bryan O'Donoghue <pure.logic@nexus-software.ie>");
+MODULE_DESCRIPTION("Intel Isolated Memory Region driver");
+MODULE_LICENSE("GPL");
-- 
1.9.1

Re: [PATCH v2 1/1] x86: Add Isolated Memory Regions for Quark X1000

[Andy Shevchenko]

On Wed, Jan 21, 2015 at 8:46 PM, Bryan O'Donoghue
<pure.logic@nexus-software.ie> 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 KiB.
>
> Quark_SecureBootPRM_330234_001.pdf section 4.5 details the concept of IMRs
> quark-x1000-datasheet.pdf section 12.7.4 details the implementation of IMRs
> in silicon.
>
> eSRAM flush, CPU Snoop, CPU SMM Mode, CPU non-SMM mode, RMU and PCIe Virtual
> Channels (VC0 and VC1) can have individual read/write access masks applied
> to them for a given memory region in Quark X1000. This enables IMRs to treat
> each memory transaction type listed above on an individual basis and to
> filter appropriately based on the IMR access mask for the memory region.
> Quark supports eight IMRs.
>
> 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 is consistent 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.
>
> Quark firmware sets up a series of locked IMRs around pieces of memory that
> firmware owns such as ACPI runtime data. During boot a series of unlocked
> IMRs are placed around items in memory to guarantee no DMA modification of
> those items can take place. Grub also places an unlocked IMR around the
> kernel boot-params data structure and compressed kernel image. It is
> necessary for the kernel to tear down all unlocked IMRs in order to ensure
> that the kernel's view of memory passed via the EFI memory map is consistent
> with the IMR memory map. Without tearing down all unlocked IMRs on boot
> transitory IMRs such as those used to protect the compressed kernel image
> will cause IMR violations and system reboots.
>
> The IMR init code tears down all unlocked IMRs and sets a protective IMR
> around the kernel .text and .rodata as one contiguous block. This sanitizes
> the IMR memory map with respect to the EFI memory map and protects the
> read-only portions of the kernel from unwarranted DMA access.
>

Few nitpicks and comments below.

> Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
> ---
>  arch/x86/Kconfig           |  25 ++
>  arch/x86/Kconfig.debug     |  13 +
>  arch/x86/include/asm/imr.h |  60 ++++
>  arch/x86/kernel/Makefile   |   1 +
>  arch/x86/kernel/imr.c      | 681 +++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 780 insertions(+)
>  create mode 100644 arch/x86/include/asm/imr.h
>  create mode 100644 arch/x86/kernel/imr.c
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index ba397bd..5af669c 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -526,6 +526,31 @@ config IOSF_MBI_DEBUG
>
>           If you don't require the option or are in doubt, say N.
>
> +config IMR
> +       bool "Isolated Memory Region support"
> +       default n
> +       depends on IOSF_MBI
> +       ---help---
> +         This option provides a means to manipulate 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 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:
> +               - eSRAM flush
> +               - Dirty CPU snoop (write only)
> +               - RMU access
> +               - PCI Virtual Channel 0/Virtual Channel 1
> +               - SMM mode
> +               - Non SMM mode
> +
> +         Quark contains a set of eight IMR registers and makes use of those
> +         registers during its 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/Kconfig.debug b/arch/x86/Kconfig.debug
> index 61bd2ad..be22820 100644
> --- a/arch/x86/Kconfig.debug
> +++ b/arch/x86/Kconfig.debug
> @@ -313,6 +313,19 @@ config DEBUG_NMI_SELFTEST
>
>           If unsure, say N.
>
> +config DEBUG_IMR_SELFTEST
> +       bool "Isolated Memory Region self test"
> +       default n
> +       depends on IMR
> +       ---help---
> +         This option enables automated sanity testing of the IMR code.
> +         Some simple tests are run to verify IMR bounds checking, alignment
> +         and overlapping. This option is really only useful if you are
> +         debugging an IMR memory map or are modifying the IMR code and want to
> +         test your changes.
> +
> +         If unsure say N.
> +
>  config X86_DEBUG_STATIC_CPU_HAS
>         bool "Debug alternatives"
>         depends on DEBUG_KERNEL
> diff --git a/arch/x86/include/asm/imr.h b/arch/x86/include/asm/imr.h
> new file mode 100644
> index 0000000..b572a81
> --- /dev/null
> +++ b/arch/x86/include/asm/imr.h
> @@ -0,0 +1,60 @@
> +/*
> + * imr.h: Isolated Memory Region API
> + *
> + * Copyright(c) 2013 Intel Corporation.
> + * Copyright(c) 2015 Bryan O'Donoghue <pure.logic@nexus-software.ie>
> + *
> + * 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 <linux/types.h>
> +
> +/*
> + * IMR agent access mask bits
> + * See section 12.7.4.7 from quark-x1000-datasheet.pdf for register
> + * definitions

What about dots at the end of sentences?

> + */
> +#define IMR_ESRAM_FLUSH                BIT(31)
> +#define IMR_CPU_SNOOP          BIT(30)         /* Applicable only to write */
> +#define IMR_RMU                        BIT(29)
> +#define IMR_VC1_SAI_ID3                BIT(15)
> +#define IMR_VC1_SAI_ID2                BIT(14)
> +#define IMR_VC1_SAI_ID1                BIT(13)
> +#define IMR_VC1_SAI_ID0                BIT(12)
> +#define IMR_VC0_SAI_ID3                BIT(11)
> +#define IMR_VC0_SAI_ID2                BIT(10)
> +#define IMR_VC0_SAI_ID1                BIT(9)
> +#define IMR_VC0_SAI_ID0                BIT(8)
> +#define IMR_CPU_0              BIT(1)          /* SMM mode */
> +#define IMR_CPU                        BIT(0)          /* Non SMM mode */
> +#define IMR_ACCESS_NONE                0
> +
> +/*
> + * Read/Write access-all bits here include some reserved bits
> + * These are the values firmware uses and are accepted by hardware.
> + * The kernel defines read/write access-all in the same was as firmware
> + * in order to have a consistent and crisp definition across firmware,
> + * bootloader and kernel
> + */
> +#define IMR_READ_ACCESS_ALL    0xBFFFFFFF
> +#define IMR_WRITE_ACCESS_ALL   0xFFFFFFFF
> +
> +/* Number of IMRs provided by Quark X1000 SoC */
> +#define QUARK_X1000_IMR_MAX    0x08
> +#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)
> +
> +int imr_add_range(unsigned long base, unsigned long size,
> +                 unsigned int rmask, unsigned int wmask, bool lock);
> +
> +int imr_remove_range(int reg, unsigned long base, unsigned long size);
> +
> +#endif /* _IMR_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..5d9bfc4
> --- /dev/null
> +++ b/arch/x86/kernel/imr.c
> @@ -0,0 +1,681 @@
> +/**
> + * intel_imr.c
> + *
> + * Copyright(c) 2013 Intel Corporation.
> + * Copyright(c) 2015 Bryan O'Donoghue <pure.logic@nexus-software.ie>
> + *
> + * 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 reference code from
> + * Intel's Quark BSP EFI, Linux and grub code.
> + *
> + * See quark-x1000-datasheet.pdf for register definitions
> + * http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/quark-x1000-datasheet.pdf
> + */
> +
> +#define pr_fmt(fmt) "imr: " fmt

Maybe more usual
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

> +
> +#include <asm-generic/sections.h>
> +#include <asm/cpu_device_id.h>
> +#include <asm/imr.h>
> +#include <asm/iosf_mbi.h>
> +#include <linux/debugfs.h>
> +#include <linux/init.h>
> +#include <linux/mm.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +
> +struct imr_device {
> +       struct dentry   *file;
> +       bool            init;
> +       struct mutex    lock;
> +       int             max_imr;
> +       int             reg_base;
> +};
> +
> +static struct imr_device imr_dev;
> +
> +/*
> + * IMR read/write mask control registers.
> + * See quark-x1000-datasheet.pdf sections 12.7.4.5 and 12.7.4.6 for
> + * bit definitions.
> + *
> + * addr_hi
> + * 31          Lock bit
> + * 30:24       Reserved
> + * 23:2                1 KiB aligned lo address
> + * 1:0         Reserved
> + *
> + * addr_hi
> + * 31:24       Reserved
> + * 23:2                1 KiB aligned hi address
> + * 1:0         Reserved
> + */
> +#define IMR_LOCK       BIT(31)
> +
> +struct imr_regs {
> +       u32 addr_lo;
> +       u32 addr_hi;
> +       u32 rmask;
> +       u32 wmask;
> +};
> +
> +#define IMR_NUM_REGS   (sizeof(struct imr_regs)/sizeof(u32))
> +#define IMR_LOCK_OFF   (IMR_NUM_REGS - 1)
> +#define IMR_SHIFT      8
> +#define imr_to_phys(x) ((x) << IMR_SHIFT)
> +#define phys_to_imr(x) ((x) >> IMR_SHIFT)
> +
> +/**
> + * imr_enabled - true if an IMR is enabled false otherwise
> + *
> + * Determines if an IMR is enabled based on address range and read/write
> + * mask. An IMR set with an address range set to zero and a read/write
> + * access mask set to all is considered to be disabled. An IMR in any
> + * other state - for example set to zero but without read/write access
> + * all is considered to be enabled. This definition of disabled is how
> + * firmware switches off an IMR and is maintained in kernel for
> + * consistency.
> + *
> + * @imr:       pointer to IMR descriptor
> + * @return:    true if IMR enabled false if disabled
> + */
> +static int imr_enabled(struct imr_regs *imr)
> +{
> +       return (imr->rmask != IMR_READ_ACCESS_ALL ||
> +               imr->wmask != IMR_WRITE_ACCESS_ALL ||
> +               imr_to_phys(imr->addr_lo) ||
> +               imr_to_phys(imr->addr_hi));
> +}
> +
> +/**
> + * imr_read - read an IMR at a given 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_regs *imr)
> +{
> +       u32 reg = imr_id * IMR_NUM_REGS + imr_dev.reg_base;
> +       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);

I would keep this in the same style like
ret =
if (ret)
 return ret;

return 0;

It might be easy to extend if needed, though it's a really minor change.

> +}
> +
> +/**
> + * imr_write - write an IMR at a given 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_regs *imr, bool lock)
> +{
> +       unsigned long flags;
> +       u32 reg = imr_id * IMR_NUM_REGS + imr_dev.reg_base;
> +       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);

Wouldn't be reg++ here as well? Below you substitute full offset which
I think points just to next register.

> +       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 - IMR_LOCK_OFF, imr->addr_lo);
> +       }
> +
> +       local_irq_restore(flags);
> +       return 0;
> +done:
> +       /*
> +        * 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.
> +        */
> +       local_irq_restore(flags);
> +       WARN(ret, "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);

Could it fit one line less?

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

I didn't remembter if I told you, but please use s->private for the
imr_dev pointer.
Everywhere in debugfs calls and if possible in other functions as well.

> +{
> +       int i;
> +       struct imr_regs imr;
> +       int ret = -ENODEV;
> +       u32 size;
> +
> +       mutex_lock(&imr_dev.lock);
> +
> +       for (i = 0; i < imr_dev.max_imr; 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 = imr_to_phys(imr.addr_hi) - imr_to_phys(imr.addr_lo) + 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_enabled(&imr) ? imr_to_phys(imr.addr_hi) + IMR_MASK : 0,
> +                          imr_enabled(&imr) ? size : 0,
> +                          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(void)
> +{
> +       imr_dev.file = debugfs_create_file("imr_state", S_IFREG | S_IRUGO, NULL,
> +                                          &imr_dev, &imr_state_ops);
> +       if (!imr_dev.file)
> +               return -ENODEV;
> +
> +       return 0;
> +}
> +
> +/**
> + * imr_debugfs_unregister
> + * Unregister debugfs hooks
> + *
> + * @imr:       IMR structure representing address and access masks
> + * @return:
> + */
> +static void imr_debugfs_unregister(void)
> +{
> +       if (!imr_dev.file)
> +               return;

Redundant check. I think I told you that already.

> +
> +       debugfs_remove(imr_dev.file);
> +}
> +#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("base 0x%08lx size 0x%08lx must align to 1KiB\n",
> +                       base, size);
> +               return -EINVAL;
> +       }
> +       return 0;
> +}
> +
> +/**
> + * imr_fixup_size - account for the IMR_ALIGN bytes that addr_hi appends
> + *
> + * IMR addr_hi has a built in offset of plus IMR_ALIGN (0x400) bytes from the
> + * value in the register. We need to subtract IMR_ALIGN bytes from input sizes
> + * as a result
> + *
> + * @size:      input size bytes
> + * @return:    reduced size
> + */
> +static unsigned long imr_fixup_size(unsigned long size)
> +{
> +       return size - IMR_ALIGN;
> +}
> +
> +/**
> + * imr_address_overlap - detects an address overlap
> + *
> + * @addr:      address to check against an existing IMR
> + * @imr:       imr being checked
> + * @return:    true for overlap false for no overlap
> + */
> +static int imr_address_overlap(unsigned long addr, struct imr_regs *imr)
> +{
> +       return addr >= imr_to_phys(imr->addr_lo) && addr <= imr_to_phys(imr->addr_hi);
> +}
> +
> +/**
> + * imr_add_range - add an Isolated Memory Region
> + *
> + * @base:      physical base address of region aligned to 1KiB
> + * @size:      physical size of region in bytes must be aligned to 1KiB
> + * @read_mask: read access mask
> + * @write_mask:        write access mask
> + * @lock:      indicates whether or not to permanently lock this region
> + * @return:    index of the IMR allocated or negative value indicating error
> + */
> +int imr_add_range(unsigned long base, unsigned long size,
> +           unsigned int rmask, unsigned int wmask, bool lock)
> +{
> +       unsigned long end = base + size;
> +       int i;
> +       struct imr_regs imr;
> +       int reg;
> +       int ret;
> +
> +       if (imr_dev.init == false)
> +               return -ENODEV;
> +
> +       ret = imr_check_range(base, size);
> +       if (ret)
> +               return ret;
> +
> +       if (size < IMR_ALIGN)
> +               return -EINVAL;
> +
> +       /* Tweak the size value */
> +       size = imr_fixup_size(size);
> +
> +       mutex_lock(&imr_dev.lock);
> +
> +       /*
> +        * Find a free IMR while checking for an existing overlapping range.
> +        * Note there's no restriction in silicon to prevent IMR overlaps.
> +        * For the sake of simplicity and ease in defining/debugging an IMR
> +        * memory map we exclude IMR overlaps.
> +        */
> +       reg = -1;
> +       for (i = 0; i < imr_dev.max_imr; i++) {
> +               ret = imr_read(i, &imr);
> +               if (ret)
> +                       goto done;
> +
> +               /* Find overlap @ base or end of requested range */
> +               if (imr_enabled(&imr)) {
> +                       if (imr_address_overlap(base, &imr)) {
> +                               ret = -EINVAL;
> +                               goto done;
> +                       }
> +                       if (imr_address_overlap(end, &imr)) {
> +                               ret = -EINVAL;
> +                               goto done;
> +                       }
> +               } else {
> +                       reg = i;
> +               }
> +       }
> +
> +       /* Error out if we have no free IMR entries */
> +       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 = 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 == 0 ? reg : ret;
> +}
> +EXPORT_SYMBOL(imr_add_range);
> +
> +/**
> + * imr_remove_range - delete an Isolated Memory Region
> + *
> + * This function allows you to delete an IMR by it's index specified by reg or
> + * by address range specified by base and size respectively. If you specify an
> + * index on it's own the base and size parameters are ignored.
> + * imr_remove_range(0, size, base); delete IMR at index 0 base/size ignored
> + * imr_remove_range(-1, base, size); delete IMR from base to base+size
> + *
> + * @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_remove_range(int reg, unsigned long base, unsigned long size)
> +{
> +       struct imr_regs imr;
> +       int found = 0, i, ret = 0;
> +       unsigned long  max = base + size;
> +
> +       if (imr_dev.init == false)
> +               return -ENODEV;
> +
> +       if (imr_check_range(base, size))
> +               return -EINVAL;
> +
> +       if (reg >= imr_dev.max_imr)
> +               return -EINVAL;
> +
> +       /* Tweak the size value */
> +       size = imr_fixup_size(size);
> +
> +       mutex_lock(&imr_dev.lock);
> +
> +       if (reg >= 0) {
> +               /* If a specific IMR is given try to use it */
> +               ret = imr_read(reg, &imr);
> +               if (ret)
> +                       goto done;
> +
> +               if (!imr_enabled(&imr) || imr.addr_lo & IMR_LOCK) {
> +                       ret = -ENODEV;
> +                       goto done;
> +               }
> +               found = 1;
> +
> +       } else {
> +               /* Search for match based on address range */
> +               for (i = 0; i < imr_dev.max_imr; 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;
> +                               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_remove_range);
> +
> +#ifdef CONFIG_DEBUG_IMR_SELFTEST
> +
> +#define SELFTEST "imr: self_test "
> +
> +/**
> + * imr_self_test_result - Print result string for self test
> + *
> + * @res:       result code - true if test passed false otherwise
> + * @fmt:       format string
> + * ...         variadic argument list
> + */
> +static void __init imr_self_test_result(int res, const char *fmt, ...)
> +{
> +       va_list vlist;
> +
> +       va_start(vlist, fmt);
> +       if (res)
> +               printk(KERN_INFO SELFTEST "pass ");
> +       else
> +               printk(KERN_ERR SELFTEST "fail ");
> +       vprintk(fmt, vlist);
> +       va_end(vlist);
> +}
> +
> +#undef SELFTEST
> +
> +/**
> + * imr_self_test
> + *
> + * Verify IMR self_test with some simple tests to verify overlap,
> + * zero sized allocations and 1 KiB sized areas.
> + *
> + * @base:      physical base address of the kernel text section
> + * @size:      extent of kernel memory to be covered from &_text to &__end_rodata
> + */
> +static void __init imr_self_test(unsigned long base, unsigned long size)
> +{
> +       const char *fmt_over = "overlapped IMR @ (0x%08lx - 0x%08lx)\n";
> +       int idx;
> +
> +       /* Test zero zero */
> +       idx = imr_add_range(0, 0, 0, 0, false);
> +       imr_self_test_result(idx < 0, "zero sized IMR\n");
> +
> +       /* Test exact overlap */
> +       idx = imr_add_range(base, size, IMR_CPU, IMR_CPU, false);
> +       imr_self_test_result(idx < 0, fmt_over, __va(base), __va(base + size));
> +
> +       /* Test overlap with base inside of existing */
> +       base += size - IMR_ALIGN;
> +       idx = imr_add_range(base, size, IMR_CPU, IMR_CPU, false);
> +       imr_self_test_result(idx < 0, fmt_over, __va(base), __va(base + size));
> +
> +       /* Test overlap with end inside of existing */
> +       base -= size + IMR_ALIGN * 2;
> +       idx = imr_add_range(base, size, IMR_CPU, IMR_CPU, false);
> +       imr_self_test_result(idx < 0, fmt_over, __va(base), __va(base + size));
> +
> +       /* Test 1 KiB works */
> +       idx = imr_add_range(0, IMR_ALIGN, IMR_READ_ACCESS_ALL,
> +                           IMR_WRITE_ACCESS_ALL, false);
> +       imr_self_test_result(idx >= 0, "1KiB IMR @ 0x00000000\n");
> +
> +       /* Tear-tow 1 KiB if previous was successful */
> +       if (idx >= 0) {
> +               idx = imr_remove_range(idx, 0, 0);
> +               imr_self_test_result(idx == 0, "teardown 1KiB @ 0x00000000\n");
> +       }
> +}
> +#endif /* CONFIG_DEBUG_IMR_SELFTEST */
> +
> +/**
> + * imr_fixup_memmap - 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.
> + * As a result if you're running a root filesystem from SD - you'll need
> + * the boot-time IMRs torn down or you'll find seemingly random resets when
> + * using your filesystem.
> + */
> +static void __init imr_fixup_memmap(void)
> +{
> +       unsigned long base  = virt_to_phys(&_text);
> +       unsigned long size = virt_to_phys(&__end_rodata) - base;

Shouldn't be phys_addr_t ?
Oh, It might be good for all address parameters in the introduced API.

> +       int i, ret;
> +
> +       /* Tear down all existing unlocked IMRs */
> +       for (i = 0; i < imr_dev.max_imr; i++)
> +               imr_remove_range(i, 0, 0);
> +
> +       /*
> +        * Setup a locked IMR around the physical extent of the kernel
> +        * from the beginning of the .text secton to the end of the
> +        * .rodata section.
> +        *
> +        * This behaviour relies on the kernel .text and .rodata
> +        * sections being physically contiguous and .rodata ending on 1
> +        * KiB aligned boundary - such as a page boundary. Linker script
> +        * The definition of this memory map is in:
> +        * arch/x86/kernel/vmlinux.lds
> +        * .text begin = &_stext
> +        * .rodata end = &__end_rodata - aligned to 4KiB
> +        */
> +       ret = imr_add_range(base, size, IMR_CPU, IMR_CPU, true);
> +       if (ret < 0)
> +               pr_err("unable to setup IMR for kernel: (%p - %p)\n",
> +                       &_text, &__end_rodata);
> +       else
> +               pr_info("protecting kernel .text - .rodata: %ldk (%p - %p)\n",
> +                       size / 1024, &_text, &__end_rodata);

size >> 10

Or, jfyi, string_helpers.c :: string_get_size(), though it prints to the buffer.

> +
> +#ifdef CONFIG_DEBUG_IMR_SELFTEST
> +       /* Run optional self test */
> +       imr_self_test(base, size);
> +#endif

I think it makes sense to move this piece to the init.
I don't see what is exceptional in this function that test belongs here.

> +}
> +
> +static const struct x86_cpu_id imr_ids[] __initconst = {
> +       { X86_VENDOR_INTEL, 5, 9 },     /* Intel Quark SoC X1000 */
> +       {}
> +};
> +MODULE_DEVICE_TABLE(x86cpu, imr_ids);
> +
> +/**
> + * imr_probe - entry point for IMR driver
> + *
> + * return: -ENODEV for no IMR support 0 if good to go
> + */
> +static int __init imr_init(void)
> +{
> +       int ret;
> +
> +       if (!x86_match_cpu(imr_ids) || !iosf_mbi_available())
> +               return -ENODEV;
> +
> +#ifdef CONFIG_DEBUG_FS
> +       ret = imr_debugfs_register();
> +       if (ret != 0)
> +               return ret;

It's non-fatal error.
Thus,
if (ret)
 pr_warn("DebugFS wasn't initialized\n");

Move it after we have imr_dev in place and supply it to debugfs as a
private pointer.

> +#endif
> +
> +       imr_dev.max_imr = QUARK_X1000_IMR_MAX;
> +       imr_dev.reg_base = QUARK_X1000_IMR_REGBASE;
> +
> +       mutex_init(&imr_dev.lock);
> +
> +       imr_dev.init = true;
> +       imr_fixup_memmap();
> +
> +       return 0;
> +}
> +
> +/**
> + * imr_exit - exit point for IMR code
> + * Deregisters debugfs, leave IMR state as-is.
> + *
> + * return:
> + */
> +static void __exit imr_exit(void)
> +{
> +#ifdef CONFIG_DEBUG_FS

I suspect you may remove all those ifdefs and compiler should shrink
not used code since debugfs has the stubs.

> +       imr_debugfs_unregister();
> +#endif
> +}
> +
> +module_init(imr_init);
> +module_exit(imr_exit);
> +
> +MODULE_AUTHOR("Bryan O'Donoghue <pure.logic@nexus-software.ie>");
> +MODULE_DESCRIPTION("Intel Isolated Memory Region driver");
> +MODULE_LICENSE("GPL");
> --
> 1.9.1
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] x86: Add Isolated Memory Regions for Quark X1000

[Bryan O'Donoghue]

On 21/01/15 20:57, Andy Shevchenko wrote:
>
> Few nitpicks and comments below.
>
>> +/*
>> + * IMR agent access mask bits
>> + * See section 12.7.4.7 from quark-x1000-datasheet.pdf for register
>> + * definitions
>
> What about dots at the end of sentences?

Murphy's law says - no matter how many times you proof read for full 
stop you'll miss at least one :)

>> +#define pr_fmt(fmt) "imr: " fmt
>
> Maybe more usual
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Sure.

>> +       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);
>
> I would keep this in the same style like
> ret =
> if (ret)
>   return ret;
>
> return 0;
>
> It might be easy to extend if needed, though it's a really minor change.

No problem

>> +
>> +       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);
>
> Wouldn't be reg++ here as well? Below you substitute full offset which
> I think points just to next register.

I don't think we want to increment below..

>
>> +       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 - IMR_LOCK_OFF, imr->addr_lo);
>> +       }

..because we calculate an offset anyway. An additional increment would 
just be unnecessary cycles.

>
> Could it fit one line less?

Yes. Will change.

>> +
>> +#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)
>
> I didn't remembter if I told you, but please use s->private for the
> imr_dev pointer.
> Everywhere in debugfs calls and if possible in other functions as well.

No missed s->private. Will incorporate for V3.

>> +}
>> +
>> +/**
>> + * imr_debugfs_unregister
>> + * Unregister debugfs hooks
>> + *
>> + * @imr:       IMR structure representing address and access masks
>> + * @return:
>> + */
>> +static void imr_debugfs_unregister(void)
>> +{
>> +       if (!imr_dev.file)
>> +               return;
>
> Redundant check. I think I told you that already.

I think you did. V3

>> +static void __init imr_fixup_memmap(void)
>> +{
>> +       unsigned long base  = virt_to_phys(&_text);
>> +       unsigned long size = virt_to_phys(&__end_rodata) - base;
>
> Shouldn't be phys_addr_t ?
> Oh, It might be good for all address parameters in the introduced API.

Well the reference MTRR code doesn't do phs_addr_t
OTOH so what. I think phys_addr_t is more representative of the data 
being passed, so will include @ V3.

>> +               pr_info("protecting kernel .text - .rodata: %ldk (%p - %p)\n",
>> +                       size / 1024, &_text, &__end_rodata);
>
> size >> 10

Andy.

It was size >> 10 for V1 and you called it out as a magic number :)

IMO, size / 1024 requires less thought to understand when reading the code.

>> +
>> +#ifdef CONFIG_DEBUG_IMR_SELFTEST
>> +       /* Run optional self test */
>> +       imr_self_test(base, size);
>> +#endif
>
> I think it makes sense to move this piece to the init.
> I don't see what is exceptional in this function that test belongs here.

Fair enough.

>> +#ifdef CONFIG_DEBUG_FS
>> +       ret = imr_debugfs_register();
>> +       if (ret != 0)
>> +               return ret;
>
> It's non-fatal error.
> Thus,
> if (ret)
>   pr_warn("DebugFS wasn't initialized\n");
>
> Move it after we have imr_dev in place and supply it to debugfs as a
> private pointer.

Agree

>> + * return:
>> + */
>> +static void __exit imr_exit(void)
>> +{
>> +#ifdef CONFIG_DEBUG_FS
>
> I suspect you may remove all those ifdefs and compiler should shrink
> not used code since debugfs has the stubs.
>
>> +       imr_debugfs_unregister();
>> +#endif
>> +}

Hrmm. I'll revist that @ V3.

Thanks for the quick feedback.

--
BOD

Re: [PATCH v2 1/1] x86: Add Isolated Memory Regions for Quark X1000

[Andy Shevchenko]

On Thu, Jan 22, 2015 at 3:27 AM, Bryan O'Donoghue
<pure.logic@nexus-software.ie> wrote:
> On 21/01/15 20:57, Andy Shevchenko wrote:

[]

>>> +       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);
>>
>>
>> Wouldn't be reg++ here as well? Below you substitute full offset which
>> I think points just to next register.
>
>
> I don't think we want to increment below..
>
>>
>>> +       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 - IMR_LOCK_OFF,
>>> imr->addr_lo);
>>> +       }
>
>
> ..because we calculate an offset anyway. An additional increment would just
> be unnecessary cycles.

Offset is a compile-time constant, right? And it should be 4.
Otherwise its meaning somehow looks confusing. I looked again and
would recommend to substitute it by NUM_REGS here and leave register
increment. I don't think it's a really big deal to waste CPU cycles
here since you use slower IOSF communication.

>>> +               pr_info("protecting kernel .text - .rodata: %ldk (%p -
>>> %p)\n",
>>> +                       size / 1024, &_text, &__end_rodata);
>>
>>
>> size >> 10
>
>
> Andy.
>
> It was size >> 10 for V1 and you called it out as a magic number :)
>
> IMO, size / 1024 requires less thought to understand when reading the code.

Oh, my bad. Now a bit modified suggestion, to add KiB inside format
string and leave  / 1024. Would it work for you?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] x86: Add Isolated Memory Regions for Quark X1000

[Bryan O'Donoghue]

On 22/01/15 08:59, Andy Shevchenko wrote:

>> It was size >> 10 for V1 and you called it out as a magic number :)
>>
>> IMO, size / 1024 requires less thought to understand when reading the code.
>
> Oh, my bad. Now a bit modified suggestion, to add KiB inside format
> string and leave  / 1024. Would it work for you?

Sure thing. Makes sense to me.

--
BOD

Re: [PATCH v2 1/1] x86: Add Isolated Memory Regions for Quark X1000

[Thomas Gleixner]

On Wed, 21 Jan 2015, Bryan O'Donoghue wrote:
>  arch/x86/Kconfig           |  25 ++
>  arch/x86/Kconfig.debug     |  13 +
>  arch/x86/include/asm/imr.h |  60 ++++
>  arch/x86/kernel/Makefile   |   1 +
>  arch/x86/kernel/imr.c      | 681 +++++++++++++++++++++++++++++++++++++++++++++

Can we please stop to dump random stuff into x86/kernel?

x86/platform ... would be a proper place for this.

Thanks,

	tglx

Re: [PATCH v2 1/1] x86: Add Isolated Memory Regions for Quark X1000

[Bryan O'Donoghue]

On 22/01/15 11:24, Thomas Gleixner wrote:
> On Wed, 21 Jan 2015, Bryan O'Donoghue wrote:
>>   arch/x86/Kconfig           |  25 ++
>>   arch/x86/Kconfig.debug     |  13 +
>>   arch/x86/include/asm/imr.h |  60 ++++
>>   arch/x86/kernel/Makefile   |   1 +
>>   arch/x86/kernel/imr.c      | 681 +++++++++++++++++++++++++++++++++++++++++++++
>
> Can we please stop to dump random stuff into x86/kernel?
>
> x86/platform ... would be a proper place for this.
>
> Thanks,
>
> 	tglx
>

Hi Thomas.

The IMR registers live in the north cluster of the SoC which is why I 
thought arch/x86/kernel would be the best place for this code.

That said, there's no technical reason we couldn't move this code to

drivers/platform/x86/intel_qrk_imr.c

Darren - would that be acceptable to you ?

Re: [PATCH v2 1/1] x86: Add Isolated Memory Regions for Quark X1000

[Bryan O'Donoghue]

> drivers/platform/x86/intel_qrk_imr.c
>
> Darren - would that be acceptable to you ?

Sorry guys typo - should read arch/x86/platform/imr.c

:)

Re: [PATCH v2 1/1] x86: Add Isolated Memory Regions for Quark X1000

[Bryan O'Donoghue]

On 22/01/15 15:02, Bryan O'Donoghue wrote:
>
>> drivers/platform/x86/intel_qrk_imr.c
>>
>> Darren - would that be acceptable to you ?
>
> Sorry guys typo - should read arch/x86/platform/imr.c
>
> :)

Ah Thomas actually if I'm understanding you correctly here

The intention would be to add arch/x86/platform/intel-quark/imr.c and to 
add X86_QUARK depending on X86_EXTENDED_PLATFORM

Which would then include IMR support

Re: [PATCH v2 1/1] x86: Add Isolated Memory Regions for Quark X1000

[Darren Hart]

On Thu, Jan 22, 2015 at 03:15:26PM +0000, Bryan O'Donoghue wrote:
> On 22/01/15 15:02, Bryan O'Donoghue wrote:
> >
> >>drivers/platform/x86/intel_qrk_imr.c
> >>
> >>Darren - would that be acceptable to you ?
> >
> >Sorry guys typo - should read arch/x86/platform/imr.c
> >
> >:)
> 
> Ah Thomas actually if I'm understanding you correctly here
> 
> The intention would be to add arch/x86/platform/intel-quark/imr.c and to add

Right, that was my understanding.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v2 1/1] x86: Add Isolated Memory Regions for Quark X1000

[Thomas Gleixner]

On Thu, 22 Jan 2015, Bryan O'Donoghue wrote:
> On 22/01/15 15:02, Bryan O'Donoghue wrote:
> > 
> > > drivers/platform/x86/intel_qrk_imr.c
> > > 
> > > Darren - would that be acceptable to you ?
> > 
> > Sorry guys typo - should read arch/x86/platform/imr.c
> > 
> > :)
> 
> Ah Thomas actually if I'm understanding you correctly here
> 
> The intention would be to add arch/x86/platform/intel-quark/imr.c and to add
> X86_QUARK depending on X86_EXTENDED_PLATFORM
> 
> Which would then include IMR support

That makes sense, especially if there is more Quark in the pipeline.

Thanks,

	tglx
---
German		English
Quark 		curd  [cheese]
Quark		quark [physics]
Quark		nonsense [slang]

RE: [PATCH v2 1/1] x86: Add Isolated Memory Regions for Quark X1000

[Ong, Boon Leong]

Hi Bryan, there are 1 serious bug & couple of minor bugs that I caught... please fix

>-----Original Message-----
>From: Bryan O'Donoghue [mailto:pure.logic@nexus-software.ie]
>Sent: Wednesday, January 21, 2015 10:46 AM
>To: tglx@linutronix.de; mingo@redhat.com; hpa@zytor.com; x86@kernel.org;
>dvhart@infradead.org; andy.shevchenko@gmail.com; Ong, Boon Leong; linux-
>kernel@vger.kernel.org
>Cc: Bryan O'Donoghue
>Subject: [PATCH v2 1/1] x86: Add Isolated Memory Regions for Quark X1000
>
>From V1 comment:
Suggest to add a statement on 3 different types of IMR: General IMR, Host Memory
I/O Boundary IMR & System Management Mode IMR. Then, call out that this patch
is meant to support general IMR type only.

>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 KiB.
>
>Quark_SecureBootPRM_330234_001.pdf section 4.5 details the concept of IMRs
>quark-x1000-datasheet.pdf section 12.7.4 details the implementation of IMRs
>in silicon.
>
>eSRAM flush, CPU Snoop, CPU SMM Mode, CPU non-SMM mode, RMU and PCIe
>Virtual
>Channels (VC0 and VC1) can have individual read/write access masks applied
>to them for a given memory region in Quark X1000. This enables IMRs to treat
>each memory transaction type listed above on an individual basis and to
>filter appropriately based on the IMR access mask for the memory region.
>Quark supports eight IMRs.
For CPU Snoop, to add (write-only) 

<snip>

>+
>+/**
>+ * imr_enabled - true if an IMR is enabled false otherwise
>+ *
>+ * Determines if an IMR is enabled based on address range and read/write
>+ * mask. An IMR set with an address range set to zero and a read/write
>+ * access mask set to all is considered to be disabled. An IMR in any
>+ * other state - for example set to zero but without read/write access
>+ * all is considered to be enabled. This definition of disabled is how
>+ * firmware switches off an IMR and is maintained in kernel for
>+ * consistency.
>+ *
>+ * @imr:	pointer to IMR descriptor
>+ * @return:	true if IMR enabled false if disabled
>+ */
>+static int imr_enabled(struct imr_regs *imr)
Do we want to make it inline perhaps since it is 1 liner?

>+{
>+	return (imr->rmask != IMR_READ_ACCESS_ALL ||
>+		imr->wmask != IMR_WRITE_ACCESS_ALL ||
>+		imr_to_phys(imr->addr_lo) ||
>+		imr_to_phys(imr->addr_hi));
>+}
>+


>+/**
>+ * imr_write - write an IMR at a given 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_regs *imr, bool lock)
>+{
<snip>

>+
>+	/* 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 - IMR_LOCK_OFF, imr->addr_lo);
>+	}
>+
A bug ... 
We should take ret from above into consideration and not assume it always ret=0 below

>+	local_irq_restore(flags);
>+	return 0;
>+done:
>+	/*
>+	 * 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.
>+	 */
>+	local_irq_restore(flags);
>+	WARN(ret, "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;
>+}
>+
<snip>

>+
>+/**
>+ * imr_fixup_size - account for the IMR_ALIGN bytes that addr_hi appends
>+ *
>+ * IMR addr_hi has a built in offset of plus IMR_ALIGN (0x400) bytes from the
>+ * value in the register. We need to subtract IMR_ALIGN bytes from input sizes
>+ * as a result
>+ *
>+ * @size:	input size bytes
>+ * @return:	reduced size
>+ */
>+static unsigned long imr_fixup_size(unsigned long size)
We can make it inline perhaps since it is 1 liner ?

>+{
>+	return size - IMR_ALIGN;
>+}
>+

>+/**
>+ * imr_address_overlap - detects an address overlap
>+ *
>+ * @addr:	address to check against an existing IMR
>+ * @imr:	imr being checked
>+ * @return:	true for overlap false for no overlap
>+ */
Inline?

>+static int imr_address_overlap(unsigned long addr, struct imr_regs *imr)
>+{
>+	return addr >= imr_to_phys(imr->addr_lo) && addr <=
>imr_to_phys(imr->addr_hi);
>+}


>+
>+/**
>+ * imr_add_range - add an Isolated Memory Region
>+ *
>+ * @base:	physical base address of region aligned to 1KiB
>+ * @size:	physical size of region in bytes must be aligned to 1KiB
>+ * @read_mask:	read access mask
>+ * @write_mask:	write access mask
>+ * @lock:	indicates whether or not to permanently lock this region
>+ * @return:	index of the IMR allocated or negative value indicating error
>+ */
>+int imr_add_range(unsigned long base, unsigned long size,
>+	    unsigned int rmask, unsigned int wmask, bool lock)
>+{
>+	unsigned long end = base + size;
>+	int i;
>+	struct imr_regs imr;
>+	int reg;
>+	int ret;
>+
>+	if (imr_dev.init == false)
>+		return -ENODEV;
>+
>+	ret = imr_check_range(base, size);
>+	if (ret)
>+		return ret;
>+
>+	if (size < IMR_ALIGN)
>+		return -EINVAL;
I believe this is redundant because imr_check_range() has test for (size & IMR_MASK) 
which means if the size is indeed smaller than 0x400, the test will caught it anyway. 

>+
>+	/* Tweak the size value */
>+	size = imr_fixup_size(size);
>+
>+	mutex_lock(&imr_dev.lock);
>+
>+	/*
>+	 * Find a free IMR while checking for an existing overlapping range.
>+	 * Note there's no restriction in silicon to prevent IMR overlaps.
>+	 * For the sake of simplicity and ease in defining/debugging an IMR
>+	 * memory map we exclude IMR overlaps.
>+	 */
>+	reg = -1;
>+	for (i = 0; i < imr_dev.max_imr; i++) {
>+		ret = imr_read(i, &imr);
>+		if (ret)
>+			goto done;
>+
>+		/* Find overlap @ base or end of requested range */
>+		if (imr_enabled(&imr)) {
>+			if (imr_address_overlap(base, &imr)) {
>+				ret = -EINVAL;
>+				goto done;
>+			}
>+			if (imr_address_overlap(end, &imr)) {
>+				ret = -EINVAL;
>+				goto done;
>+			}
>+		} else {
>+			reg = i;
>+		}
>+	}
>+
>+	/* Error out if we have no free IMR entries */
>+	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);
Do we want to account for the 'size fixup' above on 'end'
>+
>+	/* Allocate IMR */
>+	imr.addr_lo = phys_to_imr(base);
>+	imr.addr_hi = phys_to_imr(end);

The fix-up size above is never factored here ...
'end-size' should be the correct one

>+	imr.rmask = rmask;
>+	imr.wmask = wmask;
>+
>+	ret = imr_write(reg, &imr, lock);
>+
>+done:
>+	mutex_unlock(&imr_dev.lock);
>+	return ret == 0 ? reg : ret;
>+}
>+EXPORT_SYMBOL(imr_add_range);



>+
>+/**
>+ * imr_remove_range - delete an Isolated Memory Region
>+ *
>+ * This function allows you to delete an IMR by it's index specified by reg or
It's --> its

>+ * by address range specified by base and size respectively. If you specify an
>+ * index on it's own the base and size parameters are ignored.
its ..

>+ * imr_remove_range(0, size, base); delete IMR at index 0 base/size ignored
>+ * imr_remove_range(-1, base, size); delete IMR from base to base+size
>+ *
>+ * @reg:	imr index to remove
>+ * @base:	physical base address of region aligned to 4k
It should be 1-KiB. 

>+ * @size:	physical size of region in bytes
Please add " aligned to 1-KiB"

>+ * @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_remove_range(int reg, unsigned long base, unsigned long size)
>+{
>+	struct imr_regs imr;
>+	int found = 0, i, ret = 0;
Please make each of the defined variables as individual line here..

>+	unsigned long  max = base + size;
>+
>+	if (imr_dev.init == false)
>+		return -ENODEV;
>+
>+	if (imr_check_range(base, size))
>+		return -EINVAL;
>+
>+	if (reg >= imr_dev.max_imr)
>+		return -EINVAL;
>+
>+	/* Tweak the size value */
>+	size = imr_fixup_size(size);
>+
>+	mutex_lock(&imr_dev.lock);
>+
>+	if (reg >= 0) {
>+		/* If a specific IMR is given try to use it */
>+		ret = imr_read(reg, &imr);
>+		if (ret)
>+			goto done;
>+
>+		if (!imr_enabled(&imr) || imr.addr_lo & IMR_LOCK) {
>+			ret = -ENODEV;
>+			goto done;
>+		}
>+		found = 1;
>+
>+	} else {
>+		/* Search for match based on address range */
>+		for (i = 0; i < imr_dev.max_imr; i++) {
>+			ret = imr_read(reg, &imr);
A serious bug here.... 'reg' should be 'i' . We enter this branch if reg=-1
Is there a miss in your test case? 

>+			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)) {
I think we need to take care of the size that has been fix-up here ...

>+				found = 1;
>+				reg = i;
>+				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_remove_range);
>+
>+#ifdef CONFIG_DEBUG_IMR_SELFTEST
<snip>

>+/**
>+ * imr_self_test
>+ *
>+ * Verify IMR self_test with some simple tests to verify overlap,
>+ * zero sized allocations and 1 KiB sized areas.
>+ *
>+ * @base:	physical base address of the kernel text section
>+ * @size:	extent of kernel memory to be covered from &_text to
>&__end_rodata
>+ */
>+static void __init imr_self_test(unsigned long base, unsigned long size)
>+{
>+	const char *fmt_over = "overlapped IMR @ (0x%08lx - 0x%08lx)\n";
>+	int idx;
>+
>+	/* Test zero zero */
>+	idx = imr_add_range(0, 0, 0, 0, false);
>+	imr_self_test_result(idx < 0, "zero sized IMR\n");
>+
>+	/* Test exact overlap */
>+	idx = imr_add_range(base, size, IMR_CPU, IMR_CPU, false);
>+	imr_self_test_result(idx < 0, fmt_over, __va(base), __va(base + size));
>+
>+	/* Test overlap with base inside of existing */
>+	base += size - IMR_ALIGN;
>+	idx = imr_add_range(base, size, IMR_CPU, IMR_CPU, false);
>+	imr_self_test_result(idx < 0, fmt_over, __va(base), __va(base + size));
>+
>+	/* Test overlap with end inside of existing */
>+	base -= size + IMR_ALIGN * 2;
>+	idx = imr_add_range(base, size, IMR_CPU, IMR_CPU, false);
>+	imr_self_test_result(idx < 0, fmt_over, __va(base), __va(base + size));
>+
>+	/* Test 1 KiB works */
>+	idx = imr_add_range(0, IMR_ALIGN, IMR_READ_ACCESS_ALL,
>+			    IMR_WRITE_ACCESS_ALL, false);
>+	imr_self_test_result(idx >= 0, "1KiB IMR @ 0x00000000\n");
>+
>+	/* Tear-tow 1 KiB if previous was successful */
>+	if (idx >= 0) {
>+		idx = imr_remove_range(idx, 0, 0);
>+		imr_self_test_result(idx == 0, "teardown 1KiB @
>0x00000000\n");
>+	}
Ok, since there is a serious bug for imr_remove_range(-1, ... ), please add a test case for that. 

>+}
>+#endif /* CONFIG_DEBUG_IMR_SELFTEST */
>+
>+/**
>+ * imr_fixup_memmap - 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.
>+ * As a result if you're running a root filesystem from SD - you'll need
>+ * the boot-time IMRs torn down or you'll find seemingly random resets when
>+ * using your filesystem.
>+ */
>+static void __init imr_fixup_memmap(void)
>+{
>+	unsigned long base  = virt_to_phys(&_text);
>+	unsigned long size = virt_to_phys(&__end_rodata) - base;
What about the size fixup to be consistent? 
We should not guard more than it is needed ..... 

>+	int i, ret;
Two int declaration line here.

>+
>+	/* Tear down all existing unlocked IMRs */
>+	for (i = 0; i < imr_dev.max_imr; i++)
>+		imr_remove_range(i, 0, 0);
>+
>+	/*
>+	 * Setup a locked IMR around the physical extent of the kernel
>+	 * from the beginning of the .text secton to the end of the
>+	 * .rodata section.
>+	 *
>+	 * This behaviour relies on the kernel .text and .rodata
>+	 * sections being physically contiguous and .rodata ending on 1
>+	 * KiB aligned boundary - such as a page boundary. Linker script
>+	 * The definition of this memory map is in:
>+	 * arch/x86/kernel/vmlinux.lds
>+	 * .text begin = &_stext
>+	 * .rodata end = &__end_rodata - aligned to 4KiB
>+	 */
>+	ret = imr_add_range(base, size, IMR_CPU, IMR_CPU, true);
>+	if (ret < 0)
>+		pr_err("unable to setup IMR for kernel: (%p - %p)\n",
>+			&_text, &__end_rodata);
>+	else
>+		pr_info("protecting kernel .text - .rodata: %ldk (%p - %p)\n",
>+			size / 1024, &_text, &__end_rodata);
>+
>+#ifdef CONFIG_DEBUG_IMR_SELFTEST
>+	/* Run optional self test */
>+	imr_self_test(base, size);
>+#endif
>+}
>+
>+static const struct x86_cpu_id imr_ids[] __initconst = {
>+	{ X86_VENDOR_INTEL, 5, 9 },	/* Intel Quark SoC X1000 */
>+	{}
>+};
>+MODULE_DEVICE_TABLE(x86cpu, imr_ids);
>+
>+/**
>+ * imr_probe - entry point for IMR driver
>+ *
>+ * return: -ENODEV for no IMR support 0 if good to go
>+ */
>+static int __init imr_init(void)
>+{
>+	int ret;
>+
>+	if (!x86_match_cpu(imr_ids) || !iosf_mbi_available())
>+		return -ENODEV;
>+
>+#ifdef CONFIG_DEBUG_FS
>+	ret = imr_debugfs_register();
>+	if (ret != 0)
>+		return ret;
>+#endif
>+
>+	imr_dev.max_imr = QUARK_X1000_IMR_MAX;
>+	imr_dev.reg_base = QUARK_X1000_IMR_REGBASE;
>+
>+	mutex_init(&imr_dev.lock);
>+
>+	imr_dev.init = true;
>+	imr_fixup_memmap();
>+
>+	return 0;
>+}
>+
>+/**
>+ * imr_exit - exit point for IMR code
>+ * Deregisters debugfs, leave IMR state as-is.
>+ *
>+ * return:
>+ */
>+static void __exit imr_exit(void)
>+{
>+#ifdef CONFIG_DEBUG_FS
>+	imr_debugfs_unregister();
>+#endif
>+}
>+
>+module_init(imr_init);
>+module_exit(imr_exit);
>+
>+MODULE_AUTHOR("Bryan O'Donoghue <pure.logic@nexus-software.ie>");
>+MODULE_DESCRIPTION("Intel Isolated Memory Region driver");
>+MODULE_LICENSE("GPL");
>--
>1.9.1

Re: [PATCH v2 1/1] x86: Add Isolated Memory Regions for Quark X1000

[Andy Shevchenko]

On Sat, Jan 24, 2015 at 3:48 AM, Ong, Boon Leong
<boon.leong.ong@intel.com> wrote:

>>+static int imr_enabled(struct imr_regs *imr)
> Do we want to make it inline perhaps since it is 1 liner?

Since it is declared static I would even suggest the new name is_imr_enabled().

[]

>>+int imr_remove_range(int reg, unsigned long base, unsigned long size)
>>+{
>>+      struct imr_regs imr;
>>+      int found = 0, i, ret = 0;
> Please make each of the defined variables as individual line here..

I would suggest to type i as unsigned int and found as bool.

[]

>>+              if (!imr_enabled(&imr) || imr.addr_lo & IMR_LOCK) {
>>+                      ret = -ENODEV;
>>+                      goto done;
>>+              }
>>+              found = 1;
>>+

Redundant empty line.

>>+      } else {
>>+              /* Search for match based on address range */
>>+              for (i = 0; i < imr_dev.max_imr; i++) {
>>+                      ret = imr_read(reg, &imr);
> A serious bug here.... 'reg' should be 'i' . We enter this branch if reg=-1
> Is there a miss in your test case?
>
>>+                      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)) {
> I think we need to take care of the size that has been fix-up here ...
>
>>+                              found = 1;
>>+                              reg = i;

According to your comment this line becomes redundant.

[]

>>+static void __init imr_fixup_memmap(void)
>>+{
>>+      unsigned long base  = virt_to_phys(&_text);
>>+      unsigned long size = virt_to_phys(&__end_rodata) - base;
> What about the size fixup to be consistent?
> We should not guard more than it is needed .....
>
>>+      int i, ret;
> Two int declaration line here.

unsigned int i; ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] x86: Add Isolated Memory Regions for Quark X1000

[Bryan O'Donoghue]

On 24/01/15 01:48, Ong, Boon Leong wrote:

Skipping stuff I agree with.

>  From V1 comment:
> Suggest to add a statement on 3 different types of IMR: General IMR, Host Memory
> I/O Boundary IMR & System Management Mode IMR. Then, call out that this patch
> is meant to support general IMR type only.

Hmm - There's no mention of grouping like that in the documentation, nor 
in released silicon - to my knowledge.

Also why do you want a statement added saying that it supports CPU only 
mode ?

This patch will support adding IMRs for SMM mode - if calling code wants 
do do that - it's just imr_add_range(base, size, SMM, SMM);

Same thing with virtual-channels, RMU, etc.


>> +	ret = imr_check_range(base, size);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (size < IMR_ALIGN)
>> +		return -EINVAL;
> I believe this is redundant because imr_check_range() has test for (size & IMR_MASK)
> which means if the size is indeed smaller than 0x400, the test will caught it anyway.

Nope.

(0 & 0x3FF) == 0

We need to bounds check for a zero size.

I'll change it to

if (size == 0)
	return -EINVAL;

to avoid confusion.

>> +
>> +	/* Tweak the size value */
>> +	size = imr_fixup_size(size);
>> +	pr_debug("IMR %d phys 0x%08lx-0x%08lx rmask 0x%08x wmask
>> 0x%08x\n",
>> +		reg, base, end, rmask, wmask);
> Do we want to account for the 'size fixup' above on 'end'
>> +
>> +	/* Allocate IMR */
>> +	imr.addr_lo = phys_to_imr(base);
>> +	imr.addr_hi = phys_to_imr(end);
>
> The fix-up size above is never factored here ...
> 'end-size' should be the correct one

hmmm.

The correct fix is

size = imr_fixup_size(size);
end = base + size;

>> +	} else {
>> +		/* Search for match based on address range */
>> +		for (i = 0; i < imr_dev.max_imr; i++) {
>> +			ret = imr_read(reg, &imr);
> A serious bug here.... 'reg' should be 'i' . We enter this branch if reg=-1
> Is there a miss in your test case?

Hmm you're right.

Turns out there's only the one test case for imr_del_range();

Good catch.

--
BOD

Re: [PATCH v2 1/1] x86: Add Isolated Memory Regions for Quark X1000

[Bryan O'Donoghue]

On 24/01/15 11:02, Andy Shevchenko wrote:
> On Sat, Jan 24, 2015 at 3:48 AM, Ong, Boon Leong
> <boon.leong.ong@intel.com> wrote:
>
>>> +static int imr_enabled(struct imr_regs *imr)
>> Do we want to make it inline perhaps since it is 1 liner?
>
> Since it is declared static I would even suggest the new name is_imr_enabled().

I think imr_is_enabled() is a better name. Every other function is imr_ 
prefixed.

>>> +
>>> +                      if ((imr_to_phys(imr.addr_lo) == base) &&
>>> +                          (imr_to_phys(imr.addr_hi) == max)) {
>> I think we need to take care of the size that has been fix-up here ...
>>
>>> +                              found = 1;
>>> +                              reg = i;
>
> According to your comment this line becomes redundant.
>

Ah but I still think though.

We eventually do an imr_write(reg, &imr); which I think reads better 
than imr_write(i, &imr);

--
BOD

Re: [PATCH v2 1/1] x86: Add Isolated Memory Regions for Quark X1000

[Bryan O'Donoghue]

On 24/01/15 21:56, Bryan O'Donoghue wrote:
>>>> +
>>>> +                      if ((imr_to_phys(imr.addr_lo) == base) &&
>>>> +                          (imr_to_phys(imr.addr_hi) == max)) {
>>> I think we need to take care of the size that has been fix-up here ...
>>>
>>>> +                              found = 1;
>>>> +                              reg = i;
>>
>> According to your comment this line becomes redundant.
>>
>
> Ah but I still think though.

"Ah but I still think *I'll keep* that though

:)

> We eventually do an imr_write(reg, &imr); which I think reads better
> than imr_write(i, &imr);
>
> --
> BOD