At present, we have the following important changes for arm64 memory layout: -1. redesigned ARM64 kernel virtual memory layout and associated KASLR support that was introduced in Linux 4.6. And NEW_VMEMMAP is used to flag it. -2. memory layout flipped just right before introducing 52-bits kernel. -3. introducing of vabits_actual and phyvirt_offset in kernel -4. removing phyvirt_offset. These changes have effects on PTOV()/VTOP() formula. So introducing a dedicate field mmlayout_flags to record it. Among above, 2 and 3 are introduced closely, and are not distinguished in current implement. And this patch also keep this practice and use vabits_actual as a hint to flag mem flipped. Signed-off-by: Pingfan Liu <piliu(a)redhat.com&gt; Cc: HAGIO KAZUHITO <k-hagio-ab(a)nec.com&gt; Cc: Lianbo Jiang <lijiang(a)redhat.com&gt; Cc: Bhupesh Sharma <bhupesh.sharma(a)linaro.org&gt; To: crash-utility(a)redhat.com --- arm64.c | 11 +++++++++++ defs.h | 3 +++ 2 files changed, 14 insertions(+) diff --git a/arm64.c b/arm64.c index 8934961..98138b2 100644 --- a/arm64.c +++ b/arm64.c @@ -560,6 +560,10 @@ arm64_dump_machdep_table(ulong arg) fprintf(fp, "%sMACHDEP_BT_TEXT", others++ ? "|" : ""); if (machdep->flags & NEW_VMEMMAP) fprintf(fp, "%sNEW_VMEMMAP", others++ ? "|" : ""); + if (machdep->flags & FLIPPED_VM) + fprintf(fp, "%sFLIPPED_VM", others++ ? "|" : ""); + if (machdep->flags & HAS_PHYSVIRT_OFFSET) + fprintf(fp, "%sHAS_PHYSVIRT_OFFSET", others++ ? "|" : ""); fprintf(fp, ")\n"); fprintf(fp, " kvbase: %lx\n", machdep->kvbase); @@ -994,6 +998,7 @@ arm64_calc_physvirt_offset(void) if (READMEM(pc->mfd, &physvirt_offset, sizeof(physvirt_offset), sp->value, sp->value - machdep->machspec->kimage_voffset) > 0) { + machdep->flags |= HAS_PHYSVIRT_OFFSET; ms->physvirt_offset = physvirt_offset; } } @@ -3923,6 +3928,7 @@ arm64_calc_VA_BITS(void) if (kernel_symbol_exists("vabits_actual")) { if (pc->flags & PROC_KCORE) { vabits_actual = symbol_value_from_proc_kallsyms("vabits_actual"); + machdep->flags |= FLIPPED_VM; if ((vabits_actual != BADVAL) && (READMEM(pc->mfd, &value, sizeof(ulong), vabits_actual, KCORE_USE_VADDR) > 0)) { if (CRASHDEBUG(1)) @@ -3953,6 +3959,11 @@ arm64_calc_VA_BITS(void) machdep->machspec->VA_BITS_ACTUAL = value; machdep->machspec->VA_BITS = value; machdep->machspec->VA_START = _VA_START(machdep->machspec->VA_BITS_ACTUAL); + /* + * The mm flip commit is introduced before 52-bits VA, which is before the + * commit to export NUMBER(TCR_EL1_T1SZ) + */ + machdep->flags |= FLIPPED_VM; } else if (machdep->machspec->VA_BITS_ACTUAL) { machdep->machspec->VA_BITS = machdep->machspec->VA_BITS_ACTUAL; machdep->machspec->VA_START = _VA_START(machdep->machspec->VA_BITS_ACTUAL); diff --git a/defs.h b/defs.h index 396d61a..24d41e7 100644 --- a/defs.h +++ b/defs.h @@ -3208,6 +3208,8 @@ typedef signed int s32; #define NEW_VMEMMAP (0x80) #define VM_L4_4K (0x100) #define UNW_4_14 (0x200) +#define FLIPPED_VM (0x400) +#define HAS_PHYSVIRT_OFFSET (0x800) /* * Get kimage_voffset from /dev/crash @@ -3290,6 +3292,7 @@ struct arm64_pt_regs { struct machine_specific { ulong flags; ulong userspace_top; + ulong mmlayout_flags; ulong page_offset; ulong vmalloc_start_addr; ulong vmalloc_end; -- 2.29.2

Hi Lianbo, Thanks for your review. Cc Kazu, do you prefer to send out V9 immediately? Or wait for a days? Thanks, Pingfan On Sun, Jun 13, 2021 at 12:39 PM lijiang <lijiang(a)redhat.com&gt; wrote:

-----Original Message----- This "else if" is for "--machdep vabits_actual=" option for Linux 5.4 and later not having the NUMBER(TCR_EL1_T1SZ), those also should have flipped VM. So we can simply add the following at the beginning or the end of the "if vabits_actual exists" block like this? --- a/arm64.c +++ b/arm64.c @@ -3960,6 +3960,7 @@ arm64_calc_VA_BITS(void) error(FATAL, "cannot determine VA_BITS_ACTUAL\n"); } + machdep->flags |= FLIPPED_VM; return; } Thanks, Kazu

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(a)redhat.com&gt; Cc: HAGIO KAZUHITO <k-hagio-ab(a)nec.com&gt; Cc: Lianbo Jiang <lijiang(a)redhat.com&gt; Cc: Bhupesh Sharma <bhupesh.sharma(a)linaro.org&gt; To: crash-utility(a)redhat.com --- 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; + } 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; } @@ -1085,7 +1106,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->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; + } else if (machdep->machspec->kimage_voffset) return addr - machdep->machspec->kimage_voffset; else /* no randomness */ diff --git a/defs.h b/defs.h index 24d41e7..786186a 100644 --- a/defs.h +++ b/defs.h @@ -3086,11 +3086,6 @@ typedef u64 pte_t; #define _64BIT_ #define MACHINE_TYPE "ARM64" -#define PTOV(X) \ - ((unsigned long)(X) - (machdep->machspec->physvirt_offset)) - -#define VTOP(X) arm64_VTOP((ulong)(X)) - #define USERSPACE_TOP (machdep->machspec->userspace_top) #define PAGE_OFFSET (machdep->machspec->page_offset) #define VMALLOC_START (machdep->machspec->vmalloc_start_addr) @@ -3100,6 +3095,14 @@ typedef u64 pte_t; #define MODULES_VADDR (machdep->machspec->modules_vaddr) #define MODULES_END (machdep->machspec->modules_end) +/* + * The formula: + * #define __phys_to_virt(x) ((unsigned long)((x) - PHYS_OFFSET) | PAGE_OFFSET) + */ +#define PTOV(X) arm64_PTOV((ulong)(X)) + +#define VTOP(X) arm64_VTOP((ulong)(X)) + #define IS_VMALLOC_ADDR(X) arm64_IS_VMALLOC_ADDR((ulong)(X)) #define PAGEBASE(X) (((ulong)(X)) & (ulong)machdep->pagemask) @@ -3300,7 +3303,10 @@ struct machine_specific { ulong vmemmap_end; ulong modules_vaddr; ulong modules_end; + /* real physical offset */ ulong phys_offset; + /* read from kernel symbol memstart_addr */ + long phys_offset_nominal; ulong __exception_text_start; ulong __exception_text_end; struct arm64_pt_regs *panic_task_regs; @@ -5903,6 +5909,7 @@ void unwind_backtrace(struct bt_info *); void arm64_init(int); void arm64_dump_machdep_table(ulong); ulong arm64_VTOP(ulong); +ulong arm64_PTOV(ulong); int arm64_IS_VMALLOC_ADDR(ulong); ulong arm64_swp_type(ulong); ulong arm64_swp_offset(ulong); -- 2.29.2

