Re: [PATCH v4 12/13] s390/kexec: refactor for kernel/Kconfig.kexec

[Alexander Gordeev <agordeev@linux.ibm.com>]

On Wed, Jul 05, 2023 at 08:49:58AM -0700, Nathan Chancellor wrote:
...
> I just bisected the following build failure visible with 'ARCH=s390
> allnoconfig' to this change as commit 842ce0e1dafa ("s390/kexec:
> refactor for kernel/Kconfig.kexec") in -next.
> 
>   arch/s390/kernel/machine_kexec.c:120:37: warning: 'struct kimage' declared inside parameter list will not be visible outside of this definition or declaration
>     120 | static bool kdump_csum_valid(struct kimage *image)
>         |                                     ^~~~~~
>   arch/s390/kernel/machine_kexec.c:188:34: warning: 'struct kimage' declared inside parameter list will not be visible outside of this definition or declaration
>     188 | int machine_kexec_prepare(struct kimage *image)
>         |                                  ^~~~~~
>   arch/s390/kernel/machine_kexec.c: In function 'machine_kexec_prepare':
>   arch/s390/kernel/machine_kexec.c:192:18: error: invalid use of undefined type 'struct kimage'
>     192 |         if (image->type == KEXEC_TYPE_CRASH)
>         |                  ^~
>   arch/s390/kernel/machine_kexec.c:192:28: error: 'KEXEC_TYPE_CRASH' undeclared (first use in this function); did you mean 'KEXEC_ON_CRASH'?
>     192 |         if (image->type == KEXEC_TYPE_CRASH)
>         |                            ^~~~~~~~~~~~~~~~
>         |                            KEXEC_ON_CRASH
>   arch/s390/kernel/machine_kexec.c:192:28: note: each undeclared identifier is reported only once for each function it appears in
>   arch/s390/kernel/machine_kexec.c:196:18: error: invalid use of undefined type 'struct kimage'
>     196 |         if (image->type != KEXEC_TYPE_DEFAULT)
>         |                  ^~
>   arch/s390/kernel/machine_kexec.c:196:28: error: 'KEXEC_TYPE_DEFAULT' undeclared (first use in this function); did you mean 'KEXEC_ARCH_DEFAULT'?
>     196 |         if (image->type != KEXEC_TYPE_DEFAULT)
>         |                            ^~~~~~~~~~~~~~~~~~
>         |                            KEXEC_ARCH_DEFAULT
>   In file included from arch/s390/include/asm/thread_info.h:31,
>                    from include/linux/thread_info.h:60,
>                    from arch/s390/include/asm/preempt.h:6,
>                    from include/linux/preempt.h:79,
>                    from arch/s390/include/asm/percpu.h:5,
>                    from include/linux/irqflags.h:18,
>                    from include/linux/rcupdate.h:26,
>                    from include/linux/rculist.h:11,
>                    from include/linux/pid.h:5,
>                    from include/linux/sched.h:14,
>                    from include/linux/ratelimit.h:6,
>                    from include/linux/dev_printk.h:16,
>                    from include/linux/device.h:15,
>                    from arch/s390/kernel/machine_kexec.c:9:
>   arch/s390/kernel/machine_kexec.c:200:48: error: invalid use of undefined type 'struct kimage'
>     200 |         reboot_code_buffer = page_to_virt(image->control_code_page);
>         |                                                ^~
>   arch/s390/include/asm/page.h:186:58: note: in definition of macro '__va'
>     186 | #define __va(x)                 ((void *)(unsigned long)(x))
>         |                                                          ^
>   arch/s390/include/asm/page.h:194:38: note: in expansion of macro 'pfn_to_phys'
>     194 | #define pfn_to_virt(pfn)        __va(pfn_to_phys(pfn))
>         |                                      ^~~~~~~~~~~
>   arch/s390/include/asm/page.h:199:33: note: in expansion of macro 'pfn_to_virt'
>     199 | #define page_to_virt(page)      pfn_to_virt(page_to_pfn(page))
>         |                                 ^~~~~~~~~~~
>   include/asm-generic/memory_model.h:64:21: note: in expansion of macro '__page_to_pfn'
>      64 | #define page_to_pfn __page_to_pfn
>         |                     ^~~~~~~~~~~~~
>   arch/s390/kernel/machine_kexec.c:200:30: note: in expansion of macro 'page_to_virt'
>     200 |         reboot_code_buffer = page_to_virt(image->control_code_page);
>         |                              ^~~~~~~~~~~~
>   arch/s390/kernel/machine_kexec.c: At top level:
>   arch/s390/kernel/machine_kexec.c:207:35: warning: 'struct kimage' declared inside parameter list will not be visible outside of this definition or declaration
>     207 | void machine_kexec_cleanup(struct kimage *image)
>         |                                   ^~~~~~
>   arch/s390/kernel/machine_kexec.c: In function '__do_machine_kexec':
>   arch/s390/kernel/machine_kexec.c:243:40: error: invalid use of undefined type 'struct kimage'
>     243 |         data_mover = page_to_phys(image->control_code_page);
>         |                                        ^~
>   arch/s390/include/asm/page.h:189:35: note: in definition of macro 'pfn_to_phys'
>     189 | #define pfn_to_phys(pfn)        ((pfn) << PAGE_SHIFT)
>         |                                   ^~~
>   include/asm-generic/memory_model.h:64:21: note: in expansion of macro '__page_to_pfn'
>      64 | #define page_to_pfn __page_to_pfn
>         |                     ^~~~~~~~~~~~~
>   arch/s390/kernel/machine_kexec.c:243:22: note: in expansion of macro 'page_to_phys'
>     243 |         data_mover = page_to_phys(image->control_code_page);
>         |                      ^~~~~~~~~~~~
>   arch/s390/kernel/machine_kexec.c:244:36: error: invalid use of undefined type 'struct kimage'
>     244 |         entry = virt_to_phys(&image->head);
>         |                                    ^~
>   In file included from arch/s390/kernel/machine_kexec.c:27:
>   arch/s390/kernel/machine_kexec.c:252:40: error: invalid use of undefined type 'struct kimage'
>     252 |                    unsigned long, image->start,
>         |                                        ^~
>   arch/s390/include/asm/stacktrace.h:101:32: note: in definition of macro 'CALL_LARGS_2'
>     101 |         long arg2 = (long)(t2)(a2)
>         |                                ^~
>   arch/s390/include/asm/stacktrace.h:216:9: note: in expansion of macro 'CALL_LARGS_3'
>     216 |         CALL_LARGS_##nr(__VA_ARGS__);                                   \
>         |         ^~~~~~~~~~~
>   arch/s390/kernel/machine_kexec.c:250:9: note: in expansion of macro 'call_nodat'
>     250 |         call_nodat(3, void, (relocate_kernel_t)data_mover,
>         |         ^~~~~~~~~~
>   In file included from include/linux/irqflags.h:15:
>   arch/s390/kernel/machine_kexec.c:252:40: error: invalid use of undefined type 'struct kimage'
>     252 |                    unsigned long, image->start,
>         |                                        ^~
>   include/linux/typecheck.h:11:16: note: in definition of macro 'typecheck'
>      11 |         typeof(x) __dummy2; \
>         |                ^
>   arch/s390/include/asm/stacktrace.h:136:9: note: in expansion of macro 'CALL_TYPECHECK_2'
>     136 |         CALL_TYPECHECK_2(__VA_ARGS__);                                  \
>         |         ^~~~~~~~~~~~~~~~
>   arch/s390/include/asm/stacktrace.h:219:9: note: in expansion of macro 'CALL_TYPECHECK_3'
>     219 |         CALL_TYPECHECK_##nr(__VA_ARGS__);                               \
>         |         ^~~~~~~~~~~~~~~
>   arch/s390/kernel/machine_kexec.c:250:9: note: in expansion of macro 'call_nodat'
>     250 |         call_nodat(3, void, (relocate_kernel_t)data_mover,
>         |         ^~~~~~~~~~
>   include/linux/typecheck.h:12:25: warning: comparison of distinct pointer types lacks a cast
>      12 |         (void)(&__dummy == &__dummy2); \
>         |                         ^~
>   arch/s390/include/asm/stacktrace.h:134:9: note: in expansion of macro 'typecheck'
>     134 |         typecheck(t, a)
>         |         ^~~~~~~~~
>   arch/s390/include/asm/stacktrace.h:136:9: note: in expansion of macro 'CALL_TYPECHECK_2'
>     136 |         CALL_TYPECHECK_2(__VA_ARGS__);                                  \
>         |         ^~~~~~~~~~~~~~~~
>   arch/s390/include/asm/stacktrace.h:219:9: note: in expansion of macro 'CALL_TYPECHECK_3'
>     219 |         CALL_TYPECHECK_##nr(__VA_ARGS__);                               \
>         |         ^~~~~~~~~~~~~~~
>   arch/s390/kernel/machine_kexec.c:250:9: note: in expansion of macro 'call_nodat'
>     250 |         call_nodat(3, void, (relocate_kernel_t)data_mover,
>         |         ^~~~~~~~~~
>   arch/s390/kernel/machine_kexec.c: At top level:
>   arch/s390/kernel/machine_kexec.c:278:27: warning: 'struct kimage' declared inside parameter list will not be visible outside of this definition or declaration
>     278 | void machine_kexec(struct kimage *image)
>         |                           ^~~~~~
>   arch/s390/kernel/machine_kexec.c: In function 'machine_kexec':
>   arch/s390/kernel/machine_kexec.c:280:18: error: invalid use of undefined type 'struct kimage'
>     280 |         if (image->type == KEXEC_TYPE_CRASH && !kdump_csum_valid(image))
>         |                  ^~
>   arch/s390/kernel/machine_kexec.c:280:28: error: 'KEXEC_TYPE_CRASH' undeclared (first use in this function); did you mean 'KEXEC_ON_CRASH'?
>     280 |         if (image->type == KEXEC_TYPE_CRASH && !kdump_csum_valid(image))
>         |                            ^~~~~~~~~~~~~~~~
>         |                            KEXEC_ON_CRASH
>   arch/s390/kernel/machine_kexec.c:280:66: error: passing argument 1 of 'kdump_csum_valid' from incompatible pointer type [-Werror=incompatible-pointer-types]
>     280 |         if (image->type == KEXEC_TYPE_CRASH && !kdump_csum_valid(image))
>         |                                                                  ^~~~~
>         |                                                                  |
>         |                                                                  struct kimage *
>   arch/s390/kernel/machine_kexec.c:120:45: note: expected 'struct kimage *' but argument is of type 'struct kimage *'
>     120 | static bool kdump_csum_valid(struct kimage *image)
>         |                              ~~~~~~~~~~~~~~~^~~~~
>   cc1: some warnings being treated as errors
> 
> I don't think this change is equivalent for s390, which had
> 
>   config KEXEC
>       def_bool y
>       select KEXEC_CORE
> 
> but it is now the equivalent of
> 
>   config KEXEC
>       bool "Enable kexec system call"
>       default y
> 
> which enables KEXEC by default but it also allows KEXEC to be disabled
> for s390 now, because it is a user-visible symbol, not one that is
> unconditionally enabled no matter what. If s390 can tolerate KEXEC being
> user selectable, then I assume the fix is just adjusting
> arch/s390/kernel/Makefile to only build the machine_kexec files when
> CONFIG_KEXEC_CORE is set:
> 
> diff --git a/arch/s390/kernel/Makefile b/arch/s390/kernel/Makefile
> index 6b2a051e1f8a..a06b39da95f0 100644
> --- a/arch/s390/kernel/Makefile
> +++ b/arch/s390/kernel/Makefile
> @@ -37,10 +37,10 @@ CFLAGS_unwind_bc.o	+= -fno-optimize-sibling-calls
>  obj-y	:= head64.o traps.o time.o process.o earlypgm.o early.o setup.o idle.o vtime.o
>  obj-y	+= processor.o syscall.o ptrace.o signal.o cpcmd.o ebcdic.o nmi.o
>  obj-y	+= debug.o irq.o ipl.o dis.o diag.o vdso.o cpufeature.o
> -obj-y	+= sysinfo.o lgr.o os_info.o machine_kexec.o
> +obj-y	+= sysinfo.o lgr.o os_info.o
>  obj-y	+= runtime_instr.o cache.o fpu.o dumpstack.o guarded_storage.o sthyi.o
>  obj-y	+= entry.o reipl.o relocate_kernel.o kdebugfs.o alternative.o
> -obj-y	+= nospec-branch.o ipl_vmparm.o machine_kexec_reloc.o unwind_bc.o
> +obj-y	+= nospec-branch.o ipl_vmparm.o unwind_bc.o
>  obj-y	+= smp.o text_amode31.o stacktrace.o abs_lowcore.o
>  
>  extra-y				+= vmlinux.lds
> @@ -66,6 +66,7 @@ obj-$(CONFIG_CRASH_DUMP)	+= crash_dump.o
>  obj-$(CONFIG_UPROBES)		+= uprobes.o
>  obj-$(CONFIG_JUMP_LABEL)	+= jump_label.o
>  
> +obj-$(CONFIG_KEXEC_CORE)	+= machine_kexec.o machine_kexec_reloc.o
>  obj-$(CONFIG_KEXEC_FILE)	+= machine_kexec_file.o kexec_image.o
>  obj-$(CONFIG_KEXEC_FILE)	+= kexec_elf.o
>  
> 
> Otherwise, the prompt for KEXEC could be made conditional on some ARCH
> symbol so that architectures can opt out of it.

Hi Nathan,

Thanks a lot for looking into it!
With few modification the fix would looke like below.
It probably needs to be a pre-requisite for this series:


[PATCH] s390/kexec: make machine_kexec depend on CONFIG_KEXEC_CORE

Make machine_kexec.o and relocate_kernel.o depend on
CONFIG_KEXEC_CORE option as other architectures do.

Still generate machine_kexec_reloc.o unconditionally,
since arch_kexec_do_relocs() function is neded by the
decompressor.

Probably, #include <asm/kexec.h> could be be removed from
machine_kexec_reloc.c source as well, but that would revert
commit 155e6c706125 ("s390/kexec: add missing include to
machine_kexec_reloc.c").

Suggested-by: Nathan Chancellor <nathan@kernel.org>
Reported-by: Nathan Chancellor <nathan@kernel.org>
Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
---
 arch/s390/kernel/Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kernel/Makefile b/arch/s390/kernel/Makefile
index 8d7514c72bb8..0df2b88cc0da 100644
--- a/arch/s390/kernel/Makefile
+++ b/arch/s390/kernel/Makefile
@@ -37,9 +37,9 @@ CFLAGS_unwind_bc.o	+= -fno-optimize-sibling-calls
 obj-y	:= head64.o traps.o time.o process.o earlypgm.o early.o setup.o idle.o vtime.o
 obj-y	+= processor.o syscall.o ptrace.o signal.o cpcmd.o ebcdic.o nmi.o
 obj-y	+= debug.o irq.o ipl.o dis.o diag.o vdso.o cpufeature.o
-obj-y	+= sysinfo.o lgr.o os_info.o machine_kexec.o
+obj-y	+= sysinfo.o lgr.o os_info.o
 obj-y	+= runtime_instr.o cache.o fpu.o dumpstack.o guarded_storage.o sthyi.o
-obj-y	+= entry.o reipl.o relocate_kernel.o kdebugfs.o alternative.o
+obj-y	+= entry.o reipl.o kdebugfs.o alternative.o
 obj-y	+= nospec-branch.o ipl_vmparm.o machine_kexec_reloc.o unwind_bc.o
 obj-y	+= smp.o text_amode31.o stacktrace.o abs_lowcore.o
 
@@ -63,6 +63,7 @@ obj-$(CONFIG_RETHOOK)		+= rethook.o
 obj-$(CONFIG_FUNCTION_TRACER)	+= ftrace.o
 obj-$(CONFIG_FUNCTION_TRACER)	+= mcount.o
 obj-$(CONFIG_CRASH_DUMP)	+= crash_dump.o
+obj-$(CONFIG_KEXEC_CORE)	+= machine_kexec.o relocate_kernel.o
 obj-$(CONFIG_UPROBES)		+= uprobes.o
 obj-$(CONFIG_JUMP_LABEL)	+= jump_label.o
 
> Cheers,
> Nathan

Thanks!