All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC arm64] samples/bpf: explicitly exclude sysreg sections with asm macros
@ 2017-03-09 23:18 Andy Gospodarek
  2017-03-10 17:52 ` Will Deacon
  2017-03-10 18:13 ` Mark Rutland
  0 siblings, 2 replies; 17+ messages in thread
From: Andy Gospodarek @ 2017-03-09 23:18 UTC (permalink / raw)
  To: linux-arm-kernel

The previous fix to workaround compilation issues for samples/bpf on arm64 was
to prevent inclusion of code in sysregs.h.  This is not sustainable at this
point since other header files need access to sysregs.h for multiple
defintions.  The fact that the bpf samples cannot be compiled on arm64 is
fairly well documented on the iovisor-dev mailing and other places without a
clear-cut solution other than waiting for the llvm fix.  This attempts to
resolve that by dropping the fix to define _ASM_SYSREG_H on the clang command
line, and replace it with one that is wrapped around the offending code.  I
recognize this feels a bit fragile, but the current situation is not great
either and it seems that we are more likely to see users if the current code
actually compiles and runs.

Despite now being able to compile and run bpf programs on arm64 there
are still known warnings when building:

./include/net/sock.h:2322:35: warning: value size does not match
register size specified by the constraint and modifier
[-Wasm-operand-widths]
        smp_store_release(&sk->sk_state, newstate);
                                         ^
./include/net/sock.h:2322:2: note: use constraint modifier "w"
        smp_store_release(&sk->sk_state, newstate);
        ^
./include/asm-generic/barrier.h:157:33: note: expanded from macro
                                ^
./arch/arm64/include/asm/barrier.h:62:23: note: expanded from macro
'__smp_store_release'
                asm volatile ("stlr %1, %0"                     \
                                    ^
Discussed here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63359

Fixes: 30b50aa61201 ("bpf: samples: exclude asm/sysreg.h for arm64")
Signed-off-by: Andy Gospodarek <gospo@broadcom.com>
---
 arch/arm64/include/asm/sysreg.h | 3 ++-
 samples/bpf/Makefile            | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index ac24b6e..91ee2db 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -287,6 +287,7 @@
 #else
 
 #include <linux/types.h>
+#ifndef __AVOID_ASM_MACROS__
 
 asm(
 "	.irp	num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30\n"
@@ -302,7 +303,7 @@ asm(
 	__emit_inst(0xd5000000|(\\sreg)|(.L__reg_num_\\rt))
 "	.endm\n"
 );
-
+#endif
 /*
  * Unlike read_cpuid, calls to read_sysreg are never expected to be
  * optimized away or replaced with synthetic values.
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 09e9d53..fca2c13 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -178,7 +178,7 @@ $(src)/*.c: verify_target_bpf
 # useless for BPF samples.
 $(obj)/%.o: $(src)/%.c
 	$(CLANG) $(NOSTDINC_FLAGS) $(LINUXINCLUDE) $(EXTRA_CFLAGS) \
-		-D__KERNEL__ -D__ASM_SYSREG_H -Wno-unused-value -Wno-pointer-sign \
+		-D__KERNEL__ -D__AVOID_ASM_MACROS__ -Wno-unused-value -Wno-pointer-sign \
 		-Wno-compare-distinct-pointer-types \
 		-Wno-gnu-variable-sized-type-not-at-end \
 		-Wno-address-of-packed-member -Wno-tautological-compare \
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [RFC arm64] samples/bpf: explicitly exclude sysreg sections with asm macros
  2017-03-09 23:18 [RFC arm64] samples/bpf: explicitly exclude sysreg sections with asm macros Andy Gospodarek
@ 2017-03-10 17:52 ` Will Deacon
  2017-03-10 19:26   ` Andy Gospodarek
  2017-03-10 18:13 ` Mark Rutland
  1 sibling, 1 reply; 17+ messages in thread
From: Will Deacon @ 2017-03-10 17:52 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Andy,

On Thu, Mar 09, 2017 at 06:18:12PM -0500, Andy Gospodarek wrote:
> The previous fix to workaround compilation issues for samples/bpf on arm64 was
> to prevent inclusion of code in sysregs.h.  This is not sustainable at this
> point since other header files need access to sysregs.h for multiple
> defintions.  The fact that the bpf samples cannot be compiled on arm64 is
> fairly well documented on the iovisor-dev mailing and other places without a
> clear-cut solution other than waiting for the llvm fix.  This attempts to
> resolve that by dropping the fix to define _ASM_SYSREG_H on the clang command
> line, and replace it with one that is wrapped around the offending code.  I
> recognize this feels a bit fragile, but the current situation is not great
> either and it seems that we are more likely to see users if the current code
> actually compiles and runs.

Why does asm cause compilation to fail?

> Despite now being able to compile and run bpf programs on arm64 there
> are still known warnings when building:
> 
> ./include/net/sock.h:2322:35: warning: value size does not match
> register size specified by the constraint and modifier
> [-Wasm-operand-widths]
>         smp_store_release(&sk->sk_state, newstate);
>                                          ^
> ./include/net/sock.h:2322:2: note: use constraint modifier "w"
>         smp_store_release(&sk->sk_state, newstate);
>         ^
> ./include/asm-generic/barrier.h:157:33: note: expanded from macro
>                                 ^
> ./arch/arm64/include/asm/barrier.h:62:23: note: expanded from macro
> '__smp_store_release'
>                 asm volatile ("stlr %1, %0"                     \

I don't think this is relevant to the message for this commit.

> Discussed here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63359
> 
> Fixes: 30b50aa61201 ("bpf: samples: exclude asm/sysreg.h for arm64")
> Signed-off-by: Andy Gospodarek <gospo@broadcom.com>
> ---
>  arch/arm64/include/asm/sysreg.h | 3 ++-
>  samples/bpf/Makefile            | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index ac24b6e..91ee2db 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -287,6 +287,7 @@
>  #else
>  
>  #include <linux/types.h>
> +#ifndef __AVOID_ASM_MACROS__

Yeah, this is pretty horrible. It would prefer to have a script that
generates the header files needed by clang, without adding these ifdefs
whenever something new fails to compile, but I don't understand why things
are failing at all at the moment.

Will

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC arm64] samples/bpf: explicitly exclude sysreg sections with asm macros
  2017-03-09 23:18 [RFC arm64] samples/bpf: explicitly exclude sysreg sections with asm macros Andy Gospodarek
  2017-03-10 17:52 ` Will Deacon
@ 2017-03-10 18:13 ` Mark Rutland
  2017-03-10 19:35   ` Andy Gospodarek
  1 sibling, 1 reply; 17+ messages in thread
From: Mark Rutland @ 2017-03-10 18:13 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Mar 09, 2017 at 06:18:12PM -0500, Andy Gospodarek wrote:
> The previous fix to workaround compilation issues for samples/bpf on arm64 was
> to prevent inclusion of code in sysregs.h.  This is not sustainable at this
> point since other header files need access to sysregs.h for multiple
> defintions.  The fact that the bpf samples cannot be compiled on arm64 is
> fairly well documented on the iovisor-dev mailing and other places without a
> clear-cut solution other than waiting for the llvm fix.  This attempts to
> resolve that by dropping the fix to define _ASM_SYSREG_H on the clang command
> line, and replace it with one that is wrapped around the offending code.  I
> recognize this feels a bit fragile, but the current situation is not great
> either and it seems that we are more likely to see users if the current code
> actually compiles and runs.
> 
> Despite now being able to compile and run bpf programs on arm64 there
> are still known warnings when building:
> 
> ./include/net/sock.h:2322:35: warning: value size does not match
> register size specified by the constraint and modifier
> [-Wasm-operand-widths]
>         smp_store_release(&sk->sk_state, newstate);
>                                          ^
> ./include/net/sock.h:2322:2: note: use constraint modifier "w"
>         smp_store_release(&sk->sk_state, newstate);
>         ^
> ./include/asm-generic/barrier.h:157:33: note: expanded from macro
>                                 ^
> ./arch/arm64/include/asm/barrier.h:62:23: note: expanded from macro
> '__smp_store_release'
>                 asm volatile ("stlr %1, %0"                     \

These examples have nothing to do with sysreg.h, so that implies more of
this is going to be necessary...

> Discussed here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63359
> 
> Fixes: 30b50aa61201 ("bpf: samples: exclude asm/sysreg.h for arm64")
> Signed-off-by: Andy Gospodarek <gospo@broadcom.com>
> ---
>  arch/arm64/include/asm/sysreg.h | 3 ++-
>  samples/bpf/Makefile            | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index ac24b6e..91ee2db 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -287,6 +287,7 @@
>  #else
>  
>  #include <linux/types.h>
> +#ifndef __AVOID_ASM_MACROS__
>  
>  asm(
>  "	.irp	num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30\n"
> @@ -302,7 +303,7 @@ asm(
>  	__emit_inst(0xd5000000|(\\sreg)|(.L__reg_num_\\rt))
>  "	.endm\n"
>  );
> -
> +#endif

... and this is stupidly messy.

>From my PoV, NAK to having to sprinkle this in headers.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC arm64] samples/bpf: explicitly exclude sysreg sections with asm macros
  2017-03-10 17:52 ` Will Deacon
@ 2017-03-10 19:26   ` Andy Gospodarek
  2017-03-10 20:41     ` Mark Rutland
  0 siblings, 1 reply; 17+ messages in thread
From: Andy Gospodarek @ 2017-03-10 19:26 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Mar 10, 2017 at 05:52:30PM +0000, Will Deacon wrote:
> Hi Andy,
> 
> On Thu, Mar 09, 2017 at 06:18:12PM -0500, Andy Gospodarek wrote:
> > The previous fix to workaround compilation issues for samples/bpf on arm64 was
> > to prevent inclusion of code in sysregs.h.  This is not sustainable at this
> > point since other header files need access to sysregs.h for multiple
> > defintions.  The fact that the bpf samples cannot be compiled on arm64 is
> > fairly well documented on the iovisor-dev mailing and other places without a
> > clear-cut solution other than waiting for the llvm fix.  This attempts to
> > resolve that by dropping the fix to define _ASM_SYSREG_H on the clang command
> > line, and replace it with one that is wrapped around the offending code.  I
> > recognize this feels a bit fragile, but the current situation is not great
> > either and it seems that we are more likely to see users if the current code
> > actually compiles and runs.
> 
> Why does asm cause compilation to fail?
> 

It isn't the ASM itself that causes the compilation to fail, it's the
ASM macros included inside the new ifdef that are problematic.  Here is
what is seen per object file:

# make M=samples/bpf 
  HOSTCC  samples/bpf/xdp1_user.o
  HOSTLD  samples/bpf/xdp1
  HOSTLD  samples/bpf/xdp2
clang  -nostdinc -isystem /usr/lib/gcc/aarch64-linux-gnu/5/include
-I./arch/arm64/include -I./arch/arm64/include/generated/uapi
-I./arch/arm64/include/generated  -I./include
-I./arch/arm64/include/uapi -I./include/uapi -I./include/generated/uapi
-include ./include/linux/kconfig.h  \
	-D__KERNEL__ -D__AVOID_ASM_MACROS__ -Wno-unused-value
-Wno-pointer-sign \
	-Wno-compare-distinct-pointer-types \
	-Wno-gnu-variable-sized-type-not-at-end \
	-Wno-address-of-packed-member -Wno-tautological-compare \
	-O2 -emit-llvm -c samples/bpf/xdp1_kern.c -o -| llc -march=bpf
-filetype=obj -o samples/bpf/xdp1_kern.o
In file included from samples/bpf/xdp1_kern.c:14:
In file included from ./include/linux/ipv6.h:82:
In file included from ./include/linux/tcp.h:23:
./include/net/sock.h:2319:35: warning: value size does not match
register size specified by the constraint and modifier
[-Wasm-operand-widths]
        smp_store_release(&sk->sk_state, newstate);
                                         ^
./include/net/sock.h:2319:2: note: use constraint modifier "w"
        smp_store_release(&sk->sk_state, newstate);
        ^
./include/asm-generic/barrier.h:157:33: note: expanded from macro
'smp_store_release'
#define smp_store_release(p, v) __smp_store_release(p, v)
                                ^
./arch/arm64/include/asm/barrier.h:62:23: note: expanded from macro
'__smp_store_release'
                asm volatile ("stlr %1, %0"
\
                                    ^
1 warning generated.
LLVM ERROR: Inline asm not supported by this streamer because we don't  <----  THIS LINE
have an asm parser for this target

samples/bpf/Makefile:180: recipe for target 'samples/bpf/xdp1_kern.o'
failed
make[1]: *** [samples/bpf/xdp1_kern.o] Error 1
Makefile:1490: recipe for target '_module_samples/bpf' failed
make: *** [_module_samples/bpf] Error 2

> > Despite now being able to compile and run bpf programs on arm64 there
> > are still known warnings when building:
> > 
> > ./include/net/sock.h:2322:35: warning: value size does not match
> > register size specified by the constraint and modifier
> > [-Wasm-operand-widths]
> >         smp_store_release(&sk->sk_state, newstate);
> >                                          ^
> > ./include/net/sock.h:2322:2: note: use constraint modifier "w"
> >         smp_store_release(&sk->sk_state, newstate);
> >         ^
> > ./include/asm-generic/barrier.h:157:33: note: expanded from macro
> >                                 ^
> > ./arch/arm64/include/asm/barrier.h:62:23: note: expanded from macro
> > '__smp_store_release'
> >                 asm volatile ("stlr %1, %0"                     \
> 
> I don't think this is relevant to the message for this commit.
> 

I'm happy to drop that from the final commit.  Thanks for that feedback.

One of my goals with this RFC was to be excplicit that adding the
functionality to exclude the ASM macros doesn't magically fix
everything.


> > Discussed here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63359
> > 
> > Fixes: 30b50aa61201 ("bpf: samples: exclude asm/sysreg.h for arm64")
> > Signed-off-by: Andy Gospodarek <gospo@broadcom.com>
> > ---
> >  arch/arm64/include/asm/sysreg.h | 3 ++-
> >  samples/bpf/Makefile            | 2 +-
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> > index ac24b6e..91ee2db 100644
> > --- a/arch/arm64/include/asm/sysreg.h
> > +++ b/arch/arm64/include/asm/sysreg.h
> > @@ -287,6 +287,7 @@
> >  #else
> >  
> >  #include <linux/types.h>
> > +#ifndef __AVOID_ASM_MACROS__
> 
> Yeah, this is pretty horrible. It would prefer to have a script that
> generates the header files needed by clang, without adding these ifdefs
> whenever something new fails to compile, but I don't understand why things
> are failing at all at the moment.

I agree it's not great.

I'm not sure I want to see multiple copies of the same file in the tree
(all of these files are in the kernel tree).  We could consider
something like the include/ vs include/uapi split (of course this isn't
a uapi issue but I suspect you know what I mrean).

One other solution would be to move these macros out to another file
(like sysreg-macros.h or similar) and then continue to use a trick
similar to what we are using not to exclude those macros.  It would need
to be clear to ARCH maintainers that these macros would need to be there
for the forseeable future.  
> 
> Will

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC arm64] samples/bpf: explicitly exclude sysreg sections with asm macros
  2017-03-10 18:13 ` Mark Rutland
@ 2017-03-10 19:35   ` Andy Gospodarek
  0 siblings, 0 replies; 17+ messages in thread
From: Andy Gospodarek @ 2017-03-10 19:35 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Mar 10, 2017 at 06:13:30PM +0000, Mark Rutland wrote:
> On Thu, Mar 09, 2017 at 06:18:12PM -0500, Andy Gospodarek wrote:
> > The previous fix to workaround compilation issues for samples/bpf on arm64 was
> > to prevent inclusion of code in sysregs.h.  This is not sustainable at this
> > point since other header files need access to sysregs.h for multiple
> > defintions.  The fact that the bpf samples cannot be compiled on arm64 is
> > fairly well documented on the iovisor-dev mailing and other places without a
> > clear-cut solution other than waiting for the llvm fix.  This attempts to
> > resolve that by dropping the fix to define _ASM_SYSREG_H on the clang command
> > line, and replace it with one that is wrapped around the offending code.  I
> > recognize this feels a bit fragile, but the current situation is not great
> > either and it seems that we are more likely to see users if the current code
> > actually compiles and runs.
> > 
> > Despite now being able to compile and run bpf programs on arm64 there
> > are still known warnings when building:
> > 
> > ./include/net/sock.h:2322:35: warning: value size does not match
> > register size specified by the constraint and modifier
> > [-Wasm-operand-widths]
> >         smp_store_release(&sk->sk_state, newstate);
> >                                          ^
> > ./include/net/sock.h:2322:2: note: use constraint modifier "w"
> >         smp_store_release(&sk->sk_state, newstate);
> >         ^
> > ./include/asm-generic/barrier.h:157:33: note: expanded from macro
> >                                 ^
> > ./arch/arm64/include/asm/barrier.h:62:23: note: expanded from macro
> > '__smp_store_release'
> >                 asm volatile ("stlr %1, %0"                     \
> 
> These examples have nothing to do with sysreg.h, so that implies more of
> this is going to be necessary...

Sorry I included this bit of information -- it appears this is causing
some confusion.  You are correct this is unrelated to sysreg.h -- it is
just the warnings still generated when compiling the tools.  I'll drop
this from the final changelog.

> > Discussed here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63359
> > 
> > Fixes: 30b50aa61201 ("bpf: samples: exclude asm/sysreg.h for arm64")
> > Signed-off-by: Andy Gospodarek <gospo@broadcom.com>
> > ---
> >  arch/arm64/include/asm/sysreg.h | 3 ++-
> >  samples/bpf/Makefile            | 2 +-
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> > index ac24b6e..91ee2db 100644
> > --- a/arch/arm64/include/asm/sysreg.h
> > +++ b/arch/arm64/include/asm/sysreg.h
> > @@ -287,6 +287,7 @@
> >  #else
> >  
> >  #include <linux/types.h>
> > +#ifndef __AVOID_ASM_MACROS__
> >  
> >  asm(
> >  "	.irp	num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30\n"
> > @@ -302,7 +303,7 @@ asm(
> >  	__emit_inst(0xd5000000|(\\sreg)|(.L__reg_num_\\rt))
> >  "	.endm\n"
> >  );
> > -
> > +#endif
> 
> ... and this is stupidly messy.
> 
> From my PoV, NAK to having to sprinkle this in headers.

That's fine with me, but I'm hopeful that if enough heads get together
we can come up with a solution to deal with this.  Check out my reply to
Will and see if those other options seem like less maintenance-heavy
options.

Ultimately it seems to me like movement of some of those code out of
sysregs.h feels like the correct answer -- just hoping for some help
from those who may have more responsibiliy for long term maint.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC arm64] samples/bpf: explicitly exclude sysreg sections with asm macros
  2017-03-10 19:26   ` Andy Gospodarek
@ 2017-03-10 20:41     ` Mark Rutland
  2017-03-15 18:31       ` Andy Gospodarek
  0 siblings, 1 reply; 17+ messages in thread
From: Mark Rutland @ 2017-03-10 20:41 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

On Fri, Mar 10, 2017 at 02:26:56PM -0500, Andy Gospodarek wrote:
> On Fri, Mar 10, 2017 at 05:52:30PM +0000, Will Deacon wrote:
> > On Thu, Mar 09, 2017 at 06:18:12PM -0500, Andy Gospodarek wrote:
> > > The previous fix to workaround compilation issues for samples/bpf on arm64 was
> > > to prevent inclusion of code in sysregs.h.  This is not sustainable at this
> > > point since other header files need access to sysregs.h for multiple
> > > defintions.  The fact that the bpf samples cannot be compiled on arm64 is
> > > fairly well documented on the iovisor-dev mailing and other places without a
> > > clear-cut solution other than waiting for the llvm fix.  This attempts to
> > > resolve that by dropping the fix to define _ASM_SYSREG_H on the clang command
> > > line, and replace it with one that is wrapped around the offending code.  I
> > > recognize this feels a bit fragile, but the current situation is not great
> > > either and it seems that we are more likely to see users if the current code
> > > actually compiles and runs.
> > 
> > Why does asm cause compilation to fail?
> 
> It isn't the ASM itself that causes the compilation to fail, it's the
> ASM macros included inside the new ifdef that are problematic.  Here is
> what is seen per object file:

> ./arch/arm64/include/asm/barrier.h:62:23: note: expanded from macro
> '__smp_store_release'
>                 asm volatile ("stlr %1, %0"
> \
>                                     ^
> 1 warning generated.
> LLVM ERROR: Inline asm not supported by this streamer because we don't  <----  THIS LINE
> have an asm parser for this target

... so as far as I can see it's the presence of any inline assembly that
the tool cannot handle, as LLVM tells us.

So why isn't this a problem on other architectures? They're going to
have equivalent inline asm getting pulled in, and these samples
presumably compiled on at least on architecture in the past.

Does the Tool parse x86 asm without issue, say?

> One other solution would be to move these macros out to another file
> (like sysreg-macros.h or similar) and then continue to use a trick
> similar to what we are using not to exclude those macros.  It would need
> to be clear to ARCH maintainers that these macros would need to be there
> for the forseeable future.  

That's not going to fly. We can't factor out each individual piece of
asm without making the architecure ports significantly harder to
maintain.

As above, it's not clear to me why this issue doesn't affect other
architectures. It sounds like it may simply be a deficiency in the
version of LLVM being used, that could be solved by updating that.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC arm64] samples/bpf: explicitly exclude sysreg sections with asm macros
  2017-03-10 20:41     ` Mark Rutland
@ 2017-03-15 18:31       ` Andy Gospodarek
  2017-03-15 19:17         ` Mark Rutland
  0 siblings, 1 reply; 17+ messages in thread
From: Andy Gospodarek @ 2017-03-15 18:31 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Mar 10, 2017 at 08:41:13PM +0000, Mark Rutland wrote:
> Hi,
> 
> On Fri, Mar 10, 2017 at 02:26:56PM -0500, Andy Gospodarek wrote:
> > On Fri, Mar 10, 2017 at 05:52:30PM +0000, Will Deacon wrote:
> > > On Thu, Mar 09, 2017 at 06:18:12PM -0500, Andy Gospodarek wrote:
> > > > The previous fix to workaround compilation issues for samples/bpf on arm64 was
> > > > to prevent inclusion of code in sysregs.h.  This is not sustainable at this
> > > > point since other header files need access to sysregs.h for multiple
> > > > defintions.  The fact that the bpf samples cannot be compiled on arm64 is
> > > > fairly well documented on the iovisor-dev mailing and other places without a
> > > > clear-cut solution other than waiting for the llvm fix.  This attempts to
> > > > resolve that by dropping the fix to define _ASM_SYSREG_H on the clang command
> > > > line, and replace it with one that is wrapped around the offending code.  I
> > > > recognize this feels a bit fragile, but the current situation is not great
> > > > either and it seems that we are more likely to see users if the current code
> > > > actually compiles and runs.
> > > 
> > > Why does asm cause compilation to fail?
> > 
> > It isn't the ASM itself that causes the compilation to fail, it's the
> > ASM macros included inside the new ifdef that are problematic.  Here is
> > what is seen per object file:
> 
> > ./arch/arm64/include/asm/barrier.h:62:23: note: expanded from macro
> > '__smp_store_release'
> >                 asm volatile ("stlr %1, %0"
> > \
> >                                     ^
> > 1 warning generated.
> > LLVM ERROR: Inline asm not supported by this streamer because we don't  <----  THIS LINE
> > have an asm parser for this target
> 
> ... so as far as I can see it's the presence of any inline assembly that
> the tool cannot handle, as LLVM tells us.
> 
> So why isn't this a problem on other architectures? They're going to
> have equivalent inline asm getting pulled in, and these samples
> presumably compiled on at least on architecture in the past.
> 
> Does the Tool parse x86 asm without issue, say?

As far as I can tell (hoping my regex is proper), x86 doesn't have any
inline assembly that introduces macros like this in any h-files.

> 
> > One other solution would be to move these macros out to another file
> > (like sysreg-macros.h or similar) and then continue to use a trick
> > similar to what we are using not to exclude those macros.  It would need
> > to be clear to ARCH maintainers that these macros would need to be there
> > for the forseeable future.  
> 
> That's not going to fly. We can't factor out each individual piece of
> asm without making the architecure ports significantly harder to
> maintain.

I'm not explicitly asking for that as I agree filtering out all asm
would be unreasonable.  It seems to me that the specific
not-in-a-function inline asm (that in this case creates these two new
macros) is what is in question.

What's most interesting is that that originally sysreg.h contained on
the mrs_s and msr_s macros with commit 72c58395.  Is the right answer
actually to move more of the code that has crept into sysreg.h out to
some other file since it clearly isn't just macros anymore?

> As above, it's not clear to me why this issue doesn't affect other
> architectures. It sounds like it may simply be a deficiency in the
> version of LLVM being used, that could be solved by updating that.

I build llvm and friends from source and tested it before submitting
this patch (I needed to do it based on the older versions included in
the userspace on my distro), so I knew that a simple upgrade to the
latest wasn't going to do it.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC arm64] samples/bpf: explicitly exclude sysreg sections with asm macros
  2017-03-15 18:31       ` Andy Gospodarek
@ 2017-03-15 19:17         ` Mark Rutland
  2017-03-15 20:54           ` Andy Gospodarek
  0 siblings, 1 reply; 17+ messages in thread
From: Mark Rutland @ 2017-03-15 19:17 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

On Wed, Mar 15, 2017 at 02:31:30PM -0400, Andy Gospodarek wrote:
> On Fri, Mar 10, 2017 at 08:41:13PM +0000, Mark Rutland wrote:
> > On Fri, Mar 10, 2017 at 02:26:56PM -0500, Andy Gospodarek wrote:
> > > On Fri, Mar 10, 2017 at 05:52:30PM +0000, Will Deacon wrote:
> > > > On Thu, Mar 09, 2017 at 06:18:12PM -0500, Andy Gospodarek wrote:

> > > > Why does asm cause compilation to fail?
> > > 
> > > It isn't the ASM itself that causes the compilation to fail, it's the
> > > ASM macros included inside the new ifdef that are problematic.  Here is
> > > what is seen per object file:
> > 
> > > ./arch/arm64/include/asm/barrier.h:62:23: note: expanded from macro
> > > '__smp_store_release'
> > >                 asm volatile ("stlr %1, %0"
> > > \
> > >                                     ^
> > > 1 warning generated.
> > > LLVM ERROR: Inline asm not supported by this streamer because we don't  <----  THIS LINE
> > > have an asm parser for this target
> > 
> > ... so as far as I can see it's the presence of any inline assembly that
> > the tool cannot handle, as LLVM tells us.

> > So why isn't this a problem on other architectures? They're going to
> > have equivalent inline asm getting pulled in, and these samples
> > presumably compiled on at least on architecture in the past.

> As far as I can tell (hoping my regex is proper), x86 doesn't have any
> inline assembly that introduces macros like this in any h-files.

Just to be clear, is the error quoted above at all relevant here?

The error is for an asm statement which does not use assembly macros, in
a different file.

In the error message, LLVM explicitly states that it does not have a
parser for this target's assembly -- which would be a problem for *any*
inline assembly.

How does that specific error relate to the use of assembly macros within
sysreg.h?

... has the wrong error been quoted above?

... does LLVM dump a misleading error message after seeing an assembly
macro (which it presumably can't handle)?

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC arm64] samples/bpf: explicitly exclude sysreg sections with asm macros
  2017-03-15 19:17         ` Mark Rutland
@ 2017-03-15 20:54           ` Andy Gospodarek
  2017-03-16 10:42             ` Mark Rutland
  0 siblings, 1 reply; 17+ messages in thread
From: Andy Gospodarek @ 2017-03-15 20:54 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Mar 15, 2017 at 07:17:41PM +0000, Mark Rutland wrote:
> Hi,
> 
> On Wed, Mar 15, 2017 at 02:31:30PM -0400, Andy Gospodarek wrote:
> > On Fri, Mar 10, 2017 at 08:41:13PM +0000, Mark Rutland wrote:
> > > On Fri, Mar 10, 2017 at 02:26:56PM -0500, Andy Gospodarek wrote:
> > > > On Fri, Mar 10, 2017 at 05:52:30PM +0000, Will Deacon wrote:
> > > > > On Thu, Mar 09, 2017 at 06:18:12PM -0500, Andy Gospodarek wrote:
> 
> > > > > Why does asm cause compilation to fail?
> > > > 
> > > > It isn't the ASM itself that causes the compilation to fail, it's the
> > > > ASM macros included inside the new ifdef that are problematic.  Here is
> > > > what is seen per object file:
> > > 
> > > > ./arch/arm64/include/asm/barrier.h:62:23: note: expanded from macro
> > > > '__smp_store_release'
> > > >                 asm volatile ("stlr %1, %0"
> > > > \
> > > >                                     ^
> > > > 1 warning generated.
> > > > LLVM ERROR: Inline asm not supported by this streamer because we don't  <----  THIS LINE
> > > > have an asm parser for this target
> > > 
> > > ... so as far as I can see it's the presence of any inline assembly that
> > > the tool cannot handle, as LLVM tells us.
> 
> > > So why isn't this a problem on other architectures? They're going to
> > > have equivalent inline asm getting pulled in, and these samples
> > > presumably compiled on at least on architecture in the past.
> 
> > As far as I can tell (hoping my regex is proper), x86 doesn't have any
> > inline assembly that introduces macros like this in any h-files.
> 
> Just to be clear, is the error quoted above at all relevant here?
> 
> The error is for an asm statement which does not use assembly macros, in
> a different file.
> 
> In the error message, LLVM explicitly states that it does not have a
> parser for this target's assembly -- which would be a problem for *any*
> inline assembly.
> 
> How does that specific error relate to the use of assembly macros within
> sysreg.h?

This is the only error that is relevant:

LLVM ERROR: Inline asm not supported by this streamer because we don't have an asm parser for this target

Upon inspection it does not appear that this is explicitly related to
assembly macros, just simple inline assembly.  I recall reading a thread
on either the llvm or iovisor mailing-list that pointed at the macro
being the issue, but even if I remove the macro the compile fails until
I put the assembly into a function.

> ... has the wrong error been quoted above?

The other lines related to errors in barrier.h can be ignored this is
not a new issue or one that is a major concern to me.  As I said before
I probably mistakenly added to much detail to the commit message as it
clearly caused confusion.

> ... does LLVM dump a misleading error message after seeing an assembly
> macro (which it presumably can't handle)?

It does seem a bit misleading -- or at a minimum it seems vague.

Is this the only inline assembly outside a function that is not inside
#ifdef __ASSEMBLY__ in the arm64 tree?

Thanks for keeping the thread alive, Mark. (Seriously)

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC arm64] samples/bpf: explicitly exclude sysreg sections with asm macros
  2017-03-15 20:54           ` Andy Gospodarek
@ 2017-03-16 10:42             ` Mark Rutland
  2017-03-16 21:04               ` Andy Gospodarek
  0 siblings, 1 reply; 17+ messages in thread
From: Mark Rutland @ 2017-03-16 10:42 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Mar 15, 2017 at 04:54:04PM -0400, Andy Gospodarek wrote:
> On Wed, Mar 15, 2017 at 07:17:41PM +0000, Mark Rutland wrote:
> > 
> > On Wed, Mar 15, 2017 at 02:31:30PM -0400, Andy Gospodarek wrote:
> > > On Fri, Mar 10, 2017 at 08:41:13PM +0000, Mark Rutland wrote:
> > > > On Fri, Mar 10, 2017 at 02:26:56PM -0500, Andy Gospodarek wrote:
> > > > > On Fri, Mar 10, 2017 at 05:52:30PM +0000, Will Deacon wrote:
 
> > > > > It isn't the ASM itself that causes the compilation to fail, it's the
> > > > > ASM macros included inside the new ifdef that are problematic.  Here is
> > > > > what is seen per object file:
> > > > 
> > > > > ./arch/arm64/include/asm/barrier.h:62:23: note: expanded from macro
> > > > > '__smp_store_release'
> > > > >                 asm volatile ("stlr %1, %0"
> > > > > \
> > > > >                                     ^
> > > > > 1 warning generated.
> > > > > LLVM ERROR: Inline asm not supported by this streamer because we don't  <----  THIS LINE
> > > > > have an asm parser for this target
> > > > 
> > > > ... so as far as I can see it's the presence of any inline assembly that
> > > > the tool cannot handle, as LLVM tells us.

> > In the error message, LLVM explicitly states that it does not have a
> > parser for this target's assembly -- which would be a problem for *any*
> > inline assembly.
> > 
> > How does that specific error relate to the use of assembly macros within
> > sysreg.h?
> 
> This is the only error that is relevant:
> 
> LLVM ERROR: Inline asm not supported by this streamer because we don't have an asm parser for this target
> 
> Upon inspection it does not appear that this is explicitly related to
> assembly macros, just simple inline assembly. 

Indeed; as Will and I stated previously.

> Is this the only inline assembly outside a function that is not inside
> #ifdef __ASSEMBLY__ in the arm64 tree?

An __ASSEMBLY__ guard is irrelevant. We need the compiler to handle the
asm() statement, not the assembler (which cannot even parse it).

The kernel uses asm statements outside of functions elsewhere. For
example, take a look in <linux/export.h>. See that EXPORT_SYMBOL() uses
__CRC_SYMBOL(), which uses asm statements. The EXPORT_SYMBOL() is used
outside of functions, on all architectures, arm64 and x86 included.

I don't see why being inside a function matters. Given the error
message, the toolchain presumably can't parse inline assembly in any
context. We have static inline functions with assembly in headers, which
are presumably affected.

It sounds like the toolchain you are using is lacking in functionality
that is presumably present when targeting x86, or the error messages are
simply misleading. In either case, there's a toolchain issue to be
addressed, and not a kernel issue.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC arm64] samples/bpf: explicitly exclude sysreg sections with asm macros
  2017-03-16 10:42             ` Mark Rutland
@ 2017-03-16 21:04               ` Andy Gospodarek
  2017-03-17 11:11                 ` Robin Murphy
  2017-03-17 17:17                 ` Mark Rutland
  0 siblings, 2 replies; 17+ messages in thread
From: Andy Gospodarek @ 2017-03-16 21:04 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Mar 16, 2017 at 10:42:04AM +0000, Mark Rutland wrote:
> On Wed, Mar 15, 2017 at 04:54:04PM -0400, Andy Gospodarek wrote:
> > On Wed, Mar 15, 2017 at 07:17:41PM +0000, Mark Rutland wrote:
> > > 
> > > On Wed, Mar 15, 2017 at 02:31:30PM -0400, Andy Gospodarek wrote:
> > > > On Fri, Mar 10, 2017 at 08:41:13PM +0000, Mark Rutland wrote:
> > > > > On Fri, Mar 10, 2017 at 02:26:56PM -0500, Andy Gospodarek wrote:
> > > > > > On Fri, Mar 10, 2017 at 05:52:30PM +0000, Will Deacon wrote:
>  
> > > > > > It isn't the ASM itself that causes the compilation to fail, it's the
> > > > > > ASM macros included inside the new ifdef that are problematic.  Here is
> > > > > > what is seen per object file:
> > > > > 
> > > > > > ./arch/arm64/include/asm/barrier.h:62:23: note: expanded from macro
> > > > > > '__smp_store_release'
> > > > > >                 asm volatile ("stlr %1, %0"
> > > > > > \
> > > > > >                                     ^
> > > > > > 1 warning generated.
> > > > > > LLVM ERROR: Inline asm not supported by this streamer because we don't  <----  THIS LINE
> > > > > > have an asm parser for this target
> > > > > 
> > > > > ... so as far as I can see it's the presence of any inline assembly that
> > > > > the tool cannot handle, as LLVM tells us.
> 
> > > In the error message, LLVM explicitly states that it does not have a
> > > parser for this target's assembly -- which would be a problem for *any*
> > > inline assembly.
> > > 
> > > How does that specific error relate to the use of assembly macros within
> > > sysreg.h?
> > 
> > This is the only error that is relevant:
> > 
> > LLVM ERROR: Inline asm not supported by this streamer because we don't have an asm parser for this target
> > 
> > Upon inspection it does not appear that this is explicitly related to
> > assembly macros, just simple inline assembly. 
> 
> Indeed; as Will and I stated previously.
> 
> > Is this the only inline assembly outside a function that is not inside
> > #ifdef __ASSEMBLY__ in the arm64 tree?
> 
> An __ASSEMBLY__ guard is irrelevant. We need the compiler to handle the
> asm() statement, not the assembler (which cannot even parse it).
> 
> The kernel uses asm statements outside of functions elsewhere. For
> example, take a look in <linux/export.h>. See that EXPORT_SYMBOL() uses
> __CRC_SYMBOL(), which uses asm statements. The EXPORT_SYMBOL() is used
> outside of functions, on all architectures, arm64 and x86 included.
> 
> I don't see why being inside a function matters. Given the error
> message, the toolchain presumably can't parse inline assembly in any
> context. We have static inline functions with assembly in headers, which
> are presumably affected.

You are correct that it appears llvm cannot process any inline asm for
arm64.  Sadly (or luckily) the inability to process this is not a
hard-requirement for llvm to properly compile code that includes these
h-files -- as long as none of #defines/c-macros/inline functions are
actually *called* in the code I'm trying to compile.

That is the tricky part about this entire problem and why excluding the
inline assembly allows me to compile properly while leaving all the
other assembly inside macros or inline functions is just fine.

Sorry I continue to harp on this, but I've been testing XDP on ARM64
using some Broadcom SoCs and I'm seeing some really nice performance
benefits to being able to process so many frames per second using a
smaller footprint device.  I'd like to see the ARM64 XDP user community
grow, but without the ability to easily compile the samples in the
upstream kernel tree today I just don't see this user- or developer-base
growing.  

> It sounds like the toolchain you are using is lacking in functionality
> that is presumably present when targeting x86, or the error messages are
> simply misleading. In either case, there's a toolchain issue to be
> addressed, and not a kernel issue.

While I generally agree isn't the entire reason this inline asm exists
-- to fix a binutils issue?  :-)

commit 72c5839515260dce966cd24f54436e6583288e6c
Author: Catalin Marinas <catalin.marinas@arm.com>
Date:   Thu Jul 24 14:14:42 2014 +0100

    arm64: gicv3: Allow GICv3 compilation with older binutils
   
    GICv3 introduces new system registers accessible with the full msr/mrs
    syntax (e.g. mrs x0, Sop0_op1_CRm_CRn_op2). However, only recent
    binutils understand the new syntax. This patch introduces msr_s/mrs_s
    assembly macros which generate the equivalent instructions above and
    converts the existing GICv3 code (both drivers/irqchip/ and
    arch/arm64/kernel/).

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC arm64] samples/bpf: explicitly exclude sysreg sections with asm macros
  2017-03-16 21:04               ` Andy Gospodarek
@ 2017-03-17 11:11                 ` Robin Murphy
  2017-03-17 13:58                   ` Andy Gospodarek
  2017-03-17 17:17                 ` Mark Rutland
  1 sibling, 1 reply; 17+ messages in thread
From: Robin Murphy @ 2017-03-17 11:11 UTC (permalink / raw)
  To: linux-arm-kernel

On 16/03/17 21:04, Andy Gospodarek wrote:
> On Thu, Mar 16, 2017 at 10:42:04AM +0000, Mark Rutland wrote:
>> On Wed, Mar 15, 2017 at 04:54:04PM -0400, Andy Gospodarek wrote:
>>> On Wed, Mar 15, 2017 at 07:17:41PM +0000, Mark Rutland wrote:
>>>>
>>>> On Wed, Mar 15, 2017 at 02:31:30PM -0400, Andy Gospodarek wrote:
>>>>> On Fri, Mar 10, 2017 at 08:41:13PM +0000, Mark Rutland wrote:
>>>>>> On Fri, Mar 10, 2017 at 02:26:56PM -0500, Andy Gospodarek wrote:
>>>>>>> On Fri, Mar 10, 2017 at 05:52:30PM +0000, Will Deacon wrote:
>>  
>>>>>>> It isn't the ASM itself that causes the compilation to fail, it's the
>>>>>>> ASM macros included inside the new ifdef that are problematic.  Here is
>>>>>>> what is seen per object file:
>>>>>>
>>>>>>> ./arch/arm64/include/asm/barrier.h:62:23: note: expanded from macro
>>>>>>> '__smp_store_release'
>>>>>>>                 asm volatile ("stlr %1, %0"
>>>>>>> \
>>>>>>>                                     ^
>>>>>>> 1 warning generated.
>>>>>>> LLVM ERROR: Inline asm not supported by this streamer because we don't  <----  THIS LINE
>>>>>>> have an asm parser for this target
>>>>>>
>>>>>> ... so as far as I can see it's the presence of any inline assembly that
>>>>>> the tool cannot handle, as LLVM tells us.
>>
>>>> In the error message, LLVM explicitly states that it does not have a
>>>> parser for this target's assembly -- which would be a problem for *any*
>>>> inline assembly.
>>>>
>>>> How does that specific error relate to the use of assembly macros within
>>>> sysreg.h?
>>>
>>> This is the only error that is relevant:
>>>
>>> LLVM ERROR: Inline asm not supported by this streamer because we don't have an asm parser for this target
>>>
>>> Upon inspection it does not appear that this is explicitly related to
>>> assembly macros, just simple inline assembly. 
>>
>> Indeed; as Will and I stated previously.
>>
>>> Is this the only inline assembly outside a function that is not inside
>>> #ifdef __ASSEMBLY__ in the arm64 tree?
>>
>> An __ASSEMBLY__ guard is irrelevant. We need the compiler to handle the
>> asm() statement, not the assembler (which cannot even parse it).
>>
>> The kernel uses asm statements outside of functions elsewhere. For
>> example, take a look in <linux/export.h>. See that EXPORT_SYMBOL() uses
>> __CRC_SYMBOL(), which uses asm statements. The EXPORT_SYMBOL() is used
>> outside of functions, on all architectures, arm64 and x86 included.
>>
>> I don't see why being inside a function matters. Given the error
>> message, the toolchain presumably can't parse inline assembly in any
>> context. We have static inline functions with assembly in headers, which
>> are presumably affected.
> 
> You are correct that it appears llvm cannot process any inline asm for
> arm64.  Sadly (or luckily) the inability to process this is not a
> hard-requirement for llvm to properly compile code that includes these
> h-files -- as long as none of #defines/c-macros/inline functions are
> actually *called* in the code I'm trying to compile.
> 
> That is the tricky part about this entire problem and why excluding the
> inline assembly allows me to compile properly while leaving all the
> other assembly inside macros or inline functions is just fine.
> 
> Sorry I continue to harp on this, but I've been testing XDP on ARM64
> using some Broadcom SoCs and I'm seeing some really nice performance
> benefits to being able to process so many frames per second using a
> smaller footprint device.  I'd like to see the ARM64 XDP user community
> grow, but without the ability to easily compile the samples in the
> upstream kernel tree today I just don't see this user- or developer-base
> growing.  
> 
>> It sounds like the toolchain you are using is lacking in functionality
>> that is presumably present when targeting x86, or the error messages are
>> simply misleading. In either case, there's a toolchain issue to be
>> addressed, and not a kernel issue.
> 
> While I generally agree isn't the entire reason this inline asm exists
> -- to fix a binutils issue?  :-)

Binutils is required to build the arm64 kernel; if certain versions of
binutils packaged by arm64 distros can't cope with certain instructions
we need to use, we have to do what we can to make it build. However, if
LLVM cannot cope with inline assembly on arm64, then it's pretty clear
that the kernel cannot be built with LLVM anyway, thus there is nothing
to work around in that regard. Sorry, you've got a straw man there ;)

Robin.

> commit 72c5839515260dce966cd24f54436e6583288e6c
> Author: Catalin Marinas <catalin.marinas@arm.com>
> Date:   Thu Jul 24 14:14:42 2014 +0100
> 
>     arm64: gicv3: Allow GICv3 compilation with older binutils
>    
>     GICv3 introduces new system registers accessible with the full msr/mrs
>     syntax (e.g. mrs x0, Sop0_op1_CRm_CRn_op2). However, only recent
>     binutils understand the new syntax. This patch introduces msr_s/mrs_s
>     assembly macros which generate the equivalent instructions above and
>     converts the existing GICv3 code (both drivers/irqchip/ and
>     arch/arm64/kernel/).
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC arm64] samples/bpf: explicitly exclude sysreg sections with asm macros
  2017-03-17 11:11                 ` Robin Murphy
@ 2017-03-17 13:58                   ` Andy Gospodarek
  2017-03-17 16:57                     ` Robin Murphy
  0 siblings, 1 reply; 17+ messages in thread
From: Andy Gospodarek @ 2017-03-17 13:58 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Mar 17, 2017 at 11:11:59AM +0000, Robin Murphy wrote:
> On 16/03/17 21:04, Andy Gospodarek wrote:
> > 
> > While I generally agree isn't the entire reason this inline asm exists
> > -- to fix a binutils issue?  :-)
> 
> Binutils is required to build the arm64 kernel; if certain versions of
> binutils packaged by arm64 distros can't cope with certain instructions
> we need to use, we have to do what we can to make it build. However, if
> LLVM cannot cope with inline assembly on arm64, then it's pretty clear
> that the kernel cannot be built with LLVM anyway, thus there is nothing
> to work around in that regard. Sorry, you've got a straw man there ;)

I know, right?  I'll see if I can come up with something that appears to
be less fragile and/or intrusive to the arm64 arch code as I'd really
like to stop carrying a patch in my devel tree for this.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC arm64] samples/bpf: explicitly exclude sysreg sections with asm macros
  2017-03-17 13:58                   ` Andy Gospodarek
@ 2017-03-17 16:57                     ` Robin Murphy
  2017-03-17 17:04                       ` Mark Rutland
  2017-03-17 17:43                       ` Andy Gospodarek
  0 siblings, 2 replies; 17+ messages in thread
From: Robin Murphy @ 2017-03-17 16:57 UTC (permalink / raw)
  To: linux-arm-kernel

On 17/03/17 13:58, Andy Gospodarek wrote:
> On Fri, Mar 17, 2017 at 11:11:59AM +0000, Robin Murphy wrote:
>> On 16/03/17 21:04, Andy Gospodarek wrote:
>>>
>>> While I generally agree isn't the entire reason this inline asm exists
>>> -- to fix a binutils issue?  :-)
>>
>> Binutils is required to build the arm64 kernel; if certain versions of
>> binutils packaged by arm64 distros can't cope with certain instructions
>> we need to use, we have to do what we can to make it build. However, if
>> LLVM cannot cope with inline assembly on arm64, then it's pretty clear
>> that the kernel cannot be built with LLVM anyway, thus there is nothing
>> to work around in that regard. Sorry, you've got a straw man there ;)
> 
> I know, right?  I'll see if I can come up with something that appears to
> be less fragile and/or intrusive to the arm64 arch code as I'd really
> like to stop carrying a patch in my devel tree for this.

I'm honestly not sure if I'm serious or not, but:

diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 72c58675973e..f52ebad7b096 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -154,6 +154,6 @@ $(src)/*.c: verify_target_bpf
 # useless for BPF samples.
 $(obj)/%.o: $(src)/%.c
        $(CLANG) $(NOSTDINC_FLAGS) $(LINUXINCLUDE) $(EXTRA_CFLAGS) \
-               -D__KERNEL__ -D__ASM_SYSREG_H -Wno-unused-value
-Wno-pointer-sign \
+               -D__KERNEL__ -D'asm(...)' -Wno-unused-value
-Wno-pointer-sign \
                -Wno-compare-distinct-pointer-types \
                -O2 -emit-llvm -c $< -o -| $(LLC) -march=bpf
-filetype=obj -o $@


?

Robin.

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [RFC arm64] samples/bpf: explicitly exclude sysreg sections with asm macros
  2017-03-17 16:57                     ` Robin Murphy
@ 2017-03-17 17:04                       ` Mark Rutland
  2017-03-17 17:43                       ` Andy Gospodarek
  1 sibling, 0 replies; 17+ messages in thread
From: Mark Rutland @ 2017-03-17 17:04 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Mar 17, 2017 at 04:57:20PM +0000, Robin Murphy wrote:
> I'm honestly not sure if I'm serious or not, but:

/me shakes the magic 8 ball...

> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index 72c58675973e..f52ebad7b096 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -154,6 +154,6 @@ $(src)/*.c: verify_target_bpf
>  # useless for BPF samples.
>  $(obj)/%.o: $(src)/%.c
>         $(CLANG) $(NOSTDINC_FLAGS) $(LINUXINCLUDE) $(EXTRA_CFLAGS) \
> -               -D__KERNEL__ -D__ASM_SYSREG_H -Wno-unused-value
> -Wno-pointer-sign \
> +               -D__KERNEL__ -D'asm(...)' -Wno-unused-value
> -Wno-pointer-sign \
>                 -Wno-compare-distinct-pointer-types \
>                 -O2 -emit-llvm -c $< -o -| $(LLC) -march=bpf
> -filetype=obj -o $@

... "Concentrate and ask again".

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC arm64] samples/bpf: explicitly exclude sysreg sections with asm macros
  2017-03-16 21:04               ` Andy Gospodarek
  2017-03-17 11:11                 ` Robin Murphy
@ 2017-03-17 17:17                 ` Mark Rutland
  1 sibling, 0 replies; 17+ messages in thread
From: Mark Rutland @ 2017-03-17 17:17 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Mar 16, 2017 at 05:04:55PM -0400, Andy Gospodarek wrote:
> On Thu, Mar 16, 2017 at 10:42:04AM +0000, Mark Rutland wrote:
> > On Wed, Mar 15, 2017 at 04:54:04PM -0400, Andy Gospodarek wrote:
> > > On Wed, Mar 15, 2017 at 07:17:41PM +0000, Mark Rutland wrote:
> > > > On Wed, Mar 15, 2017 at 02:31:30PM -0400, Andy Gospodarek wrote:
> > > > > On Fri, Mar 10, 2017 at 08:41:13PM +0000, Mark Rutland wrote:

> > > > > > ... so as far as I can see it's the presence of any inline assembly that
> > > > > > the tool cannot handle, as LLVM tells us.

> > > LLVM ERROR: Inline asm not supported by this streamer because we
> > > don't have an asm parser for this target
> > > 
> > > Upon inspection it does not appear that this is explicitly related to
> > > assembly macros, just simple inline assembly. 

> You are correct that it appears llvm cannot process any inline asm for
> arm64.  Sadly (or luckily) the inability to process this is not a
> hard-requirement for llvm to properly compile code that includes these
> h-files -- as long as none of #defines/c-macros/inline functions are
> actually *called* in the code I'm trying to compile.

The Linux-internal arm64 headers are there to build the kernel using a
supported toolchain. From my PoV, if you want to use the Linux-internal
arm64 headers, you need to be compatible with the toolchain(s) that we
target. 

I would imagine the x86 developers would have a similar feeling, were
this issue to crop up on x86.

> Sorry I continue to harp on this, but I've been testing XDP on ARM64
> using some Broadcom SoCs and I'm seeing some really nice performance
> benefits to being able to process so many frames per second using a
> smaller footprint device.  I'd like to see the ARM64 XDP user community
> grow, but without the ability to easily compile the samples in the
> upstream kernel tree today I just don't see this user- or developer-base
> growing.  

Please talk to the LLVM community, and see what is necessary in order to
make it compatible with the toolchains we support.

> > It sounds like the toolchain you are using is lacking in functionality
> > that is presumably present when targeting x86, or the error messages are
> > simply misleading. In either case, there's a toolchain issue to be
> > addressed, and not a kernel issue.
> 
> While I generally agree isn't the entire reason this inline asm exists
> -- to fix a binutils issue?  :-)
> 
> commit 72c5839515260dce966cd24f54436e6583288e6c
> Author: Catalin Marinas <catalin.marinas@arm.com>
> Date:   Thu Jul 24 14:14:42 2014 +0100
> 
>     arm64: gicv3: Allow GICv3 compilation with older binutils

We introduced this code so as to *retain* compatibility with a toolchain
that we explicitly supported, but accidentally broke with the addition
of the GICv3 code.

We have clearly never supoprted a toolchain that cannot parse inline
asembly.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC arm64] samples/bpf: explicitly exclude sysreg sections with asm macros
  2017-03-17 16:57                     ` Robin Murphy
  2017-03-17 17:04                       ` Mark Rutland
@ 2017-03-17 17:43                       ` Andy Gospodarek
  1 sibling, 0 replies; 17+ messages in thread
From: Andy Gospodarek @ 2017-03-17 17:43 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Mar 17, 2017 at 04:57:20PM +0000, Robin Murphy wrote:
> On 17/03/17 13:58, Andy Gospodarek wrote:
> > On Fri, Mar 17, 2017 at 11:11:59AM +0000, Robin Murphy wrote:
> >> On 16/03/17 21:04, Andy Gospodarek wrote:
> >>>
> >>> While I generally agree isn't the entire reason this inline asm exists
> >>> -- to fix a binutils issue?  :-)
> >>
> >> Binutils is required to build the arm64 kernel; if certain versions of
> >> binutils packaged by arm64 distros can't cope with certain instructions
> >> we need to use, we have to do what we can to make it build. However, if
> >> LLVM cannot cope with inline assembly on arm64, then it's pretty clear
> >> that the kernel cannot be built with LLVM anyway, thus there is nothing
> >> to work around in that regard. Sorry, you've got a straw man there ;)
> > 
> > I know, right?  I'll see if I can come up with something that appears to
> > be less fragile and/or intrusive to the arm64 arch code as I'd really
> > like to stop carrying a patch in my devel tree for this.
> 
> I'm honestly not sure if I'm serious or not, but:
> 
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index 72c58675973e..f52ebad7b096 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -154,6 +154,6 @@ $(src)/*.c: verify_target_bpf
>  # useless for BPF samples.
>  $(obj)/%.o: $(src)/%.c
>         $(CLANG) $(NOSTDINC_FLAGS) $(LINUXINCLUDE) $(EXTRA_CFLAGS) \
> -               -D__KERNEL__ -D__ASM_SYSREG_H -Wno-unused-value
> -Wno-pointer-sign \
> +               -D__KERNEL__ -D'asm(...)' -Wno-unused-value
> -Wno-pointer-sign \
>                 -Wno-compare-distinct-pointer-types \
>                 -O2 -emit-llvm -c $< -o -| $(LLC) -march=bpf
> -filetype=obj -o $@
> 

Hahahaha, I know what you mean.

Scarily, I had a similar thought.  :-) 

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2017-03-17 17:43 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-09 23:18 [RFC arm64] samples/bpf: explicitly exclude sysreg sections with asm macros Andy Gospodarek
2017-03-10 17:52 ` Will Deacon
2017-03-10 19:26   ` Andy Gospodarek
2017-03-10 20:41     ` Mark Rutland
2017-03-15 18:31       ` Andy Gospodarek
2017-03-15 19:17         ` Mark Rutland
2017-03-15 20:54           ` Andy Gospodarek
2017-03-16 10:42             ` Mark Rutland
2017-03-16 21:04               ` Andy Gospodarek
2017-03-17 11:11                 ` Robin Murphy
2017-03-17 13:58                   ` Andy Gospodarek
2017-03-17 16:57                     ` Robin Murphy
2017-03-17 17:04                       ` Mark Rutland
2017-03-17 17:43                       ` Andy Gospodarek
2017-03-17 17:17                 ` Mark Rutland
2017-03-10 18:13 ` Mark Rutland
2017-03-10 19:35   ` Andy Gospodarek

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.