s390 sets "elfcorehdr_addr = ELFCORE_ADDR_MAX;" early during
setup_arch() to deactivate the "elfcorehdr= kernel" parameter, resulting in
is_kdump_kernel() returning "false".

During vmcore_init()->elfcorehdr_alloc(), if on a dump kernel and
allocation succeeded, elfcorehdr_addr will be set to a valid address and
is_kdump_kernel() will consequently return "true".

We want to make is_kdump_kernel() return a consistent result during
all boot stages, and properly return "true" if we are actually in a kdump
environment -- just like we already do on powerpc where we indicate "false"
in fadump environments, as added in commit b098f1c32365 ("powerpc/fadump:
make is_kdump_kernel() return false when fadump is active").

Similarly provide a custom is_kdump_kernel() implementation that will only
return "true" in kdump environments, and will do so consistently during
boot.

Update the documentation of is_dump_available().

Cc: Heiko Carstens <hca@linux.ibm.com>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: Alexander Gordeev <agordeev@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
Cc: Sven Schnelle <svens@linux.ibm.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---

This is v3 of [1], split out from the virtio-mem stuff.

I played more with having virtio-mem built in as a module on current
upstream and at least for virtio-mem this change *might* currently not be
required (built-in virtio-mem driver seems to get probed after fs_init();
I recall this behavior was different 4 years ago with my RFCs where I
first decided to craft this patch).

But this change sounds like a reasonable cleanup to me in any case.

v1 -> v2:
* Use "oldmem_data.start" and add a comment to is_kdump_kernel()
* Update dump_available() documentation
* Rewrote patch subject/description

[1] https://lore.kernel.org/all/20241014144622.876731-2-david@redhat.com/

---
 arch/s390/include/asm/kexec.h |  3 +++
 arch/s390/kernel/crash_dump.c | 11 +++++++++++
 arch/s390/kernel/smp.c        | 16 ++++++++--------
 3 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/arch/s390/include/asm/kexec.h b/arch/s390/include/asm/kexec.h
index 1bd08eb56d5f..9084b750350d 100644
--- a/arch/s390/include/asm/kexec.h
+++ b/arch/s390/include/asm/kexec.h
@@ -94,6 +94,9 @@ void arch_kexec_protect_crashkres(void);
 
 void arch_kexec_unprotect_crashkres(void);
 #define arch_kexec_unprotect_crashkres arch_kexec_unprotect_crashkres
+
+bool is_kdump_kernel(void);
+#define is_kdump_kernel is_kdump_kernel
 #endif
 
 #ifdef CONFIG_KEXEC_FILE
diff --git a/arch/s390/kernel/crash_dump.c b/arch/s390/kernel/crash_dump.c
index edae13416196..d9301c00852e 100644
--- a/arch/s390/kernel/crash_dump.c
+++ b/arch/s390/kernel/crash_dump.c
@@ -237,6 +237,17 @@ int remap_oldmem_pfn_range(struct vm_area_struct *vma, unsigned long from,
 						       prot);
 }
 
