[PATCH] crash: use macro to add crashk_res into iomem early for specific arch

[PATCH] crash: use macro to add crashk_res into iomem early for specific arch

[Baoquan He]

There are regression reports[1][2] that crashkernel region on x86_64 can't
be added into iomem tree sometime. This causes the later failure of kdump
loading.

This happened after commit 4a693ce65b18 ("kdump: defer the insertion of
crashkernel resources") was merged.

Even though, these reported issues are proved to be related to other
component, they are just exposed after above commmit applied, I still
would like to keep crashk_res and crashk_low_res being added into iomem
early as before because the early adding has been always there on x86_64
and working very well. For safety of kdump, Let's change it back.

Here, add a macro HAVE_ARCH_ADD_CRASH_RES_TO_IOMEM_EARLY to limit that
only ARCH defining the macro can have the early adding
crashk_res/_low_res into iomem. Then define
HAVE_ARCH_ADD_CRASH_RES_TO_IOMEM_EARLY on x86 to enable it.

Note: In reserve_crashkernel_low(), there's a remnant of crashk_low_res
hanlding which was mistakenly added back in commit 85fcde402db1 ("kexec:
split crashkernel reservation code out from crash_core.c").

[1]
[PATCH V2] x86/kexec: do not update E820 kexec table for setup_data
https://lore.kernel.org/all/Zfv8iCL6CT2JqLIC@darkstar.users.ipa.redhat.com/T/#u

[2]
Question about Address Range Validation in Crash Kernel Allocation
https://lore.kernel.org/all/4eeac1f733584855965a2ea62fa4da58@huawei.com/T/#u

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 arch/x86/include/asm/crash_reserve.h | 2 ++
 kernel/crash_reserve.c               | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/arch/x86/include/asm/crash_reserve.h b/arch/x86/include/asm/crash_reserve.h
index 152239f95541..4681a543eba3 100644
--- a/arch/x86/include/asm/crash_reserve.h
+++ b/arch/x86/include/asm/crash_reserve.h
@@ -39,4 +39,6 @@ static inline unsigned long crash_low_size_default(void)
 #endif
 }
 
+# define HAVE_ARCH_ADD_CRASH_RES_TO_IOMEM_EARLY
+
 #endif /* _X86_CRASH_RESERVE_H */
diff --git a/kernel/crash_reserve.c b/kernel/crash_reserve.c
index bbb6c3cb00e4..066668799f75 100644
--- a/kernel/crash_reserve.c
+++ b/kernel/crash_reserve.c
@@ -366,7 +366,9 @@ static int __init reserve_crashkernel_low(unsigned long long low_size)
 
 	crashk_low_res.start = low_base;
 	crashk_low_res.end   = low_base + low_size - 1;
+#ifdef HAVE_ARCH_ADD_CRASH_RES_TO_IOMEM_EARLY
 	insert_resource(&iomem_resource, &crashk_low_res);
+#endif
 #endif
 	return 0;
 }
@@ -448,8 +450,12 @@ void __init reserve_crashkernel_generic(char *cmdline,
 
 	crashk_res.start = crash_base;
 	crashk_res.end = crash_base + crash_size - 1;
+#ifdef HAVE_ARCH_ADD_CRASH_RES_TO_IOMEM_EARLY
+	insert_resource(&iomem_resource, &crashk_res);
+#endif
 }
 
+#ifndef HAVE_ARCH_ADD_CRASH_RES_TO_IOMEM_EARLY
 static __init int insert_crashkernel_resources(void)
 {
 	if (crashk_res.start < crashk_res.end)
@@ -462,3 +468,4 @@ static __init int insert_crashkernel_resources(void)
 }
 early_initcall(insert_crashkernel_resources);
 #endif
+#endif
-- 
2.41.0

Re: [PATCH] crash: use macro to add crashk_res into iomem early for specific arch

[Ingo Molnar]

* Baoquan He <bhe@redhat.com> wrote:

