[PATCH v2 0/3] powerpc/fsl: Speculation barrier for NXP PowerPC Book3E

Implement barrier_nospec for NXP PowerPC Book3E processors. 

Diana Craciun (3):
  Disable the speculation barrier from the command line
  Add barrier_nospec implementation for NXP PowerPC Book3E
  Implement cpu_show_spectre_v1/v2 for NXP PowerPC Book3E

 arch/powerpc/Kconfig               |  2 +-
 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     | 42 +++++++++++++++++++++++++++++++++++++-
 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 ++++++++++++++++++++++++++++++-
 10 files changed, 105 insertions(+), 8 deletions(-)

--
History:

v1 --> v2
- added implementation for cpu_show_spectre_x functions
- the mitigation is no longer enabled through device tree options
2.5.5

The speculation barrier can be disabled from the command line
with the parameter: "nospectre_v1".

Signed-off-by: Diana Craciun <redacted>
---
 arch/powerpc/kernel/security.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
index 3eb9c45..c55e102 100644
--- a/arch/powerpc/kernel/security.c
+++ b/arch/powerpc/kernel/security.c

@@ -16,6 +16,7 @@
 unsigned long powerpc_security_features __read_mostly = SEC_FTR_DEFAULT;
 
 bool barrier_nospec_enabled;
+static bool no_nospec;
 
 static void enable_barrier_nospec(bool enable)
 {

@@ -42,9 +43,18 @@ void setup_barrier_nospec(void)
 	enable = security_ftr_enabled(SEC_FTR_FAVOUR_SECURITY) &&
 		 security_ftr_enabled(SEC_FTR_BNDS_CHK_SPEC_BAR);
 
-	enable_barrier_nospec(enable);
+	if (!no_nospec)
+		enable_barrier_nospec(enable);
 }
 
