s390 currently always results in is_kdump_kernel() == false, because
it sets "elfcorehdr_addr = ELFCORE_ADDR_MAX;" early during setup_arch to
deactivate the elfcorehdr= kernel parameter.
Let's follow the powerpc example and implement our own logic.
This is required for virtio-mem to reliably identify a kdump
environment to not try hotplugging memory.
Tested-by: Mario Casquero <mcasquer@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
arch/s390/include/asm/kexec.h | 4 ++++
arch/s390/kernel/crash_dump.c | 6 ++++++
2 files changed, 10 insertions(+)
diff --git a/arch/s390/include/asm/kexec.h b/arch/s390/include/asm/kexec.h
index 1bd08eb56d5f..bd20543515f5 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
@@ -107,4 +110,5 @@ int arch_kexec_apply_relocations_add(struct purgatory_info *pi,
int arch_kimage_file_post_load_cleanup(struct kimage *image);
#define arch_kimage_file_post_load_cleanup arch_kimage_file_post_load_cleanup
#endif
+
#endif /*_S390_KEXEC_H */
diff --git a/arch/s390/kernel/crash_dump.c b/arch/s390/kernel/crash_dump.c
index edae13416196..cca1827d3d2e 100644
--- a/arch/s390/kernel/crash_dump.c
+++ b/arch/s390/kernel/crash_dump.c
@@ -237,6 +237,12 @@ int remap_oldmem_pfn_range(struct vm_area_struct *vma, unsigned long from,
prot);
}
+bool is_kdump_kernel(void)
+{
+ return oldmem_data.start && !is_ipl_type_dump();
+}
+EXPORT_SYMBOL_GPL(is_kdump_kernel);
+
static const char *nt_name(Elf64_Word type)
{
const char *name = "LINUX";
--
2.46.1On Mon, Oct 14, 2024 at 04:46:13PM +0200, David Hildenbrand wrote: > s390 currently always results in is_kdump_kernel() == false, because > it sets "elfcorehdr_addr = ELFCORE_ADDR_MAX;" early during setup_arch to > deactivate the elfcorehdr= kernel parameter. > > Let's follow the powerpc example and implement our own logic. > > This is required for virtio-mem to reliably identify a kdump > environment to not try hotplugging memory. > > Tested-by: Mario Casquero <mcasquer@redhat.com> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > arch/s390/include/asm/kexec.h | 4 ++++ > arch/s390/kernel/crash_dump.c | 6 ++++++ > 2 files changed, 10 insertions(+) Looks like this could work. But the comment in smp.c above dump_available() needs to be updated. Are you willing to do that, or should I provide an addon patch?
On 14.10.24 20:20, Heiko Carstens wrote:
> On Mon, Oct 14, 2024 at 04:46:13PM +0200, David Hildenbrand wrote:
>> s390 currently always results in is_kdump_kernel() == false, because
>> it sets "elfcorehdr_addr = ELFCORE_ADDR_MAX;" early during setup_arch to
>> deactivate the elfcorehdr= kernel parameter.
>>
>> Let's follow the powerpc example and implement our own logic.
>>
>> This is required for virtio-mem to reliably identify a kdump
>> environment to not try hotplugging memory.
>>
>> Tested-by: Mario Casquero <mcasquer@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> arch/s390/include/asm/kexec.h | 4 ++++
>> arch/s390/kernel/crash_dump.c | 6 ++++++
>> 2 files changed, 10 insertions(+)
>
> Looks like this could work. But the comment in smp.c above
> dump_available() needs to be updated.
A right, I remember that there was some outdated documentation.
>
> Are you willing to do that, or should I provide an addon patch?
>
I can squash the following:
diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c
index 4df56fdb2488..a4f538876462 100644
--- a/arch/s390/kernel/smp.c
+++ b/arch/s390/kernel/smp.c
@@ -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 old Kdump mode where the old kernel stored the CPU state
+ * does no longer exist: setup_arch explicitly deactivates the elfcorehdr=
+ * kernel parameter. The is_kudmp_kernel() implementation on s390 is independent
+ * of the elfcorehdr= parameter.
*/
static bool dump_available(void)
{
Does that sound reasonable? I'm not so sure about the "2) stand-alone kdump for
SCSI/NVMe (zfcp/nvme dump with swapped memory)": is that really "kdump" ?
--
Cheers,
David / dhildenb
On Mon, Oct 14, 2024 at 09:26:03PM +0200, David Hildenbrand wrote: > On 14.10.24 20:20, Heiko Carstens wrote: > > Looks like this could work. But the comment in smp.c above > > dump_available() needs to be updated. > > A right, I remember that there was some outdated documentation. > > > > > Are you willing to do that, or should I provide an addon patch? > > > > I can squash the following: > > diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c > index 4df56fdb2488..a4f538876462 100644 > --- a/arch/s390/kernel/smp.c > +++ b/arch/s390/kernel/smp.c > @@ -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 old Kdump mode where the old kernel stored the CPU state To be consistent with the rest of the comment, please write kdump in all lower case characters, please. > + * does no longer exist: setup_arch explicitly deactivates the elfcorehdr= > + * kernel parameter. The is_kudmp_kernel() implementation on s390 is independent Typo: kudmp. > Does that sound reasonable? I'm not so sure about the "2) stand-alone kdump for > SCSI/NVMe (zfcp/nvme dump with swapped memory)": is that really "kdump" ? Yes, it is some sort of kdump, even though a bit odd. But the comment as it is doesn't need to be changed. Only at the very top, please also change: "There are four cases" into "There are three cases". Then it all looks good. Thanks a lot!
On 15.10.24 10:30, Heiko Carstens wrote:
> On Mon, Oct 14, 2024 at 09:26:03PM +0200, David Hildenbrand wrote:
>> On 14.10.24 20:20, Heiko Carstens wrote:
>>> Looks like this could work. But the comment in smp.c above
>>> dump_available() needs to be updated.
>>
>> A right, I remember that there was some outdated documentation.
>>
>>>
>>> Are you willing to do that, or should I provide an addon patch?
>>>
>>
>> I can squash the following:
>>
>> diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c
>> index 4df56fdb2488..a4f538876462 100644
>> --- a/arch/s390/kernel/smp.c
>> +++ b/arch/s390/kernel/smp.c
>> @@ -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 old Kdump mode where the old kernel stored the CPU state
>
> To be consistent with the rest of the comment, please write kdump in
> all lower case characters, please.
It obviously was too late in the evening for me :) Thanks!
>
>> + * does no longer exist: setup_arch explicitly deactivates the elfcorehdr=
>> + * kernel parameter. The is_kudmp_kernel() implementation on s390 is independent
>
> Typo: kudmp.
>
>> Does that sound reasonable? I'm not so sure about the "2) stand-alone kdump for
>> SCSI/NVMe (zfcp/nvme dump with swapped memory)": is that really "kdump" ?
>
> Yes, it is some sort of kdump, even though a bit odd.
My concern is that we'll now have
bool is_kdump_kernel(void)
{
return oldmem_data.start && !is_ipl_type_dump();
}
Which matches 3), but if 2) is also called "kdump", then should it
actually be
bool is_kdump_kernel(void)
{
return oldmem_data.start;
}
?
When I wrote that code I was rather convinced that the variant in this
patch is the right thing to do.
--
Cheers,
David / dhildenb
On Tue, Oct 15, 2024 at 10:41:21AM +0200, David Hildenbrand wrote:
> On 15.10.24 10:30, Heiko Carstens wrote:
> > On Mon, Oct 14, 2024 at 09:26:03PM +0200, David Hildenbrand wrote:
> > > On 14.10.24 20:20, Heiko Carstens wrote:
> > > > Looks like this could work. But the comment in smp.c above
> > > > dump_available() needs to be updated.
> > >
> > > A right, I remember that there was some outdated documentation.
...
> My concern is that we'll now have
>
> bool is_kdump_kernel(void)
> {
> return oldmem_data.start && !is_ipl_type_dump();
> }
>
> Which matches 3), but if 2) is also called "kdump", then should it actually
> be
>
> bool is_kdump_kernel(void)
> {
> return oldmem_data.start;
> }
>
> ?
>
> When I wrote that code I was rather convinced that the variant in this patch
> is the right thing to do.
Oh well, we simply of too many dump options. I'll let Alexander and
Mikhail better comment on this :)
Alexander, Mikhail, the discussion starts here, and we need a sane
is_kdump_kernel() for s390:
https://lore.kernel.org/all/20241014144622.876731-1-david@redhat.com/
On 15.10.24 12:08, Heiko Carstens wrote:
> On Tue, Oct 15, 2024 at 10:41:21AM +0200, David Hildenbrand wrote:
>> On 15.10.24 10:30, Heiko Carstens wrote:
>>> On Mon, Oct 14, 2024 at 09:26:03PM +0200, David Hildenbrand wrote:
>>>> On 14.10.24 20:20, Heiko Carstens wrote:
>>>>> Looks like this could work. But the comment in smp.c above
>>>>> dump_available() needs to be updated.
>>>>
>>>> A right, I remember that there was some outdated documentation.
>
> ...
>
>> My concern is that we'll now have
>>
>> bool is_kdump_kernel(void)
>> {
>> return oldmem_data.start && !is_ipl_type_dump();
>> }
>>
>> Which matches 3), but if 2) is also called "kdump", then should it actually
>> be
>>
>> bool is_kdump_kernel(void)
>> {
>> return oldmem_data.start;
>> }
>>
>> ?
>>
>> When I wrote that code I was rather convinced that the variant in this patch
>> is the right thing to do.
>
> Oh well, we simply of too many dump options. I'll let Alexander and
> Mikhail better comment on this :)
Thanks!
>
> Alexander, Mikhail, the discussion starts here, and we need a sane
> is_kdump_kernel() for s390:
> https://lore.kernel.org/all/20241014144622.876731-1-david@redhat.com/
>
With the following cleanup in mind:
From 9fbbff43f725a8482ce9e85857316ab8484ff3c8 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@redhat.com>
Date: Tue, 15 Oct 2024 11:07:43 +0200
Subject: [PATCH] s390: use !dump_available() instead of "!oldmem_data.start &&
!is_ipl_type_dump()"
Let's make the code more readable:
!dump_available()
-> !(oldmem_data.start || is_ipl_type_dump())
-> !oldmem_data.start && !is_ipl_type_dump()
Signed-off-by: David Hildenbrand <david@redhat.com>
---
arch/s390/kernel/crash_dump.c | 2 +-
arch/s390/kernel/os_info.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/s390/kernel/crash_dump.c b/arch/s390/kernel/crash_dump.c
index cca1827d3d2e..fbc5de66d03b 100644
--- a/arch/s390/kernel/crash_dump.c
+++ b/arch/s390/kernel/crash_dump.c
@@ -609,7 +609,7 @@ int elfcorehdr_alloc(unsigned long long *addr, unsigned long long *size)
u64 hdr_off;
/* If we are not in kdump or zfcp/nvme dump mode return */
- if (!oldmem_data.start && !is_ipl_type_dump())
+ if (!dump_available())
return 0;
/* If we cannot get HSA size for zfcp/nvme dump return error */
if (is_ipl_type_dump() && !sclp.hsa_size)
diff --git a/arch/s390/kernel/os_info.c b/arch/s390/kernel/os_info.c
index b695f980bbde..09578f400ef7 100644
--- a/arch/s390/kernel/os_info.c
+++ b/arch/s390/kernel/os_info.c
@@ -148,7 +148,7 @@ static void os_info_old_init(void)
if (os_info_init)
return;
- if (!oldmem_data.start && !is_ipl_type_dump())
+ if (!dump_available())
goto fail;
if (copy_oldmem_kernel(&addr, __LC_OS_INFO, sizeof(addr)))
goto fail;
--
2.46.1
It looks like we create /proc/vmcore, allocating the elfcorehdr essentially
if dump_available() && (!is_ipl_type_dump() || !sclp.hsa_size).
So in the condition, we would have previously made is_kdump_kernel() happt *after* the
fs init calls.
So I'm wondering if is_kdump_kernel() should simply translate to dump_available(). It
doesn't sound completely right to create /proc/vmcore for "standard zfcp/nvme dump",
but that is what we currently do.
I would have thought we have that special zcore thingy for that.
So I think we should either have
bool is_kdump_kernel(void)
{
return dump_available();
}
Or
bool is_kdump_kernel(void)
{
return dump_available() && (!is_ipl_type_dump() || !sclp.hsa_size);
}
I'd prefer the first version.
--
Cheers,
David / dhildenb
Hi David,
> My concern is that we'll now have
>
> bool is_kdump_kernel(void)
> {
> return oldmem_data.start && !is_ipl_type_dump();
> }
>
> Which matches 3), but if 2) is also called "kdump", then should it actually
> be
>
> bool is_kdump_kernel(void)
> {
> return oldmem_data.start;
> }
>
> ?
>
> When I wrote that code I was rather convinced that the variant in this patch
> is the right thing to do.
A short explanation about what a stand-alone kdump is.
* First, it's not really a _regular_ kdump activated with kexec-tools and
executed by Linux itself but a regular stand-alone dump (SCSI) from the
FW's perspective (one has to use HMC or dumpconf to execute it and not
with kexec-tools like for the _regular_ kdump).
* One has to reserve crashkernel memory region in the old crashed kernel
even if it remains unused until the dump starts.
* zipl uses regular kdump kernel and initramfs to create stand-alone
dumper images and to write them to a dump disk which is used for
IPLIng the stand-alone dumper.
* The zipl bootloader takes care of transferring the old kernel memory
saved in HSA by the FW to the crashkernel memory region reserved by the old
crashed kernel before it enters the dumper. The HSA memory is released
by the zipl bootloader _before_ the dumper image is entered,
therefore, we cannot use HSA to read old kernel memory, and instead
use memory from crashkernel region, just like the regular kdump.
* is_ipl_type_dump() will be true for a stand-alone kdump because we IPL
the dumper like a regular stand-alone dump (e.g. zfcpdump).
* Summarized, zipl bootloader prepares an environment which is expected by
the regular kdump for a stand-alone kdump dumper before it is entered.
In my opinion, the correct version of is_kdump_kernel() would be
bool is_kdump_kernel(void)
{
return oldmem_data.start;
}
because Linux kernel doesn't differentiate between both the regular
and the stand-alone kdump where it matters while performing dumper
operations (e.g. reading saved old kernel memory from crashkernel memory region).
Furthermore, if i'm not mistaken then the purpose of is_kdump_kernel()
is to tell us whether Linux kernel runs in a kdump like environment and not
whether the current mode is identical to the proper and true kdump,
right ? And if stand-alone kdump swims like a duck, quacks like one, then it
is one, regardless how it was started, by kexecing or IPLing
from a disk.
The stand-alone kdump has a very special use case which most users will
never encounter. And usually, one just takes zfcpdump instead which is
more robust and much smaller considering how big kdump initrd can get.
stand-alone kdump dumper images cannot exceed HSA memory limit on a Z machine.
Regards
Alex
>>
>> When I wrote that code I was rather convinced that the variant in this patch
>> is the right thing to do.
>
> A short explanation about what a stand-alone kdump is.
>
> * First, it's not really a _regular_ kdump activated with kexec-tools and
> executed by Linux itself but a regular stand-alone dump (SCSI) from the
> FW's perspective (one has to use HMC or dumpconf to execute it and not
> with kexec-tools like for the _regular_ kdump).
Ah, that makes sense.
> * One has to reserve crashkernel memory region in the old crashed kernel
> even if it remains unused until the dump starts.
> * zipl uses regular kdump kernel and initramfs to create stand-alone
> dumper images and to write them to a dump disk which is used for
> IPLIng the stand-alone dumper.
> * The zipl bootloader takes care of transferring the old kernel memory
> saved in HSA by the FW to the crashkernel memory region reserved by the old
> crashed kernel before it enters the dumper. The HSA memory is released
> by the zipl bootloader _before_ the dumper image is entered,
> therefore, we cannot use HSA to read old kernel memory, and instead
> use memory from crashkernel region, just like the regular kdump.
> * is_ipl_type_dump() will be true for a stand-alone kdump because we IPL
> the dumper like a regular stand-alone dump (e.g. zfcpdump).
> * Summarized, zipl bootloader prepares an environment which is expected by
> the regular kdump for a stand-alone kdump dumper before it is entered.
Thanks for the details!
>
> In my opinion, the correct version of is_kdump_kernel() would be
>
> bool is_kdump_kernel(void)
> {
> return oldmem_data.start;
> }
>
> because Linux kernel doesn't differentiate between both the regular
> and the stand-alone kdump where it matters while performing dumper
> operations (e.g. reading saved old kernel memory from crashkernel memory region).
>
Right, but if we consider "/proc/vmcore is available", a better version
would IMHO be:
bool is_kdump_kernel(void)
{
return dump_available();
}
Because that is mostly (not completely) how is_kdump_kernel() would have
worked right now *after* we had the elfcorehdr_alloc() during the
fs_init call.
> Furthermore, if i'm not mistaken then the purpose of is_kdump_kernel()
> is to tell us whether Linux kernel runs in a kdump like environment and not
> whether the current mode is identical to the proper and true kdump,
> right ? And if stand-alone kdump swims like a duck, quacks like one, then it
> is one, regardless how it was started, by kexecing or IPLing
> from a disk.
Same thinking here.
>
> The stand-alone kdump has a very special use case which most users will
> never encounter. And usually, one just takes zfcpdump instead which is
> more robust and much smaller considering how big kdump initrd can get.
> stand-alone kdump dumper images cannot exceed HSA memory limit on a Z machine.
Makes sense, so it boils down to either
bool is_kdump_kernel(void)
{
return oldmem_data.start;
}
Which means is_kdump_kernel() can be "false" even though /proc/vmcore is
available or
bool is_kdump_kernel(void)
{
return dump_available();
}
Which means is_kdump_kernel() can never be "false" if /proc/vmcore is
available. There is the chance of is_kdump_kernel() being "true" if
"elfcorehdr_alloc()" fails with -ENODEV.
You're call :) Thanks!
--
Cheers,
David / dhildenb
Hi David,
David Hildenbrand <david@redhat.com> writes:
> Makes sense, so it boils down to either
>
> bool is_kdump_kernel(void)
> {
> return oldmem_data.start;
> }
>
> Which means is_kdump_kernel() can be "false" even though /proc/vmcore is
> available or
>
> bool is_kdump_kernel(void)
> {
> return dump_available();
> }
>
> Which means is_kdump_kernel() can never be "false" if /proc/vmcore is
> available. There is the chance of is_kdump_kernel() being "true" if
> "elfcorehdr_alloc()" fails with -ENODEV.
Do you consider is_kdump_kernel() returning "true" in case of zfcpdump or
nvme/eckd+ldipl dump (also called NGDump) okay ? Because
dump_available() would return "true" in such cases too.
If yes then please explain why, i might have missed a previous
explanation from you.
I'm afraid everyone will make wrong assumptions while reading the name
of is_kdump_kernel() and assuming that it only applies to kdump or
kdump-alike dumps (like stand-alone kdump), and, therefore, introduce
bugs because the name of the function conveys the wrong idea to code
readers. I consider dump_available() as a superset of is_kdump_kernel()
and, therefore, to me they are not equivalent.
I have the feeling you consider is_kdump_kernel() equivalent to
"/proc/vmcore" being present and not really saying anything about
whether kdump is active ?
Regards
Alex
Am 21.10.24 um 14:46 schrieb Alexander Egorenkov:
> Hi David,
>
> David Hildenbrand <david@redhat.com> writes:
>
>> Makes sense, so it boils down to either
>>
>> bool is_kdump_kernel(void)
>> {
>> return oldmem_data.start;
>> }
>>
>> Which means is_kdump_kernel() can be "false" even though /proc/vmcore is
>> available or
>>
>> bool is_kdump_kernel(void)
>> {
>> return dump_available();
>> }
>>
>> Which means is_kdump_kernel() can never be "false" if /proc/vmcore is
>> available. There is the chance of is_kdump_kernel() being "true" if
>> "elfcorehdr_alloc()" fails with -ENODEV.
Thanks for having another look!
>
> Do you consider is_kdump_kernel() returning "true" in case of zfcpdump or
> nvme/eckd+ldipl dump (also called NGDump) okay ? Because
> dump_available() would return "true" in such cases too.
> If yes then please explain why, i might have missed a previous
> explanation from you.
I consider it okay because this is the current behavior after elfcorehdr_alloc()
succeeded and set elfcorehdr_addr.
Not sure if it is the right think to do, though :)
Whatever we do, we should achieve on s390 that the result of is_kdump_kernel()
is consistent throughout the booting stages, just like on all other architectures.
Right now it goes from false->true when /proc/vmcore gets initialized (and only
if it gets initialized properly).
>
> I'm afraid everyone will make wrong assumptions while reading the name
> of is_kdump_kernel() and assuming that it only applies to kdump or
> kdump-alike dumps (like stand-alone kdump), and, therefore, introduce
> bugs because the name of the function conveys the wrong idea to code
> readers. I consider dump_available() as a superset of is_kdump_kernel()
> and, therefore, to me they are not equivalent.
> > I have the feeling you consider is_kdump_kernel() equivalent to
> "/proc/vmcore" being present and not really saying anything about
> whether kdump is active ?
Yes, but primarily because this is the existing handling.
Staring at the powerpc implementation:
/*
* Return true only when kexec based kernel dump capturing method is used.
* This ensures all restritions applied for kdump case are not automatically
* applied for fadump case.
*/
bool is_kdump_kernel(void)
{
return !is_fadump_active() && elfcorehdr_addr != ELFCORE_ADDR_MAX;
}
EXPORT_SYMBOL_GPL(is_kdump_kernel);
Which was added by
commit b098f1c32365304633077d73e4ae21c72d4241b3
Author: Hari Bathini <hbathini@linux.ibm.com>
Date: Tue Sep 12 13:59:50 2023 +0530
powerpc/fadump: make is_kdump_kernel() return false when fadump is active
Currently, is_kdump_kernel() returns true in crash dump capture kernel
for both kdump and fadump crash dump capturing methods, as both these
methods set elfcorehdr_addr. Some restrictions enforced for crash dump
capture kernel, based on is_kdump_kernel(), are specifically meant for
kdump case and not desirable for fadump - eg. IO queues restriction in
device drivers. So, define is_kdump_kernel() to return false when f/w
assisted dump is active.
For my purpose (virtio-mem), it's sufficient to only support "kexec triggered
kdump" either way, so I don't care.
So for me it's good enough to have
bool is_kdump_kernel(void)
{
return oldmem_data.start;
}
And trying to document the situation in a comment like powerpc does :)
--
Cheers,
David / dhildenb
Hi David,
David Hildenbrand <david@redhat.com> writes:
> Staring at the powerpc implementation:
>
> /*
> * Return true only when kexec based kernel dump capturing method is used.
> * This ensures all restritions applied for kdump case are not automatically
> * applied for fadump case.
> */
> bool is_kdump_kernel(void)
> {
> return !is_fadump_active() && elfcorehdr_addr != ELFCORE_ADDR_MAX;
> }
> EXPORT_SYMBOL_GPL(is_kdump_kernel);
Thanks for the pointer.
I would say power's version is semantically equivalent to what i have in
mind for s390 :) If a dump kernel is running, but not a stand-alone
one (apart from sa kdump), then it's a kdump kernel.
Regards
Alex
On Mon, Oct 21, 2024 at 04:45:59PM +0200, David Hildenbrand wrote:
> For my purpose (virtio-mem), it's sufficient to only support "kexec
> triggered kdump" either way, so I don't care.
>
> So for me it's good enough to have
>
> bool is_kdump_kernel(void)
> {
> return oldmem_data.start;
> }
>
> And trying to document the situation in a comment like powerpc does :)
Then let's go forward with this, since as Alexander wrote, this is returning
what is actually happening. If this is not sufficient or something breaks we
can still address this.
On 23.10.24 09:42, Heiko Carstens wrote:
> On Mon, Oct 21, 2024 at 04:45:59PM +0200, David Hildenbrand wrote:
>> For my purpose (virtio-mem), it's sufficient to only support "kexec
>> triggered kdump" either way, so I don't care.
>>
>> So for me it's good enough to have
>>
>> bool is_kdump_kernel(void)
>> {
>> return oldmem_data.start;
>> }
>>
>> And trying to document the situation in a comment like powerpc does :)
>
> Then let's go forward with this, since as Alexander wrote, this is returning
> what is actually happening. If this is not sufficient or something breaks we
> can still address this.
>
Yes, I'll send this change separately from the other virtio-mem stuff
out today.
--
Cheers,
David / dhildenb
On 16.10.24 17:47, David Hildenbrand wrote:
>>>
>>> When I wrote that code I was rather convinced that the variant in this patch
>>> is the right thing to do.
>>
>> A short explanation about what a stand-alone kdump is.
>>
>> * First, it's not really a _regular_ kdump activated with kexec-tools and
>> executed by Linux itself but a regular stand-alone dump (SCSI) from the
>> FW's perspective (one has to use HMC or dumpconf to execute it and not
>> with kexec-tools like for the _regular_ kdump).
>
> Ah, that makes sense.
>
>> * One has to reserve crashkernel memory region in the old crashed kernel
>> even if it remains unused until the dump starts.
>> * zipl uses regular kdump kernel and initramfs to create stand-alone
>> dumper images and to write them to a dump disk which is used for
>> IPLIng the stand-alone dumper.
>> * The zipl bootloader takes care of transferring the old kernel memory
>> saved in HSA by the FW to the crashkernel memory region reserved by the old
>> crashed kernel before it enters the dumper. The HSA memory is released
>> by the zipl bootloader _before_ the dumper image is entered,
>> therefore, we cannot use HSA to read old kernel memory, and instead
>> use memory from crashkernel region, just like the regular kdump.
>> * is_ipl_type_dump() will be true for a stand-alone kdump because we IPL
>> the dumper like a regular stand-alone dump (e.g. zfcpdump).
>> * Summarized, zipl bootloader prepares an environment which is expected by
>> the regular kdump for a stand-alone kdump dumper before it is entered.
>
> Thanks for the details!
>
>>
>> In my opinion, the correct version of is_kdump_kernel() would be
>>
>> bool is_kdump_kernel(void)
>> {
>> return oldmem_data.start;
>> }
>>
>> because Linux kernel doesn't differentiate between both the regular
>> and the stand-alone kdump where it matters while performing dumper
>> operations (e.g. reading saved old kernel memory from crashkernel memory region).
>>
>
> Right, but if we consider "/proc/vmcore is available", a better version
> would IMHO be:
>
> bool is_kdump_kernel(void)
> {
> return dump_available();
> }
>
> Because that is mostly (not completely) how is_kdump_kernel() would have
> worked right now *after* we had the elfcorehdr_alloc() during the
> fs_init call.
>
>
>> Furthermore, if i'm not mistaken then the purpose of is_kdump_kernel()
>> is to tell us whether Linux kernel runs in a kdump like environment and not
>> whether the current mode is identical to the proper and true kdump,
>> right ? And if stand-alone kdump swims like a duck, quacks like one, then it
>> is one, regardless how it was started, by kexecing or IPLing
>> from a disk.
>
> Same thinking here.
>
>>
>> The stand-alone kdump has a very special use case which most users will
>> never encounter. And usually, one just takes zfcpdump instead which is
>> more robust and much smaller considering how big kdump initrd can get.
>> stand-alone kdump dumper images cannot exceed HSA memory limit on a Z machine.
>
> Makes sense, so it boils down to either
>
> bool is_kdump_kernel(void)
> {
> return oldmem_data.start;
> }
>
> Which means is_kdump_kernel() can be "false" even though /proc/vmcore is
> available or
>
> bool is_kdump_kernel(void)
> {
> return dump_available();
> }
>
> Which means is_kdump_kernel() can never be "false" if /proc/vmcore is
> available. There is the chance of is_kdump_kernel() being "true" if
> "elfcorehdr_alloc()" fails with -ENODEV.
>
>
> You're call :) Thanks!
>
What I think we should do is the following (improved comment + patch
description), but I'll do whatever you think is better:
From e86194b5195c743eff33f563796b9c725fecc65f Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@redhat.com>
Date: Wed, 4 Sep 2024 14:57:10 +0200
Subject: [PATCH] s390/kdump: provide custom is_kdump_kernel()
s390 currently always results in is_kdump_kernel() == false until
vmcore_init()->elfcorehdr_alloc() ran, because it sets
"elfcorehdr_addr = ELFCORE_ADDR_MAX;" early during setup_arch to deactivate
any elfcorehdr= kernel parameter.
Let's follow the powerpc example and implement our own logic. Let's use
"dump_available()", because this is mostly (with one exception when
elfcorehdr_alloc() fails with -ENODEV) when we would create /proc/vmcore
and when is_kdump_kernel() would have returned "true" after
vmcore_init().
This is required for virtio-mem to reliably identify a kdump
environment before vmcore_init() was called to not try hotplugging memory.
Update the documentation above dump_available().
Tested-by: Mario Casquero <mcasquer@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
arch/s390/include/asm/kexec.h | 4 ++++
arch/s390/kernel/crash_dump.c | 6 ++++++
arch/s390/kernel/smp.c | 16 ++++++++--------
3 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/arch/s390/include/asm/kexec.h b/arch/s390/include/asm/kexec.h
index 1bd08eb56d5f..bd20543515f5 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
@@ -107,4 +110,5 @@ int arch_kexec_apply_relocations_add(struct purgatory_info *pi,
int arch_kimage_file_post_load_cleanup(struct kimage *image);
#define arch_kimage_file_post_load_cleanup arch_kimage_file_post_load_cleanup
#endif
+
#endif /*_S390_KEXEC_H */
diff --git a/arch/s390/kernel/crash_dump.c b/arch/s390/kernel/crash_dump.c
index 51313ed7e617..43bbaf534dd2 100644
--- a/arch/s390/kernel/crash_dump.c
+++ b/arch/s390/kernel/crash_dump.c
@@ -237,6 +237,12 @@ int remap_oldmem_pfn_range(struct vm_area_struct *vma, unsigned long from,
prot);
}
+bool is_kdump_kernel(void)
+{
+ return dump_available();
+}
+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..bd41e35a27a0 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 old kdump mode where the old kernel stored the CPU state
+ * 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, and is purely based on dump_available().
*/
static bool dump_available(void)
{
--
2.46.1
--
Cheers,
David / dhildenb
On 15.10.24 10:41, David Hildenbrand wrote:
> On 15.10.24 10:30, Heiko Carstens wrote:
>> On Mon, Oct 14, 2024 at 09:26:03PM +0200, David Hildenbrand wrote:
>>> On 14.10.24 20:20, Heiko Carstens wrote:
>>>> Looks like this could work. But the comment in smp.c above
>>>> dump_available() needs to be updated.
>>>
>>> A right, I remember that there was some outdated documentation.
>>>
>>>>
>>>> Are you willing to do that, or should I provide an addon patch?
>>>>
>>>
>>> I can squash the following:
>>>
>>> diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c
>>> index 4df56fdb2488..a4f538876462 100644
>>> --- a/arch/s390/kernel/smp.c
>>> +++ b/arch/s390/kernel/smp.c
>>> @@ -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 old Kdump mode where the old kernel stored the CPU state
>>
>> To be consistent with the rest of the comment, please write kdump in
>> all lower case characters, please.
>
> It obviously was too late in the evening for me :) Thanks!
>
>>
>>> + * does no longer exist: setup_arch explicitly deactivates the elfcorehdr=
>>> + * kernel parameter. The is_kudmp_kernel() implementation on s390 is independent
>>
>> Typo: kudmp.
>>
>>> Does that sound reasonable? I'm not so sure about the "2) stand-alone kdump for
>>> SCSI/NVMe (zfcp/nvme dump with swapped memory)": is that really "kdump" ?
>>
>> Yes, it is some sort of kdump, even though a bit odd.
>
> My concern is that we'll now have
>
> bool is_kdump_kernel(void)
> {
> return oldmem_data.start && !is_ipl_type_dump();
> }
>
> Which matches 3), but if 2) is also called "kdump", then should it
> actually be
>
> bool is_kdump_kernel(void)
> {
> return oldmem_data.start;
> }
>
> ?
>
> When I wrote that code I was rather convinced that the variant in this
> patch is the right thing to do.
>
I think we can do some follow up cleanups, assuming is_kdump_kernel() here is correct:
diff --git a/arch/s390/kernel/crash_dump.c b/arch/s390/kernel/crash_dump.c
index cca1827d3d2e..fbc5de66d03b 100644
--- a/arch/s390/kernel/crash_dump.c
+++ b/arch/s390/kernel/crash_dump.c
@@ -609,7 +609,7 @@ int elfcorehdr_alloc(unsigned long long *addr, unsigned long long *size)
u64 hdr_off;
/* If we are not in kdump or zfcp/nvme dump mode return */
- if (!oldmem_data.start && !is_ipl_type_dump())
+ if (!dump_available())
return 0;
/* If we cannot get HSA size for zfcp/nvme dump return error */
if (is_ipl_type_dump() && !sclp.hsa_size)
diff --git a/arch/s390/kernel/os_info.c b/arch/s390/kernel/os_info.c
index b695f980bbde..09578f400ef7 100644
--- a/arch/s390/kernel/os_info.c
+++ b/arch/s390/kernel/os_info.c
@@ -148,7 +148,7 @@ static void os_info_old_init(void)
if (os_info_init)
return;
- if (!oldmem_data.start && !is_ipl_type_dump())
+ if (!dump_available())
goto fail;
if (copy_oldmem_kernel(&addr, __LC_OS_INFO, sizeof(addr)))
goto fail;
diff --git a/drivers/s390/char/zcore.c b/drivers/s390/char/zcore.c
index 33cebb91b933..6a194b4f6ba5 100644
--- a/drivers/s390/char/zcore.c
+++ b/drivers/s390/char/zcore.c
@@ -300,9 +300,7 @@ static int __init zcore_init(void)
unsigned char arch;
int rc;
- if (!is_ipl_type_dump())
- return -ENODATA;
- if (oldmem_data.start)
+ if (is_kdump_kernel())
return -ENODATA;
zcore_dbf = debug_register("zcore", 4, 1, 4 * sizeof(long));
--
Cheers,
David / dhildenb
On 15.10.24 10:53, David Hildenbrand wrote:
> On 15.10.24 10:41, David Hildenbrand wrote:
>> On 15.10.24 10:30, Heiko Carstens wrote:
>>> On Mon, Oct 14, 2024 at 09:26:03PM +0200, David Hildenbrand wrote:
>>>> On 14.10.24 20:20, Heiko Carstens wrote:
>>>>> Looks like this could work. But the comment in smp.c above
>>>>> dump_available() needs to be updated.
>>>>
>>>> A right, I remember that there was some outdated documentation.
>>>>
>>>>>
>>>>> Are you willing to do that, or should I provide an addon patch?
>>>>>
>>>>
>>>> I can squash the following:
>>>>
>>>> diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c
>>>> index 4df56fdb2488..a4f538876462 100644
>>>> --- a/arch/s390/kernel/smp.c
>>>> +++ b/arch/s390/kernel/smp.c
>>>> @@ -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 old Kdump mode where the old kernel stored the CPU state
>>>
>>> To be consistent with the rest of the comment, please write kdump in
>>> all lower case characters, please.
>>
>> It obviously was too late in the evening for me :) Thanks!
>>
>>>
>>>> + * does no longer exist: setup_arch explicitly deactivates the elfcorehdr=
>>>> + * kernel parameter. The is_kudmp_kernel() implementation on s390 is independent
>>>
>>> Typo: kudmp.
>>>
>>>> Does that sound reasonable? I'm not so sure about the "2) stand-alone kdump for
>>>> SCSI/NVMe (zfcp/nvme dump with swapped memory)": is that really "kdump" ?
>>>
>>> Yes, it is some sort of kdump, even though a bit odd.
>>
>> My concern is that we'll now have
>>
>> bool is_kdump_kernel(void)
>> {
>> return oldmem_data.start && !is_ipl_type_dump();
>> }
>>
>> Which matches 3), but if 2) is also called "kdump", then should it
>> actually be
>>
>> bool is_kdump_kernel(void)
>> {
>> return oldmem_data.start;
>> }
>>
>> ?
>>
>> When I wrote that code I was rather convinced that the variant in this
>> patch is the right thing to do.
>>
>
> I think we can do some follow up cleanups, assuming is_kdump_kernel() here is correct:
>
> diff --git a/arch/s390/kernel/crash_dump.c b/arch/s390/kernel/crash_dump.c
> index cca1827d3d2e..fbc5de66d03b 100644
> --- a/arch/s390/kernel/crash_dump.c
> +++ b/arch/s390/kernel/crash_dump.c
> @@ -609,7 +609,7 @@ int elfcorehdr_alloc(unsigned long long *addr, unsigned long long *size)
> u64 hdr_off;
>
> /* If we are not in kdump or zfcp/nvme dump mode return */
> - if (!oldmem_data.start && !is_ipl_type_dump())
> + if (!dump_available())
> return 0;
> /* If we cannot get HSA size for zfcp/nvme dump return error */
> if (is_ipl_type_dump() && !sclp.hsa_size)
> diff --git a/arch/s390/kernel/os_info.c b/arch/s390/kernel/os_info.c
> index b695f980bbde..09578f400ef7 100644
> --- a/arch/s390/kernel/os_info.c
> +++ b/arch/s390/kernel/os_info.c
> @@ -148,7 +148,7 @@ static void os_info_old_init(void)
>
> if (os_info_init)
> return;
> - if (!oldmem_data.start && !is_ipl_type_dump())
> + if (!dump_available())
> goto fail;
> if (copy_oldmem_kernel(&addr, __LC_OS_INFO, sizeof(addr)))
> goto fail;
> diff --git a/drivers/s390/char/zcore.c b/drivers/s390/char/zcore.c
> index 33cebb91b933..6a194b4f6ba5 100644
> --- a/drivers/s390/char/zcore.c
> +++ b/drivers/s390/char/zcore.c
> @@ -300,9 +300,7 @@ static int __init zcore_init(void)
> unsigned char arch;
> int rc;
>
> - if (!is_ipl_type_dump())
> - return -ENODATA;
> - if (oldmem_data.start)
> + if (is_kdump_kernel())
> return -ENODATA;
>
> zcore_dbf = debug_register("zcore", 4, 1, 4 * sizeof(long));
>
>
Ugh, ignore the last one, I'm just confused about dumping options on
s390x at this point :)
--
Cheers,
David / dhildenb
© 2016 - 2024 Red Hat, Inc.