10:53 p.m.
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(a)os.amperecomputing.com>
>>> ---
>>> 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