* 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; 44+ 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] 44+ 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; 44+ 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] 44+ 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; 44+ 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] 44+ 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; 44+ 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] 44+ 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; 44+ 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] 44+ 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-07 23:45 ` Ong, Boon Leong
2015-01-07 23:45 ` Ong, Boon Leong
1 sibling, 0 replies; 44+ 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] 44+ messages in thread* RE: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000
@ 2015-01-07 23:45 ` Ong, Boon Leong
0 siblings, 0 replies; 44+ 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] 44+ 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
-1 siblings, 0 replies; 44+ 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] 44+ messages in thread
* Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000
@ 2015-01-08 12:10 ` Bryan O'Donoghue
0 siblings, 0 replies; 44+ 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] 44+ 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
-1 siblings, 0 replies; 44+ 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] 44+ messages in thread
* RE: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000
@ 2015-01-08 14:52 ` Ong, Boon Leong
0 siblings, 0 replies; 44+ 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] 44+ 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
@ 2015-01-08 0:04 ` Ong, Boon Leong
2015-01-06 7:36 ` Darren Hart
2015-01-08 0:04 ` Ong, Boon Leong
2 siblings, 0 replies; 44+ 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] 44+ messages in thread* RE: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000
@ 2015-01-08 0:04 ` Ong, Boon Leong
0 siblings, 0 replies; 44+ 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] 44+ 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
-1 siblings, 0 replies; 44+ 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] 44+ messages in thread* Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000
@ 2015-01-08 13:08 ` Bryan O'Donoghue
0 siblings, 0 replies; 44+ 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] 44+ 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
-1 siblings, 0 replies; 44+ 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] 44+ messages in thread
* RE: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000
@ 2015-01-08 14:45 ` Ong, Boon Leong
0 siblings, 0 replies; 44+ 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] 44+ 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
-1 siblings, 0 replies; 44+ 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] 44+ messages in thread* Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000
@ 2015-01-08 15:11 ` Bryan O'Donoghue
0 siblings, 0 replies; 44+ 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] 44+ 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
-1 siblings, 0 replies; 44+ 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] 44+ messages in thread* Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000
@ 2015-01-09 3:44 ` Darren Hart
0 siblings, 0 replies; 44+ 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] 44+ messages in thread