Re: [Crash-utility] [PATCHv8 2/4] crash-utility/arm64: store phy_offset and memstart_addr separately

-----Original Message----- > Crash encounters a bug like the following: > ... > SECTION_SIZE_BITS: 30 > CONFIG_ARM64_VA_BITS: 52 > VA_BITS_ACTUAL: 48 > (calculated) VA_BITS: 48 > PAGE_OFFSET: ffff000000000000 > VA_START: ffff800000000000 > modules: ffff800008000000 - ffff80000fffffff > vmalloc: ffff800010000000 - ffffffdfdffeffff > kernel image: ffff800010000000 - ffff800012750000 > vmemmap: ffffffdfffe00000 - ffffffffffffffff > > <readmem: ffff800011c53bc8, KVADDR, "nr_irqs", 4, (FOE), b47bdc> > <read_kdump: addr: ffff800011c53bc8 paddr: eb453bc8 cnt: 4> > read_netdump: addr: ffff800011c53bc8 paddr: eb453bc8 cnt: 4 offset: 1c73bc8 > irq_stack_ptr: > type: 1, TYPE_CODE_PTR > target_typecode: 8, TYPE_CODE_INT > target_length: 8 > length: 8 > GNU_GET_DATATYPE[thread_union]: returned via gdb_error_hook > <readmem: ffff000b779c0050, KVADDR, "IRQ stack pointer", 8, (ROE), 3a37bea0> > <read_kdump: addr: ffff000b779c0050 paddr: fff1000bf79c0050 cnt: 8> > read_netdump: READ_ERROR: offset not found for paddr: fff1000bf79c0050 > crash: read error: kernel virtual address: ffff000b779c0050 type: "IRQ stack pointer" > <readmem: ffff000b77a60050, KVADDR, "IRQ stack pointer", 8, (ROE), 3a37bea8> > <read_kdump: addr: ffff000b77a60050 paddr: fff1000bf7a60050 cnt: 8> > read_netdump: READ_ERROR: offset not found for paddr: fff1000bf7a60050 > ... > > Apparently, for a normal system, the 'paddr: fff1000bf79c0050' is > unreasonable. > > This bug connects with kernel commit 7bc1a0f9e176 ("arm64: mm: use > single quantity to represent the PA to VA translation"), memstart_addr > can be negative, which makes it different from real phys_offset. If > using memstart_addr to calculate the real paddr, the unreasonable paddr > will be got. > > Furthermore, in crash utility, PTOV() needs memstart_addr to calculate > VA from PA, while getting PFN offset in a dumpfile, phys_offset is > required. > > To serve the different purpose, using phys_offset_nominal and > phys_offset to store them. > > Signed-off-by: Pingfan Liu <piliu(a)redhat.com&gt; > Cc: HAGIO KAZUHITO <k-hagio-ab(a)nec.com&gt; > Cc: Lianbo Jiang <lijiang(a)redhat.com&gt; > Cc: Bhupesh Sharma <bhupesh.sharma(a)linaro.org&gt; > To: crash-utility(a)redhat.com > --- > arm64.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++---- > defs.h | 17 ++++++++++---- > 2 files changed, 78 insertions(+), 10 deletions(-) > > diff --git a/arm64.c b/arm64.c > index 98138b2..b3b3242 100644 > --- a/arm64.c > +++ b/arm64.c > @@ -23,6 +23,10 @@ > #include <sys/ioctl.h> > > #define NOT_IMPLEMENTED(X) error((X), "%s: function not implemented\n", __func__) > +/* > + * _PAGE_OFFSET() refers to arch/arm64/include/asm/memory.h > + */ > +#define _PAGE_OFFSET(va) (-1UL << (va)) > > static struct machine_specific arm64_machine_specific = { 0 }; > static int arm64_verify_symbol(const char *, ulong, char); > @@ -691,6 +695,7 @@ arm64_dump_machdep_table(ulong arg) > fprintf(fp, " kimage_voffset: %016lx\n", ms->kimage_voffset); > } > fprintf(fp, " phys_offset: %lx\n", ms->phys_offset); > + fprintf(fp, " phys_offset_nominal: %lx\n", ms->phys_offset_nominal); > fprintf(fp, "__exception_text_start: %lx\n", ms->__exception_text_start); > fprintf(fp, " __exception_text_end: %lx\n", ms->__exception_text_end); > fprintf(fp, " __irqentry_text_start: %lx\n", ms->__irqentry_text_start); > @@ -991,7 +996,17 @@ arm64_calc_physvirt_offset(void) > ulong physvirt_offset; > struct syment *sp; > > - ms->physvirt_offset = ms->phys_offset - ms->page_offset; > + /* if flipped but having 'physvirt_offset', ms->physvirt_offset is overwritten in this func */ > + if (machdep->flags & FLIPPED_VM) { > + /* > + * source arch/arm64/include/asm/memory.h > + * #define __lm_to_phys(addr) (((addr) & ~PAGE_OFFSET) + PHYS_OFFSET) > + * the part "addr & ~PAGE_OFFSET" is done in arm64_VTOP() > + */ > + ms->physvirt_offset = ms->phys_offset_nominal; > + } else { > + ms->physvirt_offset = ms->phys_offset - ms->page_offset; > + } Can this be moved after the following "if ((sp = kernel_symbol_search(..." block? I think it's more natural and readable than the overwriting. i.e.: if ((sp = kernel_symbol_search("physvirt_offset")) && machdep->machspec->kimage_voffset) { if (READMEM(...) > 0) { ... return; } } if (machdep->flags & FLIPPED_VM) { ...