On Mon, Jun 28, 2021 at 06:31:02AM +0000, HAGIO KAZUHITO(萩尾 一仁) wrote: Reasonable. Adopt. 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? 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 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

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

On Wed, Jun 30, 2021 at 05:42:27AM +0000, HAGIO KAZUHITO(萩尾 一仁) wrote: 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; Thanks for this good suggestion. I am composing a new patch based on it. 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

Nitpicking: kernels >= 5.4

On RHEL9, crash hits a bug when executing "crash /proc/kcore" : seek error: kernel virtual address: ffff6a0f3fff0000 type: "pmd page" The kernel virtual address does not vary with va_actual bits, instead, is determined by configuration value. But crash does not observe this fact. Since vabits_actual related kernel commit is introduced after arm64 mm layout flip commit, so changes are safe under the condition if (ms->VA_BITS_ACTUAL), and keep else branch untouched. Signed-off-by: Pingfan Liu <piliu(a)redhat.com&gt; Cc: HAGIO KAZUHITO <k-hagio-ab(a)nec.com&gt; Cc: Lianbo Jiang <lijiang(a)redhat.com&gt; Cc: Bhupesh Sharma <bhupesh.sharma(a)linaro.org&gt; To: crash-utility(a)redhat.com --- arm64.c | 7 ++++--- defs.h | 1 + 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/arm64.c b/arm64.c index 72958be..9a77628 100644 --- a/arm64.c +++ b/arm64.c @@ -224,9 +224,10 @@ arm64_init(int when) /* vabits_actual introduced after mm flip, so it should be flipped layout */ if (ms->VA_BITS_ACTUAL) { - ms->page_offset = ARM64_FLIP_PAGE_OFFSET_ACTUAL; - machdep->identity_map_base = ARM64_FLIP_PAGE_OFFSET_ACTUAL; - machdep->kvbase = ARM64_FLIP_PAGE_OFFSET_ACTUAL; + ms->page_offset = ARM64_FLIP_PAGE_OFFSET; + /* useless on arm64 */ + machdep->identity_map_base = ARM64_FLIP_PAGE_OFFSET; + machdep->kvbase = ARM64_FLIP_PAGE_OFFSET; ms->userspace_top = ARM64_USERSPACE_TOP_ACTUAL; } else { ms->page_offset = ARM64_PAGE_OFFSET; diff --git a/defs.h b/defs.h index bc3dead..1ed9c5d 100644 --- a/defs.h +++ b/defs.h @@ -3233,6 +3233,7 @@ typedef signed int s32; #define ARM64_PAGE_OFFSET ((0xffffffffffffffffUL) \ << (machdep->machspec->VA_BITS - 1)) /* kernels > v5.4 the kernel VA space is flipped */ +#define ARM64_FLIP_PAGE_OFFSET (-(1UL) << machdep->machspec->CONFIG_ARM64_VA_BITS) #define ARM64_FLIP_PAGE_OFFSET_ACTUAL ((0xffffffffffffffffUL) \ - ((1UL) << machdep->machspec->VA_BITS_ACTUAL) + 1) -- 2.29.2