> There are regression reports[1][2] that crashkernel region on x86_64 can't
> be added into iomem tree sometime. This causes the later failure of kdump
> loading.
> 
> This happened after commit 4a693ce65b18 ("kdump: defer the insertion of
> crashkernel resources") was merged.
> 
> Even though, these reported issues are proved to be related to other
> component, they are just exposed after above commmit applied, I still
> would like to keep crashk_res and crashk_low_res being added into iomem
> early as before because the early adding has been always there on x86_64
> and working very well. For safety of kdump, Let's change it back.
> 
> Here, add a macro HAVE_ARCH_ADD_CRASH_RES_TO_IOMEM_EARLY to limit that
> only ARCH defining the macro can have the early adding
> crashk_res/_low_res into iomem. Then define
> HAVE_ARCH_ADD_CRASH_RES_TO_IOMEM_EARLY on x86 to enable it.
> 
> Note: In reserve_crashkernel_low(), there's a remnant of crashk_low_res
> hanlding which was mistakenly added back in commit 85fcde402db1 ("kexec:
> split crashkernel reservation code out from crash_core.c").
> 
> [1]
> [PATCH V2] x86/kexec: do not update E820 kexec table for setup_data
> https://lore.kernel.org/all/Zfv8iCL6CT2JqLIC@darkstar.users.ipa.redhat.com/T/#u
> 
> [2]
> Question about Address Range Validation in Crash Kernel Allocation
> https://lore.kernel.org/all/4eeac1f733584855965a2ea62fa4da58@huawei.com/T/#u
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  arch/x86/include/asm/crash_reserve.h | 2 ++
>  kernel/crash_reserve.c               | 7 +++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/arch/x86/include/asm/crash_reserve.h b/arch/x86/include/asm/crash_reserve.h
> index 152239f95541..4681a543eba3 100644
> --- a/arch/x86/include/asm/crash_reserve.h
> +++ b/arch/x86/include/asm/crash_reserve.h
> @@ -39,4 +39,6 @@ static inline unsigned long crash_low_size_default(void)
>  #endif
>  }
>  
> +# define HAVE_ARCH_ADD_CRASH_RES_TO_IOMEM_EARLY
> +

Any reason for that stray space?

Thanks,

	Ingo

Re: [PATCH] crash: use macro to add crashk_res into iomem early for specific arch

[Baoquan He]

On 03/24/24 at 05:06am, Ingo Molnar wrote:
> 
> * Baoquan He <bhe@redhat.com> wrote:
> 
......snip
> > ---
> >  arch/x86/include/asm/crash_reserve.h | 2 ++
> >  kernel/crash_reserve.c               | 7 +++++++
> >  2 files changed, 9 insertions(+)
> > 
> > diff --git a/arch/x86/include/asm/crash_reserve.h b/arch/x86/include/asm/crash_reserve.h
> > index 152239f95541..4681a543eba3 100644
> > --- a/arch/x86/include/asm/crash_reserve.h
> > +++ b/arch/x86/include/asm/crash_reserve.h
> > @@ -39,4 +39,6 @@ static inline unsigned long crash_low_size_default(void)
> >  #endif
> >  }
> >  
> > +# define HAVE_ARCH_ADD_CRASH_RES_TO_IOMEM_EARLY
> > +
> 
> Any reason for that stray space?

No clear reason. I saw stray space was added for macro definning when my
below patch was merged, not sure if this is preferred. And there are
a lot of "# define " when searching with 'git grep "# define " arch/x86/include/'.

commit 85fcde402db1 ("kexec: split crashkernel reservation code out from crash_core.c")

Re: [PATCH] crash: use macro to add crashk_res into iomem early for specific arch

[Ingo Molnar]

* Baoquan He <bhe@redhat.com> wrote:

> On 03/24/24 at 05:06am, Ingo Molnar wrote:
> > 
> > * Baoquan He <bhe@redhat.com> wrote:
> > 
> ......snip
> > > ---
> > >  arch/x86/include/asm/crash_reserve.h | 2 ++
> > >  kernel/crash_reserve.c               | 7 +++++++
> > >  2 files changed, 9 insertions(+)
> > > 
> > > diff --git a/arch/x86/include/asm/crash_reserve.h b/arch/x86/include/asm/crash_reserve.h
> > > index 152239f95541..4681a543eba3 100644
> > > --- a/arch/x86/include/asm/crash_reserve.h
> > > +++ b/arch/x86/include/asm/crash_reserve.h
> > > @@ -39,4 +39,6 @@ static inline unsigned long crash_low_size_default(void)
> > >  #endif
> > >  }
> > >  
> > > +# define HAVE_ARCH_ADD_CRASH_RES_TO_IOMEM_EARLY
> > > +
> > 
> > Any reason for that stray space?
> 
> No clear reason. I saw stray space was added for macro definning when my
> below patch was merged, not sure if this is preferred.

No, it's not preferred - and I don't see any stray spaces added in the 
code added by:

> commit 85fcde402db1 ("kexec: split crashkernel reservation code out from crash_core.c")

Anyway, please just remove it.

> And there are a lot of "# define " when searching with 'git grep "# 
> define " arch/x86/include/'.

The overwhelming majority of those are not standalone defines like 
yours, but nested/conditional defines where the space is justified:

