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

[Shijie Huang <shijie@xxxxxxxxxxxxxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> > 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

--
Crash-utility mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxxxxxx
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki