* [PATCH 0/2] x86: Add IMR support to Quark/Galileo @ 2014-12-29 17:23 Bryan O'Donoghue 2014-12-29 17:23 ` [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 Bryan O'Donoghue ` (4 more replies) 0 siblings, 5 replies; 47+ messages in thread From: Bryan O'Donoghue @ 2014-12-29 17:23 UTC (permalink / raw) To: tglx, mingo, hpa, x86, dvhart, platform-driver-x86, linux-kernel Cc: Bryan O'Donoghue This patchset adds an IMR driver to the kernel plus platform code for Intel Galileo Gen1/Gen2 boards. IMRs: Quark SoC X1000 ships with a set of registers called Isolated Memory Regions IMRs provide fine grained memory access control to various system agents within the SoC such as CPU SMM/non-SMM mode, PCIe virtual channels, CPU snoop cycles, eSRAM flush cycles and the RMU. In simple terms, IMRs provide a mechanism to protect memory regions from unwarranted access by system agents that should not have access to that memory. IMRs support a lock bit. Once a lock bit is set for an individual IMR it is not possible to tear down that IMR without performing a cold boot of the system. IMRs support reporting of violations. The SoC system can be configured to reboot immediately when an IMR violation has taken place. Immediate reboot of the system on IMR violation is recommended and is currently how Quark BIOS configures the system. As an example Galileo boards ship with an IMR around the ACPI runtime services memory and if a DMA read/write cycle were to occur to this region of memory this would trigger the IMR violation mechansim. Galileo: Intel's Arduino compatible Galileo boards boot to Linux with IMRs protecting the compressed kernel image and boot params data structure. The memory that the compressed kernel and boot params data structure is in, is marked as usable memory by the EFI memory map. As a result it is possible for memory marked as processor read/write only in an IMR to be given to devices in the SoC for the purposes of DMA by way of dma_alloc_coherent. A DMA to a region of memory by a system agent which is not allowed access this memory result in a system reset. Without tearing down the IMRs placed around the compressed kernel image and boot params data structure there is a high risk of triggering an inadvertent system reset when performing DMA actions with any of the peripherals that support DMA in Quark such as the MMC, Ethernet or USB host/device. Therefore Galileo specific platform code is the second component of this patchset. The platform code tears-down every unlocked IMR to ensure no conflict exists between the IMR usage during boot and the EFI memory map. In addition an IMR is placed around the kernel's .text section to ensure no invalid access to kernel code can happen by way of spurious DMA, SMM or RMU read/write cycles. This code gets compiled into the kernel because we want to run the code early before any DMA has taken place. The prime examples of DMA transactions resetting the system are mouting a root filesystem on MMC or mouting a root filesystem over NFS. Bryan O'Donoghue (2): x86: Add Isolated Memory Regions for Quark X1000 platform/x86 Add Intel Galileo platform specific setup arch/x86/Kconfig | 23 ++ arch/x86/include/asm/imr.h | 79 ++++++ arch/x86/include/asm/intel-quark.h | 31 ++ arch/x86/kernel/Makefile | 1 + arch/x86/kernel/imr.c | 529 +++++++++++++++++++++++++++++++++++ drivers/platform/x86/Kconfig | 15 + drivers/platform/x86/Makefile | 1 + drivers/platform/x86/intel_galileo.c | 175 ++++++++++++ 8 files changed, 854 insertions(+) create mode 100644 arch/x86/include/asm/imr.h create mode 100644 arch/x86/include/asm/intel-quark.h create mode 100644 arch/x86/kernel/imr.c create mode 100644 drivers/platform/x86/intel_galileo.c -- 1.9.1 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2014-12-29 17:23 [PATCH 0/2] x86: Add IMR support to Quark/Galileo Bryan O'Donoghue @ 2014-12-29 17:23 ` Bryan O'Donoghue 2014-12-31 15:05 ` Andy Shevchenko ` (2 more replies) 2014-12-29 17:23 ` [PATCH 2/2] platform/x86: Add Intel Galileo platform specific setup Bryan O'Donoghue ` (3 subsequent siblings) 4 siblings, 3 replies; 47+ messages in thread From: Bryan O'Donoghue @ 2014-12-29 17:23 UTC (permalink / raw) To: tglx, mingo, hpa, x86, dvhart, platform-driver-x86, linux-kernel Cc: 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 kilobyte. Quark_SecureBootPRM_330234_001.pdf section 4.5 details the concept of IMRs eSRAM flush, CPU Snoop, CPU SMM Mode, CPU non-SMM mode, PMU and VC0/VC1 can have individual read/write access masks applied to them for a given memory region in Quark X1000. Quark supports eightIMR sets. Since all of the DMA capable SoC components in the X1000 are mapped to VC0 it is possible to define sections of memory as invalid for DMA write operations originating from Ethernet, USB, SD and any other DMA capable south-cluster component on VC0. Similarly it is possible to mark kernel memory as non-SMM mode read/write only or to mark BIOS runtime memory as SMM mode accessible only depending on the particular memory footprint on a given system. On an IMR violation Quark SoC X1000 systems are configured to reset the system, so ensuring that the IMR memory map agrees with the EFI provided memory map is critical to ensure no IMR violations reset the system. The API for accessing IMRs is based on MTRR code but doesn't provide a /proc or /sys interface to manipulate IMRs. Defining the size and extent of IMRs is exclusively the domain of in-kernel code. Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie> --- arch/x86/Kconfig | 23 ++ arch/x86/include/asm/imr.h | 79 ++++++ arch/x86/include/asm/intel-quark.h | 31 +++ arch/x86/kernel/Makefile | 1 + arch/x86/kernel/imr.c | 529 +++++++++++++++++++++++++++++++++++++ 5 files changed, 663 insertions(+) create mode 100644 arch/x86/include/asm/imr.h create mode 100644 arch/x86/include/asm/intel-quark.h create mode 100644 arch/x86/kernel/imr.c diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index ba397bd..8303ca2 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -526,6 +526,29 @@ config IOSF_MBI_DEBUG If you don't require the option or are in doubt, say N. +config IMR + tristate "Isolated Memory Region support" + default m + depends on IOSF_MBI + ---help--- + This option enables support for Isolated Memory Regions. + IMRs are a set of registers that define read and write access masks + to prohibit certain system agents from accessing memory with 1k + granularity. + IMRs make it possible to control read/write access to an address + by hardware agents inside the SoC. Read and write masks can be + defined for + - SMM mode + - Non SMM mode + - PCI VC0/VC1 + - CPU snoop + - eSRAM flush + - PMU access + Quark contains a set of IMR registers and makes use of those + registers during it's bootup process. + + If you are running on a Galileo/Quark say Y here + config X86_RDC321X bool "RDC R-321x SoC" depends on X86_32 diff --git a/arch/x86/include/asm/imr.h b/arch/x86/include/asm/imr.h new file mode 100644 index 0000000..fe8f3b8 --- /dev/null +++ b/arch/x86/include/asm/imr.h @@ -0,0 +1,79 @@ +/* + * imr.h: Isolated Memory Region API + * + * Copyright(c) 2013 Intel Corporation. + * Copyright(c) 2014 Bryan O'Donoghue <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 <asm/intel-quark.h> +#include <linux/types.h> + +/* + * Right now IMRs are not reported via CPUID though it'd be really great if + * future silicon did report via CPUID for this feature bringing it in-line with + * other similar features - like MTRRs, MSRs etc. + * + * A macro is defined here which is an analog to the other cpu_has_x type + * features + */ +#define cpu_has_imr cpu_is_quark + +/* IMR agent access mask bits */ +#define IMR_ESRAM_FLUSH 0x80000000 +#define IMR_CPU_SNOOP 0x40000000 +#define IMR_HB_ACCESS 0x20000000 +#define IMR_VC1_ID3 0x00008000 +#define IMR_VC1_ID2 0x00004000 +#define IMR_VC1_ID1 0x00002000 +#define IMR_VC1_ID0 0x00001000 +#define IMR_VC0_ID3 0x00000800 +#define IMR_VC0_ID2 0x00000400 +#define IMR_VC0_ID1 0x00000200 +#define IMR_VC0_ID0 0x00000100 +#define IMR_SMM 0x00000002 +#define IMR_NON_SMM 0x00000001 +#define IMR_ACCESS_NONE 0x00000000 + +/* Definition of read/write masks from published BSP code */ +#define IMR_READ_ACCESS_ALL 0xBFFFFFFF +#define IMR_WRITE_ACCESS_ALL 0xFFFFFFFF + +/* Number of IMRs provided by Quark X1000 SoC */ +#define QUARK_X1000_IMR_NUM 0x07 +#define QUARK_X1000_IMR_REGBASE 0x40 + +/* IMR alignment bits - only bits 31:10 are checked for IMR validity */ +#define IMR_ALIGN 0x400 +#define IMR_MASK (IMR_ALIGN - 1) + +/** + * imr_add_range - Add an Isolated Memory Region + * @base: Physical base address of region aligned to 4k + * @size: Physical size of region in bytes + * @read_mask: Read access mask + * @write_mask: Write access mask + * @lock: Indicates whether or not to permenantly lock this region + * @return: Index of the IMR allocated or negative value indicating error + */ +int imr_add(unsigned long base, unsigned long size, + unsigned int rmask, unsigned int wmask, bool lock); + +/** + * imr_del_range - Delete an Isolated Memory Region + * @reg: IMR index to remove + * @base: Physical base address of region aligned to 4k + * @size: Physical size of region in bytes + * @return: -EINVAL on invalid range or out or range id + * -ENODEV if reg is valid but no IMR exists or is locked + * 0 on success + */ +int imr_del(int reg, unsigned long base, unsigned long size); + +#endif /* _IMR_H */ diff --git a/arch/x86/include/asm/intel-quark.h b/arch/x86/include/asm/intel-quark.h new file mode 100644 index 0000000..f51ac8c --- /dev/null +++ b/arch/x86/include/asm/intel-quark.h @@ -0,0 +1,31 @@ +/* + * intel-quark.h: Quark shared defines and helper functions + * + * Copyright 2014 Bryan O'Donoghue <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 _ASM_X86_INTEL_QUARK_H +#define _ASM_X86_INTEL_QUARK_H + +#include <asm/processor.h> + +#ifdef CONFIG_X86_32 +static inline int cpu_is_quark(const struct cpuinfo_x86 *c) +{ + return (c->x86_vendor == X86_VENDOR_INTEL && + c->x86 == 5 && c->x86_model == 9); +} +#else +static inline int cpu_is_quark(const struct cpuinfo_x86 *c) +{ + return 0; +} +#endif + +#endif /* _ASM_X86_INTEL_QUARK_H */ + diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile index 5d4502c..0252de5 100644 --- a/arch/x86/kernel/Makefile +++ b/arch/x86/kernel/Makefile @@ -104,6 +104,7 @@ obj-$(CONFIG_EFI) += sysfb_efi.o obj-$(CONFIG_PERF_EVENTS) += perf_regs.o obj-$(CONFIG_TRACING) += tracepoint.o obj-$(CONFIG_IOSF_MBI) += iosf_mbi.o +obj-$(CONFIG_IMR) += imr.o obj-$(CONFIG_PMC_ATOM) += pmc_atom.o ### diff --git a/arch/x86/kernel/imr.c b/arch/x86/kernel/imr.c new file mode 100644 index 0000000..4115138 --- /dev/null +++ b/arch/x86/kernel/imr.c @@ -0,0 +1,529 @@ +/** + * intel_imr.c + * + * Copyright(c) 2013 Intel Corporation. + * Copyright(c) 2014 Bryan O'Donoghue <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 refernece code from Intel's + * Quark BSP EFI, Linux and grub code. + */ +#include <asm/intel-quark.h> +#include <asm/imr.h> +#include <asm/iosf_mbi.h> +#include <linux/debugfs.h> +#include <linux/init.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/types.h> + +struct imr_device { + struct dentry *debugfs_dir; + struct mutex lock; + int num; + int used; + int reg_base; +}; + +static struct imr_device imr_dev; + +/** + * Values derived from published code in Quark BSP + * + * addr_lo + * 31 Lock bit + * 30 Enable bit - not implemented on Quark X1000 + * 29:25 Reserved + * 24:2 1 kilobyte aligned lo address + * 1:0 Reserved + * + * addr_hi + * 31:25 Reserved + * 24:2 1 kilobyte aligned hi address + * 1:0 Reserved + */ +#define IMR_LOCK 0x80000000 +#define IMR_ENABLE 0x04000000 + +struct imr { + u32 addr_lo; + u32 addr_hi; + u32 rmask; + u32 wmask; +}; + +#define IMR_NUM_REGS (sizeof(struct imr)/sizeof(u32)) +#define IMR_SHIFT 8 +#define imr_to_phys(x) (x << IMR_SHIFT) +#define phys_to_imr(x) (x >> IMR_SHIFT) + +/** + * imr_enabled + * Determines if an IMR is enabled based on address range + * + * @imr: Pointer to IMR descriptor + * @return true if IMR enabled false if disabled + */ +static int imr_enabled(struct imr *imr) +{ + return (imr_to_phys(imr->addr_lo) && imr_to_phys(imr->addr_hi)); +} + +/** + * imr_read + * Read an IMR at a given imr index. Requires caller to hold imr mutex + * + * @imr_id: IMR entry to read + * @imr: IMR structure representing address and access masks + * @return 0 on success or error code passed from mbi_iosf on failure + */ +static int imr_read(u32 imr_id, struct imr *imr) +{ + u32 reg = imr_dev.reg_base + (imr_id * IMR_NUM_REGS); + int ret; + + ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ, + reg, &imr->addr_lo); + if (ret) + return ret; + + ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ, + ++reg, &imr->addr_hi); + if (ret) + return ret; + + ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ, + ++reg, &imr->rmask); + if (ret) + return ret; + + return iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ, + ++reg, &imr->wmask); +} + +/** + * imr_write + * Write an IMR at a given imr index. Requires caller to hold imr mutex + * Note lock bits need to be written independently of address bits + * + * @imr_id: IMR entry to write + * @imr: IMR structure representing address and access masks + * @lock: Indicates if the IMR lock bit should be applied + * @return 0 on success or error code passed from mbi_iosf on failure + */ +static int imr_write(u32 imr_id, struct imr *imr, bool lock) +{ + unsigned long flags; + u32 reg = imr_dev.reg_base + (imr_id * IMR_NUM_REGS); + u32 reg_lock = reg; + int ret; + + local_irq_save(flags); + + ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE, reg, + imr->addr_lo); + if (ret) + goto done; + + ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE, + ++reg, imr->addr_hi); + if (ret) + goto done; + + ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE, + ++reg, imr->rmask); + if (ret) + goto done; + + ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE, + ++reg, imr->wmask); + if (ret) + goto done; + + /* Lock bit must be set separately to addr_lo address bits */ + if (lock) { + imr->addr_lo |= IMR_LOCK; + ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE, + reg_lock, imr->addr_lo); + } + +done: + local_irq_restore(flags); + + /* + * If writing to the IOSF failed then we're in an unknown state + * likely a very bad state. An IMR in an invalid state will almost + * certainly lead to a memory access violation. + */ + if (ret) + WARN(1, "IOSF-MBI write fail range 0x%08x-0x%08x unreliable\n", + imr_to_phys(imr->addr_lo), + imr_to_phys(imr->addr_hi) + IMR_MASK); + + return ret; +} + +#ifdef CONFIG_DEBUG_FS + +/** + * imr_dbgfs_state_show + * Print state of IMR registers + * + * @s: pointer to seq_file for output + * @unused: unused parameter + * @return 0 on success or error code passed from mbi_iosf on failure + */ +static int imr_dbgfs_state_show(struct seq_file *s, void *unused) +{ + int i, ret = -ENODEV; + struct imr imr; + u32 size; + + mutex_lock(&imr_dev.lock); + + for (i = 0; i <= imr_dev.num; i++) { + + ret = imr_read(i, &imr); + if (ret) + break; + + /* + * Remember to add IMR_ALIGN bytes to size to indicate the + * inherent IMR_ALIGN size bytes contained in the masked away + * lower ten bits + */ + size = 0; + if (imr_enabled(&imr)) { + size = imr_to_phys(imr.addr_hi) - + imr_to_phys(imr.addr_lo); + size += IMR_ALIGN; + } + seq_printf(s, "imr%02i: base=0x%08x, end=0x%08x, size=0x%08x " + "rmask=0x%08x, wmask=0x%08x, %s, %s\n", i, + imr_to_phys(imr.addr_lo), + imr_to_phys(imr.addr_hi) + IMR_MASK, size, + imr.rmask, imr.wmask, + imr_enabled(&imr) ? "enabled " : "disabled", + imr.addr_lo & IMR_LOCK ? "locked" : "unlocked"); + } + + mutex_unlock(&imr_dev.lock); + + return ret; +} + +/** + * imr_state_open + * Debugfs open callback + * + * @inode: pointer to struct inode + * @file: pointer to struct file + * @return result of single open + */ +static int imr_state_open(struct inode *inode, struct file *file) +{ + return single_open(file, imr_dbgfs_state_show, inode->i_private); +} + +static const struct file_operations imr_state_ops = { + .open = imr_state_open, + .read = seq_read, + .llseek = seq_lseek, + .release = single_release, +}; + +/** + * imr_debugfs_register + * Register debugfs hooks + * + * @imr: IMR structure representing address and access masks + * @return 0 on success - errno on failure + */ +static int imr_debugfs_register(struct imr_device *imr_dev) +{ + struct dentry *dir, *f; + + dir = debugfs_create_dir("imr", NULL); + if (!dir) + return -ENOMEM; + + f = debugfs_create_file("state", S_IFREG | S_IRUGO, + dir, imr_dev, &imr_state_ops); + if (!f) + goto err; + + imr_dev->debugfs_dir = dir; + + return 0; +err: + return -ENODEV; +} + +/** + * imr_debugfs_unregister + * Unregister debugfs hooks + * + * @imr: IMR structure representing address and access masks + * @return none + */ +static void imr_debugfs_unregister(struct imr_device *imr_dev) +{ + if (!imr_dev->debugfs_dir) + return; + + debugfs_remove_recursive(imr_dev->debugfs_dir); + imr_dev->debugfs_dir = NULL; +} + +#else + +/** + * imr_debugfs_register + * Register debugfs hooks + * + * @imr: IMR structure representing address and access masks + * @return 0 on success - errno on failure + */ +static int imr_debugfs_register(struct imr_device *imr_dev) +{ + return 0; +} + +/** + * imr_debugfs_unregister + * Dummy hook for !CONFIG_DEBUG_FS + * + * @imr: IMR structure representing address and access masks + * @return none + */ +static void imr_debugfs_unregister(struct imr_device *imr_dev) +{ +} + +#endif /* CONFIG_DEBUG_FS */ + +/** + * imr_check_range + * Check the passed address range for an IMR is aligned + * + * @base: base address of intended IMR + * @size: size of intended IMR + * @return zero on valid range -EINVAL on unaligned base/size + */ +static int imr_check_range(unsigned long base, unsigned long size) +{ + if ((base & (IMR_MASK)) || (size & (IMR_MASK))) { + pr_warn("imr: base 0x%08lx size 0x%08lx must align to 1k\n", + base, size); + dump_stack(); + return -EINVAL; + } + return 0; +} + +/** + * imr_add_range + * Add an Isolated Memory Region + * + * @base: Physical base address of region aligned to 4k + * @size: Physical size of region in bytes + * @read_mask: Read access mask + * @write_mask: Write access mask + * @lock: Indicates whether or not to permenantly lock this region + * @return: Index of the IMR allocated or negative value indicating error + */ +int imr_add(unsigned long base, unsigned long size, + unsigned int rmask, unsigned int wmask, bool lock) +{ + unsigned long end = base + size; + struct imr imr; + int reg, i, overlap, ret; + + if (imr_check_range(base, size)) + return -EINVAL; + + if (!size) { + pr_warn("imr: invalid size zero!\n"); + return -EINVAL; + } + + mutex_lock(&imr_dev.lock); + + /* Find an free IMR while checking for an existing overlapping range */ + overlap = 0; + reg = -1; + for (i = 0; i <= imr_dev.num; i++) { + ret = imr_read(i, &imr); + if (ret) + goto done; + + /* Find overlap */ + if (imr_enabled(&imr)) { + if (base >= imr_to_phys(imr.addr_lo) && + base <= imr_to_phys(imr.addr_hi)) { + overlap = 1; + break; + } + if (end >= imr_to_phys(imr.addr_lo) && + end <= imr_to_phys(imr.addr_hi)) { + overlap = 1; + break; + } + } else { + reg = i; + } + } + + /* Error out if we have an overlap or no free IMR entries */ + if (overlap) { + ret = -EINVAL; + goto done; + } + if (reg == -1) { + ret = -ENODEV; + goto done; + } + + pr_debug("IMR %d phys 0x%08lx-0x%08lx rmask 0x%08x wmask 0x%08x\n", + reg, base, end, rmask, wmask); + + /* Allocate IMR */ + imr.addr_lo = IMR_ENABLE | phys_to_imr(base); + imr.addr_hi = phys_to_imr(end); + imr.rmask = rmask; + imr.wmask = wmask; + + ret = imr_write(reg, &imr, lock); + +done: + mutex_unlock(&imr_dev.lock); + return ret; +} +EXPORT_SYMBOL(imr_add); + +/** + * imr_del_range + * Delete an Isolated Memory Region + * + * @reg: IMR index to remove + * @base: Physical base address of region aligned to 4k + * @size: Physical size of region in bytes + * @return: -EINVAL on invalid range or out or range id + * -ENODEV if reg is valid but no IMR exists or is locked + * 0 on success + */ +int imr_del(int reg, unsigned long base, unsigned long size) +{ + struct imr imr; + int found = 0, i, ret = 0; + unsigned long max = base + size; + + if (imr_check_range(base, size)) + return -EINVAL; + + if (reg > imr_dev.num) + return -EINVAL; + + mutex_lock(&imr_dev.lock); + + /* if a specific IMR is given try to use it */ + if (reg >= 0) { + ret = imr_read(reg, &imr); + if (ret) + goto done; + + if (!imr_enabled(&imr) || imr.addr_lo & IMR_LOCK) { + ret = -ENODEV; + goto done; + } + found = 1; + } + + /* search for match based on address range */ + for (i = 0; i <= imr_dev.num && found == 0; i++) { + ret = imr_read(reg, &imr); + if (ret) + goto done; + + if (!imr_enabled(&imr) || imr.addr_lo & IMR_LOCK) + continue; + + if ((imr_to_phys(imr.addr_lo) == base) && + (imr_to_phys(imr.addr_hi) == max)) { + found = 1; + reg = i; + } + } + + if (found == 0) { + ret = -ENODEV; + goto done; + } + + /* Tear down the IMR */ + imr.addr_lo = 0; + imr.addr_hi = 0; + imr.rmask = IMR_READ_ACCESS_ALL; + imr.wmask = IMR_WRITE_ACCESS_ALL; + + ret = imr_write(reg, &imr, false); + +done: + mutex_unlock(&imr_dev.lock); + return ret; +} +EXPORT_SYMBOL(imr_del); + +/** + * intel_imr_probe + * entry point for IMR driver + * + * return: -ENODEV for no IMR support 0 if good to go + */ +static int __init intel_imr_init(void) +{ + struct cpuinfo_x86 *c = &cpu_data(cpu); + + if (!cpu_has_imr(c)) + return -ENODEV; + + if (iosf_mbi_available() == false) + return -ENODEV; + + if (cpu_is_quark(c)) { + imr_dev.num = QUARK_X1000_IMR_NUM; + imr_dev.reg_base = QUARK_X1000_IMR_REGBASE; + } else { + pr_err("Unknown IMR implementation\n"); + return -ENODEV; + } + + mutex_init(&imr_dev.lock); + + return imr_debugfs_register(&imr_dev); +} + +/** + * intel_imr_exit + * exit point for IMR code. Deregisters debugfs, leave IMR state as-is. + * + * return: none + */ +static void __exit intel_imr_exit(void) +{ + imr_debugfs_unregister(&imr_dev); +} + +module_init(intel_imr_init); +module_exit(intel_imr_exit); + +MODULE_AUTHOR("Bryan O'Donoghue <pure.logic@nexus-software.ie>"); +MODULE_DESCRIPTION("Intel Isolated Memory Region driver"); +MODULE_LICENSE("GPL"); -- 1.9.1 ^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2014-12-29 17:23 ` [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 Bryan O'Donoghue @ 2014-12-31 15:05 ` Andy Shevchenko 2015-01-01 20:11 ` Bryan O'Donoghue 2015-01-06 7:36 ` Darren Hart 2015-01-08 0:04 ` Ong, Boon Leong 2 siblings, 1 reply; 47+ messages in thread From: Andy Shevchenko @ 2014-12-31 15:05 UTC (permalink / raw) To: Bryan O'Donoghue Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, dvhart, platform-driver-x86, linux-kernel On Mon, Dec 29, 2014 at 7:23 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 kilobyte. 1 KiB? (Check entire patchset). > > Quark_SecureBootPRM_330234_001.pdf section 4.5 details the concept of IMRs Missed dot at the end of sentence. > eSRAM flush, CPU Snoop, CPU SMM Mode, CPU non-SMM mode, PMU and VC0/VC1 can > have individual read/write access masks applied to them for a given memory > region in Quark X1000. Quark supports eightIMR sets. Missed space. > Since all of the DMA capable SoC components in the X1000 are mapped to VC0 > it is possible to define sections of memory as invalid for DMA write > operations originating from Ethernet, USB, SD and any other DMA capable > south-cluster component on VC0. Similarly it is possible to mark kernel > memory as non-SMM mode read/write only or to mark BIOS runtime memory as SMM > mode accessible only depending on the particular memory footprint on a given > system. > > On an IMR violation Quark SoC X1000 systems are configured to reset the > system, so ensuring that the IMR memory map agrees with the EFI provided > memory map is critical to ensure no IMR violations reset the system. > > The API for accessing IMRs is based on MTRR code but doesn't provide a /proc > or /sys interface to manipulate IMRs. Defining the size and extent of IMRs > is exclusively the domain of in-kernel code. > > Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie> > --- > arch/x86/Kconfig | 23 ++ > arch/x86/include/asm/imr.h | 79 ++++++ > arch/x86/include/asm/intel-quark.h | 31 +++ > arch/x86/kernel/Makefile | 1 + > arch/x86/kernel/imr.c | 529 +++++++++++++++++++++++++++++++++++++ > 5 files changed, 663 insertions(+) > create mode 100644 arch/x86/include/asm/imr.h > create mode 100644 arch/x86/include/asm/intel-quark.h > create mode 100644 arch/x86/kernel/imr.c > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index ba397bd..8303ca2 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -526,6 +526,29 @@ config IOSF_MBI_DEBUG > > If you don't require the option or are in doubt, say N. > > +config IMR > + tristate "Isolated Memory Region support" > + default m > + depends on IOSF_MBI > + ---help--- > + This option enables support for Isolated Memory Regions. > + IMRs are a set of registers that define read and write access masks > + to prohibit certain system agents from accessing memory with 1k 1 KiB > + granularity. > + IMRs make it possible to control read/write access to an address > + by hardware agents inside the SoC. Read and write masks can be > + defined for > + - SMM mode > + - Non SMM mode > + - PCI VC0/VC1 > + - CPU snoop > + - eSRAM flush > + - PMU access > + Quark contains a set of IMR registers and makes use of those > + registers during it's bootup process. > + > + If you are running on a Galileo/Quark say Y here > + > config X86_RDC321X > bool "RDC R-321x SoC" > depends on X86_32 > diff --git a/arch/x86/include/asm/imr.h b/arch/x86/include/asm/imr.h > new file mode 100644 > index 0000000..fe8f3b8 > --- /dev/null > +++ b/arch/x86/include/asm/imr.h > @@ -0,0 +1,79 @@ > +/* > + * imr.h: Isolated Memory Region API > + * > + * Copyright(c) 2013 Intel Corporation. > + * Copyright(c) 2014 Bryan O'Donoghue <pure.logic@nexus-software.ie> 2015 everywhere I guess. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; version 2 > + * of the License. > + */ > +#ifndef _IMR_H > +#define _IMR_H > + > +#include <asm/intel-quark.h> > +#include <linux/types.h> > + > +/* > + * Right now IMRs are not reported via CPUID though it'd be really great if > + * future silicon did report via CPUID for this feature bringing it in-line with > + * other similar features - like MTRRs, MSRs etc. > + * > + * A macro is defined here which is an analog to the other cpu_has_x type > + * features > + */ > +#define cpu_has_imr cpu_is_quark > + > +/* IMR agent access mask bits */ > +#define IMR_ESRAM_FLUSH 0x80000000 > +#define IMR_CPU_SNOOP 0x40000000 > +#define IMR_HB_ACCESS 0x20000000 > +#define IMR_VC1_ID3 0x00008000 > +#define IMR_VC1_ID2 0x00004000 > +#define IMR_VC1_ID1 0x00002000 > +#define IMR_VC1_ID0 0x00001000 > +#define IMR_VC0_ID3 0x00000800 > +#define IMR_VC0_ID2 0x00000400 > +#define IMR_VC0_ID1 0x00000200 > +#define IMR_VC0_ID0 0x00000100 > +#define IMR_SMM 0x00000002 > +#define IMR_NON_SMM 0x00000001 > +#define IMR_ACCESS_NONE 0x00000000 Can it be defined via BIT(x)? > + > +/* Definition of read/write masks from published BSP code */ > +#define IMR_READ_ACCESS_ALL 0xBFFFFFFF > +#define IMR_WRITE_ACCESS_ALL 0xFFFFFFFF > + > +/* Number of IMRs provided by Quark X1000 SoC */ > +#define QUARK_X1000_IMR_NUM 0x07 > +#define QUARK_X1000_IMR_REGBASE 0x40 > + > +/* IMR alignment bits - only bits 31:10 are checked for IMR validity */ > +#define IMR_ALIGN 0x400 > +#define IMR_MASK (IMR_ALIGN - 1) > + > +/** > + * imr_add_range - Add an Isolated Memory Region > + * @base: Physical base address of region aligned to 4k > + * @size: Physical size of region in bytes > + * @read_mask: Read access mask > + * @write_mask: Write access mask > + * @lock: Indicates whether or not to permenantly lock this region It would be nice to indent descriptions after colon and use small letter at the beginning. (Check entire patchset) > + * @return: Index of the IMR allocated or negative value indicating error Usually it goes as a separate section called Return like: * Return: * foo if A, otherwise bar. > + */ Entire comment block should be in *.c file only. > +int imr_add(unsigned long base, unsigned long size, Leave _range suffix here and in the other one. It would be better I think. > + unsigned int rmask, unsigned int wmask, bool lock); > + > +/** > + * imr_del_range - Delete an Isolated Memory Region > + * @reg: IMR index to remove > + * @base: Physical base address of region aligned to 4k > + * @size: Physical size of region in bytes > + * @return: -EINVAL on invalid range or out or range id > + * -ENODEV if reg is valid but no IMR exists or is locked > + * 0 on success > + */ > +int imr_del(int reg, unsigned long base, unsigned long size); Same comments as for add_range. Could it be imr_remove_range() ? > + > +#endif /* _IMR_H */ > diff --git a/arch/x86/include/asm/intel-quark.h b/arch/x86/include/asm/intel-quark.h > new file mode 100644 > index 0000000..f51ac8c > --- /dev/null > +++ b/arch/x86/include/asm/intel-quark.h > @@ -0,0 +1,31 @@ > +/* > + * intel-quark.h: Quark shared defines and helper functions > + * > + * Copyright 2014 Bryan O'Donoghue <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 _ASM_X86_INTEL_QUARK_H > +#define _ASM_X86_INTEL_QUARK_H > + > +#include <asm/processor.h> > + > +#ifdef CONFIG_X86_32 > +static inline int cpu_is_quark(const struct cpuinfo_x86 *c) > +{ > + return (c->x86_vendor == X86_VENDOR_INTEL && > + c->x86 == 5 && c->x86_model == 9); > +} > +#else > +static inline int cpu_is_quark(const struct cpuinfo_x86 *c) > +{ > + return 0; > +} > +#endif > + > +#endif /* _ASM_X86_INTEL_QUARK_H */ Could we use x86_match_cpu() instead? > + > diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile > index 5d4502c..0252de5 100644 > --- a/arch/x86/kernel/Makefile > +++ b/arch/x86/kernel/Makefile > @@ -104,6 +104,7 @@ obj-$(CONFIG_EFI) += sysfb_efi.o > obj-$(CONFIG_PERF_EVENTS) += perf_regs.o > obj-$(CONFIG_TRACING) += tracepoint.o > obj-$(CONFIG_IOSF_MBI) += iosf_mbi.o > +obj-$(CONFIG_IMR) += imr.o > obj-$(CONFIG_PMC_ATOM) += pmc_atom.o > > ### > diff --git a/arch/x86/kernel/imr.c b/arch/x86/kernel/imr.c > new file mode 100644 > index 0000000..4115138 > --- /dev/null > +++ b/arch/x86/kernel/imr.c > @@ -0,0 +1,529 @@ > +/** > + * intel_imr.c > + * > + * Copyright(c) 2013 Intel Corporation. > + * Copyright(c) 2014 Bryan O'Donoghue <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 refernece code from Intel's > + * Quark BSP EFI, Linux and grub code. > + */ > +#include <asm/intel-quark.h> > +#include <asm/imr.h> > +#include <asm/iosf_mbi.h> > +#include <linux/debugfs.h> > +#include <linux/init.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/types.h> > + > +struct imr_device { > + struct dentry *debugfs_dir; > + struct mutex lock; > + int num; > + int used; > + int reg_base; > +}; > + > +static struct imr_device imr_dev; > + > +/** Just /*. > + * Values derived from published code in Quark BSP > + * > + * addr_lo > + * 31 Lock bit > + * 30 Enable bit - not implemented on Quark X1000 > + * 29:25 Reserved > + * 24:2 1 kilobyte aligned lo address > + * 1:0 Reserved > + * > + * addr_hi > + * 31:25 Reserved > + * 24:2 1 kilobyte aligned hi address > + * 1:0 Reserved > + */ > +#define IMR_LOCK 0x80000000 > +#define IMR_ENABLE 0x04000000 Use BIT(x). > + > +struct imr { > + u32 addr_lo; > + u32 addr_hi; > + u32 rmask; > + u32 wmask; > +}; > + > +#define IMR_NUM_REGS (sizeof(struct imr)/sizeof(u32)) > +#define IMR_SHIFT 8 > +#define imr_to_phys(x) (x << IMR_SHIFT) > +#define phys_to_imr(x) (x >> IMR_SHIFT) x -> (x) > + > +/** > + * imr_enabled > + * Determines if an IMR is enabled based on address range > + * > + * @imr: Pointer to IMR descriptor > + * @return true if IMR enabled false if disabled > + */ > +static int imr_enabled(struct imr *imr) > +{ > + return (imr_to_phys(imr->addr_lo) && imr_to_phys(imr->addr_hi)); No need to surround the return value by parentheses. > +} > + > +/** > + * imr_read > + * Read an IMR at a given imr index. Requires caller to hold imr mutex Summary. * imr_read - read an IMR at a given index This one goes to the Description I guess. * Description: * Requires caller to hold IMR device mutex. Same for imr_write(). > + * > + * @imr_id: IMR entry to read > + * @imr: IMR structure representing address and access masks > + * @return 0 on success or error code passed from mbi_iosf on failure > + */ > +static int imr_read(u32 imr_id, struct imr *imr) > +{ > + u32 reg = imr_dev.reg_base + (imr_id * IMR_NUM_REGS); No need to have parentheses. Same for _write(). > + int ret; > + > + ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ, > + reg, &imr->addr_lo); > + if (ret) > + return ret; > + > + ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ, > + ++reg, &imr->addr_hi); Could you use reg++ in previous line and so on? One more idea, if you make a union inside the structure you may do this in a loop. struct imr { union { struct imr { ... }; u32 regs[IMR_NUM_REGS]; }; } Mostly same for _write(). > + if (ret) > + return ret; > + > + ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ, > + ++reg, &imr->rmask); > + if (ret) > + return ret; > + > + return iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ, > + ++reg, &imr->wmask); > +} > + > +/** > + * imr_write > + * Write an IMR at a given imr index. Requires caller to hold imr mutex > + * Note lock bits need to be written independently of address bits > + * > + * @imr_id: IMR entry to write > + * @imr: IMR structure representing address and access masks > + * @lock: Indicates if the IMR lock bit should be applied > + * @return 0 on success or error code passed from mbi_iosf on failure > + */ > +static int imr_write(u32 imr_id, struct imr *imr, bool lock) > +{ > + unsigned long flags; > + u32 reg = imr_dev.reg_base + (imr_id * IMR_NUM_REGS); > + u32 reg_lock = reg; Do we need a separate variable? Would (reg - IMR_NUM_REGS) work for you? > + int ret; > + > + local_irq_save(flags); > + > + ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE, reg, > + imr->addr_lo); > + if (ret) > + goto done; > + > + ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE, > + ++reg, imr->addr_hi); > + if (ret) > + goto done; > + > + ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE, > + ++reg, imr->rmask); > + if (ret) > + goto done; > + > + ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE, > + ++reg, imr->wmask); > + if (ret) > + goto done; > + > + /* Lock bit must be set separately to addr_lo address bits */ > + if (lock) { > + imr->addr_lo |= IMR_LOCK; > + ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE, > + reg_lock, imr->addr_lo); > + } > + > +done: > + local_irq_restore(flags); Could you do like local_irq_restore(flags); return 0; done: local_irq_restore(flags); WARN(...) return ret; ? > + > + /* > + * If writing to the IOSF failed then we're in an unknown state > + * likely a very bad state. An IMR in an invalid state will almost > + * certainly lead to a memory access violation. > + */ > + if (ret) > + WARN(1, "IOSF-MBI write fail range 0x%08x-0x%08x unreliable\n", > + imr_to_phys(imr->addr_lo), > + imr_to_phys(imr->addr_hi) + IMR_MASK); > + > + return ret; > +} > + > +#ifdef CONFIG_DEBUG_FS > + > +/** > + * imr_dbgfs_state_show > + * Print state of IMR registers > + * > + * @s: pointer to seq_file for output > + * @unused: unused parameter > + * @return 0 on success or error code passed from mbi_iosf on failure > + */ > +static int imr_dbgfs_state_show(struct seq_file *s, void *unused) > +{ > + int i, ret = -ENODEV; > + struct imr imr; > + u32 size; > + > + mutex_lock(&imr_dev.lock); It seems you may get the imr_dev via parameters. I suggest to avoid using global variables as much as possible. > + > + for (i = 0; i <= imr_dev.num; i++) { num is not num, but last one? Sounds confusing for me. > + > + ret = imr_read(i, &imr); > + if (ret) > + break; > + > + /* > + * Remember to add IMR_ALIGN bytes to size to indicate the > + * inherent IMR_ALIGN size bytes contained in the masked away > + * lower ten bits > + */ > + size = 0; It might be an else branch. > + if (imr_enabled(&imr)) { > + size = imr_to_phys(imr.addr_hi) - > + imr_to_phys(imr.addr_lo); Could it be one line? > + size += IMR_ALIGN; Could it fit this one too? > + } > + seq_printf(s, "imr%02i: base=0x%08x, end=0x%08x, size=0x%08x " > + "rmask=0x%08x, wmask=0x%08x, %s, %s\n", i, > + imr_to_phys(imr.addr_lo), > + imr_to_phys(imr.addr_hi) + IMR_MASK, size, > + imr.rmask, imr.wmask, > + imr_enabled(&imr) ? "enabled " : "disabled", > + imr.addr_lo & IMR_LOCK ? "locked" : "unlocked"); > + } > + > + mutex_unlock(&imr_dev.lock); > + > + return ret; > +} > + > +/** > + * imr_state_open > + * Debugfs open callback > + * > + * @inode: pointer to struct inode > + * @file: pointer to struct file > + * @return result of single open > + */ > +static int imr_state_open(struct inode *inode, struct file *file) > +{ > + return single_open(file, imr_dbgfs_state_show, inode->i_private); > +} > + > +static const struct file_operations imr_state_ops = { > + .open = imr_state_open, > + .read = seq_read, > + .llseek = seq_lseek, > + .release = single_release, > +}; > + > +/** > + * imr_debugfs_register > + * Register debugfs hooks > + * > + * @imr: IMR structure representing address and access masks > + * @return 0 on success - errno on failure > + */ > +static int imr_debugfs_register(struct imr_device *imr_dev) > +{ > + struct dentry *dir, *f; > + > + dir = debugfs_create_dir("imr", NULL); > + if (!dir) > + return -ENOMEM; if (IS_ERR(dir)) return PTR_ERR(); Though it seems not a case when you have this under ifdef. > + > + f = debugfs_create_file("state", S_IFREG | S_IRUGO, > + dir, imr_dev, &imr_state_ops); > + if (!f) > + goto err; Are you planing to extend the debugfs contents? Would it be okay to use only one file for now? > + > + imr_dev->debugfs_dir = dir; > + > + return 0; > +err: > + return -ENODEV; No need to have separate label for plain return. > +} > + > +/** > + * imr_debugfs_unregister > + * Unregister debugfs hooks > + * > + * @imr: IMR structure representing address and access masks > + * @return none > + */ > +static void imr_debugfs_unregister(struct imr_device *imr_dev) > +{ > + if (!imr_dev->debugfs_dir) > + return; Redundant check. > + > + debugfs_remove_recursive(imr_dev->debugfs_dir); > + imr_dev->debugfs_dir = NULL; No need to assign NULL. > +} No need to put this function under ifdef - debugfs has the stubs. Or add ifdefs around places where they are called and remove those dummy functions below. > + > +#else > + > +/** > + * imr_debugfs_register > + * Register debugfs hooks > + * > + * @imr: IMR structure representing address and access masks > + * @return 0 on success - errno on failure > + */ > +static int imr_debugfs_register(struct imr_device *imr_dev) > +{ > + return 0; > +} > + > +/** > + * imr_debugfs_unregister > + * Dummy hook for !CONFIG_DEBUG_FS > + * > + * @imr: IMR structure representing address and access masks > + * @return none > + */ > +static void imr_debugfs_unregister(struct imr_device *imr_dev) > +{ > +} > + > +#endif /* CONFIG_DEBUG_FS */ > + > +/** > + * imr_check_range > + * Check the passed address range for an IMR is aligned > + * > + * @base: base address of intended IMR > + * @size: size of intended IMR > + * @return zero on valid range -EINVAL on unaligned base/size > + */ > +static int imr_check_range(unsigned long base, unsigned long size) > +{ > + if ((base & (IMR_MASK)) || (size & (IMR_MASK))) { Too many parentheses. Looks like you may do it less. > + pr_warn("imr: base 0x%08lx size 0x%08lx must align to 1k\n", Can you define pr_fmt() ? 1 KiB. > + base, size); > + dump_stack(); dump_stack is really needed here? In that case why not to use WARN()? > + return -EINVAL; > + } > + return 0; > +} > + > +/** > + * imr_add_range > + * Add an Isolated Memory Region > + * > + * @base: Physical base address of region aligned to 4k > + * @size: Physical size of region in bytes > + * @read_mask: Read access mask > + * @write_mask: Write access mask > + * @lock: Indicates whether or not to permenantly lock this region > + * @return: Index of the IMR allocated or negative value indicating error > + */ > +int imr_add(unsigned long base, unsigned long size, > + unsigned int rmask, unsigned int wmask, bool lock) > +{ > + unsigned long end = base + size; > + struct imr imr; > + int reg, i, overlap, ret; > + > + if (imr_check_range(base, size)) > + return -EINVAL; ret = imr_(); if (ret) return ret; > + > + if (!size) { > + pr_warn("imr: invalid size zero!\n"); > + return -EINVAL; > + } > + > + mutex_lock(&imr_dev.lock); > + > + /* Find an free IMR while checking for an existing overlapping range */ > + overlap = 0; > + reg = -1; > + for (i = 0; i <= imr_dev.num; i++) { > + ret = imr_read(i, &imr); > + if (ret) > + goto done; > + > + /* Find overlap */ > + if (imr_enabled(&imr)) { > + if (base >= imr_to_phys(imr.addr_lo) && > + base <= imr_to_phys(imr.addr_hi)) { > + overlap = 1; > + break; Maybe ret = -EINVAL; goto done; > + } > + if (end >= imr_to_phys(imr.addr_lo) && > + end <= imr_to_phys(imr.addr_hi)) { > + overlap = 1; > + break; Ditto. > + } You may use one condition. If you still want to have them split you may create a helper to check for overlap. > + } else { > + reg = i; > + } > + } > + > + /* Error out if we have an overlap or no free IMR entries */ > + if (overlap) { > + ret = -EINVAL; > + goto done; > + } ...and remove overlap variable and this condition. > + if (reg == -1) { > + ret = -ENODEV; > + goto done; > + } > + > + pr_debug("IMR %d phys 0x%08lx-0x%08lx rmask 0x%08x wmask 0x%08x\n", > + reg, base, end, rmask, wmask); > + > + /* Allocate IMR */ > + imr.addr_lo = IMR_ENABLE | phys_to_imr(base); > + imr.addr_hi = phys_to_imr(end); > + imr.rmask = rmask; > + imr.wmask = wmask; > + > + ret = imr_write(reg, &imr, lock); > + > +done: > + mutex_unlock(&imr_dev.lock); > + return ret; > +} > +EXPORT_SYMBOL(imr_add); > + > +/** > + * imr_del_range > + * Delete an Isolated Memory Region > + * > + * @reg: IMR index to remove > + * @base: Physical base address of region aligned to 4k > + * @size: Physical size of region in bytes > + * @return: -EINVAL on invalid range or out or range id > + * -ENODEV if reg is valid but no IMR exists or is locked > + * 0 on success > + */ > +int imr_del(int reg, unsigned long base, unsigned long size) > +{ > + struct imr imr; > + int found = 0, i, ret = 0; > + unsigned long max = base + size; > + > + if (imr_check_range(base, size)) > + return -EINVAL; > + > + if (reg > imr_dev.num) > + return -EINVAL; > + > + mutex_lock(&imr_dev.lock); > + > + /* if a specific IMR is given try to use it */ Use capital letter to start a comment. Check a whole code. > + if (reg >= 0) { > + ret = imr_read(reg, &imr); > + if (ret) > + goto done; > + > + if (!imr_enabled(&imr) || imr.addr_lo & IMR_LOCK) { > + ret = -ENODEV; > + goto done; > + } > + found = 1; You may put a loop to the else branch instead of checking found at each iteration. > + } > + > + /* search for match based on address range */ > + for (i = 0; i <= imr_dev.num && found == 0; i++) { > + ret = imr_read(reg, &imr); > + if (ret) > + goto done; > + > + if (!imr_enabled(&imr) || imr.addr_lo & IMR_LOCK) > + continue; > + > + if ((imr_to_phys(imr.addr_lo) == base) && > + (imr_to_phys(imr.addr_hi) == max)) { > + reg = i; > + found = 1; break; > + } > + } > + > + if (found == 0) { > + ret = -ENODEV; > + goto done; > + } > + > + /* Tear down the IMR */ > + imr.addr_lo = 0; > + imr.addr_hi = 0; > + imr.rmask = IMR_READ_ACCESS_ALL; > + imr.wmask = IMR_WRITE_ACCESS_ALL; > + > + ret = imr_write(reg, &imr, false); > + > +done: > + mutex_unlock(&imr_dev.lock); > + return ret; > +} > +EXPORT_SYMBOL(imr_del); > + > +/** > + * intel_imr_probe > + * entry point for IMR driver > + * > + * return: -ENODEV for no IMR support 0 if good to go > + */ > +static int __init intel_imr_init(void) > +{ > + struct cpuinfo_x86 *c = &cpu_data(cpu); > + > + if (!cpu_has_imr(c)) > + return -ENODEV; > + > + if (iosf_mbi_available() == false) > + return -ENODEV; > + > + if (cpu_is_quark(c)) { > + imr_dev.num = QUARK_X1000_IMR_NUM; > + imr_dev.reg_base = QUARK_X1000_IMR_REGBASE; > + } else { > + pr_err("Unknown IMR implementation\n"); > + return -ENODEV; > + } > + > + mutex_init(&imr_dev.lock); > + > + return imr_debugfs_register(&imr_dev); > +} > + > +/** > + * intel_imr_exit > + * exit point for IMR code. Deregisters debugfs, leave IMR state as-is. > + * > + * return: none > + */ > +static void __exit intel_imr_exit(void) > +{ > + imr_debugfs_unregister(&imr_dev); > +} > + > +module_init(intel_imr_init); > +module_exit(intel_imr_exit); > + > +MODULE_AUTHOR("Bryan O'Donoghue <pure.logic@nexus-software.ie>"); > +MODULE_DESCRIPTION("Intel Isolated Memory Region driver"); > +MODULE_LICENSE("GPL"); > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ Happy New Year! -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2014-12-31 15:05 ` Andy Shevchenko @ 2015-01-01 20:11 ` Bryan O'Donoghue 0 siblings, 0 replies; 47+ messages in thread From: Bryan O'Donoghue @ 2015-01-01 20:11 UTC (permalink / raw) To: Andy Shevchenko Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, dvhart, platform-driver-x86, linux-kernel On 31/12/14 15:05, Andy Shevchenko wrote: <snip> >> +int imr_del(int reg, unsigned long base, unsigned long size); > > Same comments as for add_range. > Could it be imr_remove_range() ? Can be. You've actually caught me out there, function name is "imr_del" function description says imr_del_range(). Amazing to think I proof read this file a number of times for exactly thing type of thing I'm happy with imr_add_range(); imr_remove_range(); >> + >> +#endif /* _IMR_H */ >> diff --git a/arch/x86/include/asm/intel-quark.h b/arch/x86/include/asm/intel-quark.h >> new file mode 100644 >> index 0000000..f51ac8c >> --- /dev/null >> +++ b/arch/x86/include/asm/intel-quark.h <snip> >> +#ifdef CONFIG_X86_32 >> +static inline int cpu_is_quark(const struct cpuinfo_x86 *c) >> +{ >> + return (c->x86_vendor == X86_VENDOR_INTEL && >> + c->x86 == 5 && c->x86_model == 9); >> +} >> +#else >> +static inline int cpu_is_quark(const struct cpuinfo_x86 *c) >> +{ >> + return 0; >> +} >> +#endif >> + >> +#endif /* _ASM_X86_INTEL_QUARK_H */ > > Could we use x86_match_cpu() instead? I don't see why not. I'll kill this header and call out to x86_match_cpu() in the Galileo and IMR code. >> + ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ, >> + reg, &imr->addr_lo); >> + if (ret) >> + return ret; >> + >> + ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ, >> + ++reg, &imr->addr_hi); > > Could you use reg++ in previous line and so on? Yes I could. Originally wrote the code with a pre-increment and then changed the flow later on. Post-increment will work just as good now and is more readable. > One more idea, if you make a union inside the structure you may do > this in a loop. > struct imr { > union { > struct imr { > ... > }; > u32 regs[IMR_NUM_REGS]; > }; > } > Mostly same for _write(). Since reg is incrementing on each iosf_mbi_dothing() that can work. >> + u32 reg = imr_dev.reg_base + (imr_id * IMR_NUM_REGS); >> + u32 reg_lock = reg; > > Do we need a separate variable? Would (reg - IMR_NUM_REGS) work for you? Sure. A separate integer isn't required, I can make that change. >> +done: >> + local_irq_restore(flags); > > Could you do like > > local_irq_restore(flags); > return 0; > done: > local_irq_restore(flags); > WARN(...) > return ret; > ? Doesn't change the logic so yeah, works for me. >> +static int imr_dbgfs_state_show(struct seq_file *s, void *unused) >> +{ >> + int i, ret = -ENODEV; >> + struct imr imr; >> + u32 size; >> + >> + mutex_lock(&imr_dev.lock); > > It seems you may get the imr_dev via parameters. I suggest to avoid > using global variables as much as possible. Appreciated, you're right globals are bad. I think in this case the mutex at that scope is necessary though. Same with the debugfs handle. >> + >> + for (i = 0; i <= imr_dev.num; i++) { > > num is not num, but last one? Sounds confusing for me. Will change. >> + >> + f = debugfs_create_file("state", S_IFREG | S_IRUGO, >> + dir, imr_dev, &imr_state_ops); >> + if (!f) >> + goto err; > > Are you planing to extend the debugfs contents? Would it be okay to > use only one file for now? No not planning to extend. OK with the one file. >> +} > > No need to put this function under ifdef - debugfs has the stubs. Ah - wasn't aware of that. Great stuff - I'll kill the ifdefs - thanks ! > Can you define pr_fmt() ? > > 1 KiB. Yep will do :g/pr_info/s//pr_fmt/g > >> + base, size); >> + dump_stack(); > > dump_stack is really needed here? In that case why not to use WARN()? Hmm - I nope - dump_stack() isn't relevant since it's an in-kernel API. Did an unthinking copy/past from the mtrr code I'll zap that. >> + if (imr_check_range(base, size)) >> + return -EINVAL; > > ret = imr_(); > if (ret) > return ret; Good catch. >> + for (i = 0; i <= imr_dev.num; i++) { >> + ret = imr_read(i, &imr); >> + if (ret) >> + goto done; >> + >> + /* Find overlap */ >> + if (imr_enabled(&imr)) { >> + if (base >= imr_to_phys(imr.addr_lo) && >> + base <= imr_to_phys(imr.addr_hi)) { > >> + overlap = 1; >> + break; > > Maybe > ret = -EINVAL; > goto done; Agree - I like that change. Waste of time to complete the loop if an overlap is detected. >> + if (reg >= 0) { >> + ret = imr_read(reg, &imr); >> + if (ret) >> + goto done; >> + >> + if (!imr_enabled(&imr) || imr.addr_lo & IMR_LOCK) { >> + ret = -ENODEV; >> + goto done; >> + } >> + found = 1; > > You may put a loop to the else branch instead of checking found at > each iteration. Sure. > Happy New Year! Thanks for the feedback Andy ! Best wishes in the New Year. BOD ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2014-12-29 17:23 ` [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 Bryan O'Donoghue 2014-12-31 15:05 ` Andy Shevchenko @ 2015-01-06 7:36 ` Darren Hart 2015-01-06 13:43 ` Bryan O'Donoghue 2015-01-08 0:04 ` Ong, Boon Leong 2 siblings, 1 reply; 47+ messages in thread From: Darren Hart @ 2015-01-06 7:36 UTC (permalink / raw) To: Bryan O'Donoghue Cc: tglx, mingo, hpa, x86, platform-driver-x86, linux-kernel, Boon Leong Ong On Mon, Dec 29, 2014 at 05:23:02PM +0000, Bryan O'Donoghue wrote: > Intel's Quark X1000 SoC contains a set of registers called Isolated Memory > Regions. IMRs are accessed over the IOSF mailbox interface. IMRs are areas > carved out of memory that define read/write access rights to the various > system agents within the Quark system. For a given agent in the system it is > possible to specify if that agent may read or write an area of memory > defined by an IMR with a granularity of 1 kilobyte. > > Quark_SecureBootPRM_330234_001.pdf section 4.5 details the concept of IMRs I won't repeat Andy's review, so please incorporate his suggestions unless I specifically contradict him below... (which I don't intend to do...) > > eSRAM flush, CPU Snoop, CPU SMM Mode, CPU non-SMM mode, PMU and VC0/VC1 can > have individual read/write access masks applied to them for a given memory > region in Quark X1000. Quark supports eightIMR sets. > > Since all of the DMA capable SoC components in the X1000 are mapped to VC0 Please define less common acronymns the first time they are used: Virtual Channel 0 (VC0) > it is possible to define sections of memory as invalid for DMA write > operations originating from Ethernet, USB, SD and any other DMA capable > south-cluster component on VC0. Similarly it is possible to mark kernel > memory as non-SMM mode read/write only or to mark BIOS runtime memory as SMM > mode accessible only depending on the particular memory footprint on a given > system. > > On an IMR violation Quark SoC X1000 systems are configured to reset the > system, so ensuring that the IMR memory map agrees with the EFI provided > memory map is critical to ensure no IMR violations reset the system. > > The API for accessing IMRs is based on MTRR code but doesn't provide a /proc > or /sys interface to manipulate IMRs. Defining the size and extent of IMRs > is exclusively the domain of in-kernel code. > This provides a good description of what IMRs are and how they are used on X1000 SoCs, but it doesn't tell me what to expect in the following patch. > Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie> Since you have Intel (C) below and then your own, are you the sole author? > --- > arch/x86/Kconfig | 23 ++ > arch/x86/include/asm/imr.h | 79 ++++++ > arch/x86/include/asm/intel-quark.h | 31 +++ > arch/x86/kernel/Makefile | 1 + > arch/x86/kernel/imr.c | 529 +++++++++++++++++++++++++++++++++++++ > 5 files changed, 663 insertions(+) > create mode 100644 arch/x86/include/asm/imr.h > create mode 100644 arch/x86/include/asm/intel-quark.h > create mode 100644 arch/x86/kernel/imr.c > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index ba397bd..8303ca2 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -526,6 +526,29 @@ config IOSF_MBI_DEBUG > > If you don't require the option or are in doubt, say N. > > +config IMR > + tristate "Isolated Memory Region support" > + default m > + depends on IOSF_MBI > + ---help--- > + This option enables support for Isolated Memory Regions. It supports manipulating them specifically, correct? No support is needed to trigger an IMR violation and reboot the system, that happens in hardware/firmware without any OS involvement. So we're specifically providing the means to manipulate the IMRs. > + IMRs are a set of registers that define read and write access masks > + to prohibit certain system agents from accessing memory with 1k > + granularity. New line > + 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 colon > + - SMM mode > + - Non SMM mode > + - PCI VC0/VC1 > + - CPU snoop > + - eSRAM flush > + - PMU access New line > + Quark contains a set of IMR registers and makes use of those set of eight? > + registers during it's bootup process. its > + > + If you are running on a Galileo/Quark say Y here period > + > config X86_RDC321X > bool "RDC R-321x SoC" > depends on X86_32 > diff --git a/arch/x86/include/asm/imr.h b/arch/x86/include/asm/imr.h > new file mode 100644 > index 0000000..fe8f3b8 > --- /dev/null > +++ b/arch/x86/include/asm/imr.h > @@ -0,0 +1,79 @@ > +/* > + * imr.h: Isolated Memory Region API > + * > + * Copyright(c) 2013 Intel Corporation. > + * Copyright(c) 2014 Bryan O'Donoghue <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 <asm/intel-quark.h> > +#include <linux/types.h> > + > +/* > + * Right now IMRs are not reported via CPUID though it'd be really great if > + * future silicon did report via CPUID for this feature bringing it in-line with > + * other similar features - like MTRRs, MSRs etc. I think we can drop the editorializing :-) /* * Unfortunately, IMRs are not reported via CPUID, unlike MTRRs, MSRs, etc. * Define a macro analogous to cpu_has_x type features. */ > + * > + * A macro is defined here which is an analog to the other cpu_has_x type > + * features > + */ > +#define cpu_has_imr cpu_is_quark > + > +/* IMR agent access mask bits */ > +#define IMR_ESRAM_FLUSH 0x80000000 > +#define IMR_CPU_SNOOP 0x40000000 > +#define IMR_HB_ACCESS 0x20000000 > +#define IMR_VC1_ID3 0x00008000 > +#define IMR_VC1_ID2 0x00004000 > +#define IMR_VC1_ID1 0x00002000 > +#define IMR_VC1_ID0 0x00001000 > +#define IMR_VC0_ID3 0x00000800 > +#define IMR_VC0_ID2 0x00000400 > +#define IMR_VC0_ID1 0x00000200 > +#define IMR_VC0_ID0 0x00000100 > +#define IMR_SMM 0x00000002 > +#define IMR_NON_SMM 0x00000001 > +#define IMR_ACCESS_NONE 0x00000000 > + > +/* Definition of read/write masks from published BSP code */ > +#define IMR_READ_ACCESS_ALL 0xBFFFFFFF > +#define IMR_WRITE_ACCESS_ALL 0xFFFFFFFF > + > +/* Number of IMRs provided by Quark X1000 SoC */ > +#define QUARK_X1000_IMR_NUM 0x07 Hrm, I thought there were 8? ... > diff --git a/arch/x86/kernel/imr.c b/arch/x86/kernel/imr.c > new file mode 100644 > index 0000000..4115138 > --- /dev/null > +++ b/arch/x86/kernel/imr.c > @@ -0,0 +1,529 @@ > +/** > + * intel_imr.c > + * > + * Copyright(c) 2013 Intel Corporation. > + * Copyright(c) 2014 Bryan O'Donoghue <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 refernece code from Intel's > + * Quark BSP EFI, Linux and grub code. The above text block is oddly formatted. Please wrap to a uniform width (72 or so) and use a blank line between paragraphs. > + */ > +#include <asm/intel-quark.h> > +#include <asm/imr.h> > +#include <asm/iosf_mbi.h> > +#include <linux/debugfs.h> > +#include <linux/init.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/types.h> > + > +struct imr_device { > + struct dentry *debugfs_dir; > + struct mutex lock; > + int num; > + int used; > + int reg_base; > +}; > + > +static struct imr_device imr_dev; > + > +/** > + * Values derived from published code in Quark BSP Also in the datasheet here: https://communities.intel.com/servlet/JiveServlet/previewBody/21828-102-2-25120/329676_QuarkDatasheet.pdf > + * > + * addr_lo > + * 31 Lock bit > + * 30 Enable bit - not implemented on Quark X1000 With the exception of bit 30, also marked as reserved per the above datasheet. I'd prefer to reference the datasheet where possible rather than soon to be obsolete code (assuming this is meant to replace it?) > + * 29:25 Reserved > + * 24:2 1 kilobyte aligned lo address The above spec uses 23:2. Is it incorrect? Section 12.7.4.5 > + * 1:0 Reserved > + * > + * addr_hi > + * 31:25 Reserved > + * 24:2 1 kilobyte aligned hi address > + * 1:0 Reserved > + */ > +#define IMR_LOCK 0x80000000 > +#define IMR_ENABLE 0x04000000 > + > +struct imr { > + u32 addr_lo; > + u32 addr_hi; > + u32 rmask; > + u32 wmask; > +}; > + > +#define IMR_NUM_REGS (sizeof(struct imr)/sizeof(u32)) Perhaps call the imr imr_regs so nobody breaks this in the future by adding something other than a u32 to it? Alternatively, use a datatype that enforces this... like the union Andy suggested... > +#define IMR_SHIFT 8 > +#define imr_to_phys(x) (x << IMR_SHIFT) > +#define phys_to_imr(x) (x >> IMR_SHIFT) > + > +/** > + * imr_enabled > + * Determines if an IMR is enabled based on address range > + * > + * @imr: Pointer to IMR descriptor > + * @return true if IMR enabled false if disabled > + */ > +static int imr_enabled(struct imr *imr) > +{ > + return (imr_to_phys(imr->addr_lo) && imr_to_phys(imr->addr_hi)); What are testing here? You have bit 30 marked as "Enabled" above (but it isn't in the datasheet). With Reserved bits in each register, this test for non-zero doesn't seem robust. > +} ... > +/** > + * imr_write > + * Write an IMR at a given imr index. Requires caller to hold imr mutex > + * Note lock bits need to be written independently of address bits > + * > + * @imr_id: IMR entry to write > + * @imr: IMR structure representing address and access masks > + * @lock: Indicates if the IMR lock bit should be applied > + * @return 0 on success or error code passed from mbi_iosf on failure > + */ > +static int imr_write(u32 imr_id, struct imr *imr, bool lock) > +{ > + unsigned long flags; > + u32 reg = imr_dev.reg_base + (imr_id * IMR_NUM_REGS); > + u32 reg_lock = reg; > + int ret; > + > + local_irq_save(flags); > + > + ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE, reg, > + imr->addr_lo); > + if (ret) > + goto done; > + > + ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE, > + ++reg, imr->addr_hi); > + if (ret) > + goto done; > + > + ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE, > + ++reg, imr->rmask); > + if (ret) > + goto done; > + > + ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE, > + ++reg, imr->wmask); > + if (ret) > + goto done; > + > + /* Lock bit must be set separately to addr_lo address bits */ > + if (lock) { > + imr->addr_lo |= IMR_LOCK; > + ret = iosf_mbi_write(QRK_MBI_UNIT_MM, QRK_MBI_MM_WRITE, > + reg_lock, imr->addr_lo); > + } > + > +done: > + local_irq_restore(flags); > + > + /* > + * If writing to the IOSF failed then we're in an unknown state comma at end > + * likely a very bad state. An IMR in an invalid state will almost > + * certainly lead to a memory access violation. > + */ > + if (ret) > + WARN(1, "IOSF-MBI write fail range 0x%08x-0x%08x unreliable\n", No need for the redundant if test. WARN(ret, "IOSF... > + imr_to_phys(imr->addr_lo), > + imr_to_phys(imr->addr_hi) + IMR_MASK); > + > + return ret; > +} > + > +#ifdef CONFIG_DEBUG_FS > + > +/** > + * imr_dbgfs_state_show > + * Print state of IMR registers > + * > + * @s: pointer to seq_file for output > + * @unused: unused parameter > + * @return 0 on success or error code passed from mbi_iosf on failure > + */ > +static int imr_dbgfs_state_show(struct seq_file *s, void *unused) > +{ > + int i, ret = -ENODEV; One line per variable please (per CodingStyle). ... > +/** > + * imr_debugfs_register > + * Register debugfs hooks > + * > + * @imr: IMR structure representing address and access masks > + * @return 0 on success - errno on failure > + */ > +static int imr_debugfs_register(struct imr_device *imr_dev) > +{ > + struct dentry *dir, *f; > + > + dir = debugfs_create_dir("imr", NULL); > + if (!dir) > + return -ENOMEM; > + > + f = debugfs_create_file("state", S_IFREG | S_IRUGO, > + dir, imr_dev, &imr_state_ops); > + if (!f) > + goto err; return -ENODEV; And drop the err: label below. > + > + imr_dev->debugfs_dir = dir; > + > + return 0; > +err: > + return -ENODEV; > +} ... > +/** > + * imr_debugfs_unregister > + * Dummy hook for !CONFIG_DEBUG_FS > + * > + * @imr: IMR structure representing address and access masks > + * @return none > + */ > +static void imr_debugfs_unregister(struct imr_device *imr_dev) > +{ > +} > + > +#endif /* CONFIG_DEBUG_FS */ > + > +/** > + * imr_check_range > + * Check the passed address range for an IMR is aligned > + * > + * @base: base address of intended IMR > + * @size: size of intended IMR > + * @return zero on valid range -EINVAL on unaligned base/size > + */ > +static int imr_check_range(unsigned long base, unsigned long size) > +{ > + if ((base & (IMR_MASK)) || (size & (IMR_MASK))) { > + pr_warn("imr: base 0x%08lx size 0x%08lx must align to 1k\n", > + base, size); > + dump_stack(); > + return -EINVAL; > + } > + return 0; > +} > + > +/** > + * imr_add_range > + * Add an Isolated Memory Region > + * > + * @base: Physical base address of region aligned to 4k > + * @size: Physical size of region in bytes > + * @read_mask: Read access mask > + * @write_mask: Write access mask > + * @lock: Indicates whether or not to permenantly lock this region > + * @return: Index of the IMR allocated or negative value indicating error > + */ > +int imr_add(unsigned long base, unsigned long size, > + unsigned int rmask, unsigned int wmask, bool lock) > +{ > + unsigned long end = base + size; > + struct imr imr; > + int reg, i, overlap, ret; > + > + if (imr_check_range(base, size)) > + return -EINVAL; > + > + if (!size) { > + pr_warn("imr: invalid size zero!\n"); > + return -EINVAL; > + } > + > + mutex_lock(&imr_dev.lock); > + > + /* Find an free IMR while checking for an existing overlapping range */ > + overlap = 0; > + reg = -1; > + for (i = 0; i <= imr_dev.num; i++) { > + ret = imr_read(i, &imr); > + if (ret) > + goto done; > + > + /* Find overlap */ > + if (imr_enabled(&imr)) { > + if (base >= imr_to_phys(imr.addr_lo) && > + base <= imr_to_phys(imr.addr_hi)) { > + overlap = 1; > + break; > + } > + if (end >= imr_to_phys(imr.addr_lo) && > + end <= imr_to_phys(imr.addr_hi)) { > + overlap = 1; > + break; > + } > + } else { > + reg = i; > + } > + } > + > + /* Error out if we have an overlap or no free IMR entries */ According to the datasheet, overlapping ranges are valid, and access must be granted by all IMRs associated with a given address. Why declare an overlap as invalid here? > + if (overlap) { > + ret = -EINVAL; > + goto done; > + } > + if (reg == -1) { > + ret = -ENODEV; > + goto done; > + } ... Thanks, -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2015-01-06 7:36 ` Darren Hart @ 2015-01-06 13:43 ` Bryan O'Donoghue 2015-01-06 16:54 ` Darren Hart 2015-01-07 23:45 ` Ong, Boon Leong 0 siblings, 2 replies; 47+ messages in thread From: Bryan O'Donoghue @ 2015-01-06 13:43 UTC (permalink / raw) To: Darren Hart Cc: tglx, mingo, hpa, x86, platform-driver-x86, linux-kernel, Boon Leong Ong On 06/01/15 07:36, Darren Hart wrote: >> Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie> > > Since you have Intel (C) below and then your own, are you the sole author? Yes, for the platform code. Platform code tears-down IMRs and sets-up up a new one around the kernel. Quark BSP does a similar thing in a different place. To be safe I'm happy to add a Intel (C) to this file anyway. >> +config IMR >> + tristate "Isolated Memory Region support" >> + default m >> + depends on IOSF_MBI >> + ---help--- >> + This option enables support for Isolated Memory Regions. > > It supports manipulating them specifically, correct? No support is needed to > trigger an IMR violation and reboot the system, that happens in > hardware/firmware without any OS involvement. Exactly. > So we're specifically providing the means to manipulate the IMRs. True - I'll add that statement to the description. >> +/* >> + * Right now IMRs are not reported via CPUID though it'd be really great if >> + * future silicon did report via CPUID for this feature bringing it in-line with >> + * other similar features - like MTRRs, MSRs etc. > > I think we can drop the editorializing :-) :) > > /* > * Unfortunately, IMRs are not reported via CPUID, unlike MTRRs, MSRs, etc. > * Define a macro analogous to cpu_has_x type features. > */ Done. >> +/* Definition of read/write masks from published BSP code */ >> +#define IMR_READ_ACCESS_ALL 0xBFFFFFFF >> +#define IMR_WRITE_ACCESS_ALL 0xFFFFFFFF >> + >> +/* Number of IMRs provided by Quark X1000 SoC */ >> +#define QUARK_X1000_IMR_NUM 0x07 > > Hrm, I thought there were 8? There are. All of the loops look like this for (i = 0; i <= imr_dev.num; i++) aka for (i = 0; i <= QUARK_X1000_IMR_NUM; i++) Worth changing IMR_NUM to 8 and changing the loops to < IMR_NUM to remove confusion. > Also in the datasheet here: > > https://communities.intel.com/servlet/JiveServlet/previewBody/21828-102-2-25120/329676_QuarkDatasheet.pdf > >> + * >> + * addr_lo >> + * 31 Lock bit >> + * 30 Enable bit - not implemented on Quark X1000 > > With the exception of bit 30, also marked as reserved per the above datasheet. Fair point. I'll refer to the spec directly for all of the bits. >> +struct imr { >> + u32 addr_lo; >> + u32 addr_hi; >> + u32 rmask; >> + u32 wmask; >> +}; >> + >> +#define IMR_NUM_REGS (sizeof(struct imr)/sizeof(u32)) > > Perhaps call the imr imr_regs so nobody breaks this in the future by adding > something other than a u32 to it? Alternatively, use a datatype that enforces > this... like the union Andy suggested... I'll take a look and see which suggestion fits better >> +#define IMR_SHIFT 8 >> +#define imr_to_phys(x) (x << IMR_SHIFT) >> +#define phys_to_imr(x) (x >> IMR_SHIFT) >> + >> +/** >> + * imr_enabled >> + * Determines if an IMR is enabled based on address range >> + * >> + * @imr: Pointer to IMR descriptor >> + * @return true if IMR enabled false if disabled >> + */ >> +static int imr_enabled(struct imr *imr) >> +{ >> + return (imr_to_phys(imr->addr_lo) && imr_to_phys(imr->addr_hi)); > > What are testing here? You have bit 30 marked as "Enabled" above (but it isn't > in the datasheet). With Reserved bits in each register, this test for non-zero > doesn't seem robust. Good question. Bit 30 is not present in X1000 silicon, though is present in the BSP code. Lets forget about all non-X1000 cases for now since X1000 is the only processor currently available. On X1000 the only means of determining if an IMR is enabled is if it's address bits are set to some address everybody agrees means 'off', since the enable bit doesn't exist. Everybody in this case is ROM, BIOS, 2nd stage bootloader and kernel. In the BSP code, that address is zero. So in lieu of bit 30 'enable' we have address 0x00000000. The kernel could define an alternative address to 0x00000000 but, then this would break with the convention in BIOS etc. Since BIOS and grub code both use 0x00000000 as the 'off' address I think it makes sense for the kernel to continue to use that address. >> + /* Error out if we have an overlap or no free IMR entries */ > > According to the datasheet, overlapping ranges are valid, and access must be > granted by all IMRs associated with a given address. Why declare an overlap as > invalid here? Simplicity. It's more straight forward to define your IMR memory map if you don't allow the address ranges to stomp all over each other. None of the BSP reference code does overlap. If there's an argument for an overlap use-case it can be supported pretty simply by simply removing the overlap checks. My own view is that it's not really desirable and easier to debug IMRs generally on a platform if overlaps aren't allowed. Best, BOD ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2015-01-06 13:43 ` Bryan O'Donoghue @ 2015-01-06 16:54 ` Darren Hart 2015-01-07 23:45 ` Ong, Boon Leong 1 sibling, 0 replies; 47+ messages in thread From: Darren Hart @ 2015-01-06 16:54 UTC (permalink / raw) To: Bryan O'Donoghue Cc: tglx, mingo, hpa, x86, platform-driver-x86, linux-kernel, Boon Leong Ong On Tue, Jan 06, 2015 at 01:43:23PM +0000, Bryan O'Donoghue wrote: > On 06/01/15 07:36, Darren Hart wrote: > > >>Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie> > > > >Since you have Intel (C) below and then your own, are you the sole author? > > Yes, for the platform code. > > Platform code tears-down IMRs and sets-up up a new one around the kernel. > Quark BSP does a similar thing in a different place. > > To be safe I'm happy to add a Intel (C) to this file anyway. I was just checking if we needed to credit another individual with code authorship. ... > >>+#define IMR_SHIFT 8 > >>+#define imr_to_phys(x) (x << IMR_SHIFT) > >>+#define phys_to_imr(x) (x >> IMR_SHIFT) > >>+ > >>+/** > >>+ * imr_enabled > >>+ * Determines if an IMR is enabled based on address range > >>+ * > >>+ * @imr: Pointer to IMR descriptor > >>+ * @return true if IMR enabled false if disabled > >>+ */ > >>+static int imr_enabled(struct imr *imr) > >>+{ > >>+ return (imr_to_phys(imr->addr_lo) && imr_to_phys(imr->addr_hi)); > > > >What are testing here? You have bit 30 marked as "Enabled" above (but it isn't > >in the datasheet). With Reserved bits in each register, this test for non-zero > >doesn't seem robust. > > Good question. > > Bit 30 is not present in X1000 silicon, though is present in the BSP code. > Lets forget about all non-X1000 cases for now since X1000 is the only > processor currently available. > > On X1000 the only means of determining if an IMR is enabled is if it's > address bits are set to some address everybody agrees means 'off', since the > enable bit doesn't exist. Everybody in this case is ROM, BIOS, 2nd stage > bootloader and kernel. > OK, that's non-obvious, so let's add a comment. > In the BSP code, that address is zero. So in lieu of bit 30 'enable' we have > address 0x00000000. > > The kernel could define an alternative address to 0x00000000 but, then this > would break with the convention in BIOS etc. > > Since BIOS and grub code both use 0x00000000 as the 'off' address I think it > makes sense for the kernel to continue to use that address. So, both lo and hi don't need to be non-zero then, either one being non-zero would constitute "enabled"? Should the above test be an || then? > > >>+ /* Error out if we have an overlap or no free IMR entries */ > > > >According to the datasheet, overlapping ranges are valid, and access must be > >granted by all IMRs associated with a given address. Why declare an overlap as > >invalid here? > > Simplicity. > > It's more straight forward to define your IMR memory map if you don't allow > the address ranges to stomp all over each other. > > None of the BSP reference code does overlap. > > If there's an argument for an overlap use-case it can be supported pretty > simply by simply removing the overlap checks. > > My own view is that it's not really desirable and easier to debug IMRs > generally on a platform if overlaps aren't allowed. > OK, let's add a comment to that effect. -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 47+ messages in thread
* RE: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2015-01-06 13:43 ` Bryan O'Donoghue 2015-01-06 16:54 ` Darren Hart @ 2015-01-07 23:45 ` Ong, Boon Leong 2015-01-08 12:10 ` Bryan O'Donoghue 1 sibling, 1 reply; 47+ messages in thread From: Ong, Boon Leong @ 2015-01-07 23:45 UTC (permalink / raw) To: Bryan O'Donoghue, Darren Hart Cc: tglx, mingo, hpa, x86, platform-driver-x86, linux-kernel >>> +/** >>> + * imr_enabled >>> + * Determines if an IMR is enabled based on address range >>> + * >>> + * @imr: Pointer to IMR descriptor >>> + * @return true if IMR enabled false if disabled >>> + */ >>> +static int imr_enabled(struct imr *imr) { >>> + return (imr_to_phys(imr->addr_lo) && imr_to_phys(imr->addr_hi)); >> >> What are testing here? You have bit 30 marked as "Enabled" above (but >> it isn't in the datasheet). With Reserved bits in each register, this >> test for non-zero doesn't seem robust. > >Good question. > >Bit 30 is not present in X1000 silicon, though is present in the BSP code. Lets >forget about all non-X1000 cases for now since X1000 is the only processor >currently available. > >On X1000 the only means of determining if an IMR is enabled is if it's address >bits are set to some address everybody agrees means 'off', since the enable >bit doesn't exist. Everybody in this case is ROM, BIOS, 2nd stage bootloader >and kernel. > >In the BSP code, that address is zero. So in lieu of bit 30 'enable' we have >address 0x00000000. > >The kernel could define an alternative address to 0x00000000 but, then this >would break with the convention in BIOS etc. > >Since BIOS and grub code both use 0x00000000 as the 'off' address I think it >makes sense for the kernel to continue to use that address. Just add on top of what Daren mentioned in another mail, based on the Quark document, the base address can start from zero. Say lo=0, hi=0, and WM & RM may be changed from default value, 1st 1KiB will be marked as IMR. It seems to me that there is no good way to test if an IMR is 'occupied' and/or 'enabled' since enable-bit is not available. But, what is benefit of testing against lo=0 & hi=0? The logic to calculate size will work under lo=0 & hi=0 anway. > >>> + /* Error out if we have an overlap or no free IMR entries */ >> >> According to the datasheet, overlapping ranges are valid, and access >> must be granted by all IMRs associated with a given address. Why >> declare an overlap as invalid here? > >Simplicity. > >It's more straight forward to define your IMR memory map if you don't allow >the address ranges to stomp all over each other. > >None of the BSP reference code does overlap. > >If there's an argument for an overlap use-case it can be supported pretty >simply by simply removing the overlap checks. > >My own view is that it's not really desirable and easier to debug IMRs >generally on a platform if overlaps aren't allowed. I do agree on the benefit listed above. Perhaps, you can add explanation here to mention the design decision. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2015-01-07 23:45 ` Ong, Boon Leong @ 2015-01-08 12:10 ` Bryan O'Donoghue 2015-01-08 14:52 ` Ong, Boon Leong 0 siblings, 1 reply; 47+ messages in thread From: Bryan O'Donoghue @ 2015-01-08 12:10 UTC (permalink / raw) To: Ong, Boon Leong, Darren Hart Cc: tglx, mingo, hpa, x86, platform-driver-x86, linux-kernel On 07/01/15 23:45, Ong, Boon Leong wrote: >> Since BIOS and grub code both use 0x00000000 as the 'off' address I think it >> makes sense for the kernel to continue to use that address. > > Just add on top of what Daren mentioned in another mail, based on the Quark document, > the base address can start from zero. Say lo=0, hi=0, and WM & RM may be changed from default value, > 1st 1KiB will be marked as IMR. It seems to me that there is no good way to test if an IMR is 'occupied' and/or 'enabled' > since enable-bit is not available. But, what is benefit of testing against lo=0 & hi=0? The logic to calculate size will work under > lo=0 & hi=0 anway. Hi Boon Leong. I think it does make sense to add a check for rmask and wmask in the 'access all' state when determining if an IMR is enabled on X1000 or not. >> My own view is that it's not really desirable and easier to debug IMRs >> generally on a platform if overlaps aren't allowed. > I do agree on the benefit listed above. Perhaps, you can add explanation here > to mention the design decision. Will do. ^ permalink raw reply [flat|nested] 47+ messages in thread
* RE: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2015-01-08 12:10 ` Bryan O'Donoghue @ 2015-01-08 14:52 ` Ong, Boon Leong 0 siblings, 0 replies; 47+ messages in thread From: Ong, Boon Leong @ 2015-01-08 14:52 UTC (permalink / raw) To: Bryan O'Donoghue Cc: tglx, mingo, hpa, x86, platform-driver-x86, linux-kernel, Darren Hart, andy.shevchenko >On 07/01/15 23:45, Ong, Boon Leong wrote: >>> Since BIOS and grub code both use 0x00000000 as the 'off' address I >>> think it makes sense for the kernel to continue to use that address. >> >> Just add on top of what Daren mentioned in another mail, based on the >> Quark document, the base address can start from zero. Say lo=0, hi=0, >> and WM & RM may be changed from default value, 1st 1KiB will be marked as >IMR. It seems to me that there is no good way to test if an IMR is 'occupied' >and/or 'enabled' >> since enable-bit is not available. But, what is benefit of testing >> against lo=0 & hi=0? The logic to calculate size will work under >> lo=0 & hi=0 anway. > >Hi Boon Leong. > >I think it does make sense to add a check for rmask and wmask in the 'access all' >state when determining if an IMR is enabled on X1000 or not Ya, checking against rmask & wmask whether they have been changed from default stage would help here. Thanks ^ permalink raw reply [flat|nested] 47+ messages in thread
* RE: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2014-12-29 17:23 ` [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 Bryan O'Donoghue 2014-12-31 15:05 ` Andy Shevchenko 2015-01-06 7:36 ` Darren Hart @ 2015-01-08 0:04 ` Ong, Boon Leong 2015-01-08 13:08 ` Bryan O'Donoghue 2 siblings, 1 reply; 47+ messages in thread From: Ong, Boon Leong @ 2015-01-08 0:04 UTC (permalink / raw) To: Bryan O'Donoghue, tglx, mingo, hpa, x86, dvhart, platform-driver-x86, linux-kernel >-----Original Message----- >From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel- >owner@vger.kernel.org] On Behalf Of Bryan O'Donoghue >Sent: Monday, December 29, 2014 9:23 AM >To: tglx@linutronix.de; mingo@redhat.com; hpa@zytor.com; x86@kernel.org; >dvhart@infradead.org; platform-driver-x86@vger.kernel.org; linux- >kernel@vger.kernel.org >Cc: Bryan O'Donoghue >Subject: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 > 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 kilobyte. > >Quark_SecureBootPRM_330234_001.pdf section 4.5 details the concept of >IMRs Would it be better if we provide a URL to the pdf? > >eSRAM flush, CPU Snoop, CPU SMM Mode, CPU non-SMM mode, PMU and PMU? You meant RMU - Remote Management Unit <snippet removed> > >diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index ba397bd..8303ca2 >100644 >--- a/arch/x86/Kconfig >+++ b/arch/x86/Kconfig >@@ -526,6 +526,29 @@ config IOSF_MBI_DEBUG > > If you don't require the option or are in doubt, say N. > >+config IMR >+ tristate "Isolated Memory Region support" >+ default m >+ depends on IOSF_MBI >+ ---help--- >+ This option enables support for Isolated Memory Regions. >+ IMRs are a set of registers that define read and write access masks >+ to prohibit certain system agents from accessing memory with 1k >+ granularity. >+ IMRs make it possible to control read/write access to an address >+ by hardware agents inside the SoC. Read and write masks can be >+ defined for >+ - SMM mode >+ - Non SMM mode >+ - PCI VC0/VC1 >+ - CPU snoop Add "(write mask only)" at the end. >+ - eSRAM flush >+ - PMU access Do you mean RMU (Remote Management Unit) as documented in data-sheet? >+ Quark contains a set of IMR registers and makes use of those >+ registers during it's bootup process. >+ >+ If you are running on a Galileo/Quark say Y here >+ > config X86_RDC321X > bool "RDC R-321x SoC" > depends on X86_32 >diff --git a/arch/x86/include/asm/imr.h b/arch/x86/include/asm/imr.h new >file mode 100644 index 0000000..fe8f3b8 >--- /dev/null >+++ b/arch/x86/include/asm/imr.h >@@ -0,0 +1,79 @@ >+/* >+ * imr.h: Isolated Memory Region API >+ * >+ * Copyright(c) 2013 Intel Corporation. >+ * Copyright(c) 2014 Bryan O'Donoghue <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 <asm/intel-quark.h> >+#include <linux/types.h> >+ >+/* >+ * Right now IMRs are not reported via CPUID though it'd be really >+great if >+ * future silicon did report via CPUID for this feature bringing it >+in-line with >+ * other similar features - like MTRRs, MSRs etc. >+ * >+ * A macro is defined here which is an analog to the other cpu_has_x >+type >+ * features >+ */ >+#define cpu_has_imr cpu_is_quark We don't really need this #define method since we are using x86_match_cpu(). So, please remove them. >+ >+/* IMR agent access mask bits */ >+#define IMR_ESRAM_FLUSH 0x80000000 >+#define IMR_CPU_SNOOP 0x40000000 Suggest to add a comment to mark CPU_SNOOP as applicable for write-mask only. >+#define IMR_HB_ACCESS 0x20000000 >+#define IMR_VC1_ID3 0x00008000 >+#define IMR_VC1_ID2 0x00004000 >+#define IMR_VC1_ID1 0x00002000 >+#define IMR_VC1_ID0 0x00001000 >+#define IMR_VC0_ID3 0x00000800 >+#define IMR_VC0_ID2 0x00000400 >+#define IMR_VC0_ID1 0x00000200 >+#define IMR_VC0_ID0 0x00000100 >+#define IMR_SMM 0x00000002 >+#define IMR_NON_SMM 0x00000001 Per data-sheet, this is called CPU_0 and CPU0. Suggest to stick with the name used in datasheet... >+#define IMR_ACCESS_NONE 0x00000000 >+ >+/* Definition of read/write masks from published BSP code */ >+#define IMR_READ_ACCESS_ALL 0xBFFFFFFF >+#define IMR_WRITE_ACCESS_ALL 0xFFFFFFFF >+ >+/* Number of IMRs provided by Quark X1000 SoC */ >+#define QUARK_X1000_IMR_NUM 0x07 >+#define QUARK_X1000_IMR_REGBASE 0x40 >+ >+/* IMR alignment bits - only bits 31:10 are checked for IMR validity */ >+#define IMR_ALIGN 0x400 >+#define IMR_MASK (IMR_ALIGN - 1) >+ >+/** >+ * imr_add_range - Add an Isolated Memory Region >+ * @base: Physical base address of region aligned to 4k >+ * @size: Physical size of region in bytes Please add side-note that 'size' should be 1-KiB aligned. Or should we consider auto-alignment feature... >+ * @read_mask: Read access mask >+ * @write_mask: Write access mask >+ * @lock: Indicates whether or not to permenantly lock this region Typo: permanently >+ * @return: Index of the IMR allocated or negative value indicating error >+ */ > + int imr_add(unsigned long base, unsigned long size, >+ unsigned int rmask, unsigned int wmask, bool lock); >+ >+/** >+ * imr_del_range - Delete an Isolated Memory Region >+ * @reg: IMR index to remove >+ * @base: Physical base address of region aligned to 4k >+ * @size: Physical size of region in bytes >+ * @return: -EINVAL on invalid range or out or range id >+ * -ENODEV if reg is valid but no IMR exists or is locked >+ * 0 on success >+ */ >+int imr_del(int reg, unsigned long base, unsigned long size); How about just offer imr delete based index-only as returned by imr_add()? We just need to check if that IMR is locked. If locked, then bail out. Else, we will zero-out IMR register for that index to remove it. >+ >+#endif /* _IMR_H */ <snippet> >diff --git a/arch/x86/kernel/imr.c b/arch/x86/kernel/imr.c new file mode >100644 index 0000000..4115138 >--- /dev/null >+++ b/arch/x86/kernel/imr.c >@@ -0,0 +1,529 @@ >+/** >+ * intel_imr.c >+ * >+ * Copyright(c) 2013 Intel Corporation. >+ * Copyright(c) 2014 Bryan O'Donoghue <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 refernece code from Intel's Typo: reference >+ * Quark BSP EFI, Linux and grub code. >+ */ >+#include <asm/intel-quark.h> >+#include <asm/imr.h> >+#include <asm/iosf_mbi.h> >+#include <linux/debugfs.h> >+#include <linux/init.h> >+#include <linux/module.h> >+#include <linux/platform_device.h> >+#include <linux/types.h> >+ >+struct imr_device { >+ struct dentry *debugfs_dir; >+ struct mutex lock; >+ int num; >+ int used; This 'used' variable is not used elsewhere, please removed. Instead, suggest to rename 'init' field which is set during probe() function if it is Quark. In all exported functions imr_add() & imr_delete(), we have test logic against init to check if we should bail-out earlier (being defensive towards erroneous used of imr_xxx exported functions.) >+ int reg_base; >+}; >+ >+static struct imr_device imr_dev; >+ >+/** >+ * Values derived from published code in Quark BSP >+ * >+ * addr_lo >+ * 31 Lock bit >+ * 30 Enable bit - not implemented on Quark X1000 >+ * 29:25 Reserved >+ * 24:2 1 kilobyte aligned lo address >+ * 1:0 Reserved >+ * >+ * addr_hi >+ * 31:25 Reserved >+ * 24:2 1 kilobyte aligned hi address >+ * 1:0 Reserved >+ */ >+#define IMR_LOCK 0x80000000 >+#define IMR_ENABLE 0x04000000 Enable bit is not present per data-sheet. So, we can remove this #define. >+ >+struct imr { >+ u32 addr_lo; >+ u32 addr_hi; >+ u32 rmask; >+ u32 wmask; >+}; >+ >+#define IMR_NUM_REGS (sizeof(struct imr)/sizeof(u32)) >+#define IMR_SHIFT 8 >+#define imr_to_phys(x) (x << IMR_SHIFT) >+#define phys_to_imr(x) (x >> IMR_SHIFT) What about address masking (0xFFFFFC)? Don't we need that? >+ >+#ifdef CONFIG_DEBUG_FS >+ >+/** >+ * imr_dbgfs_state_show >+ * Print state of IMR registers >+ * >+ * @s: pointer to seq_file for output >+ * @unused: unused parameter >+ * @return 0 on success or error code passed from mbi_iosf on failure >+ */ >+static int imr_dbgfs_state_show(struct seq_file *s, void *unused) { >+ int i, ret = -ENODEV; >+ struct imr imr; >+ u32 size; >+ >+ mutex_lock(&imr_dev.lock); >+ >+ for (i = 0; i <= imr_dev.num; i++) { >+ >+ ret = imr_read(i, &imr); >+ if (ret) >+ break; >+ >+ /* >+ * Remember to add IMR_ALIGN bytes to size to indicate the >+ * inherent IMR_ALIGN size bytes contained in the masked away >+ * lower ten bits >+ */ >+ size = 0; >+ if (imr_enabled(&imr)) { >+ size = imr_to_phys(imr.addr_hi) - >+ imr_to_phys(imr.addr_lo); >+ size += IMR_ALIGN; >+ } As explained in my separate email, even under lo=0 & hi=0, the size computed is still valid. So, I believe that the test here is redundant. >+ seq_printf(s, "imr%02i: base=0x%08x, end=0x%08x, size=0x%08x "o= >+ "rmask=0x%08x, wmask=0x%08x, %s, %s\n", i, >+ imr_to_phys(imr.addr_lo), >+ imr_to_phys(imr.addr_hi) + IMR_MASK, size, >+ imr.rmask, imr.wmask, >+ imr_enabled(&imr) ? "enabled " : "disabled", IMR enable-bit is not present and imr_enabled() test is not robust. Suggest to remove this indication which is not reliable. >+ imr.addr_lo & IMR_LOCK ? "locked" : "unlocked"); >+ } >+ >+ mutex_unlock(&imr_dev.lock); >+ >+ return ret; >+} >+ <snippet removed> >+ >+/** >+ * imr_add_range >+ * Add an Isolated Memory Region >+ * >+ * @base: Physical base address of region aligned to 4k >+ * @size: Physical size of region in bytes Please add side-note that 'size' should be 1-KiB aligned. >+ * @read_mask: Read access mask >+ * @write_mask: Write access mask >+ * @lock: Indicates whether or not to permenantly lock this region Typo: permanently >+ * @return: Index of the IMR allocated or negative value indicating >+error */ int imr_add(unsigned long base, unsigned long size, >+ unsigned int rmask, unsigned int wmask, bool lock) { >+ unsigned long end = base + size; >+ struct imr imr; >+ int reg, i, overlap, ret; >+ As explained above in imr_dev struct, we can add test against imr_dev.init here and bail-out if this function is incorrectly used. <snippet removed> >+ >+ /* Allocate IMR */ >+ imr.addr_lo = IMR_ENABLE | phys_to_imr(base); Please drop "IMR_ENABLE" here since not there is no such register bit-30. >+ >+/** >+ * imr_del_range >+ * Delete an Isolated Memory Region >+ * >+ * @reg: IMR index to remove >+ * @base: Physical base address of region aligned to 4k >+ * @size: Physical size of region in bytes >+ * @return: -EINVAL on invalid range or out or range id >+ * -ENODEV if reg is valid but no IMR exists or is locked >+ * 0 on success >+ */ >+int imr_del(int reg, unsigned long base, unsigned long size) { >+ struct imr imr; >+ int found = 0, i, ret = 0; >+ unsigned long max = base + size; >+ >+ if (imr_check_range(base, size)) >+ return -EINVAL; >+ >+ if (reg > imr_dev.num) >+ return -EINVAL; >+ >+ mutex_lock(&imr_dev.lock); >+ >+ /* if a specific IMR is given try to use it */ >+ if (reg >= 0) { >+ ret = imr_read(reg, &imr); >+ if (ret) >+ goto done; >+ >+ if (!imr_enabled(&imr) || imr.addr_lo & IMR_LOCK) { >+ ret = -ENODEV; >+ goto done; >+ } >+ found = 1; >+ } >+ >+ /* search for match based on address range */ >+ for (i = 0; i <= imr_dev.num && found == 0; i++) { >+ ret = imr_read(reg, &imr); >+ if (ret) >+ goto done; >+ >+ if (!imr_enabled(&imr) || imr.addr_lo & IMR_LOCK) >+ continue; >+ >+ if ((imr_to_phys(imr.addr_lo) == base) && >+ (imr_to_phys(imr.addr_hi) == max)) { >+ found = 1; >+ reg = i; >+ } >+ } >+ >+ if (found == 0) { >+ ret = -ENODEV; >+ goto done; >+ } >+ >+ /* Tear down the IMR */ >+ imr.addr_lo = 0; >+ imr.addr_hi = 0; >+ imr.rmask = IMR_READ_ACCESS_ALL; >+ imr.wmask = IMR_WRITE_ACCESS_ALL; >+ >+ ret = imr_write(reg, &imr, false); >+ >+done: >+ mutex_unlock(&imr_dev.lock); >+ return ret; >+} >+EXPORT_SYMBOL(imr_del); As suggested earlier, we can just offer imr delete based index-only as returned by imr_add()? We just need to check if that IMR is locked. If locked, then bail out. Else, we will zero-out IMR register for that index to remove it. This should simplify the for-loop above. Please consider... >+ >+/** >+ * intel_imr_probe >+ * entry point for IMR driver >+ * >+ * return: -ENODEV for no IMR support 0 if good to go */ static int >+__init intel_imr_init(void) { >+ struct cpuinfo_x86 *c = &cpu_data(cpu); >+ >+ if (!cpu_has_imr(c)) >+ return -ENODEV; >+ >+ if (iosf_mbi_available() == false) >+ return -ENODEV; >+ >+ if (cpu_is_quark(c)) { >+ imr_dev.num = QUARK_X1000_IMR_NUM; >+ imr_dev.reg_base = QUARK_X1000_IMR_REGBASE; >+ } else { >+ pr_err("Unknown IMR implementation\n"); >+ return -ENODEV; >+ } We can just have: if (!x86_match_cpu(soc_imr_ids) || !iosf_mbi_available()) { pr_info("IMR init failed due to IOSF_MBI not available or SoC is not Quark.\n"); return -ENODEV; } else { imr_dev.num = QUARK_X1000_IMR_NUM; imr_dev.reg_base = QUARK_X1000_IMR_REGBASE; } Where the below is added before intel_imr_init() function static const struct x86_cpu_id soc_imr_ids[] = { { X86_VENDOR_INTEL, 5, 9}, /* Intel Quark SoC X1000 */ {} }; MODULE_DEVICE_TABLE(x86cpu, soc_imr_ids); ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2015-01-08 0:04 ` Ong, Boon Leong @ 2015-01-08 13:08 ` Bryan O'Donoghue 2015-01-08 14:45 ` Ong, Boon Leong 0 siblings, 1 reply; 47+ messages in thread From: Bryan O'Donoghue @ 2015-01-08 13:08 UTC (permalink / raw) To: Ong, Boon Leong, tglx, mingo, hpa, x86, dvhart, platform-driver-x86, linux-kernel On 08/01/15 00:04, Ong, Boon Leong wrote: Hi Boon Leong - skipping the simple stuff. >> +/** >> + * imr_del_range - Delete an Isolated Memory Region >> + * @reg: IMR index to remove >> + * @base: Physical base address of region aligned to 4k >> + * @size: Physical size of region in bytes >> + * @return: -EINVAL on invalid range or out or range id >> + * -ENODEV if reg is valid but no IMR exists or is locked >> + * 0 on success >> + */ >> +int imr_del(int reg, unsigned long base, unsigned long size); > > How about just offer imr delete based index-only as returned by imr_add()? > We just need to check if that IMR is locked. If locked, then bail out. > Else, we will zero-out IMR register for that index to remove it. Hmm. The MTRR API this is based on allows you to specific an address range and I think that makes sense for an IMR API too because - say you want to tear down the IMR around the kernel .text area - but you don't know which IMR it is. You'd just do unsigned long base = virt_to_phys(&_text); unsigned long size = virt_to_phys(&_sinittext) - base - IMR_ALIGN; imr_del(-1, base, size); Rather than having to know which specific index to kill. Also later silicon may have more - or less IMR indices - so deleting based on an address range is a valuable feature I think. > if (!x86_match_cpu(soc_imr_ids) || !iosf_mbi_available()) { > pr_info("IMR init failed due to IOSF_MBI not available or SoC is not Quark.\n"); > return -ENODEV; > } else { > imr_dev.num = QUARK_X1000_IMR_NUM; > imr_dev.reg_base = QUARK_X1000_IMR_REGBASE; > } That works for me. -- BOD ^ permalink raw reply [flat|nested] 47+ messages in thread
* RE: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2015-01-08 13:08 ` Bryan O'Donoghue @ 2015-01-08 14:45 ` Ong, Boon Leong 2015-01-08 15:11 ` Bryan O'Donoghue 0 siblings, 1 reply; 47+ messages in thread From: Ong, Boon Leong @ 2015-01-08 14:45 UTC (permalink / raw) To: Bryan O'Donoghue Cc: tglx, mingo, hpa, x86, dvhart, platform-driver-x86, linux-kernel, andy.shevchenko >>> +/** >>> + * imr_del_range - Delete an Isolated Memory Region >>> + * @reg: IMR index to remove >>> + * @base: Physical base address of region aligned to 4k >>> + * @size: Physical size of region in bytes >>> + * @return: -EINVAL on invalid range or out or range id >>> + * -ENODEV if reg is valid but no IMR exists or is locked >>> + * 0 on success >>> + */ >>> +int imr_del(int reg, unsigned long base, unsigned long size); >> >> How about just offer imr delete based index-only as returned by imr_add()? >> We just need to check if that IMR is locked. If locked, then bail out. >> Else, we will zero-out IMR register for that index to remove it. > >Hmm. > >The MTRR API this is based on allows you to specific an address range and I think >that makes sense for an IMR API too because - say you want to tear down the >IMR around the kernel .text area - but you don't know which IMR it is. > >You'd just do > >unsigned long base = virt_to_phys(&_text); unsigned long size = >virt_to_phys(&_sinittext) - base - IMR_ALIGN; > >imr_del(-1, base, size); > >Rather than having to know which specific index to kill. Also later silicon may >have more - or less IMR indices - so deleting based on an address range is a >valuable feature I think. imr_add() returns IMR index, so I would expect that I can use the index directly if I need to remove it. Suggest to split the imr_del() into 2 functions:- (1) by address + size (2) by IMR index At current implementation, it does not support (2) only because it fails at imr_check_range(). ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2015-01-08 14:45 ` Ong, Boon Leong @ 2015-01-08 15:11 ` Bryan O'Donoghue 2015-01-09 3:44 ` Darren Hart 0 siblings, 1 reply; 47+ messages in thread From: Bryan O'Donoghue @ 2015-01-08 15:11 UTC (permalink / raw) To: Ong, Boon Leong Cc: tglx, mingo, hpa, x86, dvhart, platform-driver-x86, linux-kernel, andy.shevchenko > Suggest to split the imr_del() into 2 functions:- > (1) by address + size > (2) by IMR index > At current implementation, it does not support (2) only because it fails at > imr_check_range(). Hi Boon Leong. I'll have a think about that :) Just on imr_del() though, it does support removal by way of index. +static void __init intel_galileo_imr_init(void) +{ + unsigned long base = virt_to_phys(&_text); + unsigned long size = virt_to_phys(&_sinittext) - base - IMR_ALIGN; + int i, ret; + + /* Tear down all existing unlocked IMRs */ + for (i = 0; i <= QUARK_X1000_IMR_NUM; i++) + imr_del(i, 0, 0); That's what the platform code has to do for every unlocked IMR, to make sure there are no stale IMRs left that could conflict with the EFI memory map ! -- BOD ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2015-01-08 15:11 ` Bryan O'Donoghue @ 2015-01-09 3:44 ` Darren Hart 0 siblings, 0 replies; 47+ messages in thread From: Darren Hart @ 2015-01-09 3:44 UTC (permalink / raw) To: Bryan O'Donoghue Cc: Ong, Boon Leong, tglx, mingo, hpa, x86, platform-driver-x86, linux-kernel, andy.shevchenko On Thu, Jan 08, 2015 at 03:11:35PM +0000, Bryan O'Donoghue wrote: > > >Suggest to split the imr_del() into 2 functions:- > >(1) by address + size > >(2) by IMR index > >At current implementation, it does not support (2) only because it fails at > >imr_check_range(). > > Hi Boon Leong. > > I'll have a think about that :) > > Just on imr_del() though, it does support removal by way of index. > > +static void __init intel_galileo_imr_init(void) > +{ > + unsigned long base = virt_to_phys(&_text); > + unsigned long size = virt_to_phys(&_sinittext) - base - IMR_ALIGN; > + int i, ret; > + > + /* Tear down all existing unlocked IMRs */ > + for (i = 0; i <= QUARK_X1000_IMR_NUM; i++) > + imr_del(i, 0, 0); > > That's what the platform code has to do for every unlocked IMR, to make sure > there are no stale IMRs left that could conflict with the EFI memory map ! I'm OK with a single function so long as by index works without having to specify the address. Please update the kernel doc to describe this usage though. -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 2/2] platform/x86: Add Intel Galileo platform specific setup 2014-12-29 17:23 [PATCH 0/2] x86: Add IMR support to Quark/Galileo Bryan O'Donoghue 2014-12-29 17:23 ` [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 Bryan O'Donoghue @ 2014-12-29 17:23 ` Bryan O'Donoghue 2014-12-31 15:25 ` Andy Shevchenko ` (2 more replies) 2014-12-31 10:12 ` [PATCH 0/2] x86: Add IMR support to Quark/Galileo Andy Shevchenko ` (2 subsequent siblings) 4 siblings, 3 replies; 47+ messages in thread From: Bryan O'Donoghue @ 2014-12-29 17:23 UTC (permalink / raw) To: tglx, mingo, hpa, x86, dvhart, platform-driver-x86, linux-kernel Cc: Bryan O'Donoghue Intel Galileo Gen1 and Gen2 boot to Linux from EFI and grub with IMR registers enabled around the compressed kernel image and boot params data structure. The purpose of the IMRs around the compressed kernel and boot params data structure is to ensure that no spurious data writes from any agent within the Quark system can corrupt the kernel/boot-params data during boot. The kernel needs to tear-down the IMRs placed around the compressed kernel image and boot-params data structure since the EFI memory map marks the two regions of memory as usable memory and the kernel will happily reuse these memory regions. Without tearing down the boot-time IMRs drivers run the significant risk of violating one of the stale IMRs since dma_alloc_coherent can hand addresses to DMA capable south cluster peripherals such as the SD, Ethernet, USB host/device, which will then cause an IMR access violation when a DMA read/write occurs to the address ranges in question. Since the Quark EFI bringup code configures the system to reset on an IMR violation, this means that common operations such as mouting an SD based root filesystem, communicating with a USB device or sending Ethernet traffic can cause an immediate system reset. IMR usage during system boot on Galileo is detailed in Quark_SecureBoot_PRM_330234_001.pdf. This document details each IMR used during the boot process and the data being protected by that IMR. The kernel needs tidy-up IMRs used during the boot process to ensure an IMR violation doesn't cause a system reset. This platform code does two things. Firstly it tears down the boot-time IMRs used to protect the compressed kernel image and boot params data structure. Secondly it sets up an IMR around the kernel's text section from &_sinittext - &_text ensuring that on the CPU in non-SMM mode can read/write this address range. A spurious DMA write to the kernel's .text section will then cause an IMR violation and system reset, consistent with the current Galileo BSP behaviour. Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie> --- drivers/platform/x86/Kconfig | 15 +++ drivers/platform/x86/Makefile | 1 + drivers/platform/x86/intel_galileo.c | 175 +++++++++++++++++++++++++++++++++++ 3 files changed, 191 insertions(+) create mode 100644 drivers/platform/x86/intel_galileo.c diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 638e7970..e384dcd 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -804,6 +804,21 @@ config INTEL_OAKTRAIL enable/disable the Camera, WiFi, BT etc. devices. If in doubt, say Y here; it will only load on supported platforms. +config INTEL_GALILEO + bool "Intel Galileo Platform Support" + depends on X86_32 && PCI + select IOSF_MBI + select IMR + ---help--- + Intel Galileo platform support. This code is used to tear-down + BIOS and Grub Isolated Memory Regions used during bootup. + This sanitises the IMR memory map to agree with the EFI/e820 + memory map, without this code your IMR memory map will conflict + with the EFI memory map and it's highly likely DMA accesses initiated + by Ethernet, SD and/or USB will result in a system reset. + + If in doubt, say Y here, the code will only run on a Galileo Gen1/Gen2 + config SAMSUNG_Q10 tristate "Samsung Q10 Extras" depends on ACPI diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index f82232b..a0c013d 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -51,6 +51,7 @@ obj-$(CONFIG_SAMSUNG_LAPTOP) += samsung-laptop.o obj-$(CONFIG_MXM_WMI) += mxm-wmi.o obj-$(CONFIG_INTEL_MID_POWER_BUTTON) += intel_mid_powerbtn.o obj-$(CONFIG_INTEL_OAKTRAIL) += intel_oaktrail.o +obj-$(CONFIG_INTEL_GALILEO) += intel_galileo.o obj-$(CONFIG_SAMSUNG_Q10) += samsung-q10.o obj-$(CONFIG_APPLE_GMUX) += apple-gmux.o obj-$(CONFIG_INTEL_RST) += intel-rst.o diff --git a/drivers/platform/x86/intel_galileo.c b/drivers/platform/x86/intel_galileo.c new file mode 100644 index 0000000..2a555aa --- /dev/null +++ b/drivers/platform/x86/intel_galileo.c @@ -0,0 +1,175 @@ +/* + * intel_galileo.c - Intel Galileo platform support. + * + * Copyright(c) 2013 Intel Corporation. + * Copyright(c) 2014 Bryan O'Donoghue <pure.logic@nexus-software.ie> + * + * This platform code provides an entry point to do Galileo specific + * setup. Critically IMRs are policed to ensure the EFI provided memory + * map informing the kernel of it's available memory is consistent with + * the IMR lock-down + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + */ +#include <asm-generic/sections.h> +#include <asm/imr.h> +#include <asm/intel-quark.h> +#include <linux/dmi.h> +#include <linux/module.h> +#include <linux/mm.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/platform_device.h> + +enum { + GALILEO_UNKNOWN = 0, + GALILEO_QRK_GEN1, + GALILEO_QRK_GEN2, +}; + +static struct dmi_system_id galileo_baseboards[] = { + { + .ident = "Galileo", + .matches = { + DMI_MATCH(DMI_BOARD_VENDOR, "Intel"), + DMI_MATCH(DMI_BOARD_NAME, "Galileo"), + }, + .driver_data = (void *)GALILEO_QRK_GEN1, + }, + { + .ident = "GalileoGen2", + .matches = { + DMI_MATCH(DMI_BOARD_VENDOR, "Intel"), + DMI_MATCH(DMI_BOARD_NAME, "GalileoGen2"), + }, + .driver_data = (void *)GALILEO_QRK_GEN2, + }, + {} +}; + +#ifdef DEBUG +#define SANITY "IMR : sanity error " + +/** + * intel_galileo_imr_sanity + * Verify IMR sanity with some simple tests to verify + * overlap, zero sized allocations + * + * @base: Physical base address of the kernel text section + * @size: Extent of kernel memory to be covered from &_text to &_sinittext + * @return: none + */ +static void __init +intel_galileo_imr_sanity(unsigned long base, unsigned long size) +{ + /* Test zero zero */ + if (imr_add(0, 0, 0, 0, true) == 0) + pr_err(SANITY "zero sized IMR @ 0x00000000\n"); + + /* Test overlap */ + if (imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true) == 0) + pr_err(SANITY "overlapped IMR @ (0x%08lx - 0x%08lx)\n", + base, base + size); + + /* Try overlap - IMR_ALIGN */ + base = base + size - IMR_ALIGN; + if (imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true) == 0) + pr_err(SANITY "overlapped IMR @ (0x%08lx - 0x%08lx)\n", + base, base + size); +} +#endif + +/** + * intel_galileo_imr_init + * + * Tear down IMRs used during bootup. BIOS and Grub + * both setup IMRs around compressed kernel, initrd memory + * that need to be removed before the kernel hands out one + * of the IMR encased addresses to a downstream DMA agent + * such as the SD or Ethernet. IMRs on Galileo are setup to + * immediately reset the system on violation - so if you're + * running a root filesystem from SD - you'll need the IMRs + * torn down or you'll find seemingly random resets when using + * your filesystem. + */ +static void __init intel_galileo_imr_init(void) +{ + unsigned long base = virt_to_phys(&_text); + unsigned long size = virt_to_phys(&_sinittext) - base - IMR_ALIGN; + int i, ret; + + /* Tear down all existing unlocked IMRs */ + for (i = 0; i <= QUARK_X1000_IMR_NUM; i++) + imr_del(i, 0, 0); + + /* + * Setup an IMR around the physical extent of the kernel + * Non-SMM mode core read/write from/to kernel physical region. + * IMR locked. + */ + ret = imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true); + if (ret) + pr_err("Unable to setup IMR for kernel: (%p - %p)\n", + &_text, &__init_begin); + else + pr_info("IMR protect kernel memory: %ldk (%p - %p)\n", + size >> 10, &_text, &__init_begin); +#ifdef DEBUG + intel_galileo_imr_sanity(base, size); +#endif +} + +/** + * intel_galileo_init + * + * Identify a Galileo Gen1 or Gen2. If found run code to sanitise the + * kernel memory space of IMRs that are inconsistent with the EFI memory map. + */ +static int __init intel_galileo_init(void) +{ + int ret = 0, type = GALILEO_UNKNOWN; + struct cpuinfo_x86 *c = &cpu_data(cpu); + const struct dmi_system_id *system_id; + + if (!cpu_is_quark(c)) + return -ENODEV; + + system_id = dmi_first_match(galileo_baseboards); + + /* BIOS releases 0.7.5 and 0.8.0 do not provide DMI strings */ + if (system_id != NULL) { + type = (int)system_id->driver_data; + } else { + pr_info("Galileo Gen1 BIOS version <= 0.8.0\n"); + type = GALILEO_QRK_GEN1; + } + + switch (type) { + case GALILEO_QRK_GEN1: + case GALILEO_QRK_GEN2: + intel_galileo_imr_init(); + break; + default: + ret = -ENODEV; + } + + return ret; +} + +static void __exit intel_galileo_exit(void) +{ +} + +module_init(intel_galileo_init); +module_exit(intel_galileo_exit); + +MODULE_AUTHOR("Bryan O'Donoghue <pure.logic@nexus-software.ie>"); +MODULE_DESCRIPTION("Intel Galileo platform driver"); +MODULE_LICENSE("GPL v2"); -- 1.9.1 ^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH 2/2] platform/x86: Add Intel Galileo platform specific setup 2014-12-29 17:23 ` [PATCH 2/2] platform/x86: Add Intel Galileo platform specific setup Bryan O'Donoghue @ 2014-12-31 15:25 ` Andy Shevchenko 2015-01-09 1:00 ` Ong, Boon Leong 2015-01-09 4:46 ` Darren Hart 2 siblings, 0 replies; 47+ messages in thread From: Andy Shevchenko @ 2014-12-31 15:25 UTC (permalink / raw) To: Bryan O'Donoghue Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, dvhart, platform-driver-x86, linux-kernel On Mon, Dec 29, 2014 at 7:23 PM, Bryan O'Donoghue <pure.logic@nexus-software.ie> wrote: > Intel Galileo Gen1 and Gen2 boot to Linux from EFI and grub with IMR > registers enabled around the compressed kernel image and boot params data > structure. > > The purpose of the IMRs around the compressed kernel and boot params data > structure is to ensure that no spurious data writes from any agent within > the Quark system can corrupt the kernel/boot-params data during boot. > > The kernel needs to tear-down the IMRs placed around the compressed kernel > image and boot-params data structure since the EFI memory map marks the two > regions of memory as usable memory and the kernel will happily reuse these > memory regions. Without tearing down the boot-time IMRs drivers run the > significant risk of violating one of the stale IMRs since dma_alloc_coherent > can hand addresses to DMA capable south cluster peripherals such as the SD, > Ethernet, USB host/device, which will then cause an IMR access violation > when a DMA read/write occurs to the address ranges in question. > > Since the Quark EFI bringup code configures the system to reset on an IMR > violation, this means that common operations such as mouting an SD based > root filesystem, communicating with a USB device or sending Ethernet traffic > can cause an immediate system reset. > > IMR usage during system boot on Galileo is detailed in > Quark_SecureBoot_PRM_330234_001.pdf. This document details each IMR used > during the boot process and the data being protected by that IMR. The kernel > needs tidy-up IMRs used during the boot process to ensure an IMR violation > doesn't cause a system reset. > > This platform code does two things. > > Firstly it tears down the boot-time IMRs used to protect the compressed > kernel image and boot params data structure. > > Secondly it sets up an IMR around the kernel's text section from &_sinittext > - &_text ensuring that on the CPU in non-SMM mode can read/write this > address range. A spurious DMA write to the kernel's .text section will > then cause an IMR violation and system reset, consistent with the current > Galileo BSP behaviour. > > Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie> > --- > drivers/platform/x86/Kconfig | 15 +++ > drivers/platform/x86/Makefile | 1 + > drivers/platform/x86/intel_galileo.c | 175 +++++++++++++++++++++++++++++++++++ > 3 files changed, 191 insertions(+) > create mode 100644 drivers/platform/x86/intel_galileo.c > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 638e7970..e384dcd 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -804,6 +804,21 @@ config INTEL_OAKTRAIL > enable/disable the Camera, WiFi, BT etc. devices. If in doubt, say Y > here; it will only load on supported platforms. > > +config INTEL_GALILEO > + bool "Intel Galileo Platform Support" > + depends on X86_32 && PCI > + select IOSF_MBI > + select IMR > + ---help--- > + Intel Galileo platform support. This code is used to tear-down > + BIOS and Grub Isolated Memory Regions used during bootup. > + This sanitises the IMR memory map to agree with the EFI/e820 > + memory map, without this code your IMR memory map will conflict > + with the EFI memory map and it's highly likely DMA accesses initiated > + by Ethernet, SD and/or USB will result in a system reset. > + > + If in doubt, say Y here, the code will only run on a Galileo Gen1/Gen2 > + > config SAMSUNG_Q10 > tristate "Samsung Q10 Extras" > depends on ACPI > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > index f82232b..a0c013d 100644 > --- a/drivers/platform/x86/Makefile > +++ b/drivers/platform/x86/Makefile > @@ -51,6 +51,7 @@ obj-$(CONFIG_SAMSUNG_LAPTOP) += samsung-laptop.o > obj-$(CONFIG_MXM_WMI) += mxm-wmi.o > obj-$(CONFIG_INTEL_MID_POWER_BUTTON) += intel_mid_powerbtn.o > obj-$(CONFIG_INTEL_OAKTRAIL) += intel_oaktrail.o > +obj-$(CONFIG_INTEL_GALILEO) += intel_galileo.o > obj-$(CONFIG_SAMSUNG_Q10) += samsung-q10.o > obj-$(CONFIG_APPLE_GMUX) += apple-gmux.o > obj-$(CONFIG_INTEL_RST) += intel-rst.o > diff --git a/drivers/platform/x86/intel_galileo.c b/drivers/platform/x86/intel_galileo.c > new file mode 100644 > index 0000000..2a555aa > --- /dev/null > +++ b/drivers/platform/x86/intel_galileo.c > @@ -0,0 +1,175 @@ > +/* > + * intel_galileo.c - Intel Galileo platform support. > + * > + * Copyright(c) 2013 Intel Corporation. > + * Copyright(c) 2014 Bryan O'Donoghue <pure.logic@nexus-software.ie> > + * > + * This platform code provides an entry point to do Galileo specific > + * setup. Critically IMRs are policed to ensure the EFI provided memory > + * map informing the kernel of it's available memory is consistent with > + * the IMR lock-down Missed dot at the end. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + */ Define pr_fmt(). > +#include <asm-generic/sections.h> > +#include <asm/imr.h> > +#include <asm/intel-quark.h> > +#include <linux/dmi.h> > +#include <linux/module.h> > +#include <linux/mm.h> > +#include <linux/init.h> > +#include <linux/io.h> > +#include <linux/platform_device.h> > + > +enum { > + GALILEO_UNKNOWN = 0, > + GALILEO_QRK_GEN1, > + GALILEO_QRK_GEN2, > +}; > + > +static struct dmi_system_id galileo_baseboards[] = { > + { > + .ident = "Galileo", > + .matches = { > + DMI_MATCH(DMI_BOARD_VENDOR, "Intel"), > + DMI_MATCH(DMI_BOARD_NAME, "Galileo"), > + }, > + .driver_data = (void *)GALILEO_QRK_GEN1, > + }, > + { > + .ident = "GalileoGen2", > + .matches = { > + DMI_MATCH(DMI_BOARD_VENDOR, "Intel"), > + DMI_MATCH(DMI_BOARD_NAME, "GalileoGen2"), > + }, > + .driver_data = (void *)GALILEO_QRK_GEN2, > + }, > + {} > +}; > + > +#ifdef DEBUG Move this inside function. > +#define SANITY "IMR : sanity error " You may define this before function and undefine later. (Actually you missed undef) > + > +/** > + * intel_galileo_imr_sanity > + * Verify IMR sanity with some simple tests to verify > + * overlap, zero sized allocations > + * > + * @base: Physical base address of the kernel text section > + * @size: Extent of kernel memory to be covered from &_text to &_sinittext > + * @return: none Redundant. > + */ > +static void __init > +intel_galileo_imr_sanity(unsigned long base, unsigned long size) > +{ > + /* Test zero zero */ > + if (imr_add(0, 0, 0, 0, true) == 0) > + pr_err(SANITY "zero sized IMR @ 0x00000000\n"); > + > + /* Test overlap */ > + if (imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true) == 0) > + pr_err(SANITY "overlapped IMR @ (0x%08lx - 0x%08lx)\n", > + base, base + size); > + > + /* Try overlap - IMR_ALIGN */ > + base = base + size - IMR_ALIGN; base += size ... > + if (imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true) == 0) > + pr_err(SANITY "overlapped IMR @ (0x%08lx - 0x%08lx)\n", > + base, base + size); > +} > +#endif > + > +/** > + * intel_galileo_imr_init > + * > + * Tear down IMRs used during bootup. BIOS and Grub > + * both setup IMRs around compressed kernel, initrd memory > + * that need to be removed before the kernel hands out one > + * of the IMR encased addresses to a downstream DMA agent > + * such as the SD or Ethernet. IMRs on Galileo are setup to > + * immediately reset the system on violation - so if you're > + * running a root filesystem from SD - you'll need the IMRs > + * torn down or you'll find seemingly random resets when using > + * your filesystem. Split this to Summary and Description. > + */ > +static void __init intel_galileo_imr_init(void) > +{ > + unsigned long base = virt_to_phys(&_text); > + unsigned long size = virt_to_phys(&_sinittext) - base - IMR_ALIGN; > + int i, ret; > + > + /* Tear down all existing unlocked IMRs */ > + for (i = 0; i <= QUARK_X1000_IMR_NUM; i++) NUM or last one? Again confusing. > + imr_del(i, 0, 0); > + > + /* > + * Setup an IMR around the physical extent of the kernel > + * Non-SMM mode core read/write from/to kernel physical region. > + * IMR locked. > + */ > + ret = imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true); > + if (ret) > + pr_err("Unable to setup IMR for kernel: (%p - %p)\n", > + &_text, &__init_begin); > + else > + pr_info("IMR protect kernel memory: %ldk (%p - %p)\n", > + size >> 10, &_text, &__init_begin); 10 is a magic number. What is for? > +#ifdef DEBUG Move this inside the function. > + intel_galileo_imr_sanity(base, size); > +#endif > +} > + > +/** > + * intel_galileo_init > + * > + * Identify a Galileo Gen1 or Gen2. If found run code to sanitise the > + * kernel memory space of IMRs that are inconsistent with the EFI memory map. Split this to parts. > + */ > +static int __init intel_galileo_init(void) > +{ > + int ret = 0, type = GALILEO_UNKNOWN; > + struct cpuinfo_x86 *c = &cpu_data(cpu); > + const struct dmi_system_id *system_id; > + > + if (!cpu_is_quark(c)) x86_match_cpu() ? > + return -ENODEV; > + > + system_id = dmi_first_match(galileo_baseboards); > + > + /* BIOS releases 0.7.5 and 0.8.0 do not provide DMI strings */ > + if (system_id != NULL) { > + type = (int)system_id->driver_data; > + } else { > + pr_info("Galileo Gen1 BIOS version <= 0.8.0\n"); > + type = GALILEO_QRK_GEN1; > + } > + > + switch (type) { > + case GALILEO_QRK_GEN1: > + case GALILEO_QRK_GEN2: > + intel_galileo_imr_init(); > + break; > + default: > + ret = -ENODEV; return -ENODEV; and remove ret variable. > + } > + > + return ret; return 0; > +} > + > +static void __exit intel_galileo_exit(void) > +{ > +} Do we need empty exit function at all? > + > +module_init(intel_galileo_init); > +module_exit(intel_galileo_exit); > + > +MODULE_AUTHOR("Bryan O'Donoghue <pure.logic@nexus-software.ie>"); > +MODULE_DESCRIPTION("Intel Galileo platform driver"); > +MODULE_LICENSE("GPL v2"); > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 47+ messages in thread
* RE: [PATCH 2/2] platform/x86: Add Intel Galileo platform specific setup 2014-12-29 17:23 ` [PATCH 2/2] platform/x86: Add Intel Galileo platform specific setup Bryan O'Donoghue 2014-12-31 15:25 ` Andy Shevchenko @ 2015-01-09 1:00 ` Ong, Boon Leong 2015-01-09 2:11 ` Bryan O'Donoghue 2015-01-09 4:46 ` Darren Hart 2 siblings, 1 reply; 47+ messages in thread From: Ong, Boon Leong @ 2015-01-09 1:00 UTC (permalink / raw) To: Bryan O'Donoghue Cc: tglx, mingo, hpa, x86, dvhart, platform-driver-x86, linux-kernel, andy.shevchenko >Subject: [PATCH 2/2] platform/x86: Add Intel Galileo platform specific setup > [snippet removed] >Since the Quark EFI bringup code configures the system to reset on an IMR Typo: bring-up >violation, this means that common operations such as mouting an SD based root Typo: mounting [snippet removed] >+config INTEL_GALILEO >+ bool "Intel Galileo Platform Support" >+ depends on X86_32 && PCI >+ select IOSF_MBI >+ select IMR >+ ---help--- >+ Intel Galileo platform support. This code is used to tear-down >+ BIOS and Grub Isolated Memory Regions used during bootup. Missing dash. Should be "boot-up". [snippet removed] >diff --git a/drivers/platform/x86/intel_galileo.c >b/drivers/platform/x86/intel_galileo.c >new file mode 100644 >index 0000000..2a555aa >--- /dev/null >+++ b/drivers/platform/x86/intel_galileo.c >@@ -0,0 +1,175 @@ >+/* >+ * intel_galileo.c - Intel Galileo platform support. >+ * >+ * Copyright(c) 2013 Intel Corporation. >+ * Copyright(c) 2014 Bryan O'Donoghue <pure.logic@nexus-software.ie> >+ * >+ * This platform code provides an entry point to do Galileo specific >+ * setup. Critically IMRs are policed to ensure the EFI provided memory Critically --> Critical >+ * map informing the kernel of it's available memory is consistent with It's --> its [snippet removed] >+ >+enum { >+ GALILEO_UNKNOWN = 0, >+ GALILEO_QRK_GEN1, >+ GALILEO_QRK_GEN2, >+}; Suggest to drop QRK to kill confusion that it is Quark Gen 1 & 2. [snippet removed] >+#ifdef DEBUG >+#define SANITY "IMR : sanity error " >+ >+/** Per coding style, please just use /* and kill the extra * above and below. >+ * intel_galileo_imr_sanity >+ * Verify IMR sanity with some simple tests to verify >+ * overlap, zero sized allocations >+ * >+ * @base: Physical base address of the kernel text section >+ * @size: Extent of kernel memory to be covered from &_text to >+&_sinittext >+ * @return: none >+ */ >+static void __init >+intel_galileo_imr_sanity(unsigned long base, unsigned long size) { >+ /* Test zero zero */ >+ if (imr_add(0, 0, 0, 0, true) == 0) >+ pr_err(SANITY "zero sized IMR @ 0x00000000\n"); A side-discussion on imr_add(): I would think that we should allow 1KiB IMR setting. Current imr_add() logic is prohibiting it. So, the 'size' input should be at least 1KiB and imr_add() internal logic will adjust 'hi' by -1KiB. Please consider .. >+ >+ /* Test overlap */ >+ if (imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true) == 0) >+ pr_err(SANITY "overlapped IMR @ (0x%08lx - 0x%08lx)\n", >+ base, base + size); >+ >+ /* Try overlap - IMR_ALIGN */ >+ base = base + size - IMR_ALIGN; >+ if (imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true) == 0) >+ pr_err(SANITY "overlapped IMR @ (0x%08lx - 0x%08lx)\n", >+ base, base + size); >+} >+#endif >+ >+/** >+ * intel_galileo_imr_init >+ * >+ * Tear down IMRs used during bootup. BIOS and Grub boot-up. >+ * both setup IMRs around compressed kernel, initrd memory >+ * that need to be removed before the kernel hands out one >+ * of the IMR encased addresses to a downstream DMA agent >+ * such as the SD or Ethernet. IMRs on Galileo are setup to >+ * immediately reset the system on violation - so if you're >+ * running a root filesystem from SD - you'll need the IMRs >+ * torn down or you'll find seemingly random resets when using >+ * your filesystem. >+ */ >+static void __init intel_galileo_imr_init(void) { >+ unsigned long base = virt_to_phys(&_text); >+ unsigned long size = virt_to_phys(&_sinittext) - base - IMR_ALIGN; >+ int i, ret; >+ >+ /* Tear down all existing unlocked IMRs */ >+ for (i = 0; i <= QUARK_X1000_IMR_NUM; i++) >+ imr_del(i, 0, 0); >+ >+ /* >+ * Setup an IMR around the physical extent of the kernel >+ * Non-SMM mode core read/write from/to kernel physical region. >+ * IMR locked. >+ */ >+ ret = imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true); >+ if (ret) >+ pr_err("Unable to setup IMR for kernel: (%p - %p)\n", >+ &_text, &__init_begin); >+ else >+ pr_info("IMR protect kernel memory: %ldk (%p - %p)\n", >+ size >> 10, &_text, &__init_begin); Perhaps we want to be consistent between using __init_begin & _sinittext? >+#ifdef DEBUG >+ intel_galileo_imr_sanity(base, size); >+#endif >+} >+ >+/** >+ * intel_galileo_init >+ * >+ * Identify a Galileo Gen1 or Gen2. If found run code to sanitise the >+ * kernel memory space of IMRs that are inconsistent with the EFI memory >map. >+ */ >+static int __init intel_galileo_init(void) { >+ int ret = 0, type = GALILEO_UNKNOWN; type is assigned to either GALILEO_GEN1|2 below anyway. So, we don't need to declare to UNKNOWN? [snippet removed] ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 2/2] platform/x86: Add Intel Galileo platform specific setup 2015-01-09 1:00 ` Ong, Boon Leong @ 2015-01-09 2:11 ` Bryan O'Donoghue 0 siblings, 0 replies; 47+ messages in thread From: Bryan O'Donoghue @ 2015-01-09 2:11 UTC (permalink / raw) To: Ong, Boon Leong Cc: tglx, mingo, hpa, x86, dvhart, platform-driver-x86, linux-kernel, andy.shevchenko On 09/01/15 01:00, Ong, Boon Leong wrote: >> +static void __init >> +intel_galileo_imr_sanity(unsigned long base, unsigned long size) { >> + /* Test zero zero */ >> + if (imr_add(0, 0, 0, 0, true) == 0) >> + pr_err(SANITY "zero sized IMR @ 0x00000000\n"); > > A side-discussion on imr_add(): > I would think that we should allow 1KiB IMR setting. Current imr_add() logic > is prohibiting it. Hi Boon Leong. Ermm, it does allow 1 KiB IMR regions. The following code works on the unmodifed V1 driver. /* Test 1 KiB works */ idx = imr_add(0, IMR_ALIGN, IMR_READ_ACCESS_ALL, IMR_WRITE_ACCESS_ALL,false); if (idx < 0) pr_err(SANITY "Couldn't add an IMR @ 0x%04x bytes\n", IMR_ALIGN); Note IMR_ALIGN is 0x400 I'll add that test to the set of sanity tests in V2 just to put your mind at ease though. > So, the 'size' input should be at least 1KiB and imr_add() > internal logic will adjust 'hi' by -1KiB. Please consider .. Hmm. Actually I had a response all typed out for you why I didn't want an API to presume to modify the size of my input from the user's POV but, thinking about it twice - I agree with you. V2 will subtract IMR_ALIGN (0x400) bytes from the size. It's stupid to have to subtract IMR_ALIGN bytes on the input - and assumes the user of the API understands how the hardware works - but, of course the point of an API is so that the user of it doesn't *have* to understand that. Good call. -- BOD ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 2/2] platform/x86: Add Intel Galileo platform specific setup 2014-12-29 17:23 ` [PATCH 2/2] platform/x86: Add Intel Galileo platform specific setup Bryan O'Donoghue 2014-12-31 15:25 ` Andy Shevchenko 2015-01-09 1:00 ` Ong, Boon Leong @ 2015-01-09 4:46 ` Darren Hart 2015-01-09 11:17 ` Bryan O'Donoghue 2 siblings, 1 reply; 47+ messages in thread From: Darren Hart @ 2015-01-09 4:46 UTC (permalink / raw) To: Bryan O'Donoghue Cc: tglx, mingo, hpa, x86, platform-driver-x86, linux-kernel On Mon, Dec 29, 2014 at 05:23:03PM +0000, Bryan O'Donoghue wrote: Hi Bryan, Good discussion from Andy and Boon Leong, I'll try not to duplicate their review here. > Intel Galileo Gen1 and Gen2 boot to Linux from EFI and grub with IMR > registers enabled around the compressed kernel image and boot params data > structure. > > The purpose of the IMRs around the compressed kernel and boot params data > structure is to ensure that no spurious data writes from any agent within > the Quark system can corrupt the kernel/boot-params data during boot. > > The kernel needs to tear-down the IMRs placed around the compressed kernel > image and boot-params data structure since the EFI memory map marks the two > regions of memory as usable memory and the kernel will happily reuse these > memory regions. Without tearing down the boot-time IMRs drivers run the > significant risk of violating one of the stale IMRs since dma_alloc_coherent > can hand addresses to DMA capable south cluster peripherals such as the SD, > Ethernet, USB host/device, which will then cause an IMR access violation > when a DMA read/write occurs to the address ranges in question. > > Since the Quark EFI bringup code configures the system to reset on an IMR > violation, this means that common operations such as mouting an SD based > root filesystem, communicating with a USB device or sending Ethernet traffic > can cause an immediate system reset. > > IMR usage during system boot on Galileo is detailed in > Quark_SecureBoot_PRM_330234_001.pdf. This document details each IMR used > during the boot process and the data being protected by that IMR. The kernel > needs tidy-up IMRs used during the boot process to ensure an IMR violation > doesn't cause a system reset. does not (generally speaking, please avoid contractions in documentation) > > This platform code does two things. > > Firstly it tears down the boot-time IMRs used to protect the compressed First, > kernel image and boot params data structure. > > Secondly it sets up an IMR around the kernel's text section from &_sinittext Second, > - &_text ensuring that on the CPU in non-SMM mode can read/write this > address range. A spurious DMA write to the kernel's .text section will > then cause an IMR violation and system reset, consistent with the current > Galileo BSP behaviour. > > Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie> > --- > drivers/platform/x86/Kconfig | 15 +++ > drivers/platform/x86/Makefile | 1 + > drivers/platform/x86/intel_galileo.c | 175 +++++++++++++++++++++++++++++++++++ > 3 files changed, 191 insertions(+) > create mode 100644 drivers/platform/x86/intel_galileo.c > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 638e7970..e384dcd 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -804,6 +804,21 @@ config INTEL_OAKTRAIL > enable/disable the Camera, WiFi, BT etc. devices. If in doubt, say Y > here; it will only load on supported platforms. > > +config INTEL_GALILEO > + bool "Intel Galileo Platform Support" > + depends on X86_32 && PCI > + select IOSF_MBI > + select IMR > + ---help--- > + Intel Galileo platform support. This code is used to tear-down > + BIOS and Grub Isolated Memory Regions used during bootup. > + This sanitises the IMR memory map to agree with the EFI/e820 > + memory map, without this code your IMR memory map will conflict > + with the EFI memory map and it's highly likely DMA accesses initiated it is > + by Ethernet, SD and/or USB will result in a system reset. > + > + If in doubt, say Y here, the code will only run on a Galileo Gen1/Gen2 > + > config SAMSUNG_Q10 > tristate "Samsung Q10 Extras" > depends on ACPI > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > index f82232b..a0c013d 100644 > --- a/drivers/platform/x86/Makefile > +++ b/drivers/platform/x86/Makefile > @@ -51,6 +51,7 @@ obj-$(CONFIG_SAMSUNG_LAPTOP) += samsung-laptop.o > obj-$(CONFIG_MXM_WMI) += mxm-wmi.o > obj-$(CONFIG_INTEL_MID_POWER_BUTTON) += intel_mid_powerbtn.o > obj-$(CONFIG_INTEL_OAKTRAIL) += intel_oaktrail.o > +obj-$(CONFIG_INTEL_GALILEO) += intel_galileo.o > obj-$(CONFIG_SAMSUNG_Q10) += samsung-q10.o > obj-$(CONFIG_APPLE_GMUX) += apple-gmux.o > obj-$(CONFIG_INTEL_RST) += intel-rst.o > diff --git a/drivers/platform/x86/intel_galileo.c b/drivers/platform/x86/intel_galileo.c > new file mode 100644 > index 0000000..2a555aa > --- /dev/null > +++ b/drivers/platform/x86/intel_galileo.c > @@ -0,0 +1,175 @@ > +/* > + * intel_galileo.c - Intel Galileo platform support. > + * > + * Copyright(c) 2013 Intel Corporation. > + * Copyright(c) 2014 Bryan O'Donoghue <pure.logic@nexus-software.ie> > + * > + * This platform code provides an entry point to do Galileo specific > + * setup. Critically IMRs are policed to ensure the EFI provided memory > + * map informing the kernel of it's available memory is consistent with > + * the IMR lock-down > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + */ > +#include <asm-generic/sections.h> > +#include <asm/imr.h> > +#include <asm/intel-quark.h> > +#include <linux/dmi.h> > +#include <linux/module.h> > +#include <linux/mm.h> > +#include <linux/init.h> > +#include <linux/io.h> > +#include <linux/platform_device.h> > + > +enum { > + GALILEO_UNKNOWN = 0, > + GALILEO_QRK_GEN1, > + GALILEO_QRK_GEN2, > +}; > + > +static struct dmi_system_id galileo_baseboards[] = { > + { > + .ident = "Galileo", > + .matches = { > + DMI_MATCH(DMI_BOARD_VENDOR, "Intel"), > + DMI_MATCH(DMI_BOARD_NAME, "Galileo"), > + }, > + .driver_data = (void *)GALILEO_QRK_GEN1, > + }, > + { > + .ident = "GalileoGen2", > + .matches = { > + DMI_MATCH(DMI_BOARD_VENDOR, "Intel"), > + DMI_MATCH(DMI_BOARD_NAME, "GalileoGen2"), > + }, > + .driver_data = (void *)GALILEO_QRK_GEN2, > + }, > + {} > +}; > + > +#ifdef DEBUG > +#define SANITY "IMR : sanity error " > + > +/** > + * intel_galileo_imr_sanity > + * Verify IMR sanity with some simple tests to verify > + * overlap, zero sized allocations > + * > + * @base: Physical base address of the kernel text section > + * @size: Extent of kernel memory to be covered from &_text to &_sinittext > + * @return: none > + */ > +static void __init > +intel_galileo_imr_sanity(unsigned long base, unsigned long size) > +{ > + /* Test zero zero */ > + if (imr_add(0, 0, 0, 0, true) == 0) > + pr_err(SANITY "zero sized IMR @ 0x00000000\n"); > + > + /* Test overlap */ > + if (imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true) == 0) > + pr_err(SANITY "overlapped IMR @ (0x%08lx - 0x%08lx)\n", > + base, base + size); > + > + /* Try overlap - IMR_ALIGN */ > + base = base + size - IMR_ALIGN; > + if (imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true) == 0) > + pr_err(SANITY "overlapped IMR @ (0x%08lx - 0x%08lx)\n", > + base, base + size); > +} > +#endif I'd rather see this as CONFIG_DEBUG_IMR under Kernel Hacking. What about this sanity test is galileo specific? > + > +/** > + * intel_galileo_imr_init > + * > + * Tear down IMRs used during bootup. BIOS and Grub > + * both setup IMRs around compressed kernel, initrd memory > + * that need to be removed before the kernel hands out one > + * of the IMR encased addresses to a downstream DMA agent > + * such as the SD or Ethernet. IMRs on Galileo are setup to > + * immediately reset the system on violation - so if you're > + * running a root filesystem from SD - you'll need the IMRs > + * torn down or you'll find seemingly random resets when using > + * your filesystem. > + */ > +static void __init intel_galileo_imr_init(void) > +{ > + unsigned long base = virt_to_phys(&_text); extra space > + unsigned long size = virt_to_phys(&_sinittext) - base - IMR_ALIGN; > + int i, ret; One var declaration per line please, per CodingStyle. > + > + /* Tear down all existing unlocked IMRs */ > + for (i = 0; i <= QUARK_X1000_IMR_NUM; i++) > + imr_del(i, 0, 0); > + > + /* > + * Setup an IMR around the physical extent of the kernel > + * Non-SMM mode core read/write from/to kernel physical region. > + * IMR locked. > + */ > + ret = imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true); > + if (ret) > + pr_err("Unable to setup IMR for kernel: (%p - %p)\n", > + &_text, &__init_begin); > + else > + pr_info("IMR protect kernel memory: %ldk (%p - %p)\n", > + size >> 10, &_text, &__init_begin); > +#ifdef DEBUG > + intel_galileo_imr_sanity(base, size); > +#endif > +} > + > +/** > + * intel_galileo_init > + * > + * Identify a Galileo Gen1 or Gen2. If found run code to sanitise the > + * kernel memory space of IMRs that are inconsistent with the EFI memory map. > + */ > +static int __init intel_galileo_init(void) > +{ > + int ret = 0, type = GALILEO_UNKNOWN; One var per line, ret last. Order by descending line length when in doubt. > + struct cpuinfo_x86 *c = &cpu_data(cpu); > + const struct dmi_system_id *system_id; > + > + if (!cpu_is_quark(c)) > + return -ENODEV; > + > + system_id = dmi_first_match(galileo_baseboards); > + > + /* BIOS releases 0.7.5 and 0.8.0 do not provide DMI strings */ > + if (system_id != NULL) { > + type = (int)system_id->driver_data; > + } else { > + pr_info("Galileo Gen1 BIOS version <= 0.8.0\n"); > + type = GALILEO_QRK_GEN1; So this will load on any Quark device, Galileo or not, that doesn't provide a system_id. Is there any reason we need to support 0.8.0 and earlier firmware? I'd prefer not to successfully load a driver on the wrong platform because we assume the user knows what they are doing :-) This effective converts this from a "platform driver" to a "board file" - the bad kind. > + } > + > + switch (type) { > + case GALILEO_QRK_GEN1: > + case GALILEO_QRK_GEN2: > + intel_galileo_imr_init(); > + break; > + default: > + ret = -ENODEV; > + } > + > + return ret; > +} > + > +static void __exit intel_galileo_exit(void) > +{ > +} > + > +module_init(intel_galileo_init); > +module_exit(intel_galileo_exit); > + > +MODULE_AUTHOR("Bryan O'Donoghue <pure.logic@nexus-software.ie>"); > +MODULE_DESCRIPTION("Intel Galileo platform driver"); > +MODULE_LICENSE("GPL v2"); > -- > 1.9.1 > > Thanks, -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 2/2] platform/x86: Add Intel Galileo platform specific setup 2015-01-09 4:46 ` Darren Hart @ 2015-01-09 11:17 ` Bryan O'Donoghue 2015-01-09 11:29 ` Bryan O'Donoghue 2015-01-10 6:54 ` Darren Hart 0 siblings, 2 replies; 47+ messages in thread From: Bryan O'Donoghue @ 2015-01-09 11:17 UTC (permalink / raw) To: Darren Hart; +Cc: tglx, mingo, hpa, x86, platform-driver-x86, linux-kernel On 09/01/15 04:46, Darren Hart wrote: >> + /* Try overlap - IMR_ALIGN */ >> + base = base + size - IMR_ALIGN; >> + if (imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true) == 0) >> + pr_err(SANITY "overlapped IMR @ (0x%08lx - 0x%08lx)\n", >> + base, base + size); >> +} >> +#endif > > I'd rather see this as CONFIG_DEBUG_IMR under Kernel Hacking. > > What about this sanity test is galileo specific? Exactly nothing. I think it's a fair point to make this * CONFIG_DEBUG_IMR * Embedded in the IMR init code >> + /* BIOS releases 0.7.5 and 0.8.0 do not provide DMI strings */ >> + if (system_id != NULL) { >> + type = (int)system_id->driver_data; >> + } else { >> + pr_info("Galileo Gen1 BIOS version <= 0.8.0\n"); >> + type = GALILEO_QRK_GEN1; > > So this will load on any Quark device, Galileo or not, that doesn't provide a > system_id. Is there any reason we need to support 0.8.0 and earlier firmware? Every Galileo Gen1 device ships with firmware version 0.7.5. You can do an EFI capsule update to 0.8.0 which still isn't DMI-enabled - or you can go and get a firmware greater than 0.9.0 and get DMI strings. > I'd prefer not to successfully load a driver on the wrong platform because we > assume the user knows what they are doing :-) This effective converts this from > a "platform driver" to a "board file" - the bad kind. That would be a problem if there were any other X1000 boards that don't provide DMI strings but only Galileo Gen1 out of the box is DMI deprived, so for that reason I think falling back to assume Gen1 when you've identified a Quark X1000 is the right thing to do. Right now the only Quark X1000 devices that real users in the field have are Galileo boards which either identify with DMI strings (Gen2 and upgraded Gen1 boards) - or don't identify with DMI strings Gen1 @ 0.7.5 and 0.8.0 Also consider that any new X1000 based systems running Linux will be based on the latest reference firmware from Intel which provides DMI identifiers, so Galileo Gen3 if it is in the making will have a DMI string to identify itself as a Gen3, same with a Gen2 and upgraded Gen1. The only X1000 based boards without DMI strings are going to be Galileo Gen1 devices @ firmware version 0.7.5 and 0.8.0, so I don't think we will end up in a situation of loading the wrong platform code, rather we'll accommodate the older firmware this way All that said - there *is* an alternative for 0.7.5 and 0.8.0 firmware but, I know you won't like it. Prior to 0.9.0 firmware Galileo boards were identified by 1. Mapping a section of SPI flash 2. Finding a specific header at a know location on SPI flash 3. Extracting a unique platform ID field from that header 4. Examining that platform ID field and loading Galileo specific drivers if found So in theory we could go this route if you feel that fallback the fallback described above isn't robust enough. I'm OK to add that code in for V2 and we can decide which is the more desirable way to do it - fallback or extract of platform id from SPI flash and then finalise at V3 -- BOD ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 2/2] platform/x86: Add Intel Galileo platform specific setup 2015-01-09 11:17 ` Bryan O'Donoghue @ 2015-01-09 11:29 ` Bryan O'Donoghue 2015-01-09 14:11 ` Ong, Boon Leong 2015-01-10 6:54 ` Darren Hart 1 sibling, 1 reply; 47+ messages in thread From: Bryan O'Donoghue @ 2015-01-09 11:29 UTC (permalink / raw) To: Darren Hart; +Cc: tglx, mingo, hpa, x86, platform-driver-x86, linux-kernel On 09/01/15 11:17, Bryan O'Donoghue wrote: >> So this will load on any Quark device, Galileo or not, that doesn't >> provide a >> system_id. Is there any reason we need to support 0.8.0 and earlier >> firmware? > > Every Galileo Gen1 device ships with firmware version 0.7.5. You can do > an EFI capsule update to 0.8.0 which still isn't DMI-enabled - or you > can go and get a firmware greater than 0.9.0 and get DMI strings. I should add to that. Hundreds of the Gen1 devices were given away @ MakerFaire Rome 2013, and AFAIK thousands were given to universities. So there's probably a few thousands boards floating around with the 0.7.5 firmware that we want to support. Like I say - I think the fall-back is the simplest option and is also safe. I can add in the old way of identifying the boards and we can review it for appropriateness. I didn't include that old method for the fallback because it involves mapping SPI flash and parsing custom headers but - it works - even if it is ugly. -- BOD ^ permalink raw reply [flat|nested] 47+ messages in thread
* RE: [PATCH 2/2] platform/x86: Add Intel Galileo platform specific setup 2015-01-09 11:29 ` Bryan O'Donoghue @ 2015-01-09 14:11 ` Ong, Boon Leong 0 siblings, 0 replies; 47+ messages in thread From: Ong, Boon Leong @ 2015-01-09 14:11 UTC (permalink / raw) To: Bryan O'Donoghue, Darren Hart Cc: tglx, mingo, hpa, x86, platform-driver-x86, linux-kernel > >On 09/01/15 11:17, Bryan O'Donoghue wrote: >>> So this will load on any Quark device, Galileo or not, that doesn't >>> provide a system_id. Is there any reason we need to support 0.8.0 and >>> earlier firmware? >> >> Every Galileo Gen1 device ships with firmware version 0.7.5. You can >> do an EFI capsule update to 0.8.0 which still isn't DMI-enabled - or >> you can go and get a firmware greater than 0.9.0 and get DMI strings. > >I should add to that. Hundreds of the Gen1 devices were given away @ >MakerFaire Rome 2013, and AFAIK thousands were given to universities. So >there's probably a few thousands boards floating around with the 0.7.5 >firmware that we want to support. > >Like I say - I think the fall-back is the simplest option and is also safe. I can add in >the old way of identifying the boards and we can review it for appropriateness. >I didn't include that old method for the fallback because it involves mapping SPI >flash and parsing custom headers but - it works - even if it is ugly. > I agree with Bryan on his view-point. The logic to default to Galileo Gen-1 with older-firmware. seems reasonable to me too. We cannot for sure older Galileo Gen-1 user will upgrade to newer version of firmware. I suspect that many will not (because lack of understanding of how UEFI capsule can do that or lack of equipment to flash it too, especially university students). So for the lack of better & robust approach, the default fallback when DMI string is not detectable to GENv1 seems reasonable. My 2 cents. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 2/2] platform/x86: Add Intel Galileo platform specific setup 2015-01-09 11:17 ` Bryan O'Donoghue 2015-01-09 11:29 ` Bryan O'Donoghue @ 2015-01-10 6:54 ` Darren Hart 2015-01-11 1:53 ` Bryan O'Donoghue 1 sibling, 1 reply; 47+ messages in thread From: Darren Hart @ 2015-01-10 6:54 UTC (permalink / raw) To: Bryan O'Donoghue Cc: tglx, mingo, hpa, x86, platform-driver-x86, linux-kernel, Matthew Garrett, Boon Leong Ong, Mel Gorman On Fri, Jan 09, 2015 at 11:17:43AM +0000, Bryan O'Donoghue wrote: > On 09/01/15 04:46, Darren Hart wrote: > >>+ /* Try overlap - IMR_ALIGN */ > >>+ base = base + size - IMR_ALIGN; > >>+ if (imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true) == 0) > >>+ pr_err(SANITY "overlapped IMR @ (0x%08lx - 0x%08lx)\n", > >>+ base, base + size); > >>+} > >>+#endif > > > >I'd rather see this as CONFIG_DEBUG_IMR under Kernel Hacking. > > > >What about this sanity test is galileo specific? > > Exactly nothing. > > I think it's a fair point to make this > * CONFIG_DEBUG_IMR > * Embedded in the IMR init code > > >>+ /* BIOS releases 0.7.5 and 0.8.0 do not provide DMI strings */ > >>+ if (system_id != NULL) { > >>+ type = (int)system_id->driver_data; > >>+ } else { > >>+ pr_info("Galileo Gen1 BIOS version <= 0.8.0\n"); > >>+ type = GALILEO_QRK_GEN1; > > > >So this will load on any Quark device, Galileo or not, that doesn't provide a > >system_id. Is there any reason we need to support 0.8.0 and earlier firmware? > > Every Galileo Gen1 device ships with firmware version 0.7.5. You can do an > EFI capsule update to 0.8.0 which still isn't DMI-enabled - or you can go > and get a firmware greater than 0.9.0 and get DMI strings. > > >I'd prefer not to successfully load a driver on the wrong platform because we > >assume the user knows what they are doing :-) This effective converts this from > >a "platform driver" to a "board file" - the bad kind. > > That would be a problem if there were any other X1000 boards that don't > provide DMI strings but only Galileo Gen1 out of the box is DMI deprived, so > for that reason I think falling back to assume Gen1 when you've identified a > Quark X1000 is the right thing to do. > > Right now the only Quark X1000 devices that real users in the field have are > Galileo boards which either identify with DMI strings (Gen2 and upgraded > Gen1 boards) - or don't identify with DMI strings Gen1 @ 0.7.5 and 0.8.0 > > Also consider that any new X1000 based systems running Linux will be based > on the latest reference firmware from Intel which provides DMI identifiers, > so Galileo Gen3 if it is in the making will have a DMI string to identify > itself as a Gen3, same with a Gen2 and upgraded Gen1. > > The only X1000 based boards without DMI strings are going to be Galileo Gen1 > devices @ firmware version 0.7.5 and 0.8.0, so I don't think we will end up > in a situation of loading the wrong platform code, rather we'll accommodate > the older firmware this way > > All that said - there *is* an alternative for 0.7.5 and 0.8.0 firmware but, > I know you won't like it. > > Prior to 0.9.0 firmware Galileo boards were identified by > > 1. Mapping a section of SPI flash > 2. Finding a specific header at a know location on SPI flash > 3. Extracting a unique platform ID field from that header > 4. Examining that platform ID field and loading Galileo specific drivers if > found > > So in theory we could go this route if you feel that fallback the fallback > described above isn't robust enough. > > I'm OK to add that code in for V2 and we can decide which is the more > desirable way to do it - fallback or extract of platform id from SPI flash > and then finalise at V3 I'm wondering if there is a need for this to be a platform driver at all. Could we, instead, tear down any unlocked IMRs at boot as part of the IMR driver itself, as well as lock the kernel .text. Firmware has the option of making an IMR mandatory by locking it. If it isn't locked, what use is it to the OS without a lot more context? Any sort of policy enforced with IMRs is a user-space decision, so clearing out the unlocked ones and then handing off to user-space seems like a sane default mode to me. I discussed this with Boon Leong this afternoon and we couldn't come up with a justification for a platform-specific driver to do what this does. Cc'ing Mel for his thoughts on the VM bit. Cc'ing Matthew for his take on platform driver appropriateness. -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 2/2] platform/x86: Add Intel Galileo platform specific setup 2015-01-10 6:54 ` Darren Hart @ 2015-01-11 1:53 ` Bryan O'Donoghue 0 siblings, 0 replies; 47+ messages in thread From: Bryan O'Donoghue @ 2015-01-11 1:53 UTC (permalink / raw) To: Darren Hart Cc: tglx, mingo, hpa, x86, platform-driver-x86, linux-kernel, Matthew Garrett, Boon Leong Ong, Mel Gorman > I'm wondering if there is a need for this to be a platform driver at > all. Could we, instead, tear down any unlocked IMRs at boot as part of the IMR > driver itself, as well as lock the kernel .text. Firmware has the option of > making an IMR mandatory by locking it. If it isn't locked, what use is it to the > OS without a lot more context? Any sort of policy enforced with IMRs is a > user-space decision, so clearing out the unlocked ones and then handing off to > user-space seems like a sane default mode to me. > > I discussed this with Boon Leong this afternoon and we couldn't come up with a > justification for a platform-specific driver to do what this does. I think there's no technical reason impeding what you've just suggested in the IMR init and I'm happy to move the code in there since basically any platform with IMR registers could justifiably a) Tear down everything that's unlocked b) Place a default IMR around the .text section No reason it the world that that policy needs to be Galileo specific - and it's a valid argument to say - this behaviour can simply be an IMR policy in the kernel and less code for the same functionality, that adds consistency across x86 is a good thing. I'll zap the Galileo platform code entirely for V2 unless there's a counter-argument from someone. -- BOD ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/2] x86: Add IMR support to Quark/Galileo 2014-12-29 17:23 [PATCH 0/2] x86: Add IMR support to Quark/Galileo Bryan O'Donoghue 2014-12-29 17:23 ` [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 Bryan O'Donoghue 2014-12-29 17:23 ` [PATCH 2/2] platform/x86: Add Intel Galileo platform specific setup Bryan O'Donoghue @ 2014-12-31 10:12 ` Andy Shevchenko 2014-12-31 11:59 ` Bryan O'Donoghue 2015-01-02 2:02 ` Darren Hart 2015-01-02 4:24 ` Darren Hart 2015-01-06 6:00 ` Darren Hart 4 siblings, 2 replies; 47+ messages in thread From: Andy Shevchenko @ 2014-12-31 10:12 UTC (permalink / raw) To: Bryan O'Donoghue Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, dvhart, platform-driver-x86, linux-kernel On Mon, Dec 29, 2014 at 7:23 PM, Bryan O'Donoghue <pure.logic@nexus-software.ie> wrote: > This patchset adds an IMR driver to the kernel plus platform code for > Intel Galileo Gen1/Gen2 boards. [] > Bryan O'Donoghue (2): > x86: Add Isolated Memory Regions for Quark X1000 > platform/x86 Add Intel Galileo platform specific setup I'm going to review this soon, but here few comments below. > arch/x86/Kconfig | 23 ++ > arch/x86/include/asm/imr.h | 79 ++++++ > arch/x86/include/asm/intel-quark.h | 31 ++ Could it be just a quark.h? Like for ce4100. Those intel- prefixes in the modules looks awkward especially when pathname consists x86 already. > arch/x86/kernel/Makefile | 1 + > arch/x86/kernel/imr.c | 529 +++++++++++++++++++++++++++++++++++ > drivers/platform/x86/Kconfig | 15 + > drivers/platform/x86/Makefile | 1 + > drivers/platform/x86/intel_galileo.c | 175 ++++++++++++ Here what about to make an hierarchy like: intel/galileo.c intel/mid/... would be those modules with intel_mid_ prefixes in future. See my proposal regarding to drivers/mfd [1] > 8 files changed, 854 insertions(+) > create mode 100644 arch/x86/include/asm/imr.h > create mode 100644 arch/x86/include/asm/intel-quark.h > create mode 100644 arch/x86/kernel/imr.c > create mode 100644 drivers/platform/x86/intel_galileo.c [1] http://www.spinics.net/lists/kernel/msg1886819.html -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/2] x86: Add IMR support to Quark/Galileo 2014-12-31 10:12 ` [PATCH 0/2] x86: Add IMR support to Quark/Galileo Andy Shevchenko @ 2014-12-31 11:59 ` Bryan O'Donoghue 2015-01-02 2:02 ` Darren Hart 1 sibling, 0 replies; 47+ messages in thread From: Bryan O'Donoghue @ 2014-12-31 11:59 UTC (permalink / raw) To: Andy Shevchenko Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, dvhart, platform-driver-x86, linux-kernel On 31/12/14 10:12, Andy Shevchenko wrote: > On Mon, Dec 29, 2014 at 7:23 PM, Bryan O'Donoghue > <pure.logic@nexus-software.ie> wrote: >> This patchset adds an IMR driver to the kernel plus platform code for >> Intel Galileo Gen1/Gen2 boards. > > [] > >> Bryan O'Donoghue (2): >> x86: Add Isolated Memory Regions for Quark X1000 >> platform/x86 Add Intel Galileo platform specific setup > > I'm going to review this soon, but here few comments below. > >> arch/x86/Kconfig | 23 ++ >> arch/x86/include/asm/imr.h | 79 ++++++ >> arch/x86/include/asm/intel-quark.h | 31 ++ > > Could it be just a quark.h? Like for ce4100. > Those intel- prefixes in the modules looks awkward especially when > pathname consists x86 already. Sure thing. I agree there's no real logic to prefixing with intel. >> arch/x86/kernel/Makefile | 1 + >> arch/x86/kernel/imr.c | 529 +++++++++++++++++++++++++++++++++++ >> drivers/platform/x86/Kconfig | 15 + >> drivers/platform/x86/Makefile | 1 + >> drivers/platform/x86/intel_galileo.c | 175 ++++++++++++ > > Here what about to make an hierarchy like: > intel/galileo.c > intel/mid/... would be those modules with intel_mid_ prefixes in > future. See my proposal regarding to drivers/mfd [1] > >> 8 files changed, 854 insertions(+) >> create mode 100644 arch/x86/include/asm/imr.h >> create mode 100644 arch/x86/include/asm/intel-quark.h >> create mode 100644 arch/x86/kernel/imr.c >> create mode 100644 drivers/platform/x86/intel_galileo.c > > > [1] http://www.spinics.net/lists/kernel/msg1886819.html That works for me will remember to include for V2. BOD ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/2] x86: Add IMR support to Quark/Galileo 2014-12-31 10:12 ` [PATCH 0/2] x86: Add IMR support to Quark/Galileo Andy Shevchenko 2014-12-31 11:59 ` Bryan O'Donoghue @ 2015-01-02 2:02 ` Darren Hart 1 sibling, 0 replies; 47+ messages in thread From: Darren Hart @ 2015-01-02 2:02 UTC (permalink / raw) To: Andy Shevchenko Cc: Bryan O'Donoghue, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, platform-driver-x86, linux-kernel On Wed, Dec 31, 2014 at 12:12:58PM +0200, Andy Shevchenko wrote: > On Mon, Dec 29, 2014 at 7:23 PM, Bryan O'Donoghue > <pure.logic@nexus-software.ie> wrote: > > This patchset adds an IMR driver to the kernel plus platform code for > > Intel Galileo Gen1/Gen2 boards. > > [] > > > Bryan O'Donoghue (2): > > x86: Add Isolated Memory Regions for Quark X1000 > > platform/x86 Add Intel Galileo platform specific setup > > I'm going to review this soon, but here few comments below. Thanks for your review Andy, good advice throughout. > > > arch/x86/Kconfig | 23 ++ > > arch/x86/include/asm/imr.h | 79 ++++++ > > arch/x86/include/asm/intel-quark.h | 31 ++ > > Could it be just a quark.h? Like for ce4100. > Those intel- prefixes in the modules looks awkward especially when > pathname consists x86 already. > > > arch/x86/kernel/Makefile | 1 + > > arch/x86/kernel/imr.c | 529 +++++++++++++++++++++++++++++++++++ > > drivers/platform/x86/Kconfig | 15 + > > drivers/platform/x86/Makefile | 1 + > > drivers/platform/x86/intel_galileo.c | 175 ++++++++++++ > > Here what about to make an hierarchy like: > intel/galileo.c > intel/mid/... would be those modules with intel_mid_ prefixes in > future. See my proposal regarding to drivers/mfd [1] As Bryan is only adding one file to the platform/drivers/x86, let's skip the reorg as part of this patch series. We can consider that separately if someone wants to make the argument that the time has come to add another directory layer. -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/2] x86: Add IMR support to Quark/Galileo 2014-12-29 17:23 [PATCH 0/2] x86: Add IMR support to Quark/Galileo Bryan O'Donoghue ` (2 preceding siblings ...) 2014-12-31 10:12 ` [PATCH 0/2] x86: Add IMR support to Quark/Galileo Andy Shevchenko @ 2015-01-02 4:24 ` Darren Hart 2015-01-06 6:00 ` Darren Hart 4 siblings, 0 replies; 47+ messages in thread From: Darren Hart @ 2015-01-02 4:24 UTC (permalink / raw) To: Bryan O'Donoghue Cc: tglx, mingo, hpa, x86, platform-driver-x86, linux-kernel On Mon, Dec 29, 2014 at 05:23:01PM +0000, Bryan O'Donoghue wrote: > This patchset adds an IMR driver to the kernel plus platform code for > Intel Galileo Gen1/Gen2 boards. I will be taking a closer look at this on Monday the 5th when I return from vacation. -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/2] x86: Add IMR support to Quark/Galileo 2014-12-29 17:23 [PATCH 0/2] x86: Add IMR support to Quark/Galileo Bryan O'Donoghue ` (3 preceding siblings ...) 2015-01-02 4:24 ` Darren Hart @ 2015-01-06 6:00 ` Darren Hart 2015-01-06 13:56 ` Bryan O'Donoghue 4 siblings, 1 reply; 47+ messages in thread From: Darren Hart @ 2015-01-06 6:00 UTC (permalink / raw) To: Bryan O'Donoghue Cc: tglx, mingo, hpa, x86, platform-driver-x86, linux-kernel On Mon, Dec 29, 2014 at 05:23:01PM +0000, Bryan O'Donoghue wrote: > This patchset adds an IMR driver to the kernel plus platform code for > Intel Galileo Gen1/Gen2 boards. > > IMRs: > Quark SoC X1000 ships with a set of registers called Isolated Memory Regions > IMRs provide fine grained memory access control to various system agents > within the SoC such as CPU SMM/non-SMM mode, PCIe virtual channels, CPU snoop > cycles, eSRAM flush cycles and the RMU. In simple terms, IMRs provide a > mechanism to protect memory regions from unwarranted access by system agents > that should not have access to that memory. > > IMRs support a lock bit. Once a lock bit is set for an individual IMR it is > not possible to tear down that IMR without performing a cold boot of the > system. IMRs support reporting of violations. The SoC system can be > configured to reboot immediately when an IMR violation has taken place. > Immediate reboot of the system on IMR violation is recommended and is > currently how Quark BIOS configures the system. > > As an example Galileo boards ship with an IMR around the ACPI runtime > services memory and if a DMA read/write cycle were to occur to this region > of memory this would trigger the IMR violation mechansim. > > Galileo: > Intel's Arduino compatible Galileo boards boot to Linux with IMRs protecting > the compressed kernel image and boot params data structure. The memory that What is the motivation behind this? > the compressed kernel and boot params data structure is in, is marked as > usable memory by the EFI memory map. As a result it is possible for memory Based on your response to the above, is marking this memory as usable a bad idea in general? Or just bad in certain situations? > marked as processor read/write only in an IMR to be given to devices in the > SoC for the purposes of DMA by way of dma_alloc_coherent. New line > A DMA to a region of memory by a system agent which is not allowed access > this memory result in a system reset. Without tearing down the IMRs placed > around the compressed kernel image and boot params data structure there is a > high risk of triggering an inadvertent system reset when performing DMA > actions with any of the peripherals that support DMA in Quark such as the > MMC, Ethernet or USB host/device. > > Therefore Galileo specific platform code is the second component of this > patchset. The platform code tears-down every unlocked IMR to ensure no The firmware sets these IMRs, but does not lock them then, correct? > conflict exists between the IMR usage during boot and the EFI memory map. In > addition an IMR is placed around the kernel's .text section to ensure no > invalid access to kernel code can happen by way of spurious DMA, SMM or RMU > read/write cycles. This code gets compiled into the kernel because we want > to run the code early before any DMA has taken place. The prime examples of > DMA transactions resetting the system are mouting a root filesystem on MMC mounting > or mouting a root filesystem over NFS. mounting -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/2] x86: Add IMR support to Quark/Galileo 2015-01-06 6:00 ` Darren Hart @ 2015-01-06 13:56 ` Bryan O'Donoghue 2015-01-06 16:48 ` Darren Hart 0 siblings, 1 reply; 47+ messages in thread From: Bryan O'Donoghue @ 2015-01-06 13:56 UTC (permalink / raw) To: Darren Hart; +Cc: tglx, mingo, hpa, x86, platform-driver-x86, linux-kernel On 06/01/15 06:00, Darren Hart wrote: >> Galileo: >> Intel's Arduino compatible Galileo boards boot to Linux with IMRs protecting >> the compressed kernel image and boot params data structure. The memory that > > What is the motivation behind this? The main motivation is to place an IMR around the kernel which if violated by a wayward DMA transaction would immediately cause an IMR violation. Secondary motivation is demonstration of usage of IMRs in a run-time context and validation of the IMR enabling code for setup not just tear-down. An IMR around the run-time kernel .text area, is what the BSP does and so I thought it was worth maintaining. To be clear though, the requirement is to sanitize boot time IMRs, the setup of an IMR around the run-time kernel is optional, the system will run just fine without it. >> the compressed kernel and boot params data structure is in, is marked as >> usable memory by the EFI memory map. As a result it is possible for memory > > Based on your response to the above, is marking this memory as usable a bad idea > in general? Or just bad in certain situations? The EFI memory map is 100% correct. The area of memory that grub places a compressed kernel image should be reusable by kernel. >> A DMA to a region of memory by a system agent which is not allowed access >> this memory result in a system reset. Without tearing down the IMRs placed >> around the compressed kernel image and boot params data structure there is a >> high risk of triggering an inadvertent system reset when performing DMA >> actions with any of the peripherals that support DMA in Quark such as the >> MMC, Ethernet or USB host/device. >> >> Therefore Galileo specific platform code is the second component of this >> patchset. The platform code tears-down every unlocked IMR to ensure no > > The firmware sets these IMRs, but does not lock them then, correct? Correct. Firmware locks the IMRs around ACPI runtime data data. In the patch here, we cycle though every unlocked IMR and zap it - which will include tear-down of the IMRs placed around the compressed kernel image and boot params data structure. Firmware puts those IMRs in place to ensure no invalid DMA, SMM access to the compressed kernel image during boot can take place. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/2] x86: Add IMR support to Quark/Galileo 2015-01-06 13:56 ` Bryan O'Donoghue @ 2015-01-06 16:48 ` Darren Hart 2015-01-06 17:23 ` Bryan O'Donoghue 0 siblings, 1 reply; 47+ messages in thread From: Darren Hart @ 2015-01-06 16:48 UTC (permalink / raw) To: Bryan O'Donoghue Cc: tglx, mingo, hpa, x86, platform-driver-x86, linux-kernel On Tue, Jan 06, 2015 at 01:56:25PM +0000, Bryan O'Donoghue wrote: > On 06/01/15 06:00, Darren Hart wrote: > >>Galileo: > >>Intel's Arduino compatible Galileo boards boot to Linux with IMRs protecting > >>the compressed kernel image and boot params data structure. The memory that > > > >What is the motivation behind this? > > The main motivation is to place an IMR around the kernel which if violated > by a wayward DMA transaction would immediately cause an IMR violation. > Secondary motivation is demonstration of usage of IMRs in a run-time context > and validation of the IMR enabling code for setup not just tear-down. > > An IMR around the run-time kernel .text area, is what the BSP does and so I > thought it was worth maintaining. > > To be clear though, the requirement is to sanitize boot time IMRs, the setup > of an IMR around the run-time kernel is optional, the system will run just > fine without it. > > >>the compressed kernel and boot params data structure is in, is marked as > >>usable memory by the EFI memory map. As a result it is possible for memory > > > >Based on your response to the above, is marking this memory as usable a bad idea > >in general? Or just bad in certain situations? > > The EFI memory map is 100% correct. The area of memory that grub places a > compressed kernel image should be reusable by kernel. > > >>A DMA to a region of memory by a system agent which is not allowed access > >>this memory result in a system reset. Without tearing down the IMRs placed > >>around the compressed kernel image and boot params data structure there is a > >>high risk of triggering an inadvertent system reset when performing DMA > >>actions with any of the peripherals that support DMA in Quark such as the > >>MMC, Ethernet or USB host/device. > >> > >>Therefore Galileo specific platform code is the second component of this > >>patchset. The platform code tears-down every unlocked IMR to ensure no > > > >The firmware sets these IMRs, but does not lock them then, correct? > > Correct. Firmware locks the IMRs around ACPI runtime data data. > > In the patch here, we cycle though every unlocked IMR and zap it - which > will include tear-down of the IMRs placed around the compressed kernel image > and boot params data structure. Firmware puts those IMRs in place to ensure > no invalid DMA, SMM access to the compressed kernel image during boot can > take place. Thanks Bryan, this clears these questions up for me. Are we confident that the tear-down of the IMRs around the compressed kernel image happens early enough and deterministically, such that there is no race condition in which a driver could get DMA into this memory? -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/2] x86: Add IMR support to Quark/Galileo 2015-01-06 16:48 ` Darren Hart @ 2015-01-06 17:23 ` Bryan O'Donoghue 0 siblings, 0 replies; 47+ messages in thread From: Bryan O'Donoghue @ 2015-01-06 17:23 UTC (permalink / raw) To: Darren Hart; +Cc: tglx, mingo, hpa, x86, platform-driver-x86, linux-kernel On 06/01/15 16:48, Darren Hart wrote: >> Correct. Firmware locks the IMRs around ACPI runtime data data. >> >> In the patch here, we cycle though every unlocked IMR and zap it - which >> will include tear-down of the IMRs placed around the compressed kernel image >> and boot params data structure. Firmware puts those IMRs in place to ensure >> no invalid DMA, SMM access to the compressed kernel image during boot can >> take place. > > Thanks Bryan, this clears these questions up for me. > > Are we confident that the tear-down of the IMRs around the compressed kernel > image happens early enough and deterministically, such that there is no race > condition in which a driver could get DMA into this memory? Yes confident :) Tested by baking IMR-platform code + MMC into the kernel and mounting the rootfs on Galileo from MMC. Also tested the reverse case. Attempting to mount an SD kills every version of the tip-of-tree I've used with an IMR reset. Just to be really sure, it's also been tested with the initial 0.7.5 and 0.8.0 release which didn't have DMI strings to identify the board. So - with this change in place tip-of-tree should work for everybody on Galileo Gen1/Gen2. It should get the Galileo Debian people out of the business of needing to have a separate kernel to boot - though obvious more enabling work needs to happen in SoC space still. http://sourceforge.net/projects/galileodebian/ Also I'm hoping - though I don't have the exact details of this - that the crash/non-boot situation for CentOs on Galileo may be as a result of IMRs, so hopefully will be of use there too. ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v6 0/2] x86: Add IMR support to Quark/Galileo @ 2015-01-28 18:36 Bryan O'Donoghue 2015-01-28 18:36 ` [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 Bryan O'Donoghue 0 siblings, 1 reply; 47+ messages in thread From: Bryan O'Donoghue @ 2015-01-28 18:36 UTC (permalink / raw) To: tglx, mingo, hpa, x86, dvhart, andy.shevchenko, boon.leong.ong, linux-kernel Cc: 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 v5: - Full stops Added to all comments - including one-liners. Andy Shevchenko - %zu and %zx Used to print size_t variables Andy Shevchenko - imr_selftest Split into seperate module Andy Shevchenko Changes since v4: - Quark added as a platform in a separate patch to IMRs Andy Shevchenko - imr_read Add extra reg++ to end of read for consistency Andy Shevchenko - ret Declarations of return variable moved to end of each declaration block Andy Shevchenko - Parameters passed to debugfs hooks Andy Shevchenko - struct imr_dev *idev = &imr_dev; Used globally for entry point functions Andy Shevchenko/Bryan O'Donoghue - struct imr_dev * passed to static functions Andy Shevchenko/Bryan O'Donoghue - Comment in imr_fixup_memmap Paried down and made more readable Andy Shevchenko - imr_fixup_memmap/imr_selftest phys_addr_t used for base Andy Shevchenko - phys_addr_t/size_t Supplemented globally in place of unsigned long where appropriate Bryan O'Donoghue - print routines to take %pa globally In place of pr_info("0x%08lx\n", (unsigned long)some_phs_addr_t_variable); Andy Shevchenko - debugfs hook moved to after init = true Andy Shevchenko Changes since v3: - Remove reference to imr.o in arch/x86/kernel/Makefile Bryan O'Donoghue Changes since v2: - Move IMR code to arch/x86/platform/intel-quark/imr.c Thomas Gleixner/Darren Hart - #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt Andy Shevchenko - ret = iosf_mbi_read() Style made consistent in imr_write Andy Shevchenko - reg++/IMR_NUM_REGS Offset used for lock bit in imr_write Andy Shevchenko - debugfs s->private pointer used Andy Shevchenko - debugfs Conditional compilation defines removed Andy Shevchenko - debugfs Failure to hook debugfs treated as non-fatal Andy Shevchenko - phys_addr_t Updated API to use phys_addr_t in place of unsigned long for base param Andy Shevchenko - printk "KiB" instead of "k" Andy Shevchenko - imr_enabled -> static inline imr_is_enabled Ong, Boon Leong/Andy Shevchenko - imr_write trap final ret from iosf_mbi_write for lock bit in imr_write - bugfix Ong, Boon Leong - imr_fixup_size -> static inline imr_fixup_size Ong, Boon Leong - imr_address_overlap -> imr_address_overlap Ong, Boon Leong - imr_add_range End address in imr_add_range calculated after imr_fixup_size() Ong, Boon Leong - imr_del_range Pass i in place of reg in imr_del_range() - bugfix Ong, Boon Leong - Add test case imr_del_range(-1, addr, size) Ong, Boon Leong/Andy Shevchenko - Added text "aligned to 1 KiB" removed reference to "4 k" Ong, Boon Leong - imr_is_enabled Definition of enabled updated to be negation of disabled Bryan O'Donoghue - imr_add_range Add check for adding an IMR in the disabled state Bryan O'Donoghue - Add test case IMR @ invalid address, @0 with rmask/wmask=CPU, @ 0 size 0x800 Bryan O'Donoghue - Add WARN() to failed IMR test in print routine Bryan O'Donoghue - Update license to Dual BSD/GPL Reflect licensing in Intel reference code Bryan O'Donoghue 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 (2): x86: Add Isolated Memory Regions for Quark X1000 x86, quark: Add Intel Quark platform support arch/x86/Kconfig | 16 + arch/x86/Kconfig.debug | 12 + arch/x86/include/asm/imr.h | 60 +++ arch/x86/platform/Makefile | 1 + arch/x86/platform/intel-quark/Makefile | 2 + arch/x86/platform/intel-quark/imr.c | 616 +++++++++++++++++++++++++++ arch/x86/platform/intel-quark/imr_selftest.c | 135 ++++++ drivers/platform/x86/Kconfig | 25 ++ 8 files changed, 867 insertions(+) create mode 100644 arch/x86/include/asm/imr.h create mode 100644 arch/x86/platform/intel-quark/Makefile create mode 100644 arch/x86/platform/intel-quark/imr.c create mode 100644 arch/x86/platform/intel-quark/imr_selftest.c -- 1.9.1 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2015-01-28 18:36 [PATCH v6 " Bryan O'Donoghue @ 2015-01-28 18:36 ` Bryan O'Donoghue 2015-01-29 5:38 ` Darren Hart ` (2 more replies) 0 siblings, 3 replies; 47+ messages in thread From: Bryan O'Donoghue @ 2015-01-28 18:36 UTC (permalink / raw) To: tglx, mingo, hpa, x86, dvhart, andy.shevchenko, boon.leong.ong, linux-kernel Cc: 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 write-only, 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.debug | 12 + arch/x86/include/asm/imr.h | 60 +++ arch/x86/platform/intel-quark/Makefile | 2 + arch/x86/platform/intel-quark/imr.c | 616 +++++++++++++++++++++++++++ arch/x86/platform/intel-quark/imr_selftest.c | 135 ++++++ drivers/platform/x86/Kconfig | 25 ++ 6 files changed, 850 insertions(+) create mode 100644 arch/x86/include/asm/imr.h create mode 100644 arch/x86/platform/intel-quark/Makefile create mode 100644 arch/x86/platform/intel-quark/imr.c create mode 100644 arch/x86/platform/intel-quark/imr_selftest.c diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug index 61bd2ad..fcf5701 100644 --- a/arch/x86/Kconfig.debug +++ b/arch/x86/Kconfig.debug @@ -313,6 +313,18 @@ config DEBUG_NMI_SELFTEST If unsure, say N. +config DEBUG_IMR_SELFTEST + bool "Isolated Memory Region self test" + default n + depends on INTEL_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 here. 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..0542dc2 --- /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 way 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(phys_addr_t base, size_t size, + unsigned int rmask, unsigned int wmask, bool lock); + +int imr_remove_range(int reg, phys_addr_t base, size_t size); + +#endif /* _IMR_H */ diff --git a/arch/x86/platform/intel-quark/Makefile b/arch/x86/platform/intel-quark/Makefile new file mode 100644 index 0000000..9cc57ed --- /dev/null +++ b/arch/x86/platform/intel-quark/Makefile @@ -0,0 +1,2 @@ +obj-$(CONFIG_INTEL_IMR) += imr.o +obj-$(CONFIG_DEBUG_IMR_SELFTEST) += imr_selftest.o diff --git a/arch/x86/platform/intel-quark/imr.c b/arch/x86/platform/intel-quark/imr.c new file mode 100644 index 0000000..6157a3b --- /dev/null +++ b/arch/x86/platform/intel-quark/imr.c @@ -0,0 +1,616 @@ +/** + * 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) 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/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_SHIFT 8 +#define imr_to_phys(x) ((x) << IMR_SHIFT) +#define phys_to_imr(x) ((x) >> IMR_SHIFT) + +/** + * imr_is_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 inline int imr_is_enabled(struct imr_regs *imr) +{ + return !(imr->rmask == IMR_READ_ACCESS_ALL && + imr->wmask == IMR_WRITE_ACCESS_ALL && + imr_to_phys(imr->addr_lo) == 0 && + imr_to_phys(imr->addr_hi) == 0); +} + +/** + * imr_read - read an IMR at a given index. + * + * Requires caller to hold imr mutex. + * + * @idev: pointer to imr_device structure. + * @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(struct imr_device *idev, u32 imr_id, struct imr_regs *imr) +{ + u32 reg = imr_id * IMR_NUM_REGS + idev->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; + + ret = iosf_mbi_read(QRK_MBI_UNIT_MM, QRK_MBI_MM_READ, + reg++, &imr->wmask); + if (ret) + return ret; + + return 0; +} + +/** + * 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. + * + * @idev: pointer to imr_device structure. + * @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(struct imr_device *idev, u32 imr_id, + struct imr_regs *imr, bool lock) +{ + unsigned long flags; + u32 reg = imr_id * IMR_NUM_REGS + idev->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_NUM_REGS, imr->addr_lo); + if (ret) + goto done; + } + + 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; +} + +/** + * 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) +{ + phys_addr_t base; + phys_addr_t end; + int i; + struct imr_device *idev = s->private; + struct imr_regs imr; + size_t size; + int ret = -ENODEV; + + mutex_lock(&idev->lock); + + for (i = 0; i < idev->max_imr; i++) { + + ret = imr_read(idev, 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. + */ + if (imr_is_enabled(&imr)) { + base = imr_to_phys(imr.addr_lo); + end = imr_to_phys(imr.addr_hi) + IMR_MASK; + } else { + base = 0; + end = 0; + } + size = end - base; + seq_printf(s, "imr%02i: base=%pa, end=%pa, size=0x%08zx " + "rmask=0x%08x, wmask=0x%08x, %s, %s\n", i, + &base, &end, size, imr.rmask, imr.wmask, + imr_is_enabled(&imr) ? "enabled " : "disabled", + imr.addr_lo & IMR_LOCK ? "locked" : "unlocked"); + } + + mutex_unlock(&idev->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. + * + * @idev: pointer to imr_device structure. + * @return: 0 on success - errno on failure. + */ +static int imr_debugfs_register(struct imr_device *idev) +{ + idev->file = debugfs_create_file("imr_state", S_IFREG | S_IRUGO, NULL, + idev, &imr_state_ops); + if (!idev->file) + return -ENOMEM; + + return 0; +} + +/** + * imr_debugfs_unregister - unregister debugfs hooks. + * + * @idev: pointer to imr_device structure. + * @return: + */ +static void imr_debugfs_unregister(struct imr_device *idev) +{ + debugfs_remove(idev->file); +} + +/** + * imr_check_range - checks 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(phys_addr_t base, size_t size) +{ + if ((base & IMR_MASK) || (size & IMR_MASK)) { + pr_warn("base %pa size 0x%08zx 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 inline size_t imr_fixup_size(size_t 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 inline int imr_address_overlap(phys_addr_t 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(phys_addr_t base, size_t size, + unsigned int rmask, unsigned int wmask, bool lock) +{ + phys_addr_t end; + unsigned int i; + struct imr_device *idev = &imr_dev; + struct imr_regs imr; + int reg; + int ret; + + if (idev->init == false) + return -ENODEV; + + ret = imr_check_range(base, size); + if (ret) + return ret; + + if (size == 0) + return -EINVAL; + + /* Tweak the size value. */ + size = imr_fixup_size(size); + end = base + size; + + /* + * Check for reserved IMR value common to firmware, kernel and grub + * indicating a disabled IMR. + */ + imr.addr_lo = phys_to_imr(base); + imr.addr_hi = phys_to_imr(end); + imr.rmask = rmask; + imr.wmask = wmask; + if (!imr_is_enabled(&imr)) + return -EINVAL; + + mutex_lock(&idev->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 < idev->max_imr; i++) { + ret = imr_read(idev, i, &imr); + if (ret) + goto done; + + /* Find overlap @ base or end of requested range. */ + if (imr_is_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 %pa-%pa 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(idev, reg, &imr, lock); + +done: + mutex_unlock(&idev->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 its index specified by reg or + * by address range specified by base and size respectively. If you specify an + * index on its 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 1 KiB. + * @size: physical size of region in bytes 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, phys_addr_t base, size_t size) +{ + phys_addr_t end; + bool found = false; + unsigned int i; + struct imr_device *idev = &imr_dev; + struct imr_regs imr; + int ret = 0; + + if (idev->init == false) + return -ENODEV; + + if (imr_check_range(base, size)) + return -EINVAL; + + if (reg >= idev->max_imr) + return -EINVAL; + + /* Tweak the size value. */ + size = imr_fixup_size(size); + end = base + size; + + mutex_lock(&idev->lock); + + if (reg >= 0) { + /* If a specific IMR is given try to use it. */ + ret = imr_read(idev, reg, &imr); + if (ret) + goto done; + + if (!imr_is_enabled(&imr) || imr.addr_lo & IMR_LOCK) { + ret = -ENODEV; + goto done; + } + found = true; + } else { + /* Search for match based on address range. */ + for (i = 0; i < idev->max_imr; i++) { + ret = imr_read(idev, i, &imr); + if (ret) + goto done; + + if (!imr_is_enabled(&imr) || imr.addr_lo & IMR_LOCK) + continue; + + if ((imr_to_phys(imr.addr_lo) == base) && + (imr_to_phys(imr.addr_hi) == end)) { + found = true; + reg = i; + break; + } + } + } + + if (found == false) { + 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(idev, reg, &imr, false); + +done: + mutex_unlock(&idev->lock); + return ret; +} +EXPORT_SYMBOL(imr_remove_range); + +/** + * 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. + * + * @idev: pointer to imr_device structure. + * @return: + */ +static void __init imr_fixup_memmap(struct imr_device *idev) +{ + phys_addr_t base = virt_to_phys(&_text); + size_t size = virt_to_phys(&__end_rodata) - base; + int i; + int ret; + + /* Tear down all existing unlocked IMRs. */ + for (i = 0; i < idev->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 as one physically contiguous block. + */ + 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: %zu KiB (%p - %p)\n", + size / 1024, &_text, &__end_rodata); + +} + +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_init - entry point for IMR driver. + * + * return: -ENODEV for no IMR support 0 if good to go. + */ +static int __init imr_init(void) +{ + struct imr_device *idev = &imr_dev; + int ret; + + if (!x86_match_cpu(imr_ids) || !iosf_mbi_available()) + return -ENODEV; + + idev->max_imr = QUARK_X1000_IMR_MAX; + idev->reg_base = QUARK_X1000_IMR_REGBASE; + idev->init = true; + + mutex_init(&idev->lock); + ret = imr_debugfs_register(idev); + if (ret != 0) + pr_warn("debugfs register failed!\n"); + imr_fixup_memmap(idev); + return 0; +} + +/** + * imr_exit - exit point for IMR code. + * + * Deregisters debugfs, leave IMR state as-is. + * + * return: + */ +static void __exit imr_exit(void) +{ + imr_debugfs_unregister(&imr_dev); +} + +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("Dual BSD/GPL"); diff --git a/arch/x86/platform/intel-quark/imr_selftest.c b/arch/x86/platform/intel-quark/imr_selftest.c new file mode 100644 index 0000000..ddecd6e --- /dev/null +++ b/arch/x86/platform/intel-quark/imr_selftest.c @@ -0,0 +1,135 @@ +/** + * imr_selftest.c + * + * Copyright(c) 2013 Intel Corporation. + * Copyright(c) 2015 Bryan O'Donoghue <pure.logic@nexus-software.ie> + * + * IMR self test. The purpose of this module is to run a set of tests on the + * IMR API to validate it's sanity. We check for overlapping, reserved + * addresses and setup/teardown sanity. + * + */ + +#include <asm-generic/sections.h> +#include <asm/imr.h> +#include <linux/init.h> +#include <linux/mm.h> +#include <linux/module.h> +#include <linux/types.h> + +#define SELFTEST KBUILD_MODNAME ": 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); + WARN(res == 0, "test failed"); +} +#undef SELFTEST + +/** + * imr_self_test + * + * Verify IMR self_test with some simple tests to verify overlap, + * zero sized allocations and 1 KiB sized areas. + * + */ +static void __init imr_self_test(void) +{ + phys_addr_t base = virt_to_phys(&_text); + size_t size = virt_to_phys(&__end_rodata) - base; + 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 that a 1 KiB IMR @ zero with read/write all will bomb out. */ + 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 - access-all\n"); + + /* Test that a 1 KiB IMR @ zero with CPU only will work. */ + idx = imr_add_range(0, IMR_ALIGN, IMR_CPU, IMR_CPU, false); + imr_self_test_result(idx >= 0, "1KiB IMR @ 0x00000000 - cpu access\n"); + if (idx >= 0) { + /* For an index removal address doesn't matter. */ + idx = imr_remove_range(idx, 0, 0); + imr_self_test_result(idx == 0, "index-teardown - cpu access\n"); + } + + /* Test 2 KiB works - remove based on index. */ + size = IMR_ALIGN * 2; + idx = imr_add_range(0, size, IMR_READ_ACCESS_ALL, + IMR_WRITE_ACCESS_ALL, false); + imr_self_test_result(idx >= 0, "2KiB IMR @ 0x00000000\n"); + if (idx >= 0) { + /* For an index removal address doesn't matter. */ + idx = imr_remove_range(idx, 0, 0); + imr_self_test_result(idx == 0, "index-teardown 2KiB\n"); + } + + /* Test 1 KiB - remove based on address range. */ + idx = imr_add_range(0, size, IMR_READ_ACCESS_ALL, + IMR_WRITE_ACCESS_ALL, false); + imr_self_test_result(idx >= 0, "2KiB IMR @ 0x00000000\n"); + if (idx >= 0) { + idx = imr_remove_range(-1, 0, size); + imr_self_test_result(idx == 0, "addr-teardown 2KiB\n"); + } +} + +/** + * imr_self_test_init - entry point for IMR driver. + * + * return: -ENODEV for no IMR support 0 if good to go. + */ +static int __init imr_self_test_init(void) +{ + imr_self_test(); + return 0; +} + +/** + * imr_self_test_exit - exit point for IMR code. + * + * return: + */ +static void __exit imr_self_test_exit(void) +{ +} + +module_init(imr_self_test_init); +module_exit(imr_self_test_exit); + +MODULE_AUTHOR("Bryan O'Donoghue <pure.logic@nexus-software.ie>"); +MODULE_DESCRIPTION("Intel Isolated Memory Region self-test driver"); +MODULE_LICENSE("Dual BSD/GPL"); diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 638e7970..9752761 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -735,6 +735,31 @@ config INTEL_IPS functionality. If in doubt, say Y here; it will only load on supported platforms. +config INTEL_IMR + bool "Intel Isolated Memory Region support" + default n + depends on X86_INTEL_QUARK && 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 IBM_RTL tristate "Device driver to enable PRTL support" depends on X86 && PCI -- 1.9.1 ^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2015-01-28 18:36 ` [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 Bryan O'Donoghue @ 2015-01-29 5:38 ` Darren Hart 2015-01-29 7:44 ` Ingo Molnar 2015-01-29 9:59 ` Ong, Boon Leong 2 siblings, 0 replies; 47+ messages in thread From: Darren Hart @ 2015-01-29 5:38 UTC (permalink / raw) To: Bryan O'Donoghue Cc: tglx, mingo, hpa, x86, andy.shevchenko, boon.leong.ong, linux-kernel On Wed, Jan 28, 2015 at 06:36:25PM +0000, Bryan O'Donoghue 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 write-only, 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> Most of my concerns were addressed by V3 or so, but I've followed along and concur with the subsequent improvements. Reviewed-by: Darren Hart <dvhart@linux.intel.com> -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2015-01-28 18:36 ` [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 Bryan O'Donoghue 2015-01-29 5:38 ` Darren Hart @ 2015-01-29 7:44 ` Ingo Molnar 2015-01-29 10:08 ` Andy Shevchenko 2015-01-29 13:27 ` Bryan O'Donoghue 2015-01-29 9:59 ` Ong, Boon Leong 2 siblings, 2 replies; 47+ messages in thread From: Ingo Molnar @ 2015-01-29 7:44 UTC (permalink / raw) To: Bryan O'Donoghue Cc: tglx, mingo, hpa, x86, dvhart, andy.shevchenko, boon.leong.ong, linux-kernel * 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. A couple of minor details: > + 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_NUM_REGS, imr->addr_lo); > + if (ret) > + goto done; > + } > + > + 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; > +} looks like the 'done' label really wants to be 'error' or 'failed'? Also, if this message ever triggers in practice, if the IMR is in a likely bad state, we might as well attempt to turn it off, so that we have a chance to get the log out and all that? > + > +/** > + * 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) > +{ > + phys_addr_t base; > + phys_addr_t end; > + int i; > + struct imr_device *idev = s->private; > + struct imr_regs imr; > + size_t size; > + int ret = -ENODEV; > + > + mutex_lock(&idev->lock); > + > + for (i = 0; i < idev->max_imr; i++) { > > +int imr_add_range(phys_addr_t base, size_t size, > + unsigned int rmask, unsigned int wmask, bool lock) > +{ > + phys_addr_t end; > + unsigned int i; > + struct imr_device *idev = &imr_dev; > + struct imr_regs imr; > + int reg; > + int ret; > + > + if (idev->init == false) > + return -ENODEV; You might want to add a WARN_ONCE() here? Someone tried to add an IMR but has done it too early. Return values get ignored so often, kernel messages not so much. (Just like imr_check_range() does emit warnings.) > + ret = imr_check_range(base, size); > + if (ret) > + return ret; > + > + if (size == 0) > + return -EINVAL; Looks like the zero-size check could be added to imr_check_range(), and be renamed to imr_check_params() or so? > + /* Tweak the size value. */ > + size = imr_fixup_size(size); So why not add a 'raw_size' (or so) variable and use that, to not confuse user provided size with the offset-corrected size? Then imr_fixup_size() could be renamed to imr_raw_size() as well, making it slightly easier on the eyes as well. > + end = base + size; > + > + /* > + * Check for reserved IMR value common to firmware, kernel and grub > + * indicating a disabled IMR. > + */ > + imr.addr_lo = phys_to_imr(base); > + imr.addr_hi = phys_to_imr(end); > + imr.rmask = rmask; > + imr.wmask = wmask; > + if (!imr_is_enabled(&imr)) > + return -EINVAL; > + > + mutex_lock(&idev->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 < idev->max_imr; i++) { > + ret = imr_read(idev, i, &imr); > + if (ret) > + goto done; > + > + /* Find overlap @ base or end of requested range. */ > + if (imr_is_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 %pa-%pa rmask 0x%08x wmask 0x%08x\n", > + reg, &base, &end, rmask, wmask); > + > + /* Allocate IMR. */ Technically we don't 'allocate' the IMR here, we write to an existing, currently disabled IMR, right? > + imr.addr_lo = phys_to_imr(base); > + imr.addr_hi = phys_to_imr(end); > + imr.rmask = rmask; > + imr.wmask = wmask; > + > + ret = imr_write(idev, reg, &imr, lock); > + > +done: So 'done:' should really be named 'fail:'. > + mutex_unlock(&idev->lock); > + return ret == 0 ? reg : ret; So we have a variable named 'ret', but it turns out to not be the return value after all? Why not just 'ret'? It's not like users of this API are supposed to know about the internals of the IMR code: in which register it was stored and such. (See my comments below about the removal API.) If this is simply an MTRR leftover then good riddance! > +} > +EXPORT_SYMBOL(imr_add_range); That should really be EXPORT_SYMBOL_GPL(). > +/** > + * imr_remove_range - delete an Isolated Memory Region. > + * > + * This function allows you to delete an IMR by its index specified by reg or > + * by address range specified by base and size respectively. If you specify an > + * index on its 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 1 KiB. > + * @size: physical size of region in bytes 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, phys_addr_t base, size_t size) I think this API should lose the MTRR-ism as well: instead of allowing this weird deletion by index and by address as well, just split it in two variants, and only allow external users to delete by address. That will simplify things and will allow future enhancements as well: like transparent merging and even reshuffling of IMRs, should the need arise. > +{ > + phys_addr_t end; > + bool found = false; > + unsigned int i; > + struct imr_device *idev = &imr_dev; > + struct imr_regs imr; > + int ret = 0; > + > + if (idev->init == false) > + return -ENODEV; > + > + if (imr_check_range(base, size)) > + return -EINVAL; > + > + if (reg >= idev->max_imr) > + return -EINVAL; > + > + /* Tweak the size value. */ > + size = imr_fixup_size(size); > + end = base + size; > + > + mutex_lock(&idev->lock); > + > + if (reg >= 0) { > + /* If a specific IMR is given try to use it. */ > + ret = imr_read(idev, reg, &imr); > + if (ret) > + goto done; > + > + if (!imr_is_enabled(&imr) || imr.addr_lo & IMR_LOCK) { > + ret = -ENODEV; > + goto done; > + } > + found = true; By my simplification suggestions above this branch would go away from the external API. > + } else { > + /* Search for match based on address range. */ > + for (i = 0; i < idev->max_imr; i++) { > + ret = imr_read(idev, i, &imr); > + if (ret) > + goto done; > + > + if (!imr_is_enabled(&imr) || imr.addr_lo & IMR_LOCK) > + continue; > + > + if ((imr_to_phys(imr.addr_lo) == base) && > + (imr_to_phys(imr.addr_hi) == end)) { > + found = true; > + reg = i; > + break; > + } > + } > + } > + > + if (found == false) { So writing this as: if (!found) { is a lot more readable, right? > + 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(idev, reg, &imr, false); So 'ret' here gets mixed with other potential failure modes. If imr_write() fails here then that's a highly anomalous internal failure - so it might be better to add a WARN_ON(ret) or so. > + > +done: s/done/failed/, or so. > + mutex_unlock(&idev->lock); > + return ret; > +} > +EXPORT_SYMBOL(imr_remove_range); Please make all new kernel internal symbol exports _GPL. > + > +/** > + * 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. > + * > + * @idev: pointer to imr_device structure. > + * @return: > + */ > +static void __init imr_fixup_memmap(struct imr_device *idev) > +{ > + phys_addr_t base = virt_to_phys(&_text); > + size_t size = virt_to_phys(&__end_rodata) - base; > + int i; > + int ret; > + > + /* Tear down all existing unlocked IMRs. */ > + for (i = 0; i < idev->max_imr; i++) > + imr_remove_range(i, 0, 0); This would use a simpler variant of imr_remove_range() which is basically just an imr_write(). It might even be written as an imr_clear() helper. > + > + /* > + * 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 as one physically contiguous block. > + */ > + 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: %zu KiB (%p - %p)\n", > + size / 1024, &_text, &__end_rodata); Curly braces around multi-line statements and all that. > --- /dev/null > +++ b/arch/x86/platform/intel-quark/imr_selftest.c > @@ -0,0 +1,135 @@ > +/** > + * imr_selftest.c > + * > + * Copyright(c) 2013 Intel Corporation. > + * Copyright(c) 2015 Bryan O'Donoghue <pure.logic@nexus-software.ie> > + * > + * IMR self test. The purpose of this module is to run a set of tests on the > + * IMR API to validate it's sanity. We check for overlapping, reserved > + * addresses and setup/teardown sanity. > + * > + */ Looks like we could shrink the kernel source code by 3 bytes there! In any case, I don't see any major problems with this code, so if it's fixed it could go into v3.20. Thanks, Ingo ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2015-01-29 7:44 ` Ingo Molnar @ 2015-01-29 10:08 ` Andy Shevchenko 2015-01-29 10:12 ` Bryan O'Donoghue 2015-01-29 13:27 ` Bryan O'Donoghue 1 sibling, 1 reply; 47+ messages in thread From: Andy Shevchenko @ 2015-01-29 10:08 UTC (permalink / raw) To: Ingo Molnar Cc: Bryan O'Donoghue, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, dvhart, Ong, Boon Leong, linux-kernel On Thu, Jan 29, 2015 at 9:44 AM, Ingo Molnar <mingo@kernel.org> wrote: > * Bryan O'Donoghue <pure.logic@nexus-software.ie> wrote: [] > In any case, I don't see any major problems with this code, so if > it's fixed it could go into v3.20. Brian, it would be really nice to have it in 3.20 since we have several drivers already in upstream or be ready for 3.20. It would be then first upstream kernel with minimal Quark support. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2015-01-29 10:08 ` Andy Shevchenko @ 2015-01-29 10:12 ` Bryan O'Donoghue 2015-01-29 13:47 ` Ong, Boon Leong 2015-01-29 15:15 ` Ong, Boon Leong 0 siblings, 2 replies; 47+ messages in thread From: Bryan O'Donoghue @ 2015-01-29 10:12 UTC (permalink / raw) To: Andy Shevchenko, Ingo Molnar Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, dvhart, Ong, Boon Leong, linux-kernel On 29/01/15 10:08, Andy Shevchenko wrote: > On Thu, Jan 29, 2015 at 9:44 AM, Ingo Molnar <mingo@kernel.org> wrote: >> * Bryan O'Donoghue <pure.logic@nexus-software.ie> wrote: > > [] > >> In any case, I don't see any major problems with this code, so if >> it's fixed it could go into v3.20. > > Brian, it would be really nice to have it in 3.20 since we have > several drivers already in upstream or be ready for 3.20. > It would be then first upstream kernel with minimal Quark support. NP lads - I'll get a patch out today ^ permalink raw reply [flat|nested] 47+ messages in thread
* RE: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2015-01-29 10:12 ` Bryan O'Donoghue @ 2015-01-29 13:47 ` Ong, Boon Leong 2015-01-29 15:22 ` Bryan O'Donoghue 2015-01-29 15:15 ` Ong, Boon Leong 1 sibling, 1 reply; 47+ messages in thread From: Ong, Boon Leong @ 2015-01-29 13:47 UTC (permalink / raw) To: Bryan O'Donoghue Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, dvhart, linux-kernel, Andy Shevchenko, Ingo Molnar Bryan, Once you have the next revision ready, I would like to test it on my end across both Galileo Gen v1 & v2. Cheers, BL >-----Original Message----- >From: Bryan O'Donoghue [mailto:pure.logic@nexus-software.ie] >Sent: Thursday, January 29, 2015 6:12 PM >To: Andy Shevchenko; Ingo Molnar >Cc: Thomas Gleixner; Ingo Molnar; H. Peter Anvin; x86@kernel.org; >dvhart@infradead.org; Ong, Boon Leong; linux-kernel@vger.kernel.org >Subject: Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 > >On 29/01/15 10:08, Andy Shevchenko wrote: >> On Thu, Jan 29, 2015 at 9:44 AM, Ingo Molnar <mingo@kernel.org> wrote: >>> * Bryan O'Donoghue <pure.logic@nexus-software.ie> wrote: >> >> [] >> >>> In any case, I don't see any major problems with this code, so if >>> it's fixed it could go into v3.20. >> >> Brian, it would be really nice to have it in 3.20 since we have >> several drivers already in upstream or be ready for 3.20. >> It would be then first upstream kernel with minimal Quark support. > >NP lads - I'll get a patch out today ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2015-01-29 13:47 ` Ong, Boon Leong @ 2015-01-29 15:22 ` Bryan O'Donoghue 2015-01-29 15:32 ` Ong, Boon Leong 0 siblings, 1 reply; 47+ messages in thread From: Bryan O'Donoghue @ 2015-01-29 15:22 UTC (permalink / raw) To: Ong, Boon Leong Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, dvhart, linux-kernel, Andy Shevchenko, Ingo Molnar On 29/01/15 13:47, Ong, Boon Leong wrote: > Bryan, > > Once you have the next revision ready, I would like to test it on my end across both Galileo Gen v1 & v2. > > Cheers, > BL Andy/BL - thanks for taking the time to test. I may end up dropping the imr_del_range() tests based on index as a result of changing the external interface as suggested by Ingo. If you guys have a clear preference to keep those tests shout now and I'll re-merge the imr_selftest code into the main chunk of IMR code. -- BOD ^ permalink raw reply [flat|nested] 47+ messages in thread
* RE: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2015-01-29 15:22 ` Bryan O'Donoghue @ 2015-01-29 15:32 ` Ong, Boon Leong 2015-01-29 15:40 ` Bryan O'Donoghue 0 siblings, 1 reply; 47+ messages in thread From: Ong, Boon Leong @ 2015-01-29 15:32 UTC (permalink / raw) To: Bryan O'Donoghue Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, dvhart, linux-kernel, Andy Shevchenko, Ingo Molnar >-----Original Message----- >From: Bryan O'Donoghue [mailto:pure.logic@nexus-software.ie] >Sent: Thursday, January 29, 2015 11:22 PM >To: Ong, Boon Leong >Cc: Thomas Gleixner; Ingo Molnar; H. Peter Anvin; x86@kernel.org; >dvhart@infradead.org; linux-kernel@vger.kernel.org; Andy Shevchenko; Ingo >Molnar >Subject: Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 > >On 29/01/15 13:47, Ong, Boon Leong wrote: >> Bryan, >> >> Once you have the next revision ready, I would like to test it on my end across >both Galileo Gen v1 & v2. >> >> Cheers, >> BL > >Andy/BL - thanks for taking the time to test. > >I may end up dropping the imr_del_range() tests based on index as a result of >changing the external interface as suggested by Ingo. It would be nice to have two variants (1) index based & (2) address based. > >If you guys have a clear preference to keep those tests shout now and I'll re- >merge the imr_selftest code into the main chunk of IMR code. Ok, once you have that ready, I will re-run the test on my end. > >-- >BOD ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2015-01-29 15:32 ` Ong, Boon Leong @ 2015-01-29 15:40 ` Bryan O'Donoghue 2015-01-29 16:12 ` Bryan O'Donoghue 0 siblings, 1 reply; 47+ messages in thread From: Bryan O'Donoghue @ 2015-01-29 15:40 UTC (permalink / raw) To: Ong, Boon Leong Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, dvhart, linux-kernel, Andy Shevchenko, Ingo Molnar On 29/01/15 15:32, Ong, Boon Leong wrote: > > >> -----Original Message----- >> From: Bryan O'Donoghue [mailto:pure.logic@nexus-software.ie] >> Sent: Thursday, January 29, 2015 11:22 PM >> To: Ong, Boon Leong >> Cc: Thomas Gleixner; Ingo Molnar; H. Peter Anvin; x86@kernel.org; >> dvhart@infradead.org; linux-kernel@vger.kernel.org; Andy Shevchenko; Ingo >> Molnar >> Subject: Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 >> >> On 29/01/15 13:47, Ong, Boon Leong wrote: >>> Bryan, >>> >>> Once you have the next revision ready, I would like to test it on my end across >> both Galileo Gen v1 & v2. >>> >>> Cheers, >>> BL >> >> Andy/BL - thanks for taking the time to test. >> >> I may end up dropping the imr_del_range() tests based on index as a result of >> changing the external interface as suggested by Ingo. > It would be nice to have two variants (1) index based & (2) address based. Understood. The direction from Ingo was to have address based external interface imr_del_range() and support an index based internal imr_clear() - internally. So - in order to get test coverage - I'll move the self-test code back into the main IMR code Not as pretty that way - but better coverage :) ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2015-01-29 15:40 ` Bryan O'Donoghue @ 2015-01-29 16:12 ` Bryan O'Donoghue 2015-01-29 16:26 ` Ong, Boon Leong 0 siblings, 1 reply; 47+ messages in thread From: Bryan O'Donoghue @ 2015-01-29 16:12 UTC (permalink / raw) To: Ong, Boon Leong Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, dvhart, linux-kernel, Andy Shevchenko, Ingo Molnar On 29/01/15 15:40, Bryan O'Donoghue wrote: >> It would be nice to have two variants (1) index based & (2) address >> based. > > Understood. The direction from Ingo was to have address based external > interface imr_del_range() and support an index based internal > imr_clear() - internally. > > So - in order to get test coverage - I'll move the self-test code back > into the main IMR code > > Not as pretty that way - but better coverage :) Talking to myself in public... Second (third) thought - there's no advantage to moving the test code back in - since imr_add_range() won't return the index anymore... ^ permalink raw reply [flat|nested] 47+ messages in thread
* RE: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2015-01-29 16:12 ` Bryan O'Donoghue @ 2015-01-29 16:26 ` Ong, Boon Leong 0 siblings, 0 replies; 47+ messages in thread From: Ong, Boon Leong @ 2015-01-29 16:26 UTC (permalink / raw) To: Bryan O'Donoghue, Ingo Molnar Cc: Thomas Gleixner, H. Peter Anvin, x86, dvhart, linux-kernel, Andy Shevchenko, Ingo Molnar >On 29/01/15 15:40, Bryan O'Donoghue wrote: >>> It would be nice to have two variants (1) index based & (2) address >>> based. >> >> Understood. The direction from Ingo was to have address based external >> interface imr_del_range() and support an index based internal >> imr_clear() - internally. >> >> So - in order to get test coverage - I'll move the self-test code back >> into the main IMR code It will be nice to have the separation of test code. However, that also means the index-based variant needs to be external function. Ingo, what is your preference? >> >> Not as pretty that way - but better coverage :) > >Talking to myself in public... > >Second (third) thought - there's no advantage to moving the test code back in - >since imr_add_range() won't return the index anymore... It does. See below from v6: + ret = imr_write(idev, reg, &imr, lock); ^^ =========== IMR ID + +done: + mutex_unlock(&idev->lock); + return ret == 0 ? reg : ret; ^^ ============ IMR ID +} +EXPORT_SYMBOL(imr_add_range); ^ permalink raw reply [flat|nested] 47+ messages in thread
* RE: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2015-01-29 10:12 ` Bryan O'Donoghue 2015-01-29 13:47 ` Ong, Boon Leong @ 2015-01-29 15:15 ` Ong, Boon Leong 1 sibling, 0 replies; 47+ messages in thread From: Ong, Boon Leong @ 2015-01-29 15:15 UTC (permalink / raw) To: Bryan O'Donoghue Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, dvhart, linux-kernel, Andy Shevchenko, Ingo Molnar Hi Bryan, These two patches requires another patch "x86: Re-enable IO-APIC for non-SMP X86_32" to be included to make sure we have the right build for Quark. I tested v6 on Galileo Gen v2 just now. I am happy with it. Many thanks. Below are the logs: root@quark:/proc# dmesg | grep imr [ 3.727884] imr: protecting kernel .text - .rodata: 9240 KiB (c1000000 - c1906000) [ 3.736421] imr_selftest: self_test pass zero sized IMR [ 3.742413] imr_selftest: self_test pass overlapped IMR @ (0xc1000000 - 0xc1906000) [ 3.751164] imr_selftest: self_test pass overlapped IMR @ (0xc1905c00 - 0xc220bc00) [ 3.759916] imr_selftest: self_test pass overlapped IMR @ (0xc0fff400 - 0xc1905400) [ 3.768566] imr_selftest: self_test pass 1KiB IMR @ 0x00000000 - access-all [ 3.776542] imr_selftest: self_test pass 1KiB IMR @ 0x00000000 - cpu access [ 3.784442] imr_selftest: self_test pass index-teardown - cpu access [ 3.791690] imr_selftest: self_test pass 2KiB IMR @ 0x00000000 [ 3.798396] imr_selftest: self_test pass index-teardown 2KiB [ 3.804957] imr_selftest: self_test pass 2KiB IMR @ 0x00000000 [ 3.811616] imr_selftest: self_test pass addr-teardown 2KiB root@quark:/proc# cat /sys/kernel/debug/imr_state imr00: base=0x00000000, end=0x00000000, size=0x00000000 rmask=0xbfffffff, wmask=0xffffffff, disabled, unlocked imr01: base=0x00000000, end=0x00000000, size=0x00000000 rmask=0xbfffffff, wmask=0xffffffff, disabled, unlocked imr02: base=0x00000000, end=0x00000000, size=0x00000000 rmask=0xbfffffff, wmask=0xffffffff, disabled, unlocked imr03: base=0x00000000, end=0x00000000, size=0x00000000 rmask=0xbfffffff, wmask=0xffffffff, disabled, unlocked imr04: base=0x00000000, end=0x00000000, size=0x00000000 rmask=0xbfffffff, wmask=0xffffffff, disabled, unlocked imr05: base=0x00000000, end=0x00000000, size=0x00000000 rmask=0xbfffffff, wmask=0xffffffff, disabled, unlocked imr06: base=0x00000000, end=0x00000000, size=0x00000000 rmask=0xbfffffff, wmask=0xffffffff, disabled, unlocked imr07: base=0x01000000, end=0x01905fff, size=0x00905fff rmask=0x00000001, wmask=0x00000001, enabled , locked >-----Original Message----- >From: Ong, Boon Leong >Sent: Thursday, January 29, 2015 9:47 PM >To: 'Bryan O'Donoghue' >Cc: Thomas Gleixner; Ingo Molnar; H. Peter Anvin; x86@kernel.org; >dvhart@infradead.org; linux-kernel@vger.kernel.org; Andy Shevchenko; Ingo >Molnar >Subject: RE: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 > >Bryan, > >Once you have the next revision ready, I would like to test it on my end across >both Galileo Gen v1 & v2. > >Cheers, >BL > >>-----Original Message----- >>From: Bryan O'Donoghue [mailto:pure.logic@nexus-software.ie] >>Sent: Thursday, January 29, 2015 6:12 PM >>To: Andy Shevchenko; Ingo Molnar >>Cc: Thomas Gleixner; Ingo Molnar; H. Peter Anvin; x86@kernel.org; >>dvhart@infradead.org; Ong, Boon Leong; linux-kernel@vger.kernel.org >>Subject: Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark >>X1000 >> >>On 29/01/15 10:08, Andy Shevchenko wrote: >>> On Thu, Jan 29, 2015 at 9:44 AM, Ingo Molnar <mingo@kernel.org> wrote: >>>> * Bryan O'Donoghue <pure.logic@nexus-software.ie> wrote: >>> >>> [] >>> >>>> In any case, I don't see any major problems with this code, so if >>>> it's fixed it could go into v3.20. >>> >>> Brian, it would be really nice to have it in 3.20 since we have >>> several drivers already in upstream or be ready for 3.20. >>> It would be then first upstream kernel with minimal Quark support. >> >>NP lads - I'll get a patch out today ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2015-01-29 7:44 ` Ingo Molnar 2015-01-29 10:08 ` Andy Shevchenko @ 2015-01-29 13:27 ` Bryan O'Donoghue 1 sibling, 0 replies; 47+ messages in thread From: Bryan O'Donoghue @ 2015-01-29 13:27 UTC (permalink / raw) To: Ingo Molnar Cc: tglx, mingo, hpa, x86, dvhart, andy.shevchenko, boon.leong.ong, linux-kernel On 29/01/15 07:44, Ingo Molnar wrote: Hi Ingo. I'll transmit those changes in with the following exception >> + ret = imr_write(idev, reg, &imr, false); > > So 'ret' here gets mixed with other potential failure modes. > > If imr_write() fails here then that's a highly anomalous internal > failure - so it might be better to add a WARN_ON(ret) or so. > imr_write() already does a WARN - so no need to do it again. ^ permalink raw reply [flat|nested] 47+ messages in thread
* RE: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 2015-01-28 18:36 ` [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 Bryan O'Donoghue 2015-01-29 5:38 ` Darren Hart 2015-01-29 7:44 ` Ingo Molnar @ 2015-01-29 9:59 ` Ong, Boon Leong 2 siblings, 0 replies; 47+ messages in thread From: Ong, Boon Leong @ 2015-01-29 9:59 UTC (permalink / raw) To: Bryan O'Donoghue Cc: tglx, mingo, hpa, x86, dvhart, andy.shevchenko, linux-kernel Bryan, a minor fix on the config below... >diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug index >61bd2ad..fcf5701 100644 >--- a/arch/x86/Kconfig.debug >+++ b/arch/x86/Kconfig.debug >@@ -313,6 +313,18 @@ config DEBUG_NMI_SELFTEST > > If unsure, say N. > >+config DEBUG_IMR_SELFTEST >+ bool "Isolated Memory Region self test" >+ default n >+ depends on INTEL_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 here. Miss a newline here. > config X86_DEBUG_STATIC_CPU_HAS > bool "Debug alternatives" > depends on DEBUG_KERNEL ^ permalink raw reply [flat|nested] 47+ messages in thread
end of thread, other threads:[~2015-01-29 16:28 UTC | newest] Thread overview: 47+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-12-29 17:23 [PATCH 0/2] x86: Add IMR support to Quark/Galileo Bryan O'Donoghue 2014-12-29 17:23 ` [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 Bryan O'Donoghue 2014-12-31 15:05 ` Andy Shevchenko 2015-01-01 20:11 ` Bryan O'Donoghue 2015-01-06 7:36 ` Darren Hart 2015-01-06 13:43 ` Bryan O'Donoghue 2015-01-06 16:54 ` Darren Hart 2015-01-07 23:45 ` Ong, Boon Leong 2015-01-08 12:10 ` Bryan O'Donoghue 2015-01-08 14:52 ` Ong, Boon Leong 2015-01-08 0:04 ` Ong, Boon Leong 2015-01-08 13:08 ` Bryan O'Donoghue 2015-01-08 14:45 ` Ong, Boon Leong 2015-01-08 15:11 ` Bryan O'Donoghue 2015-01-09 3:44 ` Darren Hart 2014-12-29 17:23 ` [PATCH 2/2] platform/x86: Add Intel Galileo platform specific setup Bryan O'Donoghue 2014-12-31 15:25 ` Andy Shevchenko 2015-01-09 1:00 ` Ong, Boon Leong 2015-01-09 2:11 ` Bryan O'Donoghue 2015-01-09 4:46 ` Darren Hart 2015-01-09 11:17 ` Bryan O'Donoghue 2015-01-09 11:29 ` Bryan O'Donoghue 2015-01-09 14:11 ` Ong, Boon Leong 2015-01-10 6:54 ` Darren Hart 2015-01-11 1:53 ` Bryan O'Donoghue 2014-12-31 10:12 ` [PATCH 0/2] x86: Add IMR support to Quark/Galileo Andy Shevchenko 2014-12-31 11:59 ` Bryan O'Donoghue 2015-01-02 2:02 ` Darren Hart 2015-01-02 4:24 ` Darren Hart 2015-01-06 6:00 ` Darren Hart 2015-01-06 13:56 ` Bryan O'Donoghue 2015-01-06 16:48 ` Darren Hart 2015-01-06 17:23 ` Bryan O'Donoghue 2015-01-28 18:36 [PATCH v6 " Bryan O'Donoghue 2015-01-28 18:36 ` [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000 Bryan O'Donoghue 2015-01-29 5:38 ` Darren Hart 2015-01-29 7:44 ` Ingo Molnar 2015-01-29 10:08 ` Andy Shevchenko 2015-01-29 10:12 ` Bryan O'Donoghue 2015-01-29 13:47 ` Ong, Boon Leong 2015-01-29 15:22 ` Bryan O'Donoghue 2015-01-29 15:32 ` Ong, Boon Leong 2015-01-29 15:40 ` Bryan O'Donoghue 2015-01-29 16:12 ` Bryan O'Donoghue 2015-01-29 16:26 ` Ong, Boon Leong 2015-01-29 15:15 ` Ong, Boon Leong 2015-01-29 13:27 ` Bryan O'Donoghue 2015-01-29 9:59 ` Ong, Boon Leong
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).