Re: [Crash-utility] [PATCH v2] arm64: fix kernel memory map handling for kaslr-enabled kernel

On 27/05/2016:12:18:56 PM, AKASHI Takahiro wrote: > On Thu, May 26, 2016 at 01:27:08PM +0530, Pratyush Anand wrote: > > On 26/05/2016:04:04:08 PM, AKASHI Takahiro wrote: > > > Pratyush, > > > > > > Just for debug purpose. > > > Please add the following line to *your* arch_crash_save_vmcoreinfo(): > > > > vmcoreinfo_append_str("NUMBER(kimage_voffset)=%llx\n", kimage_voffset); > > > > Thanks for the pointer. > > > > I did had VMCOREINFO_NUMBER(kimage_voffset) in arch_crash_save_vmcoreinfo(). > > > > https://github.com/pratyushanand/linux/commit/7011e478aae3e568cc8dca15b6c... > > > > But, I noticed that in crash code you have "ms->kimage_voffset = htol(string, > > QUIET, NULL);". So, the change you have suggested will work. > > I know that. It is intentional. > > > However, I think its preferable to use VMCOREINFO_NUMBER() macro instead. > > makedumpfile is able to calculate kimage_voffset correctly with that without any > > issue. > > I think that VMCOREINFO_NUMBER() is, at least originally, intended > to be used for a small *unsigned* integer. Not sure..When I see all the places where it has been used, it seems it can accommodate *long* variable.

See, read_vmcoreinfo_long() of makedumpfile [1] , which is being used to read "NUMBER" string. This function uses strtol(). So I think, we must have similar implementation if we intend to use VMCOREINFO_NUMBER(). [1] https://sourceforge.net/p/makedumpfile/code/ci/master/tree/makedumpfile.c... > > I also know that PHYS_OFFSET can now be nagative in v4.6 on arm64. > Yet I'm thinking of adding "0x" as a prefix to VMCOREINOF string. I think, we should not change existing VMCOREINFO_xxxx() macros of kernel, because it will break the applications which rely on it's current structure.

ATM, I also do not see any urgency of defining a new macro which can have implementation like vmcoreinfo_append_str("NUMBER(X)=%#llx\n", X), because exiting macros is able to take care of the needs. > > -Takahiro AKASHI > > > I will suggest to take following modification in crash code: > > > > diff --git a/arm64.c b/arm64.c > > index 6b97093..9397d6d 100644 > > --- a/arm64.c > > +++ b/arm64.c > > @@ -122,7 +122,7 @@ arm64_init(int when) > > ms = machdep->machspec; > > if (!ms->kimage_voffset && > > (string = pc->read_vmcoreinfo("NUMBER(kimage_voffset)"))) { > > - ms->kimage_voffset = htol(string, QUIET, NULL); > > + ms->kimage_voffset = dtol(string, QUIET, NULL); > > free(string); > > } > > > > diff --git a/tools.c b/tools.c > > index 384bebd..1383e43 100644 > > --- a/tools.c > > +++ b/tools.c > > @@ -877,7 +877,7 @@ dtol(char *s, int flags, int *errptr) > > s++; > > > > for (j = 0; s[j] != '\0'; j++) > > - if ((s[j] < '0' || s[j] > '9')) > > + if ( (s[j] != '-') && ((s[j] < '0' || s[j] > '9'))) > > break ; > > > > if (s[j] != '\0') { > > ~Pratyush