#ifdef CONFIG_X86_32
# define MAX_IO_APICS 64
# define MAX_LOCAL_APIC 256
#else
# define MAX_IO_APICS 128
# define MAX_LOCAL_APIC 32768
#endif

Thanks,

	Ingo

Re: [PATCH] crash: use macro to add crashk_res into iomem early for specific arch

[Baoquan He]

On 03/24/24 at 11:27am, Ingo Molnar wrote:
> 
> * Baoquan He <bhe@redhat.com> wrote:
> 
> > On 03/24/24 at 05:06am, Ingo Molnar wrote:
> > > 
> > > * Baoquan He <bhe@redhat.com> wrote:
> > > 
> > ......snip
> > > > ---
> > > >  arch/x86/include/asm/crash_reserve.h | 2 ++
> > > >  kernel/crash_reserve.c               | 7 +++++++
> > > >  2 files changed, 9 insertions(+)
> > > > 
> > > > diff --git a/arch/x86/include/asm/crash_reserve.h b/arch/x86/include/asm/crash_reserve.h
> > > > index 152239f95541..4681a543eba3 100644
> > > > --- a/arch/x86/include/asm/crash_reserve.h
> > > > +++ b/arch/x86/include/asm/crash_reserve.h
> > > > @@ -39,4 +39,6 @@ static inline unsigned long crash_low_size_default(void)
> > > >  #endif
> > > >  }
> > > >  
> > > > +# define HAVE_ARCH_ADD_CRASH_RES_TO_IOMEM_EARLY
> > > > +
> > > 
> > > Any reason for that stray space?
> > 
> > No clear reason. I saw stray space was added for macro definning when my
> > below patch was merged, not sure if this is preferred.
> 
> No, it's not preferred - and I don't see any stray spaces added in the 
> code added by:
> 
> > commit 85fcde402db1 ("kexec: split crashkernel reservation code out from crash_core.c")

Ah, the stray spaces are added in below macros defining, I thought they
are all the same. I didn't know nested/conditional defines and
standalone defines are different. Thanks for careful checking and telling.

arch/x86/include/asm/crash_reserve.h:
#ifdef CONFIG_X86_32
# define CRASH_ADDR_LOW_MAX     SZ_512M
# define CRASH_ADDR_HIGH_MAX    SZ_512M
#else
# define CRASH_ADDR_LOW_MAX     SZ_4G
# define CRASH_ADDR_HIGH_MAX    SZ_64T
#endif

> 
> Anyway, please just remove it.

Sure, will update and v2.

> 
> > And there are a lot of "# define " when searching with 'git grep "# 
> > define " arch/x86/include/'.
> 
> The overwhelming majority of those are not standalone defines like 
> yours, but nested/conditional defines where the space is justified:
> 
> #ifdef CONFIG_X86_32
> # define MAX_IO_APICS 64
> # define MAX_LOCAL_APIC 256
> #else
> # define MAX_IO_APICS 128
> # define MAX_LOCAL_APIC 32768
> #endif
> 
> Thanks,
> 
> 	Ingo
> 

[PATCH v2] crash: use macro to add crashk_res into iomem early for specific arch

[Baoquan He]

There are regression reports[1][2] that crashkernel region on x86_64 can't
be added into iomem tree sometime. This causes the later failure of kdump
loading.

