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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



-----Original Message-----
> On Thu, Oct 14, 2021 at 11:17 AM HAGIO KAZUHITO(萩尾 一仁)
> <k-hagio-ab@xxxxxxx> 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/167d37e347fe35c6f7db826e8539e192c4375564
> 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.

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;
+
                /*
                 * 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@xxxxxxxxx <mailto:er.ankurbansal@xxxxxxxxx> >
> > >       >> ---
> > >       >> 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@xxxxxxxxxx <mailto:Crash-utility@xxxxxxxxxx>
> > >       https://listman.redhat.com/mailman/listinfo/crash-utility
> > >
> > >       End of Crash-utility Digest, Vol 192, Issue 28
> > >       **********************************************
> > >
> > >
> > >
> >

--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility




[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux