Re: [Crash-utility] [PATCH] arm64 : assign page_offset and kvbase based on VA_BITS passed

Hi Lianbo, -----Original Message----- > On Mon, Oct 18, 2021 at 3:35 PM HAGIO KAZUHITO(萩尾　一仁) > <k-hagio-ab(a)nec.com&gt; wrote: > > > > -----Original Message----- > > > On Thu, Oct 14, 2021 at 11:17 AM HAGIO KAZUHITO(萩尾　一仁) > > > <k-hagio-ab(a)nec.com&gt; wrote: > > > > > > > > Hi Ankur, > > > > > > > > -----Original Message----- > > > > > >> assign page_offset and kvbase based on VA_BITS passed > > > > > >> > > > > > > > > > > >Thank you for the patch, Ankur. > > > > > >Can you help to describe the reason in detail? And what happened? > > > > > > > > > > > > > > > > > > > > Hi Lianbo, Raw ramdump without vmcoreinfo doesn't work here. > > > > > read_vmcoreinfo("NUMBER(BA_BITS)") will not work on raw ramdump. > > > > > getting below error > > > > > > > > > > > Thank you for the information, Ankur. > > > > > > > > crash_64: vmlinux and /var/tmp/ramdump_elf_XUtCMT do not match! > > > > > Usage: > > > > > crash [OPTION]... NAMELIST MEMORY-IMAGE[@address <https://github.com/address> ] (dumpfile form) > > > > > crash [OPTION]... [NAMELIST] (live system form) > > > > > Enter "crash_64 -h" for details. > > > > > > > > > > Recent change > https://github.com/crash-utility/crash/commit/167d37e347fe35c6f7db826e853... > > > is > > > > > causing this issue, before this change VA_BITS_ACTUAL was used which is passed from cmd line. > > > > > > > > ok, I see, but I'm not sure why your patch uses the (pc->flags2 & SNAP) condition. > > > > > > This seems not very reasonable, the SNAP flag has a specific use. > > > > > > > The calculation used VA_BITS_ACTUAL (== VA_BITS for ramdump) before the change, > > > > so how about the following patch? > > > >./crash > > > > diff --git a/arm64.c b/arm64.c > > > > index 7069312671cf..da78acbb013f 100644 > > > > --- a/arm64.c > > > > +++ b/arm64.c > > > > @@ -4031,6 +4031,9 @@ arm64_calc_VA_BITS(void) > > > > error(FATAL, "cannot determine VA_BITS_ACTUAL\n"); > > > > } > > > > > > > > + if (!machdep->machspec->CONFIG_ARM64_VA_BITS) /* guess */ > > > > + machdep->machspec->CONFIG_ARM64_VA_BITS = machedp->machspec->VA_BITS; > > > > + > > > > > > Looks good. > > > > Thanks, but I'm rethinking about their meanings. > > > > The VA_BITS is a macro replaced with CONFIG_ARM64_VA_BITS in the kernel > > and used to calculate PAGE_OFFSET. > > > > #define VA_BITS (CONFIG_ARM64_VA_BITS) > > #define _PAGE_OFFSET(va) (-(UL(1) << (va))) > > #define PAGE_OFFSET (_PAGE_OFFSET(VA_BITS)) > > > > It looks like we should set ms->CONFIG_ARM64_VA_BITS only for the value > > got from the kernel config or vmcoreinfo. On the other hand, ms->VA_BITS > > can be calculated from other information in crash. > > > Sounds good to me. > > > So should we use ms->VA_BITS for ARM64_FLIP_PAGE_OFFSET like this? > > > > diff --git a/arm64.c b/arm64.c > > index 7069312671cf..f33b6c28a3bc 100644 > > --- a/arm64.c > > +++ b/arm64.c > > @@ -4031,6 +4031,9 @@ arm64_calc_VA_BITS(void) > > error(FATAL, "cannot determine VA_BITS_ACTUAL\n"); > > } > > > > + if (!machdep->machspec->CONFIG_ARM64_VA_BITS) > > + machdep->machspec->VA_BITS = machdep->machspec->CONFIG_ARM64_VA_BITS; > > If the above judgment condition is true, the value of VA_BITS is > always assigned to zero? Could you please describe more details? Ugh, sorry my bad. I actually meant: if (machdep->machspec->CONFIG_ARM64_VA_BITS) machdep->machspec->VA_BITS = machdep->machspec->CONFIG_ARM64_VA_BITS; By this, ARM64_FLIP_PAGE_OFFSET with VA_BITS uses CONFIG_ARM64_VA_BITS if vmcoreinfo is available. If it's not available, ARM64_FLIP_PAGE_OFFSET uses VA_BITS guessed as equal to VA_BITS_ACTUAL.

