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

[Scott Wood <oss@buserror.net>]

On Tue, 2018-05-22 at 10:10 +0300, Diana Craciun wrote:
> Implement the barrier_nospec as a isync;sync instruction sequence.
> The implementation uses the infrastructure built for BOOK3S 64
> with the difference that for NXP platforms there is no firmware involved
> and the need for a speculation barrier is read from the device tree.
> I have used the same name for the property:
> fsl,needs-spec-barrier-for-bounds-check

Using the device tree this way means that anyone without an updated device
tree won't get the protection.  I also don't see any device tree updates --
which chips are affected?  Wouldn't it be more robust to just have the kernel
check the CPU type, especially given that it already does so for a lot of
other purposes?

> +#ifdef CONFIG_PPC_BOOK3S_64
>  void setup_barrier_nospec(void)
>  {
>  	bool enable;
> @@ -44,6 +46,12 @@ void setup_barrier_nospec(void)
>  
>  	enable_barrier_nospec(enable);
>  }
> +#elif CONFIG_PPC_FSL_BOOK3E
> +void setup_barrier_nospec(void)
> +{
> +	enable_barrier_nospec(true);
> +}
> +#endif

The call site is in FSL_BOOK3E-specific code so why not call
enable_barrier_nospec() directly from there?
  
> 
> +#ifdef CONFIG_PPC_BOOK3S_64
>  ssize_t cpu_show_meltdown(struct device *dev, struct device_attribute
> *attr, char *buf)
>  {
>  	bool thread_priv;
> @@ -155,3 +164,4 @@ ssize_t cpu_show_spectre_v2(struct device *dev, struct
> device_attribute *attr, c
>  
>  	return s.len;
>  }
> +#endif

No equivalent of this for FSL?

> +#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; /* nop */
> +	instr[1] = PPC_INST_NOP; /* 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);

Why patch nops in if not enabled?  Aren't those locations already nops?  For
that matter, how can this function even be called on FSL_BOOK3E with enable !=
true?

> diff --git a/arch/powerpc/platforms/85xx/corenet_generic.c
> b/arch/powerpc/platforms/85xx/corenet_generic.c
> index ac191a7..11bce3d 100644
> --- a/arch/powerpc/platforms/85xx/corenet_generic.c
> +++ b/arch/powerpc/platforms/85xx/corenet_generic.c
> @@ -59,6 +59,21 @@ void __init corenet_gen_pic_init(void)
>  	}
>  }
>  
> +static void setup_spec_barrier(void)
> +{
> +	struct device_node *np = of_find_node_by_name(NULL, "cpus");
> +
> +	if (np) {
> +		struct property *prop;
> +
> +		prop = of_find_property(np,
> +		       "fsl,needs-spec-barrier-for-bounds-check", NULL);
> +		if (prop)
> +			setup_barrier_nospec();
> +		of_node_put(np);
> +	}
> +}

Why is this in board code?

Should there be a way for the user to choose not to enable this (editing the
device tree doesn't count), for a use case that is not sufficiently security
sensitive to justify the performance loss?  What is the performance impact of
this patch?

-Scott