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

[Pingfan Liu]

On Wed, Jun 30, 2021 at 05:42:27AM +0000, HAGIO KAZUHITO(萩尾 一仁) wrote:
> -----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 ?

As you suggested below, just not using physvirt_offset if no such kernel symbol.
And here just keep its semantic, although it is useless in such cases
Either FLIPPED_VM or !FLIPPED_VM without HAS_PHYSVIRT_OFFSET:
          ms->physvirt_offset = ms->phys_offset_nominal - ms->page_offset;

> 
> 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;
> 

Thanks for this good suggestion. I am composing a new patch based on it.
> 
> 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.
> 
My wrong understanding of the code.

So I think there is only ms->phys_offset needed, and phys_offset_nominal can be abandoned.


Thanks,
Pingfan