>-----Original Message-----
>From: linux-kernel-ow...@vger.kernel.org [mailto:linux-kernel-
>ow...@vger.kernel.org] On Behalf Of Bryan O'Donoghue
>Sent: Monday, December 29, 2014 9:23 AM
>To: t...@linutronix.de; mi...@redhat.com; h...@zytor.com; x...@kernel.org;
>dvh...@infradead.org; platform-driver-...@vger.kernel.org; linux-
>ker...@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.lo...@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.lo...@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);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
