Makefile tweaks for handling non-replace kpatch building

[joe-lawrence]

These two commits fix and extend the ability to build non atomic replace kpatches:

1. Fixes the --no-replace kpatch-build flag so that the kpatch module build honors the setting.

2. Requested by Red Hat QE, who reuse the integration test .ko modules for their own tests -- they were not prepared for atomic-replace style patches when loading multiple kpatches. Provide a general facility to set the integration test kpatch-build options through the makefile.

[joe-lawrence]

@kamalesh-babulal : in my mind, kernel kbuild is a giant ball of spaghetti. Do we know if the -mcmodel=large flag below ever worked? Or maybe recent churn in kbuild stopped paying attention to the KBUILD_CFLAGS_MODULE environment variable?

[kamalesh-babulal]

@joe-lawrence I did a quick test, of building a livepatching module using meminfo.patch with and without the KBUILD_CFLAGS_MODULE being passed via this Makefile. The only difference I see that the -mcmodel=large flag is being passed twice with KBUILD_CFLAGS_MODULE set. gcc -Wp,-MMD,/root/.kpatch/tmp/patch/.livepatch-meminfo.mod.o.d ............ -mcmodel=medium .... -I/root/kpatch/kmod/patch -mcmodel=large -fplugin=/root/kpatch/kpatch-build/gcc-plugins/ppc64le-plugin.so ... -DMODULE -mno-save-toc-indirect -mcmodel=large -mcmodel=large -DKBUILD_BASENAME='"livepatch_meminfo.mod"' -DKBUILD_MODNAME='"livepatch_meminfo"' -D__KBUILD_MODNAME=kmod_livepatch_meminfo -c -o /root/.kpatch/tmp/patch/livepatch-meminfo.mod.o /root/.kpatch/tmp/patch/livepatch-meminfo.mod.c.

I loaded the module built without the KBUILD_CFLAGS_MODULE += -mcmodel=large flag and seems to okay. I guess, we can remove the arch specific flag from the Makefile.

[joe-lawrence]

@kamalesh-babulal : thanks for running those tests. Were they on top of this PR or against the master branch?
Then if KBUILD_CFLAGS_MODULE += -mcmodel=large is not needed in this Makefile, I can tack on another commit to remote it.

[kamalesh-babulal]

@joe-lawrence It was run against master. I re-ran it against master + this PR, verified the same two cases.

• First, where the KBUILD_CFLAGS_MODULE += -mcmodel=large flag is passed in the Makefile and the module building cmdline appended with CFLAGS_MODULE='-mcmodel=large'. It also adds one more -mcmodel=large, to Kernel Build cmdline, making it three consecutive times.

• Second, removing the KBUILD_CFLAGS_MODULE += -mcmodel=large flag in the Makefile, the cmdline used to build the module, is passed with an empty CFLAGS_MODULE='' flag and -mcmodel=large is just passed one.

Then if KBUILD_CFLAGS_MODULE += -mcmodel=large is not needed in this Makefile, I can tack on another commit to remote it.

From the simple meminfo.patch test, it looks safe to remove it.

[sm00th]

Then if KBUILD_CFLAGS_MODULE += -mcmodel=large is not needed in this Makefile, I can tack on another commit to remote it.

To be honest CFLAGS_MODULE='$(KBUILD_CFLAGS_MODULE)' doesn't seem right. Should we just change the one in kpatch-build to CFLAGS_MODULE?

[joe-lawrence]

I agree, though I am really confused right now as I was having problems passing -DKLP_REPLACE_ENABLE=false on 5.14 as it is currently coded. (I thought I accounted for the inadvertent leading space, but I can re-check). Kamalesh's test results confirm that it should have worked... so let me re-test commit again.

[ghost]

Hello, I had also concluded that the '-R' (non-replace) didn't really work, and I was planning on spending some time today to figure out what might be wrong. Fortunately, I took a look at the pull requests, and look; you had already solved it. Thanks! I understand from the above that there is some discussions as to where the CFLAGS_MODULE patch should go, but I can confirm that the current original patch does the job.
FYI, I tested on a Ubuntu 20.04 with the kmod/patch/Makefile patch applied, with three atomic patches, and it worked as it should; I could install and remove, atomically, the three patches.

[joe-lawrence]

v2:

• s/KBUILD_CFLAGS_MODULE/CFLAGS_MODULE/g [sm00th]
• remove the ppc64le -mcmodel=large setting [kamalesh-babulal]

[joe-lawrence]

Still confused when I added some debugging on top of master:

diff --git a/kmod/patch/Makefile b/kmod/patch/Makefile
index 7332774..dee71bb 100644
--- a/kmod/patch/Makefile
+++ b/kmod/patch/Makefile
@@ -1,5 +1,5 @@
 KPATCH_BUILD ?= /lib/modules/$(shell uname -r)/build
-KPATCH_MAKE = $(MAKE) -C $(KPATCH_BUILD) M=$(PWD)
+KPATCH_MAKE = $(MAKE) -C $(KPATCH_BUILD) M=$(PWD) V=1
 LDFLAGS += $(KPATCH_LDFLAGS)
 
 # ppc64le kernel modules are expected to compile with the
@@ -19,6 +19,7 @@ $(KPATCH_NAME)-objs += patch-hook.o output.o
 all: $(KPATCH_NAME).ko
 
 $(KPATCH_NAME).ko:
+       @echo KBUILD_CFLAGS_MODULE='$(KBUILD_CFLAGS_MODULE)'
        $(KPATCH_MAKE)
 
 $(obj)/$(KPATCH_NAME).o: $(src)/kpatch.lds
diff --git a/kmod/patch/livepatch-patch-hook.c b/kmod/patch/livepatch-patch-hook.c
index 120c637..7a26259 100644
--- a/kmod/patch/livepatch-patch-hook.c
+++ b/kmod/patch/livepatch-patch-hook.c
@@ -83,7 +83,10 @@
 #endif
 
 #ifndef KLP_REPLACE_ENABLE
+#warning "KLP_REPLACE_ENABLE not defined"
 #define KLP_REPLACE_ENABLE true
+#else
+#warning "KLP_REPLACE_ENABLE defined"
 #endif
 
 /*

And then did a kpatch-build -R:

% grep -e 'KBUILD_CFLAGS_MODULE=' -e 'KLP_REPLACE_ENABLE' ~/.kpatch/build.log 
KBUILD_CFLAGS_MODULE=-DKLP_REPLACE_ENABLE=false 
/root/.kpatch/tmp/patch/livepatch-patch-hook.c:86:2: warning: #warning "KLP_REPLACE_ENABLE not defined" [-Wcpp]
 #warning "KLP_REPLACE_ENABLE not defined"

Kbuild refuses to pass KLP_REPLACE_ENABLE, but when I added an unconditional -mcmodel=large in that same variable... it did show up in the build.log! (as Kamalesh's tests reported) So I have no idea why Kbuild filters out our -DKLP_REPLACE_ENABLE, but I don't think a complete root cause is necessary for this patchset. @kamalesh-babulal and @luckycat889 have also tested it, so I'm happy enough to put it up for review again.

[sm00th]

Unfortunately make's variable treatment is not very straightforward. So what I found is that automatic variables(such as those gotten from environment) are very fragile, they need to be exported to be relayed to sub-makes, moreover they can not override the ones defined in makefiles themselves (and kernels top-level makefiles indeed has CFLAGS_MODULE= line). So commandline variable is the way to go. There is another option which I think is a bit cleaner:
Instead of using an environment variable pass CFLAGS_MODULE as a cmdline option to patchbuild make call.

But I am ok with this approach as well.