+/*
+ * Return true only when we are in a kdump or stand-alone kdump environment.
+ * Note that /proc/vmcore might also be available in "standard zfcp/nvme dump"
+ * environments, where this function returns false; see dump_available().
+ */
+bool is_kdump_kernel(void)
+{
+	return oldmem_data.start;
+}
+EXPORT_SYMBOL_GPL(is_kdump_kernel);
+
 static const char *nt_name(Elf64_Word type)
 {
 	const char *name = "LINUX";
diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c
index 4df56fdb2488..455400bdafe8 100644
--- a/arch/s390/kernel/smp.c
+++ b/arch/s390/kernel/smp.c
@@ -574,7 +574,7 @@ int smp_store_status(int cpu)
 
 /*
  * Collect CPU state of the previous, crashed system.
- * There are four cases:
+ * There are three cases:
  * 1) standard zfcp/nvme dump
  *    condition: OLDMEM_BASE == NULL && is_ipl_type_dump() == true
  *    The state for all CPUs except the boot CPU needs to be collected
@@ -587,16 +587,16 @@ int smp_store_status(int cpu)
  *    with sigp stop-and-store-status. The firmware or the boot-loader
  *    stored the registers of the boot CPU in the absolute lowcore in the
  *    memory of the old system.
- * 3) kdump and the old kernel did not store the CPU state,
- *    or stand-alone kdump for DASD
- *    condition: OLDMEM_BASE != NULL && !is_kdump_kernel()
+ * 3) kdump or stand-alone kdump for DASD
+ *    condition: OLDMEM_BASE != NULL && !is_ipl_type_dump() == false
  *    The state for all CPUs except the boot CPU needs to be collected
  *    with sigp stop-and-store-status. The kexec code or the boot-loader
  *    stored the registers of the boot CPU in the memory of the old system.
- * 4) kdump and the old kernel stored the CPU state
- *    condition: OLDMEM_BASE != NULL && is_kdump_kernel()
- *    This case does not exist for s390 anymore, setup_arch explicitly
- *    deactivates the elfcorehdr= kernel parameter
+ *
+ * Note that the legacy kdump mode where the old kernel stored the CPU states
+ * does no longer exist: setup_arch() explicitly deactivates the elfcorehdr=
+ * kernel parameter. The is_kdump_kernel() implementation on s390 is independent
+ * of the elfcorehdr= parameter.
  */
 static bool dump_available(void)
 {
-- 
2.46.1

Hi David,


David Hildenbrand <david@redhat.com> writes:

> Similarly provide a custom is_kdump_kernel() implementation that will only
> return "true" in kdump environments, and will do so consistently during
> boot.
>
> Update the documentation of is_dump_available().

A small typo here:

is_dump_available() -> dump_available()

> @@ -587,16 +587,16 @@ int smp_store_status(int cpu)
>   *    with sigp stop-and-store-status. The firmware or the boot-loader
>   *    stored the registers of the boot CPU in the absolute lowcore in the
>   *    memory of the old system.
> - * 3) kdump and the old kernel did not store the CPU state,
> - *    or stand-alone kdump for DASD
> - *    condition: OLDMEM_BASE != NULL && !is_kdump_kernel()
> + * 3) kdump or stand-alone kdump for DASD
> + *    condition: OLDMEM_BASE != NULL && !is_ipl_type_dump() == false

Here is a typo in the condition, a redundant '!' before is_ipl_type_dump().

Otherwise, looks very good to me.

Reviewed-by: Alexander Egorenkov <egorenar@linux.ibm.com> 

Regards
Alex

On 23.10.24 14:12, Alexander Egorenkov wrote:
> Hi David,
> 
> 
> David Hildenbrand <david@redhat.com> writes:
> 
>> Similarly provide a custom is_kdump_kernel() implementation that will only
>> return "true" in kdump environments, and will do so consistently during
>> boot.
>>
>> Update the documentation of is_dump_available().
> 
> A small typo here:
> 
> is_dump_available() -> dump_available()
> 
>> @@ -587,16 +587,16 @@ int smp_store_status(int cpu)
>>    *    with sigp stop-and-store-status. The firmware or the boot-loader
>>    *    stored the registers of the boot CPU in the absolute lowcore in the
>>    *    memory of the old system.
>> - * 3) kdump and the old kernel did not store the CPU state,
>> - *    or stand-alone kdump for DASD
>> - *    condition: OLDMEM_BASE != NULL && !is_kdump_kernel()
>> + * 3) kdump or stand-alone kdump for DASD
>> + *    condition: OLDMEM_BASE != NULL && !is_ipl_type_dump() == false
> 
> Here is a typo in the condition, a redundant '!' before is_ipl_type_dump().
> 

Thanks for catching these! Too much going back and forth ... :)

> Otherwise, looks very good to me.

Thanks for the fast review. I assume these can be fixed up when 
applying. But please let me know if a v3 is preferred, and I can send 
one after waiting a couple of days.

-- 
Cheers,

David / dhildenb

On Wed, Oct 23, 2024 at 02:22:18PM +0200, David Hildenbrand wrote:
> On 23.10.24 14:12, Alexander Egorenkov wrote:
> > Here is a typo in the condition, a redundant '!' before is_ipl_type_dump().
> > 
> 
> Thanks for catching these! Too much going back and forth ... :)
> 
> > Otherwise, looks very good to me.
> 
> Thanks for the fast review. I assume these can be fixed up when applying.
> But please let me know if a v3 is preferred, and I can send one after
> waiting a couple of days.

Fixed typos, slightly reworded subject and commit message, and applied.
Thanks a lot!