[Crash-utility] [PATCHv2] crash-utility/arm64: store phy_offset and memstart_addr separately

[piliu]

In case of making mistake due to my limited knowledge on arm64, I also 
CC this patch to some guys from arm team. If any comment, please also 
kindly give them.

Thanks,
Pingfan

On 3/25/21 11:00 AM, Pingfan Liu wrote:
> Crash encounters a bug like the following:
>      ...
>      License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
>      This is free software: you are free to change and redistribute it.
>      There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
>      and "show warranty" for details.
>      This GDB was configured as "aarch64-unknown-linux-gnu"...
> 
>      crash: read error: kernel virtual address: ffff000f789c0050  type: "IRQ stack pointer"
>      crash: read error: kernel virtual address: ffff000f78a60050  type: "IRQ stack pointer"
>      crash: read error: kernel virtual address: ffff000f78b00050  type: "IRQ stack pointer"
>      ...
> 
> 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 phy_offset.
> 
> In crash utility, PTOV() needs memstart_addr to calculate VA from PA,
> while getting PFN offset in a dumpfile, phy_offset is required. So
> storing them separately for different purpose.
> 
> Signed-off-by: Pingfan Liu <piliu at redhat.com>
> Cc: HAGIO KAZUHITO <k-hagio-ab at nec.com>
> Cc: Lianbo Jiang <lijiang at redhat.com>
> Cc: Mark Salter <msalter at redhat.com>
> Cc: Mark Langsdorf <mlangsdo at redhat.com>
> Cc: Jeremy Linton <jlinton at redhat.com>
> To: crash-utility at redhat.com
> ---
>   arm64.c | 25 ++++++++++++++++++++++---
>   defs.h  |  1 +
>   2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/arm64.c b/arm64.c
> index 37aed07..a0bee62 100644
> --- a/arm64.c
> +++ b/arm64.c
> @@ -24,6 +24,9 @@
>   
>   #define NOT_IMPLEMENTED(X) error((X), "%s: function not implemented\n", __func__)
>   
> +#define MEMSTART_ADDR_OFFSET \
> +	(0xffffffffffffffff << 48 - 0xffffffffffffffff << 56)
> +
>   static struct machine_specific arm64_machine_specific = { 0 };
>   static int arm64_verify_symbol(const char *, ulong, char);
>   static void arm64_parse_cmdline_args(void);
> @@ -687,6 +690,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, "         memstart_addr: %lx\n", ms->memstart_addr);
>   	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);
> @@ -987,7 +991,7 @@ arm64_calc_physvirt_offset(void)
>   	ulong physvirt_offset;
>   	struct syment *sp;
>   
> -	ms->physvirt_offset = ms->phys_offset - ms->page_offset;
> +	ms->physvirt_offset = ms->memstart_addr - ms->page_offset;
>   
>   	if ((sp = kernel_symbol_search("physvirt_offset")) &&
>   			machdep->machspec->kimage_voffset) {
> @@ -1028,7 +1032,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->memstart_addr = htol(string, QUIET, NULL);
> +					if (ms->memstart_addr < 0)
> +						ms->phys_offset = ms->memstart_addr + MEMSTART_ADDR_OFFSET;
> +					else
> +						ms->phys_offset = ms->memstart_addr;
>   					free(string);
>   					return;
>   				}
> @@ -1080,7 +1088,18 @@ arm64_calc_phys_offset(void)
>   	} else if (DISKDUMP_DUMPFILE() && diskdump_phys_base(&phys_offset)) {
>   		ms->phys_offset = phys_offset;
>   	} 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->memstart_addr = 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");
> diff --git a/defs.h b/defs.h
> index 35b983a..64f2bcb 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -3290,6 +3290,7 @@ struct machine_specific {
>   	ulong modules_vaddr;
>   	ulong modules_end;
>   	ulong phys_offset;
> +	long memstart_addr;
>   	ulong __exception_text_start;
>   	ulong __exception_text_end;
>   	struct arm64_pt_regs *panic_task_regs;
>