[PATCH] arm64: disable patchable function entry on big-endian clang builds

[PATCH] arm64: disable patchable function entry on big-endian clang builds

[Arnd Bergmann]

Clang only supports the patchable_function_entry attribute on
little-endian arm64 builds, but not on big-endian:

include/linux/kasan-checks.h:16:8: error: unknown attribute 'patchable_function_entry' ignored [-Werror,-Wunknown-attributes]

Disable that configuration with another dependency. Unfortunately
the existing check is not enough, as $(cc-option) at this point does
not pass the -mbig-endian flag.

Fixes: 3b23e4991fb6 ("arm64: implement ftrace with regs")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/arm64/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 4b256fa6db7a..a33d6402b934 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -151,7 +151,7 @@ config ARM64
 	select HAVE_DMA_CONTIGUOUS
 	select HAVE_DYNAMIC_FTRACE
 	select HAVE_DYNAMIC_FTRACE_WITH_REGS \
-		if $(cc-option,-fpatchable-function-entry=2)
+		if $(cc-option,-fpatchable-function-entry=2) && !(CC_IS_CLANG && CPU_BIG_ENDIAN)
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS
 	select HAVE_FAST_GUP
 	select HAVE_FTRACE_MCOUNT_RECORD
-- 
2.26.0

Re: [PATCH] arm64: disable patchable function entry on big-endian clang builds

[Mark Rutland]

On Tue, May 05, 2020 at 04:12:36PM +0200, Arnd Bergmann wrote:
> Clang only supports the patchable_function_entry attribute on
> little-endian arm64 builds, but not on big-endian:
> 
> include/linux/kasan-checks.h:16:8: error: unknown attribute 'patchable_function_entry' ignored [-Werror,-Wunknown-attributes]
> 
> Disable that configuration with another dependency. Unfortunately
> the existing check is not enough, as $(cc-option) at this point does
> not pass the -mbig-endian flag.

Well that's unfortunate. :(

Do we know if this is deliberate and/or likely to change in future? This
practically rules out a BE distro kernel with things like PAC, which
isn't ideal.

> Fixes: 3b23e4991fb6 ("arm64: implement ftrace with regs")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

This looks fine for now, and we can add a version check in future, so:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 4b256fa6db7a..a33d6402b934 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -151,7 +151,7 @@ config ARM64
>  	select HAVE_DMA_CONTIGUOUS
>  	select HAVE_DYNAMIC_FTRACE
>  	select HAVE_DYNAMIC_FTRACE_WITH_REGS \
> -		if $(cc-option,-fpatchable-function-entry=2)
> +		if $(cc-option,-fpatchable-function-entry=2) && !(CC_IS_CLANG && CPU_BIG_ENDIAN)
>  	select HAVE_EFFICIENT_UNALIGNED_ACCESS
>  	select HAVE_FAST_GUP
>  	select HAVE_FTRACE_MCOUNT_RECORD
> -- 
> 2.26.0
> 

Re: [PATCH] arm64: disable patchable function entry on big-endian clang builds

[Torsten Duwe]

Hi Arnd, Mark and others,

this may not be worth arguing but I'm currently fighting excessive
workarounds in another area and so this triggers me, so I have to make
a remark ;-)

On Tue, 5 May 2020 15:25:56 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> On Tue, May 05, 2020 at 04:12:36PM +0200, Arnd Bergmann wrote:
> > Clang only supports the patchable_function_entry attribute on
> > little-endian arm64 builds, but not on big-endian:
> > 
> > include/linux/kasan-checks.h:16:8: error: unknown attribute
> > 'patchable_function_entry' ignored [-Werror,-Wunknown-attributes]
> > 
> > Disable that configuration with another dependency. Unfortunately
> > the existing check is not enough, as $(cc-option) at this point does
> > not pass the -mbig-endian flag.
> 
> Well that's unfortunate. :(
> 
> Do we know if this is deliberate and/or likely to change in future?
> This practically rules out a BE distro kernel with things like PAC,
> which isn't ideal.

But still better than cumulating workarounds. If clang's flags aren't
orthogonal then that's a bug in clang. If I get a vote here I'm against
it.

> > Fixes: 3b23e4991fb6 ("arm64: implement ftrace with regs")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> This looks fine for now, and we can add a version check in future, so:
                                      ^^^^^^^^^^^^^^^^^^^
see what I mean? And in the end another line of code you'll never again
get rid of.

I suggest to get a quote from clang folks first about their schedule and
regarded importance of patchable-function-entries on BE, and leave it as
is: broken on certain clang configurations. It's not the kernel's fault.

> Acked-by: Mark Rutland <mark.rutland@arm.com>
> 
> Mark.
> 
> > ---
> >  arch/arm64/Kconfig | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > index 4b256fa6db7a..a33d6402b934 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -151,7 +151,7 @@ config ARM64
> >  	select HAVE_DMA_CONTIGUOUS
> >  	select HAVE_DYNAMIC_FTRACE
> >  	select HAVE_DYNAMIC_FTRACE_WITH_REGS \
> > -		if $(cc-option,-fies on y=2)
> > +		if $(cc-option,-fpatchable-function-entry=2) &&
> > !(CC_IS_CLANG && CPU_BIG_ENDIAN) select
> > HAVE_EFFICIENT_UNALIGNED_ACCESS select HAVE_FAST_GUP
> >  	select HAVE_FTRACE_MCOUNT_RECORD
> > -- 
> > 2.26.0
> > 

Re: [PATCH] arm64: disable patchable function entry on big-endian clang builds

[Nathan Chancellor]

+ Fangrui, who implemented patchable_function_entry in LLVM/clang

On Tue, May 05, 2020 at 07:42:43PM +0200, Torsten Duwe wrote:
> Hi Arnd, Mark and others,
> 
> this may not be worth arguing but I'm currently fighting excessive
> workarounds in another area and so this triggers me, so I have to make
> a remark ;-)
> 
> On Tue, 5 May 2020 15:25:56 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > On Tue, May 05, 2020 at 04:12:36PM +0200, Arnd Bergmann wrote:
> > > Clang only supports the patchable_function_entry attribute on
> > > little-endian arm64 builds, but not on big-endian:
> > > 
> > > include/linux/kasan-checks.h:16:8: error: unknown attribute
> > > 'patchable_function_entry' ignored [-Werror,-Wunknown-attributes]
> > > 
> > > Disable that configuration with another dependency. Unfortunately
> > > the existing check is not enough, as $(cc-option) at this point does
> > > not pass the -mbig-endian flag.
> > 
> > Well that's unfortunate. :(
> > 
> > Do we know if this is deliberate and/or likely to change in future?

I am not sure this is deliberate, I don't see anything about endianness
in the commits that added this:

https://github.com/llvm/llvm-project/commit/4d1e23e3b3cd7c72a8b24dc5acb7e13c58a8de37
https://github.com/llvm/llvm-project/commit/22467e259507f5ead2a87d989251b4c951a587e4
https://github.com/llvm/llvm-project/commit/06b8e32d4fd3f634f793e3bc0bc4fdb885e7a3ac

> > This practically rules out a BE distro kernel with things like PAC,
> > which isn't ideal.

To be fair, are there big endian AArch64 distros?

https://wiki.debian.org/Arm64Port: "There is also a big-endian version
of the architecture/ABI: aarch64_be-linux-gnu but we're not supporting
that in Debian (so there is no corresponding Debian architecture name)
and hopefully will never have to. Nevertheless you might want to check
for this by way of completeness in upstream code."

OpenSUSE and Fedora don't appear to have support for big endian.

> But still better than cumulating workarounds. If clang's flags aren't
> orthogonal then that's a bug in clang. If I get a vote here I'm against
> it.
> 
> > > Fixes: 3b23e4991fb6 ("arm64: implement ftrace with regs")
> > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > 
> > This looks fine for now, and we can add a version check in future, so:
>                                       ^^^^^^^^^^^^^^^^^^^
> see what I mean? And in the end another line of code you'll never again
> get rid of.

That's a rather pessimistic attitude to have. We've been rather good
about trying to fix stuff in the compiler rather than hacking up the
kernel.

> I suggest to get a quote from clang folks first about their schedule and
> regarded importance of patchable-function-entries on BE, and leave it as
> is: broken on certain clang configurations. It's not the kernel's fault.

