subject:"\[PATCH v2 2\/3\] powerpc\/fsl\: Add barrier_nospec implementation for NXP PowerPC Book3E"

Re: [PATCH v2 2/3] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book3E

2018-07-05 Thread Diana Madalina Craciun

Hi Michael,

Thank you for the review.

On 07/03/2018 10:26 AM, Michael Ellerman wrote:
> Hi Diana,
>
> A few comments below ...
>
> Diana Craciun  writes:
>> Implement the barrier_nospec as a isync;sync instruction sequence.
>> The implementation uses the infrastructure built for BOOK3S 64.
> Do you have any details on why that sequence functions as an effective
> barrier on your chips?

It was recommended by the hardware team, I do not have details.

>
> In a lot of places we have eg:
>
>  +#if defined(CONFIG_PPC_BOOK3S_64) || defined(CONFIG_PPC_FSL_BOOK3E)
>
> Can you please add a Kconfig symbol to capture that. eg.
>
> config PPC_NOSPEC
>   bool
> default y
>   depends on PPC_BOOK3S_64 || PPC_FSL_BOOK3E
>
>
> And then use that everywhere.

OK.

>
>> diff --git a/arch/powerpc/include/asm/barrier.h 
>> b/arch/powerpc/include/asm/barrier.h
>> index f67b3f6..405d572 100644
>> --- a/arch/powerpc/include/asm/barrier.h
>> +++ b/arch/powerpc/include/asm/barrier.h
>> @@ -86,6 +86,16 @@ do {  
>> \
>>  // This also acts as a compiler barrier due to the memory clobber.
>>  #define barrier_nospec() asm (stringify_in_c(barrier_nospec_asm) ::: 
>> "memory")
>>  
>> +#elif defined(CONFIG_PPC_FSL_BOOK3E)
>> +/*
>> + * Prevent the execution of subsequent instructions speculatively using a
>> + * isync;sync instruction sequence.
>> + */
>> +#define barrier_nospec_asm NOSPEC_BARRIER_FIXUP_SECTION; nop; nop
>> +
>> +// This also acts as a compiler barrier due to the memory clobber.
>> +#define barrier_nospec() asm (stringify_in_c(barrier_nospec_asm) ::: 
>> "memory")
>> +
>>  #else /* !CONFIG_PPC_BOOK3S_64 */
>>  #define barrier_nospec_asm
>>  #define barrier_nospec()
> If we have CONFIG_PPC_NOSPEC this can be done something like:
>
> #ifdef CONFIG_PPC_BOOK3S_64
> #define NOSPEC_BARRIER_SLOT   nop
> #elif defined(CONFIG_PPC_FSL_BOOK3E)
> #define NOSPEC_BARRIER_SLOT   nop; nop
> #endif /* CONFIG_PPC_BOOK3S_64 */
>
> #ifdef CONFIG_PPC_NOSPEC
> /*
>  * Prevent execution of subsequent instructions until preceding branches have
>  * been fully resolved and are no longer executing speculatively.
>  */
> #define barrier_nospec_asm NOSPEC_BARRIER_FIXUP_SECTION; NOSPEC_BARRIER_SLOT
>
> // This also acts as a compiler barrier due to the memory clobber.
> #define barrier_nospec() asm (stringify_in_c(barrier_nospec_asm) ::: "memory")
> #else
> #define barrier_nospec_asm
> #define barrier_nospec()
> #endif /* CONFIG_PPC_NOSPEC */

OK.

>
>
>> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
>> index 2b4c40b2..d9dee43 100644
>> --- a/arch/powerpc/kernel/Makefile
>> +++ b/arch/powerpc/kernel/Makefile
>> @@ -76,7 +76,7 @@ endif
>>  obj64-$(CONFIG_HIBERNATION) += swsusp_asm64.o
>>  obj-$(CONFIG_MODULES)   += module.o module_$(BITS).o
>>  obj-$(CONFIG_44x)   += cpu_setup_44x.o
>> -obj-$(CONFIG_PPC_FSL_BOOK3E)+= cpu_setup_fsl_booke.o
>> +obj-$(CONFIG_PPC_FSL_BOOK3E)+= cpu_setup_fsl_booke.o security.o
>>  obj-$(CONFIG_PPC_DOORBELL)  += dbell.o
>>  obj-$(CONFIG_JUMP_LABEL)+= jump_label.o
> Can we instead do:
>
> obj-$(CONFIG_PPC_NOSPEC)  += security.o

OK