This happened after commit 4a693ce65b18 ("kdump: defer the insertion of
crashkernel resources") was merged.

Even though, these reported issues are proved to be related to other
component, they are just exposed after above commmit applied, I still
would like to keep crashk_res and crashk_low_res being added into iomem
early as before because the early adding has been always there on x86_64
and working very well. For safety of kdump, Let's change it back.

Here, add a macro HAVE_ARCH_ADD_CRASH_RES_TO_IOMEM_EARLY to limit that
only ARCH defining the macro can have the early adding
crashk_res/_low_res into iomem. Then define
HAVE_ARCH_ADD_CRASH_RES_TO_IOMEM_EARLY on x86 to enable it.

Note: In reserve_crashkernel_low(), there's a remnant of crashk_low_res
hanlding which was mistakenly added back in commit 85fcde402db1 ("kexec:
split crashkernel reservation code out from crash_core.c").

[1]
[PATCH V2] x86/kexec: do not update E820 kexec table for setup_data
https://lore.kernel.org/all/Zfv8iCL6CT2JqLIC@darkstar.users.ipa.redhat.com/T/#u

[2]
Question about Address Range Validation in Crash Kernel Allocation
https://lore.kernel.org/all/4eeac1f733584855965a2ea62fa4da58@huawei.com/T/#u

Signed-off-by: Baoquan He <bhe@redhat.com>
---
v1->v2:
- Remove the stray space before 'define' when defining
  HAVE_ARCH_ADD_CRASH_RES_TO_IOMEM_EARLY in asm/crash_reserve.h of x86.
  This is not suggested for standalone macro defines. Thanks to Ingo.

 arch/x86/include/asm/crash_reserve.h | 2 ++
 kernel/crash_reserve.c               | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/arch/x86/include/asm/crash_reserve.h b/arch/x86/include/asm/crash_reserve.h
index 152239f95541..7835b2cdff04 100644
--- a/arch/x86/include/asm/crash_reserve.h
+++ b/arch/x86/include/asm/crash_reserve.h
@@ -39,4 +39,6 @@ static inline unsigned long crash_low_size_default(void)
 #endif
 }
 
+#define HAVE_ARCH_ADD_CRASH_RES_TO_IOMEM_EARLY
+
 #endif /* _X86_CRASH_RESERVE_H */
diff --git a/kernel/crash_reserve.c b/kernel/crash_reserve.c
index bbb6c3cb00e4..066668799f75 100644
--- a/kernel/crash_reserve.c
+++ b/kernel/crash_reserve.c
@@ -366,7 +366,9 @@ static int __init reserve_crashkernel_low(unsigned long long low_size)
 
 	crashk_low_res.start = low_base;
 	crashk_low_res.end   = low_base + low_size - 1;
+#ifdef HAVE_ARCH_ADD_CRASH_RES_TO_IOMEM_EARLY
 	insert_resource(&iomem_resource, &crashk_low_res);
+#endif
 #endif
 	return 0;
 }
@@ -448,8 +450,12 @@ void __init reserve_crashkernel_generic(char *cmdline,
 
 	crashk_res.start = crash_base;
 	crashk_res.end = crash_base + crash_size - 1;
+#ifdef HAVE_ARCH_ADD_CRASH_RES_TO_IOMEM_EARLY
+	insert_resource(&iomem_resource, &crashk_res);
+#endif
 }
 
+#ifndef HAVE_ARCH_ADD_CRASH_RES_TO_IOMEM_EARLY
 static __init int insert_crashkernel_resources(void)
 {
 	if (crashk_res.start < crashk_res.end)
@@ -462,3 +468,4 @@ static __init int insert_crashkernel_resources(void)
 }
 early_initcall(insert_crashkernel_resources);
 #endif
+#endif
-- 
2.41.0

Re: [PATCH v2] crash: use macro to add crashk_res into iomem early for specific arch

[Andrew Morton]

On Mon, 25 Mar 2024 09:50:50 +0800 Baoquan He <bhe@redhat.com> wrote:

> There are regression reports[1][2] that crashkernel region on x86_64 can't
> be added into iomem tree sometime. This causes the later failure of kdump
> loading.

So I think a cc:stable is needed.

> This happened after commit 4a693ce65b18 ("kdump: defer the insertion of
> crashkernel resources") was merged.
> 
> Even though, these reported issues are proved to be related to other
> component, they are just exposed after above commmit applied, I still
> would like to keep crashk_res and crashk_low_res being added into iomem
> early as before because the early adding has been always there on x86_64
> and working very well. For safety of kdump, Let's change it back.

I'll use 4a693ce65b18 as the Fixes: target, since there is no
"Exposed-by-non-buggy-patch:" tag.  To tell the -stable tree
maintainers how far back in history this should be backported.

Re: [PATCH v2] crash: use macro to add crashk_res into iomem early for specific arch

[Baoquan He]

On 03/25/24 at 01:29pm, Andrew Morton wrote:
> On Mon, 25 Mar 2024 09:50:50 +0800 Baoquan He <bhe@redhat.com> wrote:
> 
> > There are regression reports[1][2] that crashkernel region on x86_64 can't
> > be added into iomem tree sometime. This causes the later failure of kdump
> > loading.
> 
> So I think a cc:stable is needed.

Yeah, I forgot this.

> 
> > This happened after commit 4a693ce65b18 ("kdump: defer the insertion of
> > crashkernel resources") was merged.
> > 
> > Even though, these reported issues are proved to be related to other
> > component, they are just exposed after above commmit applied, I still
> > would like to keep crashk_res and crashk_low_res being added into iomem
> > early as before because the early adding has been always there on x86_64
> > and working very well. For safety of kdump, Let's change it back.
> 
> I'll use 4a693ce65b18 as the Fixes: target, since there is no
> "Exposed-by-non-buggy-patch:" tag.  To tell the -stable tree
> maintainers how far back in history this should be backported.

Thanks a lot.