Re: [PATCH] crash: use macro to add crashk_res into iomem early for specific arch
On 03/24/24 at 11:27am, Ingo Molnar wrote:
>
> * Baoquan He wrote:
>
> > On 03/24/24 at 05:06am, Ingo Molnar wrote:
> > >
> > > * Baoquan He 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_MAXSZ_512M
#else
# define CRASH_ADDR_LOW_MAX SZ_4G
# define CRASH_ADDR_HIGH_MAXSZ_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
>
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH] crash: use macro to add crashk_res into iomem early for specific arch
* Baoquan He wrote:
> On 03/24/24 at 05:06am, Ingo Molnar wrote:
> >
> > * Baoquan He 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
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH] crash: use macro to add crashk_res into iomem early for specific arch
On 03/24/24 at 05:06am, Ingo Molnar wrote:
>
> * Baoquan He 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")
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH] crash: use macro to add crashk_res into iomem early for specific arch
* Baoquan He 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/zfv8icl6ct2jq...@darkstar.users.ipa.redhat.com/T/#u
>
> [2]
> Question about Address Range Validation in Crash Kernel Allocation
> https://lore.kernel.org/all/4eeac1f733584855965a2ea62fa4d...@huawei.com/T/#u
>
> Signed-off-by: Baoquan He
> ---
> 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
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
