Message ID | 20220923033004.536127-1-npiggin@gmail.com (mailing list archive) |
---|---|
State | Accepted |
Headers | show |
Series | powerpc/64s: POWER10 CPU Kconfig build option | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 10 jobs. |
snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 10 jobs. |
snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 23 jobs. |
snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
Le 23/09/2022 à 05:30, Nicholas Piggin a écrit : > This adds basic POWER10_CPU option, which builds with -mcpu=power10. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > There's quite a lot of asm and linker changes slated for the next merge > window already so I may leave the pcrel patch for next time. I think we > can add the basic POWER10 build option though. > > Thanks, > Nick > > arch/powerpc/Makefile | 7 ++++++- > arch/powerpc/platforms/Kconfig.cputype | 8 +++++++- > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile > index 8a3d69b02672..ea88af26f8c6 100644 > --- a/arch/powerpc/Makefile > +++ b/arch/powerpc/Makefile > @@ -192,9 +192,14 @@ ifdef CONFIG_476FPE_ERR46 > -T $(srctree)/arch/powerpc/platforms/44x/ppc476_modules.lds > endif > > -# No AltiVec or VSX instructions when building kernel > +# No prefix or pcrel > +KBUILD_CFLAGS += $(call cc-option,-mno-prefixed) We have lots of code to handle prefixed instructions in code_patching, and that code complexifies stuff and has a performance impact. And it is only partially taken into account, areas like ftrace don't properly take care of prefixed instructions. Should we get rid of prefixed instruction support completely in the kernel, and come back to more simple code ? > +KBUILD_CFLAGS += $(call cc-option,-mno-pcrel) > + > +# No AltiVec or VSX or MMA instructions when building kernel > KBUILD_CFLAGS += $(call cc-option,-mno-altivec) > KBUILD_CFLAGS += $(call cc-option,-mno-vsx) > +KBUILD_CFLAGS += $(call cc-option,-mno-mma) > > # No SPE instruction when building kernel > # (We use all available options to help semi-broken compilers) > diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype > index 4017be72e46f..1f7c903ea664 100644 > --- a/arch/powerpc/platforms/Kconfig.cputype > +++ b/arch/powerpc/platforms/Kconfig.cputype > @@ -171,6 +171,11 @@ config POWER9_CPU > depends on PPC_BOOK3S_64 > select ARCH_HAS_FAST_MULTIPLIER > > +config POWER10_CPU > + bool "POWER10" > + depends on PPC_BOOK3S_64 > + select ARCH_HAS_FAST_MULTIPLIER > + > config E5500_CPU > bool "Freescale e5500" > depends on PPC64 && E500 > @@ -239,6 +244,7 @@ config TARGET_CPU > default "power7" if POWER7_CPU > default "power8" if POWER8_CPU > default "power9" if POWER9_CPU > + default "power10" if POWER10_CPU > default "405" if 405_CPU > default "440" if 440_CPU > default "464" if 464_CPU
On Fri Sep 23, 2022 at 3:46 PM AEST, Christophe Leroy wrote: > > > Le 23/09/2022 à 05:30, Nicholas Piggin a écrit : > > This adds basic POWER10_CPU option, which builds with -mcpu=power10. > > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > > --- > > There's quite a lot of asm and linker changes slated for the next merge > > window already so I may leave the pcrel patch for next time. I think we > > can add the basic POWER10 build option though. > > > > Thanks, > > Nick > > > > arch/powerpc/Makefile | 7 ++++++- > > arch/powerpc/platforms/Kconfig.cputype | 8 +++++++- > > 2 files changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile > > index 8a3d69b02672..ea88af26f8c6 100644 > > --- a/arch/powerpc/Makefile > > +++ b/arch/powerpc/Makefile > > @@ -192,9 +192,14 @@ ifdef CONFIG_476FPE_ERR46 > > -T $(srctree)/arch/powerpc/platforms/44x/ppc476_modules.lds > > endif > > > > -# No AltiVec or VSX instructions when building kernel > > +# No prefix or pcrel > > +KBUILD_CFLAGS += $(call cc-option,-mno-prefixed) > > We have lots of code to handle prefixed instructions in code_patching, > and that code complexifies stuff and has a performance impact. > And it is only partially taken into account, areas like ftrace don't > properly take care of prefixed instructions. > > Should we get rid of prefixed instruction support completely in the > kernel, and come back to more simple code ? I would rather complete prefixed support in the kernel and use pcrel addressing. Actually even if we don't compile with pcrel or prefixed, there are some instructions and we will probably get more that require prefixed, possible we might want to use them in kernel. Some of it is required to handle user mode instructions too. So I think removing it is premature, but I guess it's up for debate. Thanks, Nick
On Fri, 23 Sep 2022 13:30:04 +1000, Nicholas Piggin wrote: > This adds basic POWER10_CPU option, which builds with -mcpu=power10. > > Applied to powerpc/next. [1/1] powerpc/64s: POWER10 CPU Kconfig build option https://git.kernel.org/powerpc/c/4b2a9315f20d98576e25c9e4572e9a8e028d7aa2 cheers
Le 23/09/2022 à 08:23, Nicholas Piggin a écrit : > On Fri Sep 23, 2022 at 3:46 PM AEST, Christophe Leroy wrote: >> >> >> Le 23/09/2022 à 05:30, Nicholas Piggin a écrit : >>> This adds basic POWER10_CPU option, which builds with -mcpu=power10. >>> >>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >>> --- >>> There's quite a lot of asm and linker changes slated for the next merge >>> window already so I may leave the pcrel patch for next time. I think we >>> can add the basic POWER10 build option though. >>> >>> Thanks, >>> Nick >>> >>> arch/powerpc/Makefile | 7 ++++++- >>> arch/powerpc/platforms/Kconfig.cputype | 8 +++++++- >>> 2 files changed, 13 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile >>> index 8a3d69b02672..ea88af26f8c6 100644 >>> --- a/arch/powerpc/Makefile >>> +++ b/arch/powerpc/Makefile >>> @@ -192,9 +192,14 @@ ifdef CONFIG_476FPE_ERR46 >>> -T $(srctree)/arch/powerpc/platforms/44x/ppc476_modules.lds >>> endif >>> >>> -# No AltiVec or VSX instructions when building kernel >>> +# No prefix or pcrel >>> +KBUILD_CFLAGS += $(call cc-option,-mno-prefixed) >> >> We have lots of code to handle prefixed instructions in code_patching, >> and that code complexifies stuff and has a performance impact. >> And it is only partially taken into account, areas like ftrace don't >> properly take care of prefixed instructions. >> >> Should we get rid of prefixed instruction support completely in the >> kernel, and come back to more simple code ? > > I would rather complete prefixed support in the kernel and use pcrel > addressing. Actually even if we don't compile with pcrel or prefixed, > there are some instructions and we will probably get more that require > prefixed, possible we might want to use them in kernel. Some of it is > required to handle user mode instructions too. So I think removing > it is premature, but I guess it's up for debate. Well ok, in fact I only had code_patching in mind. Code patching is only for kernel text. Today code patching is used for things like kprobe, ftrace, etc .... which really do not seems to be prepared for prefixed instructions. If you are adding -mno-prefixed, it is worth keeping that code which sometimes gives us some headacke ? Of course if there are plans to get real prefixed instruction in kernel code anytime soon, lets live with it, in that case the support should get completed. But otherwise I think it would be better to get rid of it for now, and implement it completely when we need it in years. When I see the following, I'm having hard time believing it would work with prefixed instructions in the kernel text: typedef u32 kprobe_opcode_t; struct kprobe { ... /* Saved opcode (which has been replaced with breakpoint) */ kprobe_opcode_t opcode; void arch_disarm_kprobe(struct kprobe *p) { WARN_ON_ONCE(patch_instruction(p->addr, ppc_inst(p->opcode))); } Christophe
Hi! On Fri, Sep 23, 2022 at 01:30:04PM +1000, Nicholas Piggin wrote: > This adds basic POWER10_CPU option, which builds with -mcpu=power10. > +# No prefix or pcrel > +KBUILD_CFLAGS += $(call cc-option,-mno-prefixed) > +KBUILD_CFLAGS += $(call cc-option,-mno-pcrel) Why do you disable all prefixed insns? What goes wrong if you don't? Same question for pcrel. I'm sure you want to optimise it better, but it's not clear to me how it fails now? Please say in the comment what is wrong, don't spread fear :-) > +# No AltiVec or VSX or MMA instructions when building kernel > KBUILD_CFLAGS += $(call cc-option,-mno-altivec) > KBUILD_CFLAGS += $(call cc-option,-mno-vsx) > +KBUILD_CFLAGS += $(call cc-option,-mno-mma) MMA code is never generated unless the code asks for it explicitly. This is fundamental, not just an implementations side effect. Segher
Hi! On Thu, Oct 06, 2022 at 06:07:32PM +0000, Christophe Leroy wrote: > Le 23/09/2022 à 08:23, Nicholas Piggin a écrit : > > I would rather complete prefixed support in the kernel and use pcrel > > addressing. Actually even if we don't compile with pcrel or prefixed, > > there are some instructions and we will probably get more that require > > prefixed, possible we might want to use them in kernel. Some of it is > > required to handle user mode instructions too. So I think removing > > it is premature, but I guess it's up for debate. > > Well ok, in fact I only had code_patching in mind. > > Code patching is only for kernel text. Today code patching is used for > things like kprobe, ftrace, etc .... which really do not seems to be > prepared for prefixed instructions. > > If you are adding -mno-prefixed, it is worth keeping that code which > sometimes gives us some headacke ? -mpcrel requires -mprefixed. Using PC relative addressing will be a significant performance benefit. > Of course if there are plans to get real prefixed instruction in kernel > code anytime soon, lets live with it, in that case the support should > get completed. But otherwise I think it would be better to get rid of it > for now, and implement it completely when we need it in years. The future is unstoppable, certainly the near future is :-) > When I see the following, I'm having hard time believing it would work > with prefixed instructions in the kernel text: > > typedef u32 kprobe_opcode_t; > > struct kprobe { > ... > /* Saved opcode (which has been replaced with breakpoint) */ > kprobe_opcode_t opcode; > > > void arch_disarm_kprobe(struct kprobe *p) > { > WARN_ON_ONCE(patch_instruction(p->addr, ppc_inst(p->opcode))); > } Why would it not work? Segher
On Fri Oct 7, 2022 at 5:54 AM AEST, Segher Boessenkool wrote: > Hi! > > On Fri, Sep 23, 2022 at 01:30:04PM +1000, Nicholas Piggin wrote: > > This adds basic POWER10_CPU option, which builds with -mcpu=power10. > > > +# No prefix or pcrel > > +KBUILD_CFLAGS += $(call cc-option,-mno-prefixed) > > +KBUILD_CFLAGS += $(call cc-option,-mno-pcrel) > > Why do you disable all prefixed insns? What goes wrong if you don't? Potentially things like kprobes. > Same question for pcrel. I'm sure you want to optimise it better, but > it's not clear to me how it fails now? For pcrel addressing? Bootstrapping the C environment is one, the module dynamic linker is another. Some details in this series. https://lists.ozlabs.org/pipermail/linuxppc-dev/2022-September/248521.html > > Please say in the comment what is wrong, don't spread fear :-) > > > +# No AltiVec or VSX or MMA instructions when building kernel > > KBUILD_CFLAGS += $(call cc-option,-mno-altivec) > > KBUILD_CFLAGS += $(call cc-option,-mno-vsx) > > +KBUILD_CFLAGS += $(call cc-option,-mno-mma) > > MMA code is never generated unless the code asks for it explicitly. > This is fundamental, not just an implementations side effect. Well, now it double won't be generated :) Thanks, Nick
On Fri Oct 7, 2022 at 4:07 AM AEST, Christophe Leroy wrote: > > > Le 23/09/2022 à 08:23, Nicholas Piggin a écrit : > > On Fri Sep 23, 2022 at 3:46 PM AEST, Christophe Leroy wrote: > >> > >> > >> Le 23/09/2022 à 05:30, Nicholas Piggin a écrit : > >>> This adds basic POWER10_CPU option, which builds with -mcpu=power10. > >>> > >>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > >>> --- > >>> There's quite a lot of asm and linker changes slated for the next merge > >>> window already so I may leave the pcrel patch for next time. I think we > >>> can add the basic POWER10 build option though. > >>> > >>> Thanks, > >>> Nick > >>> > >>> arch/powerpc/Makefile | 7 ++++++- > >>> arch/powerpc/platforms/Kconfig.cputype | 8 +++++++- > >>> 2 files changed, 13 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile > >>> index 8a3d69b02672..ea88af26f8c6 100644 > >>> --- a/arch/powerpc/Makefile > >>> +++ b/arch/powerpc/Makefile > >>> @@ -192,9 +192,14 @@ ifdef CONFIG_476FPE_ERR46 > >>> -T $(srctree)/arch/powerpc/platforms/44x/ppc476_modules.lds > >>> endif > >>> > >>> -# No AltiVec or VSX instructions when building kernel > >>> +# No prefix or pcrel > >>> +KBUILD_CFLAGS += $(call cc-option,-mno-prefixed) > >> > >> We have lots of code to handle prefixed instructions in code_patching, > >> and that code complexifies stuff and has a performance impact. > >> And it is only partially taken into account, areas like ftrace don't > >> properly take care of prefixed instructions. > >> > >> Should we get rid of prefixed instruction support completely in the > >> kernel, and come back to more simple code ? > > > > I would rather complete prefixed support in the kernel and use pcrel > > addressing. Actually even if we don't compile with pcrel or prefixed, > > there are some instructions and we will probably get more that require > > prefixed, possible we might want to use them in kernel. Some of it is > > required to handle user mode instructions too. So I think removing > > it is premature, but I guess it's up for debate. > > Well ok, in fact I only had code_patching in mind. > > Code patching is only for kernel text. Today code patching is used for > things like kprobe, ftrace, etc .... which really do not seems to be > prepared for prefixed instructions. > > If you are adding -mno-prefixed, it is worth keeping that code which > sometimes gives us some headacke ? > > Of course if there are plans to get real prefixed instruction in kernel > code anytime soon, lets live with it, in that case the support should > get completed. But otherwise I think it would be better to get rid of it > for now, and implement it completely when we need it in years. I have a series to enable it again, just not ready for upstream yet but it's not all that far off. https://lists.ozlabs.org/pipermail/linuxppc-dev/2022-September/248521.html > When I see the following, I'm having hard time believing it would work > with prefixed instructions in the kernel text: > > typedef u32 kprobe_opcode_t; > > struct kprobe { > ... > /* Saved opcode (which has been replaced with breakpoint) */ > kprobe_opcode_t opcode; > > > void arch_disarm_kprobe(struct kprobe *p) > { > WARN_ON_ONCE(patch_instruction(p->addr, ppc_inst(p->opcode))); > } Yeah that needs work for sure. Thanks, Nick
On Fri, Oct 07, 2022 at 07:56:09AM +1000, Nicholas Piggin wrote: > On Fri Oct 7, 2022 at 5:54 AM AEST, Segher Boessenkool wrote: > > On Fri, Sep 23, 2022 at 01:30:04PM +1000, Nicholas Piggin wrote: > > > This adds basic POWER10_CPU option, which builds with -mcpu=power10. > > > > > +# No prefix or pcrel > > > +KBUILD_CFLAGS += $(call cc-option,-mno-prefixed) > > > +KBUILD_CFLAGS += $(call cc-option,-mno-pcrel) > > > > Why do you disable all prefixed insns? What goes wrong if you don't? > > Potentially things like kprobes. So mention that? "This patch is due to an abundance of caution". What I meant to ask is if you *saw* something going wrong, not if you can imagine something going wrong. I can imagine ten gazillion things going wrong, that is not why I asked :-) > > Same question for pcrel. I'm sure you want to optimise it better, but > > it's not clear to me how it fails now? > > For pcrel addressing? Bootstrapping the C environment is one, the > module dynamic linker is another. I don't know what either of those mean. > Some details in this series. > > https://lists.ozlabs.org/pipermail/linuxppc-dev/2022-September/248521.html I've watched that series with great interest, but I don't remember anything like that? Are you refering to the commentary in 7/7? "Definitely ftrace and probes, possibly BPF and KVM have some breakage. I haven't looked closely yet."... This doesn't mean much does it :-) It can be a triviality or two. Or it could be a massive roadblock. Just say in a comment where you disable stuff that it is to prevent possible problems, this is a WIP, that kind of thing? Otherwise other people (like me :-) ) will read it and think there must be some deeper reason. Like, changing code to work with pcrel is hard or a lot of work -- it isn't :-) As you say in 0/7 yourself btw! > > > +# No AltiVec or VSX or MMA instructions when building kernel > > > KBUILD_CFLAGS += $(call cc-option,-mno-altivec) > > > KBUILD_CFLAGS += $(call cc-option,-mno-vsx) > > > +KBUILD_CFLAGS += $(call cc-option,-mno-mma) > > > > MMA code is never generated unless the code asks for it explicitly. > > This is fundamental, not just an implementations side effect. > > Well, now it double won't be generated :) Yeah, but there are many other things you can unnecessarily disable as well! :-) VMX and VSX are disabled here because the compiler *will* use those registers if it feels like it (that is, if it thinks that will be faster). MMA is a very different beast: the compiler can never know if it will be faster, to start with. Segher
On Fri Oct 7, 2022 at 9:23 AM AEST, Segher Boessenkool wrote: > On Fri, Oct 07, 2022 at 07:56:09AM +1000, Nicholas Piggin wrote: > > On Fri Oct 7, 2022 at 5:54 AM AEST, Segher Boessenkool wrote: > > > On Fri, Sep 23, 2022 at 01:30:04PM +1000, Nicholas Piggin wrote: > > > > This adds basic POWER10_CPU option, which builds with -mcpu=power10. > > > > > > > +# No prefix or pcrel > > > > +KBUILD_CFLAGS += $(call cc-option,-mno-prefixed) > > > > +KBUILD_CFLAGS += $(call cc-option,-mno-pcrel) > > > > > > Why do you disable all prefixed insns? What goes wrong if you don't? > > > > Potentially things like kprobes. > > So mention that? "This patch is due to an abundance of caution". Well it's in next now. I did say *basic*, I'm sure not changing the ABI or adding prefix instructions isn't too mysterious. > > What I meant to ask is if you *saw* something going wrong, not if you > can imagine something going wrong. I can imagine ten gazillion things > going wrong, that is not why I asked :-) > > > > Same question for pcrel. I'm sure you want to optimise it better, but > > > it's not clear to me how it fails now? > > > > For pcrel addressing? Bootstrapping the C environment is one, the > > module dynamic linker is another. > > I don't know what either of those mean. arch/powerpc/kernel/head_64.S and arch/powerpc/kernel/module_64.c Can discuss in the pcrel patch series thread if you would like to know more. > > > Some details in this series. > > > > https://lists.ozlabs.org/pipermail/linuxppc-dev/2022-September/248521.html > > I've watched that series with great interest, but I don't remember > anything like that? Are you refering to the commentary in 7/7? > "Definitely ftrace and probes, possibly BPF and KVM have some breakage. > I haven't looked closely yet."... This doesn't mean much does it :-) > It can be a triviality or two. Or it could be a massive roadblock. > > Just say in a comment where you disable stuff that it is to prevent > possible problems, this is a WIP, that kind of thing? Otherwise other > people (like me :-) ) will read it and think there must be some deeper > reason. Like, changing code to work with pcrel is hard or a lot of > work -- it isn't :-) As you say in 0/7 yourself btw! > I will describe limitations and issues a bit more in changelog of patches to enable prefix and pcrel when I submit as non-RFC candidate. It would probably not be too hard to get things to a workable state that could be merged. > > > > +# No AltiVec or VSX or MMA instructions when building kernel > > > > KBUILD_CFLAGS += $(call cc-option,-mno-altivec) > > > > KBUILD_CFLAGS += $(call cc-option,-mno-vsx) > > > > +KBUILD_CFLAGS += $(call cc-option,-mno-mma) > > > > > > MMA code is never generated unless the code asks for it explicitly. > > > This is fundamental, not just an implementations side effect. > > > > Well, now it double won't be generated :) > > Yeah, but there are many other things you can unnecessarily disable as > well! :-) > > VMX and VSX are disabled here because the compiler *will* use those > registers if it feels like it (that is, if it thinks that will be > faster). MMA is a very different beast: the compiler can never know if > it will be faster, to start with. True, but now I don't have to find the exact clause and have my lawyer confirm that it definitely probably won't change in future and break things. Thanks, Nick
"Nicholas Piggin" <npiggin@gmail.com> writes: > On Fri Oct 7, 2022 at 9:23 AM AEST, Segher Boessenkool wrote: >> On Fri, Oct 07, 2022 at 07:56:09AM +1000, Nicholas Piggin wrote: >> > On Fri Oct 7, 2022 at 5:54 AM AEST, Segher Boessenkool wrote: >> > > On Fri, Sep 23, 2022 at 01:30:04PM +1000, Nicholas Piggin wrote: ... >> > > > +# No AltiVec or VSX or MMA instructions when building kernel >> > > > KBUILD_CFLAGS += $(call cc-option,-mno-altivec) >> > > > KBUILD_CFLAGS += $(call cc-option,-mno-vsx) >> > > > +KBUILD_CFLAGS += $(call cc-option,-mno-mma) >> > > >> > > MMA code is never generated unless the code asks for it explicitly. >> > > This is fundamental, not just an implementations side effect. >> > >> > Well, now it double won't be generated :) >> >> Yeah, but there are many other things you can unnecessarily disable as >> well! :-) >> >> VMX and VSX are disabled here because the compiler *will* use those >> registers if it feels like it (that is, if it thinks that will be >> faster). MMA is a very different beast: the compiler can never know if >> it will be faster, to start with. > > True, but now I don't have to find the exact clause and have my lawyer > confirm that it definitely probably won't change in future and break > things. Right. If someone asks "does the kernel ever use MMA instructions?" we can just point at that line and we have a definite answer. No need to audit the behaviour of all GCC and Clang versions ever released. cheers
On Fri, Oct 07, 2022 at 04:31:28PM +1100, Michael Ellerman wrote: > "Nicholas Piggin" <npiggin@gmail.com> writes: > > On Fri Oct 7, 2022 at 9:23 AM AEST, Segher Boessenkool wrote: > >> On Fri, Oct 07, 2022 at 07:56:09AM +1000, Nicholas Piggin wrote: > >> > On Fri Oct 7, 2022 at 5:54 AM AEST, Segher Boessenkool wrote: > >> > > On Fri, Sep 23, 2022 at 01:30:04PM +1000, Nicholas Piggin wrote: > ... > >> > > > +# No AltiVec or VSX or MMA instructions when building kernel > >> > > > KBUILD_CFLAGS += $(call cc-option,-mno-altivec) > >> > > > KBUILD_CFLAGS += $(call cc-option,-mno-vsx) > >> > > > +KBUILD_CFLAGS += $(call cc-option,-mno-mma) > >> > > > >> > > MMA code is never generated unless the code asks for it explicitly. > >> > > This is fundamental, not just an implementations side effect. > >> > > >> > Well, now it double won't be generated :) > >> > >> Yeah, but there are many other things you can unnecessarily disable as > >> well! :-) > >> > >> VMX and VSX are disabled here because the compiler *will* use those > >> registers if it feels like it (that is, if it thinks that will be > >> faster). MMA is a very different beast: the compiler can never know if > >> it will be faster, to start with. > > > > True, but now I don't have to find the exact clause and have my lawyer > > confirm that it definitely probably won't change in future and break > > things. > > Right. If someone asks "does the kernel ever use MMA instructions?" we > can just point at that line and we have a definite answer. No need to > audit the behaviour of all GCC and Clang versions ever released. As I said, no sane compiler can use MMA ever (unless asked for it directly of course). But yeah, who knows what clang does! Segher
On Fri, Oct 07, 2022 at 10:03:38AM +1000, Nicholas Piggin wrote: > On Fri Oct 7, 2022 at 9:23 AM AEST, Segher Boessenkool wrote: > > > For pcrel addressing? Bootstrapping the C environment is one, the > > > module dynamic linker is another. > > > > I don't know what either of those mean. > > arch/powerpc/kernel/head_64.S and arch/powerpc/kernel/module_64.c > > Can discuss in the pcrel patch series thread if you would like to know > more. So "bootstrapping the C environment" is meant to mean "initialising it", like *rt*.o ("C runtime") does normally? And "module dynamic linker" is "module loader" here? Yes, those things probably need some attention for pcrel, but "bootstrapping" and "dynamic" had me scratch my head: there is nothing that pulls itself up by its bootstraps (like, the initialisation itself would be done in C code), and module loading is much closer to static loading than to dynamic loading :-) > > Just say in a comment where you disable stuff that it is to prevent > > possible problems, this is a WIP, that kind of thing? Otherwise other > > people (like me :-) ) will read it and think there must be some deeper > > reason. Like, changing code to work with pcrel is hard or a lot of > > work -- it isn't :-) As you say in 0/7 yourself btw! > > I will describe limitations and issues a bit more in changelog of patches > to enable prefix and pcrel when I submit as non-RFC candidate. It would > probably not be too hard to get things to a workable state that could be > merged. Looking forward to it! > > VMX and VSX are disabled here because the compiler *will* use those > > registers if it feels like it (that is, if it thinks that will be > > faster). MMA is a very different beast: the compiler can never know if > > it will be faster, to start with. > > True, but now I don't have to find the exact clause and have my lawyer > confirm that it definitely probably won't change in future and break > things. Your lawyer won't be able to tell you, but I can. And I did already. The reason I care about these things is that very often people look at what the kernel does as a "best practices" example. And then copy this stuff as some cargo cult incantations :-/ Segher
On Fri Oct 7, 2022 at 4:07 AM AEST, Christophe Leroy wrote: > > > Le 23/09/2022 à 08:23, Nicholas Piggin a écrit : > > On Fri Sep 23, 2022 at 3:46 PM AEST, Christophe Leroy wrote: > >> > >> > >> Le 23/09/2022 à 05:30, Nicholas Piggin a écrit : > >>> This adds basic POWER10_CPU option, which builds with -mcpu=power10. > >>> > >>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > >>> --- > >>> There's quite a lot of asm and linker changes slated for the next merge > >>> window already so I may leave the pcrel patch for next time. I think we > >>> can add the basic POWER10 build option though. > >>> > >>> Thanks, > >>> Nick > >>> > >>> arch/powerpc/Makefile | 7 ++++++- > >>> arch/powerpc/platforms/Kconfig.cputype | 8 +++++++- > >>> 2 files changed, 13 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile > >>> index 8a3d69b02672..ea88af26f8c6 100644 > >>> --- a/arch/powerpc/Makefile > >>> +++ b/arch/powerpc/Makefile > >>> @@ -192,9 +192,14 @@ ifdef CONFIG_476FPE_ERR46 > >>> -T $(srctree)/arch/powerpc/platforms/44x/ppc476_modules.lds > >>> endif > >>> > >>> -# No AltiVec or VSX instructions when building kernel > >>> +# No prefix or pcrel > >>> +KBUILD_CFLAGS += $(call cc-option,-mno-prefixed) > >> > >> We have lots of code to handle prefixed instructions in code_patching, > >> and that code complexifies stuff and has a performance impact. > >> And it is only partially taken into account, areas like ftrace don't > >> properly take care of prefixed instructions. > >> > >> Should we get rid of prefixed instruction support completely in the > >> kernel, and come back to more simple code ? > > > > I would rather complete prefixed support in the kernel and use pcrel > > addressing. Actually even if we don't compile with pcrel or prefixed, > > there are some instructions and we will probably get more that require > > prefixed, possible we might want to use them in kernel. Some of it is > > required to handle user mode instructions too. So I think removing > > it is premature, but I guess it's up for debate. > > Well ok, in fact I only had code_patching in mind. > > Code patching is only for kernel text. Today code patching is used for > things like kprobe, ftrace, etc .... which really do not seems to be > prepared for prefixed instructions. > > If you are adding -mno-prefixed, it is worth keeping that code which > sometimes gives us some headacke ? > > Of course if there are plans to get real prefixed instruction in kernel > code anytime soon, lets live with it, in that case the support should > get completed. But otherwise I think it would be better to get rid of it > for now, and implement it completely when we need it in years. > > When I see the following, I'm having hard time believing it would work > with prefixed instructions in the kernel text: > > typedef u32 kprobe_opcode_t; > > struct kprobe { > ... > /* Saved opcode (which has been replaced with breakpoint) */ > kprobe_opcode_t opcode; > > > void arch_disarm_kprobe(struct kprobe *p) > { > WARN_ON_ONCE(patch_instruction(p->addr, ppc_inst(p->opcode))); > } This actually should work. Prefixed instructions can be patched by patching the prefix with a trap or pnop, and by patching a trap/pnop back to the prefix instruction. pnop will make the suffix interpreted corretcly and skipped. trap handler will have to know it traps for a prefixed insn if it wanted to resume after the instructioni. So it is enough to save/restore the first 4 bytes of the instruction so long as there are checks to ensure we don't try to patch a suffix (which it looks like there are). Single-stepping pc-relative instructions at an alternate address could be a bigger problem if I read the kprobes code correctly, I don't really see how that would be handled with existing pc relative instructions actually like branches and addpcis. Maybe it always relies on being able to emulate those, but in that case we might not emulate all pcrel instructions? I'm not sure. If that is what kprobes relies on then it should be made more robust and have a can_single_step_at_alternate_location() filter for that. R=1 prefix could be caught there. Thanks, Nick
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile index 8a3d69b02672..ea88af26f8c6 100644 --- a/arch/powerpc/Makefile +++ b/arch/powerpc/Makefile @@ -192,9 +192,14 @@ ifdef CONFIG_476FPE_ERR46 -T $(srctree)/arch/powerpc/platforms/44x/ppc476_modules.lds endif -# No AltiVec or VSX instructions when building kernel +# No prefix or pcrel +KBUILD_CFLAGS += $(call cc-option,-mno-prefixed) +KBUILD_CFLAGS += $(call cc-option,-mno-pcrel) + +# No AltiVec or VSX or MMA instructions when building kernel KBUILD_CFLAGS += $(call cc-option,-mno-altivec) KBUILD_CFLAGS += $(call cc-option,-mno-vsx) +KBUILD_CFLAGS += $(call cc-option,-mno-mma) # No SPE instruction when building kernel # (We use all available options to help semi-broken compilers) diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype index 4017be72e46f..1f7c903ea664 100644 --- a/arch/powerpc/platforms/Kconfig.cputype +++ b/arch/powerpc/platforms/Kconfig.cputype @@ -171,6 +171,11 @@ config POWER9_CPU depends on PPC_BOOK3S_64 select ARCH_HAS_FAST_MULTIPLIER +config POWER10_CPU + bool "POWER10" + depends on PPC_BOOK3S_64 + select ARCH_HAS_FAST_MULTIPLIER + config E5500_CPU bool "Freescale e5500" depends on PPC64 && E500 @@ -239,6 +244,7 @@ config TARGET_CPU default "power7" if POWER7_CPU default "power8" if POWER8_CPU default "power9" if POWER9_CPU + default "power10" if POWER10_CPU default "405" if 405_CPU default "440" if 440_CPU default "464" if 464_CPU
This adds basic POWER10_CPU option, which builds with -mcpu=power10. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- There's quite a lot of asm and linker changes slated for the next merge window already so I may leave the pcrel patch for next time. I think we can add the basic POWER10 build option though. Thanks, Nick arch/powerpc/Makefile | 7 ++++++- arch/powerpc/platforms/Kconfig.cputype | 8 +++++++- 2 files changed, 13 insertions(+), 2 deletions(-)