* [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ
@ 2012-11-13 16:08 Etienne CARRIERE
2012-11-13 17:07 ` Russell King - ARM Linux
2012-11-13 17:17 ` Dave Martin
0 siblings, 2 replies; 17+ messages in thread
From: Etienne CARRIERE @ 2012-11-13 16:08 UTC (permalink / raw)
To: linux-arm-kernel
From: Etienne Carriere <etienne.carriere@stericsson.com>
Secure code in TrustZone space may need to perform L2 cache
maintenance operations. A shared mutex is required to synchronize
linux l2cc maintenance and TZ l2cc maintenance.
The TZ mutex is an "arch_spinlock": a 32bit DDR cell (ARMv7-A mutex).
Linux L2 cache driver must lock TZ mutex if enabled.
Signed-off-by: Etienne Carriere <etienne.carriere@stericsson.com>
---
arch/arm/include/asm/outercache.h | 9 ++++
arch/arm/mm/cache-l2x0.c | 87 +++++++++++++++++++++++++++++----------
2 files changed, 74 insertions(+), 22 deletions(-)
diff --git a/arch/arm/include/asm/outercache.h b/arch/arm/include/asm/outercache.h
index 53426c6..7aa5eac 100644
--- a/arch/arm/include/asm/outercache.h
+++ b/arch/arm/include/asm/outercache.h
@@ -35,6 +35,7 @@ struct outer_cache_fns {
#endif
void (*set_debug)(unsigned long);
void (*resume)(void);
+ bool (*tz_mutex)(unsigned long);
};
#ifdef CONFIG_OUTER_CACHE
@@ -81,6 +82,13 @@ static inline void outer_resume(void)
outer_cache.resume();
}
+static inline bool outer_tz_mutex(unsigned long addr)
+{
+ if (outer_cache.tz_mutex)
+ return outer_cache.tz_mutex(addr);
+ return false;
+}
+
#else
static inline void outer_inv_range(phys_addr_t start, phys_addr_t end)
@@ -92,6 +100,7 @@ static inline void outer_flush_range(phys_addr_t start, phys_addr_t end)
static inline void outer_flush_all(void) { }
static inline void outer_inv_all(void) { }
static inline void outer_disable(void) { }
+static inline bool outer_tz_mutex(unsigned long addr) { return false; }
#endif
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index a53fd2a..eacdc74 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -41,6 +41,26 @@ struct l2x0_of_data {
void (*resume)(void);
};
+/*
+ * arch_spinlock (single 32bit DDR mutex cell) pointer to synchronise
+ * L2CC maintenance between linux world and secure world (ARM TZ).
+ */
+arch_spinlock_t *l2x0_tz_mutex;
+
+#define l2x0_spin_lock_irqsave(flags) \
+ do { \
+ raw_spin_lock_irqsave(&l2x0_lock, flags); \
+ if (l2x0_tz_mutex) \
+ arch_spin_lock(l2x0_tz_mutex); \
+ } while (0)
+
+#define l2x0_spin_unlock_irqrestore(flags) \
+ do { \
+ if (l2x0_tz_mutex) \
+ arch_spin_unlock(l2x0_tz_mutex); \
+ raw_spin_unlock_irqrestore(&l2x0_lock, flags); \
+ } while (0)
+
static inline void cache_wait_way(void __iomem *reg, unsigned long mask)
{
/* wait for cache operation by line or way to complete */
@@ -126,9 +146,9 @@ static void l2x0_cache_sync(void)
{
unsigned long flags;
- raw_spin_lock_irqsave(&l2x0_lock, flags);
+ l2x0_spin_lock_irqsave(flags);
cache_sync();
- raw_spin_unlock_irqrestore(&l2x0_lock, flags);
+ l2x0_spin_unlock_irqrestore(flags);
}
static void __l2x0_flush_all(void)
@@ -145,9 +165,9 @@ static void l2x0_flush_all(void)
unsigned long flags;
/* clean all ways */
- raw_spin_lock_irqsave(&l2x0_lock, flags);
+ l2x0_spin_lock_irqsave(flags);
__l2x0_flush_all();
- raw_spin_unlock_irqrestore(&l2x0_lock, flags);
+ l2x0_spin_unlock_irqrestore(flags);
}
static void l2x0_clean_all(void)
@@ -155,11 +175,11 @@ static void l2x0_clean_all(void)
unsigned long flags;
/* clean all ways */
- raw_spin_lock_irqsave(&l2x0_lock, flags);
+ l2x0_spin_lock_irqsave(flags);
writel_relaxed(l2x0_way_mask, l2x0_base + L2X0_CLEAN_WAY);
cache_wait_way(l2x0_base + L2X0_CLEAN_WAY, l2x0_way_mask);
cache_sync();
- raw_spin_unlock_irqrestore(&l2x0_lock, flags);
+ l2x0_spin_unlock_irqrestore(flags);
}
static void l2x0_inv_all(void)
@@ -167,13 +187,13 @@ static void l2x0_inv_all(void)
unsigned long flags;
/* invalidate all ways */
- raw_spin_lock_irqsave(&l2x0_lock, flags);
+ l2x0_spin_lock_irqsave(flags);
/* Invalidating when L2 is enabled is a nono */
BUG_ON(readl(l2x0_base + L2X0_CTRL) & 1);
writel_relaxed(l2x0_way_mask, l2x0_base + L2X0_INV_WAY);
cache_wait_way(l2x0_base + L2X0_INV_WAY, l2x0_way_mask);
cache_sync();
- raw_spin_unlock_irqrestore(&l2x0_lock, flags);
+ l2x0_spin_unlock_irqrestore(flags);
}
static void l2x0_inv_range(unsigned long start, unsigned long end)
@@ -181,7 +201,7 @@ static void l2x0_inv_range(unsigned long start, unsigned long end)
void __iomem *base = l2x0_base;
unsigned long flags;
- raw_spin_lock_irqsave(&l2x0_lock, flags);
+ l2x0_spin_lock_irqsave(flags);
if (start & (CACHE_LINE_SIZE - 1)) {
start &= ~(CACHE_LINE_SIZE - 1);
debug_writel(0x03);
@@ -206,13 +226,13 @@ static void l2x0_inv_range(unsigned long start, unsigned long end)
}
if (blk_end < end) {
- raw_spin_unlock_irqrestore(&l2x0_lock, flags);
- raw_spin_lock_irqsave(&l2x0_lock, flags);
+ l2x0_spin_unlock_irqrestore(flags);
+ l2x0_spin_lock_irqsave(flags);
}
}
cache_wait(base + L2X0_INV_LINE_PA, 1);
cache_sync();
- raw_spin_unlock_irqrestore(&l2x0_lock, flags);
+ l2x0_spin_unlock_irqrestore(flags);
}
static void l2x0_clean_range(unsigned long start, unsigned long end)
@@ -225,7 +245,7 @@ static void l2x0_clean_range(unsigned long start, unsigned long end)
return;
}
- raw_spin_lock_irqsave(&l2x0_lock, flags);
+ l2x0_spin_lock_irqsave(flags);
start &= ~(CACHE_LINE_SIZE - 1);
while (start < end) {
unsigned long blk_end = start + min(end - start, 4096UL);
@@ -236,13 +256,13 @@ static void l2x0_clean_range(unsigned long start, unsigned long end)
}
if (blk_end < end) {
- raw_spin_unlock_irqrestore(&l2x0_lock, flags);
- raw_spin_lock_irqsave(&l2x0_lock, flags);
+ l2x0_spin_unlock_irqrestore(flags);
+ l2x0_spin_lock_irqsave(flags);
}
}
cache_wait(base + L2X0_CLEAN_LINE_PA, 1);
cache_sync();
- raw_spin_unlock_irqrestore(&l2x0_lock, flags);
+ l2x0_spin_unlock_irqrestore(flags);
}
static void l2x0_flush_range(unsigned long start, unsigned long end)
@@ -255,7 +275,7 @@ static void l2x0_flush_range(unsigned long start, unsigned long end)
return;
}
- raw_spin_lock_irqsave(&l2x0_lock, flags);
+ l2x0_spin_lock_irqsave(flags);
start &= ~(CACHE_LINE_SIZE - 1);
while (start < end) {
unsigned long blk_end = start + min(end - start, 4096UL);
@@ -268,24 +288,24 @@ static void l2x0_flush_range(unsigned long start, unsigned long end)
debug_writel(0x00);
if (blk_end < end) {
- raw_spin_unlock_irqrestore(&l2x0_lock, flags);
- raw_spin_lock_irqsave(&l2x0_lock, flags);
+ l2x0_spin_unlock_irqrestore(flags);
+ l2x0_spin_lock_irqsave(flags);
}
}
cache_wait(base + L2X0_CLEAN_INV_LINE_PA, 1);
cache_sync();
- raw_spin_unlock_irqrestore(&l2x0_lock, flags);
+ l2x0_spin_unlock_irqrestore(flags);
}
static void l2x0_disable(void)
{
unsigned long flags;
- raw_spin_lock_irqsave(&l2x0_lock, flags);
+ l2x0_spin_lock_irqsave(flags);
__l2x0_flush_all();
writel_relaxed(0, l2x0_base + L2X0_CTRL);
dsb();
- raw_spin_unlock_irqrestore(&l2x0_lock, flags);
+ l2x0_spin_unlock_irqrestore(flags);
}
static void l2x0_unlock(u32 cache_id)
@@ -307,6 +327,28 @@ static void l2x0_unlock(u32 cache_id)
}
}
+/* Enable/disable external mutex shared with TZ code */
+static bool l2x0_tz_mutex_cfg(unsigned long addr)
+{
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&l2x0_lock, flags);
+
+ if (addr && l2x0_tz_mutex && (addr != (uint)l2x0_tz_mutex)) {
+ raw_spin_unlock_irqrestore(&l2x0_lock, flags);
+ pr_err("%s: a TZ mutex is already enabled\n", __func__);
+ return false;
+ }
+
+ l2x0_tz_mutex = (arch_spinlock_t *)addr;
+ /* insure mutex ptr is updated before lock is released */
+ smp_wmb();
+
+ raw_spin_unlock_irqrestore(&l2x0_lock, flags);
+ pr_debug("\n%s: %sable TZ mutex\n\n", __func__, (addr) ? "en" : "dis");
+ return true;
+}
+
void __init l2x0_init(void __iomem *base, u32 aux_val, u32 aux_mask)
{
u32 aux;
@@ -380,6 +422,7 @@ void __init l2x0_init(void __iomem *base, u32 aux_val, u32 aux_mask)
outer_cache.inv_all = l2x0_inv_all;
outer_cache.disable = l2x0_disable;
outer_cache.set_debug = l2x0_set_debug;
+ outer_cache.tz_mutex = l2x0_tz_mutex_cfg;
printk(KERN_INFO "%s cache controller enabled\n", type);
printk(KERN_INFO "l2x0: %d ways, CACHE_ID 0x%08x, AUX_CTRL 0x%08x, Cache size: %d B\n",
--
1.7.11.3
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121113/b0f6414c/attachment-0001.html>
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ
2012-11-13 16:08 [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ Etienne CARRIERE
@ 2012-11-13 17:07 ` Russell King - ARM Linux
2012-11-14 9:18 ` Etienne CARRIERE ST
2012-11-13 17:17 ` Dave Martin
1 sibling, 1 reply; 17+ messages in thread
From: Russell King - ARM Linux @ 2012-11-13 17:07 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Nov 13, 2012 at 05:08:14PM +0100, Etienne CARRIERE wrote:
> From: Etienne Carriere <etienne.carriere@stericsson.com>
>
> Secure code in TrustZone space may need to perform L2 cache
> maintenance operations. A shared mutex is required to synchronize
> linux l2cc maintenance and TZ l2cc maintenance.
>
> The TZ mutex is an "arch_spinlock": a 32bit DDR cell (ARMv7-A mutex).
> Linux L2 cache driver must lock TZ mutex if enabled.
>...
> +#define l2x0_spin_lock_irqsave(flags) \
> + do { \
> + raw_spin_lock_irqsave(&l2x0_lock, flags); \
> + if (l2x0_tz_mutex) \
> + arch_spin_lock(l2x0_tz_mutex); \
> + } while (0)
Right, so, what this tells me is that the implementation of the spinlock
in the secure software is potentially the same as the kernel's spinlock.
The kernel's spinlock implementation is GPL'd. If the secure side spinlock
implementation is a copy of the kernel's spinlock implementation, then that
implementation is also GPL'd, which, as it is linked with the rest of the
secure software, either the whole of the secure software suite is GPL'd
_or_ it is in violation of the GPL.
I think someone has some explaining to do.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ
2012-11-13 16:08 [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ Etienne CARRIERE
2012-11-13 17:07 ` Russell King - ARM Linux
@ 2012-11-13 17:17 ` Dave Martin
2012-11-13 19:22 ` Abhimanyu Kapur
2012-11-14 10:13 ` Etienne CARRIERE ST
1 sibling, 2 replies; 17+ messages in thread
From: Dave Martin @ 2012-11-13 17:17 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Nov 13, 2012 at 04:08:14PM +0000, Etienne CARRIERE wrote:
> From: Etienne Carriere <etienne.carriere@stericsson.com>
>
>
>
> Secure code in TrustZone space may need to perform L2 cache
>
> maintenance operations. A shared mutex is required to synchronize
>
> linux l2cc maintenance and TZ l2cc maintenance.
1) Why is this not a denial-of-service risk? (i.e., why can the Normal
World not simply hold the mutex forever?)
2) The memory types on both sides have to be _exactly_ the same, not
just "cached", otherwise unpredictable behaviour may occur when accessing
the mutex. It is not obvious how this is ensured.
3) Many people seem to delegate L2 cache maintenence requests via SMC
in these situations. Can you do that instead?
Cheers
---Dave
> The TZ mutex is an "arch_spinlock": a 32bit DDR cell (ARMv7-A mutex).
>
> Linux L2 cache driver must lock TZ mutex if enabled.
>
>
>
> Signed-off-by: Etienne Carriere <etienne.carriere@stericsson.com>
>
> ---
>
> arch/arm/include/asm/outercache.h | 9 ++++
>
> arch/arm/mm/cache-l2x0.c | 87 +++++++++++++++++++++++++++++----------
>
> 2 files changed, 74 insertions(+), 22 deletions(-)
>
>
>
> diff --git a/arch/arm/include/asm/outercache.h b/arch/arm/include/asm/
> outercache.h
>
> index 53426c6..7aa5eac 100644
>
> --- a/arch/arm/include/asm/outercache.h
>
> +++ b/arch/arm/include/asm/outercache.h
>
> @@ -35,6 +35,7 @@ struct outer_cache_fns {
>
> #endif
>
> void (*set_debug)(unsigned long);
>
> void (*resume)(void);
>
> + bool (*tz_mutex)(unsigned long);
>
> };
>
> #ifdef CONFIG_OUTER_CACHE
>
> @@ -81,6 +82,13 @@ static inline void outer_resume(void)
>
> outer_cache.resume();
>
> }
>
> +static inline bool outer_tz_mutex(unsigned long addr)
>
> +{
>
> + if (outer_cache.tz_mutex)
>
> + return outer_cache.tz_mutex(addr);
>
> + return false;
>
> +}
>
> +
>
> #else
>
> static inline void outer_inv_range(phys_addr_t start, phys_addr_t end)
>
> @@ -92,6 +100,7 @@ static inline void outer_flush_range(phys_addr_t start,
> phys_addr_t end)
>
> static inline void outer_flush_all(void) { }
>
> static inline void outer_inv_all(void) { }
>
> static inline void outer_disable(void) { }
>
> +static inline bool outer_tz_mutex(unsigned long addr) { return false; }
>
> #endif
>
> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
>
> index a53fd2a..eacdc74 100644
>
> --- a/arch/arm/mm/cache-l2x0.c
>
> +++ b/arch/arm/mm/cache-l2x0.c
>
> @@ -41,6 +41,26 @@ struct l2x0_of_data {
>
> void (*resume)(void);
>
> };
>
> +/*
>
> + * arch_spinlock (single 32bit DDR mutex cell) pointer to synchronise
>
> + * L2CC maintenance between linux world and secure world (ARM TZ).
>
> + */
>
> +arch_spinlock_t *l2x0_tz_mutex;
>
> +
>
> +#define l2x0_spin_lock_irqsave(flags) \
>
> + do
> {
> \
>
> + raw_spin_lock_irqsave(&l2x0_lock, flags);
> \
>
> + if (l2x0_tz_mutex)
> \
>
> + arch_spin_lock(l2x0_tz_mutex);
> \
>
> + } while (0)
>
> +
>
> +#define l2x0_spin_unlock_irqrestore(flags) \
>
> + do
> {
> \
>
> + if (l2x0_tz_mutex)
> \
>
> + arch_spin_unlock(l2x0_tz_mutex);
> \
>
> + raw_spin_unlock_irqrestore(&l2x0_lock, flags);
> \
>
> + } while (0)
>
> +
>
> static inline void cache_wait_way(void __iomem *reg, unsigned long mask)
>
> {
>
> /* wait for cache operation by line or way to complete */
>
> @@ -126,9 +146,9 @@ static void l2x0_cache_sync(void)
>
> {
>
> unsigned long flags;
>
> - raw_spin_lock_irqsave(&l2x0_lock, flags);
>
> + l2x0_spin_lock_irqsave(flags);
>
> cache_sync();
>
> - raw_spin_unlock_irqrestore(&l2x0_lock, flags);
>
> + l2x0_spin_unlock_irqrestore(flags);
>
> }
>
> static void __l2x0_flush_all(void)
>
> @@ -145,9 +165,9 @@ static void l2x0_flush_all(void)
>
> unsigned long flags;
>
> /* clean all ways */
>
> - raw_spin_lock_irqsave(&l2x0_lock, flags);
>
> + l2x0_spin_lock_irqsave(flags);
>
> __l2x0_flush_all();
>
> - raw_spin_unlock_irqrestore(&l2x0_lock, flags);
>
> + l2x0_spin_unlock_irqrestore(flags);
>
> }
>
> static void l2x0_clean_all(void)
>
> @@ -155,11 +175,11 @@ static void l2x0_clean_all(void)
>
> unsigned long flags;
>
> /* clean all ways */
>
> - raw_spin_lock_irqsave(&l2x0_lock, flags);
>
> + l2x0_spin_lock_irqsave(flags);
>
> writel_relaxed(l2x0_way_mask, l2x0_base + L2X0_CLEAN_WAY);
>
> cache_wait_way(l2x0_base + L2X0_CLEAN_WAY, l2x0_way_mask);
>
> cache_sync();
>
> - raw_spin_unlock_irqrestore(&l2x0_lock, flags);
>
> + l2x0_spin_unlock_irqrestore(flags);
>
> }
>
> static void l2x0_inv_all(void)
>
> @@ -167,13 +187,13 @@ static void l2x0_inv_all(void)
>
> unsigned long flags;
>
> /* invalidate all ways */
>
> - raw_spin_lock_irqsave(&l2x0_lock, flags);
>
> + l2x0_spin_lock_irqsave(flags);
>
> /* Invalidating when L2 is enabled is a nono */
>
> BUG_ON(readl(l2x0_base + L2X0_CTRL) & 1);
>
> writel_relaxed(l2x0_way_mask, l2x0_base + L2X0_INV_WAY);
>
> cache_wait_way(l2x0_base + L2X0_INV_WAY, l2x0_way_mask);
>
> cache_sync();
>
> - raw_spin_unlock_irqrestore(&l2x0_lock, flags);
>
> + l2x0_spin_unlock_irqrestore(flags);
>
> }
>
> static void l2x0_inv_range(unsigned long start, unsigned long end)
>
> @@ -181,7 +201,7 @@ static void l2x0_inv_range(unsigned long start, unsigned
> long end)
>
> void __iomem *base = l2x0_base;
>
> unsigned long flags;
>
> - raw_spin_lock_irqsave(&l2x0_lock, flags);
>
> + l2x0_spin_lock_irqsave(flags);
>
> if (start & (CACHE_LINE_SIZE - 1)) {
>
> start &= ~(CACHE_LINE_SIZE - 1);
>
> debug_writel(0x03);
>
> @@ -206,13 +226,13 @@ static void l2x0_inv_range(unsigned long start, unsigned
> long end)
>
> }
>
> if (blk_end < end) {
>
> - raw_spin_unlock_irqrestore(&
> l2x0_lock, flags);
>
> - raw_spin_lock_irqsave(&
> l2x0_lock, flags);
>
> + l2x0_spin_unlock_irqrestore
> (flags);
>
> + l2x0_spin_lock_irqsave(flags);
>
> }
>
> }
>
> cache_wait(base + L2X0_INV_LINE_PA, 1);
>
> cache_sync();
>
> - raw_spin_unlock_irqrestore(&l2x0_lock, flags);
>
> + l2x0_spin_unlock_irqrestore(flags);
>
> }
>
> static void l2x0_clean_range(unsigned long start, unsigned long end)
>
> @@ -225,7 +245,7 @@ static void l2x0_clean_range(unsigned long start, unsigned
> long end)
>
> return;
>
> }
>
> - raw_spin_lock_irqsave(&l2x0_lock, flags);
>
> + l2x0_spin_lock_irqsave(flags);
>
> start &= ~(CACHE_LINE_SIZE - 1);
>
> while (start < end) {
>
> unsigned long blk_end = start + min(end - start,
> 4096UL);
>
> @@ -236,13 +256,13 @@ static void l2x0_clean_range(unsigned long start,
> unsigned long end)
>
> }
>
> if (blk_end < end) {
>
> - raw_spin_unlock_irqrestore(&
> l2x0_lock, flags);
>
> - raw_spin_lock_irqsave(&
> l2x0_lock, flags);
>
> + l2x0_spin_unlock_irqrestore
> (flags);
>
> + l2x0_spin_lock_irqsave(flags);
>
> }
>
> }
>
> cache_wait(base + L2X0_CLEAN_LINE_PA, 1);
>
> cache_sync();
>
> - raw_spin_unlock_irqrestore(&l2x0_lock, flags);
>
> + l2x0_spin_unlock_irqrestore(flags);
>
> }
>
> static void l2x0_flush_range(unsigned long start, unsigned long end)
>
> @@ -255,7 +275,7 @@ static void l2x0_flush_range(unsigned long start, unsigned
> long end)
>
> return;
>
> }
>
> - raw_spin_lock_irqsave(&l2x0_lock, flags);
>
> + l2x0_spin_lock_irqsave(flags);
>
> start &= ~(CACHE_LINE_SIZE - 1);
>
> while (start < end) {
>
> unsigned long blk_end = start + min(end - start,
> 4096UL);
>
> @@ -268,24 +288,24 @@ static void l2x0_flush_range(unsigned long start,
> unsigned long end)
>
> debug_writel(0x00);
>
> if (blk_end < end) {
>
> - raw_spin_unlock_irqrestore(&
> l2x0_lock, flags);
>
> - raw_spin_lock_irqsave(&
> l2x0_lock, flags);
>
> + l2x0_spin_unlock_irqrestore
> (flags);
>
> + l2x0_spin_lock_irqsave(flags);
>
> }
>
> }
>
> cache_wait(base + L2X0_CLEAN_INV_LINE_PA, 1);
>
> cache_sync();
>
> - raw_spin_unlock_irqrestore(&l2x0_lock, flags);
>
> + l2x0_spin_unlock_irqrestore(flags);
>
> }
>
> static void l2x0_disable(void)
>
> {
>
> unsigned long flags;
>
> - raw_spin_lock_irqsave(&l2x0_lock, flags);
>
> + l2x0_spin_lock_irqsave(flags);
>
> __l2x0_flush_all();
>
> writel_relaxed(0, l2x0_base + L2X0_CTRL);
>
> dsb();
>
> - raw_spin_unlock_irqrestore(&l2x0_lock, flags);
>
> + l2x0_spin_unlock_irqrestore(flags);
>
> }
>
> static void l2x0_unlock(u32 cache_id)
>
> @@ -307,6 +327,28 @@ static void l2x0_unlock(u32 cache_id)
>
> }
>
> }
>
> +/* Enable/disable external mutex shared with TZ code */
>
> +static bool l2x0_tz_mutex_cfg(unsigned long addr)
>
> +{
>
> + unsigned long flags;
>
> +
>
> + raw_spin_lock_irqsave(&l2x0_lock, flags);
>
> +
>
> + if (addr && l2x0_tz_mutex && (addr != (uint)l2x0_tz_mutex)) {
>
> + raw_spin_unlock_irqrestore(&l2x0_lock, flags);
>
> + pr_err("%s: a TZ mutex is already enabled\n",
> __func__);
>
> + return false;
>
> + }
>
> +
>
> + l2x0_tz_mutex = (arch_spinlock_t *)addr;
>
> + /* insure mutex ptr is updated before lock is released */
>
> + smp_wmb();
>
> +
>
> + raw_spin_unlock_irqrestore(&l2x0_lock, flags);
>
> + pr_debug("\n%s: %sable TZ mutex\n\n", __func__, (addr) ? "en" :
> "dis");
>
> + return true;
>
> +}
>
> +
>
> void __init l2x0_init(void __iomem *base, u32 aux_val, u32 aux_mask)
>
> {
>
> u32 aux;
>
> @@ -380,6 +422,7 @@ void __init l2x0_init(void __iomem *base, u32 aux_val, u32
> aux_mask)
>
> outer_cache.inv_all = l2x0_inv_all;
>
> outer_cache.disable = l2x0_disable;
>
> outer_cache.set_debug = l2x0_set_debug;
>
> + outer_cache.tz_mutex = l2x0_tz_mutex_cfg;
>
> printk(KERN_INFO "%s cache controller enabled\n", type);
>
> printk(KERN_INFO "l2x0: %d ways, CACHE_ID 0x%08x, AUX_CTRL
> 0x%08x, Cache size: %d B\n",
>
>
>
> --
>
> 1.7.11.3
>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ
2012-11-13 17:17 ` Dave Martin
@ 2012-11-13 19:22 ` Abhimanyu Kapur
2012-11-14 10:15 ` Etienne CARRIERE ST
2012-11-14 10:13 ` Etienne CARRIERE ST
1 sibling, 1 reply; 17+ messages in thread
From: Abhimanyu Kapur @ 2012-11-13 19:22 UTC (permalink / raw)
To: linux-arm-kernel
> Date: Tue, 13 Nov 2012 17:17:45 +0000
> From: dave.martin at linaro.org
> To: etienne.carriere at st.com
> Subject: Re: [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ
> CC: rabin.vincent at stericsson.com; linux at arm.linux.org.uk; srinidhi.kasagar at stericsson.com; Marc.Zyngier at arm.com; Catalin.Marinas at arm.com; linus.walleij at linaro.org; Will.Deacon at arm.com; linux-arm-kernel at lists.infradead.org
>
> On Tue, Nov 13, 2012 at 04:08:14PM +0000, Etienne CARRIERE wrote:
> > From: Etienne Carriere <etienne.carriere@stericsson.com>
> >
> >
> >
> > Secure code in TrustZone space may need to perform L2 cache
> >
> > maintenance operations. A shared mutex is required to synchronize
> >
> > linux l2cc maintenance and TZ l2cc maintenance.
>
If you are using PL310 with thrustzone support then the L2 cache lines are secure bit tagged ; your design should be such that the secure (TZ) side only does operations on secure cache lines and non-secure side does operations only on non-secure cache lines. So each entity (TZ and nonTZ) if maintains their own cache and ensures integrity before switching over via monitor, this might not be needed.
Abhi> 1) Why is this not a denial-of-service risk? (i.e., why can the Normal> World not simply hold the mutex forever?)
>
> 2) The memory types on both sides have to be _exactly_ the same, not
> just "cached", otherwise unpredictable behaviour may occur when accessing
> the mutex. It is not obvious how this is ensured.
>
> 3) Many people seem to delegate L2 cache maintenence requests via SMC
> in these situations. Can you do that instead?
>
>
> Cheers
> ---Dave
>
> > The TZ mutex is an "arch_spinlock": a 32bit DDR cell (ARMv7-A mutex).
> >
> > Linux L2 cache driver must lock TZ mutex if enabled.
> >
> >
> >
> > Signed-off-by: Etienne Carriere <etienne.carriere@stericsson.com>
> >
> > ---
> >
> > arch/arm/include/asm/outercache.h | 9 ++++
> >
> > arch/arm/mm/cache-l2x0.c | 87 +++++++++++++++++++++++++++++----------
> >
> > 2 files changed, 74 insertions(+), 22 deletions(-)
> >
> >
> >
> > diff --git a/arch/arm/include/asm/outercache.h b/arch/arm/include/asm/
> > outercache.h
> >
> > index 53426c6..7aa5eac 100644
> >
> > --- a/arch/arm/include/asm/outercache.h
> >
> > +++ b/arch/arm/include/asm/outercache.h
> >
> > @@ -35,6 +35,7 @@ struct outer_cache_fns {
> >
> > #endif
> >
> > void (*set_debug)(unsigned long);
> >
> > void (*resume)(void);
> >
> > + bool (*tz_mutex)(unsigned long);
> >
> > };
> >
> > #ifdef CONFIG_OUTER_CACHE
> >
> > @@ -81,6 +82,13 @@ static inline void outer_resume(void)
> >
> > outer_cache.resume();
> >
> > }
> >
> > +static inline bool outer_tz_mutex(unsigned long addr)
> >
> > +{
> >
> > + if (outer_cache.tz_mutex)
> >
> > + return outer_cache.tz_mutex(addr);
> >
> > + return false;
> >
> > +}
> >
> > +
> >
> > #else
> >
> > static inline void outer_inv_range(phys_addr_t start, phys_addr_t end)
> >
> > @@ -92,6 +100,7 @@ static inline void outer_flush_range(phys_addr_t start,
> > phys_addr_t end)
> >
> > static inline void outer_flush_all(void) { }
> >
> > static inline void outer_inv_all(void) { }
> >
> > static inline void outer_disable(void) { }
> >
> > +static inline bool outer_tz_mutex(unsigned long addr) { return false; }
> >
> > #endif
> >
> > diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
> >
> > index a53fd2a..eacdc74 100644
> >
> > --- a/arch/arm/mm/cache-l2x0.c
> >
> > +++ b/arch/arm/mm/cache-l2x0.c
> >
> > @@ -41,6 +41,26 @@ struct l2x0_of_data {
> >
> > void (*resume)(void);
> >
> > };
> >
> > +/*
> >
> > + * arch_spinlock (single 32bit DDR mutex cell) pointer to synchronise
> >
> > + * L2CC maintenance between linux world and secure world (ARM TZ).
> >
> > + */
> >
> > +arch_spinlock_t *l2x0_tz_mutex;
> >
> > +
> >
> > +#define l2x0_spin_lock_irqsave(flags) \
> >
> > + do
> > {
> > \
> >
> > + raw_spin_lock_irqsave(&l2x0_lock, flags);
> > \
> >
> > + if (l2x0_tz_mutex)
> > \
> >
> > + arch_spin_lock(l2x0_tz_mutex);
> > \
> >
> > + } while (0)
> >
> > +
> >
> > +#define l2x0_spin_unlock_irqrestore(flags) \
> >
> > + do
> > {
> > \
> >
> > + if (l2x0_tz_mutex)
> > \
> >
> > + arch_spin_unlock(l2x0_tz_mutex);
> > \
> >
> > + raw_spin_unlock_irqrestore(&l2x0_lock, flags);
> > \
> >
> > + } while (0)
> >
> > +
> >
> > static inline void cache_wait_way(void __iomem *reg, unsigned long mask)
> >
> > {
> >
> > /* wait for cache operation by line or way to complete */
> >
> > @@ -126,9 +146,9 @@ static void l2x0_cache_sync(void)
> >
> > {
> >
> > unsigned long flags;
> >
> > - raw_spin_lock_irqsave(&l2x0_lock, flags);
> >
> > + l2x0_spin_lock_irqsave(flags);
> >
> > cache_sync();
> >
> > - raw_spin_unlock_irqrestore(&l2x0_lock, flags);
> >
> > + l2x0_spin_unlock_irqrestore(flags);
> >
> > }
> >
> > static void __l2x0_flush_all(void)
> >
> > @@ -145,9 +165,9 @@ static void l2x0_flush_all(void)
> >
> > unsigned long flags;
> >
> > /* clean all ways */
> >
> > - raw_spin_lock_irqsave(&l2x0_lock, flags);
> >
> > + l2x0_spin_lock_irqsave(flags);
> >
> > __l2x0_flush_all();
> >
> > - raw_spin_unlock_irqrestore(&l2x0_lock, flags);
> >
> > + l2x0_spin_unlock_irqrestore(flags);
> >
> > }
> >
> > static void l2x0_clean_all(void)
> >
> > @@ -155,11 +175,11 @@ static void l2x0_clean_all(void)
> >
> > unsigned long flags;
> >
> > /* clean all ways */
> >
> > - raw_spin_lock_irqsave(&l2x0_lock, flags);
> >
> > + l2x0_spin_lock_irqsave(flags);
> >
> > writel_relaxed(l2x0_way_mask, l2x0_base + L2X0_CLEAN_WAY);
> >
> > cache_wait_way(l2x0_base + L2X0_CLEAN_WAY, l2x0_way_mask);
> >
> > cache_sync();
> >
> > - raw_spin_unlock_irqrestore(&l2x0_lock, flags);
> >
> > + l2x0_spin_unlock_irqrestore(flags);
> >
> > }
> >
> > static void l2x0_inv_all(void)
> >
> > @@ -167,13 +187,13 @@ static void l2x0_inv_all(void)
> >
> > unsigned long flags;
> >
> > /* invalidate all ways */
> >
> > - raw_spin_lock_irqsave(&l2x0_lock, flags);
> >
> > + l2x0_spin_lock_irqsave(flags);
> >
> > /* Invalidating when L2 is enabled is a nono */
> >
> > BUG_ON(readl(l2x0_base + L2X0_CTRL) & 1);
> >
> > writel_relaxed(l2x0_way_mask, l2x0_base + L2X0_INV_WAY);
> >
> > cache_wait_way(l2x0_base + L2X0_INV_WAY, l2x0_way_mask);
> >
> > cache_sync();
> >
> > - raw_spin_unlock_irqrestore(&l2x0_lock, flags);
> >
> > + l2x0_spin_unlock_irqrestore(flags);
> >
> > }
> >
> > static void l2x0_inv_range(unsigned long start, unsigned long end)
> >
> > @@ -181,7 +201,7 @@ static void l2x0_inv_range(unsigned long start, unsigned
> > long end)
> >
> > void __iomem *base = l2x0_base;
> >
> > unsigned long flags;
> >
> > - raw_spin_lock_irqsave(&l2x0_lock, flags);
> >
> > + l2x0_spin_lock_irqsave(flags);
> >
> > if (start & (CACHE_LINE_SIZE - 1)) {
> >
> > start &= ~(CACHE_LINE_SIZE - 1);
> >
> > debug_writel(0x03);
> >
> > @@ -206,13 +226,13 @@ static void l2x0_inv_range(unsigned long start, unsigned
> > long end)
> >
> > }
> >
> > if (blk_end < end) {
> >
> > - raw_spin_unlock_irqrestore(&
> > l2x0_lock, flags);
> >
> > - raw_spin_lock_irqsave(&
> > l2x0_lock, flags);
> >
> > + l2x0_spin_unlock_irqrestore
> > (flags);
> >
> > + l2x0_spin_lock_irqsave(flags);
> >
> > }
> >
> > }
> >
> > cache_wait(base + L2X0_INV_LINE_PA, 1);
> >
> > cache_sync();
> >
> > - raw_spin_unlock_irqrestore(&l2x0_lock, flags);
> >
> > + l2x0_spin_unlock_irqrestore(flags);
> >
> > }
> >
> > static void l2x0_clean_range(unsigned long start, unsigned long end)
> >
> > @@ -225,7 +245,7 @@ static void l2x0_clean_range(unsigned long start, unsigned
> > long end)
> >
> > return;
> >
> > }
> >
> > - raw_spin_lock_irqsave(&l2x0_lock, flags);
> >
> > + l2x0_spin_lock_irqsave(flags);
> >
> > start &= ~(CACHE_LINE_SIZE - 1);
> >
> > while (start < end) {
> >
> > unsigned long blk_end = start + min(end - start,
> > 4096UL);
> >
> > @@ -236,13 +256,13 @@ static void l2x0_clean_range(unsigned long start,
> > unsigned long end)
> >
> > }
> >
> > if (blk_end < end) {
> >
> > - raw_spin_unlock_irqrestore(&
> > l2x0_lock, flags);
> >
> > - raw_spin_lock_irqsave(&
> > l2x0_lock, flags);
> >
> > + l2x0_spin_unlock_irqrestore
> > (flags);
> >
> > + l2x0_spin_lock_irqsave(flags);
> >
> > }
> >
> > }
> >
> > cache_wait(base + L2X0_CLEAN_LINE_PA, 1);
> >
> > cache_sync();
> >
> > - raw_spin_unlock_irqrestore(&l2x0_lock, flags);
> >
> > + l2x0_spin_unlock_irqrestore(flags);
> >
> > }
> >
> > static void l2x0_flush_range(unsigned long start, unsigned long end)
> >
> > @@ -255,7 +275,7 @@ static void l2x0_flush_range(unsigned long start, unsigned
> > long end)
> >
> > return;
> >
> > }
> >
> > - raw_spin_lock_irqsave(&l2x0_lock, flags);
> >
> > + l2x0_spin_lock_irqsave(flags);
> >
> > start &= ~(CACHE_LINE_SIZE - 1);
> >
> > while (start < end) {
> >
> > unsigned long blk_end = start + min(end - start,
> > 4096UL);
> >
> > @@ -268,24 +288,24 @@ static void l2x0_flush_range(unsigned long start,
> > unsigned long end)
> >
> > debug_writel(0x00);
> >
> > if (blk_end < end) {
> >
> > - raw_spin_unlock_irqrestore(&
> > l2x0_lock, flags);
> >
> > - raw_spin_lock_irqsave(&
> > l2x0_lock, flags);
> >
> > + l2x0_spin_unlock_irqrestore
> > (flags);
> >
> > + l2x0_spin_lock_irqsave(flags);
> >
> > }
> >
> > }
> >
> > cache_wait(base + L2X0_CLEAN_INV_LINE_PA, 1);
> >
> > cache_sync();
> >
> > - raw_spin_unlock_irqrestore(&l2x0_lock, flags);
> >
> > + l2x0_spin_unlock_irqrestore(flags);
> >
> > }
> >
> > static void l2x0_disable(void)
> >
> > {
> >
> > unsigned long flags;
> >
> > - raw_spin_lock_irqsave(&l2x0_lock, flags);
> >
> > + l2x0_spin_lock_irqsave(flags);
> >
> > __l2x0_flush_all();
> >
> > writel_relaxed(0, l2x0_base + L2X0_CTRL);
> >
> > dsb();
> >
> > - raw_spin_unlock_irqrestore(&l2x0_lock, flags);
> >
> > + l2x0_spin_unlock_irqrestore(flags);
> >
> > }
> >
> > static void l2x0_unlock(u32 cache_id)
> >
> > @@ -307,6 +327,28 @@ static void l2x0_unlock(u32 cache_id)
> >
> > }
> >
> > }
> >
> > +/* Enable/disable external mutex shared with TZ code */
> >
> > +static bool l2x0_tz_mutex_cfg(unsigned long addr)
> >
> > +{
> >
> > + unsigned long flags;
> >
> > +
> >
> > + raw_spin_lock_irqsave(&l2x0_lock, flags);
> >
> > +
> >
> > + if (addr && l2x0_tz_mutex && (addr != (uint)l2x0_tz_mutex)) {
> >
> > + raw_spin_unlock_irqrestore(&l2x0_lock, flags);
> >
> > + pr_err("%s: a TZ mutex is already enabled\n",
> > __func__);
> >
> > + return false;
> >
> > + }
> >
> > +
> >
> > + l2x0_tz_mutex = (arch_spinlock_t *)addr;
> >
> > + /* insure mutex ptr is updated before lock is released */
> >
> > + smp_wmb();
> >
> > +
> >
> > + raw_spin_unlock_irqrestore(&l2x0_lock, flags);
> >
> > + pr_debug("\n%s: %sable TZ mutex\n\n", __func__, (addr) ? "en" :
> > "dis");
> >
> > + return true;
> >
> > +}
> >
> > +
> >
> > void __init l2x0_init(void __iomem *base, u32 aux_val, u32 aux_mask)
> >
> > {
> >
> > u32 aux;
> >
> > @@ -380,6 +422,7 @@ void __init l2x0_init(void __iomem *base, u32 aux_val, u32
> > aux_mask)
> >
> > outer_cache.inv_all = l2x0_inv_all;
> >
> > outer_cache.disable = l2x0_disable;
> >
> > outer_cache.set_debug = l2x0_set_debug;
> >
> > + outer_cache.tz_mutex = l2x0_tz_mutex_cfg;
> >
> > printk(KERN_INFO "%s cache controller enabled\n", type);
> >
> > printk(KERN_INFO "l2x0: %d ways, CACHE_ID 0x%08x, AUX_CTRL
> > 0x%08x, Cache size: %d B\n",
> >
> >
> >
> > --
> >
> > 1.7.11.3
> >
> >
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121113/ce5f15d9/attachment-0001.html>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ
2012-11-13 17:07 ` Russell King - ARM Linux
@ 2012-11-14 9:18 ` Etienne CARRIERE ST
2012-11-14 9:51 ` Will Deacon
2012-11-14 10:21 ` Russell King - ARM Linux
0 siblings, 2 replies; 17+ messages in thread
From: Etienne CARRIERE ST @ 2012-11-14 9:18 UTC (permalink / raw)
To: linux-arm-kernel
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Tuesday, November 13, 2012 6:07 PM
>
> Right, so, what this tells me is that the implementation of the spinlock
> in the secure software is potentially the same as the kernel's spinlock.
> The kernel's spinlock implementation is GPL'd. If the secure side spinlock
> implementation is a copy of the kernel's spinlock implementation, then
> that implementation is also GPL'd (...).
>
> I think someone has some explaining to do.
It does.
The TZ implementation of the shared mutex is the ARM native mutex instructions set LDREX/STREX, protected by a native ARM IRQ disabling.
I believe this is native ARMv7 natural handling and not subject to GPLization.
This is why the mutex added for TZ shared of L2 cache maintenance is a "arch_spinlock". It assumes linux handles the spinlock as a native ARM native mutex DDR cell, handled from the ARMv7-A LDREX/STREX.
I think this is the weakest part of the driver since they is no insurance that linux pristine arch_spinlock will not evolve into something else (i.e features in raw_spinclock).
etienne
-----Original Message-----
To: Etienne CARRIERE
Cc: linux-arm-kernel at lists.infradead.org; Will Deacon; Marc Zyngier; Catalin Marinas; Linus Walleij (linus.walleij at linaro.org); Rabin VINCENT; Srinidhi KASAGAR
Subject: Re: [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ
On Tue, Nov 13, 2012 at 05:08:14PM +0100, Etienne CARRIERE wrote:
> From: Etienne Carriere <etienne.carriere@stericsson.com>
>
> Secure code in TrustZone space may need to perform L2 cache
> maintenance operations. A shared mutex is required to synchronize
> linux l2cc maintenance and TZ l2cc maintenance.
>
> The TZ mutex is an "arch_spinlock": a 32bit DDR cell (ARMv7-A mutex).
> Linux L2 cache driver must lock TZ mutex if enabled.
>...
> +#define l2x0_spin_lock_irqsave(flags) \
> + do { \
> + raw_spin_lock_irqsave(&l2x0_lock, flags); \
> + if (l2x0_tz_mutex) \
> + arch_spin_lock(l2x0_tz_mutex); \
> + } while (0)
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ
2012-11-14 9:18 ` Etienne CARRIERE ST
@ 2012-11-14 9:51 ` Will Deacon
2012-11-14 10:40 ` Etienne CARRIERE ST
2012-11-14 10:21 ` Russell King - ARM Linux
1 sibling, 1 reply; 17+ messages in thread
From: Will Deacon @ 2012-11-14 9:51 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Nov 14, 2012 at 09:18:31AM +0000, Etienne CARRIERE ST wrote:
> This is why the mutex added for TZ shared of L2 cache maintenance is a "arch_spinlock". It assumes linux handles the spinlock as a native ARM native mutex DDR cell, handled from the ARMv7-A LDREX/STREX.
> I think this is the weakest part of the driver since they is no insurance that linux pristine arch_spinlock will not evolve into something else (i.e features in raw_spinclock).
How does this work with our ticket lock approach? Can you swizzle your
locking implementation by inspecting the kernel version of the code running
on the non-secure side? (I sincerely hope not...)
Will
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ
2012-11-13 17:17 ` Dave Martin
2012-11-13 19:22 ` Abhimanyu Kapur
@ 2012-11-14 10:13 ` Etienne CARRIERE ST
1 sibling, 0 replies; 17+ messages in thread
From: Etienne CARRIERE ST @ 2012-11-14 10:13 UTC (permalink / raw)
To: linux-arm-kernel
> Tue 11-13-2012 6:18 PM
> From: Dave Martin <dave.martin@linaro.org>
>
> > Secure code in TrustZone space may need to perform L2 cache
> > maintenance operations. A shared mutex is required to synchronize
> > linux l2cc maintenance and TZ l2cc maintenance.
>
> 1) Why is this not a denial-of-service risk? (i.e., why can the Normal
> World not simply hold the mutex forever?)
On SoC's, it is likely that some HW IP are controlled with ARM NS compliant HW support, thus needed for TZ execution (RAMs, ...).
It is likely that nonTZ world, in such situation, could disable a clock, regulator, ... and arm TZ executions, including cache content.
An easy "secu denial of service" hack, is just to reprogram linux based OS to prevent any or selected calls to secure code.
Since linux OS is an untrusted environment, one could just reprogram it to prevent call to secure tagged features.
It is the responsibility of the TZ implementation to insure that secure content is not corrupted nor accessed to from nonTZ world.
> 2) The memory types on both sides have to be _exactly_ the same,
> not just "cached", otherwise unpredictable behaviour may occur when
> accessing the mutex. It is not obvious how this is ensured.
Ok, I will cross check this.
The mapping of the mutex/spinlock "lock" field is under responsibility of the TZ API driver.
Each platform TZ API should define the expected mapping configuration.
> 3) Many people seem to delegate L2 cache maintenence requests
> via SMC in these situations. Can you do that instead?
We can have code that runs in TZ world and heavily processes data, benefitting from cache support, then needing the flush data to DDR.
In such case, TZ would not need to request linux to request cache line maintenance.
Regards,
Etienne
>
> Cheers
> ---Dave
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ
2012-11-13 19:22 ` Abhimanyu Kapur
@ 2012-11-14 10:15 ` Etienne CARRIERE ST
2012-11-14 17:22 ` Catalin Marinas
0 siblings, 1 reply; 17+ messages in thread
From: Etienne CARRIERE ST @ 2012-11-14 10:15 UTC (permalink / raw)
To: linux-arm-kernel
> Tue 11-13-2012 8:23 PM
> From: Abhimanyu Kapur <abhimanyu.kapur@outlook.com>
>
> > Secure code in TrustZone space may need to perform L2 cache
> > maintenance operations. A shared mutex is required to synchronize
> > linux l2cc maintenance and TZ l2cc maintenance.
>
> If you are using PL310 with thrustzone support then the L2 cache lines
> are secure bit tagged ; your design should be such that the secure (TZ)
> side only does operations on secure cache lines and non-secure side
> does operations only on non-secure cache lines. So each entity (TZ
> and nonTZ) if maintains their own cache and ensures integrity before
> switching over via monitor, this might not be needed.
I don't think 2 cores can safely write the LX20_CLEAN/INV_LINE_PA registers of the PL310 at the same time, even if targeting different lines.
Each core TZ code must protect against other core (being UnSecure or TZ mode). The core IRQ disabling is required to protect from deadlock nonTZ and TZ worlds.
Regards,
etienne
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ
2012-11-14 9:18 ` Etienne CARRIERE ST
2012-11-14 9:51 ` Will Deacon
@ 2012-11-14 10:21 ` Russell King - ARM Linux
2012-11-14 10:45 ` Etienne CARRIERE ST
1 sibling, 1 reply; 17+ messages in thread
From: Russell King - ARM Linux @ 2012-11-14 10:21 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Nov 14, 2012 at 10:18:31AM +0100, Etienne CARRIERE ST wrote:
> > From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> > Sent: Tuesday, November 13, 2012 6:07 PM
> >
> > Right, so, what this tells me is that the implementation of the spinlock
> > in the secure software is potentially the same as the kernel's spinlock.
> > The kernel's spinlock implementation is GPL'd. If the secure side spinlock
> > implementation is a copy of the kernel's spinlock implementation, then
> > that implementation is also GPL'd (...).
> >
> > I think someone has some explaining to do.
>
> It does.
>
> The TZ implementation of the shared mutex is the ARM native mutex
> instructions set LDREX/STREX, protected by a native ARM IRQ disabling.
> I believe this is native ARMv7 natural handling and not subject to
> GPLization.
Maybe, maybe not. There's more to locking than just the instructions
used - there's the values which are used to represent "locked" and
"unlocked", and then there's the ticket locking implementation found
in recent kernels which defines the value to have more meaning than
just "locked" and "unlocked".
With ticket locking, you need to know the details of the implementation
to be able to lock successfully, and that basically means copying the
implementation.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ
2012-11-14 9:51 ` Will Deacon
@ 2012-11-14 10:40 ` Etienne CARRIERE ST
0 siblings, 0 replies; 17+ messages in thread
From: Etienne CARRIERE ST @ 2012-11-14 10:40 UTC (permalink / raw)
To: linux-arm-kernel
Sorry, I don't catch "swizzle your implementation" ?
You are referring to the raw_spinlock LOCKBREAK stuf ?
I will look deeper into linux code to check is some deadlock could appear.
The shared mutex arch spinlock is locked after raw_spinlick locking and released before l2x0 native raw_spinlock releasing.
It is intended to leave kernel preemption stuff inside the linux world.
etienne
-----Original Message-----
From: Will Deacon [mailto:will.deacon at arm.com]
Sent: Wednesday, November 14, 2012 10:51 AM
To: Etienne CARRIERE ST
Cc: Russell King - ARM Linux; linux-arm-kernel at lists.infradead.org; Marc Zyngier; Catalin Marinas; Linus Walleij; Rabin VINCENT; Srinidhi KASAGAR
Subject: Re: [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ
On Wed, Nov 14, 2012 at 09:18:31AM +0000, Etienne CARRIERE ST wrote:
> This is why the mutex added for TZ shared of L2 cache maintenance is a "arch_spinlock". It assumes linux handles the spinlock as a native ARM native mutex DDR cell, handled from the ARMv7-A LDREX/STREX.
> I think this is the weakest part of the driver since they is no insurance that linux pristine arch_spinlock will not evolve into something else (i.e features in raw_spinclock).
How does this work with our ticket lock approach? Can you swizzle your locking implementation by inspecting the kernel version of the code running on the non-secure side? (I sincerely hope not...)
Will
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ
2012-11-14 10:21 ` Russell King - ARM Linux
@ 2012-11-14 10:45 ` Etienne CARRIERE ST
2012-11-14 10:50 ` Russell King - ARM Linux
0 siblings, 1 reply; 17+ messages in thread
From: Etienne CARRIERE ST @ 2012-11-14 10:45 UTC (permalink / raw)
To: linux-arm-kernel
> Sent: Wednesday, November 14, 2012 11:21 AM
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
>
> > > Right, so, what this tells me is that the implementation of the
> > > spinlock in the secure software is potentially the same as the
> > > kernel's spinlock.
> > > The kernel's spinlock implementation is GPL'd. If the secure side
> > > spinlock implementation is a copy of the kernel's spinlock
> > > implementation, then that implementation is also GPL'd (...).
> > >
> > > I think someone has some explaining to do.
> >
> > It does.
> >
> > The TZ implementation of the shared mutex is the ARM native mutex
> > instructions set LDREX/STREX, protected by a native ARM IRQ disabling.
> > I believe this is native ARMv7 natural handling and not subject to
> > GPLization.
>
> Maybe, maybe not. There's more to locking than just the instructions used
> - there's the values which are used to represent "locked" and "unlocked",
> and then there's the ticket locking implementation found in recent kernels
> which defines the value to have more meaning than just "locked" and "unlocked".
>
> With ticket locking, you need to know the details of the implementation to be
> able to lock successfully, and that basically means copying the implementation.
Thanks for notifying. I will look at the recent release to see if our architecture complies with Kernel code.
Our mutex implementation is based on 0/1 lock/unlock value in target mutex DDR cell.
etienne
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ
2012-11-14 10:45 ` Etienne CARRIERE ST
@ 2012-11-14 10:50 ` Russell King - ARM Linux
2012-11-14 11:02 ` Etienne CARRIERE ST
0 siblings, 1 reply; 17+ messages in thread
From: Russell King - ARM Linux @ 2012-11-14 10:50 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Nov 14, 2012 at 11:45:42AM +0100, Etienne CARRIERE ST wrote:
> Thanks for notifying. I will look at the recent release to see if our
> architecture complies with Kernel code.
>
> Our mutex implementation is based on 0/1 lock/unlock value in target
> mutex DDR cell.
Right, so we have another bug here: it is incompatible with the current
ARM spinlock implementation - and using it on the same memory location
as a spinlock will corrupt the spinlock.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ
2012-11-14 10:50 ` Russell King - ARM Linux
@ 2012-11-14 11:02 ` Etienne CARRIERE ST
0 siblings, 0 replies; 17+ messages in thread
From: Etienne CARRIERE ST @ 2012-11-14 11:02 UTC (permalink / raw)
To: linux-arm-kernel
> > Thanks for notifying. I will look at the recent release to see if our
> > architecture complies with Kernel code.
> >
> > Our mutex implementation is based on 0/1 lock/unlock value in target
> > mutex DDR cell.
>
> Right, so we have another bug here: it is incompatible with the current ARM
> spinlock implementation - and using it on the same memory location as a
> spinlock will corrupt the spinlock.
Right.
I see our architecture complies with kernels before v3.6-rc1.
Until then, it seems we can use our patch, but we must plan something else for the more recent kernels.
etienne
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ
2012-11-14 10:15 ` Etienne CARRIERE ST
@ 2012-11-14 17:22 ` Catalin Marinas
2012-11-15 13:46 ` Etienne CARRIERE ST
0 siblings, 1 reply; 17+ messages in thread
From: Catalin Marinas @ 2012-11-14 17:22 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Nov 14, 2012 at 10:15:46AM +0000, Etienne CARRIERE ST wrote:
> > Tue 11-13-2012 8:23 PM
> > From: Abhimanyu Kapur <abhimanyu.kapur@outlook.com>
> >
> > > Secure code in TrustZone space may need to perform L2 cache
> > > maintenance operations. A shared mutex is required to synchronize
> > > linux l2cc maintenance and TZ l2cc maintenance.
> >
> > If you are using PL310 with thrustzone support then the L2 cache lines
> > are secure bit tagged ; your design should be such that the secure (TZ)
> > side only does operations on secure cache lines and non-secure side
> > does operations only on non-secure cache lines. So each entity (TZ
> > and nonTZ) if maintains their own cache and ensures integrity before
> > switching over via monitor, this might not be needed.
>
> I don't think 2 cores can safely write the LX20_CLEAN/INV_LINE_PA
> registers of the PL310 at the same time, even if targeting different
> lines.
Actually for the clean/invalidate operations by PA you can safely write
the registers from two different processors as they get serialised by
the hardware. What you don't get is protection around the background
operations (clean/inv by way). I think it depends on how the PL310 is
wired on your hardware but trying to do a PA operation while a
background one is in progress would trigger an external abort.
So the NS world could simply start a background cache operation without
taking the lock while the secure world thinks that it has the lock and
tries to do a PA operation which would abort.
My advise is to simply ignore the shared locking and only do atomic PA
operations on the secure side. The secure side also needs to poll for
the completion of any background operation that was started in
non-secure world. Of course, there is still a race, in which case,
depending on the hardware implementation, you would need to trap any
possible aborts while in secure mode when writing the PL310 registers.
--
Catalin
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ
2012-11-14 17:22 ` Catalin Marinas
@ 2012-11-15 13:46 ` Etienne CARRIERE ST
2012-11-15 13:55 ` Catalin Marinas
0 siblings, 1 reply; 17+ messages in thread
From: Etienne CARRIERE ST @ 2012-11-15 13:46 UTC (permalink / raw)
To: linux-arm-kernel
Thanks for the details.
I think we will internally use our hack in l2x0 spin locking to prevent collision, based on old arch_spinlock support.
For a definitive outer cache support in TZ, I think we will go into a RPC/SMC based architecture.
I mean a basic SMC to request secured outer cache maintenance, called from linux L2 cache driver based on its native spinlock, whatever it is.
This will indeed require some modification in L2 cache driver (cache-l2x0.c).
We will work on that once our TZ implementation support such RPC/SMC features for L2 cache maintenance.
Thanks to all for the feedbacks.
Regards,
etienne
-----Original Message-----
From: Catalin Marinas [mailto:catalin.marinas at arm.com]
Sent: Wednesday, November 14, 2012 6:22 PM
To: Etienne CARRIERE ST
Cc: Abhimanyu Kapur; Dave Martin; Rabin VINCENT; Russell King; Srinidhi KASAGAR; Marc Zyngier; Linus Walleij (linus.walleij at linaro.org); Will Deacon; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ
On Wed, Nov 14, 2012 at 10:15:46AM +0000, Etienne CARRIERE ST wrote:
> > Tue 11-13-2012 8:23 PM
> > From: Abhimanyu Kapur <abhimanyu.kapur@outlook.com>
> >
> > > Secure code in TrustZone space may need to perform L2 cache
> > > maintenance operations. A shared mutex is required to synchronize
> > > linux l2cc maintenance and TZ l2cc maintenance.
> >
> > If you are using PL310 with thrustzone support then the L2 cache
> > lines are secure bit tagged ; your design should be such that the
> > secure (TZ) side only does operations on secure cache lines and
> > non-secure side does operations only on non-secure cache lines. So
> > each entity (TZ and nonTZ) if maintains their own cache and ensures
> > integrity before switching over via monitor, this might not be needed.
>
> I don't think 2 cores can safely write the LX20_CLEAN/INV_LINE_PA
> registers of the PL310 at the same time, even if targeting different
> lines.
Actually for the clean/invalidate operations by PA you can safely write the registers from two different processors as they get serialised by the hardware. What you don't get is protection around the background operations (clean/inv by way). I think it depends on how the PL310 is wired on your hardware but trying to do a PA operation while a background one is in progress would trigger an external abort.
So the NS world could simply start a background cache operation without taking the lock while the secure world thinks that it has the lock and tries to do a PA operation which would abort.
My advise is to simply ignore the shared locking and only do atomic PA operations on the secure side. The secure side also needs to poll for the completion of any background operation that was started in non-secure world. Of course, there is still a race, in which case, depending on the hardware implementation, you would need to trap any possible aborts while in secure mode when writing the PL310 registers.
--
Catalin
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ
2012-11-15 13:46 ` Etienne CARRIERE ST
@ 2012-11-15 13:55 ` Catalin Marinas
2012-11-15 14:10 ` Etienne CARRIERE ST
0 siblings, 1 reply; 17+ messages in thread
From: Catalin Marinas @ 2012-11-15 13:55 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Nov 15, 2012 at 01:46:29PM +0000, Etienne CARRIERE ST wrote:
> I think we will internally use our hack in l2x0 spin locking to
> prevent collision, based on old arch_spinlock support.
Or just handle the collision in the secure code and make sure you don't
start background operations. If you don't use background operations, you
don't need any locking.
--
Catalin
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ
2012-11-15 13:55 ` Catalin Marinas
@ 2012-11-15 14:10 ` Etienne CARRIERE ST
0 siblings, 0 replies; 17+ messages in thread
From: Etienne CARRIERE ST @ 2012-11-15 14:10 UTC (permalink / raw)
To: linux-arm-kernel
If I understood well, you stated that on-going background operations on L2 can jeopardize PA operations.
So the secure code must insure other CPUs do not perform other L2 operations, including non-PA based operations.
Linux protects both PA and WAY operations using the very same spinlock.
We must hold this lock while performing any PA operations from secure world.
I am right ?
etienne
-----Original Message-----
From: Catalin Marinas [mailto:catalin.marinas at arm.com]
Sent: Thursday, November 15, 2012 2:56 PM
To: Etienne CARRIERE ST
Cc: Abhimanyu Kapur; Dave Martin; Rabin VINCENT; Russell King; Srinidhi KASAGAR; Marc Zyngier; Linus Walleij (linus.walleij at linaro.org); Will Deacon; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ
On Thu, Nov 15, 2012 at 01:46:29PM +0000, Etienne CARRIERE ST wrote:
> I think we will internally use our hack in l2x0 spin locking to
> prevent collision, based on old arch_spinlock support.
Or just handle the collision in the secure code and make sure you don't start background operations. If you don't use background operations, you don't need any locking.
--
Catalin
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2012-11-15 14:10 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-11-13 16:08 [PATCH 1/2] arm/mm: L2CC shared mutex with ARM TZ Etienne CARRIERE
2012-11-13 17:07 ` Russell King - ARM Linux
2012-11-14 9:18 ` Etienne CARRIERE ST
2012-11-14 9:51 ` Will Deacon
2012-11-14 10:40 ` Etienne CARRIERE ST
2012-11-14 10:21 ` Russell King - ARM Linux
2012-11-14 10:45 ` Etienne CARRIERE ST
2012-11-14 10:50 ` Russell King - ARM Linux
2012-11-14 11:02 ` Etienne CARRIERE ST
2012-11-13 17:17 ` Dave Martin
2012-11-13 19:22 ` Abhimanyu Kapur
2012-11-14 10:15 ` Etienne CARRIERE ST
2012-11-14 17:22 ` Catalin Marinas
2012-11-15 13:46 ` Etienne CARRIERE ST
2012-11-15 13:55 ` Catalin Marinas
2012-11-15 14:10 ` Etienne CARRIERE ST
2012-11-14 10:13 ` Etienne CARRIERE ST
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.