Re: [PATCH 3/3] powerpc: Include -m32 / -m64 for stack protector Kconfig test

[Nathan Chancellor <nathan@kernel.org>]

On Tue, Oct 08, 2024 at 07:14:26AM +0200, Christophe Leroy wrote:
> Le 08/10/2024 à 06:22, Nathan Chancellor a écrit :
> > Kbuild uses the powerpc64le-linux-gnu target for clang, which causes the
> > Kconfig check for 32-bit powerpc stack protector support to fail because
> > nothing flips the target to 32-bit:
> > 
> >    $ clang --target=powerpc64le-linux-gnu \
> >            -mstack-protector-guard=tls
> >            -mstack-protector-guard-reg=r2 \
> >            -mstack-protector-guard-offset=0 \
> >            -x c -c -o /dev/null /dev/null
> >    clang: error: invalid value 'r2' in 'mstack-protector-guard-reg=', expected one of: r13
> 
> Why is there any restriction at all on which register can be used ? I can't
> see such restriction in GCC documentation :
> https://gcc.gnu.org/onlinedocs/gcc/RS_002f6000-and-PowerPC-Options.html

Keith may be able to answer this better than I can but I believe this is
an existing restriction in the LLVM code that dates back to the original
stack protector support:

https://github.com/llvm/llvm-project/commit/a1d8bc559761c94dded3d1e7200cb264a982d1c5

It always uses r2 for 32-bit and r13 for 64-bit for loading the stack
canary. Keith's patch basically just allows customizing the offset,
rather than the register (ultimately for simplicity in the compiler
patch I believe, since we really only care about supporting what the
kernel uses).

> > Use the Kconfig macro '$(m32-flag)', which expands to '-m32' when
> > supported, in the stack protector support cc-option call to properly
> > switch the target to a 32-bit one, which matches what happens in Kbuild.
> > While the 64-bit macro does not strictly need it, add the equivalent
> > 64-bit option for symmetry.
> > 
> > Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> > ---
> >   arch/powerpc/Kconfig | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > index eb98050b8c016bb23887a9d669d29e69d933c9c8..6aaca48955a34b2a38af1415bfa36f74f35c3f3e 100644
> > --- a/arch/powerpc/Kconfig
> > +++ b/arch/powerpc/Kconfig
> > @@ -275,8 +275,8 @@ config PPC
> >   	select HAVE_RSEQ
> >   	select HAVE_SETUP_PER_CPU_AREA		if PPC64
> >   	select HAVE_SOFTIRQ_ON_OWN_STACK
> > -	select HAVE_STACKPROTECTOR		if PPC32 && $(cc-option,-mstack-protector-guard=tls -mstack-protector-guard-reg=r2 -mstack-protector-guard-offset=0)
> > -	select HAVE_STACKPROTECTOR		if PPC64 && $(cc-option,-mstack-protector-guard=tls -mstack-protector-guard-reg=r13 -mstack-protector-guard-offset=0)
> > +	select HAVE_STACKPROTECTOR		if PPC32 && $(cc-option,$(m32-flag) -mstack-protector-guard=tls -mstack-protector-guard-reg=r2 -mstack-protector-guard-offset=0)
> > +	select HAVE_STACKPROTECTOR		if PPC64 && $(cc-option,$(m64-flag) -mstack-protector-guard=tls -mstack-protector-guard-reg=r13 -mstack-protector-guard-offset=0)
> 
> You modify the exact same line than Patch 1, if this patch is really
> required it should be squashed into patch 1 I think.

I believe it will still be required based on my comment above. I can
certainly squash it into patch 1, although the commit message might get
a bit wordy with both explanations in there. I suppose it still makes
sense to do so since "fix the Kconfig test" can encompass both issues
easily.

Cheers,
Nathan