Date:	Thu, 8 Jan 2015 00:04:33 +0000
From:	"Ong, Boon Leong" <boon.leong.ong@...el.com>
To:	Bryan O'Donoghue <pure.logic@...us-software.ie>,
	"tglx@...utronix.de" <tglx@...utronix.de>,
	"mingo@...hat.com" <mingo@...hat.com>,
	"hpa@...or.com" <hpa@...or.com>, "x86@...nel.org" <x86@...nel.org>,
	"dvhart@...radead.org" <dvhart@...radead.org>,
	"platform-driver-x86@...r.kernel.org" 
	<platform-driver-x86@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000

>-----Original Message-----
>From: linux-kernel-owner@...r.kernel.org [mailto:linux-kernel-
>owner@...r.kernel.org] On Behalf Of Bryan O'Donoghue
>Sent: Monday, December 29, 2014 9:23 AM
>To: tglx@...utronix.de; mingo@...hat.com; hpa@...or.com; x86@...nel.org;
>dvhart@...radead.org; platform-driver-x86@...r.kernel.org; linux-
>kernel@...r.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@...us-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@...us-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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists