| Message ID | 20211228132612.1860-2-thunder.leizhen@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | support reserving crashkernel above 4G on arm64 kdump | expand |
Hi, Dave, Baoquan, Borislav:
What do you think about the introduction of parse_crashkernel_high_low()? If everyone
doesn't object, I'll bring it to the next version. But I'll make some adjustments to the
patches, see below. If there's any objection, I still strongly recommend removing the
parameters "system_ram" and "crash_base" of parse_crashkernel_{high,low}().
How about splitting __parse_crashkernel() into two parts? One for parsing
"crashkernel=X[@offset]", another one for parsing "crashkernel=X,{high,low}" and other
suffixes in the future. So the parameter requirements are clear at the lowest level.
On 2021/12/28 21:26, Zhen Lei wrote:
> The bootup command line option crashkernel=Y,low is valid only when
> crashkernel=X,high is specified. Putting their parsing into a separate
> function makes the code logic clearer and easier to understand the strong
> dependencies between them.
>
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
> include/linux/crash_core.h | 3 +++
> kernel/crash_core.c | 35 +++++++++++++++++++++++++++++++++++
> 2 files changed, 38 insertions(+)
>
> diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> index de62a722431e7db..2d3a64761d18998 100644
> --- a/include/linux/crash_core.h
> +++ b/include/linux/crash_core.h
> @@ -83,5 +83,8 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,
> unsigned long long *crash_size, unsigned long long *crash_base);
> int parse_crashkernel_low(char *cmdline, unsigned long long system_ram,
> unsigned long long *crash_size, unsigned long long *crash_base);
> +int __init parse_crashkernel_high_low(char *cmdline,
> + unsigned long long *high_size,
> + unsigned long long *low_size);
>
> #endif /* LINUX_CRASH_CORE_H */
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index eb53f5ec62c900f..8966beaf7c4fd52 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -295,6 +295,41 @@ int __init parse_crashkernel_low(char *cmdline,
> "crashkernel=", suffix_tbl[SUFFIX_LOW]);
> }
>
> +/**
> + * parse_crashkernel_high_low - Parsing "crashkernel=X,high" and possible
> + * "crashkernel=Y,low".
> + * @cmdline: The bootup command line.
> + * @high_size: Save the memory size specified by "crashkernel=X,high".
> + * @low_size: Save the memory size specified by "crashkernel=Y,low" or "-1"
> + * if it's not specified.
> + *
> + * Returns 0 on success, else a negative status code.
> + */
> +int __init parse_crashkernel_high_low(char *cmdline,
> + unsigned long long *high_size,
> + unsigned long long *low_size)
> +{
> + int ret;
> + unsigned long long base;
> +
> + BUG_ON(!high_size || !low_size);
> +
> + /* crashkernel=X,high */
> + ret = parse_crashkernel_high(cmdline, 0, high_size, &base);
> + if (ret)
> + return ret;
> +
> + if (*high_size <= 0)
> + return -EINVAL;
> +
> + /* crashkernel=Y,low */
> + ret = parse_crashkernel_low(cmdline, 0, low_size, &base);
> + if (ret)
> + *low_size = -1;
> +
> + return 0;
> +}
> +
> Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type,
> void *data, size_t data_len)
> {
>
On Thu, Dec 30, 2021 at 06:14:59PM +0800, Leizhen (ThunderTown) wrote: > > Hi, Dave, Baoquan, Borislav: > What do you think about the introduction of parse_crashkernel_high_low()? If everyone > doesn't object, I'll bring it to the next version. But I'll make some adjustments to the > patches, see below. If there's any objection, I still strongly recommend removing the > parameters "system_ram" and "crash_base" of parse_crashkernel_{high,low}(). > > How about splitting __parse_crashkernel() into two parts? One for parsing > "crashkernel=X[@offset]", another one for parsing "crashkernel=X,{high,low}" and other > suffixes in the future. So the parameter requirements are clear at the lowest level. First of all, please do not top post! Now, I already explained to you what I'd like to see: https://lore.kernel.org/r/Ycs3kpZD/vpoo1AX@zn.tnic yet you still don't get it. So let me make myself clear: in its current form, this is not really an improvement so for all x86 changes: NAKed-by: Borislav Petkov <bp@suse.de>
On 2021/12/30 18:40, Borislav Petkov wrote: > On Thu, Dec 30, 2021 at 06:14:59PM +0800, Leizhen (ThunderTown) wrote: >> >> Hi, Dave, Baoquan, Borislav: >> What do you think about the introduction of parse_crashkernel_high_low()? If everyone >> doesn't object, I'll bring it to the next version. But I'll make some adjustments to the >> patches, see below. If there's any objection, I still strongly recommend removing the >> parameters "system_ram" and "crash_base" of parse_crashkernel_{high,low}(). >> >> How about splitting __parse_crashkernel() into two parts? One for parsing >> "crashkernel=X[@offset]", another one for parsing "crashkernel=X,{high,low}" and other >> suffixes in the future. So the parameter requirements are clear at the lowest level. > > First of all, please do not top post! > > Now, I already explained to you what I'd like to see: > > https://lore.kernel.org/r/Ycs3kpZD/vpoo1AX@zn.tnic > > yet you still don't get it. > > So let me make myself clear: in its current form, this is not really an > improvement so for all x86 changes: > > NAKed-by: Borislav Petkov <bp@suse.de> > OK, thanks for your immediate reply, so I can take less detours.
On 2021/12/30 19:08, Leizhen (ThunderTown) wrote: > > > On 2021/12/30 18:40, Borislav Petkov wrote: >> On Thu, Dec 30, 2021 at 06:14:59PM +0800, Leizhen (ThunderTown) wrote: >>> >>> Hi, Dave, Baoquan, Borislav: >>> What do you think about the introduction of parse_crashkernel_high_low()? If everyone >>> doesn't object, I'll bring it to the next version. But I'll make some adjustments to the >>> patches, see below. If there's any objection, I still strongly recommend removing the >>> parameters "system_ram" and "crash_base" of parse_crashkernel_{high,low}(). >>> >>> How about splitting __parse_crashkernel() into two parts? One for parsing >>> "crashkernel=X[@offset]", another one for parsing "crashkernel=X,{high,low}" and other >>> suffixes in the future. So the parameter requirements are clear at the lowest level. >> >> First of all, please do not top post! >> >> Now, I already explained to you what I'd like to see: >> >> https://lore.kernel.org/r/Ycs3kpZD/vpoo1AX@zn.tnic >> >> yet you still don't get it. >> >> So let me make myself clear: in its current form, this is not really an >> improvement so for all x86 changes: >> >> NAKed-by: Borislav Petkov <bp@suse.de> Hi Borislav: I'm sorry to bother you again. Do you mind if I make the following changes? I can't stand so many comments appearing twice. Even if the size needs to be changed in the future, mode "low_size = CRASH_LOW_SIZE_MIN + <increment>" can be used for adaptation without affecting other architectures. diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c index e04f5e6eb33f453..da485ee51a9929e 100644 --- a/arch/x86/kernel/setup.c +++ b/arch/x86/kernel/setup.c @@ -428,16 +428,7 @@ static int __init reserve_crashkernel_low(void) /* crashkernel=Y,low */ ret = parse_crashkernel_low(boot_command_line, low_mem_limit, &low_size, &base); if (ret) { - /* - * two parts from kernel/dma/swiotlb.c: - * -swiotlb size: user-specified with swiotlb= or default. - * - * -swiotlb overflow buffer: now hardcoded to 32k. We round it - * to 8M for other buffers that may need to stay low too. Also - * make sure we allocate enough extra low memory so that we - * don't run out of DMA buffers for 32-bit devices. - */ - low_size = max(swiotlb_size_or_default() + (8UL << 20), 256UL << 20); + low_size = CRASH_LOW_SIZE_MIN; } else { /* passed with crashkernel=0,low ? */ if (!low_size) diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h index de62a722431e7db..c85b15814312b7e 100644 --- a/include/linux/crash_core.h +++ b/include/linux/crash_core.h @@ -69,6 +69,17 @@ phys_addr_t paddr_vmcoreinfo_note(void); #define VMCOREINFO_CONFIG(name) \ vmcoreinfo_append_str("CONFIG_%s=y\n", #name) +/* + * two parts from kernel/dma/swiotlb.c: + * -swiotlb size: user-specified with swiotlb= or default. + * + * -swiotlb overflow buffer: now hardcoded to 32k. We round it + * to 8M for other buffers that may need to stay low too. Also + * make sure we allocate enough extra low memory so that we + * don't run out of DMA buffers for 32-bit devices. + */ +#define CRASH_LOW_SIZE_MIN max(swiotlb_size_or_default() + (8UL << 20), 256UL << 20) + extern unsigned char *vmcoreinfo_data; extern size_t vmcoreinfo_size; extern u32 *vmcoreinfo_note; >> > > OK, thanks for your immediate reply, so I can take less detours. >
On 2021/12/31 17:22, Leizhen (ThunderTown) wrote: > > > On 2021/12/30 19:08, Leizhen (ThunderTown) wrote: >> >> >> On 2021/12/30 18:40, Borislav Petkov wrote: >>> On Thu, Dec 30, 2021 at 06:14:59PM +0800, Leizhen (ThunderTown) wrote: >>>> >>>> Hi, Dave, Baoquan, Borislav: >>>> What do you think about the introduction of parse_crashkernel_high_low()? If everyone >>>> doesn't object, I'll bring it to the next version. But I'll make some adjustments to the >>>> patches, see below. If there's any objection, I still strongly recommend removing the >>>> parameters "system_ram" and "crash_base" of parse_crashkernel_{high,low}(). >>>> >>>> How about splitting __parse_crashkernel() into two parts? One for parsing >>>> "crashkernel=X[@offset]", another one for parsing "crashkernel=X,{high,low}" and other >>>> suffixes in the future. So the parameter requirements are clear at the lowest level. >>> >>> First of all, please do not top post! >>> >>> Now, I already explained to you what I'd like to see: >>> >>> https://lore.kernel.org/r/Ycs3kpZD/vpoo1AX@zn.tnic >>> >>> yet you still don't get it. >>> >>> So let me make myself clear: in its current form, this is not really an >>> improvement so for all x86 changes: >>> >>> NAKed-by: Borislav Petkov <bp@suse.de> > > Hi Borislav: > I'm sorry to bother you again. Do you mind if I make the following changes? > I can't stand so many comments appearing twice. Even if the size needs to be > changed in the future, mode "low_size = CRASH_LOW_SIZE_MIN + <increment>" can > be used for adaptation without affecting other architectures. I rethink it, the default value of default_nslabs is IO_TLB_DEFAULT_SIZE=64M. The value of default_nslabs can only be changed by swiotlb_adjust_size() and bootup command line option "swiotlb=". Currently, swiotlb_adjust_size() is invoked only on x86, so I can just ignore it on arm64. Then, 64M is much smaller than 256M, the first kernel works fine with the default 64M on arm64, and I don't think the second kernel needs to grow to 256M. Therefore, I think swiotlb_adjust_size() is probably a pseudo requirement for arm64. So I will directly use 256M on arm64. If anyone gets into trouble, he/she can add it back. Besides, there is also "crashkernel=Y,low" can be used. > > > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c > index e04f5e6eb33f453..da485ee51a9929e 100644 > --- a/arch/x86/kernel/setup.c > +++ b/arch/x86/kernel/setup.c > @@ -428,16 +428,7 @@ static int __init reserve_crashkernel_low(void) > /* crashkernel=Y,low */ > ret = parse_crashkernel_low(boot_command_line, low_mem_limit, &low_size, &base); > if (ret) { > - /* > - * two parts from kernel/dma/swiotlb.c: > - * -swiotlb size: user-specified with swiotlb= or default. > - * > - * -swiotlb overflow buffer: now hardcoded to 32k. We round it > - * to 8M for other buffers that may need to stay low too. Also > - * make sure we allocate enough extra low memory so that we > - * don't run out of DMA buffers for 32-bit devices. > - */ > - low_size = max(swiotlb_size_or_default() + (8UL << 20), 256UL << 20); > + low_size = CRASH_LOW_SIZE_MIN; > } else { > /* passed with crashkernel=0,low ? */ > if (!low_size) > diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h > index de62a722431e7db..c85b15814312b7e 100644 > --- a/include/linux/crash_core.h > +++ b/include/linux/crash_core.h > @@ -69,6 +69,17 @@ phys_addr_t paddr_vmcoreinfo_note(void); > #define VMCOREINFO_CONFIG(name) \ > vmcoreinfo_append_str("CONFIG_%s=y\n", #name) > > +/* > + * two parts from kernel/dma/swiotlb.c: > + * -swiotlb size: user-specified with swiotlb= or default. > + * > + * -swiotlb overflow buffer: now hardcoded to 32k. We round it > + * to 8M for other buffers that may need to stay low too. Also > + * make sure we allocate enough extra low memory so that we > + * don't run out of DMA buffers for 32-bit devices. > + */ > +#define CRASH_LOW_SIZE_MIN max(swiotlb_size_or_default() + (8UL << 20), 256UL << 20) > + > extern unsigned char *vmcoreinfo_data; > extern size_t vmcoreinfo_size; > extern u32 *vmcoreinfo_note; > > >>> >> >> OK, thanks for your immediate reply, so I can take less detours. >> >
On 12/28/21 7:26 AM, Zhen Lei wrote: > The bootup command line option crashkernel=Y,low is valid only when > crashkernel=X,high is specified. Putting their parsing into a separate > function makes the code logic clearer and easier to understand the strong > dependencies between them. > > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > Acked-by: John Donnelly <john.p.donnelly@oracle.com> > --- > include/linux/crash_core.h | 3 +++ > kernel/crash_core.c | 35 +++++++++++++++++++++++++++++++++++ > 2 files changed, 38 insertions(+) > > diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h > index de62a722431e7db..2d3a64761d18998 100644 > --- a/include/linux/crash_core.h > +++ b/include/linux/crash_core.h > @@ -83,5 +83,8 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram, > unsigned long long *crash_size, unsigned long long *crash_base); > int parse_crashkernel_low(char *cmdline, unsigned long long system_ram, > unsigned long long *crash_size, unsigned long long *crash_base); > +int __init parse_crashkernel_high_low(char *cmdline, > + unsigned long long *high_size, > + unsigned long long *low_size); > > #endif /* LINUX_CRASH_CORE_H */ > diff --git a/kernel/crash_core.c b/kernel/crash_core.c > index eb53f5ec62c900f..8966beaf7c4fd52 100644 > --- a/kernel/crash_core.c > +++ b/kernel/crash_core.c > @@ -295,6 +295,41 @@ int __init parse_crashkernel_low(char *cmdline, > "crashkernel=", suffix_tbl[SUFFIX_LOW]); > } > > +/** > + * parse_crashkernel_high_low - Parsing "crashkernel=X,high" and possible > + * "crashkernel=Y,low". > + * @cmdline: The bootup command line. > + * @high_size: Save the memory size specified by "crashkernel=X,high". > + * @low_size: Save the memory size specified by "crashkernel=Y,low" or "-1" > + * if it's not specified. > + * > + * Returns 0 on success, else a negative status code. > + */ > +int __init parse_crashkernel_high_low(char *cmdline, > + unsigned long long *high_size, > + unsigned long long *low_size) > +{ > + int ret; > + unsigned long long base; > + > + BUG_ON(!high_size || !low_size); > + > + /* crashkernel=X,high */ > + ret = parse_crashkernel_high(cmdline, 0, high_size, &base); > + if (ret) > + return ret; > + > + if (*high_size <= 0) > + return -EINVAL; > + > + /* crashkernel=Y,low */ > + ret = parse_crashkernel_low(cmdline, 0, low_size, &base); > + if (ret) > + *low_size = -1; > + > + return 0; > +} > + > Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type, > void *data, size_t data_len) > {
diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h index de62a722431e7db..2d3a64761d18998 100644 --- a/include/linux/crash_core.h +++ b/include/linux/crash_core.h @@ -83,5 +83,8 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram, unsigned long long *crash_size, unsigned long long *crash_base); int parse_crashkernel_low(char *cmdline, unsigned long long system_ram, unsigned long long *crash_size, unsigned long long *crash_base); +int __init parse_crashkernel_high_low(char *cmdline, + unsigned long long *high_size, + unsigned long long *low_size); #endif /* LINUX_CRASH_CORE_H */ diff --git a/kernel/crash_core.c b/kernel/crash_core.c index eb53f5ec62c900f..8966beaf7c4fd52 100644 --- a/kernel/crash_core.c +++ b/kernel/crash_core.c @@ -295,6 +295,41 @@ int __init parse_crashkernel_low(char *cmdline, "crashkernel=", suffix_tbl[SUFFIX_LOW]); } +/** + * parse_crashkernel_high_low - Parsing "crashkernel=X,high" and possible + * "crashkernel=Y,low". + * @cmdline: The bootup command line. + * @high_size: Save the memory size specified by "crashkernel=X,high". + * @low_size: Save the memory size specified by "crashkernel=Y,low" or "-1" + * if it's not specified. + * + * Returns 0 on success, else a negative status code. + */ +int __init parse_crashkernel_high_low(char *cmdline, + unsigned long long *high_size, + unsigned long long *low_size) +{ + int ret; + unsigned long long base; + + BUG_ON(!high_size || !low_size); + + /* crashkernel=X,high */ + ret = parse_crashkernel_high(cmdline, 0, high_size, &base); + if (ret) + return ret; + + if (*high_size <= 0) + return -EINVAL; + + /* crashkernel=Y,low */ + ret = parse_crashkernel_low(cmdline, 0, low_size, &base); + if (ret) + *low_size = -1; + + return 0; +} + Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type, void *data, size_t data_len) {
The bootup command line option crashkernel=Y,low is valid only when crashkernel=X,high is specified. Putting their parsing into a separate function makes the code logic clearer and easier to understand the strong dependencies between them. Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> --- include/linux/crash_core.h | 3 +++ kernel/crash_core.c | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+)