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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jun 29, 2021 at 07:07:21AM +0000, HAGIO KAZUHITO(萩尾 一仁) wrote:
> -----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.
> 
OK, got the picture. I think you are right.

>         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

I will ditto.
>                                 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;

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 smy 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.


Thanks,

Pingfan

--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility




[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux