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 Mon, Jun 28, 2021 at 06:31:02AM +0000, HAGIO KAZUHITO(萩尾 一仁) wrote:
> -----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@xxxxxxxxxx>
> > Cc: HAGIO KAZUHITO <k-hagio-ab@xxxxxxx>
> > Cc: Lianbo Jiang <lijiang@xxxxxxxxxx>
> > Cc: Bhupesh Sharma <bhupesh.sharma@xxxxxxxxxx>
> > To: crash-utility@xxxxxxxxxx
> > ---
> >  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) {
> 	...
> 
Reasonable. Adopt.

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

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?
> 
> > @@ -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))) {
 		/*
 		 * 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?

Not sure that I catch your meaning. Replacing _PAGE_OFFSET(machdep->machspec->CONFIG_ARM64_VA_BITS) with PAGE_OFFSET ?
Something like this ? 

diff --git a/arm64.c b/arm64.c
index 9a77628..0917613 100644
--- a/arm64.c
+++ b/arm64.c
@@ -1241,15 +1241,14 @@ arm64_VTOP(ulong addr)
 			ulong paddr;
 
 			if (!(machdep->flags & FLIPPED_VM) || (machdep->flags & HAS_PHYSVIRT_OFFSET)) {
-				paddr = addr;
+				return addr + machdep->machspec->physvirt_offset;
 			} else {
 				/*
 				 * #define __lm_to_phys(addr)	(((addr) & ~PAGE_OFFSET) + PHYS_OFFSET)
 				 */
-				paddr = addr & ~ _PAGE_OFFSET(machdep->machspec->CONFIG_ARM64_VA_BITS);
+				paddr = addr & ~ PAGE_OFFSET;
+				return paddr + machdep->machspec->physvirt_offset;
 			}
-			paddr += machdep->machspec->physvirt_offset;
-			return paddr;
 		}
 		else if (machdep->machspec->kimage_voffset)
 			return addr - machdep->machspec->kimage_voffset;



Again, appreciate your review.


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