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

Eric DeVolder
2023-07-06T16:08:00Z

On 7/6/23 10:58, Alexander Gordeev wrote:
> 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!

A bit of additional information. I've corrected the problem with s390 and now all config files pass
with the olddefconfig, allyesconfig and allnoconfig targets (using approach outlined in the cover
letter). What I did to resolve the last s390 problem is that I realized that KEXEC was
unconditionally set, so I did the same by adding 'select KEXEC' to the config S390 section.

I'm not saying you shouldn't do the above changes, but wanted to make you aware that we're attacking
the same problem...

I'm running my final regression run here (takes about 8 hours to get through all 385 now) and then I
hope to post v5 within a day.

Thanks,
eric