7:49 p.m.
Hi Lianbo,
在 2023/12/12 20:18, Lianbo Jiang 写道:
Hi, Shijie
Thank you for the update.
On 12/1/23 23:04, Huang Shijie wrote:
> Add get_value_vmcoreinfo() to get the value for @name in vmcoreinfo.
>
> 1.) add macro GET_SYMBOL/GET_OFFSET/GET_SIZE to simplify the code.
> 2.) use get_value_vmcoreinfo to simplify the arm64 code.
>
> Signed-off-by: Huang Shijie <shijie(a)os.amperecomputing.com>
> ---
> v1 -->v2:
> 1.) more powerful get_value_vmcoreinfo().
Refactoring the code looks like a good idea, but I did not see much
benefit from the current changes.
The benefit is :
4 files changed, 113 insertions(+), 189 deletions(-)
The code is reduced by 76 lines (I did not change other archs, such as
RISCV..)
Is it possible to extend the existing arm64_get_vmcoreinfo_ul() as below?
static int arm64_get_vmcoreinfo_ul(unsigned long *vaddr, const char*
label, int base)
And the variable 'base' may be 10 or 16 for now, because we can know
whether the string is a decimal string or hexadecimal string before
invoking it, that could be handled in this function, and this won't
touch other arches code.
It is okay to use the base. But how to handle the "label"?
In the vmcoreinfo, there are several types:
"NUMBER(XX)=="
"OFFSET(XX)=="
"SYMBO(XX)=="
"LENGTH(XX)=="
"SIZE(XX)=="
...
Do you have any better solution to handle them?
Any thoughts?
> 2.) I only replace the code in arm64, since I do not have
It seems that the generic code has been changed, eg. kernel.c and
symbols.c.
> boards for other ARCHs, such as x86/ppc.
> ---
> arm64.c | 87 ++++++------------------
> defs.h | 10 +++
> kernel.c | 198 ++++++++++++++++++++++--------------------------------
> symbols.c | 7 +-
> 4 files changed, 113 insertions(+), 189 deletions(-)
>
> diff --git a/arm64.c b/arm64.c
> index 2b6b0e5..e131102 100644
> --- a/arm64.c
> +++ b/arm64.c
> @@ -160,11 +160,8 @@ arm64_init(int when)
> if (!ms->kimage_voffset && STREQ(pc->live_memsrc,
> "/dev/crash"))
> ioctl(pc->mfd, DEV_CRASH_ARCH_DATA, &ms->kimage_voffset);
> - if (!ms->kimage_voffset &&
> - (string = pc->read_vmcoreinfo("NUMBER(kimage_voffset)")))
{
> - ms->kimage_voffset = htol(string, QUIET, NULL);
> - free(string);
> - }
> + if (!ms->kimage_voffset)
> + get_value_vmcoreinfo("kimage_voffset",
> &ms->kimage_voffset, NUMBER_H_TYPE);
> if (ms->kimage_voffset ||
> (ACTIVE() &&
> (symbol_value_from_proc_kallsyms("kimage_voffset") != BADVAL))) {
> @@ -443,9 +440,8 @@ arm64_init(int when)
> arm64_get_section_size_bits();
> if (!machdep->max_physmem_bits) {
> - if ((string =
> pc->read_vmcoreinfo("NUMBER(MAX_PHYSMEM_BITS)"))) {
> - machdep->max_physmem_bits = atol(string);
> - free(string);
> + if (get_value_vmcoreinfo("MAX_PHYSMEM_BITS",
> &machdep->max_physmem_bits, NUMBER_TYPE)) {
> + /* nothing */;
> } else if (machdep->machspec->VA_BITS == 52) /* guess */
> machdep->max_physmem_bits = _MAX_PHYSMEM_BITS_52;
> else if (THIS_KERNEL_VERSION >= LINUX(3,17,0))
> @@ -572,19 +568,6 @@ static int
> arm64_get_struct_page_max_shift(struct machine_specific *ms)
> return (int)ceil(log2(ms->struct_page_size));
> }
> -/* Return TRUE if we succeed, return FALSE on failure. */
> -static int arm64_get_vmcoreinfo_ul(unsigned long *vaddr, const char*
> label)
> -{
> - char *string = pc->read_vmcoreinfo(label);
> -
> - if (!string)
> - return FALSE;
> -
> - *vaddr = strtoul(string, NULL, 0);
> - free(string);
> - return TRUE;
> -}
> -
> /*
> * The change is caused by the kernel patch since v5.18-rc1:
> * "arm64: crash_core: Export MODULES, VMALLOC, and VMEMMAP ranges"
> @@ -594,21 +577,21 @@ static struct kernel_range
> *arm64_get_range_v5_18(struct machine_specific *ms)
> struct kernel_range *r = &tmp_range;
> /* Get the MODULES_VADDR ~ MODULES_END */
> - if (!arm64_get_vmcoreinfo_ul(&r->modules_vaddr,
> "NUMBER(MODULES_VADDR)"))
> + if (!get_value_vmcoreinfo("MODULES_VADDR", &r->modules_vaddr,
> NUMBER_H_TYPE))
> return NULL;
> - if (!arm64_get_vmcoreinfo_ul(&r->modules_end,
> "NUMBER(MODULES_END)"))
> + if (!get_value_vmcoreinfo("MODULES_END", &r->modules_end,
> NUMBER_H_TYPE))
> return NULL;
> /* Get the VMEMMAP_START ~ VMEMMAP_END */
> - if (!arm64_get_vmcoreinfo_ul(&r->vmemmap_vaddr,
> "NUMBER(VMEMMAP_START)"))
> + if (!get_value_vmcoreinfo("VMEMMAP_START", &r->vmemmap_vaddr,
> NUMBER_H_TYPE))
> return NULL;
> - if (!arm64_get_vmcoreinfo_ul(&r->vmemmap_end,
> "NUMBER(VMEMMAP_END)"))
> + if (!get_value_vmcoreinfo("VMEMMAP_END", &r->vmemmap_end,
> NUMBER_H_TYPE))
> return NULL;
> /* Get the VMALLOC_START ~ VMALLOC_END */
> - if (!arm64_get_vmcoreinfo_ul(&r->vmalloc_start_addr,
> "NUMBER(VMALLOC_START)"))
> + if (!get_value_vmcoreinfo("VMALLOC_START",
> &r->vmalloc_start_addr, NUMBER_H_TYPE))
> return NULL;
> - if (!arm64_get_vmcoreinfo_ul(&r->vmalloc_end,
> "NUMBER(VMALLOC_END)"))
> + if (!get_value_vmcoreinfo("VMALLOC_END", &r->vmalloc_end,
> NUMBER_H_TYPE))
> return NULL;
> return r;
> @@ -888,12 +871,7 @@ range_failed:
> /* Get the size of struct page {} */
> static void arm64_get_struct_page_size(struct machine_specific *ms)
> {
> - char *string;
> -
> - string = pc->read_vmcoreinfo("SIZE(page)");
> - if (string)
> - ms->struct_page_size = atol(string);
> - free(string);
> + get_value_vmcoreinfo("page", &ms->struct_page_size,
SIZE_TYPE);
> }
> /*
> @@ -1469,16 +1447,12 @@ arm64_calc_phys_offset(void)
> physaddr_t paddr;
> ulong vaddr;
> struct syment *sp;
> - char *string;
> if ((machdep->flags & NEW_VMEMMAP) &&
> ms->kimage_voffset && (sp =
> kernel_symbol_search("memstart_addr"))) {
> if (pc->flags & PROC_KCORE) {
> - if ((string =
> pc->read_vmcoreinfo("NUMBER(PHYS_OFFSET)"))) {
> - ms->phys_offset = htol(string, QUIET, NULL);
> - free(string);
> + if (get_value_vmcoreinfo("PHYS_OFFSET",
> &ms->phys_offset, NUMBER_H_TYPE))
> return;
> - }
> vaddr =
> symbol_value_from_proc_kallsyms("memstart_addr");
> if (vaddr == BADVAL)
> vaddr = sp->value;
> @@ -1560,9 +1534,8 @@ arm64_get_section_size_bits(void)
> } else
> machdep->section_size_bits = _SECTION_SIZE_BITS;
> - if ((string =
> pc->read_vmcoreinfo("NUMBER(SECTION_SIZE_BITS)"))) {
> - machdep->section_size_bits = atol(string);
> - free(string);
> + if (get_value_vmcoreinfo("SECTION_SIZE_BITS",
> &machdep->section_size_bits, NUMBER_TYPE)) {
> + /* nothing */
> } else if (kt->ikconfig_flags & IKCONFIG_AVAIL) {
> if ((ret = get_kernel_config("CONFIG_MEMORY_HOTPLUG",
> NULL)) == IKCONFIG_Y) {
> if ((ret =
> get_kernel_config("CONFIG_HOTPLUG_SIZE_BITS", &string)) ==
IKCONFIG_STR)
> @@ -1581,15 +1554,11 @@ arm64_get_section_size_bits(void)
> static int
> arm64_kdump_phys_base(ulong *phys_offset)
> {
> - char *string;
> struct syment *sp;
> physaddr_t paddr;
> - if ((string = pc->read_vmcoreinfo("NUMBER(PHYS_OFFSET)"))) {
> - *phys_offset = htol(string, QUIET, NULL);
> - free(string);
> + if (get_value_vmcoreinfo("PHYS_OFFSET", phys_offset,
> NUMBER_H_TYPE))
> return TRUE;
> - }
> if ((machdep->flags & NEW_VMEMMAP) &&
> machdep->machspec->kimage_voffset &&
> @@ -4592,10 +4561,9 @@ static int
> arm64_set_va_bits_by_tcr(void)
> {
> ulong value;
> - char *string;
> - if ((string = pc->read_vmcoreinfo("NUMBER(TCR_EL1_T1SZ)")) ||
> - (string = pc->read_vmcoreinfo("NUMBER(tcr_el1_t1sz)"))) {
> + if (get_value_vmcoreinfo("TCR_EL1_T1SZ", &value, NUMBER_H_TYPE)
||
> + get_value_vmcoreinfo("tcr_el1_t1sz", &value, NUMBER_H_TYPE))
{
> /* See ARMv8 ARM for the description of
> * TCR_EL1.T1SZ and how it can be used
> * to calculate the vabits_actual
> @@ -4604,10 +4572,9 @@ arm64_set_va_bits_by_tcr(void)
> * Basically:
> * vabits_actual = 64 - T1SZ;
> */
> - value = 64 - strtoll(string, NULL, 0);
> + value = 64 - value;
> if (CRASHDEBUG(1))
> fprintf(fp, "vmcoreinfo : vabits_actual: %ld\n", value);
> - free(string);
> machdep->machspec->VA_BITS_ACTUAL = value;
> machdep->machspec->VA_BITS = value;
> machdep->machspec->VA_START =
> _VA_START(machdep->machspec->VA_BITS_ACTUAL);
> @@ -4623,13 +4590,8 @@ arm64_calc_VA_BITS(void)
> int bitval;
> struct syment *sp;
> ulong vabits_actual, value;
> - char *string;
> - if ((string = pc->read_vmcoreinfo("NUMBER(VA_BITS)"))) {
> - value = atol(string);
> - free(string);
> - machdep->machspec->CONFIG_ARM64_VA_BITS = value;
> - }
> + get_value_vmcoreinfo("VA_BITS",
> &machdep->machspec->CONFIG_ARM64_VA_BITS, NUMBER_TYPE);
> if (kernel_symbol_exists("vabits_actual")) {
> if (pc->flags & PROC_KCORE) {
> @@ -4754,10 +4716,8 @@ arm64_calc_virtual_memory_ranges(void)
> ulong PUD_SIZE = UNINITIALIZED;
> if (!machdep->machspec->CONFIG_ARM64_VA_BITS) {
> - if ((string = pc->read_vmcoreinfo("NUMBER(VA_BITS)"))) {
> - value = atol(string);
> - free(string);
> - machdep->machspec->CONFIG_ARM64_VA_BITS = value;
> + if (get_value_vmcoreinfo("VA_BITS",
> &ms->CONFIG_ARM64_VA_BITS, NUMBER_TYPE)) {
> + /* nothing */
> } else if (kt->ikconfig_flags & IKCONFIG_AVAIL) {
> if ((ret = get_kernel_config("CONFIG_ARM64_VA_BITS",
> &string)) == IKCONFIG_STR)
> @@ -4852,11 +4812,8 @@ arm64_swp_offset(ulong pte)
> static void arm64_calc_KERNELPACMASK(void)
> {
> ulong value;
> - char *string;
> - if ((string = pc->read_vmcoreinfo("NUMBER(KERNELPACMASK)"))) {
> - value = htol(string, QUIET, NULL);
> - free(string);
> + if (get_value_vmcoreinfo("KERNELPACMASK", &value, NUMBER_H_TYPE))
{
> machdep->machspec->CONFIG_ARM64_KERNELPACMASK = value;
> if (CRASHDEBUG(1))
> fprintf(fp, "CONFIG_ARM64_KERNELPACMASK: %lx\n", value);
> diff --git a/defs.h b/defs.h
> index 1fe2d0b..282fcc1 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -6055,6 +6055,16 @@ int hide_offline_cpu(int);
> int get_highest_cpu_online(void);
> int get_highest_cpu_present(void);
> int get_cpus_to_display(void);
> +enum vmcore_type {
> + SYMBOL_TYPE,
> + OFFSET_TYPE,
> + NUMBER_TYPE,
> + SIZE_TYPE,
> + LENGTH_TYPE,
> + NUMBER_H_TYPE,
> + MAX_TYPE,
> +};
> +bool get_value_vmcoreinfo(const char *name, ulong *v, enum
> vmcore_type type);
> void get_log_from_vmcoreinfo(char *file);
> int in_cpu_map(int, int);
> void paravirt_init(void);
> diff --git a/kernel.c b/kernel.c
> index 6dcf414..3f3d908 100644
> --- a/kernel.c
> +++ b/kernel.c
> @@ -104,6 +104,46 @@ static void check_vmcoreinfo(void);
> static int is_pvops_xen(void);
> static int get_linux_banner_from_vmlinux(char *, size_t);
> +static char *type_names[MAX_TYPE] = {
> + "SYMBOL",
> + "OFFSET",
> + "NUMBER", /* for NUMBER_TYPE */
+ "SIZE",
> + "LENGTH",
> + "NUMBER", /* for NUMBER_H_TYPE */
> +};
> +
This confused me a bit.
In the current kernel, the code in /kernel/crash_core.c creates the
NUMBER(XX)=%ld
But some ARCHs , such as arch/arm64/kernel/crash_core.c
NUMBER(XX)=%lx
So we need two different types for the "NUMBER" type.
Thanks
Huang Shijie