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

[Shijie Huang <shijie@xxxxxxxxxxxxxxxxxxxxxxxxxx>]

Hi Lianbo,

在 2023/12/14 12:10, Lianbo Jiang 写道:
> [EXTERNAL EMAIL NOTICE: This email originated from an external sender. 
> Please be mindful of safe email handling and proprietary information 
> protection practices.]
>
>
> On 12/13/23 09:49, Shijie Huang wrote:
>
> > 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"?
> Please do not touch the "label", it keeps the same key-value pairs in
> the VMCOREINFO data. It is easy to debug if there are any changes, and
> is more readable to me.
okay.
> > 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?
> Let me give an example, just an idea.
>
> Try to extend the existing arm64_get_vmcoreinfo_ul() as below:
>
> static int arm64_get_vmcoreinfo_ul(unsigned long *vaddr, const char*
> label, int base)
> {
>         int err = 0;
>         char *string = pc->read_vmcoreinfo(label);
>
>         if (!string)
>                 return FALSE;
>
>         switch(base)
>         {
>         case NUM_HEX:
>                 *vaddr = strtoul(string, NULL, 16);
>                 break;
>         case NUM_DEC:
>                 *vaddr = strtoul(string, NULL, 10);
>                 break;
>         default:
>                 err++;
>                 error(INFO, "Unknown type: 0x%x, (NUM_HEX|NUM_DEC)\n",
> base);
>         }
>
>         free(string);
>         return (err > 0) ? FALSE : TRUE;
> }
>
>
> And it can be invoked in arm64 code, for example:
>
> @@ -160,11 +159,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)
> + arm64_get_vmcoreinfo_ul(&ms->kimage_voffset, "NUMBER(kimage_voffset)",
> NUM_HEX);
>
>                 if (ms->kimage_voffset ||
>                     (ACTIVE() &&
> (symbol_value_from_proc_kallsyms("kimage_voffset") != BADVAL))) {
> @@ -185,11 +181,8 @@ arm64_init(int when)
>                 if (kernel_symbol_exists("kimage_voffset"))
>                         machdep->flags |= NEW_VMEMMAP;
>
> -               if (!machdep->pagesize &&
> -                   (string = pc->read_vmcoreinfo("PAGESIZE"))) {
> -                       machdep->pagesize = atoi(string);
> -                       free(string);
> -               }
> +               if (!machdep->pagesize)
> + arm64_get_vmcoreinfo_ul((ulong*)&machdep->pagesize, "PAGESIZE", 
> NUM_DEC);
>
> Furthermore, this way can be extended to other arches, but it can be
> done in another patch, make it become generic code. Anyway, you might
> need to try it.

Okay, I will keep the changes in the arm64 code, and do not change the 
generic code.


> > > 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
> arm64_get_vmcoreinfo_ul(&value, "NUMBER(XX)", NUM_DEC);
>
>
> > But some ARCHs , such as arch/arm64/kernel/crash_core.c
> >
> >      NUMBER(XX)=%lx
> arm64_get_vmcoreinfo_ul(&value, "NUMBER(XX)", NUM_HEX);
>
> As I mentioned, we can know if the string is a decimal string or
> hexadecimal string according to the kernel code.
Got it.

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