> > if ((sp = kernel_symbol_search("physvirt_offset")) && > machdep->machspec->kimage_voffset) { > @@ -1007,6 +1022,8 @@ arm64_calc_physvirt_offset(void) > static void > arm64_calc_phys_offset(void) > { > +#define MEMSTART_ADDR_OFFSET _PAGE_OFFSET(48) - _PAGE_OFFSET(52) > + > struct machine_specific *ms = machdep->machspec; > ulong phys_offset; > > @@ -1033,7 +1050,11 @@ arm64_calc_phys_offset(void) > ms->kimage_voffset && (sp = kernel_symbol_search("memstart_addr"))) { > if (pc->flags & PROC_KCORE) { > if ((string = pc->read_vmcoreinfo("NUMBER(PHYS_OFFSET)"))) { > - ms->phys_offset = htol(string, QUIET, NULL); > + ms->phys_offset_nominal = htol(string, QUIET, NULL); > + if (ms->phys_offset_nominal < 0) > + ms->phys_offset = ms->phys_offset_nominal + > MEMSTART_ADDR_OFFSET; > + else > + ms->phys_offset = ms->phys_offset_nominal; > free(string); > return; > } If it's /dev/mem (not /proc/kcore), the memstart_addr value is still negative? It's no problem?

> @@ -1085,7 +1106,18 @@ arm64_calc_phys_offset(void) > } else if (DISKDUMP_DUMPFILE() && diskdump_phys_base(&phys_offset)) { > ms->phys_offset = phys_offset; makedumpfile also set kdump_sub_header.phys_base to NUMBER(PHYS_OFFSET) ?

> } else if (KDUMP_DUMPFILE() && arm64_kdump_phys_base(&phys_offset)) { > - ms->phys_offset = phys_offset; > + /* > + * When running a 52bits kernel on 48bits hardware. Kernel plays a trick: > + * if (IS_ENABLED(CONFIG_ARM64_VA_BITS_52) && (vabits_actual != 52)) > + * memstart_addr -= _PAGE_OFFSET(48) - _PAGE_OFFSET(52); > + * > + * In crash, this should be detected to get a real physical start address. > + */ > + ms->phys_offset_nominal = phys_offset; > + if ((long)phys_offset < 0) > + ms->phys_offset = phys_offset + MEMSTART_ADDR_OFFSET; > + else > + ms->phys_offset = phys_offset; > } else { > error(WARNING, > "phys_offset cannot be determined from the dumpfile.\n"); > @@ -1175,6 +1207,23 @@ arm64_init_kernel_pgd(void) > vt->kernel_pgd[i] = value; > } > > +ulong arm64_PTOV(ulong paddr) > +{ > + ulong v; > + struct machine_specific *ms = machdep->machspec; > + > + /* > + * Either older kernel before kernel has 'physvirt_offset' or newer kernel which > + * removes 'physvirt_offset' has the same formula > + */ > + if (!(machdep->flags & HAS_PHYSVIRT_OFFSET)) > + v = (paddr - ms->physvirt_offset) | PAGE_OFFSET; > + else > + v = paddr - ms->physvirt_offset; > + > + return v; > +} > + > ulong > arm64_VTOP(ulong addr) > { > @@ -1185,8 +1234,20 @@ arm64_VTOP(ulong addr) > return addr - machdep->machspec->kimage_voffset; > } > > - if (addr >= machdep->machspec->page_offset) > - return addr + machdep->machspec->physvirt_offset; > + if (addr >= machdep->machspec->page_offset) { > + ulong paddr; > + > + if (!(machdep->flags & FLIPPED_VM) || (machdep->flags & HAS_PHYSVIRT_OFFSET)) { > + paddr = addr; > + } else { > + /* > + * #define __lm_to_phys(addr) (((addr) & ~PAGE_OFFSET) + PHYS_OFFSET) > + */ > + paddr = addr & ~ _PAGE_OFFSET(machdep->machspec->CONFIG_ARM64_VA_BITS); > + } > + paddr += machdep->machspec->physvirt_offset; > + return paddr; Hmm, complex. It should be symmetric to PTOV, why differences?