[PATCH] powerpc/64s: POWER10 CPU Kconfig build option

[PATCH] powerpc/64s: POWER10 CPU Kconfig build option

[Nicholas Piggin]

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)
+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
-- 
2.37.2

Re: [PATCH] powerpc/64s: POWER10 CPU Kconfig build option

[Christophe Leroy]

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

Re: [PATCH] powerpc/64s: POWER10 CPU Kconfig build option

[Nicholas Piggin]

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

Re: [PATCH] powerpc/64s: POWER10 CPU Kconfig build option

[Christophe Leroy]

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

Re: [PATCH] powerpc/64s: POWER10 CPU Kconfig build option

[Segher Boessenkool]

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

Re: [PATCH] powerpc/64s: POWER10 CPU Kconfig build option

[Nicholas Piggin]

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

Re: [PATCH] powerpc/64s: POWER10 CPU Kconfig build option

[Nicholas Piggin]

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

Re: [PATCH] powerpc/64s: POWER10 CPU Kconfig build option

[Michael Ellerman]

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

Re: [PATCH] powerpc/64s: POWER10 CPU Kconfig build option

[Segher Boessenkool]

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

Re: [PATCH] powerpc/64s: POWER10 CPU Kconfig build option

[Nicholas Piggin]

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

Re: [PATCH] powerpc/64s: POWER10 CPU Kconfig build option

[Segher Boessenkool]

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

Re: [PATCH] powerpc/64s: POWER10 CPU Kconfig build option

[Nicholas Piggin]

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

Re: [PATCH] powerpc/64s: POWER10 CPU Kconfig build option

[Michael Ellerman]

"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

Re: [PATCH] powerpc/64s: POWER10 CPU Kconfig build option

[Segher Boessenkool]

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

Re: [PATCH] powerpc/64s: POWER10 CPU Kconfig build option

[Segher Boessenkool]

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