And I found that ARM64_FLIP_PAGE_OFFSET_ACTUAL is not used actually. I think we can remove it. Will send a patch for testing later. Thanks, Kazu > > Thanks. > Lianbo > > + > > /* > > * The mm flip commit is introduced before 52-bits VA, which is before the > > * commit to export NUMBER(TCR_EL1_T1SZ) > > diff --git a/defs.h b/defs.h > > index cbd45e52f9da..c4438b357259 100644 > > --- a/defs.h > > +++ b/defs.h > > @@ -3239,7 +3239,7 @@ typedef signed int s32; > > #define ARM64_PAGE_OFFSET ((0xffffffffffffffffUL) \ > > << (machdep->machspec->VA_BITS - 1)) > > /* kernels >= v5.4 the kernel VA space is flipped */ > > -#define ARM64_FLIP_PAGE_OFFSET (-(1UL) << machdep->machspec->CONFIG_ARM64_VA_BITS) > > +#define ARM64_FLIP_PAGE_OFFSET (-(1UL) << machdep->machspec->VA_BITS) > > #define ARM64_FLIP_PAGE_OFFSET_ACTUAL ((0xffffffffffffffffUL) \ > > - ((1UL) << machdep->machspec->VA_BITS_ACTUAL) + 1) > > > > > > > > > > > /* > > > > * The mm flip commit is introduced before 52-bits VA, which is before the > > > > * commit to export NUMBER(TCR_EL1_T1SZ) > > > > > > > > > > BTW: Can we also try to get its value from kernel config? But this > > > relies on kernel config: CONFIG_IKCONFIG. > > > > The IKCONFIG_AVAIL flag is set by IKCFG_INIT here: > > > > read_in_kernel_config(IKCFG_INIT); > > kernel_init(); > > machdep_init(POST_GDB); > > > > The get_kernel_config() cannot be used in machdep_init(PRE_GDB). > > > > Thanks, > > Kazu > > > > > > > > > > diff --git a/arm64.c b/arm64.c > > > index 7069312..79f9929 100644 > > > --- a/arm64.c > > > +++ b/arm64.c > > > @@ -3989,6 +3989,10 @@ arm64_calc_VA_BITS(void) > > > value = atol(string); > > > free(string); > > > machdep->machspec->CONFIG_ARM64_VA_BITS = value; > > > + } else if (kt->ikconfig_flags & IKCONFIG_AVAIL) { > > > + if ((get_kernel_config("CONFIG_ARM64_VA_BITS", > > > + &string)) == IKCONFIG_STR) > > > + machdep->machspec->CONFIG_ARM64_VA_BITS = atol(string); > > > } > > > > > > if (kernel_symbol_exists("vabits_actual")) { > > > > > > Thanks. > > > Lianbo > > > > > 3987 static void > > > > > 3988 arm64_calc_VA_BITS(void) > > > > > 3989 { > > > > > 3990 int bitval; > > > > > 3991 struct syment *sp; > > > > > 3992 ulong vabits_actual, value; > > > > > 3993 char *string; > > > > > 3994 > > > > > 3995 if ((string = pc->read_vmcoreinfo("NUMBER(VA_BITS)"))) { > > > > > 3996 value = atol(string); > > > > > 3997 free(string); > > > > > 3998 machdep->machspec->CONFIG_ARM64_VA_BITS = value; > > > > > 3999 } > > > > > > > > > > > > > > > > > > > > > > > > > >Thanks. > > > > > >Lianbo > > > > > > > > > > >> Change-Id: I525f3c7fd91e1f06e909c2f4c1749c44c068baea > > > > > >> Signed-off-by: Ankur Bansal <er.ankurbansal(a)gmail.com <mailto:er.ankurbansal@gmail.com> > > > > > > > >> --- > > > > > >> arm64.c | 15 +++++++++++---- > > > > > >> 1 file changed, 11 insertions(+), 4 deletions(-) > > > > > >> > > > > > >> diff --git a/arm64.c b/arm64.c > > > > > >> index 7069312..2dc77f7 100644 > > > > > >> --- a/arm64.c > > > > > >> +++ b/arm64.c > > > > > >> @@ -220,10 +220,17 @@ arm64_init(int when) > > > > > >> > > > > > >> /* vabits_actual introduced after mm flip, so it should be flipped layout > */ > > > > > >> if (ms->VA_BITS_ACTUAL) { > > > > > >> - ms->page_offset = ARM64_FLIP_PAGE_OFFSET; > > > > > >> - /* useless on arm64 */ > > > > > >> - machdep->identity_map_base = ARM64_FLIP_PAGE_OFFSET; > > > > > >> - machdep->kvbase = ARM64_FLIP_PAGE_OFFSET; > > > > > >> + if ((pc->flags2 & SNAP)) { > > > > > >> + ms->page_offset = ARM64_FLIP_PAGE_OFFSET; > > > > > >> + /* useless on arm64 */ > > > > > >> + machdep->identity_map_base = ARM64_FLIP_PAGE_OFFSET; > > > > > >> + machdep->kvbase = ARM64_FLIP_PAGE_OFFSET; > > > > > >> + } > > > > > >> + else{ > > > > > >> + ms->page_offset = ARM64_FLIP_PAGE_OFFSET_ACTUAL; > > > > > >> + machdep->identity_map_base = > ARM64_FLIP_PAGE_OFFSET_ACTUAL; > > > > > >> + machdep->kvbase = ARM64_FLIP_PAGE_OFFSET_ACTUAL; > > > > > >> + } > > > > > >> ms->userspace_top = ARM64_USERSPACE_TOP_ACTUAL; > > > > > >> } else { > > > > > >> ms->page_offset = ARM64_PAGE_OFFSET; > > > > > >> -- > > > > > >> 2.7.4 > > > > > > > > > > > > > > > > > > > > ------------------------------ > > > > > > > > > > -- > > > > > Crash-utility mailing list > > > > > Crash-utility(a)redhat.com <mailto:Crash-utility@redhat.com> > > > > > https://listman.redhat.com/mailman/listinfo/crash-utility > > > > > > > > > > End of Crash-utility Digest, Vol 192, Issue 28 > > > > > ********************************************** > > > > > > > > > > > > > > > > > > >