Message ID | 20230901190438.375678-1-hbathini@linux.ibm.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | [1/2] vmcore: allow alternate dump capturing methods to export vmcore without is_kdump_kernel() | expand |
Hi Hari, On 09/02/23 at 12:34am, Hari Bathini wrote: > Currently, is_kdump_kernel() returns true when elfcorehdr_addr is set. > While elfcorehdr_addr is set for kexec based kernel dump mechanism, > alternate dump capturing methods like fadump [1] also set it to export > the vmcore. is_kdump_kernel() is used to restrict resources in crash > dump capture kernel but such restrictions may not be desirable for > fadump. Allow is_kdump_kernel() to be defined differently for such > scenarios. With this, is_kdump_kernel() could be false while vmcore > is usable. So, introduce is_crashdump_kernel() to return true when > elfcorehdr_addr is set and use it for vmcore related checks. I got what is done in these two patches, but didn't get why they need be done. vmcore_unusable()/is_vmcore_usable() are only unitilized in ia64. Why do you care if it's is_crashdump_kernel() or is_kdump_kernel()? If you want to override the generic is_kdump_kernel() with powerpc's own is_kdump_kernel(), your below change is enough to allow you to do that. I can't see why is_crashdump_kernel() is needed. Could you explain that specifically? #ifndef is_kdump_kernel static inline bool is_kdump_kernel(void) { return elfcorehdr_addr != ELFCORE_ADDR_MAX; } #endif Thanks Baoquan > [1] https://docs.kernel.org/powerpc/firmware-assisted-dump.html > > Signed-off-by: Hari Bathini <hbathini@linux.ibm.com> > --- > include/linux/crash_dump.h | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h > index 0f3a656293b0..1052a0faf0dd 100644 > --- a/include/linux/crash_dump.h > +++ b/include/linux/crash_dump.h > @@ -50,6 +50,7 @@ void vmcore_cleanup(void); > #define vmcore_elf64_check_arch(x) (elf_check_arch(x) || vmcore_elf_check_arch_cross(x)) > #endif > > +#ifndef is_kdump_kernel > /* > * is_kdump_kernel() checks whether this kernel is booting after a panic of > * previous kernel or not. This is determined by checking if previous kernel > @@ -64,6 +65,19 @@ static inline bool is_kdump_kernel(void) > { > return elfcorehdr_addr != ELFCORE_ADDR_MAX; > } > +#endif > + > +/* > + * Return true if this is a dump capture kernel, where vmcore needs to be > + * exported, irrespective of the dump capture mechanism in use. > + * > + * Same as is_kdump_kernel() unless arch specific code defines is_kdump_kernel() > + * differently while supporting other dump capturing mechanisms. > + */ > +static inline bool is_crashdump_kernel(void) > +{ > + return elfcorehdr_addr != ELFCORE_ADDR_MAX; > +} > > /* is_vmcore_usable() checks if the kernel is booting after a panic and > * the vmcore region is usable. > @@ -75,7 +89,7 @@ static inline bool is_kdump_kernel(void) > > static inline int is_vmcore_usable(void) > { > - return is_kdump_kernel() && elfcorehdr_addr != ELFCORE_ADDR_ERR ? 1 : 0; > + return is_crashdump_kernel() && elfcorehdr_addr != ELFCORE_ADDR_ERR ? 1 : 0; > } > > /* vmcore_unusable() marks the vmcore as unusable, > @@ -84,7 +98,7 @@ static inline int is_vmcore_usable(void) > > static inline void vmcore_unusable(void) > { > - if (is_kdump_kernel()) > + if (is_crashdump_kernel()) > elfcorehdr_addr = ELFCORE_ADDR_ERR; > } > > -- > 2.41.0 >
Hi Baoquan, Thanks for the review... On 03/09/23 9:06 am, Baoquan He wrote: > Hi Hari, > > On 09/02/23 at 12:34am, Hari Bathini wrote: >> Currently, is_kdump_kernel() returns true when elfcorehdr_addr is set. >> While elfcorehdr_addr is set for kexec based kernel dump mechanism, >> alternate dump capturing methods like fadump [1] also set it to export >> the vmcore. is_kdump_kernel() is used to restrict resources in crash >> dump capture kernel but such restrictions may not be desirable for >> fadump. Allow is_kdump_kernel() to be defined differently for such >> scenarios. With this, is_kdump_kernel() could be false while vmcore >> is usable. So, introduce is_crashdump_kernel() to return true when >> elfcorehdr_addr is set and use it for vmcore related checks. > > I got what is done in these two patches, but didn't get why they need be > done. vmcore_unusable()/is_vmcore_usable() are only unitilized in ia64. > Why do you care if it's is_crashdump_kernel() or is_kdump_kernel()? > If you want to override the generic is_kdump_kernel() with powerpc's own > is_kdump_kernel(), your below change is enough to allow you to do that. > I can't see why is_crashdump_kernel() is needed. Could you explain that > specifically? You mean to just remove is_kdump_kernel() check in is_vmcore_usable() & vmcore_unusable() functions? Replaced generic is_crashdump_kernel() function instead, that returns true for any dump capturing method, irrespective of whether is_kdump_kernel() returns true or false. For fadump case, is_kdump_kernel() will return false after patch 2/2. Thanks Hari
On 09/04/23 at 08:04pm, Hari Bathini wrote: > Hi Baoquan, > > Thanks for the review... > > On 03/09/23 9:06 am, Baoquan He wrote: > > Hi Hari, > > > > On 09/02/23 at 12:34am, Hari Bathini wrote: > > > Currently, is_kdump_kernel() returns true when elfcorehdr_addr is set. > > > While elfcorehdr_addr is set for kexec based kernel dump mechanism, > > > alternate dump capturing methods like fadump [1] also set it to export > > > the vmcore. is_kdump_kernel() is used to restrict resources in crash > > > dump capture kernel but such restrictions may not be desirable for > > > fadump. Allow is_kdump_kernel() to be defined differently for such > > > scenarios. With this, is_kdump_kernel() could be false while vmcore > > > is usable. So, introduce is_crashdump_kernel() to return true when > > > elfcorehdr_addr is set and use it for vmcore related checks. > > > > I got what is done in these two patches, but didn't get why they need be > > done. vmcore_unusable()/is_vmcore_usable() are only unitilized in ia64. > > Why do you care if it's is_crashdump_kernel() or is_kdump_kernel()? > > If you want to override the generic is_kdump_kernel() with powerpc's own > > is_kdump_kernel(), your below change is enough to allow you to do that. > > I can't see why is_crashdump_kernel() is needed. Could you explain that > > specifically? > > You mean to just remove is_kdump_kernel() check in is_vmcore_usable() & > vmcore_unusable() functions? Replaced generic is_crashdump_kernel() > function instead, that returns true for any dump capturing method, > irrespective of whether is_kdump_kernel() returns true or false. > For fadump case, is_kdump_kernel() will return false after patch 2/2. OK, I could understand what you want to achieve. You want to make is_kdump_kernel() only return true for kdump, while is_vmcore_usable() returns true for both kdump and fadump. IIUC, can we change as below? It could make code clearer and more straightforward. I don't think adding another is_crashdump_kernel() is a good idea, that would be a torture for non-powerpc people reading code when they need differentiate between kdump and crashdump. diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h index 0f3a656293b0..102a8b710b38 100644 --- a/include/linux/crash_dump.h +++ b/include/linux/crash_dump.h @@ -50,6 +50,7 @@ void vmcore_cleanup(void); #define vmcore_elf64_check_arch(x) (elf_check_arch(x) || vmcore_elf_check_arch_cross(x)) #endif +#ifndef is_kdump_active /* * is_kdump_kernel() checks whether this kernel is booting after a panic of * previous kernel or not. This is determined by checking if previous kernel @@ -64,6 +65,14 @@ static inline bool is_kdump_kernel(void) { return elfcorehdr_addr != ELFCORE_ADDR_MAX; } +#endif + +#ifndef is_fadump_active +static inline bool is_fadump_active(void) +{ + return false; +} +#endif /* is_vmcore_usable() checks if the kernel is booting after a panic and * the vmcore region is usable. @@ -75,7 +84,8 @@ static inline bool is_kdump_kernel(void) static inline int is_vmcore_usable(void) { - return is_kdump_kernel() && elfcorehdr_addr != ELFCORE_ADDR_ERR ? 1 : 0; + return (is_kdump_kernel() || is_fadump_active()) + && elfcorehdr_addr != ELFCORE_ADDR_ERR ? 1 : 0; } /* vmcore_unusable() marks the vmcore as unusable, @@ -84,7 +94,7 @@ static inline int is_vmcore_usable(void) static inline void vmcore_unusable(void) { - if (is_kdump_kernel()) + if (is_kdump_kernel() || is_fadump_active()) elfcorehdr_addr = ELFCORE_ADDR_ERR; }
On 05/09/23 8:00 am, Baoquan He wrote: > On 09/04/23 at 08:04pm, Hari Bathini wrote: >> Hi Baoquan, >> >> Thanks for the review... >> >> On 03/09/23 9:06 am, Baoquan He wrote: >>> Hi Hari, >>> >>> On 09/02/23 at 12:34am, Hari Bathini wrote: >>>> Currently, is_kdump_kernel() returns true when elfcorehdr_addr is set. >>>> While elfcorehdr_addr is set for kexec based kernel dump mechanism, >>>> alternate dump capturing methods like fadump [1] also set it to export >>>> the vmcore. is_kdump_kernel() is used to restrict resources in crash >>>> dump capture kernel but such restrictions may not be desirable for >>>> fadump. Allow is_kdump_kernel() to be defined differently for such >>>> scenarios. With this, is_kdump_kernel() could be false while vmcore >>>> is usable. So, introduce is_crashdump_kernel() to return true when >>>> elfcorehdr_addr is set and use it for vmcore related checks. >>> >>> I got what is done in these two patches, but didn't get why they need be >>> done. vmcore_unusable()/is_vmcore_usable() are only unitilized in ia64. >>> Why do you care if it's is_crashdump_kernel() or is_kdump_kernel()? >>> If you want to override the generic is_kdump_kernel() with powerpc's own >>> is_kdump_kernel(), your below change is enough to allow you to do that. >>> I can't see why is_crashdump_kernel() is needed. Could you explain that >>> specifically? >> >> You mean to just remove is_kdump_kernel() check in is_vmcore_usable() & >> vmcore_unusable() functions? Replaced generic is_crashdump_kernel() >> function instead, that returns true for any dump capturing method, >> irrespective of whether is_kdump_kernel() returns true or false. >> For fadump case, is_kdump_kernel() will return false after patch 2/2. > > OK, I could understand what you want to achieve. You want to make > is_kdump_kernel() only return true for kdump, while is_vmcore_usable() > returns true for both kdump and fadump. > > IIUC, can we change as below? It could make code clearer and more > straightforward. I don't think adding another is_crashdump_kernel() > is a good idea, that would be a torture for non-powerpc people reading > code when they need differentiate between kdump and crashdump. > Sure, Baoquan. Posted v2 based on your suggestion. Thanks Hari
diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h index 0f3a656293b0..1052a0faf0dd 100644 --- a/include/linux/crash_dump.h +++ b/include/linux/crash_dump.h @@ -50,6 +50,7 @@ void vmcore_cleanup(void); #define vmcore_elf64_check_arch(x) (elf_check_arch(x) || vmcore_elf_check_arch_cross(x)) #endif +#ifndef is_kdump_kernel /* * is_kdump_kernel() checks whether this kernel is booting after a panic of * previous kernel or not. This is determined by checking if previous kernel @@ -64,6 +65,19 @@ static inline bool is_kdump_kernel(void) { return elfcorehdr_addr != ELFCORE_ADDR_MAX; } +#endif + +/* + * Return true if this is a dump capture kernel, where vmcore needs to be + * exported, irrespective of the dump capture mechanism in use. + * + * Same as is_kdump_kernel() unless arch specific code defines is_kdump_kernel() + * differently while supporting other dump capturing mechanisms. + */ +static inline bool is_crashdump_kernel(void) +{ + return elfcorehdr_addr != ELFCORE_ADDR_MAX; +} /* is_vmcore_usable() checks if the kernel is booting after a panic and * the vmcore region is usable. @@ -75,7 +89,7 @@ static inline bool is_kdump_kernel(void) static inline int is_vmcore_usable(void) { - return is_kdump_kernel() && elfcorehdr_addr != ELFCORE_ADDR_ERR ? 1 : 0; + return is_crashdump_kernel() && elfcorehdr_addr != ELFCORE_ADDR_ERR ? 1 : 0; } /* vmcore_unusable() marks the vmcore as unusable, @@ -84,7 +98,7 @@ static inline int is_vmcore_usable(void) static inline void vmcore_unusable(void) { - if (is_kdump_kernel()) + if (is_crashdump_kernel()) elfcorehdr_addr = ELFCORE_ADDR_ERR; }
Currently, is_kdump_kernel() returns true when elfcorehdr_addr is set. While elfcorehdr_addr is set for kexec based kernel dump mechanism, alternate dump capturing methods like fadump [1] also set it to export the vmcore. is_kdump_kernel() is used to restrict resources in crash dump capture kernel but such restrictions may not be desirable for fadump. Allow is_kdump_kernel() to be defined differently for such scenarios. With this, is_kdump_kernel() could be false while vmcore is usable. So, introduce is_crashdump_kernel() to return true when elfcorehdr_addr is set and use it for vmcore related checks. [1] https://docs.kernel.org/powerpc/firmware-assisted-dump.html Signed-off-by: Hari Bathini <hbathini@linux.ibm.com> --- include/linux/crash_dump.h | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)