>
>> diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
>> index c55e102..797c975 100644
>> --- a/arch/powerpc/kernel/security.c
>> +++ b/arch/powerpc/kernel/security.c
>> @@ -13,7 +13,9 @@
>>  #include 
>>  
>>  
>> +#ifdef CONFIG_PPC_BOOK3S_64
>>  unsigned long powerpc_security_features __read_mostly = SEC_FTR_DEFAULT;
>> +#endif /* CONFIG_PPC_BOOK3S_64 */
> Why are you making that book3s specific?
>
> By doing that you then can't use the existing versions of
> setup_barrier_nospec() and cpu_show_spectre_v1/v2().
>
>> @@ -24,6 +26,7 @@ static void enable_barrier_nospec(bool enable)
>>  do_barrier_nospec_fixups(enable);
>>  }
>>  
>> +#ifdef CONFIG_PPC_BOOK3S_64
>>  void setup_barrier_nospec(void)
>>  {
>>  bool enable;
>> @@ -46,6 +49,15 @@ void setup_barrier_nospec(void)
>>  if (!no_nospec)
>>  enable_barrier_nospec(enable);
>>  }
>> +#endif /* CONFIG_PPC_BOOK3S_64 */
>> +
>> +#ifdef CONFIG_PPC_FSL_BOOK3E
>> +void setup_barrier_nospec(void)
>> +{
>> +if (!no_nospec)
>> +enable_barrier_nospec(true);
>> +}
>> +#endif /* CONFIG_PPC_FSL_BOOK3E */
> eg. that is identical to the existing version except for the feature check:
>
>   enable = security_ftr_enabled(SEC_FTR_FAVOUR_SECURITY) &&
>security_ftr_enabled(SEC_FTR_BNDS_CHK_SPEC_BAR);
>
>
> Both those bits are enabled by default, so you shouldn't need to elide
> that check.
>
> So basically you should be able to use the existing
> setup_barrier_nospec() if you just remove the ifdef around
> powerpc_security_features. Or am I missing something?

OK. I was under the impression that those bits are not enabled by
default. But obviously I was wrong. In this case I will use the existing
function.

>
>
>> diff

Re: [PATCH v2 2/3] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book3E

2018-07-03 Thread Michael Ellerman

Hi Diana,

A few comments below ...

Diana Craciun  writes:
> Implement the barrier_nospec as a isync;sync instruction sequence.
> The implementation uses the infrastructure built for BOOK3S 64.

Do you have any details on why that sequence functions as an effective
barrier on your chips?

In a lot of places we have eg:

 +#if defined(CONFIG_PPC_BOOK3S_64) || defined(CONFIG_PPC_FSL_BOOK3E)

Can you please add a Kconfig symbol to capture that. eg.

config PPC_NOSPEC
bool
default y
depends on PPC_BOOK3S_64 || PPC_FSL_BOOK3E


And then use that everywhere.

> diff --git a/arch/powerpc/include/asm/barrier.h 
> b/arch/powerpc/include/asm/barrier.h
> index f67b3f6..405d572 100644
> --- a/arch/powerpc/include/asm/barrier.h
> +++ b/arch/powerpc/include/asm/barrier.h
> @@ -86,6 +86,16 @@ do {   
> \
>  // This also acts as a compiler barrier due to the memory clobber.
>  #define barrier_nospec() asm (stringify_in_c(barrier_nospec_asm) ::: 
> "memory")
>  
> +#elif defined(CONFIG_PPC_FSL_BOOK3E)
> +/*
> + * Prevent the execution of subsequent instructions speculatively using a
> + * isync;sync instruction sequence.
> + */
> +#define barrier_nospec_asm NOSPEC_BARRIER_FIXUP_SECTION; nop; nop
> +
> +// This also acts as a compiler barrier due to the memory clobber.
> +#define barrier_nospec() asm (stringify_in_c(barrier_nospec_asm) ::: 
> "memory")
> +
>  #else /* !CONFIG_PPC_BOOK3S_64 */
>  #define barrier_nospec_asm
>  #define barrier_nospec()

If we have CONFIG_PPC_NOSPEC this can be done something like:

#ifdef CONFIG_PPC_BOOK3S_64
#define NOSPEC_BARRIER_SLOT nop
#elif defined(CONFIG_PPC_FSL_BOOK3E)
#define NOSPEC_BARRIER_SLOT nop; nop
#endif /* CONFIG_PPC_BOOK3S_64 */

#ifdef CONFIG_PPC_NOSPEC
/*
 * Prevent execution of subsequent instructions until preceding branches have
 * been fully resolved and are no longer executing speculatively.
 */
#define barrier_nospec_asm NOSPEC_BARRIER_FIXUP_SECTION; NOSPEC_BARRIER_SLOT

// This also acts as a compiler barrier due to the memory clobber.
#define barrier_nospec() asm (stringify_in_c(barrier_nospec_asm) ::: "memory")
#else
#define barrier_nospec_asm
#define barrier_nospec()
#endif /* CONFIG_PPC_NOSPEC */


> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> index 2b4c40b2..d9dee43 100644
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -76,7 +76,7 @@ endif
>  obj64-$(CONFIG_HIBERNATION)  += swsusp_asm64.o
>  obj-$(CONFIG_MODULES)+= module.o module_$(BITS).o
>  obj-$(CONFIG_44x)+= cpu_setup_44x.o
> -obj-$(CONFIG_PPC_FSL_BOOK3E) += cpu_setup_fsl_booke.o
> +obj-$(CONFIG_PPC_FSL_BOOK3E) += cpu_setup_fsl_booke.o security.o
>  obj-$(CONFIG_PPC_DOORBELL)   += dbell.o
>  obj-$(CONFIG_JUMP_LABEL) += jump_label.o

Can we instead do:

obj-$(CONFIG_PPC_NOSPEC)+= security.o

> diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
> index c55e102..797c975 100644
> --- a/arch/powerpc/kernel/security.c
> +++ b/arch/powerpc/kernel/security.c
> @@ -13,7 +13,9 @@
>  #include 
>  
>  
> +#ifdef CONFIG_PPC_BOOK3S_64
>  unsigned long powerpc_security_features __read_mostly = SEC_FTR_DEFAULT;
> +#endif /* CONFIG_PPC_BOOK3S_64 */

Why are you making that book3s specific?

By doing that you then can't use the existing versions of
setup_barrier_nospec() and cpu_show_spectre_v1/v2().

> @@ -24,6 +26,7 @@ static void enable_barrier_nospec(bool enable)
>   do_barrier_nospec_fixups(enable);
>  }
>  
> +#ifdef CONFIG_PPC_BOOK3S_64
>  void setup_barrier_nospec(void)
>  {
>   bool enable;
> @@ -46,6 +49,15 @@ void setup_barrier_nospec(void)
>   if (!no_nospec)
>   enable_barrier_nospec(enable);
>  }
> +#endif /* CONFIG_PPC_BOOK3S_64 */
> +
> +#ifdef CONFIG_PPC_FSL_BOOK3E
> +void setup_barrier_nospec(void)
> +{
> + if (!no_nospec)
> + enable_barrier_nospec(true);
> +}
> +#endif /* CONFIG_PPC_FSL_BOOK3E */

eg. that is identical to the existing version except for the feature check:

enable = security_ftr_enabled(SEC_FTR_FAVOUR_SECURITY) &&
 security_ftr_enabled(SEC_FTR_BNDS_CHK_SPEC_BAR);


Both those bits are enabled by default, so you shouldn't need to elide
that check.

So basically you should be able to use the existing
setup_barrier_nospec() if you just remove the ifdef around
powerpc_security_features. Or am I missing something?


> diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
> index 7445748..80c1e6e 100644
> --- a/arch/powerpc/kernel/setup_32.c
> +++ b/arch/powerpc/kernel/setup_32.c
> @@ -116,6 +116,11 @@ notrace void __init machine_init(u64 dt_ptr)
>   /* Do some early initialization based on the flat device tree */
>   early_init_devtree(__va(dt_ptr));
>  
> + /* Apply the speculation barrier fixup */
> +#ifdef CONFIG_PPC_FSL_BOOK3E
> + setup_barrier_nospec();
> +#endif

[PATCH v2 2/3] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book3E

2018-06-11 Thread Diana Craciun

Implement the barrier_nospec as a isync;sync instruction sequence.
The implementation uses the infrastructure built for BOOK3S 64.

Signed-off-by: Diana Craciun 
---
 arch/powerpc/include/asm/barrier.h | 10 ++
 arch/powerpc/include/asm/setup.h   |  2 +-
 arch/powerpc/kernel/Makefile   |  2 +-
 arch/powerpc/kernel/module.c   |  5 +++--
 arch/powerpc/kernel/security.c | 15 +++
 arch/powerpc/kernel/setup_32.c |  5 +
 arch/powerpc/kernel/setup_64.c |  6 ++
 arch/powerpc/kernel/vmlinux.lds.S  |  4 +++-
 arch/powerpc/lib/feature-fixups.c  | 35 ++-
 9 files changed, 78 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/include/asm/barrier.h 
