Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E

Hi Scott,=0A=
=0A=
Thanks for the review.=0A=
=0A=
On 05/22/2018 11:31 PM, Scott Wood wrote:=0A=

On Tue, 2018-05-22 at 10:10 +0300, Diana Craciun wrote:=0A=

quoted

Implement the barrier_nospec as a isync;sync instruction sequence.=0A=
The implementation uses the infrastructure built for BOOK3S 64=0A=
with the difference that for NXP platforms there is no firmware involved=

=0A=

quoted

and the need for a speculation barrier is read from the device tree.=0A=
I have used the same name for the property:=0A=
fsl,needs-spec-barrier-for-bounds-check=0A=

Using the device tree this way means that anyone without an updated devic=

e=0A=

tree won't get the protection.  I also don't see any device tree updates =

--=0A=

which chips are affected?=0A=

=0A=
I was planning to have the device tree changes in a different patch-set.=0A=
The affected cores are e500, e500mc, e5500, e6500.=0A=
=0A=

  Wouldn't it be more robust to just have the kernel=0A=
check the CPU type, especially given that it already does so for a lot of=

=0A=

other purposes?=0A=

=0A=
Yes, I think that it might be a better solution not to use the device=0A=
tree at all.=0A=
=0A=

=0A=

quoted

+#ifdef CONFIG_PPC_BOOK3S_64=0A=
 void setup_barrier_nospec(void)=0A=
 {=0A=
 	bool enable;=0A=

@@ -44,6 +46,12 @@ void setup_barrier_nospec(void)=0A=
 =0A=
 	enable_barrier_nospec(enable);=0A=
 }=0A=
+#elif CONFIG_PPC_FSL_BOOK3E=0A=
+void setup_barrier_nospec(void)=0A=
+{=0A=
+	enable_barrier_nospec(true);=0A=
+}=0A=
+#endif=0A=

The call site is in FSL_BOOK3E-specific code so why not call=0A=
enable_barrier_nospec() directly from there?=0A=

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

  =0A=

quoted

+#ifdef CONFIG_PPC_BOOK3S_64=0A=
 ssize_t cpu_show_meltdown(struct device *dev, struct device_attribute=

=0A=

quoted

*attr, char *buf)=0A=
 {=0A=
 	bool thread_priv;=0A=

@@ -155,3 +164,4 @@ ssize_t cpu_show_spectre_v2(struct device *dev, stru=

ct=0A=

quoted

device_attribute *attr, c=0A=
 =0A=
 	return s.len;=0A=
 }=0A=
+#endif=0A=

No equivalent of this for FSL?=0A=

=0A=
There will be an equivalent for this for FSL as well. I was planning to=0A=
send a different patch for this.=0A=
=0A=

=0A=

quoted

+#ifdef CONFIG_PPC_FSL_BOOK3E=0A=
+void do_barrier_nospec_fixups_range(bool enable, void *fixup_start, voi=

d=0A=

quoted

*fixup_end)=0A=
+{=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; /* nop */=0A=
+	instr[1] =3D PPC_INST_NOP; /* nop */=0A=
+=0A=
+	if (enable) {=0A=
+		pr_info("barrier_nospec: using isync; sync as a speculation=0A=
barrier\n");=0A=
+		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=
+=0A=
+	pr_debug("barrier-nospec: patched %d locations\n", i);=0A=

Why patch nops in if not enabled?  Aren't those locations already nops?  =

For=0A=

that matter, how can this function even be called on FSL_BOOK3E with enab=

le !=3D=0A=

true?=0A=

=0A=
There is some code in arch/powerpc/kernel/security.c which allows=0A=
control of barrier_nospec via debugfs.=0A=
=0A=

=0A=

quoted

diff --git a/arch/powerpc/platforms/85xx/corenet_generic.c=0A=

b/arch/powerpc/platforms/85xx/corenet_generic.c=0A=
index ac191a7..11bce3d 100644=0A=

--- a/arch/powerpc/platforms/85xx/corenet_generic.c=0A=
+++ b/arch/powerpc/platforms/85xx/corenet_generic.c=0A=

@@ -59,6 +59,21 @@ void __init corenet_gen_pic_init(void)=0A=
 	}=0A=
 }=0A=
 =0A=
+static void setup_spec_barrier(void)=0A=
+{=0A=
+	struct device_node *np =3D of_find_node_by_name(NULL, "cpus");=0A=
+=0A=
+	if (np) {=0A=
+		struct property *prop;=0A=
+=0A=
+		prop =3D of_find_property(np,=0A=
+		       "fsl,needs-spec-barrier-for-bounds-check", NULL);=0A=
+		if (prop)=0A=
+			setup_barrier_nospec();=0A=
+		of_node_put(np);=0A=
+	}=0A=
+}=0A=

Why is this in board code?=0A=

=0A=
I will move it away from the board code.=0A=
=0A=

=0A=
Should there be a way for the user to choose not to enable this (editing =

the=0A=

device tree doesn't count), for a use case that is not sufficiently secur=

ity=0A=

sensitive to justify the performance loss?  What is the performance impac=

t of=0A=

this patch?=0A=

=0A=
My reason was that on the other architectures Spectre variant 1=0A=
mitigations are not disabled either. But I think that it might be a good=0A=
idea to add a bootarg parameter to disable the barrier.=0A=
=0A=
Regards,=0A=
=0A=
Diana=0A=
=0A=