[Crash-utility] [PATCH 01/16] Add MIPS64 framework code support
Youling Tang
tangyouling at loongson.cn
Mon Mar 15 06:16:46 UTC 2021
On 03/15/2021 01:41 PM, HAGIO KAZUHITO(萩尾 一仁) wrote:
> -----Original Message-----
>> On 03/12/2021 04:36 PM, HAGIO KAZUHITO(萩尾 一仁) wrote:
>>> -----Original Message-----
>>>> -----Original Message-----
>>>>> Hi Kazu,
>>>>>
>>>>> On 03/12/2021 12:21 PM, HAGIO KAZUHITO(萩尾 一仁) wrote:
>>>>>
>>>>>> -----Original Message-----
>>>>>>>>> --- a/diskdump.c
>>>>>>>>> +++ b/diskdump.c
>>>>>>>>> @@ -594,6 +594,9 @@ restart:
>>>>>>>>> else if (STRNEQ(header->utsname.machine, "mips") &&
>>>>>>>>> machine_type_mismatch(file, "MIPS", NULL, 0))
>>>>>>>>> goto err;
>>>>>>>>> + else if (STRNEQ(header->utsname.machine, "mips64") &&
>>>>>>>>> + machine_type_mismatch(file, "MIPS", NULL, 0))
>>>>>>>>> + goto err;
>>>>>>>> Why do you make MACHINE_TYPE the same as the MIPS one?
>>>>>>>> With this, doesn't a MIPS64 crash match a MIPS vmcore?
>>>>>>> The value of the machine type e_machine in mips32 or mips64 is MIPS, which
>>>>>>> corresponds to EM_MIPS.
>>>>>>> The definition in gdb-7.6/include/elf/common.h:110 is as follows:
>>>>>>> #define EM_MIPS 8 /* MIPS R3000 */
>>>>>>> But there is no related definition of EM_MIPS64 or other mips64, so both
>>>>>>> mips32 and mips64 should use EM_MIPS, and the corresponding e_machine is
>>>>>>> MIPS.
>>>>>>>
>>>>>>> If the MACHINE_TYPE of mips64 is defined as MIPS64, as follows:
>>>>>>> define MACHINE_TYPE "MIPS64"
>>>>>>>
>>>>>>> The following error will appear when running crash:
>>>>>>> WARNING: machine type mismatch:
>>>>>>> crash utility: MIPS64
>>>>>>> vmcore: MIPS
>>>>>> Then, is there any problem with this?
>>>>>> machine_type_mismatch(file, "MIPS64", NULL, 0))
>>>>>>
>>>>>> This can prevent a mips64 crash from trying to open a mips32 vmcore
>>>>>> and the reverse.
>>>>> Read the ELF header information of vmocre (whether it is mips32 or mips64),
>>>>> and the obtained machine type is MIPS, not MIPS64. In mips64, if you use
>>>>> machine_type_mismatch(file, "MIPS64", NULL, 0)), there will be a machine
>>>>> type
>>>>> mismatch. You can only use machine_type_mismatch(file, "MIPS", NULL, 0)),
>>>>> only then It can be successfully matched to the machine type.
>>>> Probably I misunderstand something, but then need to know it..
>>>>
>>>> The machine_type_mismatch() doesn't use the ELF's EM_MIPS value.
>>>> Why do we need to match crash's MACHINE_TYPE with ELF's EM_MIPS?
>>>>
>>>> int
>>>> machine_type(char *type)
>>>> {
>>>> return STREQ(MACHINE_TYPE, type);
>>>> }
>>>>
>>>> int
>>>> machine_type_mismatch(char *file, char *e_machine, char *alt, ulong query)
>>>> {
>>>> if (machine_type(e_machine) || machine_type(alt))
>>>> return FALSE;
>>>>
>>>> if (query == KDUMP_LOCAL) /* already printed by NETDUMP_LOCAL */
>>>> return TRUE;
>>>>
>>>> error(WARNING, "machine type mismatch:\n");
>>>>
>>>> fprintf(fp, " crash utility: %s\n", MACHINE_TYPE);
>>>> fprintf(fp, " %s: %s%s%s\n\n", file, e_machine,
>>>> alt ? " or " : "", alt ? alt : "");
>>>>
>>>> return TRUE;
>>>> }
>>>>
>>>> Without the following hunk, EM_MIPS has been used only for ELF32 in symbols.c.
>>>>
>>>> --- a/symbols.c
>>>> +++ b/symbols.c
>>>> @@ -3636,6 +3636,11 @@ is_kernel(char *file)
>>>> goto bailout;
>>>> break;
>>>>
>>>> + case EM_MIPS:
>>>> + if (machine_type_mismatch(file, "MIPS", NULL, 0))
>>>> + goto bailout;
>>>> + break;
>>>> +
>>>> default:
>>>> if (machine_type_mismatch(file, "(unknown)", NULL, 0))
>>>> goto bailout;
>>>> @@ -3890,6 +3895,11 @@ is_shared_object(char *file)
>>>> if (machine_type("SPARC64"))
>>>> return TRUE;
>>>> break;
>>>> +
>>>> + case EM_MIPS:
>>>> + if (machine_type("MIPS"))
>>>> + return TRUE;
>>>> + break;
>>>> }
>>>>
>>>> So I thought that meybe we can also change these ELF64 cases to "MIPS64"
>>>> with defining MACHINE_TYPE as "MIPS64".
>>>>
>>>> This is wrong? e.g. mips32 also can use an ELF64 kernel/module binary?
>>> If there is such a case, maybe we can also use these?:
>>> machine_type_mismatch(file, "MIPS", "MIPS64", 0)
>>> machine_type("MIPS") || machine_type("MIPS64")
>>>
>>> I'm thinking that as we define the values/functions for mips64 separately
>>> from mips32, it would be reasonable to make crash's MACHINE_TYPE different
>>> from mips32 as well, and we can remove some unusual ifdefs.
>> Based on the original patch, make modifications similar to the following:
>>
>> diff --git a/defs.h b/defs.h
>> index dcc58b8..e1aa0fa 100644
>> --- a/defs.h
>> +++ b/defs.h
>> @@ -6376,7 +6376,7 @@ void s390x_dump_machdep_table(ulong);
>> #endif
>>
>> #ifdef __mips__ /* MIPS & MIPS64 */
>> -#define MACHINE_TYPE "MIPS"
>> +#define MACHINE_TYPE "MIPS64"
>> #endif
>>
>> /*
>> diff --git a/diskdump.c b/diskdump.c
>> index b4712ad..9e0fd89 100644
>> --- a/diskdump.c
>> +++ b/diskdump.c
>> @@ -595,7 +595,7 @@ restart:
>> machine_type_mismatch(file, "MIPS", NULL, 0))
>> goto err;
>> else if (STRNEQ(header->utsname.machine, "mips64") &&
>> - machine_type_mismatch(file, "MIPS", NULL, 0))
>> + machine_type_mismatch(file, "MIPS", "MIPS64", 0))
> Is this "MIPS" (mips32) needed?
> If a mips32 crash doesn't need to open a "mips64" diskdump,
> please drop it.
>
>> goto err;
>> else if (STRNEQ(header->utsname.machine, "s390x") &&
>> machine_type_mismatch(file, "S390X", NULL, 0))
>> @@ -754,7 +754,7 @@ restart:
>>
>> if (machine_type("ARM"))
>> dd->machine_type = EM_ARM;
>> - else if (machine_type("MIPS"))
>> + else if (machine_type("MIPS") || machine_type("MIPS64"))
>> dd->machine_type = EM_MIPS;
> Yes. I expected this.
>
>> else if (machine_type("X86"))
>> dd->machine_type = EM_386;
>> diff --git a/netdump.c b/netdump.c
>> index 2ca39e2..ef2f241 100644
>> --- a/netdump.c
>> +++ b/netdump.c
>> @@ -290,7 +290,7 @@ is_netdump(char *file, ulong source_query)
>> break;
>>
>> case EM_MIPS:
>> - if (machine_type_mismatch(file, "MIPS", NULL,
>> + if (machine_type_mismatch(file, "MIPS", "MIPS64",
>> source_query))
> Yes, apparently this cannot be avoidable.
>
>> goto bailout;
>> break;
>> diff --git a/symbols.c b/symbols.c
>> index b30b92c..97b3bdc 100644
>> --- a/symbols.c
>> +++ b/symbols.c
>> @@ -3634,7 +3634,7 @@ is_kernel(char *file)
>> break;
>>
>> case EM_MIPS:
>> - if (machine_type_mismatch(file, "MIPS", NULL, 0))
>> + if (machine_type_mismatch(file, "MIPS", "MIPS64", 0))
> This case statement was added by you to support mips64.
> Is this mips32 ("MIPS") needed?
>
>> goto bailout;
>> break;
>>
>> @@ -3894,7 +3894,7 @@ is_shared_object(char *file)
>> break;
>>
>> case EM_MIPS:
>> - if (machine_type("MIPS"))
>> + if (machine_type("MIPS") || machine_type("MIPS64"))
> Same as above.
>
>> return TRUE;
>> break;
>> }
>>
>> After this modification, it can run successfully.
>>
>> I personally think this scheme is feasible.
>> The MACHINE_TYPE of mips64 is defined as MIPS64, but EM_MIPS is still used
>> in the case, but the processing part is as above. This can be distinguished
>> from mips32 and use a different MACHINE_TYPE.
>>
>> Do you think this modification is feasible and look forward to your
>> suggestions?
> Yes, I still have a few questions as above, but I think a modification like this
> would be feasible.
>
> And one more thing, please add "mips64" to supported kernels in the README file
> and README data in help.c within this patchset or later.
Ok, I will support mips64 architecture information added to README and
help.c files.
Thanks,
Youling
> Thanks,
> Kazu
>
>>> Thanks,
>>> Kazu
>> Thanks,
>> Youling
>>>>>>> # readelf -h vmcore
>>>>>>> ...
>>>>>>> Type: CORE (Core file)
>>>>>>> Machine: MIPS R3000
>>>>>>> ...
>>>>>>>
>>>>>>> Therefore, the MACHINE_TYPE of mips32 and mips64 both define MIPS.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Youling
>>>> --
>>>> Crash-utility mailing list
>>>> Crash-utility at redhat.com
>>>> https://listman.redhat.com/mailman/listinfo/crash-utility
More information about the Crash-utility
mailing list