[PATCH] powerpc: Check that all cpu features are in the possible map

[PATCH] powerpc: Check that all cpu features are in the possible map

[Michael Ellerman]

cpu_has_feature() has an optimisation where it maintains a map of
possible cpu features. This allows the compiler to determine at compile
time that some cpu_has_feature() checks will always return 0, and
therefore the code guarded by the check can be elided.

However we have no logic to check whether the set of cpu features in the
current cpu spec are in the possible map. Although that should never
happen, if it does things are likely to go badly. So add a check and
print a warning.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/kernel/cputable.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/powerpc/kernel/cputable.c b/arch/powerpc/kernel/cputable.c
index 6c8dd5d..eb31713 100644
--- a/arch/powerpc/kernel/cputable.c
+++ b/arch/powerpc/kernel/cputable.c
@@ -2255,6 +2255,15 @@ static struct cpu_spec * __init setup_cpu_spec(unsigned long offset,
 	}
 #endif /* CONFIG_PPC64 || CONFIG_BOOKE */
 
+	/*
+	 * Check that all CPU features are in the possible mask. We don't want
+	 * to WARN() because we're called very early, and doing so will kill
+	 * the machine, so just printk() instead.
+	 */
+	if (t->cpu_features != (t->cpu_features & CPU_FTRS_POSSIBLE))
+		printk("WARNING: cpu spec contains impossible features! 0x%lx\n",
+			(t->cpu_features & ~CPU_FTRS_POSSIBLE));
+
 	return t;
 }
 
-- 
1.8.3.2

Re: [PATCH] powerpc: Check that all cpu features are in the possible map

[Michael Ellerman]

On Tue, 2014-03-04 at 13:44 +1100, Michael Ellerman wrote:
> diff --git a/arch/powerpc/kernel/cputable.c b/arch/powerpc/kernel/cputable.c
> index 6c8dd5d..eb31713 100644
> --- a/arch/powerpc/kernel/cputable.c
> +++ b/arch/powerpc/kernel/cputable.c
> @@ -2255,6 +2255,15 @@ static struct cpu_spec * __init setup_cpu_spec(unsigned long offset,
>  	}
>  #endif /* CONFIG_PPC64 || CONFIG_BOOKE */
>  
> +	/*
> +	 * Check that all CPU features are in the possible mask. We don't want
> +	 * to WARN() because we're called very early, and doing so will kill
> +	 * the machine, so just printk() instead.
> +	 */
> +	if (t->cpu_features != (t->cpu_features & CPU_FTRS_POSSIBLE))
> +		printk("WARNING: cpu spec contains impossible features! 0x%lx\n",
> +			(t->cpu_features & ~CPU_FTRS_POSSIBLE));


Actually even this is not safe.

printk() takes the logbuf_lock, but we don't have a paca yet, so our LOCK_TOKEN
will be some random guff in low memory. And although that's OK, we're single
threaded, it's a bit fishy.

What's worse is printk() does local_irq_save(), which will _store_ to our paca
(soft_enabled), and so we risk flipping a value somewhere.

Interestingly lockdep appears to be OK, even though we haven't initialised it
yet. printk() helpfully turns lockdep off before doing any locking.

cheers