Message ID | 20230905183604.568996-2-hbathini@linux.ibm.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
Series | [v2,1/3] powerpc/fadump: make is_fadump_active() visible for exporting vmcore | expand |
Hari Bathini <hbathini@linux.ibm.com> writes: > 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. Since, is_kdump_kernel() is used to restrict resources in > crash dump capture kernel and such restrictions are not desirable for > fadump, allow is_kdump_kernel() to be defined differently for fadump > case. With that change, include is_fadump_active() check in functions > is_vmcore_usable() & vmcore_unusable() to be able to export vmcore for > fadump case too. ... > diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h > index 0f3a656293b0..de8a9fabfb6f 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 > + > +#ifndef is_fadump_active > +/* > + * If f/w assisted dump capturing mechanism (fadump), instead of kexec based > + * dump capturing mechanism (kdump) is exporting the vmcore, then this function > + * will be defined in arch specific code to return true, when appropriate. > + */ > +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 +89,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 +99,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; > } I think it would be cleaner to decouple is_vmcore_usable() and vmcore_usable() from is_kdump_kernel(). ie, make them operate solely based on the value of elforehdr_addr: static inline int is_vmcore_usable(void) { elfcorehdr_addr != ELFCORE_ADDR_ERR && \ elfcorehdr_addr != ELFCORE_ADDR_MAX; } static inline void vmcore_unusable(void) { elfcorehdr_addr = ELFCORE_ADDR_ERR; } Then all we need on powerpc is a way to override is_kdump_kernel(). cheers
On 09/11/23 at 05:13pm, Michael Ellerman wrote: > Hari Bathini <hbathini@linux.ibm.com> writes: > > 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. Since, is_kdump_kernel() is used to restrict resources in > > crash dump capture kernel and such restrictions are not desirable for > > fadump, allow is_kdump_kernel() to be defined differently for fadump > > case. With that change, include is_fadump_active() check in functions > > is_vmcore_usable() & vmcore_unusable() to be able to export vmcore for > > fadump case too. > ... > > diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h > > index 0f3a656293b0..de8a9fabfb6f 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 > > + > > +#ifndef is_fadump_active > > +/* > > + * If f/w assisted dump capturing mechanism (fadump), instead of kexec based > > + * dump capturing mechanism (kdump) is exporting the vmcore, then this function > > + * will be defined in arch specific code to return true, when appropriate. > > + */ > > +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 +89,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 +99,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; > > } > > I think it would be cleaner to decouple is_vmcore_usable() and > vmcore_usable() from is_kdump_kernel(). > > ie, make them operate solely based on the value of elforehdr_addr: > > static inline int is_vmcore_usable(void) > { > elfcorehdr_addr != ELFCORE_ADDR_ERR && \ > elfcorehdr_addr != ELFCORE_ADDR_MAX; Agree. I fell into the blind corner of thinking earlier. Above change is better. > } > > static inline void vmcore_unusable(void) > { > elfcorehdr_addr = ELFCORE_ADDR_ERR; > } > > > Then all we need on powerpc is a way to override is_kdump_kernel(). > > cheers >
On 11/09/23 4:01 pm, Baoquan He wrote: > On 09/11/23 at 05:13pm, Michael Ellerman wrote: >> Hari Bathini <hbathini@linux.ibm.com> writes: >>> 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. Since, is_kdump_kernel() is used to restrict resources in >>> crash dump capture kernel and such restrictions are not desirable for >>> fadump, allow is_kdump_kernel() to be defined differently for fadump >>> case. With that change, include is_fadump_active() check in functions >>> is_vmcore_usable() & vmcore_unusable() to be able to export vmcore for >>> fadump case too. >> ... >>> diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h >>> index 0f3a656293b0..de8a9fabfb6f 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 >>> + >>> +#ifndef is_fadump_active >>> +/* >>> + * If f/w assisted dump capturing mechanism (fadump), instead of kexec based >>> + * dump capturing mechanism (kdump) is exporting the vmcore, then this function >>> + * will be defined in arch specific code to return true, when appropriate. >>> + */ >>> +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 +89,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 +99,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; >>> } >> >> I think it would be cleaner to decouple is_vmcore_usable() and >> vmcore_usable() from is_kdump_kernel(). >> >> ie, make them operate solely based on the value of elforehdr_addr: >> >> static inline int is_vmcore_usable(void) >> { >> elfcorehdr_addr != ELFCORE_ADDR_ERR && \ >> elfcorehdr_addr != ELFCORE_ADDR_MAX; > > Agree. I fell into the blind corner of thinking earlier. Above change > is better. Thanks for the reviews. Posted v3. - Hari
diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h index 0f3a656293b0..de8a9fabfb6f 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 + +#ifndef is_fadump_active +/* + * If f/w assisted dump capturing mechanism (fadump), instead of kexec based + * dump capturing mechanism (kdump) is exporting the vmcore, then this function + * will be defined in arch specific code to return true, when appropriate. + */ +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 +89,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 +99,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; }
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. Since, is_kdump_kernel() is used to restrict resources in crash dump capture kernel and such restrictions are not desirable for fadump, allow is_kdump_kernel() to be defined differently for fadump case. With that change, include is_fadump_active() check in functions is_vmcore_usable() & vmcore_unusable() to be able to export vmcore for fadump case too. [1] https://docs.kernel.org/powerpc/firmware-assisted-dump.html Signed-off-by: Hari Bathini <hbathini@linux.ibm.com> --- Changes in v2: * is_fadump_active() check added to is_vmcore_usable() as suggested by Baoquan. include/linux/crash_dump.h | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)