[Crash-utility] Re: [PATCH V2 1/2] Add a new helper function get_value_vmcoreinfo

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&gt; >> --- >> 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