[Crash-utility] [PATCH 01/16] Add MIPS64 framework code support
HAGIO KAZUHITO(萩尾 一仁)
k-hagio-ab at nec.com
Mon Mar 15 05:41:07 UTC 2021
-----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.
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