We can file an upstream PR (https://bugs.llvm.org) to talk about this
(although I've CC'd Fangrui) but you would rather the kernel fail to
work properly than prevent the user from being able to select that
option? Why even have the "select" or "depends on" keyword then?

That said, I do think we should hold off on this patch until we hear
from the LLVM developers.

> > Acked-by: Mark Rutland <mark.rutland@arm.com>
> > 
> > Mark.
> > 
> > > ---
> > >  arch/arm64/Kconfig | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > > index 4b256fa6db7a..a33d6402b934 100644
> > > --- a/arch/arm64/Kconfig
> > > +++ b/arch/arm64/Kconfig
> > > @@ -151,7 +151,7 @@ config ARM64
> > >  	select HAVE_DMA_CONTIGUOUS
> > >  	select HAVE_DYNAMIC_FTRACE
> > >  	select HAVE_DYNAMIC_FTRACE_WITH_REGS \
> > > -		if $(cc-option,-fies on y=2)
> > > +		if $(cc-option,-fpatchable-function-entry=2) &&
> > > !(CC_IS_CLANG && CPU_BIG_ENDIAN) select
> > > HAVE_EFFICIENT_UNALIGNED_ACCESS select HAVE_FAST_GUP
> > >  	select HAVE_FTRACE_MCOUNT_RECORD
> > > -- 
> > > 2.26.0
> > > 
> 

Cheers,
Nathan

Re: [PATCH] arm64: disable patchable function entry on big-endian clang builds

[Arnd Bergmann]

On Wed, May 6, 2020 at 5:45 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
> On Tue, May 05, 2020 at 07:42:43PM +0200, Torsten Duwe wrote:
> > On Tue, 5 May 2020 15:25:56 +0100 Mark Rutland <mark.rutland@arm.com> wrote:
> > > On Tue, May 05, 2020 at 04:12:36PM +0200, Arnd Bergmann wrote:
> > > This practically rules out a BE distro kernel with things like PAC,
> > > which isn't ideal.
>
> To be fair, are there big endian AArch64 distros?
>
> https://wiki.debian.org/Arm64Port: "There is also a big-endian version
> of the architecture/ABI: aarch64_be-linux-gnu but we're not supporting
> that in Debian (so there is no corresponding Debian architecture name)
> and hopefully will never have to. Nevertheless you might want to check
> for this by way of completeness in upstream code."
>
> OpenSUSE and Fedora don't appear to have support for big endian.

I don't think any of the binary distros ship big endian ARM64. There are
a couple of users that tend to build everything from source using Yocto
or similar embedded distros, but as far as I can tell this is getting less
common over time as applications get ported to be compatible with
big-endian, or get phased out and replaced by code running on regular
little-endian systems.

The users we see today are likely in telco, military or aerospace
settings (While earth is mostly little-endian these days, space is
definitely big-endian) that got ported from big-endian hardware, but
often with a high degree of customization and long service life.

My policy for Arm specific kernel code submissions is generally that
it should be written so it can work on either big-endian or little-endian
using the available abstractions (just like any architecture independent
code), but I don't normally expect it to be tested on big endian.

There are some important examples of code that just doesn't work
on big-endian because it's far too hard, e.g. the UEFI runtime services.
That is also ok, if anyone really needs it, they can do the work.

> > I suggest to get a quote from clang folks first about their schedule and
> > regarded importance of patchable-function-entries on BE, and leave it as
> > is: broken on certain clang configurations. It's not the kernel's fault.
>
> We can file an upstream PR (https://bugs.llvm.org) to talk about this
> (although I've CC'd Fangrui) but you would rather the kernel fail to
> work properly than prevent the user from being able to select that
> option? Why even have the "select" or "depends on" keyword then?

I definitely want all randconfig kernels to build without warnings,
and I agree with you that making it just fail at build time is not
a good solution.

> That said, I do think we should hold off on this patch until we hear
> from the LLVM developers.

+1

      Arnd

Re: [PATCH] arm64: disable patchable function entry on big-endian clang builds

[Nathan Chancellor]

On Wed, May 06, 2020 at 12:22:58PM +0200, Arnd Bergmann wrote:
> On Wed, May 6, 2020 at 5:45 AM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> > On Tue, May 05, 2020 at 07:42:43PM +0200, Torsten Duwe wrote:
> > > On Tue, 5 May 2020 15:25:56 +0100 Mark Rutland <mark.rutland@arm.com> wrote:
> > > > On Tue, May 05, 2020 at 04:12:36PM +0200, Arnd Bergmann wrote:
> > > > This practically rules out a BE distro kernel with things like PAC,
> > > > which isn't ideal.
> >
> > To be fair, are there big endian AArch64 distros?
> >
> > https://wiki.debian.org/Arm64Port: "There is also a big-endian version
> > of the architecture/ABI: aarch64_be-linux-gnu but we're not supporting
> > that in Debian (so there is no corresponding Debian architecture name)
> > and hopefully will never have to. Nevertheless you might want to check
> > for this by way of completeness in upstream code."
> >
> > OpenSUSE and Fedora don't appear to have support for big endian.
> 
> I don't think any of the binary distros ship big endian ARM64. There are
> a couple of users that tend to build everything from source using Yocto
> or similar embedded distros, but as far as I can tell this is getting less
> common over time as applications get ported to be compatible with
> big-endian, or get phased out and replaced by code running on regular
> little-endian systems.
> 
> The users we see today are likely in telco, military or aerospace
> settings (While earth is mostly little-endian these days, space is
> definitely big-endian) that got ported from big-endian hardware, but
> often with a high degree of customization and long service life.

Ah yes, that makes sense, thanks for the information and background.
Helps orient myself for the future.

> My policy for Arm specific kernel code submissions is generally that
> it should be written so it can work on either big-endian or little-endian
> using the available abstractions (just like any architecture independent
> code), but I don't normally expect it to be tested on big endian.
> 
> There are some important examples of code that just doesn't work
> on big-endian because it's far too hard, e.g. the UEFI runtime services.
> That is also ok, if anyone really needs it, they can do the work.
> 
> > > I suggest to get a quote from clang folks first about their schedule and
> > > regarded importance of patchable-function-entries on BE, and leave it as
> > > is: broken on certain clang configurations. It's not the kernel's fault.
> >
> > We can file an upstream PR (https://bugs.llvm.org) to talk about this
> > (although I've CC'd Fangrui) but you would rather the kernel fail to
> > work properly than prevent the user from being able to select that
> > option? Why even have the "select" or "depends on" keyword then?
> 
> I definitely want all randconfig kernels to build without warnings,
> and I agree with you that making it just fail at build time is not
> a good solution.
> 
> > That said, I do think we should hold off on this patch until we hear
> > from the LLVM developers.
> 
> +1
> 
>       Arnd

Glad we are on the same page.

Cheers,
Nathan

Re: [PATCH] arm64: disable patchable function entry on big-endian clang builds

[Fangrui Song]

On 2020-05-06, Nathan Chancellor wrote:
>On Wed, May 06, 2020 at 12:22:58PM +0200, Arnd Bergmann wrote:
>> On Wed, May 6, 2020 at 5:45 AM Nathan Chancellor
>> <natechancellor@gmail.com> wrote:
>> > On Tue, May 05, 2020 at 07:42:43PM +0200, Torsten Duwe wrote:
>> > > On Tue, 5 May 2020 15:25:56 +0100 Mark Rutland <mark.rutland@arm.com> wrote:
>> > > > On Tue, May 05, 2020 at 04:12:36PM +0200, Arnd Bergmann wrote:
>> > > > This practically rules out a BE distro kernel with things like PAC,
>> > > > which isn't ideal.
>> >
>> > To be fair, are there big endian AArch64 distros?
>> >
>> > https://wiki.debian.org/Arm64Port: "There is also a big-endian version
>> > of the architecture/ABI: aarch64_be-linux-gnu but we're not supporting
>> > that in Debian (so there is no corresponding Debian architecture name)
>> > and hopefully will never have to. Nevertheless you might want to check
>> > for this by way of completeness in upstream code."
>> >
>> > OpenSUSE and Fedora don't appear to have support for big endian.
>>
>> I don't think any of the binary distros ship big endian ARM64. There are
>> a couple of users that tend to build everything from source using Yocto
>> or similar embedded distros, but as far as I can tell this is getting less
>> common over time as applications get ported to be compatible with
>> big-endian, or get phased out and replaced by code running on regular
>> little-endian systems.
>>
>> The users we see today are likely in telco, military or aerospace
>> settings (While earth is mostly little-endian these days, space is
>> definitely big-endian) that got ported from big-endian hardware, but
>> often with a high degree of customization and long service life.
>
>Ah yes, that makes sense, thanks for the information and background.
>Helps orient myself for the future.
>
>> My policy for Arm specific kernel code submissions is generally that
>> it should be written so it can work on either big-endian or little-endian
>> using the available abstractions (just like any architecture independent
>> code), but I don't normally expect it to be tested on big endian.
>>
>> There are some important examples of code that just doesn't work
>> on big-endian because it's far too hard, e.g. the UEFI runtime services.
>> That is also ok, if anyone really needs it, they can do the work.
>>
>> > > I suggest to get a quote from clang folks first about their schedule and
>> > > regarded importance of patchable-function-entries on BE, and leave it as
>> > > is: broken on certain clang configurations. It's not the kernel's fault.
>> >
>> > We can file an upstream PR (https://bugs.llvm.org) to talk about this
>> > (although I've CC'd Fangrui) but you would rather the kernel fail to
>> > work properly than prevent the user from being able to select that
>> > option? Why even have the "select" or "depends on" keyword then?

Created https://reviews.llvm.org/D79495 to allow the function attribute
'patchable_function_entry' on aarch64_be.
I think -fpatchable-function-entry= just works.

Note, LLD does not support aarch64_be
(https://github.com/ClangBuiltLinux/linux/issues/380).

>> I definitely want all randconfig kernels to build without warnings,
>> and I agree with you that making it just fail at build time is not
>> a good solution.
>>
>> > That said, I do think we should hold off on this patch until we hear
>> > from the LLVM developers.
>>
>> +1
>>
>>       Arnd
>
>Glad we are on the same page.
>
>Cheers,
>Nathan

Re: [PATCH] arm64: disable patchable function entry on big-endian clang builds

[Nick Desaulniers]

On Wed, May 6, 2020 at 8:46 AM 'Fangrui Song' via Clang Built Linux
<clang-built-linux@googlegroups.com> wrote:
> Created https://reviews.llvm.org/D79495 to allow the function attribute
> 'patchable_function_entry' on aarch64_be.
> I think -fpatchable-function-entry= just works.
>
> Note, LLD does not support aarch64_be
> (https://github.com/ClangBuiltLinux/linux/issues/380).

I've approved the patch. Thanks for the quick fix.  Looks like we
backported -fpatchable-function-entry= to the clang-10 release, so we
should cherry pick your fix to the release-10 branch for the clang
10.1 release.

I'd rather have this fixed on the toolchain side.

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] arm64: disable patchable function entry on big-endian clang builds

[Fangrui Song]

On 2020-05-06, Nick Desaulniers wrote:
>On Wed, May 6, 2020 at 8:46 AM 'Fangrui Song' via Clang Built Linux
><clang-built-linux@googlegroups.com> wrote:
>> Created https://reviews.llvm.org/D79495 to allow the function attribute
>> 'patchable_function_entry' on aarch64_be.
>> I think -fpatchable-function-entry= just works.
>>
>> Note, LLD does not support aarch64_be
>> (https://github.com/ClangBuiltLinux/linux/issues/380).
>
>I've approved the patch. Thanks for the quick fix.  Looks like we
>backported -fpatchable-function-entry= to the clang-10 release, so we
>should cherry pick your fix to the release-10 branch for the clang
>10.1 release.
>
>I'd rather have this fixed on the toolchain side.

+1.

Cherry picked to release/10.x
https://github.com/llvm/llvm-project/commit/98f9f73f6d2367aa8001c4d16de9d3b347febb08
I did not use any endianness-sensitive in the original implementation,
so hopefully this will not run into issues.


The scheduled rc1 of LLVM 10.0.1 will happen on May 18, 2020
(https://lists.llvm.org/pipermail/llvm-dev/2020-April/141128.html)
We should be quick if we want to test it on qemu or real hardware.