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

-----Original Message----- > > > @@ -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? > > I have no big picture about /dev/mem. > But for the value, memstart_addr's calculation is done by > arch/arm64/mm/init.c:349: memstart_addr -= _PAGE_OFFSET(48) - _PAGE_OFFSET(52); > in arm64_memblock_init(). > > Does it raise issue for /dev/mem ? Could you enlighten me about your idea? Recently /proc/kcore has had ELF note vmcoreinfo, so crash can read it, but in case of /dev/mem, it cannot be used and crash will read the memstart_addr value with the following READMEM(). In this case, I thought that the same offset adjustment would be needed in theory.

if (ACTIVE()) { ... if ((machdep->flags & NEW_VMEMMAP) && 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_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; } vaddr = symbol_value_from_proc_kallsyms("memstart_addr"); if (vaddr == BADVAL) vaddr = sp->value; paddr = KCORE_USE_VADDR; } else { vaddr = sp->value; paddr = sp->value - machdep->machspec->kimage_voffset; } if (READMEM(pc->mfd, &phys_offset, sizeof(phys_offset), vaddr, paddr) > 0) { ms->phys_offset = phys_offset; <<-- read from memstart_addr

return; } > > > > > @@ -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) ? > > I miss this diskdump. It should also have the same logic. > > diff --git a/arm64.c b/arm64.c > index 6346753..cd84a74 100644 > --- a/arm64.c > +++ b/arm64.c > @@ -1108,9 +1108,8 @@ arm64_calc_phys_offset(void) > return; > > ms->phys_offset = phys_offset; > - } else if (DISKDUMP_DUMPFILE() && diskdump_phys_base(&phys_offset)) { > - ms->phys_offset = phys_offset; > - } else if (KDUMP_DUMPFILE() && arm64_kdump_phys_base(&phys_offset)) { > + } else if ((DISKDUMP_DUMPFILE() && diskdump_phys_base(&phys_offset)) || > + (KDUMP_DUMPFILE() && arm64_kdump_phys_base(&phys_offset))) { Oh, this looks nicely done. > /* > * When running a 52bits kernel on 48bits hardware. Kernel plays a trick: > * if (IS_ENABLED(CONFIG_ARM64_VA_BITS_52) && (vabits_actual != 52)) > -- > 2.29.2 > > > > > > } 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? >

Yes, it's one of them, and this looks better, but... Hmm ok, I summarized these and smy question why they're asymmetric emerged:

if physvirt_offset exists // HAS_PHYSVIRT_OFFSET ms->physvirt_offset = read physvirt_offset; else if (machdep->flags & FLIPPED_VM) ms->physvirt_offset = ms->phys_offset_nominal; else // !FLIPPED_VM ms->physvirt_offset = ms->phys_offset - ms->page_offset; PTOV: if (machdep->flags & HAS_PHYSVIRT_OFFSET) v = paddr - ms->physvirt_offset; // looks ok else v = (paddr - ms->physvirt_offset) | PAGE_OFFSET; // Is this ok when !FLIPPED_VM ?

VTOP: if (machdep->flags & HAS_PHYSVIRT_OFFSET || !(machdep->flags & FLIPPED_VM)) p = vaddr + ms->physvirt_offset; // looks ok else p = (vaddr & ~PAGE_OFFSET) + ms->physvirt_offset; // looks ok

When !FLIPPED_VM, PTOV calculates: v = (paddr - ms->physvirt_offset) | PAGE_OFFSET = (paddr - ms->physoffset + ms->page_offset) | PAGE_OFFSET This might be not wrong in the result value because of the or operation, but looks wrong formula. So PTOV also needs the !(machdep->flags & FLIPPED_VM) condition? or I'm missing something?