b/arch/powerpc/include/asm/barrier.h
index f67b3f6..405d572 100644
--- a/arch/powerpc/include/asm/barrier.h
+++ b/arch/powerpc/include/asm/barrier.h
@@ -86,6 +86,16 @@ do { 
\
 // This also acts as a compiler barrier due to the memory clobber.
 #define barrier_nospec() asm (stringify_in_c(barrier_nospec_asm) ::: "memory")
 
+#elif defined(CONFIG_PPC_FSL_BOOK3E)
+/*
+ * Prevent the execution of subsequent instructions speculatively using a
+ * isync;sync instruction sequence.
+ */
+#define barrier_nospec_asm NOSPEC_BARRIER_FIXUP_SECTION; nop; nop
+
+// This also acts as a compiler barrier due to the memory clobber.
+#define barrier_nospec() asm (stringify_in_c(barrier_nospec_asm) ::: "memory")
+
 #else /* !CONFIG_PPC_BOOK3S_64 */
 #define barrier_nospec_asm
 #define barrier_nospec()
diff --git a/arch/powerpc/include/asm/setup.h b/arch/powerpc/include/asm/setup.h
index 8721fd0..67a2810 100644
--- a/arch/powerpc/include/asm/setup.h
+++ b/arch/powerpc/include/asm/setup.h
@@ -56,7 +56,7 @@ void setup_barrier_nospec(void);
 void do_barrier_nospec_fixups(bool enable);
 extern bool barrier_nospec_enabled;
 
-#ifdef CONFIG_PPC_BOOK3S_64
+#if defined(CONFIG_PPC_BOOK3S_64) || defined(CONFIG_PPC_FSL_BOOK3E)
 void do_barrier_nospec_fixups_range(bool enable, void *start, void *end);
 #else
 static inline void do_barrier_nospec_fixups_range(bool enable, void *start, 
void *end) { };
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 2b4c40b2..d9dee43 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -76,7 +76,7 @@ endif
 obj64-$(CONFIG_HIBERNATION)+= swsusp_asm64.o
 obj-$(CONFIG_MODULES)  += module.o module_$(BITS).o
 obj-$(CONFIG_44x)  += cpu_setup_44x.o
-obj-$(CONFIG_PPC_FSL_BOOK3E)   += cpu_setup_fsl_booke.o
+obj-$(CONFIG_PPC_FSL_BOOK3E)   += cpu_setup_fsl_booke.o security.o
 obj-$(CONFIG_PPC_DOORBELL) += dbell.o
 obj-$(CONFIG_JUMP_LABEL)   += jump_label.o
 
diff --git a/arch/powerpc/kernel/module.c b/arch/powerpc/kernel/module.c
index 1b3c683..96a9821 100644
--- a/arch/powerpc/kernel/module.c
+++ b/arch/powerpc/kernel/module.c
@@ -72,13 +72,14 @@ int module_finalize(const Elf_Ehdr *hdr,
do_feature_fixups(powerpc_firmware_features,
  (void *)sect->sh_addr,
  (void *)sect->sh_addr + sect->sh_size);
-
+#endif /* CONFIG_PPC64 */
+#if defined(CONFIG_PPC64) || defined(CONFIG_PPC_FSL_BOOK3E)
sect = find_section(hdr, sechdrs, "__spec_barrier_fixup");
if (sect != NULL)
do_barrier_nospec_fixups_range(barrier_nospec_enabled,
  (void *)sect->sh_addr,
  (void *)sect->sh_addr + sect->sh_size);
-#endif
+#endif /* CONFIG_PPC64 || CONFIG_PPC_FSL_BOOK3E */
 
sect = find_section(hdr, sechdrs, "__lwsync_fixup");
if (sect != NULL)
diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
index c55e102..797c975 100644
--- a/arch/powerpc/kernel/security.c
+++ b/arch/powerpc/kernel/security.c
@@ -13,7 +13,9 @@
 #include 
 
 
+#ifdef CONFIG_PPC_BOOK3S_64
 unsigned long powerpc_security_features __read_mostly = SEC_FTR_DEFAULT;
+#endif /* CONFIG_PPC_BOOK3S_64 */
 
 bool barrier_nospec_enabled;
 static bool no_nospec;
@@ -24,6 +26,7 @@ static void enable_barrier_nospec(bool enable)
do_barrier_nospec_fixups(enable);
 }
 
+#ifdef CONFIG_PPC_BOOK3S_64
 void setup_barrier_nospec(void)
 {
bool enable;
@@ -46,6 +49,15 @@ void setup_barrier_nospec(void)
if (!no_nospec)
enable_barrier_nospec(enable);
 }
+#endif /* CONFIG_PPC_BOOK3S_64 */
+
+#ifdef CONFIG_PPC_FSL_BOOK3E
+void setup_barrier_nospec(void)
+{
+   if (!no_nospec)
+   enable_barrier_nospec(true);
+}
+#endif /* CONFIG_PPC_FSL_BOOK3E */
 
 static int __init handle_nospectre_v1(char *p)
 {
@@ -92,6 +104,7 @@ static __init int barrier_nospec_debugfs_init(void)
 device_initcall(barrier_nospec_debugfs_init);
 #endif /* CONFIG_DEBUG_FS */
 
+#ifdef CONFIG_PPC_BOOK3S_64
 ssize_t cpu_show_meltdown(struct device *dev, struct device_attribute *attr, 
char *buf)
 {

Re: [PATCH v2 2/3] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book3E

Re: [PATCH v2 2/3] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book3E

[PATCH v2 2/3] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book3E

3 matches

Site Navigation

Mail list logo

Footer information