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

[Diana Madalina Craciun]

Hi Scott,

Thanks for the review.

On 05/22/2018 11:31 PM, Scott Wood wrote:
> 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?

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

>   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?

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

>
>> +#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?

OK

>   
>> +#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?

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

>
>> +#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?

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

>
>> 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?

I will move it away from the 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?

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

Regards,

Diana