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

-----Original Message----- > > > > > @@ -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; > > I had thought it worked for all versions before commit 5383cc6efed1 (arm64: mm: Introduce vabits_actual). > So !(machdep->flags & FLIPPED_VM) branch should use the formula. > > But it turns out to be wrong. PLS see the comment followed. > > > > > > + 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 my question why they're asymmetric emerged: > > > Thank you for the patient. > > > 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 ? > > > It works for !FLIPPED_VM. But I did make a mistake on VTOP() > > Flipped mm is introduced by 14c127c957c1 ("arm64: mm: Flip kernel VA space") > > $ git show 14c127c957c1c6070647c171e72f06e0db275ebf:arch/arm64/include/asm/memory.h | grep "#define > __phys_to_virt" > #define __phys_to_virt(x) ((unsigned long)((x) - PHYS_OFFSET) | PAGE_OFFSET) > $ git show 14c127c957c1c6070647c171e72f06e0db275ebf~1:arch/arm64/include/asm/memory.h | grep "#define > __phys_to_virt" > #define __phys_to_virt(x) ((unsigned long)((x) - PHYS_OFFSET) | PAGE_OFFSET) > > So the formula keeps unchange across filpped-mm. The same case for VTOP. > And what matters is physvirt_offset introduced by 5383cc6efed1 ("arm64: mm: Introduce vabits_actual") > > > 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 > > > > Similar, > $git show 14c127c957c1c6070647c171e72f06e0db275ebf:arch/arm64/include/asm/memory.h | grep __lm_to_phys > #define __lm_to_phys(addr) (((addr) & ~PAGE_OFFSET) + PHYS_OFFSET) > __is_lm_address(__x) ? __lm_to_phys(__x) : \ > $ git show 14c127c957c1c6070647c171e72f06e0db275ebf~1:arch/arm64/include/asm/memory.h | grep __lm_to_phys > #define __lm_to_phys(addr) (((addr) & ~PAGE_OFFSET) + PHYS_OFFSET) > __is_lm_address(__x) ? __lm_to_phys(__x) : \ > > > > > 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? > > > > So I need to drop the !(machdep->flags & FLIPPED_VM) in VTOP(), instead of adding in PTOV(). > Sorry for the confusion and hope I make it clear. If you do so, you mean that you will change ms->physvirt_offset = ms->phys_offset - ms->page_offset to ms->physvirt_offset = ms->phys_offset if !FLIPPED_VM ?

For me, it's readable and understandable to match the formulas with kernel's one. In this case, use the physvirt_offset only if physvirt_offset exists. We can do it now because we are introducing a switchable PTOV/VTOP. For example: ms->phys_offset_nominal = read phys_offset; if (ms->phys_offset_nominal < 0) ms->phys_offset = ms->phys_offset_nominal + MEMSTART_ADDR_OFFSET; else ms->phys_offset = ms->phys_offset_nominal PTOV: if (machdep->flags & HAS_PHYSVIRT_OFFSET) v = paddr - ms->physvirt_offset; else v = (paddr - ms->phys_offset_nominal) | PAGE_OFFSET VTOP: if (machdep->flags & HAS_PHYSVIRT_OFFSET) p = vaddr + ms->physvirt_offset; else p = (vaddr & ~PAGE_OFFSET) + ms->phys_offset_nominal;

btw, just to clarify, where is the phys_offset used except for PTOV and VTOP? I cannot find it.. > while getting PFN offset in a dumpfile, phys_offset is > required.