+static int __init handle_nospectre_v1(char *p)
+{
+	no_nospec = true;
+
+	return 0;
+}
+early_param("nospectre_v1", handle_nospectre_v1);
+
 #ifdef CONFIG_DEBUG_FS
 static int barrier_nospec_set(void *data, u64 val)
 {

-- 
2.5.5

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

Signed-off-by: Diana Craciun <redacted>
---
 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 <asm/setup.h>
 
 
+#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)
 {
 	bool thread_priv;

@@ -168,3 +181,5 @@ ssize_t cpu_show_spectre_v2(struct device *dev, struct device_attribute *attr, c
 
 	return s.len;
 }
+#endif /* CONFIG_PPC_BOOK3S_64 */
+

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
+
 	early_init_mmu();
 
 	setup_kdump_trampoline();

diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 7a7ce8a..b2a644a 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c

@@ -327,6 +327,12 @@ void __init early_setup(unsigned long dt_ptr)
 
 	/* Apply all the dynamic patching */
 	apply_feature_fixups();
+
+	/* Apply the speculation barrier fixup */
+#ifdef CONFIG_PPC_FSL_BOOK3E
+	setup_barrier_nospec();
+#endif /* CONFIG_PPC_FSL_BOOK3E */
+
 	setup_feature_keys();
 
 	/* Initialize the hash table or TLB handling */

diff --git a/arch/powerpc/kernel/vmlinux.lds.S b/arch/powerpc/kernel/vmlinux.lds.S
index ff73f49..af513e6 100644
--- a/arch/powerpc/kernel/vmlinux.lds.S
+++ b/arch/powerpc/kernel/vmlinux.lds.S

@@ -139,14 +139,16 @@ SECTIONS
 		*(__rfi_flush_fixup)
 		__stop___rfi_flush_fixup = .;
 	}
+#endif /* CONFIG_PPC64 */
 
+#if defined(CONFIG_PPC64) || defined(CONFIG_PPC_FSL_BOOK3E)
 	. = ALIGN(8);
 	__spec_barrier_fixup : AT(ADDR(__spec_barrier_fixup) - LOAD_OFFSET) {
 		__start___barrier_nospec_fixup = .;
 		*(__barrier_nospec_fixup)
 		__stop___barrier_nospec_fixup = .;
 	}
-#endif
+#endif /* CONFIG_PPC64 || CONFIG_PPC_FSL_BOOK3E */
 
 	EXCEPTION_TABLE(0)
 

diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-fixups.c
index 2b9173d..bea2b87 100644
--- a/arch/powerpc/lib/feature-fixups.c
+++ b/arch/powerpc/lib/feature-fixups.c

@@ -188,7 +188,40 @@ void do_barrier_nospec_fixups_range(bool enable, void *fixup_start, void *fixup_
 
 	printk(KERN_DEBUG "barrier-nospec: patched %d locations\n", i);
 }
+#endif /* CONFIG_PPC_BOOK3S_64 */
+
+#ifdef CONFIG_PPC_FSL_BOOK3E
+void do_barrier_nospec_fixups_range(bool enable, void *fixup_start, void *fixup_end)
+{
+	unsigned int instr[2], *dest;
+	long *start, *end;
+	int i;
+
+	start = fixup_start;
+	end = fixup_end;
+
+	instr[0] = PPC_INST_NOP;
+	instr[1] = PPC_INST_NOP;
+
+	if (enable) {
+		pr_info("barrier_nospec; using isync; sync as a speculation barrier\n");
+		instr[0] = PPC_INST_ISYNC;
+		instr[1] = PPC_INST_SYNC;
+	}
+
+	for (i = 0; start < end; start++, i++) {
+		dest = (void *)start + *start;
+		pr_devel("patching dest %lx\n", (unsigned long)dest);
 
+		patch_instruction(dest, instr[0]);
+		patch_instruction(dest + 1, instr[1]);
+	}
+
+	pr_debug("barrier-nospec: patched %d locations\n", i);
+}
+#endif /* CONFIG_PPC_FSL_BOOK3E */
+
+#if defined(CONFIG_PPC_BOOK3S_64) || defined(CONFIG_PPC_FSL_BOOK3E)
 void do_barrier_nospec_fixups(bool enable)
 {
 	void *start, *end;

@@ -199,7 +232,7 @@ void do_barrier_nospec_fixups(bool enable)
 	do_barrier_nospec_fixups_range(enable, start, end);
 }
 
-#endif /* CONFIG_PPC_BOOK3S_64 */
+#endif /* CONFIG_PPC_BOOK3S_64 || CONFIG_PPC_FSL_BOOK3E */
 
 void do_lwsync_fixups(unsigned long value, void *fixup_start, void *fixup_end)
 {

-- 
2.5.5

Signed-off-by: Diana Craciun <redacted>
---
 arch/powerpc/Kconfig           |  2 +-
 arch/powerpc/kernel/security.c | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 940c955..a781d60 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig

@@ -170,7 +170,7 @@ config PPC
 	select GENERIC_CLOCKEVENTS_BROADCAST	if SMP
 	select GENERIC_CMOS_UPDATE
 	select GENERIC_CPU_AUTOPROBE
-	select GENERIC_CPU_VULNERABILITIES	if PPC_BOOK3S_64
+	select GENERIC_CPU_VULNERABILITIES	if PPC_BOOK3S_64 || PPC_FSL_BOOK3E
 	select GENERIC_IRQ_SHOW
 	select GENERIC_IRQ_SHOW_LEVEL
 	select GENERIC_SMP_IDLE_THREAD

diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
index 797c975..aceaadc 100644
--- a/arch/powerpc/kernel/security.c
+++ b/arch/powerpc/kernel/security.c

@@ -183,3 +183,18 @@ ssize_t cpu_show_spectre_v2(struct device *dev, struct device_attribute *attr, c
 }
 #endif /* CONFIG_PPC_BOOK3S_64 */
 
+#ifdef CONFIG_PPC_FSL_BOOK3E
+ssize_t cpu_show_spectre_v1(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	if (barrier_nospec_enabled)
+		return sprintf(buf, "Mitigation: __user pointer sanitization\n");
+
+	return sprintf(buf, "Vulnerable\n");
+}
+
+ssize_t cpu_show_spectre_v2(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "Vulnerable\n");
+}
+#endif /* CONFIG_PPC_FSL_BOOK3E */
+

-- 
2.5.5

Hi Diana,

-----Original Message-----
From: Diana Craciun [mailto:diana.craciun@nxp.com]
Sent: Monday, June 11, 2018 6:23 PM
To: linuxppc-dev@lists.ozlabs.org
Cc: mpe@ellerman.id.au; oss@buserror.net; Leo Li <redacted>;
Bharat Bhushan [off-list ref]; Diana Madalina Craciun
[off-list ref]
Subject: [PATCH v2 3/3] powerpc/fsl: Implement cpu_show_spectre_v1/v2 for
NXP PowerPC Book3E

Please add some description

y.c

On Tue, 12 Jun 2018 02:59:11 +0000
Bharat Bhushan [off-list ref] wrote:

Hi Diana,

quoted

-----Original Message-----
From: Diana Craciun [mailto:diana.craciun@nxp.com]
Sent: Monday, June 11, 2018 6:23 PM
To: linuxppc-dev@lists.ozlabs.org
Cc: mpe@ellerman.id.au; oss@buserror.net; Leo Li
[off-list ref]; Bharat Bhushan [off-list ref];
Diana Madalina Craciun [off-list ref]
Subject: [PATCH v2 3/3] powerpc/fsl: Implement
cpu_show_spectre_v1/v2 for NXP PowerPC Book3E  

Please add some description

To me the subject is self-explanatory. It implements a kernel interface
that was already described elsewhere.

What are you missing here?

Thanks

Michal

quoted

Signed-off-by: Diana Craciun <redacted>
---
 arch/powerpc/Kconfig           |  2 +-
 arch/powerpc/kernel/security.c | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index

940c955..a781d60 100644

--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig

@@ -170,7 +170,7 @@ config PPC
 	select GENERIC_CLOCKEVENTS_BROADCAST	if SMP
 	select GENERIC_CMOS_UPDATE
 	select GENERIC_CPU_AUTOPROBE
-	select GENERIC_CPU_VULNERABILITIES	if PPC_BOOK3S_64
+	select GENERIC_CPU_VULNERABILITIES	if PPC_BOOK3S_64

|| PPC_FSL_BOOK3E
 	select GENERIC_IRQ_SHOW
 	select GENERIC_IRQ_SHOW_LEVEL
 	select GENERIC_SMP_IDLE_THREAD

diff --git a/arch/powerpc/kernel/security.c

b/arch/powerpc/kernel/security.c index 797c975..aceaadc 100644

--- a/arch/powerpc/kernel/security.c
+++ b/arch/powerpc/kernel/security.c

@@ -183,3 +183,18 @@ ssize_t cpu_show_spectre_v2(struct device *dev,

struct device_attribute *attr, c  }  #endif /* CONFIG_PPC_BOOK3S_64
*/

+#ifdef CONFIG_PPC_FSL_BOOK3E
+ssize_t cpu_show_spectre_v1(struct device *dev, struct
device_attribute +*attr, char *buf) {
+	if (barrier_nospec_enabled)
+		return sprintf(buf, "Mitigation: __user pointer
sanitization\n"); +
+	return sprintf(buf, "Vulnerable\n");
+}
+
+ssize_t cpu_show_spectre_v2(struct device *dev, struct
device_attribute +*attr, char *buf) {
+	return sprintf(buf, "Vulnerable\n");
+}
+#endif /* CONFIG_PPC_FSL_BOOK3E */
+
--
2.5.5  

Hi,=0A=
=0A=
Should I rebase the series on top of the latest kernel?=0A=
=0A=
Diana=0A=
=0A=
On 6/11/2018 3:53 PM, Diana Craciun wrote:=0A=

Implement barrier_nospec for NXP PowerPC Book3E processors. =0A=
=0A=
Diana Craciun (3):=0A=
  Disable the speculation barrier from the command line=0A=
  Add barrier_nospec implementation for NXP PowerPC Book3E=0A=
  Implement cpu_show_spectre_v1/v2 for NXP PowerPC Book3E=0A=
=0A=
 arch/powerpc/Kconfig               |  2 +-=0A=
 arch/powerpc/include/asm/barrier.h | 10 +++++++++=0A=
 arch/powerpc/include/asm/setup.h   |  2 +-=0A=
 arch/powerpc/kernel/Makefile       |  2 +-=0A=
 arch/powerpc/kernel/module.c       |  5 +++--=0A=
 arch/powerpc/kernel/security.c     | 42 ++++++++++++++++++++++++++++++++=

+++++-=0A=

 arch/powerpc/kernel/setup_32.c     |  5 +++++=0A=
 arch/powerpc/kernel/setup_64.c     |  6 ++++++=0A=
 arch/powerpc/kernel/vmlinux.lds.S  |  4 +++-=0A=
 arch/powerpc/lib/feature-fixups.c  | 35 ++++++++++++++++++++++++++++++-=

=0A=

 10 files changed, 105 insertions(+), 8 deletions(-)=0A=
=0A=
--=0A=
History:=0A=
=0A=
v1 --> v2=0A=
- added implementation for cpu_show_spectre_x functions=0A=
- the mitigation is no longer enabled through device tree options=0A=
2.5.5=0A=
=0A=
=0A=

=0A=

Diana Craciun [off-list ref] writes:

The speculation barrier can be disabled from the command line
with the parameter: "nospectre_v1".

Can you please send another patch adding it to Documentation/admin-guide/kernel-parameters.txt

You should mark it as "PPC" only for now.

cheers

Michal Such=C3=A1nek [off-list ref] writes:

On Tue, 12 Jun 2018 02:59:11 +0000
Bharat Bhushan [off-list ref] wrote:

quoted

Hi Diana,
=20

quoted

-----Original Message-----
From: Diana Craciun [mailto:diana.craciun@nxp.com]
Sent: Monday, June 11, 2018 6:23 PM
To: linuxppc-dev@lists.ozlabs.org
Cc: mpe@ellerman.id.au; oss@buserror.net; Leo Li
[off-list ref]; Bharat Bhushan [off-list ref];
Diana Madalina Craciun [off-list ref]
Subject: [PATCH v2 3/3] powerpc/fsl: Implement
cpu_show_spectre_v1/v2 for NXP PowerPC Book3E=20=20

=20
Please add some description

To me the subject is self-explanatory. It implements a kernel interface
that was already described elsewhere.

What are you missing here?

It should at least explain why it's reimplementing a function that
already exists for powerpc. ie. Why can't the existing version be used?

cheers

Hi Diana,

A few comments below ...

Diana Craciun [off-list ref] 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.

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 */

Can we instead do:

obj-$(CONFIG_PPC_NOSPEC)	+= security.o

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().

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?

This ifdef should be handled in a header with an empty version for the
#else case.

Can you call it from ppc_md->setup_arch() like the other platforms?

Failing that we could put it in setup_arch() after the call to
ppc_md->setup_arch(), so we can share it with powernv/pseries.

It's a bit unfortunate that we end up with two versions of that, which
are 80% the same. But merging them without ugly ifdefs would require a
bit more refactoring. So I guess it's OK for now.

cheers

Hi Michael,=0A=
=0A=
Thank you for the review.=0A=
=0A=
On 07/03/2018 10:26 AM, Michael Ellerman wrote:=0A=

Hi Diana,=0A=
=0A=
A few comments below ...=0A=
=0A=
Diana Craciun [off-list ref] writes:=0A=

quoted

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

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

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

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

=0A=
OK.=0A=
=0A=

=0A=

quoted

diff --git a/arch/powerpc/include/asm/barrier.h b/arch/powerpc/include/a=

sm/barrier.h=0A=

quoted

index f67b3f6..405d572 100644=0A=

--- a/arch/powerpc/include/asm/barrier.h=0A=
+++ b/arch/powerpc/include/asm/barrier.h=0A=

@@ -86,6 +86,16 @@ do {									\=0A=
 // This also acts as a compiler barrier due to the memory clobber.=0A=
 #define barrier_nospec() asm (stringify_in_c(barrier_nospec_asm) ::: "m=

emory")=0A=

quoted

 =0A=
+#elif defined(CONFIG_PPC_FSL_BOOK3E)=0A=
+/*=0A=
+ * Prevent the execution of subsequent instructions speculatively using=

 a=0A=

quoted

+ * isync;sync instruction sequence.=0A=
+ */=0A=
+#define barrier_nospec_asm NOSPEC_BARRIER_FIXUP_SECTION; nop; nop=0A=
+=0A=
+// This also acts as a compiler barrier due to the memory clobber.=0A=
+#define barrier_nospec() asm (stringify_in_c(barrier_nospec_asm) ::: "m=

emory")=0A=

quoted

+=0A=
 #else /* !CONFIG_PPC_BOOK3S_64 */=0A=
 #define barrier_nospec_asm=0A=
 #define barrier_nospec()=0A=

If we have CONFIG_PPC_NOSPEC this can be done something like:=0A=
=0A=
#ifdef CONFIG_PPC_BOOK3S_64=0A=
#define NOSPEC_BARRIER_SLOT	nop=0A=
#elif defined(CONFIG_PPC_FSL_BOOK3E)=0A=
#define NOSPEC_BARRIER_SLOT	nop; nop=0A=
#endif /* CONFIG_PPC_BOOK3S_64 */=0A=
=0A=
#ifdef CONFIG_PPC_NOSPEC=0A=
/*=0A=
 * Prevent execution of subsequent instructions until preceding branches =

have=0A=

 * been fully resolved and are no longer executing speculatively.=0A=
 */=0A=
#define barrier_nospec_asm NOSPEC_BARRIER_FIXUP_SECTION; NOSPEC_BARRIER_S=

LOT=0A=

=0A=
// This also acts as a compiler barrier due to the memory clobber.=0A=
#define barrier_nospec() asm (stringify_in_c(barrier_nospec_asm) ::: "mem=

ory")=0A=

#else=0A=
#define barrier_nospec_asm=0A=
#define barrier_nospec()=0A=
#endif /* CONFIG_PPC_NOSPEC */=0A=

=0A=
OK.=0A=
=0A=

=0A=
=0A=

quoted

diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile=

=0A=

quoted

index 2b4c40b2..d9dee43 100644=0A=

--- a/arch/powerpc/kernel/Makefile=0A=
+++ b/arch/powerpc/kernel/Makefile=0A=

@@ -76,7 +76,7 @@ endif=0A=
 obj64-$(CONFIG_HIBERNATION)	+=3D swsusp_asm64.o=0A=
 obj-$(CONFIG_MODULES)		+=3D module.o module_$(BITS).o=0A=
 obj-$(CONFIG_44x)		+=3D cpu_setup_44x.o=0A=
-obj-$(CONFIG_PPC_FSL_BOOK3E)	+=3D cpu_setup_fsl_booke.o=0A=
+obj-$(CONFIG_PPC_FSL_BOOK3E)	+=3D cpu_setup_fsl_booke.o security.o=0A=
 obj-$(CONFIG_PPC_DOORBELL)	+=3D dbell.o=0A=
 obj-$(CONFIG_JUMP_LABEL)	+=3D jump_label.o=0A=

Can we instead do:=0A=
=0A=
obj-$(CONFIG_PPC_NOSPEC)	+=3D security.o=0A=

=0A=
OK=0A=
=0A=

=0A=

quoted

diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/securi=

ty.c=0A=

quoted

index c55e102..797c975 100644=0A=

--- a/arch/powerpc/kernel/security.c=0A=
+++ b/arch/powerpc/kernel/security.c=0A=

@@ -13,7 +13,9 @@=0A=
 #include <asm/setup.h>=0A=
 =0A=
 =0A=
+#ifdef CONFIG_PPC_BOOK3S_64=0A=
 unsigned long powerpc_security_features __read_mostly =3D SEC_FTR_DEFAU=

LT;=0A=

quoted

+#endif /* CONFIG_PPC_BOOK3S_64 */=0A=

Why are you making that book3s specific?=0A=
=0A=
By doing that you then can't use the existing versions of=0A=
setup_barrier_nospec() and cpu_show_spectre_v1/v2().=0A=
=0A=

quoted

@@ -24,6 +26,7 @@ static void enable_barrier_nospec(bool enable)=0A=
 	do_barrier_nospec_fixups(enable);=0A=
 }=0A=
 =0A=
+#ifdef CONFIG_PPC_BOOK3S_64=0A=
 void setup_barrier_nospec(void)=0A=
 {=0A=
 	bool enable;=0A=

@@ -46,6 +49,15 @@ void setup_barrier_nospec(void)=0A=
 	if (!no_nospec)=0A=
 		enable_barrier_nospec(enable);=0A=
 }=0A=
+#endif /* CONFIG_PPC_BOOK3S_64 */=0A=
+=0A=
+#ifdef CONFIG_PPC_FSL_BOOK3E=0A=
+void setup_barrier_nospec(void)=0A=
+{=0A=
+	if (!no_nospec)=0A=
+		enable_barrier_nospec(true);=0A=
+}=0A=
+#endif /* CONFIG_PPC_FSL_BOOK3E */=0A=

eg. that is identical to the existing version except for the feature chec=

k:=0A=

=0A=
	enable =3D security_ftr_enabled(SEC_FTR_FAVOUR_SECURITY) &&=0A=
		 security_ftr_enabled(SEC_FTR_BNDS_CHK_SPEC_BAR);=0A=
=0A=
=0A=
Both those bits are enabled by default, so you shouldn't need to elide=0A=
that check.=0A=
=0A=
So basically you should be able to use the existing=0A=
setup_barrier_nospec() if you just remove the ifdef around=0A=
powerpc_security_features. Or am I missing something?=0A=

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

=0A=
=0A=

quoted

diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_=

32.c=0A=

quoted

index 7445748..80c1e6e 100644=0A=

--- a/arch/powerpc/kernel/setup_32.c=0A=
+++ b/arch/powerpc/kernel/setup_32.c=0A=

@@ -116,6 +116,11 @@ notrace void __init machine_init(u64 dt_ptr)=0A=
 	/* Do some early initialization based on the flat device tree */=0A=
 	early_init_devtree(__va(dt_ptr));=0A=
 =0A=
+	/* Apply the speculation barrier fixup */=0A=
+#ifdef CONFIG_PPC_FSL_BOOK3E=0A=
+	setup_barrier_nospec();=0A=
+#endif=0A=

This ifdef should be handled in a header with an empty version for the=0A=
#else case.=0A=

=0A=
OK=0A=
=0A=

=0A=

quoted

diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_=

64.c=0A=

quoted

index 7a7ce8a..b2a644a 100644=0A=

--- a/arch/powerpc/kernel/setup_64.c=0A=
+++ b/arch/powerpc/kernel/setup_64.c=0A=

@@ -327,6 +327,12 @@ void __init early_setup(unsigned long dt_ptr)=0A=
 =0A=
 	/* Apply all the dynamic patching */=0A=
 	apply_feature_fixups();=0A=
+=0A=
+	/* Apply the speculation barrier fixup */=0A=
+#ifdef CONFIG_PPC_FSL_BOOK3E=0A=
+	setup_barrier_nospec();=0A=
+#endif /* CONFIG_PPC_FSL_BOOK3E */=0A=

Can you call it from ppc_md->setup_arch() like the other platforms?=0A=
=0A=
Failing that we could put it in setup_arch() after the call to=0A=
ppc_md->setup_arch(), so we can share it with powernv/pseries.=0A=

=0A=
The reason for which I did not call it from the ppc_md->setup_arch() was=0A=
that from my point of view the mitigation is not specific to the=0A=
platform code, but rather to the CPU.=0A=
=0A=

=0A=

quoted

diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/featur=

e-fixups.c=0A=

quoted

index 2b9173d..bea2b87 100644=0A=

--- a/arch/powerpc/lib/feature-fixups.c=0A=
+++ b/arch/powerpc/lib/feature-fixups.c=0A=

@@ -188,7 +188,40 @@ void do_barrier_nospec_fixups_range(bool enable, vo=

id *fixup_start, void *fixup_=0A=

quoted

 =0A=
 	printk(KERN_DEBUG "barrier-nospec: patched %d locations\n", i);=0A=
 }=0A=
+#endif /* CONFIG_PPC_BOOK3S_64 */=0A=
+=0A=
+#ifdef CONFIG_PPC_FSL_BOOK3E=0A=
+void do_barrier_nospec_fixups_range(bool enable, void *fixup_start, voi=

d *fixup_end)=0A=

quoted

+{=0A=
+	unsigned int instr[2], *dest;=0A=
+	long *start, *end;=0A=
+	int i;=0A=
+=0A=
+	start =3D fixup_start;=0A=
+	end =3D fixup_end;=0A=
+=0A=
+	instr[0] =3D PPC_INST_NOP;=0A=
+	instr[1] =3D PPC_INST_NOP;=0A=
+=0A=
+	if (enable) {=0A=
+		pr_info("barrier_nospec; using isync; sync as a speculation barrier\n=

");=0A=

quoted

+		instr[0] =3D PPC_INST_ISYNC;=0A=
+		instr[1] =3D PPC_INST_SYNC;=0A=
+	}=0A=
+=0A=
+	for (i =3D 0; start < end; start++, i++) {=0A=
+		dest =3D (void *)start + *start;=0A=
+		pr_devel("patching dest %lx\n", (unsigned long)dest);=0A=
 =0A=
+		patch_instruction(dest, instr[0]);=0A=
+		patch_instruction(dest + 1, instr[1]);=0A=
+	}=0A=
+=0A=
+	pr_debug("barrier-nospec: patched %d locations\n", i);=0A=
+}=0A=
+#endif /* CONFIG_PPC_FSL_BOOK3E */=0A=

It's a bit unfortunate that we end up with two versions of that, which=0A=
are 80% the same. But merging them without ugly ifdefs would require a=0A=
bit more refactoring. So I guess it's OK for now.=0A=
=0A=
cheers=0A=
=0A=

Regards,=0A=
=0A=
Diana=0A=
=0A=

On 07/03/2018 10:26 AM, Michael Ellerman wrote:=0A=

Michal Such=E1nek [off-list ref] writes:=0A=

quoted

On Tue, 12 Jun 2018 02:59:11 +0000=0A=
Bharat Bhushan [off-list ref] wrote:=0A=
=0A=

quoted

Hi Diana,=0A=
=0A=

quoted

-----Original Message-----=0A=
From: Diana Craciun [mailto:diana.craciun@nxp.com]=0A=
Sent: Monday, June 11, 2018 6:23 PM=0A=
To: linuxppc-dev@lists.ozlabs.org=0A=
Cc: mpe@ellerman.id.au; oss@buserror.net; Leo Li=0A=
[off-list ref]; Bharat Bhushan [off-list ref];=0A=
Diana Madalina Craciun [off-list ref]=0A=
Subject: [PATCH v2 3/3] powerpc/fsl: Implement=0A=
cpu_show_spectre_v1/v2 for NXP PowerPC Book3E  =0A=

Please add some description=0A=

To me the subject is self-explanatory. It implements a kernel interface=

=0A=

quoted

that was already described elsewhere.=0A=
=0A=
What are you missing here?=0A=

It should at least explain why it's reimplementing a function that=0A=
already exists for powerpc. ie. Why can't the existing version be used?=

=0A=

=0A=
cheers=0A=
=0A=

OK. I think I can use the cpu_show_spectre_v1 and for now I can use=0A=
cpu_show_spectre_v2 as well (the patches are under development for=0A=
mitigating Spectre v2). But I cannot use cpu_show_meltdown because it=0A=
uses references to variables that are specific to BOOK3S_64. But I do=0A=
not need a special implementation for cpu_show_meltdown because our=0A=
platform is not vulnerable to Meltdown. So, I will just ifdef the=0A=
cpu_show_meltdown and leave the default implementation.=0A=
=0A=
Diana=0A=
=0A=
=0A=
=0A=
=0A=