BCC cross compilation support #1569
Conversation
joelagnel
requested review from
4ast,
drzaeus77 and
yonghong-song
as code owners
|
[buildbot, ok to test] |
| const char *archenv = getenv("ARCH"); | ||
| if (!archenv) | ||
| archenv = getenv("BCC_ARCH"); | ||
|
|
There was a problem hiding this comment.
what is the difference between ARCH and BCC_ARCH? Should we use an environment variable or a cmake macro definition?
There was a problem hiding this comment.
CMake macro definition would make it static then right? We want it to be dynamic. This is indeed how the kernel is built in a cross compiler environment as well. One does something like this in kernel build directory, for example:
ARCH=arm64 make zImage
Similarly, we need BCC to adapt to the user's cross compile environment like the kernel does.
There was a problem hiding this comment.
okay, env seems fine here. What is the different between ARCH and BCC_ARCH?
There was a problem hiding this comment.
I just thought of giving BCC_ARCH as an additional option since most (all?) env vars or macros are prefixed with BCC_. There's no difference between them.
There was a problem hiding this comment.
Can we just one, e.g., BCC_TARGET_ARCH? Having two is not really necessary here, in my opinion.
There was a problem hiding this comment.
Yes I agree we can have one. Hope you agree that lets just keep ARCH because this what the kernel uses as well so it will simplify the kernel developer's workflow here.
| archenv = getenv("BCC_ARCH"); | ||
|
|
||
| if (archenv) { | ||
| if (getenv("BCC_KBUILD_DEBUG")) |
There was a problem hiding this comment.
Can we use one of existing debug flags for output here? For example, debug=4 for preprocessor result can be used since this impacts the clang commands?
There was a problem hiding this comment.
I'll look into that, thanks
There was a problem hiding this comment.
Sure, I'll reuse DEBUG_PREPROCESSOR to print the chosen ARCH. Can I also reuse DEBUG_PREPROCESSOR to print the kernel source directory path or does another flag make more sense?
There was a problem hiding this comment.
You can just reuse DEBUG_PREPROCESSOR to print the kernel source directoryy path.
There was a problem hiding this comment.
Could you think of a way to put the code dealing different architectures in one place (header is okay), so the main.c file will be clean?
There was a problem hiding this comment.
@yonghong-song didn't follow your comment. When you say main.c, do you mean the /virtual/main.c file?
Also the code dealing with different architectures is already in a separate header (helpers.h). Do you mean the code shared between loader.cc and b_frontend_action.cc should be made common? I think we can do that using function pointers and enums. So we could define one enum per arch, and pass a function pointer callback to generic code that handles different arches, which would call then call the callback after detecting the ARCH. That would keep the "arch detection logic" separate from the code that "uses the arch and does different things".
There was a problem hiding this comment.
Note that I would keep helpers.h as is in the original patch though since that code isn't shared by the CC code (unless I missed something).
There was a problem hiding this comment.
Sorry for confusion. yes, I do mean the code shared between loader.cc and b_frontend_action.cc should be made common.
| string kdir = string(KERNEL_MODULES_DIR) + "/" + un.release; | ||
| auto kernel_path_info = get_kernel_path_info (kdir); | ||
| string kdir, kpath; | ||
| const char *kpath_env = ::getenv("BCC_KERNEL_SOURCE"); |
There was a problem hiding this comment.
Again about env variable vs. cmake macro? I would prefer cmake macro to make the code less dependent on external factors. If use cmake macro, maybe KERNEL_SOURCE_DIR as the macro name?
There was a problem hiding this comment.
Same reason as above, BCC should adapt to different kernel sources that the same BCC installation can point to dynamically. We shouldn't restrict this to a statically known path.
There was a problem hiding this comment.
Okay with me for this.
joelagnel
force-pushed
the
bcc-cross-compile
branch
from
579c9b9
to
befd346
Compare
joelagnel
force-pushed
the
bcc-cross-compile
branch
2 times, most recently
from
96f5230
to
0efbe01
Compare
Many developers have kernel sources at an specific location. Allow clang to build from there by allowing to specify a single absolute path to the kernel sources through a new env var BCC_KERNEL_SOURCE. Signed-off-by: Joel Fernandes <[email protected]>
BCC at the moment builds eBPF only considering the local architecture instead of the one that the user's target kernel is running on. For cross-compiler environments, the ARCH environment variable is used to specify which ARCH to build the kernel for. In this patch we add support to read ARCH and if that's not set, then fallback to detecting based on local architecture. This patch borrows some code from a patch doing a similar thing for eBPF samples in the kenrel that I submitted recently [1] [1] https://patchwork.kernel.org/patch/9961801/ Signed-off-by: Joel Fernandes <[email protected]>
joelagnel
force-pushed
the
bcc-cross-compile
branch
from
0efbe01
to
1086952
Compare
|
Simplified the patch, its much cleaner. Let me know comments if any. |
|
Yes, I agree. thanks |
These patches make it possible to build eBPF programs from a custom kernel source directory and a custom user-specified ARCH. This support is one of the pieces for the effort to make BCC work across systems using BPFd.