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

[Lianbo Jiang <lijiang@xxxxxxxxxx>]

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.


> 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.

> > 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.


Thanks.

Lianbo

> 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