[Crash-utility] ramdump support for va_bits_actual

[Dave Anderson]

----- Original Message -----
> Hi Dave,
> 
> On Sat, Apr 18, 2020 at 2:08 PM Dave Anderson <anderson at redhat.com> wrote:
> >
> >
> >
> > ----- Original Message -----
> > > Hi Dave,
> > >
> > > Noticed that raw ramdumps of 5.4 kernel aren't working with crash tip.
> > > With the patches attached, I could get it working. Please take a look.
> > >
> > > Thanks,
> > > Vinayak
> > >
> >
> > Hi Vinayak,
> >
> > A couple quick questions come to mind...
> >
> > First, I haven't checked all possible READMEM plugins, but for example, if
> > this
> > function is run on a live system, the -1 file descriptor would cause the
> > READMEM()
> > call to fail:
> 
> 
> I changed it like this and it works for ramdump. I don't actually have
> a live setup to try this. Let me try
> to set up one.
> 
> diff --git a/arm64.c b/arm64.c
> index 04efc13..fce3f8e 100644
> --- a/arm64.c
> +++ b/arm64.c
> @@ -981,7 +981,7 @@ arm64_calc_physvirt_offset(void)
> 
>         if ((sp = kernel_symbol_search("physvirt_offset")) &&
>                         machdep->machspec->kimage_voffset) {
> -               if (READMEM(-1, &physvirt_offset, sizeof(physvirt_offset),
> +               if (READMEM(pc->mfd, &physvirt_offset, sizeof(physvirt_offset),
>                         sp->value, sp->value -
>                         machdep->machspec->kimage_voffset) > 0) {
>                                 ms->physvirt_offset = physvirt_offset;
> 
> 
> >
> >  static void
> > +arm64_calc_physvirt_offset(void)
> > +{
> > +       struct machine_specific *ms = machdep->machspec;
> > +       ulong physvirt_offset;
> > +       struct syment *sp;
> > +
> > +       ms->physvirt_offset = ms->phys_offset - ms->page_offset;
> > +
> > +       if ((sp = kernel_symbol_search("physvirt_offset")) &&
> > +                       machdep->machspec->kimage_voffset) {
> > +               if (READMEM(-1, &physvirt_offset, sizeof(physvirt_offset),
> > +                       sp->value, sp->value -
> > +                       machdep->machspec->kimage_voffset) > 0) {
> > +                               ms->physvirt_offset = physvirt_offset;
> > +               }
> > +       }
> > +
> > +       if (CRASHDEBUG(1))
> > +               fprintf(fp, "using %lx as physvirt_offset\n", ms->physvirt_offset);
> > +}
> >
> > And here -- are you missing some brackets?  (run "make warn")
> >
> 
> I did try "make warn" and it does not show any issues.Am I missing something?

I saw on a system provisioned with Fedora's latest and greatest gcc version.
I don't have the system available any more, but the warning message picked up
on the fact that your second if statement "was not guarded" by the if statement
above it.

> 
> > But regardless of that, why are you setting it back to 48 if it's greater
> > than 48?
> >
> 
> 
> I did that because machspec->CONFIG_ARM64_VA_BITS is used for calculation of
> vmemmap size. In kernel vmemmap size is calculated using VA_BITS_MIN and it is
> defined like this
> 
> #if VA_BITS > 48
> #define VA_BITS_MIN             (48)
> #else
> #define VA_BITS_MIN             (VA_BITS)
> #endif
> 
> But I realize now that its not the right thing to do, because machspec->CONFIG_ARM64_VA_BITS
> is later used in arm64_calc_VA_BITS to verify machspec->VA_BITS. So
> what about this ?
> 
> diff --git a/arm64.c b/arm64.c
> index 04efc13..a35a30e 100644
> --- a/arm64.c
> +++ b/arm64.c
> @@ -4023,8 +4023,6 @@ arm64_calc_virtual_memory_ranges(void)
>                         if ((ret = get_kernel_config("CONFIG_ARM64_VA_BITS",
>                                         &string)) == IKCONFIG_STR)
>                                 machdep->machspec->CONFIG_ARM64_VA_BITS = atol(string);
> -                               if (machdep->machspec->CONFIG_ARM64_VA_BITS > 48)
> -                                        machdep->machspec->CONFIG_ARM64_VA_BITS = 48;
>                 }
>         }
> 
> @@ -4049,7 +4047,12 @@ arm64_calc_virtual_memory_ranges(void)
>  #define STRUCT_PAGE_MAX_SHIFT   6
> 
>         if (ms->VA_BITS_ACTUAL) {
> -               vmemmap_size = (1UL) << (ms->CONFIG_ARM64_VA_BITS - machdep->pageshift - 1 + STRUCT_PAGE_MAX_SHIFT);
> +               ulong va_bits_min = 48;
> +
> +               if (machdep->machspec->CONFIG_ARM64_VA_BITS < 48)
> +                       va_bits_min = ms->CONFIG_ARM64_VA_BITS;
> +
> +               vmemmap_size = (1UL) << (va_bits_min - machdep->pageshift - 1 + STRUCT_PAGE_MAX_SHIFT);
>                 vmalloc_end = (- PUD_SIZE - vmemmap_size - KILOBYTES(64));
>                 vmemmap_start = (-vmemmap_size);
>                 ms->vmalloc_end = vmalloc_end - 1;
> 

Yeah, that looks reasonable.  But what about the parallel discussion re: vmemmap_start?

  https://www.redhat.com/archives/crash-utility/2020-April/msg00064.html

Can you send in an updated patch set with all fixes applied?

Thanks,
  Dave



 


Shouldn't it be