* Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E [not found] <1526973031-9543-1-git-send-email-diana.craciun@nxp.com> @ 2018-05-22 20:29 ` Scott Wood 2018-05-29 15:22 ` Diana Madalina Craciun 2018-05-23 8:56 ` [RESEND RFC " Diana Craciun 1 sibling, 1 reply; 10+ messages in thread From: Scott Wood @ 2018-05-22 20:29 UTC (permalink / raw) To: Diana Craciun, linuxppc-dev; +Cc: mpe, leoyang.li 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E 2018-05-22 20:29 ` [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E Scott Wood @ 2018-05-29 15:22 ` Diana Madalina Craciun 2018-05-29 19:14 ` Scott Wood 0 siblings, 1 reply; 10+ messages in thread From: Diana Madalina Craciun @ 2018-05-29 15:22 UTC (permalink / raw) To: Scott Wood, linuxppc-dev; +Cc: mpe, Leo Li 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= >> 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= >> 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= >> +#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= >> +#ifdef CONFIG_PPC_BOOK3S_64=0A= >> ssize_t cpu_show_meltdown(struct device *dev, struct device_attribute= =0A= >> *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= >> 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= >> +#ifdef CONFIG_PPC_FSL_BOOK3E=0A= >> +void do_barrier_nospec_fixups_range(bool enable, void *fixup_start, voi= d=0A= >> *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= >> 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= ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E 2018-05-29 15:22 ` Diana Madalina Craciun @ 2018-05-29 19:14 ` Scott Wood 2018-05-30 15:09 ` Diana Madalina Craciun 2018-05-31 14:21 ` Michael Ellerman 0 siblings, 2 replies; 10+ messages in thread From: Scott Wood @ 2018-05-29 19:14 UTC (permalink / raw) To: Diana Madalina Craciun, linuxppc-dev; +Cc: mpe, Leo Li On Tue, 2018-05-29 at 15:22 +0000, Diana Madalina Craciun wrote: > 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. So, all supported FSL/NXP book E chips. Why not just enable the workaround unconditionally (and revisit if NXP ever produces a book E chip that doesn't need it and/or e200 is ever supported if that's simple enough to be immune)? > > 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. OK. > > 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. Is there a specific policy reason why they allow spectre v2 to be disabled but not v1, or just a matter of not having a mechanism to disable it, or the parts which could practically be disabled not impacting performance much? -Scott ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E 2018-05-29 19:14 ` Scott Wood @ 2018-05-30 15:09 ` Diana Madalina Craciun 2018-05-31 14:21 ` Michael Ellerman 1 sibling, 0 replies; 10+ messages in thread From: Diana Madalina Craciun @ 2018-05-30 15:09 UTC (permalink / raw) To: Scott Wood, linuxppc-dev; +Cc: mpe, Leo Li On 05/29/2018 10:16 PM, Scott Wood wrote:=0A= > On Tue, 2018-05-29 at 15:22 +0000, Diana Madalina Craciun wrote:=0A= >> 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= >>>> 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 involv= ed=0A= >>>> 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 dev= ice=0A= >>> tree won't get the protection. I also don't see any device tree update= s=0A= >>> --=0A= >>> which chips are affected?=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= > So, all supported FSL/NXP book E chips. Why not just enable the workarou= nd=0A= > unconditionally (and revisit if NXP ever produces a book E chip that does= n't=0A= > need it and/or e200 is ever supported if that's simple enough to be immun= e)?=0A= =0A= I think it makes sense having in mind that all the NXP book E chips are=0A= vulnerable. e200 is not vulnerable, but it is not properly supported in=0A= the kernel anyway. So I guess I can enable the workaround=0A= unconditionally. I am wondering if it does make sense patching the=0A= instructions at all (instead just use the barrier as an isync; sync=0A= sequence always), but in this case we will loose the possibility of=0A= controlling it via debugfs at runtime.=0A= =0A= >=0A= >>> Why patch nops in if not enabled? Aren't those locations already=0A= >>> nops? For=0A= >>> that matter, how can this function even be called on FSL_BOOK3E with=0A= >>> enable !=3D=0A= >>> true?=0A= >> There is some code in arch/powerpc/kernel/security.c which allows=0A= >> control of barrier_nospec via debugfs.=0A= > OK.=0A= >=0A= >>> Should there be a way for the user to choose not to enable this (editin= g=0A= >>> the=0A= >>> device tree doesn't count), for a use case that is not sufficiently=0A= >>> security=0A= >>> sensitive to justify the performance loss? What is the performance imp= act=0A= >>> of=0A= >>> this patch?=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= > Is there a specific policy reason why they allow spectre v2 to be disable= d but=0A= > not v1, or just a matter of not having a mechanism to disable it, or the = parts=0A= > which could practically be disabled not impacting performance much?=0A= =0A= I do not know for sure but I can speculate. The other architectures read=0A= some flags set by the firmware, so I might think that they might use=0A= different versions of firmware if they do not want the mitigations. On=0A= the other hand the other architectures defined special barriers just for=0A= the purpose of preventing speculations which might be more lightweight=0A= and maybe they are not impacting the performance much. But having in=0A= mind that the NXP parts are used in embedded scenarios that might run in=0A= isolation (so no vulnerability) and that the barrier we are using is not=0A= that lightweight I think that we should have a way to disable it.=0A= =0A= Diana=0A= =0A= ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E 2018-05-29 19:14 ` Scott Wood 2018-05-30 15:09 ` Diana Madalina Craciun @ 2018-05-31 14:21 ` Michael Ellerman 2018-05-31 14:35 ` Diana Madalina Craciun 1 sibling, 1 reply; 10+ messages in thread From: Michael Ellerman @ 2018-05-31 14:21 UTC (permalink / raw) To: Scott Wood, Diana Madalina Craciun, linuxppc-dev; +Cc: Leo Li Scott Wood <oss@buserror.net> writes: > On Tue, 2018-05-29 at 15:22 +0000, Diana Madalina Craciun wrote: >> On 05/22/2018 11:31 PM, Scott Wood wrote: > >> > 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. > > Is there a specific policy reason why they allow spectre v2 to be disabled but > not v1, No. > or just a matter of not having a mechanism to disable it, Yes and no. Some of the v1 mitigation is done via masking which can't be easily patched. eg. array_index_nospec() > or the parts which could practically be disabled not impacting > performance much? That's the mean reason AIUI. We can add a nospectre_v1 command line option if necessary. cheers ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E 2018-05-31 14:21 ` Michael Ellerman @ 2018-05-31 14:35 ` Diana Madalina Craciun 2018-05-31 22:03 ` Scott Wood 0 siblings, 1 reply; 10+ messages in thread From: Diana Madalina Craciun @ 2018-05-31 14:35 UTC (permalink / raw) To: Michael Ellerman, Scott Wood, linuxppc-dev; +Cc: Leo Li On 5/31/2018 5:21 PM, Michael Ellerman wrote:=0A= > Scott Wood <oss@buserror.net> writes:=0A= >> On Tue, 2018-05-29 at 15:22 +0000, Diana Madalina Craciun wrote:=0A= >>> On 05/22/2018 11:31 PM, Scott Wood wrote:=0A= >>>> Should there be a way for the user to choose not to enable this (editi= ng=0A= >>>> the=0A= >>>> device tree doesn't count), for a use case that is not sufficiently=0A= >>>> security=0A= >>>> sensitive to justify the performance loss? What is the performance im= pact=0A= >>>> of=0A= >>>> this patch?=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 goo= d=0A= >>> idea to add a bootarg parameter to disable the barrier.=0A= >> Is there a specific policy reason why they allow spectre v2 to be disabl= ed but=0A= >> not v1,=0A= > No.=0A= >=0A= >> or just a matter of not having a mechanism to disable it,=0A= > Yes and no. Some of the v1 mitigation is done via masking which can't be= =0A= > easily patched. eg. array_index_nospec()=0A= >=0A= >> or the parts which could practically be disabled not impacting=0A= >> performance much?=0A= > That's the mean reason AIUI.=0A= >=0A= > We can add a nospectre_v1 command line option if necessary.=0A= =0A= What about nobarrier_nospec (or similar) instead of nospectre_v1 command=0A= line? We are not disabling all the v1 mitigations, the masking part will=0A= remain unchanged.=0A= =0A= Diana=0A= =0A= =0A= ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E 2018-05-31 14:35 ` Diana Madalina Craciun @ 2018-05-31 22:03 ` Scott Wood 2018-06-01 10:40 ` Michael Ellerman 0 siblings, 1 reply; 10+ messages in thread From: Scott Wood @ 2018-05-31 22:03 UTC (permalink / raw) To: Diana Madalina Craciun, Michael Ellerman, linuxppc-dev; +Cc: Leo Li On Thu, 2018-05-31 at 14:35 +0000, Diana Madalina Craciun wrote: > On 5/31/2018 5:21 PM, Michael Ellerman wrote: > > > > We can add a nospectre_v1 command line option if necessary. > > What about nobarrier_nospec (or similar) instead of nospectre_v1 command > line? We are not disabling all the v1 mitigations, the masking part will > remain unchanged. I think nospectre_v1 makes more sense as it's about the user's intentions rather than the implementation. The user is giving the kernel permission to not defend against spectre v1, and it's up to the implementation which mitigations (if any) to disable in response to that, same as any other optimization. -Scott ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E 2018-05-31 22:03 ` Scott Wood @ 2018-06-01 10:40 ` Michael Ellerman 2018-06-01 14:58 ` Diana Madalina Craciun 0 siblings, 1 reply; 10+ messages in thread From: Michael Ellerman @ 2018-06-01 10:40 UTC (permalink / raw) To: Scott Wood, Diana Madalina Craciun, linuxppc-dev; +Cc: Leo Li Scott Wood <oss@buserror.net> writes: > On Thu, 2018-05-31 at 14:35 +0000, Diana Madalina Craciun wrote: >> On 5/31/2018 5:21 PM, Michael Ellerman wrote: >> > >> > We can add a nospectre_v1 command line option if necessary. >> >> What about nobarrier_nospec (or similar) instead of nospectre_v1 command >> line? We are not disabling all the v1 mitigations, the masking part will >> remain unchanged. > > I think nospectre_v1 makes more sense as it's about the user's intentions > rather than the implementation. The user is giving the kernel permission to > not defend against spectre v1, and it's up to the implementation which > mitigations (if any) to disable in response to that, same as any other > optimization. Yeah I agree. We also have `nospectre_v2` on x86/s390 so I think keeping consistency with that is a must. cheers ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E 2018-06-01 10:40 ` Michael Ellerman @ 2018-06-01 14:58 ` Diana Madalina Craciun 0 siblings, 0 replies; 10+ messages in thread From: Diana Madalina Craciun @ 2018-06-01 14:58 UTC (permalink / raw) To: Michael Ellerman, Scott Wood, linuxppc-dev; +Cc: Leo Li On 6/1/2018 1:40 PM, Michael Ellerman wrote:=0A= > Scott Wood <oss@buserror.net> writes:=0A= >=0A= >> On Thu, 2018-05-31 at 14:35 +0000, Diana Madalina Craciun wrote:=0A= >>> On 5/31/2018 5:21 PM, Michael Ellerman wrote:=0A= >>>> We can add a nospectre_v1 command line option if necessary.=0A= >>> What about nobarrier_nospec (or similar) instead of nospectre_v1 comman= d=0A= >>> line? We are not disabling all the v1 mitigations, the masking part wil= l=0A= >>> remain unchanged.=0A= >> I think nospectre_v1 makes more sense as it's about the user's intention= s=0A= >> rather than the implementation. The user is giving the kernel permissio= n to=0A= >> not defend against spectre v1, and it's up to the implementation which= =0A= >> mitigations (if any) to disable in response to that, same as any other= =0A= >> optimization.=0A= > Yeah I agree. We also have `nospectre_v2` on x86/s390 so I think keeping= =0A= > consistency with that is a must.=0A= >=0A= > cheers=0A= >=0A= OK=0A= =0A= Diana=0A= =0A= ^ permalink raw reply [flat|nested] 10+ messages in thread
* [RESEND RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E [not found] <1526973031-9543-1-git-send-email-diana.craciun@nxp.com> 2018-05-22 20:29 ` [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E Scott Wood @ 2018-05-23 8:56 ` Diana Craciun 1 sibling, 0 replies; 10+ messages in thread From: Diana Craciun @ 2018-05-23 8:56 UTC (permalink / raw) To: linuxppc-dev; +Cc: Diana Craciun 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 Signed-off-by: Diana Craciun <diana.craciun@nxp.com> --- The patches were created on top of the BOOK3S 64 patches: https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-April/172137.html arch/powerpc/include/asm/barrier.h | 12 ++++++++- arch/powerpc/include/asm/setup.h | 2 +- arch/powerpc/kernel/Makefile | 2 +- arch/powerpc/kernel/module.c | 2 ++ arch/powerpc/kernel/security.c | 12 ++++++++- arch/powerpc/kernel/vmlinux.lds.S | 2 ++ arch/powerpc/lib/feature-fixups.c | 38 +++++++++++++++++++++++++-- arch/powerpc/platforms/85xx/corenet_generic.c | 17 ++++++++++++ 8 files changed, 81 insertions(+), 6 deletions(-) diff --git a/arch/powerpc/include/asm/barrier.h b/arch/powerpc/include/asm/barrier.h index f67b3f6..1379386 100644 --- a/arch/powerpc/include/asm/barrier.h +++ b/arch/powerpc/include/asm/barrier.h @@ -86,7 +86,17 @@ do { \ // 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 */ +#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 && !CONFIG_PPC_FSL_BOOK3E */ #define barrier_nospec_asm #define barrier_nospec() #endif diff --git a/arch/powerpc/include/asm/setup.h b/arch/powerpc/include/asm/setup.h index aeb175e8..fbc3ef7 100644 --- a/arch/powerpc/include/asm/setup.h +++ b/arch/powerpc/include/asm/setup.h @@ -55,7 +55,7 @@ void do_rfi_flush_fixups(enum l1d_flush_type types); void setup_barrier_nospec(void); void do_barrier_nospec_fixups(bool enable); -#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 a72698c..ede64a5 100644 --- a/arch/powerpc/kernel/module.c +++ b/arch/powerpc/kernel/module.c @@ -72,7 +72,9 @@ 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 +#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(true, diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c index d1b9639..01b6c55 100644 --- a/arch/powerpc/kernel/security.c +++ b/arch/powerpc/kernel/security.c @@ -12,8 +12,9 @@ #include <asm/security_features.h> #include <asm/setup.h> - +#ifdef CONFIG_PPC_BOOK3S_64 unsigned long powerpc_security_features __read_mostly = SEC_FTR_DEFAULT; +#endif static bool barrier_nospec_enabled; @@ -23,6 +24,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; @@ -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 #ifdef CONFIG_DEBUG_FS static int barrier_nospec_set(void *data, u64 val) @@ -82,6 +90,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; @@ -155,3 +164,4 @@ ssize_t cpu_show_spectre_v2(struct device *dev, struct device_attribute *attr, c return s.len; } +#endif diff --git a/arch/powerpc/kernel/vmlinux.lds.S b/arch/powerpc/kernel/vmlinux.lds.S index ff73f49..e3c0ef2 100644 --- a/arch/powerpc/kernel/vmlinux.lds.S +++ b/arch/powerpc/kernel/vmlinux.lds.S @@ -139,7 +139,9 @@ SECTIONS *(__rfi_flush_fixup) __stop___rfi_flush_fixup = .; } +#endif +#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 = .; diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-fixups.c index 3b37529..033ef28 100644 --- a/arch/powerpc/lib/feature-fixups.c +++ b/arch/powerpc/lib/feature-fixups.c @@ -162,7 +162,6 @@ void do_rfi_flush_fixups(enum l1d_flush_type types) (types & L1D_FLUSH_MTTRIG) ? "mttrig type" : "unknown"); } - void do_barrier_nospec_fixups_range(bool enable, void *fixup_start, void *fixup_end) { unsigned int instr, *dest; @@ -188,7 +187,9 @@ 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 */ +#if defined(CONFIG_PPC_BOOK3S_64) || defined(CONFIG_PPC_FSL_BOOK3E) void do_barrier_nospec_fixups(bool enable) { void *start, *end; @@ -199,7 +200,40 @@ 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 */ + +#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); + +} +#endif /* CONFIG_PPC_FSL_BOOK3E */ void do_lwsync_fixups(unsigned long value, void *fixup_start, void *fixup_end) { 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); + } +} + /* * Setup the architecture */ @@ -80,6 +95,8 @@ void __init corenet_gen_setup_arch(void) pr_info("%s board\n", ppc_md.name); + setup_spec_barrier(); + mpc85xx_qe_init(); } -- 2.5.5 ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-06-01 14:58 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1526973031-9543-1-git-send-email-diana.craciun@nxp.com>
2018-05-22 20:29 ` [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E Scott Wood
2018-05-29 15:22 ` Diana Madalina Craciun
2018-05-29 19:14 ` Scott Wood
2018-05-30 15:09 ` Diana Madalina Craciun
2018-05-31 14:21 ` Michael Ellerman
2018-05-31 14:35 ` Diana Madalina Craciun
2018-05-31 22:03 ` Scott Wood
2018-06-01 10:40 ` Michael Ellerman
2018-06-01 14:58 ` Diana Madalina Craciun
2018-05-23 8:56 ` [RESEND RFC " Diana Craciun
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).