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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux